-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(serve): establish workspace runtime ownership #8213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c7f52e7
d3ac4c3
59ec610
d9d491c
95216ea
c4a9ccf
230ea2e
d2641c4
57268b4
40771a3
b0d9f3d
7755079
90bbec5
82fee67
350741a
67d66c9
c8c7782
15c30e3
c02bd06
8d07696
bc32ca1
d634ae6
e34cce8
1c3d08a
b39737e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -92,7 +92,7 @@ Bridge closure (createHttpAcpBridge) | |||||||||||||
|
|
||||||||||||||
| | Mechanism | Scope | What it manages | | ||||||||||||||
| | ----------------------------------------- | ------------------------- | -------------------------------------------------------------------------------- | | ||||||||||||||
| | `channelIdleTimeoutMs` + `startIdleTimer` | Channel (child process) | Kills the `qwen --acp` child when ALL sessions are gone | | ||||||||||||||
| | `channelIdleTimeoutMs` + `startIdleTimer` | Channel (child process) | Unset or `0` reaps immediately; a positive value delays reap | | ||||||||||||||
| | **Session reaper** (this design) | Session (in-memory entry) | Closes individual sessions when idle | | ||||||||||||||
| | `ConnectionRegistry` sweep | ACP-over-HTTP connection | Reaps `/acp` transport-layer connections (different layer) | | ||||||||||||||
| | `writerIdleTimeoutMs` | SSE subscriber | Evicts a single stuck SSE subscriber | | ||||||||||||||
|
|
@@ -309,6 +309,8 @@ function. | |||||||||||||
|
|
||||||||||||||
| - `startSessionReaper()` is called at bridge construction time (after | ||||||||||||||
| option validation, alongside the existing `channelIdleTimeoutMs` setup). | ||||||||||||||
| Omitting that channel option or setting it to `0` reaps an idle Workspace | ||||||||||||||
| Runtime immediately; configured values must be non-negative. | ||||||||||||||
|
Comment on lines
+312
to
+313
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The sentence added by this PR — "Omitting that channel option or setting it to
Suggested change
中文说明本 PR 新增的这句话——"Omitting that channel option or setting it to — qwen3.8-max-preview via Qwen Code /review (v0.21.6) |
||||||||||||||
| - `stopSessionReaper()` is called in both `shutdown()` and `killAllSync()`. | ||||||||||||||
|
|
||||||||||||||
| ### 4.6 Interaction with existing `closeSession` callers | ||||||||||||||
|
|
@@ -367,20 +369,20 @@ generic terminal-frame handler (`isTerminalLifecycleEvent`) already handles | |||||||||||||
|
|
||||||||||||||
| ### 5.1 Unit tests (`bridge.test.ts`) | ||||||||||||||
|
|
||||||||||||||
| | # | Test | Description | | ||||||||||||||
| | --- | ------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||||||||||||||
| | 1 | Idle session is reaped after timeout | Create a session, advance time past `sessionIdleTimeoutMs`, trigger reaper tick, verify session removed from `byId` and `session_closed` event published with `reason: 'idle_timeout'` | | ||||||||||||||
| | 2 | Session with active prompt is NOT reaped | Create a session, start a prompt, advance time, verify session survives reaper tick | | ||||||||||||||
| | 3 | Session with live SSE subscriber is NOT reaped | Create a session, subscribe to its EventBus, advance time, verify session survives | | ||||||||||||||
| | 4 | Session with registered client is NOT reaped | Create a session, register a clientId, advance time, verify session survives | | ||||||||||||||
| | 5 | Reaper disabled when interval = 0 | Pass `sessionReapIntervalMs: 0`, verify no `setInterval` is armed | | ||||||||||||||
| | 6 | Reaper disabled when timeout = 0 | Pass `sessionIdleTimeoutMs: 0`, verify no `setInterval` is armed | | ||||||||||||||
| | 7 | Reaper stopped on shutdown | Call `shutdown()`, verify `clearInterval` was called | | ||||||||||||||
| | 8 | closeSession reason defaults to 'client_close' | Call `closeSession` without explicit reason, verify published event has `reason: 'client_close'` | | ||||||||||||||
| | 9 | closeSession with explicit reason | Call `closeSession` with `reason: 'idle_timeout'`, verify published event | | ||||||||||||||
| | 10 | Multiple idle sessions reaped in one tick | Create 3 idle sessions, advance time, trigger tick, verify all 3 reaped | | ||||||||||||||
| | 11 | Session with heartbeat within TTL survives | Create a session, record heartbeat, advance time to just under TTL, verify session survives | | ||||||||||||||
| | 12 | Channel idle timer triggered after last session reaped | Create 1 session (last on channel), reap it, verify `startIdleTimer` is called on the channel | | ||||||||||||||
| | # | Test | Description | | ||||||||||||||
| | --- | ------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||||||||||||||
| | 1 | Idle session is reaped after timeout | Create a session, advance time past `sessionIdleTimeoutMs`, trigger reaper tick, verify session removed from `byId` and `session_closed` event published with `reason: 'idle_timeout'` | | ||||||||||||||
| | 2 | Session with active prompt is NOT reaped | Create a session, start a prompt, advance time, verify session survives reaper tick | | ||||||||||||||
| | 3 | Session with live SSE subscriber is NOT reaped | Create a session, subscribe to its EventBus, advance time, verify session survives | | ||||||||||||||
| | 4 | Session with registered client is NOT reaped | Create a session, register a clientId, advance time, verify session survives | | ||||||||||||||
| | 5 | Reaper disabled when interval = 0 | Pass `sessionReapIntervalMs: 0`, verify no `setInterval` is armed | | ||||||||||||||
| | 6 | Reaper disabled when timeout = 0 | Pass `sessionIdleTimeoutMs: 0`, verify no `setInterval` is armed | | ||||||||||||||
| | 7 | Reaper stopped on shutdown | Call `shutdown()`, verify `clearInterval` was called | | ||||||||||||||
| | 8 | closeSession reason defaults to 'client_close' | Call `closeSession` without explicit reason, verify published event has `reason: 'client_close'` | | ||||||||||||||
| | 9 | closeSession with explicit reason | Call `closeSession` with `reason: 'idle_timeout'`, verify published event | | ||||||||||||||
| | 10 | Multiple idle sessions reaped in one tick | Create 3 idle sessions, advance time, trigger tick, verify all 3 reaped | | ||||||||||||||
| | 11 | Session with heartbeat within TTL survives | Create a session, record heartbeat, advance time to just under TTL, verify session survives | | ||||||||||||||
| | 12 | Channel idle policy evaluated after last session reaped | Create 1 session (last on channel), reap it, verify an unset timeout reaps the channel and an explicit positive timeout arms the compatibility timer | | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The rewritten test-plan row 12 claims test 12 verifies both idle-policy branches, but the actual test ( 中文说明重写后的测试计划第 12 行声称测试 12 同时验证了空闲策略的两个分支,但实际测试( — qwen3.8-max via Qwen Code /review (v0.21.10) |
||||||||||||||
|
|
||||||||||||||
| ### 5.2 Integration tests (`server.test.ts`) | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -431,4 +433,4 @@ generic terminal-frame handler (`isTerminalLifecycleEvent`) already handles | |||||||||||||
| | `closeSession` inside reaper throws, poisoning the scan loop | Each close is in its own `.catch()` — one failure doesn't block others | | ||||||||||||||
| | Reaper iteration over `byId` during concurrent `closeSession` from another path | ES2015 Map iteration tolerates deletion of current/previous keys. Double-close is idempotent (`byId.get` returns undefined → `SessionNotFoundError` caught by reaper's `.catch`). | | ||||||||||||||
| | Performance of scanning 20 sessions every 60s | Trivial — 20 Map reads + 4 field checks each. No I/O. | | ||||||||||||||
| | Channel idle timer interaction | When the last session is reaped, `closeSession` already calls `startIdleTimer` on the channel. No additional logic needed. | | ||||||||||||||
| | Channel idle timer interaction | When the last session is reaped, `closeSession` calls `startIdleTimer`; an unset or zero timeout reaps immediately, while an explicit positive timeout arms the timer. | | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] This diff rewrote the correctly-scoped invariant "an untrusted secondary must not start either" into the over-general "an untrusted workspace must not start either", but an untrusted primary still starts its ACP child via the legacy singular session routes. This document is the self-declared review-contract baseline for the hardening work. — Failure scenario: a future reviewer wielding this invariant will flag the legitimate untrusted-primary session path as a hardening violation. Suggested fix: restore the secondary scoping, e.g. "…each started child owns its pool; an untrusted secondary must not start either (an untrusted primary starts only through the legacy singular Session compatibility surface). Primary compatibility routing does not bypass route trust gates."
中文说明
本 diff 把范围正确的不变量 "an untrusted secondary must not start either" 改写成了过度泛化的 "an untrusted workspace must not start either",但不受信任的 primary 仍会通过 legacy singular session 路由启动其 ACP 子进程。该文档是硬化工作自述的评审契约基线。— 失败场景:未来拿着这条不变量评审的人会把合法的"不受信任 primary session 路径"标记为硬化违规。建议恢复 secondary 范围限定,例如:"…each started child owns its pool; an untrusted secondary must not start either (an untrusted primary starts only through the legacy singular Session compatibility surface). Primary compatibility routing does not bypass route trust gates."
— qwen3.8-max-preview via Qwen Code /review (v0.21.6)