fix(sdk): self-heal stale clientId on invalid_client_id prompts#5797
Merged
Conversation
After a daemon restart or session reload the daemon's in-memory client registration is wiped, so a prompt sent with our now-unknown clientId is rejected at admission with 400 invalid_client_id (PR #5784). That rejection happens before the turn registers, so the prompt never ran and retrying cannot double-execute. DaemonSessionClient.prompt() now wraps the admission call (both the blocking and non-blocking paths) in a self-heal: on invalid_client_id it re-registers via resumeSession to obtain a fresh clientId and retries the admission exactly once. A single-flight guard coalesces concurrent prompts so re-registration happens once; any other error (and a second invalid_client_id) propagates. Adds 6 tests covering both paths, the retry bound, the non-matching-error guard, reattach-failure propagation, and concurrent single-flight. Design: docs/superpowers/specs/2026-06-24-daemon-clientid-self-heal-design.md 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…elf-heal Adds an integration case to qwen-serve-routes.test.ts validating the three real-daemon behaviors DaemonSessionClient's clientId self-heal depends on: (1) an unregistered prompt clientId is rejected at admission with 400 invalid_client_id, (2) resume re-registers and mints a fresh clientId, and (3) retrying admission with that clientId is accepted (202). Model-free: prompt admission runs before any model call, so promptNonBlocking returns 202 on acceptance without reaching the model. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
|
Thanks for the PR, @doudouOUC! Template note: same as last review — the body is missing three required headings from the PR template ( On direction: clean follow-up to #5784 that closes the reliability loop. A daemon restart wiping in-memory client registrations and leaving the UI stuck is a real production pain point, and self-healing at the SDK layer is the right place to fix it. Clearly aligned. On approach: scope is tight and well-justified. Only Moving on to code review. 🔍 中文说明感谢贡献，@doudouOUC！ 模板提醒：与上次审查相同——PR 正文缺少模板中的三个必要标题（ 方向：#5784 的合理后续，闭合了可靠性闭环。daemon 重启清除内存中的客户端注册导致 UI 卡住是真实的生产痛点，在 SDK 层自愈是正确的位置。方向明确对齐。 方案：范围紧凑且有充分理由。仅 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal (before reading the diff): detect Comparison: the PR's approach matches this proposal exactly. The Reuse check: Correctness: the No correctness bugs, security holes, or AGENTS.md violations found. Test ResultsAll 34 DaemonSessionClient tests pass (26 existing + 8 new — up from 33/33 in the prior review). Covers blocking/non-blocking self-heal, retry-bounded, non-matching error passthrough, different 400 code passthrough, reattach failure propagation with guard clearing, and concurrent single-flight. Build — ✅ PASSESThe previous blocker is resolved. The budget was bumped from 125 KB to 127 KB in commit Full project build ( Real-Scenario TestingThis is an SDK-level change that requires a specific daemon state (stale clientId after restart) to reproduce interactively — not feasible to drive via a simple CLI invocation. The 8 unit tests using the 中文说明代码审查独立方案（阅读 diff 前）：在 prompt admission 调用上检测 对比： PR 的方案与独立方案完全一致。 复用检查： 正确性： 未发现正确性 bug、安全漏洞或 AGENTS.md 违规。 测试结果全部 34 个 DaemonSessionClient 测试通过（26 个已有 + 8 个新增——较上次审查的 33 个增加 1 个）。覆盖阻塞/非阻塞自愈、重试有界、非匹配错误透传、不同 400 错误码透传、reattach 失败传播与守卫清除、并发 single-flight。 构建 — ✅ 通过之前的阻塞项已解决。预算在 commit 真实场景测试这是 SDK 层面的变更，需要特定的 daemon 状态（重启后的过期 clientId）才能交互式复现——无法通过简单的 CLI 调用驱动。8 个使用 — Qwen Code · qwen3.7-max |
Collaborator
|
Stepping back: this is a well-reasoned fix for a real production incident. The design is clean — detect the specific admission-time rejection, re-register once, retry once, no loop. The invariants are correctly identified and documented. The tests are thorough (34/34 pass, 8 new cases covering all critical paths including the guard-clearing edge case) and the build now passes with the bumped budget. My independent proposal matched the PR's approach exactly, which is a good sign — there isn't a materially simpler path being missed. The previous blocker (browser bundle 1228 bytes over the 125 KB budget) is resolved: the author bumped the budget to 127 KB with justification, and the SDK build completes cleanly. Every other aspect was already in good shape. This is ready to ship. ✅ 中文说明退一步看：这是一个针对真实生产事故的合理修复。设计简洁——检测特定的 admission 时拒绝，重新注册一次，重试一次，无循环。不变量被正确识别并文档化。测试全面（34/34 通过，8 个新用例覆盖所有关键路径，包括守卫清除边界情况），构建在提高预算后通过。 我的独立方案与 PR 的方案完全一致，这是个好信号——没有被遗漏的更简路径。 之前的阻塞项（浏览器打包超出 125 KB 预算 1228 bytes）已解决：作者将预算提高到 127 KB 并附有理由，SDK 构建顺利通过。其他方面均已就绪。 可以合并。 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
requested changes
Jun 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Build is broken — browser daemon SDK bundle is 1228 bytes over the 128 KB budget (npm run build fails). Code and tests look great; once the budget is bumped or the bundle trimmed, this is ready. See my notes above. 🙏
Co-authored-by: Qwen-Coder <[email protected]>
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
requested changes
Jun 24, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| * Re-register this client against the (already-restored) session to obtain a | ||
| * fresh daemon-assigned `clientId`. Concurrent callers coalesce onto a single | ||
| * in-flight `resume` so we never orphan extra registrations. | ||
| */ |
Collaborator
There was a problem hiding this comment.
[Suggestion] The entire self-heal path is completely silent — no logging, no metrics, no callbacks when withClientIdSelfHeal triggers, when reattach() succeeds or fails, or when the retry is exhausted. When this fires in production at 3 AM, the oncall engineer has no signal that self-heal was attempted.
The design doc references a specific production incident (trace a76a31fe). The next time this fires, the situation will be indistinguishable from the pre-fix silent hang unless self-heal is observable.
At minimum, add a structured log (or an optional onSelfHeal?: (event: SelfHealEvent) => void callback to DaemonSessionClientOptions) at three points:
invalid_client_iddetected (old clientId, sessionId)reattachresult (new clientId or error)- Retry result (success or final failure)
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <[email protected]>
wenshao
reviewed
Jun 24, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new issues beyond the points already raised on the earlier revision (test placement, the reattaching-cleared assertion gap, isInvalidClientId branch coverage, and observability).
Independent verification of the current HEAD (abf0487):
- ✅ The previously-blocking bundle-size Critical is resolved. With the 125→127 KB budget bump,
npm run buildpasses — the daemon browser bundle is 129,228 B, within the 130,048 B budget (~820 B headroom). It would still fail the old 125 KB budget, so the bump is load-bearing. - ✅
@qwen-code/sdkunit suite passes (33/33, including the 6 new self-heal tests); ESLint clean on the changed files. - ✅ Traced the
reattach()single-flight coalescing, the lazyclientIdre-read on retry, admission-slot accounting across the retry (both paths), abort-during-reattach, and admission-time retry-safety — all correct as written. The SSE subscription is keyed bysessionId, so it survives theclientIdrefresh.
Test checks still pending, review-pr failing). The bundle blocker that drove the earlier CHANGES_REQUESTED is fixed here — worth re-approving once CI goes green.
— claude-opus-4-8[1m] via Qwen Code /qreview
wenshao
reviewed
Jun 24, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new findings on R2. The incremental changes since R1 address the Critical (bundle budget bumped to 127KB — SDK build passes) and three of the four Suggestions (test moved to its own describe block, reattach guard-clearing invariant tested, predicate branch coverage partially addressed with the 400-with-different-code test). Tests pass 34/34. Downgraded from Approve to Comment: CI still running (17 checks pending).
— qwen3.7-max via Qwen Code /review
doudouOUC
commented
Jun 24, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Critical] Browser SDK bundle exceeds 128KB budget (packages/sdk-typescript/scripts/build.js:31): The new self-heal code adds ~577 bytes. Combined with a pre-existing ~651-byte headroom deficit on main, the total is 129,228 bytes vs the 128,000 byte budget. npm run build fails — this is the CI blocker. Fix: bump MAX_DAEMON_BROWSER_BUNDLE_BYTES from 125 * 1024 to 127 * 1024.
Overall: the core self-heal logic is sound and well-tested (33/33 unit tests pass). The bundle budget bump and self-heal observability (inline below) need addressing before merge. Three Suggestions for robustness improvements also below.
| * retry the admission exactly once. Any other error (and a second | ||
| * `invalid_client_id`) propagates. | ||
| */ | ||
| private async withClientIdSelfHeal<T>(fn: () => Promise<T>): Promise<T> { |
Collaborator
Author
There was a problem hiding this comment.
[Critical] Zero logging/telemetry in the self-heal path. When isInvalidClientId matches and reattach() fires, there is no log, no counter, and no diagnostic callback. When self-heal succeeds, the recovery is invisible. When it fails, the propagated error (POST /session/:id/resume: session gone) gives no context that a prompt triggered the reattach.
An oncall engineer seeing this error has no way to know it was part of a prompt self-heal attempt, nor which stale clientId was rejected.
Consider adding a lightweight diagnostic callback on DaemonSessionClientOptions (e.g. onDiagnostic?: (msg: string) => void) and emitting at three points: (1) isInvalidClientId match, (2) reattach() success, (3) reattach() failure.
— qwen3.7-max via Qwen Code /review
| .resumeSession(this.sessionId, { workspaceCwd: this.workspaceCwd }) | ||
| .then((session) => { | ||
| // Refresh only the clientId; leave the SSE cursor and ACP state intact. | ||
| this.session.clientId = session.clientId; |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] session.clientId is typed as string | undefined (DaemonSession.clientId?). The daemon always returns a clientId in practice (invariant #3 in the design doc), but the TypeScript type doesn't enforce this. A future daemon change that makes clientId conditionally absent would silently set this.session.clientId = undefined.
Suggested change
| this.session.clientId = session.clientId; | |
| .then((session) => { | |
| if (!session.clientId) throw new Error('reattach: daemon returned no clientId'); | |
| // Refresh only the clientId; leave the SSE cursor and ACP state intact. | |
| this.session.clientId = session.clientId; | |
| }); |
— qwen3.7-max via Qwen Code /review
| // restore path resolves the workspace key before its existing-session fast | ||
| // path, and that resolution rejects a missing/relative path. | ||
| this.reattaching = this.client | ||
| .resumeSession(this.sessionId, { workspaceCwd: this.workspaceCwd }) |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] When resumeSession fails here, the DaemonHttpError propagates as POST /session/:id/resume: <detail> with no indication that this was triggered by a prompt self-heal. The TUI adapter's reportDaemonFailure will display a resume failure, and the user/oncall engineer has no idea why a resume was happening during a prompt.
Consider wrapping the error with self-heal context before rethrowing in withClientIdSelfHeal:
try {
await this.reattach();
} catch (reattachErr) {
throw new Error(
`clientId self-heal reattach failed (session=${this.sessionId}, stale clientId=${this.clientId}): ${reattachErr instanceof Error ? reattachErr.message : reattachErr}`,
{ cause: reattachErr },
);
}— qwen3.7-max via Qwen Code /review
| * fresh daemon-assigned `clientId`. Concurrent callers coalesce onto a single | ||
| * in-flight `resume` so we never orphan extra registrations. | ||
| */ | ||
| private async reattach(): Promise<void> { |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] reattach() does not forward the caller's AbortSignal to resumeSession (which has no signal parameter). If the signal aborts during the resume HTTP call, reattach() completes unconditionally and only the retry's signal.throwIfAborted() catches the abort.
This is acceptable for the primary scenario (daemon just restarted → fast response), but if the daemon is partially up (accepts TCP but hangs), the user's abort is delayed until the HTTP timeout. Low urgency — consider documenting as an accepted limitation or adding a signal parameter to resumeSession in a follow-up.
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 24, 2026
Collaborator
|
@qwen-code /triage |
| .resumeSession(this.sessionId, { workspaceCwd: this.workspaceCwd }) | ||
| .then((session) => { | ||
| // Refresh only the clientId; leave the SSE cursor and ACP state intact. | ||
| this.session.clientId = session.clientId; |
Collaborator
There was a problem hiding this comment.
[Suggestion] reattach() unconditionally assigns session.clientId without validating it's non-empty. If the daemon returns a malformed response (e.g., clientId: undefined or empty string), the client silently enters a corrupted state where subsequent prompts omit the x-qwen-client-id header entirely, producing opaque failures instead of a clear error.
Suggested change
| this.session.clientId = session.clientId; | |
| if (!session.clientId) { | |
| throw new Error('reattach: daemon returned no clientId'); | |
| } | |
| this.session.clientId = session.clientId; |
— qwen3.7-max via Qwen Code /review
| if (req.url.endsWith('/session/s-1/resume')) { | ||
| resumeCalls++; | ||
| return jsonResponse(200, { | ||
| sessionId: 's-1', |
Collaborator
There was a problem hiding this comment.
[Suggestion] The concurrent coalescing test tracks promptCalls but never asserts it. Without expect(promptCalls).toBe(4) (2 initial rejections + 2 retries), the test cannot distinguish "both prompts were retried after coalesced reattach" from a bug that silently drops one retry (which would timeout rather than fail fast). Adding the count assertion documents the expected call pattern and provides a clearer failure signal.
— qwen3.7-max via Qwen Code /review
| /* keep subscription active */ | ||
| } | ||
| })().catch(() => {}); | ||
| await vi.waitFor(() => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The non-blocking self-heal test calls waitForPendingPrompt(session, 'p-2') but doesn't verify that _pendingPrompts contains exactly one entry. If the self-heal had a bug that leaked a pending entry from the failed first attempt (e.g., by registering before admission rather than after), the test would still pass because waitForPendingPrompt only checks membership, not size. Consider adding expect([...session._pendingPrompts.keys()]).toEqual(['p-2']) to catch such leaks.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
approved these changes
Jun 24, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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
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.
What this PR does
Follow-up to #5784. That PR made stale/unregistered prompt client ids fail at admission with a synchronous
400 invalid_client_id(instead of202-then-silent-async-fail). This PR adds the client-side self-heal: whenDaemonSessionClient.prompt()receives that rejection, it re-registers to obtain a freshclientIdand retries the prompt once, so the turn proceeds without the user manually resending.Why it's needed
After a daemon restart or session reload, the daemon's in-memory client registration is wiped. A frontend still holding an older server-assigned
clientIdsends a prompt that the bridge rejects withinvalid_client_id. Before this change the UI would surface an error and require a manual resend (or, pre-#5784, hang in "processing" forever). This restores the turn automatically.This was diagnosed from a production incident where a prompt was sent by one client id while the session had been reloaded under a different id — the prompt-sending client was never registered, so the turn never produced a terminal event.
How it works
In
packages/sdk-typescript/src/daemon/DaemonSessionClient.ts:isInvalidClientId(err)— matchesDaemonHttpErrorwithstatus 400andbody.code === 'invalid_client_id'(the shape fix(daemon): Reject stale prompt client admission #5784 returns).reattach()— single-flight; callsresumeSession(no replay) with an explicitworkspaceCwdand no clientId, so the bridge issues a fresh registration. Refreshes onlythis.session.clientId; leaves the SSE cursor and ACP state untouched.withClientIdSelfHeal(fn)— runs the admission call; oninvalid_client_idit reattaches and retries exactly once; any other error (and a secondinvalid_client_id) propagates.prompt()paths (blockingclient.promptand non-blockingclient.promptNonBlocking), wrapping only the admission call so the local prompt slot is reserved once and reused across the retry.Why the retry is safe
invalid_client_idis an admission-time rejection (resolveTrustedClientIdruns before the turn registers / before202, per #5784), so the prompt never executed — retrying cannot double-execute the user's message.Tests
6 new tests in
DaemonSessionClient.test.ts(via the existingrecordingFetchHTTP harness): blocking self-heal, non-blocking self-heal, retry-bounded (no loop), non-invalid_client_iderrors not retried, reattach-failure propagation, and concurrent single-flight re-registration. FullDaemonSessionClientsuite: 33/33 pass on the merged #5784 base.Out of scope
SSE reconnection (the app layer's existing responsibility), and self-heal for other clientId-bearing methods (
cancel/btw/shell/heartbeat) — onlyprompt()self-heals.Design doc:
docs/superpowers/specs/2026-06-24-daemon-clientid-self-heal-design.md🤖 Generated with Qwen Code