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:

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, buffered Git execution in internal/git/remote.go, and BlobReader.Close, especially the stdout drain at line 110 followed by the child wait. The owning cleanup runs from shared validation.

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. 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. 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, 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:

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.

9.3 KiB Raw Blame History Unescape Escape