Repository navigation
Conversation
|
Sentinel 已审核此变更，未发现已核验问题，但发现需要人工确认的风险。 另有 2 个需要人工确认的风险；黄色 advisory 不阻塞合并，建议开发 agent review advisory 评论并逐条确认。 审核覆盖提示： 开发 agent 查询详情： 调用 MCP 工具 由 Sentinel 审核 · 目标提交 35ba6cc32a28 |
|
Sentinel 保留了 2 个尚未确认的审核候选。这些黄色条目不阻塞合并；红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时，可用行末 由 Sentinel 审核 · 目标提交 35ba6cc32a28 |
forrestlinfeng
left a comment
forrestlinfeng
left a comment
There was a problem hiding this comment.
Code review — changes requested
Good that this reuses #28's admission rather than building a second mechanism. But it inherits #28's unrecoverable-guard flaw and widens its blast radius.
Blocker
- With every
updateStatenow going through admission and throwingELOCKEDinstead of running unguarded, a stranded state admission (PID reuse, or failed release under our own PID — see the #28 review) makes every update to that file (settings, sessions, drafts, candidates) throw forever until someone deletes the guard by hand. Before this PR the worst case was one unguarded write. Needs the same age fallback as #28.
Major
- Lock lost when admission release fails right after acquire (
file-lock.js~L85-100):mutatewriteslockPathand returnsacquired, thenreleaseAdmissionthrowsELOCKED(not in the retry list) andacquirereturnsnull. The token stays with our live PID, so all later updates from every process see busy →ELOCKED. Unlink our token before rethrowing. - Dead move-aside code + stale comment (~L59-75):
reclaim()still moves the lock aside, re-checks andlinkSyncs it back, guarding a race that full admission now rules out. The comment at ~L59-60 describes that race and is now wrong. Replace with a plain unlink; update theA-moved-freshtest accordingly.
Minor
ELOCKEDnow propagates to everystores.*.updatecaller (file-stores.js).legacy.jsswallows it; the other call sites, especially hook paths that must never block a session, need an audit.if (token)at ~L124 can no longer be false.- The three-state status object plus
nullis more protocol than needed: return the token,null, or throw. - One test waits the real 2 s timeout; the spawn/marker harness is a third copy (see #28).
Also: the description notes acquire() overlaps #19 — that combination needs a real merge + CI run.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Sentinel 已审核此变更，未发现已核验问题，但发现需要人工确认的风险。 另有 5 个需要人工确认的风险；黄色 advisory 不阻塞合并，建议开发 agent review advisory 评论并逐条确认。 审核覆盖提示： 开发 agent 查询详情： 调用 MCP 工具 由 Sentinel 审核 · 目标提交 68fab84ae688 |
|
Sentinel 保留了 5 个尚未确认的审核候选。这些黄色条目不阻塞合并；红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时，可用行末 由 Sentinel 审核 · 目标提交 68fab84ae688 |
forrestlinfeng
left a comment
forrestlinfeng
left a comment
There was a problem hiding this comment.
Round 2 — not yet
Fixed: the state token is now cleaned up when admission release fails (new test at test/runtime/state-lock.test.js:22-38), and the always-true if (token) is gone. 24/24 tests pass over 3 runs.
Blocking
- Inherits #28's open major (subprocess spawn on every process's first lock) — every
updateStategoes through admission, so every hook pays it. Needs #28's fix first. - Dead move-aside code is still there —
lib/runtime/store/file-lock.js:77-93reclaim()still renames the lock aside, re-checks andlinkSyncs it back. With all creation/reclaim serialized by admission that race can't happen any more. Please replace it with a plain unlink and remove the now-impossibleA-moved-freshbranch instate-lock.test.js(~L189, ~L247).
Still open (should fix)
- The status object plus
nullfromacquire(~L104-124) — return the token,null, or throw. - Shared test harness with #28; the test that waits the real 2 s timeout.
ELOCKEDpropagation: the hook path is safe (lib/agent/driver.jscatches), but otherstores.*.updatecallers weren't audited.
Note: the full suite at this head hung once in test/hosts/antigravity/hooks.test.js and grok's session end took 1604 ms vs its 1.5 s budget; main passed the same files twice. The extra spawn latency makes timing-bound hook tests worse, so please check CI after the #28 fix.
|
Sentinel 已审核此变更，未发现已核验问题，但发现需要人工确认的风险。 另有 5 个需要人工确认的风险；黄色 advisory 不阻塞合并，建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情： 调用 MCP 工具 由 Sentinel 审核 · 目标提交 db07038a989e |
|
Sentinel 保留了 5 个尚未确认的审核候选。这些黄色条目不阻塞合并；红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时，可用行末 由 Sentinel 审核 · 目标提交 db07038a989e |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Sentinel 已审核此变更，未发现已核验问题，但发现需要人工确认的风险。 另有 5 个需要人工确认的风险；黄色 advisory 不阻塞合并，建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情： 调用 MCP 工具 由 Sentinel 审核 · 目标提交 8015ec5a68ff |
|
Sentinel 保留了 5 个尚未确认的审核候选。这些黄色条目不阻塞合并；红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时，可用行末 由 Sentinel 审核 · 目标提交 8015ec5a68ff |
(cherry picked from commit a0e143e)
|
Sentinel 已审核此变更，并发现 1 个已核验问题。 无法定位到 diff 的已核验问题释放失败仅记录在进程内，其他进程无法回收已放弃的锁🟠 严重程度：中（P2） Finding： 当前为 另有 12 个需要人工确认的风险；黄色 advisory 不阻塞合并，建议开发 agent review advisory 评论并逐条确认。 开发 agent 查询详情： 调用 MCP 工具 由 Sentinel 审核 · 目标提交 b815a6db297b |
|
Sentinel 保留了 12 个尚未确认的审核候选。这些黄色条目不阻塞合并；红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时，可用行末 由 Sentinel 审核 · 目标提交 b815a6db297b |
(cherry picked from commit 0490c99)
|
Sentinel 已审核此变更，并发现 1 个已核验问题。 无法定位到 diff 的已核验问题新增文件末尾缺少
|
|
Sentinel 保留了 16 个尚未确认的审核候选。这些黄色条目不阻塞合并；红色 P0/P1 与其他独立硬门禁不受影响。 单一猜测、缺少变更行证据或纯 checklist 的候选会统一降为 P3。
需要完整证据时，可用行末 由 Sentinel 审核 · 目标提交 41aee5e853d2 |
Summary
Stacked on #28. The additional state-file fix is commit 35ba6cc; its delta is only acquisition/updateState and a new state-lock regression file. Review #28's admission protocol first.
A dead-owner reclaimer can move a fresh state-file lock aside; a third process then enters while its legitimate owner is still mutating. The existing rename/check/restore sequence does not preserve exclusivity.
Serialize every acquisition attempt, including creation, through the shared admission protocol. Keep legacy token files. Retry transient sharing denial within the existing two-second deadline; failed admission never calls the mutator. The strict no-token ELOCKED contract also matches #19's state-safety change.
Evidence
Before: a public updateState A/B/C-process reproducer observes two mutators concurrently and loses one of three increments, before the wait-timeout fallback.
After: every increment survives. New regressions also refuse unguarded mutation, recover dead owners, retry a single sharing denial, preserve state on fatal/setup denial, and bound repeated reclaim failure.
After rebasing onto final fix: keep stale worker-lock recovery exclusive #28 head 71f98f8: combined state/worker/file-store tests 27/27 passed on Windows Node 24. Earlier state tests also passed on Node 22 before that parent refresh.
The existing four-process ×25-update workload is unchanged. All eight hosts pack. No real account/Proxy/Hub calls; final CI runs full Node 22/26.
Final head 35ba6cc passed GitHub CI run 76. External combined PR19–29 validation passed full 757/757 and packing all nine hosts on Linux Node 22 and 26.
Integration with feat: add native MiniMax Code host support #19 overlaps in acquire(): the tested external copy uses this commit's exact acquisition function and retains feat: add native MiniMax Code host support #19's readState/writeState/release changes. No Git merge was performed. That composition and its source hash are recorded; future branch integration must rerun CI.
Final combined validation (immutable source SHA256
1333f0491d643b2fed2188e6c553d0007cf9bdfd88db0d4235c9f50a2e13b610): Linux Node 22/26 full 757/757 each, pack nine hosts; Windows Node 26 scoped 394 passed / 9 existing skips, pack nine hosts. Windows Node 22 scoped 393 passed / 1 after-hook cleanup failure / 1 cancellation / 9 skips; packing was not run after that failure. The MiniMax body saved its draft and settled its candidate, and report delivery later settled, but cleanup exceeded its unchanged 20s wait. Cause remains unclassified; no rerun, assertion or timeout relaxation. All 388 source files kept their hashes after all four cells. This is a local acceptance gap, separate from each PR's successful CI.Merge Danger
Door: two-way
The state-file token format and data schema are unchanged. Failed access reports ELOCKED instead of running an unlocked mutator.
Blast Radius: persistence
Affects settings, candidates, drafts and session updates across hosts. Updated cooperating callers participate in admission; restart older hosts after updating. This PR contains #28 as its parent dependency and should not be merged independently of that review.