Skip to content

[audit] Concurrency hazards (follow-ups to #5125-5126) #5142

Description

@houko

Summary

P1/P2 follow-ups from the concurrency audit. P0 siblings: #5125 (agent_send transitive cycle), #5126 (channel_send mirror deadlock).

Sub-items

  • kill_agent leaks in-flight streaming tasks and never aborts themcrates/librefang-kernel/src/kernel/agent_runtime.rs:542-619 (vs. suspend_agent at :210, which does call stop_agent_run). kill_agent_with_purge removes registry/scheduler entries but never invokes stop_agent_run(agent_id). The matching GC cleanup at crates/librefang-kernel/src/kernel/accessors.rs:1065-1071 removes running_tasks entries for dead agents but the loop never calls e.value().abort.abort() — abort handles are dropped, not fired. Killed agent's in-flight LLM stream continues burning tokens until the provider returns. Fix: call stop_agent_run(agent_id) before scheduler/registry removal; in GC cleanup, fire abort() on the handle before removing the map entry.

  • std::env::set_var inside spawn_blocking is UB on Rust 1.74+crates/librefang-api/src/secrets_env.rs:98-106, crates/librefang-api/src/routes/skills.rs:2841-2845,3234-3238 — comments claim // SAFETY: spawn_blocking serialises the env mutation but spawn_blocking dispatches across the blocking thread pool and does NOT serialize. Two concurrent route invocations (channel-bridge reload + secret PATCH) can each call unsafe { std::env::set_var(...) } on different blocking threads — Rust 1.74's std::env::set_var docs declare this undefined behavior in the presence of any concurrent reader. Fix: serialize through a tokio::sync::Mutex<()> held across the set_var, or move env mutations to startup-only and use a process-internal env-var registry for runtime changes.

  • AppState.bridge_manager reload drops old BridgeManager without .stop() under loadcrates/librefang-api/src/channel_bridge.rs:4035-4039Arc::try_unwrap(old) only succeeds when no other strong references exist; any in-flight routes/agents.rs:6347 (state.bridge_manager.load_full() held across bm.push_message(...).await) keeps the Arc alive, so try_unwrap returns Err, the if let Ok(Some(...)) arm is skipped, and the old BridgeManager's tokio tasks are never stopped — they leak until Arc strong count hits 1 (potentially never on a busy channel). Fix: always call old.stop() regardless of try_unwrap's outcome; stop() should be safe to call on the still-shared Arc (it abort()s tasks, doesn't move).

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/kernelCore kernel (scheduling, RBAC, workflows)area/securitySecurity systems and auditingbugSomething isn't workinghas-prA pull request has been linked to this issueseverity/mediumBug or limitation with workaround / partial impact

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions