Repository navigation
fix(minimald): close the race between the cohort lock and the shim's leaf join (T73) - #2595
Conversation
…leaf (T73) An admit of a blocking host_ip member that ran between injection_leaf and the shim's own leaf join found no shim to move, so the injected process escaped the UDP 443 block. The pid-report thread now waits for the join, checks the cohort under its lock, moves the shim and what it forked into the blocked leaf and records them, or stops the injection.
…race-between-the-cohort-lock-and-the-s
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Approved: every review thread resolved at @norrietaylor resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The post-join check is in the correct place and records each move as the task asks. Three paths still leave an injected process on unblocked UDP 443 while the cohort blocks. Section 5.3 line 150 forbids that, and section 4.1 line 54 requires an inescapable cgroup placement. C1 makes settle_injection refuse when own is not empty after the move passes, as move_in does. C2 refuses the injection when the join wait times out with the shim alive. C3 stops every unmoved process on a refusal.
Ruled at 3f8b45f against gominimal/arch@109541a48c31 by claude-opus-5-5.
When the wait for the shim's join timed out, the check still moved the shim into the UDP 443 blocked leaf. A late join into the box's own leaf then undid the move. While the cohort blocks, the check now refuses the injection, and the daemon stops the shim.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A process injected while a blocking member's admit runs ends in the UDP 443 blocked leaf and is recorded under its box. A shim that does not join its leaf within the wait, while the cohort blocks, is refused and stopped, so it cannot leave the block later. Notes from the agent: Revise round 1. settle_injection now takes a |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The earlier head moved a shim that missed its leaf join, and a late join undid the move. On this head, settle_injection refuses that injection while the cohort blocks, and the daemon stops the shim. That keeps the box's cgroup placement inescapable, as §4.1 line 54 requires, and it keeps the cohort block of §5.3 line 150. No conditions remain. One point is for code review, not architecture. When a reaped shim misses the join, the refusal sends SIGKILL to a pid that the kernel can reuse.
Ruled at 9c1af7f against gominimal/arch@109541a48c31 by claude-opus-5-5.
The check after the shim's join now refuses when the box's own leaf is not empty after the last move pass. A refusal carries what stays in that leaf, and the daemon stops each of those and the shim through a pidfd, so a reused pid never gets the signal. A shim that is gone cannot join later, so what it forked is still moved, not refused.
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The compare closes the fork-storm gap and stops what a refused injection leaves, and that part conforms. But pin_pid turns every pidfd_open error into None, and the caller reads None as a gone shim. A live shim that did not join is then moved, and it can rejoin own outside the UDP 443 block. On a refusal it is not stopped. Section 4.1 line 54 says the placement must be inescapable. Make an unknown result refuse and stop, as C1 and C2 say.
Ruled at d352487 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A process injected into a host-address box while a blocking member's admit runs ends in the UDP 443 blocked leaf, is recorded under the box, and goes home when the block lifts. The daemon log has one debug line per move. When the check cannot move everything (a failed move, a fork storm past the last pass, or a live shim that did not join in time), it refuses the injection. The daemon then stops the shim and each process left in the box's own leaf, so the block never fails open. Notes from the agent: The reviewer asked for COHORT_MOVE_PASSES. That constant is private in crates/minimald/src/net/classifier.rs, and that file is outside the task's layers. So box_runtime.rs has its own INJECTION_MOVE_PASSES = 8, with the same bound. A follow-up can make the classifier constant pub(crate) and use it here. Residual: the shim's pidfd is opened when the pid-report thread receives the pid. A shim that exits and is reaped between its pre_exec send and that open gives no pidfd, and the check then treats it as gone. No signal goes to a pid that is not pinned. The new stop_stranded check runs a real |
pin_pid returns the pidfd_open error. After any other pin error the shim's liveness comes from /proc, and a refusal stops the shim and each stranded process by its pid while the own leaf still holds it.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A process injected into a host-address box while a blocking member's admit runs ends in the UDP 443 blocked leaf and is recorded under the box. When the shim cannot be pinned for a reason other than ESRCH, it is no longer taken as joined. Its liveness comes from /proc, and a refusal stops it, so the block does not fail open. Notes from the agent: Revise round 3. pin_pid now returns io::Result. Only ESRCH counts as gone. After any other pin error, proc_alive reads /proc//stat and treats a zombie as not alive. On a refusal, kill(2) stops the unpinned shim. stop_stranded skips a pid only on ESRCH. For any other pin error it reads own again and sends kill(2) by pid while own still holds the pid. The interleaving test now also checks pin_pid's ESRCH class and proc_alive on a live, zombie and reaped child. cargo clippy -p minimald --all-targets shows no minimald warnings. I did not run just clippy-strict. |
|
The runtime will not revise this pull request again on its own: the diff is +516/-7 (523 lines), past 3x the plan's estimate of 160 lines (est: 60 code, 100 tests, as #2559 reads now), at round 3 (the last trigger was the runtime's review findings). A person decides now. Remove |
There was a problem hiding this comment.
arch-review: CONDITIONS · 1 condition · the full ruling
The compare fixes pin_pid(), and only ESRCH now marks the shim gone. The new /proc fallback breaks the same rule. proc_alive() returns false for any read error, so fd exhaustion makes a live shim look gone. The check then takes the shim as joined, and a later join into the own leaf undoes the move. Section 4.1 line 54 says minimald must make the cgroup placement inescapable. Make the change in C1.
Ruled at 05e9811 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
/revise Round 4 of #2595, on the arch condition at 05e9811 (CHANGES_REQUESTED, C1, thread 4374096638) and one low finding. No round is in flight. The size cap is lifted for this epic, so the diff size is not a reason to stop.
Generated by Claude Code |
…race-between-the-cohort-lock-and-the-s
A read error on /proc/<pid>/stat other than NotFound, such as EMFILE, now leaves the shim alive, so the join check does not take a live shim as joined. The join wait also runs only for a box with host-address enforcement, the only boxes the host_ip cohort can hold.
|
Round 1 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
A process injected into a host-address box while a blocking member's admit runs ends in the cohort's UDP 443 blocked leaf and is recorded under its box. The interleaving test pins this. A live shim whose /proc stat read fails with EMFILE is no longer taken as joined. Notes from the agent: Item 1 (C1): proc_alive now calls a new stat_alive. It returns false only for ErrorKind::NotFound or a Z or X state, and true for every other read error. A stat that cannot be parsed also counts as alive. The new unit test pins Z and X as false, R and S as true, NotFound as false, and EMFILE as true. Item 2: I did not use the note's literal gate, cohort.injection_leaf(box_id).is_some(). That gate reopens the race this task closes. A blocking member's admit can land after the hook reads the cohort and before the shim joins its own leaf. Then settle_injection returns Ok(0), and the shim joins its own leaf after the admit's move, outside the block. The hook now runs only when the box has host_ip_enforcement. join_quic443_cohort admits no box without it, so the cohort never holds such a box and the check cannot change its outcome. For a box outside the cohort, this is the cost the finding names. A tighter gate needs a membership accessor on CohortQuic443 in crates/minimald/src/net/classifier.rs, which is outside this task's layers. Item 3: kill_unpinned in stop_stranded is unchanged, as the note asks. I did not run just clippy-strict. |
|
The runtime will not revise this pull request again on its own: the diff is +557/-7 (564 lines), past 3x the plan's estimate of 160 lines (est: 60 code, 100 tests, as #2559 reads now), at round 1 (the last trigger was a /revise note). A person decides now. Remove |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change conforms to §5.3 line 150 and §4.1 line 54 at this head. A stat read error other than a missing file now leaves an unpinned shim alive. The join check then refuses it while the cohort blocks, and does not take it as joined. The join wait now runs only for a box with host_ip_enforcement. That is the same value that gates cohort membership in join_quic443_cohort, so no cohort member loses the check.
Ruled at 91029c9 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 12 unresolved threads — minimal-arch[bot] crates/minimald/src/box_runtime.rs, minimal-arch[bot] crates/minimald/src/box_runtime.rs, minimal-arch[bot] crates/minimald/src/box_runtime.rs, minimal-foundry[bot] crates/minimald/src/box_runtime.rs, minimal-arch[bot] crates/minimald/src/box_runtime.rs, …. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of 91029c9d83ee withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
There was a problem hiding this comment.
Own review at 91029c9. The race fix pins the shim by pidfd and treats only ESRCH as gone, and falls back to /proc stat for any other pin error, where only NotFound or a Z/X state reads gone. A live shim that did not join is refused while the cohort blocks, and its stranded pids are killed. The join wait runs only for host_ip_enforcement boxes. The pid-reuse window in stop_stranded is accepted by the epic owner (thread 4374096487). No other findings.
Generated by Claude Code
There was a problem hiding this comment.
Babysitter approval at 91029c9: every merge gate passes at this head (arch PASS at 91029c9, zero open threads, every lane aggregator green). Squash title fix(minimald): close the race between the cohort lock and the shim's leaf join (T73) and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
|
The merge-queue run for this PR (run 40287898529, Generated by Claude Code |
T73 — Close the race between the cohort lock and the shim's leaf join
slice: S10
kind: task
covers: BEP-018
size: S
est: 60 code, 100 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: the daemon log names an injected process that the post-join check moved into the blocked leaf
observability: one debug line per post-join move
layers:
- inject:
- crates/minimald/src/box_runtime.rs: after the shim joins its leaf,
Host::commandchecks the cohort again under the lock and moves the shim into the blocked leaf and records it, or refuses the injection- crates/minimald/src/box_runtime/tests.rs: the interleaving test
test: cargo nextest run -p minimald an_admit_between_injection_leaf_and_the_join_ends_in_the_blocked_leaf
produces: none
consumes: none
proof: A process injected while a blocking member's admit runs ends in the blocked leaf and is recorded.
depends on:
- blocked by #2534
Generated by Claude Code
Proof
A process injected into a host_ip box while a blocking member's admit runs now ends in the cohort's UDP 443 blocked leaf and is recorded under its box. The new test shows the shim and its forked program in the blocked leaf after the post-join check. They go home when the blocking box departs. The daemon log has one debug line per moved pid.
Notes from the agent: The task names
Host::command. In the tree that isHost::command_in_session, and the non-test variant carries the change. The shim joins its leaf itself after exec, so the daemon cannot see the join directly. The pid-report thread (report_spawned_pid) now polls the own and blocked leaves for the shim pid, for up to 5 s, until it is there or/proc/<pid>is gone. Thensettle_injectionruns under the cohort lock. A shim that got the blocked leaf is recorded, as before, but after the join. Before this change it was recorded at once, so a lift between the record and the join left it blocked. A shim that got its own leaf moves, with everything in that leaf, into the blocked leaf when the cohort now blocks for the box, and each move is recorded. If a move fails with an error other than ESRCH, the injection is refused: the daemon SIGKILLs the failed pid and the shim, and logs a warn. When no box-leaf is set, nothing changes. Every injection into a box with a leaf now starts this short poll thread, not only the injections that got the blocked leaf. The poll is a bounded wait. The real command path iscfg(not(test)). The test drivesinjection_leaf,wait_for_injection_joinandsettle_injectionagainstStandinMoves. It does not spawn a real shim. I also rancargo nextest run -p minimald quic443(6 passed). I did not run the whole crate suite orjust clippy-strict.Tests
cargo nextest run -p minimald an_admit_between_injection_leaf_and_the_join_ends_in_the_blocked_leafVerify
min task run agent-verifyexited 101 under rustc 1.98.1, which the repository does not pin (1.99.0 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Base
The base branch moved while the coder worked; it was merged in before the tests ran.
Closes #2559