diff --git a/docs/roadmap/audit/05-resources.md b/docs/roadmap/audit/05-resources.md new file mode 100644 index 0000000..86f1dab --- /dev/null +++ b/docs/roadmap/audit/05-resources.md @@ -0,0 +1,101 @@ +# Resource management audit + +Reviewed baseline: `9545bb0` on `main`. This review traced subprocesses, +buffers, committed blobs, temporary storage, HTTP responses, upload workers, +and cancellation across the public facade, CLI, and internal packages. The +findings below are based on source inspection; no load measurements or new +tests were run. The existing [concurrency audit](04-concurrency.md) records +inherited subprocess descriptors delaying cancellation and batch-reader +shutdown, and the [security audit](07-security.md) records unbounded unique +asset redirects and remote asset reads beyond the expected size. Those remain +separate remediation items. + +## 1. Go command output is retained for the entire execution + +**Relevant code:** [gotool.execute and combinedWriter](../../../internal/gotool/gotool.go), +and its callers in [shared validation](../../../internal/validation/validation.go) +and [dependency preparation](../../../internal/preparation/preparation.go). + +Every write to the combined stdout/stderr writer is appended to an in-memory +buffer, even for status-only `Run` calls and even when output is forwarded to +a caller's writer. A large or long-running Go test suite, compiler, or module +download with voluminous diagnostics therefore retains memory proportional to +all output until the process exits. On failure, formatting the full output in +an error can add another large allocation. Multiple simultaneous operations +multiply that consumption and can exhaust the process or host. The +[internal guide](../../internal/gotool.md#command-output-ownership) already +documents the full-capture choice and its memory tradeoff. + +**Potential approaches:** Evaluate spooling status-only diagnostics to a +temporary file while streaming output, with explicit cleanup and error +handling, or select a bounded diagnostic policy after deciding how much error +output callers should retain. Preserve full output where `Tool.Output` needs +it, forwarding behavior, error identity, and useful failure diagnostics; a +truncation policy changes the current diagnostic behavior and needs a deliberate +decision. Avoid replacing memory exhaustion with unbounded, unowned disk usage. + +## 2. Forge JSON responses have no resource bound + +**Relevant code:** [getJSON, getRelease, postRelease, and decodeJSON](../../../internal/gitea/release.go), +including attachment listing in [assets.go](../../../internal/gitea/assets.go) +and upload response decoding in [upload.go](../../../internal/gitea/upload.go). + +Successful HTTP response bodies are decoded directly without a byte limit. +The decoder buffers complete JSON values, including fields that are not used, +and attachment lists and release bodies are retained in memory. A faulty or +compromised forge returning a very large value or continuously streaming an +unfinished value can consume substantial memory or keep the operation active. +The second decode waits for EOF to reject trailing values; a response that +never ends can also prevent completion. The default client has no overall +timeout, although callers can cancel through the operation context. Response +bodies are closed when these functions return; the issue is consumption before +they can return, not a missing close. + +**Potential approaches:** Choose response-size and time budgets appropriate +to the supported forge contract, potentially configurable for large release +notes or attachment lists. Enforce a distinguishable limit rather than +silently accepting truncated JSON, and retain context cancellation and +malformed-response classification. For a failed create or upload response, +preserve uncertain-write reconciliation: rejecting a response does not prove +that the remote write failed. Any default limit is a compatibility decision, +not an implied existing restriction. + +## 3. Temporary build cleanup failures are silently discarded + +**Relevant code:** [candidate build callback](../../../internal/build/candidate.go), +[native smoke build](../../../internal/build/build.go), and +[managed delivery build](../../../internal/delivery/delivery.go). Each defers +`os.RemoveAll` without inspecting its error. + +If removal fails, for example because permissions or filesystem availability +change, the operation can return success or an unrelated failure while leaving +generated binaries and their directories behind. Repeated builds can consume +disk space, and an operator is not told where to clean up. The delivery guide +describes managed output as removed when delivery returns. This is distinct +from the previously recorded cancellation hang, which can prevent cleanup +from being reached at all. + +**Potential approaches:** Capture cleanup errors and report the affected path, +joining them with an existing failure where useful. Decide how to expose a +cleanup failure after remote delivery has succeeded without misrepresenting the +already completed remote state or encouraging an unsafe retry. Keep caller-owned +prebuilt artifacts outside this cleanup path. Apply the same review to the +temporary tag-message and receipt files, which also discard removal errors. + +## 4. A failed temporary-path resolution loses the cleanup path + +**Relevant code:** [tempdir.Outside](../../../internal/tempdir/tempdir.go), +at both calls to `filepath.EvalSymlinks` after `os.MkdirTemp`. + +The resolved value is assigned back to `directory` before checking the error; +on a resolution error the cleanup uses that returned value instead of the +saved `created` path. If the returned path is empty or otherwise unusable, +the newly created directory is not removed. This can occur if path resolution +fails after creation, for example after a concurrent filesystem or permission +change. The helper returns an error without telling the caller the directory's +name, so the caller cannot perform cleanup. The directory is initially empty; +the impact is leaked temporary entries rather than leaked build artifacts. + +**Potential approach:** Keep the created path until resolution succeeds and +use it for error-path removal, including the fallback allocation. Preserve the +original resolution error and consider reporting a cleanup error as well.