Record concurrency audit findings

docs/roadmap/audit/04-concurrency.md

@@ -0,0 +1,169 @@
+# Concurrency audit
+
+Reviewed baseline: `d884ad7` on `main`, using Go 1.26.10 and Git 2.53.0 on
+Linux. The findings below remain pending. Finding 1 overlaps an existing
+correctness finding; finding 2 is an additional internal-helper weakness.
+
+## Architecture and scope
+
+The root facade coordinates candidate checking, publication, builds, preparation,
+and hosted delivery. The CLI supplies profiles, credentials, output, and process
+signals. Internal packages own the operation state and external effects;
+`internal/forge` separates shared delivery policy from the Gitea adapter.
+The `buildinfo` facade has a small, independent runtime implementation.
+
+Most orchestration is sequential. Go settings owners, candidate-tree caches,
+delivery result slices, and backend instances belong to individual operations.
+The principal concurrency mechanisms are:
+
+- `internal/process` uses context-driven subprocess cancellation, standard
+  library I/O goroutines, and `sync.Once` for piped-process waiting.
+- `internal/git.BlobReader` serializes batch exchanges with a mutex and joins
+  each read's cancellation callback before releasing the exchange.
+- `internal/gotool` serializes combined stdout/stderr capture and forwarding
+  with a mutex. Captured bytes are consumed after process completion.
+- `internal/gitea` streams multipart uploads through an `io.Pipe` and a worker
+  goroutine, joining that worker before returning the borrowed content reader.
+- `internal/runtimeinfo` caches fallback version discovery with
+  `sync.OnceValue`; the injected version is initialized at link time.
+
+The review examined those ownership and shutdown paths, callback sequencing,
+shared caches and output, build cleanup, publication guards/reconciliation,
+delivery recovery, and signal propagation. Operation-local helpers such as the
+Go settings owner and candidate tree are used sequentially by current callers;
+their mutable maps alone do not establish a concurrency defect.
+
+## Verification
+
+The repeated, uncached race checks passed:
+
+```sh
+GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -race -count=5 \
+    ./internal/process ./internal/git ./internal/gitea ./internal/runtimeinfo
+```
+
+The preceding correctness audit also passed the full ordinary and race-enabled
+suites and compiled API/dependency checks. Production code and tests are
+unchanged between those checks and this review's baseline.
+
+Temporary race-enabled programs outside this checkout reproduced the two
+findings below without race-detector diagnostics. Controlled Git substitutes
+provided readiness and release signals; short-lived descendants exercised
+inherited descriptors. Local HTTP/1.1 and HTTP/2 probes of early upload
+rejection returned promptly and produced no additional finding. These probes
+used local fixtures and placeholder credentials.
+
+No data race or unjoined multipart-worker leak was identified in the reviewed
+current orchestration paths. Race detection complements the liveness evidence;
+a clean race run does not detect a blocked operation.
+
+## 1. Medium: inherited descriptors can keep canceled operations and reader shutdown blocked
+
+**Relevant code:** [process.Run, newCommand, and PipedProcess.Wait](../../../internal/process/process.go),
+buffered Git execution in [internal/git/remote.go](../../../internal/git/remote.go),
+and [BlobReader.Close](../../../internal/git/blob_reader.go), especially the
+stdout drain at line 110 followed by the child wait. The owning cleanup runs
+from [shared validation](../../../internal/validation/validation.go).
+
+**Circumstances and mechanism:** A subprocess launches a descendant that
+inherits stdout or stderr. Cancellation kills the immediate process, while
+the descendant retains the descriptor. Buffered command execution waits for
+its standard library I/O workers to receive EOF. The batched reader's normal
+shutdown closes stdin, drains stdout to EOF, and then waits for the child.
+Both paths can remain blocked after the governing context is canceled.
+
+This is the same underlying issue recorded as finding 3 in the
+[correctness audit](01-correctness.md#3-medium-inherited-process-pipes-can-defeat-cancellation-and-recovery-deadlines).
+That audit reproduced delayed cancellation through public
+`PrepareTaggedCheckout` using a simulated SSH transport. This concurrency pass
+additionally exercised the explicitly owned batch-reader shutdown path.
+
+**Additional reproduction:** A controlled `git cat-file --batch` substitute
+returned a valid three-byte blob. After stdin reached EOF, it launched
+`sleep 3` with inherited descriptors and exited. The reader's operation context
+had a 500 ms deadline. The initial read succeeded, but `BlobReader.Close`
+returned only after approximately 3.005 seconds, with the deadline error then
+preserved. The probe ran under the race detector and completed after its
+descendant exited. A descendant retaining the descriptors indefinitely can
+keep this shutdown waiting indefinitely.
+
+**Consequences:** Cancellation can leave validation or preparation occupied
+beyond its deadline, delay temporary-output cleanup, and prevent publication
+reconciliation from returning its documented bounded unresolved outcome.
+`BlobReader.Close` holds the reader mutex during its drain, so other readers
+and repeated closes also wait for that shutdown to finish.
+
+**Potential remediation:** Give subprocess I/O a bounded cancellation/shutdown
+path. Use the standard `exec.Cmd.WaitDelay` mechanism where applicable, and
+verify that explicitly owned stdin/stdout pipes also close when their operation
+is canceled. Coordinate cancellation callbacks, pipe closure, and `Wait` so
+the child and workers are joined before the owning operation returns. Apply
+the same review to direct subprocess calls in
+[internal/build](../../../internal/build/build.go). Preserve existing context
+error identity and publication uncertainty semantics.
+
+Regression coverage should include a ready descendant retaining a descriptor
+during both buffered command execution and batch-reader cleanup. The existing
+single-process cancellation tests pass. This finding calls for completing the
+selected cancellation behavior; it does not require a general service for
+supervising every descendant process. Treat this and correctness finding 3 as
+one remediation unit.
+
+## 2. Low: a canceled queued blob read can later abort a healthy session
+
+**Relevant code:** [BlobReader.Read](../../../internal/git/blob_reader.go),
+lines 44–88. It checks the request context before acquiring `reader.mu`, then
+starts or reuses the session and registers the cancellation callback after
+acquiring the mutex. It does not recheck the request context between those
+steps. `abortLocked` records any exchange cancellation as the reader's terminal
+error.
+
+**Circumstances:** Two goroutines call `Read` on one reader with independent
+request contexts. The first holds the mutex during a slow exchange. The
+second passes its initial context check, waits for the mutex, and is canceled
+while waiting. Its cancellation callback is not registered during that wait.
+
+Once the first exchange finishes, the second acquires the mutex and proceeds
+with an already-canceled context. Its newly registered `context.AfterFunc`
+can immediately cancel the healthy child and close its pipes. The read then
+records a terminal cancellation, affecting subsequent uncanceled requests.
+
+**Reproduction:** A controlled batch process signaled when the first exchange
+was active and waited for an external release signal. An observed request
+context established that the second read had passed its initial context check
+before cancellation. The canceled request remained blocked during a 100 ms
+observation window. After releasing the first exchange:
+
+```text
+first read error: <nil>
+second read canceled: true
+next uncanceled read inherits cancellation: true
+```
+
+The race-enabled probe produced no data-race diagnostic. The defect concerns
+the ordering of cancellation and session ownership.
+
+**Consequences and current reach:** Queued canceled work can wait behind an
+unrelated exchange and then terminate its otherwise usable shared reader.
+The helper advertises serialized reads and implements mutex-based sharing,
+but current candidate validation calls it sequentially. This is an internal
+concurrent-use weakness; the audit did not identify a current public operation
+that issues these overlapping reads on one reader.
+
+**Potential remediation:** At minimum, recheck `ctx.Err()` immediately after
+acquiring the mutex and before starting an exchange or registering a callback
+that can cancel the child. This prevents a request canceled while queued from
+changing the healthy session. If queued reads should return promptly on their
+own cancellation, use cancellation-aware acquisition of the exchange slot.
+Keep current candidate-tree traversal sequential while assessing that need;
+this finding supplies no reason to parallelize validation or add locks to its
+operation-local caches. A focused helper regression can verify the healthy
+first exchange and a later uncanceled read after cancellation of queued work.
+
+## Recommendation
+
+Complete the existing subprocess-cancellation remediation first, including the
+explicit batch-reader shutdown path. Address the queued-read context recheck
+as a small internal correctness improvement; select cancellation-aware queue
+waiting according to the helper's intended concurrent-use contract. Retain the
+current operation-local architecture and joined worker lifetimes.