Repository navigation
fix(destroy): freeze a group whose member ends between two reads before destroy gives up on it - #333
Conversation
…re destroy gives up on it stopProgramGroup sent SIGSTOP only after a judgement of gone. A member of a fork chain that a scan shows can end before its autogroup read, so the judgement reads unknown, and stopProgramGroup returned with no freeze and no kill. A witness that ended before its read again made the loop judge again without a freeze. The wait after it then read the unseen chain ended, and destroy recorded destroyed while the chain ran. CI of #331 hit this in "gc keeps an exited session while a fork chain that the program left runs". Now every judgement that gets no kill and is not unproven sends SIGSTOP and judges the frozen group again: gone as before, unknown, and owned with a witness that ended. A stopped member cannot end, so the next judgement reads its autogroup and the witness, and the group gets the kill. The loop keeps its 5 attempts, and a frozen group that gets no kill still gets SIGCONT in the finally block. New unit tests: a member that ends before its autogroup read, and a witness that ends before its read again, both get SIGSTOP and then SIGKILL (both fail on main). A member whose autogroup /proc refuses now gets 5 SIGSTOP and a SIGCONT, and still no kill. facts.mjs reads the new loop. The model is unchanged: it already assumes that the freeze brings the witness into view.
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: fd2bb66074
ℹ️ 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".
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…group Codex round 1 on #333: a group that job control stopped, whose autogroup or boot id /proc refuses, reads unknown at every judgement. The new freeze sent it SIGSTOP and then SIGCONT, which resumed a program that its user stopped. The unknown verdict now says whether a member without an autogroup still runs. A stopped member cannot end between two reads, so /proc refused its autogroup, and stopProgramGroup sends no signal then. A refused boot id is unknown without a running member too, because no later judgement can prove the group without it. A running member without an autogroup still gets the freeze, once: its stopped member reads the same way, and the group gets SIGCONT. New unit test: a stopped group whose member shows no autogroup gets no signal (fails on the previous head).
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…readable-member # Conflicts: # .agents/skills/verify-session-lifecycle/coverage.json
…readable-member # Conflicts: # .agents/skills/verify-session-lifecycle/coverage.json
…the group The merge-group CI of #333 failed "gets no kill and reads alive when the autogroup of its member names no autogroup": the real group got two SIGSTOPs before its SIGCONT. A SIGSTOP takes effect only when the kernel next runs the member. A read of /proc/<pid>/stat right after kill() showed the member not yet stopped in 1,984 of 2,000 tries here, and the stop took about 30 us (max 405 us) on an idle machine. A full /proc scan usually gives it that time, but on a loaded runner the next judgement read the member running and froze the group again. The code is safe either way: a member that still runs gets another SIGSTOP, within the 5 judgements, and the group still gets its SIGCONT. stopProgramGroup now waits STOP_SETTLE_MS (1 ms) after each SIGSTOP, so the next judgement mostly reads the frozen members stopped and fewer of the 5 judgements go to a stop that has not taken effect. On one CPU shared with four busy loops, the test saw a second SIGSTOP in 6 of 24 runs without the wait and in 1 of 24 with it. The test now accepts 1 to 5 SIGSTOPs before the SIGCONT. facts.mjs reads the wait as part of the freeze.
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
The CI of #331 (head 5636b14, job 126789316550) failed "gc keeps an exited session while a fork chain that the program left runs" in all 3 attempts. Two attempts timed out with
HOST_TIMEOUT. In the third,destroyreturned ok and recordeddestroyed, but the fork chain still ran 1 s later. In the same job, "destroy ends a fork chain that no scan of /proc shows" failed once before its retry passed. A plaindestroythat reports success while the program's processes run is broken core behavior.Root cause:
stopProgramGroupsent SIGSTOP only after a judgement ofgone. A fork-chain member lives about 2 ms, and a scan can show it while it ends:readAutogroupgivesnull, so the judgement readsunknown, andstopProgramGroupreturned with no freeze and no kill.owned, but the witness ends before it is read again. The loop then judged again without a freeze, and five such judgements in a row ended with no kill.The wait after it then read the group ended: the probe found the group, but no scan showed a member (
groupHasLiveProcessgivesnull), sodestroyrecordeddestroyed. The model assumes that the freeze always brings the witness into view, and the code did not freeze in these two cases.What changes
stopProgramGroupnow freezes the group after every judgement that gets no kill, exceptunproven:goneas before,unknownwhile a member that shows no autogroup still runs, andownedwhose witness ended before it was read again. Theunknownverdict carriesrunningfor that. A stopped member cannot end, so the next judgement reads its autogroup and its witness, and the group gets the kill. The loop keeps its 5 attempts.Safety argument
unknownor after a failed witness read targets only a group id that agonefreeze already targets today. Such a judgement saw a member with the program's session id, or one that carries the token, so the group is at least as likely the program's as one that readsgone.unproven(another boot id, or members with another session id or autogroup) still gets no SIGSTOP. It returns before the freeze, as before./proc/selfdoes not name this process (another pid namespace), as before.finallyblock still sends SIGCONT to every frozen group that got no kill, also when a scan throws.unknownwhose members without an autogroup are all stopped sends no signal (Codex round 1). A refused boot id readsunknownwithrunningfalse too, so it sends no signal, as before./procrefuses gets one SIGSTOP and one SIGCONT, and still no kill: after the SIGSTOP its member is stopped and reads the same way.A SIGSTOP takes effect only when the kernel next runs the member: about 30 us on an idle machine (max 405 us here), longer under load.
stopProgramGroupwaitsSTOP_SETTLE_MS(1 ms, a synchronousAtomics.wait) after each SIGSTOP, so the next judgement mostly reads the frozen members stopped. A member that still runs then gets another SIGSTOP, within the 5 judgements. (The merge-group CI of 738daee's parent saw a real group get a second SIGSTOP. With one CPU shared with four busy loops, the second SIGSTOP came in 6 of 24 runs without the wait and in 1 of 24 with it.)No CLI JSON or exit codes change.
Validation
Red first, in
test/unit/host/programGroup.test.tswith a faked/proc:facts.mjsreads the new loop statement by statement:gonefreezes, turnsProvesGroupOwnershipandFreezesUnseenGroupFALSE.unknownturns both FALSE.FreezesUnseenGroupFALSE."gets no kill and reads alive when the autogroup of its member names no autogroup / cannot be read" accepts 1 to 5 SIGSTOPs before the SIGCONT, because the stop is asynchronous.
Real CLI, the scenario of the failed test (an exited session whose fork chain runs, then
gc, then a plaindestroy). I used a debug copy of the code that is not committed here. It adds a 5 ms delay inside the destroy CLI between the stat read and the autogroup read of each process (DBG_AUTOGROUP_DELAY_MS), or before the witness read (DBG_WITNESS_DELAY_MS), as a preemption on a loaded runner does. Results, 20 runs each:destroyrecordeddestroyedwhile the chain ranunknowngoneand froze)On main, every false
destroyedfollows a judgement ofunknownwith no freeze, and the wait's probe then found the group with no member in view. This matches CI attempt 3. Without the delays, under a CPU load of 2 cores and 10 busy loops, the traces on main showedownedjudgements whose witness had ended by its read (2 in 16 runs).tsc --noEmit,oxlint,oxfmt --check,check.sh --drift-only,test/unit(2,070 tests), thelifecyclefork-chain and thread integration tests,non-reaping-init, andother-user-group(with a setcap'dsetpriv) pass.Follow-up
A second PR changes the wait: when the probe finds the group but no scan shows a member, the group reads alive, as gc's
GcProbesUnseenGroupalready counts it. Then any future miss gives an honestHOST_TIMEOUTinstead of a falsedestroyed. That PR changes the model (FreezesUnseenGroup, R11DestroyedProgramStopped) and lands after this one.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh