Skip to content

fix: resolve abort listener leak, VAD sparse arrays, and lint issues#41

Closed
rsagacom wants to merge 1 commit into
stepfun-ai:mainfrom
rsagacom:fix/abort-listener-leak-vad-lint
Closed

fix: resolve abort listener leak, VAD sparse arrays, and lint issues#41
rsagacom wants to merge 1 commit into
stepfun-ai:mainfrom
rsagacom:fix/abort-listener-leak-vad-lint

Conversation

@rsagacom

@rsagacom rsagacom commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes three categories of issues found during code review:

Bug Fixes

1. Abort listener leak in sleep/delay (agent-loop.ts, code-mode/service.ts)

The sleep() and delay() helper functions were calling signal.removeEventListener('abort', abort) in the timer success callback. This prematurely removes the abort listener from shared AbortSignals, causing other operations waiting on the same signal to miss abort notifications.

Root cause: When a timer completes normally, it should not clean up the abort listener — the { once: true } option ensures the listener auto-removes after firing. Removing it on success breaks signal sharing patterns (e.g. combined abort signals in session management).

2. VAD edit distance sparse array initialization (vad/resolver.ts, vad/cli-handlers.ts)

new Array(n) creates sparse arrays with empty slots (not undefined values). While the DP loop fills all elements before access, this triggers unicorn/no-new-array lint warnings and is semantically incorrect for the edit distance algorithm.

Fix: Changed to Array.from({ length: n }, (_, j) => j) which correctly initializes all elements.

Code Quality

3. Unused catch parameter (stepfun-stateless.ts)

Removed unused e parameter from catch block in onMessage().

4. Prettier formatting (docs/skills/pr-review-decision.md)

Fixed formatting inconsistencies that were causing pnpm check to fail.

Manual Test Plan

  • pnpm check — all checks pass (1152 tests, 0 lint errors, format clean)
  • pnpm test -- packages/realtime/src/vad/resolver.test.ts — edit distance still returns correct results
  • pnpm test -- packages/realtime/src/vad/cli-handlers.test.ts — VAD CLI handlers still work
  • pnpm test -- packages/core/src/agent/agent-loop.test.ts — agent loop retry/sleep behavior intact
  • pnpm test -- packages/core/src/tools/code-mode/service.ts — code mode delay/abort behavior intact

Verification

  • pnpm check passes (tests: 1152 passed, lint: 0 errors, dep-guard: clean, deadcode: clean, format: clean)
  • Only 3 pre-existing lint warnings remain (unrelated to this fix)

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

- agent-loop.ts / code-mode/service.ts: remove spurious removeEventListener
  calls from timer success callbacks; the once-true listener should only
  be removed in the abort handler, otherwise shared AbortSignals lose
  listeners prematurely
- vad: replace new Array(n) with Array.from to eliminate sparse arrays
  and resolve unicorn/no-new-array lint warnings
- stepfun-stateless.ts: drop unused catch parameter 'e'

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ZouR-Ma

ZouR-Ma commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the contribution!

To get this ready to merge:

  • Reference an issue in the description with Closes #<number> (our link-check CI requires it; open one first if needed).
  • Make sure pnpm check passes locally (lint, type-check, dependency/dead-code guards, formatting, and tests — also enforced in CI).

Full conventions:
https://github.com/stepfun-ai/Step-Realtime-CLI/blob/main/CONTRIBUTING.md

@li-xiu-qi

Copy link
Copy Markdown
Contributor

Hi @Misaka477, thanks for the fixes. I noticed #68 appears to be a near-duplicate of this PR — both cover the same abort listener leak in agent-loop.ts/code-mode/service.ts, the VAD sparse array fix in cli-handlers.ts/resolver.ts, and the unused catch parameter in stepfun-stateless.ts. Could you confirm if #68 should be closed in favor of this one, or if there are meaningful differences I'm missing?

@li-xiu-qi

Copy link
Copy Markdown
Contributor

Hi @rsagacom, thanks for this PR. The repo's CONTRIBUTING.md requires every PR to reference an issue (CI link-check enforces this). This PR currently doesn't close or reference any issue. Given the overlap with #68, I'd suggest either: (a) close this PR in favor of #68 and let that one carry the fix, or (b) rebase onto #68 and add a Closes reference to #80 if you believe this version has meaningful differences.

@ZouR-Ma

ZouR-Ma commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this PR — the abort-listener diagnosis is correct: manually removing the listener from a shared signal when the timer completes normally breaks other waiters on the same signal, and relying on { once: true } for cleanup is the right call. We want this fix, and you've already carried it into the cleaner #68 (with an issue reference added) — so we're closing this PR and consolidating on #68:

  1. The VAD / stateless hunks here were superseded by #10, merged on July 10 — which is also why this PR now shows conflicts;
  2. The docs/skills/pr-review-decision.md formatting churn is unrelated to this PR's topic. The identical change also sits in #47, where we've asked for it to be dropped as well — docs formatting will get its own chore PR, so neither PR needs to carry it.

One note for #68: it needs a rebase onto the latest main as well — its VAD/stateless hunks were also superseded by #10, so drop them on rebase and keep only the two deletions in agent-loop.ts and code-mode/service.ts. Once slimmed down, we'll prioritize its review.

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.

3 participants