Remove completed concurrency audit
This commit is contained in:
Ollie Agent
parent
7351f374a3
commit
76eb5bea99
|
|
@ -1,311 +0,0 @@
|
|||
# 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
|
||||
|
||||
**Status:** Resolved by centralized fid cleanup in both 9P servers.
|
||||
|
||||
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
|
||||
|
||||
**Status:** Resolved by the turn cleanup path.
|
||||
|
||||
`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
|
||||
|
||||
**Status:** Resolved in `16bf211`.
|
||||
|
||||
`Agent.Close()` cancels the active turn, waits for completion, then closes toolsrv.
|
||||
|
||||
### H4. Feed consumers leak goroutines and connections
|
||||
|
||||
**Status:** Resolved in `2352f24`.
|
||||
|
||||
Feed consumers defer filesystem and connection cleanup, and cancellation closes the consumer resources.
|
||||
|
||||
### H5. Runtime and toolsrv replacement leaks old connections
|
||||
|
||||
**Status:** Resolved by the runtime and paused-session lifecycle fixes.
|
||||
|
||||
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
|
||||
|
||||
**Status:** Resolved by session shutdown and pause/resume lifecycle fixes.
|
||||
|
||||
Session close paths cancel the session context and release toolsrv connections instead of leaving them behind.
|
||||
|
||||
### H7. Failed toolsrv setup leaks infrastructure
|
||||
|
||||
**Status:** Resolved by toolsrv startup and session setup cleanup.
|
||||
|
||||
Startup failure paths now release created processes, keepers, sockets, and connections before returning an error.
|
||||
|
||||
### H8. Remote SSH stderr is not drained after startup
|
||||
|
||||
**Status:** Resolved in `d31d066`.
|
||||
|
||||
Remote toolsrv startup drains SSH stderr for the process lifetime into a bounded sink.
|
||||
|
||||
### H9. Dismissed toolsrv processes can become orphaned
|
||||
|
||||
**Status:** Resolved in `93e7f51` and `d5def6f`.
|
||||
|
||||
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
|
||||
|
||||
**Status:** Resolved in `c0ce185`.
|
||||
|
||||
Fallback-opened files are closed after collecting their stat information.
|
||||
|
||||
### H11. `virtfs` `Rdwr` state is not synchronized
|
||||
|
||||
**Status:** Resolved in `562e1ef`.
|
||||
|
||||
`Rdwr` state uses synchronized request, result, cancellation, and close handling with serialized writes.
|
||||
|
||||
### H12. Process-limit enforcement is not atomic
|
||||
|
||||
**Status:** Resolved in `b6009f0`.
|
||||
|
||||
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
|
||||
|
||||
**Status:** Resolved in `6a4f83b`.
|
||||
|
||||
Completed process output and metadata are bounded by absolute retention and output limits.
|
||||
|
||||
The following medium finding was verified fixed in commit `1284a83`:
|
||||
|
||||
- **M8. Bypass queue retains cancelled requests** — cancellation now removes the request from the active set, and `NextPending` discards stale queue entries.
|
||||
|
||||
The remaining medium and lower findings are listed below.
|
||||
|
||||
### M1. Workflow and prompt goroutines are unbounded
|
||||
|
||||
**Status:** Resolved in `6a8e897`.
|
||||
|
||||
Asynchronous session work is bounded, tracked, and canceled with session shutdown.
|
||||
|
||||
### M2. Parallel tool dispatch is unbounded
|
||||
|
||||
**Status:** Resolved in `062642d`.
|
||||
|
||||
Parallel tool execution uses bounded concurrency while preserving tool scheduling semantics.
|
||||
|
||||
### M3. Agent result cache is unbounded
|
||||
|
||||
**Status:** Resolved in `6c1de81`.
|
||||
|
||||
Read-only result caching has byte/count bounds and eviction.
|
||||
|
||||
### M4. Chat log and summary cache grow without bounds
|
||||
|
||||
**Status:** Resolved in `e01d578`.
|
||||
|
||||
Chat and summary caches use bounded retention.
|
||||
|
||||
### M5. FIFO retains consumed prompt references and is unbounded
|
||||
|
||||
**Status:** Resolved in `c6d9b4a`.
|
||||
|
||||
Prompt queues clear consumed entries and enforce bounded capacity.
|
||||
|
||||
### M6. Automatic compaction ignores cancellation
|
||||
|
||||
**Status:** Resolved in `7ec8583`.
|
||||
|
||||
Compaction streaming honors cancellation.
|
||||
|
||||
### M7. Event subscription lifetime is detached from server lifetime
|
||||
|
||||
**Status:** Resolved in `f41a580`.
|
||||
|
||||
Event subscriptions use the server context and terminate with the server.
|
||||
|
||||
### M8. Bypass queue retains cancelled requests
|
||||
|
||||
**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:** Resolved in `4b86ad9`.
|
||||
|
||||
`Resolve` performs request removal, result publication, and completion signaling atomically. Cancellation verifies request ownership before removal.
|
||||
|
||||
### M10. LSP shutdown is not idempotent
|
||||
|
||||
**Status:** Resolved in `55d1814`.
|
||||
|
||||
`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
|
||||
|
||||
**Status:** Resolved by the existing request cleanup.
|
||||
|
||||
Timeout and send-error paths delete the request channel from `pending`. Late responses find no pending entry and are discarded, so they do not retain the channel. Shutdown also releases all pending waiters.
|
||||
|
||||
### M12. `pushProcInterrupt` has weak connection ownership
|
||||
|
||||
**Status:** Resolved in `a1dc5b9`.
|
||||
|
||||
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:** Resolved in `6ef1fe5`.
|
||||
|
||||
`State.StartProcGC` uses `sync.Once`, and the GC goroutine exits when its context is canceled.
|
||||
|
||||
### M14. Toolsrv registry retains abandoned agent state
|
||||
|
||||
**Status:** Resolved in `1b934c4`.
|
||||
|
||||
Unloading the final tool removes the per-agent registry and revision state.
|
||||
|
||||
### M15. `DismissProc` and process GC can lose cleanup ownership
|
||||
|
||||
**Status:** Resolved by the existing process lifecycle safeguards.
|
||||
|
||||
`DismissProc` refuses to remove running processes, preserving shutdown visibility. Process GC removes exited processes after an absolute `EndTime` TTL as well as after an old output read, so abandoned completed processes cannot retain output indefinitely.
|
||||
|
||||
### M16. Remote and local socket paths are not cleaned on abnormal startup
|
||||
|
||||
**Status:** Resolved by `a9f4899` and `2ee9251`.
|
||||
|
||||
Startup failures clean up local and remote socket paths as part of process ownership.
|
||||
|
||||
### M17. Process-limit reservation is racy
|
||||
|
||||
**Status:** Resolved in `b6009f0`.
|
||||
|
||||
`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
|
||||
|
||||
**Status:** Resolved by the existing `Rdwr` lifecycle.
|
||||
|
||||
Each write creates a request context, publishes its cancellation function only while the handler is active, clears it under `stateMu` on completion, and calls `cancel` immediately after the handler returns. Close cancels only the currently active request.
|
||||
|
||||
### M19. `waitForSocket` delays cancellation
|
||||
|
||||
**Status:** Resolved in `9bbb6b9`.
|
||||
|
||||
Socket polling accepts a context, uses reusable timers, and exits when startup is canceled.
|
||||
|
||||
### M20. Random-generation errors are ignored
|
||||
|
||||
**Status:** Resolved in `e24675b`.
|
||||
|
||||
Random identifier and secret generation errors are returned and handled.
|
||||
|
||||
### M21. LSP startup failure can leak pipes
|
||||
|
||||
**Status:** Resolved by the existing startup cleanup.
|
||||
|
||||
`StartServer` closes `stdin` when `StdoutPipe` fails, and closes both acquired pipes when `cmd.Start()` fails. Pipe ownership is transferred to `Server` only after process startup succeeds.
|
||||
|
||||
### M22. Timer allocation churn in persistent loops
|
||||
|
||||
**Status:** Resolved in `c111cbd`.
|
||||
|
||||
Persistent loops use reusable timers or tickers and stop them on cancellation.
|
||||
|
||||
## Race and synchronization risks
|
||||
|
||||
### R1. Per-fid caches are accessed without synchronization
|
||||
|
||||
**Status:** Resolved by per-fid synchronization in both 9P servers.
|
||||
|
||||
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
|
||||
|
||||
**Status:** Resolved by per-auth-fid synchronization in toolsrv.
|
||||
|
||||
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
|
||||
|
||||
**Status:** Resolved by the existing 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
|
||||
|
||||
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
|
||||
|
||||
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
|
||||
|
||||
Manual validation confirms the following behaviors through the GUI, TUI, and toolsrv execution paths:
|
||||
|
||||
- blocking `Tread` via `statewait`;
|
||||
- `Tclunk` during a blocking read;
|
||||
- client-facing session IDs, names, and namespace data;
|
||||
- toolsrv tool execution.
|
||||
|
||||
One lifecycle scenario was manually validated:
|
||||
|
||||
- server shutdown during active reads; shutdown completed without hanging.
|
||||
|
||||
Loading…
Reference in New Issue