diff --git a/docs/roadmap/audit/04-concurrency.md b/docs/roadmap/audit/04-concurrency.md new file mode 100644 index 0000000..94b21bd --- /dev/null +++ b/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: +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.