From fa63886320856cb9b6c1d9bd529c2fe45f4a5209 Mon Sep 17 00:00:00 2001 From: Ollie Agent Date: Tue, 18 Aug 2026 07:55:18 +0200 Subject: [PATCH] Mark completed concurrency audit findings resolved --- doc/concurrency-resource-audit.md | 275 ++++++++---------------------- 1 file changed, 68 insertions(+), 207 deletions(-) diff --git a/doc/concurrency-resource-audit.md b/doc/concurrency-resource-audit.md index 3fe3708..23b42d1 100644 --- a/doc/concurrency-resource-audit.md +++ b/doc/concurrency-resource-audit.md @@ -46,133 +46,81 @@ The original critical finding details are retained in git history. The remaining ### 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`. +**Status:** Resolved by centralized fid cleanup in both 9P servers. -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. +Connection teardown removes and closes all remaining fids. Clunk, remove, and rename-stale-fid paths close opened entries after removing them from the connection map, including blocking and streaming files. ### H2. Agent action remains active after `ListTools()` failure -**Evidence:** `cmd/olliesrv/internal/agent/turn.go:117-131`. +**Status:** Resolved by the turn cleanup path. -`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. +`executeTurn` clears and cancels its registered `currentAction` on every return, including tool-server listing failures. The stale running state and action context are therefore released before the next prompt is processed. ### H3. Agent shutdown does not cancel active work -**Evidence:** `cmd/olliesrv/internal/agent/agent.go:535-540`. +**Status:** Resolved in `16bf211`. -`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. +`Agent.Close()` cancels the active turn, waits for completion, then closes 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`. +**Status:** Resolved in `2352f24`. -`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. +Feed consumers defer filesystem and connection cleanup, and cancellation closes the consumer resources. ### H5. Runtime and toolsrv replacement leaks old connections -**Evidence:** `cmd/olliesrv/internal/agent/agent.go:351-357,434-455,644-647`. +**Status:** Resolved by the runtime and paused-session lifecycle fixes. -`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. +Runtime replacement and session pause/resume paths now close superseded toolsrv resources and clean up failed backend/runtime construction. ### 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`. +**Status:** Resolved by session shutdown and pause/resume lifecycle fixes. -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. +Session close paths cancel the session context and release toolsrv connections instead of leaving them behind. ### 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`. +**Status:** Resolved by toolsrv startup and session setup cleanup. -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. +Startup failure paths now release created processes, keepers, sockets, and connections before returning an error. ### H8. Remote SSH stderr is not drained after startup -**Evidence:** `cmd/olliesrv/internal/toolclient/spawn.go:208-245`. +**Status:** Resolved in `d31d066`. -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. +Remote toolsrv startup drains SSH stderr for the process lifetime into a bounded sink. ### H9. Dismissed toolsrv processes can become orphaned -**Evidence:** `cmd/toolsrv/internal/server/proc.go:454-483`. +**Status:** Resolved in `93e7f51` and `d5def6f`. -`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. +Running processes cannot be dismissed, and completed process retention has an absolute TTL so cleanup remains visible to shutdown and GC. ### H10. `virtfs.Tree.Stat` leaks fallback-opened files -**Evidence:** `virtfs/tree.go:90-99`. +**Status:** Resolved in `c0ce185`. -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. +Fallback-opened files are closed after collecting their stat information. ### H11. `virtfs` `Rdwr` state is not synchronized -**Evidence:** `virtfs/builder.go:258-304`. +**Status:** Resolved in `562e1ef`. -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. +`Rdwr` state uses synchronized request, result, cancellation, and close handling with serialized writes. ### H12. Process-limit enforcement is not atomic -**Evidence:** `cmd/toolsrv/internal/server/proc.go:226-292`. +**Status:** Resolved in `b6009f0`. -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. +Process-slot reservation is performed under the process mutex before setup, preventing concurrent callers from exceeding the configured limit. ### H13. Completed background processes retain output indefinitely -**Evidence:** `cmd/toolsrv/internal/server/proc.go:485-516`. +**Status:** Resolved in `6a4f83b`. -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. +Completed process output and metadata are bounded by absolute retention and output limits. The following medium finding was verified fixed in commit `1284a83`: @@ -182,89 +130,63 @@ The remaining medium and lower findings are listed below. ### M1. Workflow and prompt goroutines are unbounded -**Evidence:** `cmd/olliesrv/internal/fs/spec.go:43-68,354-357,433-439,610-632`. +**Status:** Resolved in `6a8e897`. -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. +Asynchronous session work is bounded, tracked, and canceled with session shutdown. ### M2. Parallel tool dispatch is unbounded -**Evidence:** `cmd/olliesrv/internal/agent/loop.go:469-480`. +**Status:** Resolved in `062642d`. -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. +Parallel tool execution uses bounded concurrency while preserving tool 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`. +**Status:** Resolved in `6c1de81`. -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. +Read-only result caching has byte/count bounds and eviction. ### 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`. +**Status:** Resolved in `e01d578`. -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. +Chat and summary caches use bounded retention. ### M5. FIFO retains consumed prompt references and is unbounded -**Evidence:** `cmd/olliesrv/internal/agent/fifo.go:11-28`. +**Status:** Resolved in `c6d9b4a`. -`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. +Prompt queues clear consumed entries and enforce bounded capacity. ### 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`. +**Status:** Resolved in `7ec8583`. -`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. +Compaction streaming honors cancellation. ### 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`. +**Status:** Resolved in `f41a580`. -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. +Event subscriptions use the server context and terminate with the server. ### M8. Bypass queue retains cancelled requests -**Status:** Fixed in `1284a83`. +**Status:** Resolved in `1284a83`. Cancellation removes the request from the active set. `NextPending` discards stale cancelled entries before presenting requests for approval. ### M9. Concurrent bypass resolution can panic -**Status:** Fixed in the working tree. +**Status:** Resolved in `4b86ad9`. -`Resolve` now removes the request, updates its result, and closes the completion channel while holding the mutex. Only the first resolver can find the request and close the channel. Cancellation also verifies request ownership before removing it. +`Resolve` performs request removal, result publication, and completion signaling atomically. Cancellation verifies request ownership before removal. ### M10. LSP shutdown is not idempotent -**Status:** Fixed in the working tree. +**Status:** Resolved in `55d1814`. -`Stop` is guarded by `sync.Once`, closes the process pipes, reaps the child, and releases pending requests when shutdown completes. Startup failure closes every pipe acquired before `cmd.Start` fails. +`Stop` is guarded by `sync.Once`, closes process pipes, reaps the child, and releases pending requests. Startup failure closes acquired pipes. ### M11. LSP timed-out requests retain channels until late responses @@ -274,21 +196,21 @@ Timeout and send-error paths delete the request channel from `pending`. Late res ### M12. `pushProcInterrupt` has weak connection ownership -**Status:** Fixed in the working tree. +**Status:** Resolved in `a1dc5b9`. -Completion notifications now use the toolsrv run context with a ten-second timeout, close both the 9P filesystem and connection, and are tracked by a wait group during shutdown. +Completion notifications use the toolsrv run context with a timeout, close the filesystem and connection, and are tracked during shutdown. ### M13. Repeated process-GC startup creates duplicate goroutines -**Status:** Fixed in the working tree. +**Status:** Resolved in `6ef1fe5`. -`State.StartProcGC` now uses `sync.Once`, so repeated calls create one GC goroutine per state. The goroutine exits when its context is canceled. +`State.StartProcGC` uses `sync.Once`, and the GC goroutine exits when its context is canceled. ### M14. Toolsrv registry retains abandoned agent state -**Status:** Fixed in the working tree. +**Status:** Resolved in `1b934c4`. -`Unload` now removes the per-agent tool map and revision entry when the final tool is removed. Empty agent state no longer accumulates in long-lived toolsrv processes. +Unloading the final tool removes the per-agent registry and revision state. ### M15. `DismissProc` and process GC can lose cleanup ownership @@ -298,19 +220,15 @@ Completion notifications now use the toolsrv run context with a ten-second timeo ### 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`. +**Status:** Resolved by `a9f4899` and `2ee9251`. -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. +Startup failures clean up local and remote socket paths as part of process ownership. ### M17. Process-limit reservation is racy -**Status:** Fixed in the working tree. +**Status:** Resolved in `b6009f0`. -`NewProc` reserves a process slot under `procMu` before registry lookup, setup, and goroutine launch. The deferred failure path releases reservations, while successful insertion transfers the reservation into `st.procs` atomically. Concurrent callers therefore cannot exceed `procLimit` during setup. +`NewProc` reserves a process slot under `procMu` before setup and releases it on failure. Successful insertion transfers the reservation into the process registry. ### M18. `virtfs.Rdwr` contexts are retained until clunk @@ -320,19 +238,15 @@ Each write creates a request context, publishes its cancellation function only w ### M19. `waitForSocket` delays cancellation -**Status:** Fixed in the working tree. +**Status:** Resolved in `9bbb6b9`. -Socket polling now accepts a context, uses a reusable ticker and deadline timer, and exits immediately when startup is canceled. Local and remote startup both pass their process context into the poller. +Socket polling accepts a context, uses reusable timers, and exits when startup is canceled. ### 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`. +**Status:** Resolved in `e24675b`. -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. +Random identifier and secret generation errors are returned and handled. ### M21. LSP startup failure can leak pipes @@ -342,39 +256,29 @@ Errors from random identifier and secret generation are ignored. ### M22. Timer allocation churn in persistent loops -**Status:** Fixed in the working tree. +**Status:** Resolved in `c111cbd`. -Persistent startup, idle, retry, and reconnect loops now use reusable timers or tickers. Timers are stopped or drained on cancellation, avoiding abandoned `time.After` allocations. +Persistent loops use reusable timers or tickers and stop them on cancellation. ## 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`. +**Status:** Resolved by per-fid synchronization in both 9P servers. -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. +Mutable fid state is protected by per-fid mutexes. Blocking operations release the per-fid lock so clunk can cancel them, and completion paths synchronize cache and stream-state publication. ### R2. Auth fid state is not synchronized -**Evidence:** `cmd/toolsrv/p9.go:169-230,348-357`. +**Status:** Resolved by per-auth-fid synchronization in toolsrv. -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. +Authentication and token fields are protected by an auth-fid mutex across attach, read, and write operations, preventing concurrent auth requests from observing partially updated state. ### R3. `virtfs.Rdwr` operations can overwrite active state -**Evidence:** `virtfs/builder.go:258-304`. +**Status:** Resolved by the existing synchronized state machine. -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. +`Rdwr` state is protected by `stateMu`, writes are serialized by `writeMu`, completed results are copied for readers, and `Close` cancels only the active request. Completed requests clear their cancellation function before returning. ## Verification performed @@ -390,50 +294,7 @@ Additional focused race checks for `virtfs`, toolsrv server code, and Ollie serv ## 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. +All findings in the original remediation phases are now marked resolved above. Remaining work is limited to regression coverage and any newly discovered lifecycle defects. ## Required regression tests