fix(kernel): guard gc_sweep running_tasks removal with task_id (TOCTOU)#6317
Merged
Conversation
gc_sweep collected dead/finished (agent, session) keys and then did a bare running_tasks.remove(&key). A faster successor turn that swapped a fresh, live RunningTask into the same key between the collect and the remove was dropped and had its in-flight AbortHandle fired — so the 5-minute GC could abort a user's active turn. Snapshot the observed task_id alongside each key and remove via remove_if(&key, |_, v| v.task_id == observed) — the same #3445 stale-entry guard the streaming-cleanup path already uses in messaging.rs. A dead agent's successor, if one ever raced in, self-ejects via its own post-insert recheck, so nothing leaks. total_removed now counts actual removals rather than collected candidates. Adds a stress regression test that is deterministically green with the fix (the sweep can never remove the live successor's entry) and turns red probabilistically without it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The kernel's periodic GC sweep (
gc_sweep, runs every 5 minutes) reapsrunning_tasksentries for dead agents and finished turns.It collected the matching
(agent, session)keys into aVec, then looped doing a barerunning_tasks.remove(&key).Between the collect and the remove, a faster successor turn on the same
(agent, session)can swap a fresh, liveRunningTaskinto that key — the per-turn insert is not serialized against the sweep.The bare
remove(&key)then dropped that successor's entry and fired its in-flightAbortHandle, so the GC could abort a user's live turn.The streaming-cleanup path already protects against exactly this (the
#3445stale-entry guard:running_tasks.remove_if(&key, |_, v| v.task_id == turn_task_id));gc_sweepwas the one removal site that did not.Changes
crates/librefang-kernel/src/kernel/accessors.rs—gc_sweepsnapshots each candidate'stask_idalongside its key and removes viaremove_if(&(agent, session), |_, v| v.task_id == observed).An entry whose
task_idchanged (a successor swapped in) is left untouched; a dead agent's successor self-ejects via its own post-insert recheck, so nothing leaks.total_removednow counts actual removals instead of collected candidates.Verification
cargo clippy -p librefang-kernel --lib -- -D warnings— clean.cargo test -p librefang-kernel --lib gc_sweep— the existinggc_sweep_aborts_orphaned_running_task_5142([audit] Concurrency hazards (follow-ups to #5125-5126) #5142 abort-on-reap) andgc_sweep_preserves_agent_msg_lock_with_inflight_holderstill pass, plus the newgc_sweep_does_not_abort_live_successor_turn.cargo test -p librefang-kernel --lib kill_agent_dispatch_insert_race— the concurrent kill/dispatch race test still passes (no regression to the dead-agent reap path).cargo fmt -p librefang-kernel --check— clean.The new test is a stress test (the race is internal to one
gc_sweepcall, so it cannot be hit deterministically from a single test thread): it is deterministically green with the fix — the sweep can never remove the live successor's entry — and turns red probabilistically without it.(Run in the repo's
Dockerfile.rust-devimage against an isolated target volume, per the project's no-local-cargo rule.)Related, observed but deliberately not in this PR
In the same race family, the streaming-cleanup path (
messaging.rs, the!is_forkblocks around the Ok/Err arms) removessession_interruptsunconditionally while guarding the adjacentrunning_tasksremoval with thetask_idcheck.A faster successor's interrupt registration can therefore be wiped by its predecessor's cleanup, degrading a later
stop_session_runfrom cooperativecancel()to a hard abort.The fix there is an inline-closure reorder (remove
running_tasksfirst, gate thesession_interrupts.removeon it returningSome).That call site is inside a spawned closure and cannot be exercised by a deterministic unit test, so it warrants its own change with a race/loom-style test rather than being bundled into this one — flagged here so it is not lost.