Fix: Prevent memory leak from accumulating HRM event listeners

[JaapvanEkris]

This fixes issue #258 where the application crashes with a heap overflow after several hours of being stationary (no flywheel spinning).

The root cause was that every time createHrmPeripheral was called, a new 'heartRateMeasurement' event listener was registered without removing the old one. This caused event listener accumulation over time, particularly during idle periods when the HRM watchdog timer kept resetting.

The fix explicitly removes all 'heartRateMeasurement' listeners before registering a new one, preventing the accumulation of duplicate listeners that leads to the heap overflow.

Fixes: #258

[Abasz]

I have to go a bit deep into this to verify if this is a solution for a memory leak. Reason is that when createHrmPeripheral() runs we remove all event listeners (assuming hrmPeripheral exists) ->

openrowingmonitor/app/peripherals/PeripheralManager.js

Line 344 in 502d077

hrmPeripheral?.removeAllListeners()

So at a first glance this seems redundant to me.

If we try to create a new HRM and there has already been one there is code for proper cleaning up at the top of that function.

EDIT:

I do not see how this can be related to the watchdog, because that does not actually kill the connection, it simply resets the data to zero/undefined. This is a one shot timer.

openrowingmonitor/app/peripherals/PeripheralManager.js

Line 419 in 502d077

