Repository navigation
feat(orchestrator): template-gc mark + operator-gated sweep (fail-closed, TOCTOU-guarded, metered) - #3742
Open
AdaAibaby wants to merge 2 commits into
Open
feat(orchestrator): template-gc mark + operator-gated sweep (fail-closed, TOCTOU-guarded, metered)#3742AdaAibaby wants to merge 2 commits into
AdaAibaby wants to merge 2 commits into
Conversation
Adds the mark (dry-run) phase of template-storage garbage collection: it identifies which build prefixes are unreferenced and old enough to delete, and writes manifests. It NEVER deletes anything — physical deletion is a separate, operator-gated step that consumes the candidate manifest after quarantine and revalidation. Why this and not a reverse-reference index: a per-build index used to drive deletion is safety-critical and can drift (incomplete backfill, a crash between header upload and edge write), which is exactly the permanent corruption this line prevents. The dry-run marker instead derives everything from ground truth at run time (live roots + header closure + a full bucket scan) and deletes nothing, which is the "go/no-go, not a delete command" posture the operation requires. Pieces: - storage: new optional ObjectLister capability (mirrors MetadataReader), implemented for the AWS/S3 and filesystem backends, plus a ListObjects helper that returns ErrListUnsupported when a backend cannot enumerate — so callers fail closed instead of treating an unlistable bucket as empty. ObjectInfo carries LastModified for the grace window. - internal/templategc: a pure, injectable Mark engine. Given live roots, an AncestorReader (header-mapping closure), the bucket object listing, and a grace period, it returns a manifest of Keep / Candidates / TooNew / UnknownKeys. It fails closed if any live root's closure cannot be resolved, protects a whole build prefix when any object under it is newer than the grace cutoff (possible in-flight upload), groups objects by UUID build prefix on the "/" boundary, and surfaces non-UUID keys as unknown (never deletes them). StorageAncestorReader is the production reader (memfile + rootfs headers, unions Mapping.Builds()). - cmd/template-gc: wires storage + the engine. Reads live-root build IDs from a JSON file (the control-plane root export is a separate, careful step), scans the bucket, writes keep/candidates/too-new/unknown manifests, prints a summary, and deletes nothing. Tests: engine unit tests cover keep-ancestor-of-live-root, orphan-is- candidate, recent-object-protects-prefix (grace), fail-closed-on-unreadable- root, and unknown-key surfacing; storage tests cover fs ListObjects (paths, sizes, LastModified, prefix scoping) and the ErrListUnsupported fail-closed path. Out of scope (deliberately): physical delete/sweep, control-plane root export wiring, TOS recovery verification. This is the mark phase only.
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
October 23, 2026 11:46
This was referenced Oct 26, 2026
… orphan metrics
The mark phase alone never reclaims anything, so it cannot back-stop the online
deletion path's two-step (DB soft-delete, then best-effort storage cleanup) gap:
an artifact orphaned by a crash between those steps stays forever. This adds the
sweep (delete) phase as the reconcile back-stop.
- Sweep engine (pure, injected PrefixDeleter): deletes the intersection of an
operator-approved candidates file and a FRESH mark run. A build is deleted
only if it is STILL a fresh candidate (unreferenced AND old enough); anything
that became referenced or too-new since the manifest was written is
SkippedChanged, never deleted. This closes the TOCTOU window between the mark
that produced the manifest and the sweep that consumes it, and inherits Mark's
fail-closed guarantee (no fresh candidates => nothing deleted).
- Operator-gated: sweep is a DRY RUN unless -confirm-delete is passed. A per-
build delete failure never aborts the others (retried next sweep).
- StoragePrefixDeleter wraps DeleteObjectsWithPrefix with the bare build ID,
matching every other caller (the storage layer rejects a trailing slash).
- Orphan metrics: -metrics-file writes Prometheus textfile gauges
(template_gc_orphan_bytes etc.) so the fleet can trend orphaned storage and
alert on sustained growth, instead of eyeballing a one-off JSON manifest.
- Fixes fs ListObjects for the stricter two-return getPath on main (branch was
stale) and the empty-prefix ("whole bucket") listing case.
Tests: sweep intersection, TOCTOU skip-changed, dry-run deletes nothing,
per-build failure non-fatal, fail-closed-on-no-candidates, metrics emission;
plus an end-to-end CLI run over a local fs bucket (mark -> dry-run -> confirmed
delete) that caught the trailing-slash delete bug. go build/vet/gofmt clean.
AdaAibaby
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Template-storage garbage collection for the template bucket, in two phases that both re-derive liveness from ground truth at run time (a stale reverse-reference index is more dangerous than reading headers — a dropped edge is permanent corruption):
-sweep): re-run the mark fresh, intersect it with an operator-approved candidates file, and delete only builds that are still unreferenced and old enough.Why the sweep exists
The online deletion path (#3797/#3798) is two-step: DB soft-delete, then best-effort storage cleanup. A crash between the steps orphans an artifact forever. Mark-only could never back-stop that. The sweep is the reconcile back-stop.
Safety
-confirm-delete. So an approved manifest that went stale cannot delete a build that became referenced since it was written.SkippedChanged, never deleted.Observability
-metrics-filewrites Prometheus textfile gauges (template_gc_orphan_bytes,template_gc_swept_bytes,template_gc_sweep_confirmed, …) so the fleet can trend orphaned storage and alert on sustained growth, instead of eyeballing a one-off JSON manifest.Pieces
storageObjectListeroptional capability (AWS paginated + fs),ErrListUnsupportedso callers fail closed on an unlistable bucket.internal/templategc: pure injectableMark+Sweepengines;StorageAncestorReader(memfile+rootfs header closure);StoragePrefixDeleter;WriteOrphanMetrics.cmd/template-gc: wires storage + engines + manifests + metrics; mark and operator-gated sweep.Tests
Engine: keep-ancestor-of-live-root, orphan-is-candidate, grace, fail-closed, unknown-key surfacing; sweep intersection, TOCTOU skip-changed, dry-run deletes nothing, per-build failure non-fatal, fail-closed-on-no-candidates, metrics emission. Storage: fs
ListObjects+ErrListUnsupported. Plus an end-to-end CLI run over a local fs bucket (mark → dry-run → confirmed delete) that caught a trailing-slash delete bug.go build/vet/gofmtclean.