feat(sdk): harden daemon session client#4225
Merged
Merged
Conversation
Contributor
📋 Review SummaryThis PR hardens the 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
reviewed
May 17, 2026
wenshao
requested changes
May 17, 2026
wenshao
approved these changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Three additional notes beyond the eager-guard and subscribeEvents duplication already flagged: cursor semantics, type narrowing, and a test that pins the eager-guard behavior in place. — claude-opus-4-7 via Claude Code /review
wenshao
requested changes
May 17, 2026
156e4b2 to
e0149f5
Compare
e0149f5 to
225661a
Compare
Collaborator
Author
|
Rebased onto latest
I treated the earlier Validation after the update: cd packages/sdk-typescript && npx vitest run test/unit/DaemonSessionClient.test.ts test/unit/DaemonClient.test.ts test/unit/daemon-sse.test.ts
cd packages/sdk-typescript && npm run typecheck
cd packages/sdk-typescript && npm run build
cd packages/sdk-typescript && npx prettier --check src/daemon/DaemonSessionClient.ts test/unit/DaemonSessionClient.test.ts
npx eslint packages/sdk-typescript/src/daemon/DaemonSessionClient.ts packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts --max-warnings 0 --no-warn-ignored
npm run buildTargeted SDK tests passed: 3 files, 82 tests. Root build passed with the existing |
wenshao
requested changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: REQUEST CHANGES (1 critical issue)
Overall: This PR introduces comprehensive guardrails to DaemonSessionClient — lazy subscription guard acquisition, cursor monotonicity via Math.max, abort-signal propagation, SSE event.id validation, and extensive test coverage (141→205 lines covering all guard paths, abort, throw, and idempotent cleanup). Build, typecheck, and all 64 unit tests pass.
Key improvements over the base branch:
- Lazy guard acquisition (guard acquired on first
next(), notevents()call) prevents eager lock-out Math.maxcursor watermark prevents replay regression- Abort signal propagation cleanly cancels in-flight SSE subscriptions
validateLastEventIdenforces integer constraints at all entry points (constructor,setLastEventId,events()opts)- Test suite covers guard lifecycle, abort, throw, idempotent cleanup, and state propagation
Critical issue (see inline comment): Abandoned generators permanently lock the subscription guard — if a consumer calls events(), iterates once, then drops the generator without calling return(), the guard is never released and every subsequent subscription throws. This only affects manual .next() consumers (not for await...of), but the public API exposes next() directly.
Suggestion: Include the actual invalid value in validateLastEventId TypeError message for easier debugging.
| this.subscriptionActive = true; | ||
| return { | ||
| next: async (value?: unknown) => { | ||
| if (!released) { |
Collaborator
There was a problem hiding this comment.
[Critical] Abandoned generators permanently lock the subscription guard.
If a consumer calls events(), iterates once (acquiring the guard via acquire() here), then drops the generator without calling return(), subscriptionActive stays true forever. The finally block in iterateEvents only fires when the generator completes or is explicitly closed — not when it is simply garbage-collected. Every subsequent events().next() throws permanently.
The existing test correctly calls await abandoned.return(undefined) to demonstrate cleanup, but does not cover the no-return abandonment scenario.
In practice, for await...of calls .return() automatically, so the risk is limited to manual .next() consumers. But since the public API exposes next() directly, this is a real concern.
Possible mitigations:
- Add a
FinalizationRegistry-based cleanup (with a fallback timeout) that releases the guard if the wrapper is GC'd without being closed. - Or, add a
setTimeoutsafety net inacquire()that auto-releases after N seconds of inactivity. - Or, document that callers MUST use
for await...ofor explicitly call.return()when done.
| throw new TypeError('lastEventId must be a finite non-negative integer'); | ||
| } | ||
| return lastEventId; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] Include the actual invalid value in the error message for easier debugging:
Suggested change
| } | |
| throw new TypeError(`lastEventId must be a finite non-negative integer, got ${lastEventId}`); |
wenshao
approved these changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Local tmux verification on 225661a (rebased onto 2453b82, which now contains #4222 load/resume and #4217 events schema):
npx vitest runon DaemonSessionClient/DaemonClient/daemon-sse: 82/82 passing (16 + 48 + 18).npm run typecheck,npm run build(SDK),npx prettier --checkon the two touched files,npx eslint --max-warnings 0 --no-warn-ignoredon the two touched files, full reponpm run build: all clean (only the pre-existing vscode-companion curly lint warning, as noted in the PR description).- GitHub CI: Lint / CodeQL / Test (mac+ubuntu+win) / coverage — all green.
All concerns from the prior rounds (verified via git diff 156e4b2..225661a2 on the two files):
- [Critical] Eager-guard leak on abandoned generators —
openEventSubscriptionnow lazily acquires the guard inside the wrapper iterator'snext()via anacquire()helper; anevents()call that is never iterated does not block subsequent subscriptions. - [Critical] Missing
throw()test — newreleases the subscription guard when consumers throw into the iteratorpins thefinally-release path on the wrapper'sthrow()handler; a follow-upsession.events()succeeds without the concurrent-subscription error. - [Issue] Old eager-guard test pinning — rewritten to
await first.next()before asserting'Another event subscription is already active'on the second subscription, plus a newdoes not acquire the subscription guard until iteration startsthat pins the lazy behavior. - [Suggestion]
Math.maxcursor semantics — comment added initerateEventsclarifying the cursor is a monotonic replay watermark, so it does not regress on replayed or synthetic frames with older ids. - [Suggestion]
subscribeEvents()duplication — annotated@deprecatedto steer callers towardevents(). - [Nit]
AsyncGenerator<DaemonEvent, void, unknown>narrowing — called out under "Breaking changes" in the PR description; blast radius limited to this package.
setLastEventId, constructor seed, and per-call lastEventId reject NaN, Infinity, negatives, and non-integers via validateLastEventId, covered by the explicit TypeError assertions in the test.
LGTM.
This was referenced May 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DaemonSessionClientreplay cursor handling and event subscription lifecycle, while keeping the existingevents()andsubscribeEvents()entry points available.Last-Event-IDvalidation, and the additional tests around abort/error/replay behavior.Validation
packages/vscode-ide-companion/src/utils/editorGroupUtils.tscurly warning, which this PR does not touch.Scope / Risk
events()/subscribeEvents()now share a single guarded implementation. The guard is acquired lazily on the first.next(), so an iterator created and then abandoned does not block later subscriptions; once active, a second active stream is rejected until the first stream completes, returns, throws, or aborts.subscribeEvents()is now documented as deprecated in favor ofevents().AsyncGeneratorreturn typing is explicit asvoid, andsetLastEventId, constructor seed, and per-calllastEventIdnow reject invalid cursor values instead of sending malformedLast-Event-IDheaders.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Related to #3803 and follow-up hardening after #4201.