function onHRMWatchdogTimeout () {

This means that the timer completes after 6 second and that is it. Never runs again.

[JaapvanEkris]

This means that the timer completes after 6 second and that is it. Never runs again.

I'm sceptical as well (otherwise I just pushed it), but I couldn't exclude it either.

Copilot's analysis:

I found the memory leak. The problem is in PeripheralManager.js on lines 392–408:

Every time createHrmPeripheral is called (which happens when switching modes or restarting), a new event listener is attached to the hrmPeripheral object without removing the old one. Since hrmPeripheral?.removeAllListeners() is called on line 344, but after the old peripheral is destroyed and before a new one is created, the listeners from the previous session are never cleaned up.

The leak pattern:
1. `createHrmPeripheral(newMode)` is called
2. Line 342-345: Old peripheral is destroyed and listeners removed
3. Lines 355-390: A new peripheral is created
4. Lines 392-408: A new listener is registered AGAIN — but this is done inside the if (hrmPeripheral && ...) block
5. The function exits without unregistering this listener when the session ends or during idle time

During idle periods when no new HRM data arrives, the hrmWatchdogTimer at line 407 keeps getting reset and recreated. When the system is stationary (no rotation impulses), the watchdog timer callback onHRMWatchdogTimeout keeps re-emitting events, accumulating listener references.

The fix: Move the listener registration outside the createHrmPeripheral function, or ensure listeners are unregistered when peripherals are destroyed. Alternatively, use once() instead of on() for the heartrate measurement listener, or explicitly clean up listeners in the createHrmPeripheral function before registering new ones.

I asked Claude Opus 5 the same question, he did not find this pattern. He suspected buffers filling up: in MQTT, but also probably in BLE we keep publishing data. In BLE we even do this time driven. But if nobody actually listens, it might get queued, slowly filling up the buffer.

In MQTT I haven't considered it, so that might be a thing. But MQTT is completely event-driven, so if it survives the session, it shouldn't crash the app when idle. Buffer handling is pretty deep in BLE, so I couldn't find it directly to assess if that might cause this issue.

[Abasz]

Every time createHrmPeripheral is called (which happens when switching modes or restarting), a new event listener is attached to the hrmPeripheral object without removing the old one. Since hrmPeripheral?.removeAllListeners() is called on line 344, but after the old peripheral is destroyed and before a new one is created, the listeners from the previous session are never cleaned up.

This is factually not true.

If hrmperipheral exists we destroy it as first thing in the pipeline. Which means we cannot attach a new eventlistener to an already existing hrmPeripheral because it cannot exist at the time we call the on.

It is clearly mixing up the two methods: createBleHrmPeripheral vs createHrmPeripheral

He suspected buffers filling up: in MQTT, but also probably in BLE we keep publishing data. In BLE we even do this time driven. But if nobody actually listens, it might get queued, slowly filling up the buffer.

That I cannot exclude, this is why I asked in the issue to turn full logging on for the Peripheral so we can see what is going on when idle. But I dont see how this is possible.

I asked Claude Opus 5 the same question, he did not find this pattern.

Fable 5 may be? :D they are saying its the golden boy, the chosen one :D

[Abasz]

I did some AI assisted digging. I added my evaluation on whether it could be the issue.

Here is a full summary of every issue found and fixed:

---

Summary of identified bugs and fixes

A — HrmService.stop() missing scanner.removeAllListeners() (HrmService.js)

stop() called this.#scanner?.stopScan() but never called this.#scanner?.removeAllListeners(). If stop() was called while start() was blocked awaiting a BLE report advertisement event, the callback closure (and its captured Promise resolver) remained registered on the scanner. If ble-host keeps an internal reference to stopped scanners, the entire async start() execution context — including this (the HrmService instance) — could never be garbage-collected.

Fix: Added this.#scanner?.removeAllListeners() in stop() before stopScan(). Also added this.#scanReject?.(new Error('HrmService stopped')) and stored the scan Promise's reject in this.#scanReject so the pending await is cancelled immediately instead of hanging forever.

Verdict: While this is possible GC show stopper realistically this cannot grow and overrun the heap:

1. it is an unlikely event
2. the this cannot actually grow that large

---

B — No cancellation flag: zombie start() calls after destroy() (HrmService.js)

stop() resolves immediately when this.#connection is undefined (during the scan or connect phase). A pending manager.connect() callback could fire after stop() returned, set this.#connection, register a new once('disconnect', () => this.start()) handler, and complete service discovery — turning the destroyed service into a zombie that keeps scanning indefinitely.

Fix: Added #stopped = false field. stop() sets it to true first. start() checks the flag at entry and after every await that bridges an async phase boundary. The disconnect handler now also checks !this.#stopped before calling this.start().

Verdict: if this was the issue this could in my view create the memory leak with the indefinite scanning, but the event is unlikely

---

C — Error paths in HrmService.start() leave orphaned disconnect handlers (HrmService.js)

When the heart-rate service or measurement characteristic was not found on the connected device, the code called this.start() directly. At that point the established connection still had a once('disconnect', () => this.start()) handler registered. When the connection eventually timed out, this.start() fired a second time — creating two concurrent start() instances that could double on each subsequent cycle.

Fix: In both error paths, replaced this.start() with this.#connection?.disconnect(). Because once('disconnect', ...) is already registered, disconnecting cleanly triggers exactly one start() call via the existing mechanism.

Verdict: While if this event happens a lot due to the multiplication factor this could cause issues. However, the hrm does not connect to something that does not have these (BLE protocol prescribes the need for HR service and Measurement as mandatory so).

---

D — CpsPeripheral uses .on() instead of .once() for the disconnect handler (CpsPeripheral.js)

Unlike FtmsPeripheral and CscPeripheral, CpsPeripheral registered the reconnect callback with .on('disconnect', ...). If ble-host ever emits disconnect more than once for a connection, triggerAdvertising() would be called multiple times, spawning duplicate advertising loops.

Fix: Changed .on('disconnect', ...) to .once('disconnect', ...), consistent with all other BLE peripherals.

Verdict: this is a genuine bug/inconsistency but this is an unlikely cause.

---

E — Pm5RowingService broadcast timer not cleared when Pm5Peripheral.destroy() is called (Pm5Peripheral.js + Pm5RowingService.js)

Pm5Peripheral.destroy() removed GATT services and disconnected the client but never stopped Pm5RowingService's #timer. The timer's arrow-function callback captures this (the Pm5RowingService instance), keeping the entire service object (and everything it references) alive via the Node.js timer heap after blePeripheral was set to undefined. This is a fixed-size leak per BLE-mode switch involving PM5.

Fix: Added a stop() method to Pm5RowingService that calls clearTimeout(this.#timer). Pm5Peripheral.destroy() now calls rowingService.stop() as its first action.

Verdict: this is indeed a leak but I doubt this could cause the heap issue. This is fixed size and does not grow the required heap.

I will implement these changes and push them to this PR, as these are improvement but as stated above, they are not necessarily the solution to the issues.