fix: never hold the SQLite write lock across slow sandbox work (multi-agent decompile contention)

[branover]

What & why

Multiple agents decompiling functions in parallel were hitting repeated sqlite3.OperationalError: database is locked. The user's intuition was right and worth stating precisely: individual writes are milliseconds, so simultaneous writes should be rare — the contention wasn't two writes racing. It was one writer holding the single SQLite write lock across slow work (a Docker decompile, a probe, a network op) and starving every other writer until the 30s busy_timeout elapsed. Same failure class as #283 (recon/ingest holding the lock across their Docker phase), but in the paths #283 didn't cover.

Root cause, confirmed dynamically (not just read off the decompiler): an LLM task runs every agent-loop tool on one long-lived session. A decompile_function that PROMOTES a function node + calls edges left that write flushed-but-uncommitted (get_or_create_node/add_edge flush, don't commit). That write then stayed pinned across the next function's decompile and the intervening model round-trip. A competing-writer probe measured the hold at the full duration of the next decompile (0.00s in the per-call MCP path, ~3s = the whole "docker" window in the task path).

Then I audited every task handler, agent-loop tool, and sandbox/network call site for the same shape and fixed them together.

The fix

One discipline: commit any pending write BEFORE slow sandbox/network/subprocess work, so the lock is only ever held for the write itself. New helper release_write_lock(session) (a plain session.commit(), same semantics as record_observation's durable checkpoint / execute_recon's existing pre-probe commit).

Structural (cover the bulk):

• run_task_sync — checkpoint the running-status write (mark_running) before _dispatch, so every task type's first slow op starts with the lock free. (Also makes running visible to the UI immediately.)
• execute_llm_task agent loop — commit before the loop and after each tool call, so a graph-mutating tool's write is never held across the next tool's sandbox op or the LLM round-trip. This is the reported decompile contention.

Targeted (mid-handler writes made after the mark_running checkpoint):

• pipeline._maybe_enrich_ghidra — release before enrich_target's Ghidra decompile (recon leftovers were held across it).
• fuzzing — release before the campaign probe (task.parent_finding_id) and before the per-crash triage LLM call.
• poc — release before verify_poc executes the target.
• build — release before the compile probe; commit the fetch audit (durable=True) before the network fetch probe.
• yara.scan_target — release before the scan (sweep_project held the prior artifact's promoted nodes across the next scan).
• egress family (surfaces._egress_gate / _run_socket_probe / run_web_recon, remote.run_remote) — the allowed-egress audit before each network probe is now durable=True: audit-before-action, the same rationale the deny path already commits under, and it releases the lock before the probe.

Handlers already correct and left alone: execute_recon (already commits before its probe, #283), the batch loops (_record_progress/per-item commit), rehost (boot correctly ordered before all writes), register_service_target (write-then-return).

Verification

• New deterministic regression tests (tests/test_db_lock_contention.py):
  • the agent loop commits a tool's graph write between tool calls (a fresh session sees it committed and can write mid-loop);
  • run_task_sync commits the running status before _dispatch;
  • release_write_lock hands off the lock.
  • Non-vacuity checked: with the helper neutered, both structural tests fail — the agent-loop one with the exact database is locked error, reproducing the reported bug.
• Full offline suite green: 1710 passed, 6 skipped, 14 deselected.
• No schema change → no migration. No UI-behavior change → no ux-contract entry.

🤖 Generated with Claude Code

[branover]

Independent merge-gate review — PR #288

Verdict: APPROVE (no blocking correctness or security issues; low-severity notes below, none required before merge)

Independent reviewer (did not write this code). Ran both /code-review (high) and /security-review on git diff main...HEAD, plus a manual audit of every task handler / agent-loop tool / sandbox+network call site for the "uncommitted write held across slow work" shape.

Correctness — the durability contract holds

The critical question: does any release_write_lock / durable=True prematurely commit a synthesized finding so it survives a task failure that should roll it back? No.

• execute_llm_task: the pre-loop commit (llm_tasks.py:275) and the per-tool-call commit (llm_tasks.py:279) are both before the synthesis phase. The LLM's synthesized findings are persisted after the loop returns (llm_tasks.py:318-351) and are committed only by the outer session_scope on success — so a synthesis failure still rolls them back via run_task_sync's session.rollback(). What the new commits make durable is exactly the category the contract says to keep: static-core grounded findings, decompile observations/promoted nodes, and (if the agent calls solve_reaching_input) grounded solver findings. Consistent — arguably it even closes a latent gap where static-core findings could previously be lost on failure if no tool/observation had committed.
• execute_poc: release_write_lock (poc.py:331) precedes verify_poc; the poc finding (poc.py:335) is created after and is not committed early.
• run_task_sync: the mark_running checkpoint (worker.py:196) covers the first slow op of every task type; verified recon/unpack/ghidra_enrich[_batch]/agent_delegate/analyze_target are all covered (each subsequent slow op is preceded by a _record_progress/execute_recon/per-item commit).

Security — egress audit change does not weaken anything

Making the allowed-path record_egress(..., durable=True) (surfaces _egress_gate/_run_socket_probe/run_web_recon, remote.run_remote, build fetch) only changes when the allowed audit row commits, not whether egress is permitted:

• assert_allows_egress(dest, scope, current_policy()) runs before record_egress in every path; the loopback/private-only scope (local_network_scope/local_tcp_scope/remote_scope) and the deny path are untouched. The deny path was already durable=True, so this is symmetric.
• It strengthens audit completeness (audit-before-action now survives a probe crash / failed task, matching the deny rationale). No secret reaches the audit row (dest/tool/rationale only; remote credentials still travel out-of-band via HG_CHANNEL_SECRET).
• /security-review: no HIGH/MEDIUM findings. No new injection / deserialization / path / secret-logging surface; release_write_lock is a plain session.commit(). The change is security-neutral-to-positive.

Test quality — verified non-vacuous dynamically

I neutered release_write_lock to a no-op and re-ran tests/test_db_lock_contention.py: all three new tests fail, and the two structural ones fail with the exact sqlite3.OperationalError: database is locked — test_agent_loop_checkpoints_graph_write_between_tool_calls reproduces the reported multi-agent contention. Reverted; suite green (6 passed in 0.38s; focused touched-module run 66 passed).

Findings (all LOW / non-blocking)

1. LOW — audit-scope gap: the fuzz-campaign reaper is not covered. engine/fuzz/campaigns.py reap_campaign → _ingest_artifacts persists a fuzz_crash finding (campaigns.py:754) and, on the worker path (allow_replay_backfill=True), holds it across a target-executing replay-verify (run_json_probe, campaigns.py:1312) — and backfill_pending_representatives (campaigns.py:626) does the same. This is exactly the "uncommitted write held across a sandbox op" shape the PR set out to eliminate ("audited every ... sandbox/network call site"). It is pre-existing (not a regression) and lower-impact (a single background reaper thread, each representative replayed at most once), so it's a follow-up, not a merge blocker — but worth a release_write_lock/flush before the replay for completeness.
2. LOW — confirm intent: fuzzing.py:333 commits a persisted crash finding before triage. release_write_lock(session) sits inside the crash loop after persist_finding (fuzzing.py:324), so a fuzz_crash finding now becomes durable before the _triage LLM round-trip and survives a triage-step failure that previously rolled back with the task. This is defensible (a fuzz crash is grounded, same class as static-core findings kept on failure) and is the sole place a new commit follows a finding-persist — flagging only to confirm it's deliberate.
3. NIT — import-style inconsistency. release_write_lock is imported function-locally in poc.py/fuzzing.py/pipeline.py/yara.py/build.py but top-level in worker.py/llm_tasks.py. Harmless; hoisting the five local imports to module scope would read more uniformly.
4. NIT — build.py:577 commits inside for dest in sorted(scope.allow), so record_egress(durable=True) runs one commit per allowlist entry. A single commit after the loop would do; negligible (allowlists are tiny).

Nothing here blocks merge. Posting inline comments on (2) and (4).

[branover]

Addressed the review

Thanks for the thorough pass — verdict noted (APPROVE, no blocking issues). Follow-up commit addresses the findings:

• LOW HexGraph MVP: typed-graph VR workbench (ingest → finding → graph → spawn) #1 — fuzz-campaign reaper audit gap (now fixed, not deferred): you're right that verify_artifact holds a just-persisted fuzz_crash finding + artifact across the target-executing replay (the _ingest_crashes backfill and backfill_pending_representatives reaper paths). Since the ask was to fix every instance of this antipattern at once, I fixed it here rather than as a follow-up: release_write_lock(session) right after assert_allows_execution() in verify_artifact (campaigns.py), which is the single chokepoint every binary replay-verify caller routes through. The network branch above it already releases via its bounded-egress audit (the durable=True change), so this one commit covers all binary replay paths (verify_reproducer fallback and the poc_probe run). The grounded crash finding is meant to persist regardless, so committing it before the replay matches intent.

• NIT (inline, fuzzing.py) — commit-before-triage intent: deliberate, and I expanded the comment to say so — the crash finding is grounded (a real libFuzzer crash), so like the static-core findings it is meant to survive a best-effort triage failure; triage only annotates it.

• NIT (inline, build.py) — per-entry commit in the allowlist loop: hoisted the commit out of the loop — the egress rows are now recorded durable=False and committed once via release_write_lock(fetch_session) after the loop.

• NIT — local vs top-level import of release_write_lock: kept function-local in the engine modules (poc/fuzzing/pipeline/yara/build/campaigns) — it's the prevailing local-import style in those hot analysis paths and sidesteps any import-order sensitivity; top-level in worker/llm_tasks where the module already imports from db.session. Happy to unify if you'd prefer one way.

Re-verified: full offline suite green.