Skip to content

fix(kernel): guard gc_sweep running_tasks removal with task_id (TOCTOU)#6317

Merged
houko merged 3 commits into
mainfrom
fix-gc-sweep-toctou
Jun 25, 2026
Merged

fix(kernel): guard gc_sweep running_tasks removal with task_id (TOCTOU)#6317
houko merged 3 commits into
mainfrom
fix-gc-sweep-toctou

Conversation

@houko

@houko houko commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

The kernel's periodic GC sweep (gc_sweep, runs every 5 minutes) reaps running_tasks entries for dead agents and finished turns.
It collected the matching (agent, session) keys into a Vec, then looped doing a bare running_tasks.remove(&key).

Between the collect and the remove, a faster successor turn on the same (agent, session) can swap a fresh, live RunningTask into 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-flight AbortHandle, so the GC could abort a user's live turn.

The streaming-cleanup path already protects against exactly this (the #3445 stale-entry guard: running_tasks.remove_if(&key, |_, v| v.task_id == turn_task_id)); gc_sweep was the one removal site that did not.

Changes

  • crates/librefang-kernel/src/kernel/accessors.rsgc_sweep snapshots each candidate's task_id alongside its key and removes via remove_if(&(agent, session), |_, v| v.task_id == observed).
    An entry whose task_id changed (a successor swapped in) is left untouched; a dead agent's successor self-ejects via its own post-insert recheck, so nothing leaks.
    total_removed now 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 existing gc_sweep_aborts_orphaned_running_task_5142 ([audit] Concurrency hazards (follow-ups to #5125-5126) #5142 abort-on-reap) and gc_sweep_preserves_agent_msg_lock_with_inflight_holder still pass, plus the new gc_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_sweep call, 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-dev image 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_fork blocks around the Ok/Err arms) removes session_interrupts unconditionally while guarding the adjacent running_tasks removal with the task_id check.
A faster successor's interrupt registration can therefore be wiped by its predecessor's cleanup, degrading a later stop_session_run from cooperative cancel() to a hard abort.
The fix there is an inline-closure reorder (remove running_tasks first, gate the session_interrupts.remove on it returning Some).
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.

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.
@github-actions github-actions Bot added size/M 50-249 lines changed area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) labels Jun 25, 2026
@houko
houko merged commit daed0e6 into main Jun 25, 2026
52 of 54 checks passed
@houko
houko deleted the fix-gc-sweep-toctou branch June 25, 2026 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) size/M 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants