feat(serve): add client heartbeat (#4175 Wave 2.5 PR 9)#4235
Merged
Conversation
Contributor
📋 Review SummaryThis PR adds client heartbeat functionality to the qwen-serve daemon, enabling long-lived adapters (TUI/IDE/web) to refresh the daemon's last-seen bookkeeping for sessions. The implementation is well-designed with proper security validation, comprehensive test coverage, and maintains backward compatibility. The code follows existing patterns and architectural decisions from the codebase. 🔍 General Feedback
🎯 Specific Feedback🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a client heartbeat surface to qwen serve: a new POST /session/:id/heartbeat route, bridge bookkeeping for per-session and per-client last-seen timestamps, SDK helpers, and a client_heartbeat capability tag. This feeds future diagnostics (PR 12) and revocation policy (PR 24) from the Stage 1.5 daemon roadmap.
Changes:
- Bridge gains
recordHeartbeat/getHeartbeatState, withSessionEntrytrackingsessionLastSeenAtand aclientLastSeenAtmap cleaned up alongsideunregisterClient. - New HTTP route uses
parseClientIdHeader+sendBridgeErrorto mapInvalidClientIdError/SessionNotFoundErrorto400 invalid_client_id/404; client validation runs before any timestamp bump. - SDK
DaemonClient.heartbeat()/DaemonSessionClient.heartbeat()+HeartbeatResulttype; capability registry and protocol/user docs updated.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/capabilities.ts | Advertises client_heartbeat capability tag. |
| packages/cli/src/serve/server.ts | Registers POST /session/:id/heartbeat route, mirrors existing cancel/setModel error mapping. |
| packages/cli/src/serve/httpAcpBridge.ts | Adds heartbeat bookkeeping types, SessionEntry fields, recordHeartbeat/getHeartbeatState, and cleanup on unregisterClient. |
| packages/cli/src/serve/server.test.ts | Adds route tests (happy path, header forwarding, malformed/unknown client id, unknown session). |
| packages/cli/src/serve/httpAcpBridge.test.ts | Adds bridge tests (anonymous bump, per-client bump, not found, pre-validation, detach cleanup, snapshot immutability). |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds heartbeat(sessionId, clientId?) helper through fetchWithTimeout. |
| packages/sdk-typescript/src/daemon/DaemonSessionClient.ts | Forwards bound clientId to client.heartbeat. |
| packages/sdk-typescript/src/daemon/types.ts | Adds HeartbeatResult interface. |
| packages/sdk-typescript/src/daemon/index.ts | Re-exports HeartbeatResult. |
| packages/sdk-typescript/test/unit/DaemonClient.test.ts | Adds heartbeat unit tests (POST shape, header, 404, 400 invalid_client_id). |
| packages/sdk-typescript/test/unit/DaemonSessionClient.test.ts | Asserts bound clientId forwarded on heartbeat. |
| integration-tests/cli/qwen-serve-routes.test.ts | Updates capability snapshot for the new tag. |
| docs/developers/qwen-serve-protocol.md | Documents the new route, header, response, and capability gating. |
| docs/users/qwen-serve.md | Marks the client-heartbeat reliability gap as shipped. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
Thanks for the structured review @github-actions. Going through each item with disposition + rationale below. (Note: these are top-level summary suggestions, not inline threads, so there's no "Resolve conversation" affordance — this reply is the resolution record.) 🟢 MediumM1 — The same fact is already documented at the field declaration in
Repeating the rationale inside the 5-line M2 —
M3 — Replace Inconsistent with the existing pattern in the same file:
Empty bodies use the literal form across the file. Heartbeat follows the same convention. 🔵 LowL1 — Verify
L2 — Add "Shipped in PR #4235" reference in user docs → Won't apply (already covered) The user-doc edit links to L3 — JSDoc note that
L4 — Comment on The same idiom is repeated identically across 6 routes in Summary: 0/7 adopted. 3 items duplicate existing documentation, 2 conflict with the project's style consistency, 1 has an incorrect premise, 1 offers no observable safety improvement. PR remains as-is awaiting human review. cc @wenshao @chiga0 |
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. |
Collaborator
Author
Codex review — dispositionCodex returned one [P2] finding from the background review, and it's correct:
Verified: Adopted: pushed Codex flagged no other issues; the rest of the heartbeat surface (route, bridge bookkeeping, capability tag, tests) was reported "largely consistent". cc @wenshao @chiga0 |
Adds POST /session/:id/heartbeat plus SDK helpers so long-lived adapters (TUI/IDE/web) can refresh the daemon's last-seen bookkeeping. Bridge stores per-session and per-client timestamps behind a getHeartbeatState() snapshot accessor that PR 12 read-only diagnostics and PR 24 revocation policy will consume. - Capability tag: client_heartbeat (advertised on /capabilities.features) - Identified clients must echo X-Qwen-Client-Id; the bridge validates the id BEFORE bumping any timestamp so a forged id can't mask client absence - Per-client entries are dropped together with the registration ref-count in unregisterClient, so churn doesn't leak stale ids - getHeartbeatState returns a snapshot Map; mutating it does not leak into bridge state - Anonymous heartbeats bump only the per-session watermark Errors mirror the rest of the routes — 404 SessionNotFoundError, 400 invalid_client_id (header malformed or unknown for this session). Roadmap PR 9 from #4175. Depends on PR 7 (#4231 client identity, merged) for the trusted clientId registry. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
The published @qwen-code/sdk only exposes the root entrypoint via
`exports`; daemon subpath imports are not part of the public API.
Adding HeartbeatResult to packages/sdk-typescript/src/daemon/index.ts
made it reachable internally but not for downstream consumers writing
`import type { HeartbeatResult } from '@qwen-code/sdk'` — every other
daemon result type (PromptResult, SetModelResult, DaemonSession, etc.)
is forwarded through the root barrel, so HeartbeatResult was the only
hole in the heartbeat helper's public surface.
Inserted alphabetically between DaemonStreamLifecycleEvent and
KnownDaemonEvent to match the existing ordering convention.
d1fa1bd to
c951588
Compare
wenshao
approved these changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
This PR adds client heartbeat to qwen-serve with clean implementation across all layers (route, bridge, SDK). tsc + eslint pass cleanly, all 3471 serve/SDK tests pass (1 pre-existing flaky test unrelated to this PR). The security model is sound: clientId validation runs before any timestamp update, bearer-auth gates all routes, error shapes follow existing sendBridgeError patterns.
Minor forward-looking suggestions (not blockers):
sessionLastSeenAtis undefined until first heartbeat — initializing at spawn time would make future revocation policy code safer- Heartbeat uses the 30s
fetchTimeoutMsdefault — callers with shorter heartbeat intervals should configure a lower timeout
— DeepSeek/deepseek-v4-pro via Qwen Code /review
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.
Summary
POST /session/:id/heartbeatplus SDK helpers (DaemonClient.heartbeat()/DaemonSessionClient.heartbeat()) so long-lived adapters (TUI/IDE/web) can refresh the daemon's last-seen bookkeeping.getHeartbeatState()snapshot accessor that the future Wave 3 PR 12 read-only diagnostics route and Wave 5 PR 24 revocation policy will consume.client_heartbeatadvertised on/capabilities.features.Roadmap PR 9 from #4175 Wave 2.5. Depends on PR 7 (#4231 — daemon-stamped client identity), already merged, for the trusted
clientIdregistry.Design
POST /session/:id/heartbeat. Empty body. Returns200 { sessionId, clientId?, lastSeenAt }(server-sideDate.now()epoch ms). 404 unknown session, 400invalid_client_id(header malformed or unknown for this session — same shape used bycancel/setSessionModel).X-Qwen-Client-Id, validated via the existingparseClientIdHeader.recordHeartbeat()andgetHeartbeatState(). The newBridgeHeartbeatResultandBridgeHeartbeatStatetypes stay in this PR; the HTTP-side state route is deferred to PR 12.SessionEntrygainssessionLastSeenAt?: numberandclientLastSeenAt: Map<string, number>.Security properties
clientIdBEFORE bumping any timestamp (via the existingresolveTrustedClientId). Without this, an attacker holding a valid bearer could mask client absence by spamming heartbeats with random ids — the test'rejects an unknown client id without bumping any timestamp'covers it.unregisterClientdrops the per-clientclientLastSeenAtentry alongside the registration ref-count, so a long-lived daemon servicing churn doesn't accumulate stale ids — the very leak revocation policy (PR 24) is meant to plug.getHeartbeatStatereturns a snapshotMap; mutating it can't reach into bridge state. Type returned asReadonlyMapfor compile-time guidance, copy-on-read for runtime safety.clientId.Backward compatibility
caps.features.client_heartbeat.EXPECTED_STAGE1_FEATURESand the integration-test capability snapshot are updated in lockstep withSERVE_CAPABILITY_REGISTRY.Validation
npm run typecheck --workspace packages/cli --workspace packages/sdk-typescript✅cd packages/cli && npx vitest run src/serve/→ 245/245 passing (server / bridge / eventBus / inMemoryChannel)cd packages/sdk-typescript && npx vitest run test/unit/→ 307/307 passingnpx eslinton every changed file ✅npm run lintworkspace-wide passes for cli; sdk-typescript still hits the pre-existing ESLint rule-loading error fromProcessTransport.test.tsthat PR 7 also flagged.Tests
14 new test cases:
server.test.ts— happy / header forward / malformed header / bridgeInvalidClientIdError/ unknown sessionhttpAcpBridge.test.ts— anonymous bump / per-client bump /SessionNotFoundError/ pre-validation invariant / detach cleanup / snapshot immutabilityDaemonClient.test.ts— POST shape / header forward / 404 / 400 invalid_client_idDaemonSessionClient.test.ts— forwards boundclientIdintegration-tests/cli/qwen-serve-routes.test.ts— capability snapshot kept in syncEngineering principles checklist (Stage 1.5 wave PRs)
qwen serveStage 1 routes / SDK behavior preservedTesting Matrix
Local macOS validation covered targeted typecheck, vitest, lint. Windows/Linux left to CI.
Linked Issues
🤖 Generated with Qwen Code