builds: address build-system review follow-ups

[hiroTamada]

Summary

Implements the still-actionable follow-up items from the build-system review tracked in #54, against current main:

• Upload bounds (POST /builds): every multipart part is now read through a size-limited reader — 512 MiB for the source tarball (consistent with the 10 GB source volume) and 1 MiB for each small form field. Over-limit uploads get a clear 400 (invalid_source / invalid_request) instead of an unbounded io.ReadAll; multipart read failures surface their cause in the 400 message.
• Required secrets fail hard: FileSecretProvider now errors on missing secrets and invalid IDs (path-traversal validation preserved via exported ValidateSecretID, also enforced at the API boundary with a 400). The host reports provider errors / missing values to the builder agent over vsock (secrets_response.error), and the agent fails the build with a clear message instead of proceeding with empty secrets — including the guest-side 30s wait timeout, which previously proceeded anyway.
• Build lifecycle / shutdown: BuildQueue start functions now take a context; each run derives a cancellable context tracked by the queue, and new Manager.Shutdown / BuildQueue.Shutdown (wired into the API shutdown handler) cancel in-flight builds and wait for their goroutines. CreateBuild enqueues with context.WithoutCancel(ctx) so builds survive request cancellation while keeping trace/log values. Pending builds are never started after shutdown — they stay queued on disk for startup recovery. Cancel of a pending build also releases its context. A run cancelled by shutdown is left in its non-terminal status (never a spurious "context canceled" failure) so startup recovery re-runs it, matching crash semantics.
• Crash/partial-execution recovery: executeBuild now tolerates leftover build-source-<id> / build-config-<id> volumes from an interrupted attempt — delete and recreate once; if a leftover volume is still attached, only the stale builder-<id> instance is deleted (detaching it) before the volume delete, and a volume attached to an unknown instance is never force-deleted (clear error instead). The config-volume copy-over fallback no longer masks a failed recreate (addresses the known review finding on builds: recover crashed builds by cleaning up leftover source/config volumes #337, which this supersedes on current main).
• No fixed pre-dial sleep: waitForResult drops the unconditional 3s sleep; it dials immediately and retries with a context-aware 2s interval (30 attempts, no trailing sleep after the last one), failing fast if the builder instance stops.
• AllowedDomains removed: the build-policy field was never wired to anything; removed from BuildPolicy along with the stale "domain allowlist" claim in the builds README. Egress stays all-or-nothing via network_mode.
• Corrupt metadata visibility: listAllBuilds / listPendingBuilds take a logger and warn on unreadable/unparseable entries while still returning valid builds.
• ErrBuildInProgress doc corrected: the sentinel covers the queued-to-running cancel race; comment now says so.

Also pinned existing guarantees with tests: terminal-state protection (a cancelled build whose run later fails stays cancelled), duplicate builder-instance deletion tolerance on cancel, and the queued-race sentinel.

Test plan

• go test -tags containers_image_openpgp ./lib/builds/... — pass
• go test -race -tags containers_image_openpgp ./lib/builds/... — pass
• go test -race -tags containers_image_openpgp -run 'TestCreateBuild_' ./cmd/api/api/ — pass
• New coverage: shutdown-interrupted builds retain building for recovery (not a spurious failure), multipart limit boundaries (at/over limit for source and form fields, secret-ID rejection), provider hard-fail cases, queue shutdown cancellation/await/no-start-after-shutdown plus a race stress test, manager shutdown detaching request cancellation while preserving context values, leftover-volume recovery (already-exists, in-use with/without stale builder, config recreate failure), corrupt-metadata logging.
• go vet and gofmt clean. lib/oapi/oapi.go regenerated via make oapi-generate (spec comment only; the source field now documents the size limits).
• Full cmd/api/api suite: VM-backed tests fail in this environment (no KVM / no net-admin) identically on origin/main; all non-VM tests pass.

---

Note

Medium Risk
Touches build queue shutdown, secret handling, and volume recovery on retry—important for reliability and security, but well-tested and scoped to the build path.

Overview
Follow-up hardening for the build system: upload bounds, required secrets, graceful shutdown, and retry after interruption.

POST /builds now reads every multipart part through size limits (512 MiB for source, 1 MiB per other field) and returns clear 400s instead of unbounded reads. Secret IDs are validated at the API (ValidateSecretID) before a job is accepted.

Secrets are required end-to-end: the file provider errors on missing or traversal-prone IDs; the host sends secrets_response.error over vsock when fetch fails; the builder agent fails the build on host errors, timeouts, or missing values (no more proceeding with empty secrets after a 30s wait).

Lifecycle: BuildQueue runs use a cancellable context; Manager.Shutdown (API shutdown) cancels in-flight work and waits for goroutines. Enqueue uses context.WithoutCancel so builds outlive request cancellation but still carry trace context. Shutdown-interrupted runs stay in a non-terminal status for startup recovery. Crash retries delete and recreate leftover build-source-* / build-config-* volumes, optionally removing a stale builder-<id> instance when a volume is still attached.

Smaller changes: drop unused AllowedDomains from BuildPolicy; waitForResult dials immediately with retries (no fixed 3s sleep); corrupt build metadata is logged and skipped when listing.

^{Reviewed by Cursor Bugbot for commit 18fafa4. Bugbot is set up for automated code reviews on this repo. Configure here.}

[github-actions]

-->

✱ stlc build

✅ go code · compare

Your SDK build was successful.

generate ✅ → bootstrap ✅ → format ✅

112 files generated at 8f124a3 (pushed)

go get github.com/kernel/hypeman-go-staging@8f124a3a014d0abedd9903e9f2dfaed8ccd09521

✅ typescript code · compare

Your SDK build was successful.

generate ✅ → bootstrap ✅ → format ✅

134 files generated at 4500f80 (pushed)

Diagnostics: 💡 0 new / 5 total note

Level	Code	Message	Targets

Build metadata

Build	bd_766r5fdm-fizzy-kite
Timestamp	2026-08-10T18:58:37.720Z
stlc	8413509
Spec hash	3401acc59fc0
Config hash	9125eb22cb4c

---

This comment is auto-generated by stlc and is kept up to date as you push.
If you push new commits, re-run this workflow to update this comment.
Last updated: 2026-08-10 18:58:54 UTC

[hiroTamada]

Fable 5 review — round 1

Reviewed head 0a18d88 against #54 and #337 (including the Bugbot finding on #337). Read the full diff plus surrounding code (queue.go, manager.go, builder_agent/main.go, cmd/api/main.go shutdown handler, lib/volumes/manager.go, lib/instances/manager.go). Verified locally:

• go build -tags containers_image_openpgp ./..., go vet, gofmt clean
• go test -race -count=1 -tags containers_image_openpgp ./lib/builds/... — pass
• go test -race -count=1 -tags containers_image_openpgp -run 'Build' ./cmd/api/api/ — pass
• make oapi-generate reproduces lib/oapi/oapi.go exactly (no drift; spec change is doc-only)

What I checked strictly

• Bounded memory / multipart: every part now goes through readLimitedPart (limit+1 LimitReader), including unknown field names; part.Close() on all paths; boundary tests at/over limit for both source and small fields. No bypass found — a part is capped regardless of its Content-Disposition name, and only source gets the 512 MiB limit.
• Shutdown / cancellation ordering: wg.Add happens under the queue mutex and canStartLocked checks shutdown, so no Add-after-Wait race; pending builds are cancelled and left on disk; Shutdown is idempotent; MarkComplete/cancel/wg.Done defer order is correct. main.go drains HTTP before BuildManager.Shutdown, so no new enqueues race shutdown. Goroutine accounting verified by the shutdown/await tests and the -race stress test.
• Context-value propagation: CreateBuild enqueues with context.WithoutCancel(ctx); TestManagerShutdown_InFlightBuild pins both value propagation and detachment from request cancellation. Dedup paths in EnqueueSerial create no leaked cancels (the WithCancel is created after the dedup checks).
• Hard-fail secret protocol: both sides fail closed — host validates IDs (ValidateSecretID at API boundary and in FileSecretProvider), reports provider errors and missing values via secrets_response.error; guest fails on host error, on missing IDs in the response, on the 30s timeout, and backstops with a per-secret os.Stat (which also covers the "config not loaded yet, skipped secrets" path in handleSecretsRequest). NoOpSecretProvider's empty map is caught by missingSecretsError on the host.
• Crash recovery conservatism: deleteLeftoverBuildVolume only touches builder-<buildID> by exact name and refuses to force-delete a volume attached to an unknown holder; CreateVolumeFromArchive returns ErrAlreadyExists before consuming the reader (verified in lib/volumes/manager.go), so the source-reader reuse on retry is sound. The #337 Bugbot finding (config copy-over fallback masking a failed recreate) is fixed here and covered by TestRegisterBuildConfigVolume_RecreateFailure.
• Corruption observability / API compat / scope: listAllBuilds logging tested; AllowedDomains was never exposed in the OpenAPI spec (Go-only field, stale JSON keys unmarshal harmlessly); diff stays in scope.

Non-blocking notes (low)

1. lib/builds/manager.go runBuild + TestManagerShutdown_InFlightBuild: a build cancelled by graceful shutdown is persisted as failed with a raw context canceled-style error. Deliberate and pinned by test, but the user-facing failure reason for "server restarted" is opaque; consider mapping shutdown cancellation to a clearer error message (or leaving such builds in building for startup recovery, which the leftover-volume handling now supports).
2. cmd/api/api/builds.go readLimitedPart: the underlying io.ReadAll error is dropped ("failed to read %s field"); logging the cause would help debug malformed multipart bodies. Matches prior behavior, so purely a nice-to-have.
3. lib/builds/manager.go waitForResult: after the final failed dial attempt the loop still runs the instance check and one full 2s retry sleep before exiting with the "after retries" error — one wasted interval, cosmetic.

No blocking, high, or medium correctness or test findings remain. Fable 5 is satisfied.

[hiroTamada]

Follow-up on the round-1 Fable notes, addressed in 18fafa4:

1. Shutdown-cancelled builds marked failed — this was also a recovery regression vs main: a graceful shutdown failed in-flight builds with an opaque context canceled and they were never retried, while a crash left them building for RecoverPendingBuilds. runBuild now detects run-context cancellation (only Shutdown cancels it; request contexts are detached) and leaves the build in its non-terminal status so the next start re-runs it — safe now that leftover source/config volume recovery lands in this PR. TestManagerShutdown_InFlightBuild re-pinned to assert building + no error.
2. readLimitedPart dropping the read error — the 400 message now wraps the underlying error.
3. waitForResult trailing sleep — the loop breaks before the retry sleep on the final attempt.

go test -race -count=1 -tags containers_image_openpgp ./lib/builds/..., the cmd/api/api Build tests with -race, go vet, gofmt, and make oapi-generate (no drift) all pass.

[hiroTamada]

Fable 5 final review — post-Ralph pass

Re-reviewed the new head 18fafa4 ("builds: leave shutdown-interrupted builds for startup recovery") on top of the round-1 head, plus all prior comments.

Ralph-pass changes verified

• Shutdown-interrupted builds now recover instead of failing opaquely (lib/builds/manager.go runBuild): the ctx.Err() != nil gate is precise — the run context is only ever cancelled by queue Shutdown once a build has started (pending-cancel removes the build before StartFn runs; the goroutine's deferred cancel fires after runBuild returns). Crucially, the check is on the outer run ctx, not buildCtx (WithTimeout), so genuine build timeouts still mark the build failed rather than being mistaken for shutdown. User CancelBuild on a running build works via instance deletion, not context cancellation, so the terminal-state protection path is unchanged (still pinned by TestUpdateBuildComplete_PreservesTerminalStatus). The same gate on the waitForImageReady failure branch is consistent. TestManagerShutdown_InFlightBuild updated to pin StatusBuilding + nil error, matching crash semantics that RecoverPendingBuilds + the leftover-volume recovery from this PR make safe to re-run.
• readLimitedPart now wraps the underlying read error — clearer 400s, nothing sensitive leaked.
• waitForResult skips the trailing retry sleep after the final dial attempt; conn == nil error path still carries the last dial error.
• cmd/api/main.go / Manager.Shutdown doc comments updated to match the new semantics.

Validation on 18fafa4

• go build -tags containers_image_openpgp ./..., go vet, gofmt — clean
• go test -race -count=1 -tags containers_image_openpgp ./lib/builds/... — pass
• go test -race -count=1 -tags containers_image_openpgp -run 'Build' ./cmd/api/api/ — pass
• make oapi-generate — zero drift in lib/oapi/oapi.go

All round-1 notes are resolved and no new issues were introduced. No blocking, high, or medium findings remain. Fable 5 is satisfied after the Ralph pass.