Mark critical audit findings resolved

2026-08-17 17:49:46 +02:00 · 2026-08-17 17:49:46 +02:00 · 08a97540ec
parent 3ad431957c
commit 08a97540ec
1 changed files with 510 additions and 0 deletions
--- a/doc/concurrency-resource-audit.md
+++ b/doc/concurrency-resource-audit.md
@ -0,0 +1,510 @@
+# Concurrency and Resource Leak Audit
+
+## Scope
+
+This audit covers goroutine, memory, file descriptor, socket, subprocess, timer, channel, context, and lifecycle management across:
+
+- `cmd/olliesrv`
+- `cmd/toolsrv`
+- agent and session runtimes
+- `toolsrv` client/process management
+- `lib9p`
+- `virtfs`
+- LSP process management
+
+The findings account for Ollie's concurrent architecture: 9P requests execute concurrently, blocking files intentionally wait for events, agents serialize turns while tools may run in parallel, and toolsrv owns external processes.
+
+## Executive summary
+
+The highest-risk area is **9P connection shutdown**. Response writers can exit while request handlers continue sending. Blocking reads are not consistently tied to connection lifetime. Open fids are not closed on clunk, rename, or disconnect in all implementations. Shutdown can wait for handlers before closing the connection that would unblock them.
+
+The second major area is **subprocess ownership**. Child processes are killed without being reaped, abnormal startup can leave socket paths behind, dismissed processes can become invisible to cleanup, and background startup can wait forever for a signal that some execution paths never send.
+
+The third area is **agent and session lifecycle ownership**. Active actions, toolsrv connections, feed consumers, and caches survive failure or replacement paths. Several in-memory structures are unbounded for long-lived agents.
+
+The existing focused tests, race tests, and vet checks pass. They do not exercise the adversarial lifecycle paths listed here.
+
+## Severity
+
+- **Critical:** can permanently block shutdown or leak resources under ordinary client/process failure.
+- **High:** confirmed resource leak or severe lifecycle failure.
+- **Medium:** bounded or conditional leak, unbounded growth, or significant concurrency risk.
+- **Low:** cleanup weakness or avoidable retention with limited impact.
+
+## Resolved critical findings
+
+The following critical findings were verified fixed in these commits:
+
+- **C1. 9P response channels deadlock after writer failure** — fixed in `c652c56`.
+- **C2. Blocking reads are not tied to connection lifetime** — fixed in `6131fad` and `2352f24`; feed-consumer runtime testing remains pending.
+- **C3. Process shutdown kills children without reaping them** — fixed in `4ac3b21`.
+- **C4. Background process startup can block forever** — fixed in `3ad4319`.
+
+The original critical finding details are retained in git history. The remaining audit work starts at the high-severity findings below.
+
+## High findings
+
+### H1. Open fids are not closed on all lifecycle paths
+
+**Evidence:** `cmd/olliesrv/server.go:188-208,680-710`; `cmd/toolsrv/p9.go:50-60,539-553`.
+
+The toolsrv closes entries during `Tclunk`, but connection teardown does not close all remaining fids. The Ollie server does not consistently call `fs.File.Close()` on clunk, connection teardown, or stale fid removal after rename.
+
+**Impact:** blocking and streaming files retain subscriptions, goroutines, contexts, and other resources.
+
+**Remediation:** centralize fid cleanup. Remove fids under the connection lock, close entries outside the lock, and invoke cleanup on clunk, rename deletion, EOF, write failure, and shutdown.
+
+### H2. Agent action remains active after `ListTools()` failure
+
+**Evidence:** `cmd/olliesrv/internal/agent/turn.go:117-131`.
+
+`executeTurn` installs `currentAction` before fetching tools. If `ListTools()` fails, it sets the agent state to idle but does not clear or cancel the action.
+
+**Impact:** `IsRunning()` remains true; future prompts may queue; interrupt and compaction target stale state; the action context remains reachable.
+
+**Remediation:** use one cleanup path for every turn exit. Cancel and clear `currentAction` before returning.
+
+### H3. Agent shutdown does not cancel active work
+
+**Evidence:** `cmd/olliesrv/internal/agent/agent.go:535-540`.
+
+`Agent.Close()` closes the current toolsrv without first cancelling an active turn.
+
+**Impact:** backend streaming, tool execution, compaction, or retry work can remain blocked while its dependency is closed.
+
+**Remediation:** cancel active work, wait for the turn to exit, then close toolsrv.
+
+### H4. Feed consumers leak goroutines and connections
+
+**Evidence:** `cmd/olliesrv/internal/agent/agent.go:692-723`; `cmd/olliesrv/internal/session/session.go:277-299`.
+
+`ConsumeFeed` starts a cancellation goroutine that closes the filesystem, but early `Open` or `ReadAll` returns leave that goroutine running. The filesystem and underlying connection are not closed on every return path. Feed lifetime is tied to the session rather than the agent.
+
+**Impact:** leaked goroutines, 9P attachments, sockets, and stale consumers after agent removal.
+
+**Remediation:** use deferred cleanup for filesystem and connection; give each feed consumer an agent-scoped context and cancellation path.
+
+### H5. Runtime and toolsrv replacement leaks old connections
+
+**Evidence:** `cmd/olliesrv/internal/agent/agent.go:351-357,434-455,644-647`.
+
+`SetRuntime`, `SetToolServer`, and profile switching overwrite active runtime or toolsrv references without closing old connections. Backend construction failure after toolsrv creation also abandons the dispatcher.
+
+**Impact:** repeated profile switching accumulates Unix sockets, RPC goroutines, pending requests, registries, and process state.
+
+**Remediation:** define ownership explicitly. Close the previous runtime before replacement and clean up partially constructed runtimes on every error path.
+
+### H6. Session shutdown leaves `toolsConn` open
+
+**Evidence:** `cmd/olliesrv/internal/session/session.go:302-312,364-367`; `cmd/olliesrv/internal/session/registry.go:125-133,194-199`.
+
+Normal session close does not close `s.toolsConn`; it is closed in `Pause()` but not all shutdown paths pass through `Pause()`.
+
+**Impact:** socket and file-descriptor leaks per terminated session.
+
+**Remediation:** close `toolsConn` in the single session shutdown path and make the operation idempotent.
+
+### H7. Failed toolsrv setup leaks infrastructure
+
+**Evidence:** `cmd/olliesrv/internal/session/setup.go:121-148`; `cmd/olliesrv/internal/session/registry.go:388-428`; `cmd/olliesrv/internal/session/persist.go:240-255`.
+
+Setup creates a process, keeper, and connection. Later backend or agent construction can fail without closing already-created infrastructure. Restore loops can repeat the leak for every failed agent.
+
+**Impact:** leaked toolsrv processes, keepers, sockets, and connections.
+
+**Remediation:** install cleanup immediately after every resource acquisition and transfer ownership only after full construction succeeds.
+
+### H8. Remote SSH stderr is not drained after startup
+
+**Evidence:** `cmd/olliesrv/internal/toolclient/spawn.go:208-245`.
+
+The stderr scanner exits after detecting `listening on`; no goroutine drains stderr afterward.
+
+**Impact:** the SSH process can block when the pipe fills, causing remote toolsrv hangs.
+
+**Remediation:** drain stderr for the process lifetime into a bounded sink, or redirect it safely.
+
+### H9. Dismissed toolsrv processes can become orphaned
+
+**Evidence:** `cmd/toolsrv/internal/server/proc.go:454-483`.
+
+`DismissProc` removes a process from `st.procs` without stopping it.
+
+**Impact:** a running process becomes invisible to `KillAll`, GC, inspection, and shutdown cleanup.
+
+**Remediation:** reject dismissal while running or cancel/kill before removing the registry entry.
+
+### H10. `virtfs.Tree.Stat` leaks fallback-opened files
+
+**Evidence:** `virtfs/tree.go:90-99`.
+
+When no `statFn` exists, `Stat` calls `openFn` and returns `e.Stat()` without closing `e`.
+
+**Impact:** one resource leak per `Stat` call for resource-owning open functions.
+
+**Remediation:** close the opened file after statting it.
+
+### H11. `virtfs` `Rdwr` state is not synchronized
+
+**Evidence:** `virtfs/builder.go:258-304`.
+
+Mutable fields such as `result`, `reqCtx`, and `reqCancel` are accessed by concurrent read, write, and close operations without synchronization.
+
+**Impact:** data races, wrong cancellation, overwritten results, and work continuing after close.
+
+**Remediation:** serialize operations per `Rdwr` fid or add a mutex and explicit lifecycle state. Reject overlapping writes if the API is single-flight.
+
+### H12. Process-limit enforcement is not atomic
+
+**Evidence:** `cmd/toolsrv/internal/server/proc.go:226-292`.
+
+The process count is checked, setup occurs, and insertion happens later. Concurrent callers can all pass the limit check.
+
+**Impact:** the configured process limit can be exceeded, amplifying subprocess, memory, goroutine, and FD pressure.
+
+**Remediation:** reserve a slot before setup and release it on every failure and completion path.
+
+### H13. Completed background processes retain output indefinitely
+
+**Evidence:** `cmd/toolsrv/internal/server/proc.go:485-516`.
+
+Process GC removes exited processes only after output has been read. An abandoned process ID retains output and metadata indefinitely.
+
+**Impact:** unbounded memory retention and eventual exhaustion of the process limit.
+
+**Remediation:** add an absolute retention TTL independent of `LastRead`, plus output-size limits.
+
+## Medium and lower findings
+
+### M1. Workflow and prompt goroutines are unbounded
+
+**Evidence:** `cmd/olliesrv/internal/fs/spec.go:43-68,354-357,433-439,610-632`.
+
+Every workflow or prompt write can launch a goroutine. There is no admission control, tracking, deduplication, or per-session limit.
+
+**Impact:** bursts create large numbers of goroutines waiting on agent serialization, backend I/O, or subprocesses.
+
+**Remediation:** use bounded per-agent or per-session queues, apply backpressure, and track asynchronous work under session cancellation.
+
+### M2. Parallel tool dispatch is unbounded
+
+**Evidence:** `cmd/olliesrv/internal/agent/loop.go:469-480`.
+
+One goroutine is created per unique tool call. A model response can generate an arbitrarily large batch.
+
+**Impact:** goroutine spikes, excessive RPCs, subprocess amplification, and large temporary result buffers.
+
+**Remediation:** use a semaphore or bounded worker pool while preserving tool metadata scheduling semantics.
+
+### M3. Agent result cache is unbounded
+
+**Evidence:** `cmd/olliesrv/internal/agent/agent.go:37`; `cmd/olliesrv/internal/agent/loop.go:524-532,604-611`.
+
+Read-only results are cached indefinitely unless file metadata invalidates them. Results without path metadata may never be invalidated.
+
+**Impact:** long-running agents retain tool arguments and result strings without count, byte, or TTL limits.
+
+**Remediation:** add byte/count limits and TTL eviction. Clear caches on profile and session resets where appropriate.
+
+### M4. Chat log and summary cache grow without bounds
+
+**Evidence:** `cmd/olliesrv/internal/agent/agent.go:155-168,209-218`; `cmd/olliesrv/internal/agent/history.go:64-86,514-516,574-576`.
+
+The agent retains the full chat log and summary cache for its lifetime.
+
+**Impact:** memory grows with long-running sessions even when history compaction occurs.
+
+**Remediation:** use bounded transcript retention or disk-backed archival. Bound summary-cache entries by count and bytes.
+
+### M5. FIFO retains consumed prompt references and is unbounded
+
+**Evidence:** `cmd/olliesrv/internal/agent/fifo.go:11-28`.
+
+`Pop()` advances the slice without clearing consumed entries. Queue capacity is unlimited.
+
+**Impact:** large consumed prompts remain retained by the backing array, while producers can build an unbounded queue.
+
+**Remediation:** clear consumed slots, compact the slice, and add queue admission limits or backpressure.
+
+### M6. Automatic compaction ignores cancellation
+
+**Evidence:** `cmd/olliesrv/internal/agent/loop.go:206`; `cmd/olliesrv/internal/agent/turn.go:195,259`; `cmd/olliesrv/internal/agent/history.go:543-559`.
+
+`context.WithoutCancel` allows compaction to continue after turn, session, or parent cancellation.
+
+**Impact:** shutdown and sub-agent cancellation can wait on an unbounded backend operation.
+
+**Remediation:** use a bounded compaction context with a separate shutdown deadline.
+
+### M7. Event subscription lifetime is detached from server lifetime
+
+**Evidence:** `cmd/olliesrv/internal/fs/spec.go:82-83`; `cmd/olliesrv/internal/session/event.go:35-70`.
+
+A wildcard event subscription is created with `context.Background()`. Rebuilding the filesystem tree adds another permanent subscriber, channel, goroutine, and latest-event buffer.
+
+**Remediation:** pass the daemon or root context into the filesystem builder.
+
+### M8. Bypass queue retains cancelled requests
+
+**Evidence:** `cmd/toolsrv/internal/bypass/bypass.go:59-85`.
+
+Cancellation deletes the request from the map but leaves it in the pending channel.
+
+**Impact:** cancelled requests consume queue slots and can block valid bypass requests.
+
+**Remediation:** make cancellation observable to the pending consumer and discard cancelled entries before presenting them for approval.
+
+### M9. Concurrent bypass resolution can panic
+
+**Evidence:** `cmd/toolsrv/internal/bypass/bypass.go:88-104`.
+
+Resolution checks under a mutex, unlocks, then closes the request channel. Two concurrent resolvers can both close the same channel.
+
+**Impact:** duplicate approval messages can panic toolsrv.
+
+**Remediation:** remove and mark the request resolved atomically under the mutex. Only the winning resolver closes the completion channel.
+
+### M10. LSP shutdown is not idempotent
+
+**Evidence:** `tools/lsp/server.go:41-51,112-121`.
+
+`Stop()` has no single-owner guard. Repeated or concurrent calls can race on stdin, `Wait()`, and the reader loop. Startup failure after pipe creation does not close all pipes.
+
+**Remediation:** use `sync.Once` or an explicit lifecycle state machine; close pipes on every startup failure.
+
+### M11. LSP timed-out requests retain channels until late responses
+
+**Evidence:** `tools/lsp/server.go:146-176`.
+
+After timeout, the pending request is deleted, but a late response keeps the associated channel alive until it arrives or the process exits.
+
+**Impact:** bounded retention under repeated timeouts; potentially significant with a stalled server.
+
+**Remediation:** bound outstanding requests and make shutdown release all pending waiters.
+
+### M12. `pushProcInterrupt` has weak connection ownership
+
+**Evidence:** `cmd/toolsrv/main.go:247-258`.
+
+The function explicitly closes the filesystem but not the underlying connection. It is launched once per completed background process without a bound or timeout.
+
+**Impact:** potential socket retention and completion-goroutine pressure.
+
+**Remediation:** close both filesystem and connection, use a timeout, and track completion work under toolsrv shutdown.
+
+### M13. Repeated process-GC startup creates duplicate goroutines
+
+**Evidence:** `cmd/toolsrv/internal/server/proc.go:487-501`.
+
+`StartProcGC` starts a goroutine each time it is called. The API has no guard against duplicate starts and no stored lifecycle handle.
+
+**Impact:** duplicate scans and leaked GC goroutines if callers start multiple loops.
+
+**Remediation:** make startup idempotent or move GC ownership into `State` construction.
+
+### M14. Toolsrv registry retains abandoned agent state
+
+**Evidence:** `cmd/toolsrv/internal/registry/registry.go:15-19,47-75`.
+
+`Load` creates per-agent tool and revision maps. `Unload` removes tools but never removes empty agent maps or revision entries.
+
+**Impact:** transient or deleted agent identifiers accumulate in long-lived toolsrv processes.
+
+**Remediation:** delete empty agent state when an agent is unloaded.
+
+### M15. `DismissProc` and process GC can lose cleanup ownership
+
+**Evidence:** `cmd/toolsrv/internal/server/proc.go:454-516`.
+
+Dismissal removes a process from tracking, while GC only removes exited processes after output is read. Abandoned completed processes therefore retain output until an explicit read, and dismissed running processes evade cleanup.
+
+**Remediation:** enforce a process state machine and absolute retention TTL.
+
+### M16. Remote and local socket paths are not cleaned on abnormal startup
+
+**Evidence:** `cmd/olliesrv/internal/toolclient/spawn.go:63-65,94-99,155-160,247-259`.
+
+Startup failure and forced termination bypass graceful toolsrv socket cleanup.
+
+**Impact:** stale socket files accumulate in temporary and runtime directories.
+
+**Remediation:** make socket-path cleanup part of process ownership and defer it immediately after path creation.
+
+### M17. Process-limit reservation is racy
+
+**Evidence:** `cmd/toolsrv/internal/server/proc.go:226-292`.
+
+The count check and process insertion are separated by setup work. Concurrent requests can all pass the limit check.
+
+**Impact:** configured process limits are exceeded during bursts.
+
+**Remediation:** reserve a slot before setup and release it on all failure and completion paths.
+
+### M18. `virtfs.Rdwr` contexts are retained until clunk
+
+**Evidence:** `virtfs/builder.go:273-286`.
+
+Successful synchronous writes retain their cancel function until `Close()`. Error paths also do not consistently cancel immediately.
+
+**Impact:** request state lives longer than the operation and complicates concurrent close behavior.
+
+**Remediation:** cancel completed operations promptly and retain only the cancellation needed for genuinely active work.
+
+### M19. `waitForSocket` delays cancellation
+
+**Evidence:** `cmd/olliesrv/internal/toolclient/spawn.go:396-407`.
+
+Polling uses `time.Sleep` and has no context. Cancellation waits until the fixed polling timeout.
+
+**Impact:** bounded but avoidable startup delay and delayed failure cleanup.
+
+**Remediation:** use a context-aware timer or ticker.
+
+### M20. Random-generation errors are ignored
+
+**Evidence:** `cmd/olliesrv/internal/toolclient/spawn.go:367-371`; `cmd/olliesrv/internal/toolclient/toolsrv.go:501-505`; `cmd/toolsrv/internal/server/server.go:103-107`.
+
+Errors from random identifier and secret generation are ignored.
+
+**Impact:** failed or weak randomness can cause collisions and lifecycle confusion.
+
+**Remediation:** return and handle random-source errors.
+
+### M21. LSP startup failure can leak pipes
+
+**Evidence:** `tools/lsp/server.go:41-51`.
+
+If `cmd.Start()` fails after pipe creation, the created stdin and stdout pipes are not both closed.
+
+**Impact:** repeated failed launches can retain descriptors until collection.
+
+**Remediation:** close every successfully acquired pipe on subsequent startup failure.
+
+### M22. Timer allocation churn in persistent loops
+
+**Evidence:** `cmd/toolsrv/main.go:176-215`; retry and polling loops in agent code.
+
+Repeated `time.After` calls allocate timers that cannot be stopped when another select case wins.
+
+**Impact:** bounded retention but unnecessary timer and allocation churn.
+
+**Remediation:** use reusable timers or tickers and stop them on exit.
+
+## Race and synchronization risks
+
+### R1. Per-fid caches are accessed without synchronization
+
+**Evidence:** `cmd/olliesrv/server.go:491-537,568-573`; corresponding toolsrv paths in `cmd/toolsrv/p9.go:348-445,510-551`.
+
+Read buffers, directory caches, wait state, and entry pointers are accessed after the connection lock is released. Concurrent read, write, clunk, or close operations can race.
+
+**Impact:** data races, corrupted reads, use-after-close behavior, and unexpected buffer retention.
+
+**Remediation:** serialize operations per fid or add a per-fid mutex and closed state.
+
+### R2. Auth fid state is not synchronized
+
+**Evidence:** `cmd/toolsrv/p9.go:169-230,348-357`.
+
+Authentication state and token fields are accessed after releasing the connection lock.
+
+**Impact:** concurrent auth operations can race and produce inconsistent authentication state.
+
+**Remediation:** protect auth state with a mutex or serialize auth-fid operations.
+
+### R3. `virtfs.Rdwr` operations can overwrite active state
+
+**Evidence:** `virtfs/builder.go:258-304`.
+
+Concurrent writes can replace `reqCtx` and `reqCancel`; reads can observe partially updated results; close can cancel the wrong request.
+
+**Remediation:** define single-flight semantics or implement a synchronized state machine.
+
+## Verification performed
+
+The following checks passed during the audit:
+
+```text
+go test ./...
+go vet ./...
+go test -race ./cmd/olliesrv/internal/agent ./cmd/olliesrv/internal/session ./cmd/olliesrv/internal/toolclient ./cmd/toolsrv/internal/exec ./cmd/toolsrv/internal/server ./tools/lsp
+```
+
+Additional focused race checks for `virtfs`, toolsrv server code, and Ollie server code passed. Existing tests do not cover the adversarial lifecycle scenarios in this report.
+
+## Recommended remediation order
+
+### Phase 1: shutdown correctness
+
+1. Add connection-scoped contexts.
+2. Close sockets before waiting for blocked handlers.
+3. Make response sends cancellation-aware.
+4. Close all fids on clunk, rename, EOF, and shutdown.
+5. Track and join writer goroutines.
+6. Cancel active agent actions before closing dependencies.
+
+### Phase 2: process ownership
+
+1. Make process shutdown single-owner and idempotent.
+2. Always call `Wait()`.
+3. Remove local and remote socket paths.
+4. Fix background startup signaling.
+5. Prevent dismissal of running processes.
+6. Make process-limit reservation atomic.
+7. Bound retained process output.
+
+### Phase 3: agent and session ownership
+
+1. Close `toolsConn` on every session shutdown path.
+2. Clean up partial toolsrv setup.
+3. Close old runtimes during replacement.
+4. Tie feed consumers to agent lifecycle.
+5. Bound workflow and prompt dispatch.
+6. Add compaction cancellation policy.
+
+### Phase 4: memory bounds
+
+1. Bound result cache.
+2. Bound summary cache.
+3. Bound or archive chat log.
+4. Fix FIFO slot clearing and queue limits.
+5. Limit bypass output.
+
+### Phase 5: synchronization and API correctness
+
+1. Fix `virtfs.Tree.Stat`.
+2. Serialize `Rdwr` state.
+3. Add per-fid synchronization.
+4. Make LSP shutdown idempotent.
+5. Make bypass resolution atomic.
+6. Tie wildcard event subscriptions to root context.
+
+## Required regression tests
+
+Add tests for:
+
+- client disconnect during blocking `Tread`;
+- response writer failure with many concurrent handlers;
+- `Tclunk` during a blocking read;
+- connection EOF with open blocking fids;
+- rename with open fids below the renamed path;
+- server shutdown during active reads;
+- `ListTools()` failure after action installation;
+- agent close during backend streaming and tool execution;
+- repeated profile switching with goroutine and FD counts;
+- feed consumer error and cancellation paths;
+- background sandbox setup failure;
+- background bypass execution;
+- dismissing a running process;
+- process kill followed by verified `Wait()`;
+- local and remote socket cleanup after startup failure;
+- remote stderr output after readiness;
+- concurrent process creation at the process limit;
+- abandoned completed-process GC;
+- bypass output exceeding the limit;
+- duplicate bypass resolution;
+- concurrent `Rdwr` read/write/close;
+- concurrent fid read/write/clunk;
+- repeated filesystem-tree construction and event-subscription cleanup;
+- repeated LSP start/stop calls.
+