Skip to content

fix(bus): guard indexOf in InMemoryBus unsubscribe to prevent splice(-1)#605

Open
serhiizghama wants to merge 2 commits into
rowboatlabs:mainfrom
serhiizghama:fix/491-bus-unsubscribe-splice-guard
Open

fix(bus): guard indexOf in InMemoryBus unsubscribe to prevent splice(-1)#605
serhiizghama wants to merge 2 commits into
rowboatlabs:mainfrom
serhiizghama:fix/491-bus-unsubscribe-splice-guard

Conversation

@serhiizghama

Copy link
Copy Markdown

Problem

Closes #491.

The desktop app's InMemoryBus (apps/x/packages/core/src/application/lib/bus.ts) builds its unsubscribe closure as:

this.subscribers.get(runId)!.splice(this.subscribers.get(runId)!.indexOf(handler), 1);

indexOf is not checked for -1. If unsubscribe() is called twice (or after the handler is already gone), indexOf returns -1 and splice(-1, 1) silently removes the last handler in the array — a different, still-active subscriber.

Reproduction:

  1. Subscribe two handlers a and b to the same runId.
  2. Call a's unsubscribe.
  3. Call a's unsubscribe again.
  4. b is now gone, so it no longer receives published events.

Solution

Guard both the missing-runId entry and the -1 index so a repeated unsubscribe is a true no-op:

const handlers = this.subscribers.get(runId);
if (!handlers) return;
const idx = handlers.indexOf(handler);
if (idx >= 0) handlers.splice(idx, 1);

This matches the pattern already used by the other buses in the same package (background-tasks/bus.ts, knowledge/live-note/bus.ts).

Note: the CLI bus has an identical bug tracked separately in #492 — this PR only touches the desktop app bus for #491.

Testing

Added bus.test.ts covering event delivery, single unsubscribe, and the double-unsubscribe no-op regression.

$ npx vitest run src/application/lib/bus.test.ts
 Test Files  1 passed (1)
      Tests  3 passed (3)

npm run build (tsc) passes.

The unsubscribe closure called splice(indexOf(handler), 1) without
checking for -1. A double-unsubscribe (or unsubscribing after the
runId entry was cleared) made indexOf return -1, so splice(-1, 1)
silently removed the last remaining handler instead of doing nothing.

Guard against a missing runId entry and a -1 index so a repeated
unsubscribe is a no-op, matching the pattern already used by the
other buses in the package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bus.unsubscribe removes wrong handler when called twice

1 participant