170 lines
9.3 KiB
Markdown
170 lines
9.3 KiB
Markdown
# 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.
|