-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(channels): bound session lifetime with sessionRotation #8927
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
710a121
fa2f082
53a6777
48758ff
6dbca59
e4f78f3
3d6fedf
1759b9e
0452293
3f7ffe2
0a86d72
e570597
0646611
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 |
|---|---|---|
|
|
@@ -98,6 +98,9 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { | |
| timeout: ReturnType<typeof setTimeout>; | ||
| } | ||
| >(); | ||
| private readonly pendingSessionRequests = new Set<{ | ||
| reject: (error: Error) => void; | ||
| }>(); | ||
|
|
||
| constructor(options: AcpBridgeOptions) { | ||
| super(); | ||
|
|
@@ -149,6 +152,7 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { | |
| ); | ||
| // Do not emit sessionDied here: a full ACP process exit is handled by | ||
| // channel start crash recovery, which reloads the persisted sessions. | ||
| this.rejectPendingSessionRequests(); | ||
| this.resolvePendingPermissions(); | ||
|
Comment on lines
154
to
156
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. [Critical] Rejecting in-flight 中文说明严重:子进程退出时拒绝在途 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| this.knownSessionIds.clear(); | ||
| this.sessionBindingTokens.clear(); | ||
|
|
@@ -232,11 +236,14 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { | |
| bindingToken?: object, | ||
| ): Promise<string> { | ||
| const conn = this.ensureConnection(); | ||
| await this.registerChannelLoopMcpServer(); | ||
| const response = await conn.newSession({ cwd, mcpServers: [] }); | ||
| this.knownSessionIds.add(response.sessionId); | ||
| this.sessionBindingTokens.set(response.sessionId, bindingToken); | ||
| return response.sessionId; | ||
| const sessionId = await this.settleOnChildExit(async () => { | ||
| await this.registerChannelLoopMcpServer(); | ||
| const response = await conn.newSession({ cwd, mcpServers: [] }); | ||
| return response.sessionId; | ||
| }); | ||
| this.knownSessionIds.add(sessionId); | ||
| this.sessionBindingTokens.set(sessionId, bindingToken); | ||
| return sessionId; | ||
| } | ||
|
|
||
| async loadSession( | ||
|
|
@@ -246,11 +253,13 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { | |
| bindingToken?: object, | ||
| ): Promise<string> { | ||
| const conn = this.ensureConnection(); | ||
| await this.registerChannelLoopMcpServer(); | ||
| await conn.loadSession({ | ||
| sessionId, | ||
| cwd, | ||
| mcpServers: [], | ||
| await this.settleOnChildExit(async () => { | ||
| await this.registerChannelLoopMcpServer(); | ||
| await conn.loadSession({ | ||
| sessionId, | ||
| cwd, | ||
| mcpServers: [], | ||
| }); | ||
| }); | ||
| this.knownSessionIds.add(sessionId); | ||
| this.sessionBindingTokens.set(sessionId, bindingToken); | ||
|
|
@@ -361,6 +370,7 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { | |
| } | ||
|
|
||
| stop(): void { | ||
| this.rejectPendingSessionRequests(); | ||
|
Comment on lines
372
to
+373
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] R7-5: The 中文说明建议:本 diff 新增的 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| this.resolvePendingPermissions(); | ||
| this.knownSessionIds.clear(); | ||
| this.sessionBindingTokens.clear(); | ||
|
|
@@ -462,6 +472,40 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { | |
| return this.connection; | ||
| } | ||
|
|
||
| /** | ||
| * The ACP SDK never settles requests still awaiting a response once the | ||
| * stream ends, so a child death mid-request would hang the caller forever; | ||
| * a restore's persist suspension, in particular, would never lift. Reject | ||
| * those requests when the child exits instead. | ||
| */ | ||
| private settleOnChildExit<T>(run: () => Promise<T>): Promise<T> { | ||
| return new Promise<T>((resolve, reject) => { | ||
| const pending = { reject }; | ||
| this.pendingSessionRequests.add(pending); | ||
| run().then( | ||
| (result) => { | ||
| this.pendingSessionRequests.delete(pending); | ||
| resolve(result); | ||
| }, | ||
|
Comment on lines
+486
to
+489
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] R7-6: Test-efficacy probe (harness validated): no test fails when this success-path 中文说明建议:测试有效性探针(harness 已验证):删除成功路径上的这个 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| (error: unknown) => { | ||
| this.pendingSessionRequests.delete(pending); | ||
| reject(error); | ||
| }, | ||
|
Comment on lines
+490
to
+493
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] R7-7: Test-efficacy probe (harness validated): no test fails when this error-path 中文说明建议:测试有效性探针(harness 已验证):删除错误路径上的这个 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
| ); | ||
| }); | ||
| } | ||
|
|
||
| private rejectPendingSessionRequests(): void { | ||
| for (const pending of this.pendingSessionRequests) { | ||
| pending.reject( | ||
| new Error( | ||
| 'ACP agent process exited while a session request was in flight', | ||
| ), | ||
| ); | ||
| } | ||
| this.pendingSessionRequests.clear(); | ||
| } | ||
|
Comment on lines
+504
to
+507
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] R7-8: Test-efficacy probe (harness validated): no test fails when 中文说明建议:测试有效性探针(harness 已验证):删除 — qwen3.8-max via Qwen Code /review (v0.21.10) |
||
|
|
||
| private requestPermission( | ||
| request: RequestPermissionRequest, | ||
| ): Promise<RequestPermissionResponse> { | ||
|
|
||
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] The documented
sessionScope: singlerotation semantics in this sentence (only the triggering chat is notified; other chats sharing the session see a silent reset) have zero test coverage — every rotation test inChannelBase.test.tsuses the defaultuserscope or threads, and no test anywhere inpackages/combines single scope with rotation. This sentence was added as the R5-8 fix, and the announce target comes from the triggering message's input (the R2-8 fix) — nothing pins either in-tree. — Failure scenario: a future refactor that fetches the announce target from the stored route target (reverting R2-8), or that broadcasts the notice to every chat on a single-scope route, would silently change this user-visible documented behaviour with the entire suite green. — Suggested fix: add a ChannelBase rotation test withsessionScope: 'single': two chats share the route, hitmaxTurnsfrom chat A, assert chat A receives the rotation notice and chat B receives none while its next prompt lands on the fresh session.中文说明
建议:本句所记载的
sessionScope: single轮换语义(只通知触发轮换的那个聊天;共享该会话的其他聊天静默重置)没有任何测试覆盖——ChannelBase.test.ts中的全部轮换测试都使用默认user作用域或 thread,packages/下也没有任何测试把 single 作用域与轮换组合起来。这句话是作为 R5-8 的修复加入的,通知目标取自触发消息的输入(R2-8 的修复)——两者在仓库中都没有测试钉住。失败场景:未来若有重构把通知目标改为取自存储的路由 target(回退 R2-8),或把通知广播给 single 作用域路由上的所有聊天,都会静默改变这一用户可见的已记载行为,而整个测试套件全绿。建议修复:新增一个sessionScope: 'single'的 ChannelBase 轮换测试:两个聊天共享同一路由,从聊天 A 触达maxTurns,断言聊天 A 收到轮换通知、聊天 B 收不到通知且其下一条消息落在新会话上。— qwen3.8-max via Qwen Code /review (v0.21.10)