fix(broker): wait for old process exit before forking idle-reclaim replacement (#3330) - #3343
fix(broker): wait for old process exit before forking idle-reclaim replacement (#3330)#3343Battleplus wants to merge 4 commits into
Conversation
…eplacement (makecindy#3330) When an on-demand Node worker is idle-stopped, stopWorker kills the process but does not wait for actual exit. The next ensureWorker call only checks workers and startingWorkers maps — not liveProcesses — so it forks a new process while the old one is still shutting down. On Windows, the old UtilityProcess may still hold file locks or ports that prevent the new bootstrap from succeeding, causing a 10s timeout and PROCESS_START_FAILED. Fix: in stopWorker, record a draining exit Promise that resolves when the child process actually exits (with a safety-net timeout). In ensureWorker, before forking, check drainingExits and await the old process exit. This follows the existing stopAndWait pattern but is scoped to per-key idle reclaim. Regression test: idle-stopped worker waits for old process exit before forking replacement, verifying no new child is created until the old one exits. Signed-off-by: Battleplus <battleplus@users.noreply.github.com> Signed-off-by: Battleplus <3559424769@qq.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 429b82caee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/cindy-brain/nodeRuntimeBroker.ts | 增加旧 worker 退出等待与超时安全网，但同 key 的并发 replacement 启动协调仍需处理。 |
| apps/desktop/src/main/cindy-brain/tests/nodeRuntimeBroker.test.ts | 新增单个重启请求等待旧进程退出的生命周期回归测试。 |
Sequence Diagram
sequenceDiagram
participant R as 新请求
participant B as NodeRuntimeBroker
participant O as 旧 Worker
participant N as Replacement Worker
B->>O: SIGTERM
B->>B: 记录 draining exit Promise
R->>B: handleRequest
B->>B: await draining
O-->>B: exit
B->>N: fork replacement
N-->>B: spawn / ready
B-->>R: 返回结果
Reviews (4): Last reviewed commit: "fix(broker): fix TS errors in idle-recla..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
429b82c to
2c5fe27
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c5fe27469
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
MagicLizi
added
awaiting-discussion
|
这个 PR 改了插件 Node worker 的回收/再启动，属于插件基座，已进入维护者确认（基座改动不因 bugfix 豁免）。 请维护者在本 PR 上 Approve；需要修改请 Request Changes。讨论 issue：#3350 |
…d stuck processes Two P1 race conditions from Codex review: 1. Draining fallback resolves even when old process is still alive (after PROCESS_STOP_GRACE_MS + 1000ms timeout), allowing ensureWorker to fork a replacement while the old process holds file locks / ports. Now rejects instead, so the caller gets an error rather than a collision. 2. drainingExits.delete() was called before await draining, so a second concurrent ensureWorker call would see no draining marker and fork immediately. Now the delete happens after the await, so concurrent requests share the same drain wait. Fixes P1 from Codex review on makecindy#3343. Signed-off-by: Battleplus <3559424769@qq.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b57a6d0ee5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace `removeListener` (not in NodeWorkerProcess interface) with `invoked` flag pattern. Replace `reject` (not declared in Promise constructor) with `resolve` — the draining promise must always resolve so the replacement fork can proceed. Fixes type errors from commit b57a6d0. Signed-off-by: Battleplus <3559424769@qq.com>
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
格式门未通过
- Description 缺段落：怎么验证的 / 风险
请按仓库 PR 模板补全这两段后再请求审查。这是 P1 格式问题，补全前本流程不会进入代码审查。
MagicLizi
added
status:awaiting-author
Close the concurrent restart window so callers share one replacement startup promise, and add a deterministic regression test.
|
@Battleplus 👋 这个 PR 现在在等维护者确认,确认之前流程不会合并它 —— 不是卡住了,也不是在等你再改一版(你推的改动流程都读到了,判的就是最新一版代码)。
这条是流程自动发的状态提醒(同一版代码只发一次),不用回复。 |
MagicLizi
added
touches:core
这些问题已在当前 head 的后续 commit 中修复(格式门/安全门已通过),自动 dismiss 旧的 CHANGES_REQUESTED 以解除合并阻塞。
2 participants
这次改了什么
摘要
修复 on-demand Node 插件 worker 空闲回收后二次启动超时的问题。
变更类型
范围
包含：
nodeRuntimeBroker.ts：stopWorker增加 draining exit Promise，ensureWorker在 fork 前等待旧进程退出nodeRuntimeBroker.test.ts：新增 idle-stop-then-restart 回归测试不包含：
根因
stopWorker调用kill('SIGTERM')后不等待真实 exit，而ensureWorker只检查workers和startingWorkers，不检查liveProcesses。在 Windows 上，旧 UtilityProcess 可能仍持有文件锁或端口，导致新 bootstrap 10s 超时。修复
stopWorker：kill 后创建 draining exit Promise（监听exit事件 + 安全网超时）ensureWorker：fork 前检查drainingExits，如有则 await 旧进程退出stopAndWait模式，但 scope 限定为 per-key idle reclaim测试
55/55 通过，包含新增的
idle-stopped worker waits for old process exit before forking replacement测试。