Repository navigation
Conversation
GetExclusiveBuildsForTemplateDeletion had no Go caller, so deleting a template only soft-deleted the DB row and left its build artifacts (rootfs/kernel/FC snapshots) in storage forever. Fixes e2b-dev#3239. - SQL: JOIN envs for cluster_id (route DeleteBuild to the right cluster); joins raw envs, not active_envs, so it resolves after soft-delete. - Handler: startTemplateArtifactCleanup runs after the DB commit in a detached goroutine that is panic-safe (recover), deadline-bounded (2m), and fans out with bounded concurrency (errgroup SetLimit 8). Best-effort: nil-node builds skipped, per-build failures never abort siblings, outcomes counted via new metrics so orphaned storage is alertable. Storage errors never affect the delete response. - Logging uses the internal logger API (logger.WithError/WithBuildID); no raw zap import in the handler (adds logger.WithError helper). - processExclusiveBuilds is DB-free and unit-tested: nil-node skip, error non-fatal, nil-cluster fallback, empty input. Metrics: api.template.artifacts.deleted / .delete_failed. Note: generated sql.go hand-updated (ClusterID) since sqlc is not in CI. Snapshot-deletion artifact cleanup is deliberately a separate follow-up.
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
November 4, 2026 02:02
… reconciliation metrics Deleting a template now deletes its GCS/S3 build artifacts safely. Being DB-exclusive is NOT sufficient: intermediate cache layers are never written to the DB, so a build that is DB-exclusive to the doomed template can still be a header-mapping ancestor of another live build. Deleting its prefix orphans that ancestor and surfaces later as a silent, unrecoverable Input/output error. Changes: - Reachability guard folded into processExclusiveBuilds: a candidate is deleted only if it is NOT reachable from any live root's memfile/rootfs header closure. Fails closed (deletes nothing) when the reader is unconfigured, the live-root set can't be queried, or any live root's header can't be read. - Live-root set completion: GetLiveBuildRootsForTeamExcludingTemplate now UNIONs the team's in-progress/pending builds (status_group) on top of other active templates' assigned builds, closing the gap where a concurrent build reusing the doomed template's exclusive ancestor had no assignment row yet. - Reconciliation metrics so orphaned storage is derivable, not guessed: candidates = deleted + delete_failed + skipped_nil_node + guard_kept (success), or fail_closed>0 when nothing was deleted. fail_closed carries a low-cardinality reason (reader_unconfigured|live_roots_query|header_unreadable) and is the alert signal. deleted/failed carry a cluster attribute. team_id/build_id stay on logs/trace, not metric attributes (cardinality). - Keeps the panic-safe / deadline-bounded / bounded-fan-out background cleanup. Supersedes the separate reachability-guard PR, which had the guard but lacked the background hardening and reconciliation metrics. Tests: processExclusiveBuilds guard/skip/fail paths, fail-closed classification, and DB-backed live-root tests (in-progress-without-assignment is a root, soft-deleted/ready-unassigned excluded, team-scoped). 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.
Problem
Deleting a template soft-deletes the DB row but left the build artifacts (rootfs, kernel, FC snapshots) in GCS/S3 forever.
GetExclusiveBuildsForTemplateDeletionexisted with zero callers. Fixes #3239.Why DB-exclusive is not enough (this is the important part)
A template build stores only the bytes it changed vs its ancestors; the full image is reconstructed by following the header
BuildMapinto parent/grandparent builds. Those intermediate layers are never written to the DB (a cache hit reuses prior layers under a new build ID). So a build that is DB-exclusive to template T can still be the header-mapping ancestor of another live template's build. Deleting its{buildID}/prefix orphans that ancestor — the DB looks consistent, the other sandbox even starts, and it only surfaces as a silent, unrecoverableInput/output errorwhen a missing block is read. No versioning ⇒ permanent.What this PR does
Reachability guard (folded into
processExclusiveBuilds): a DB-exclusive candidate is deleted only if it is not reachable from any live root's memfile/rootfs header closure. Fails closed (deletes nothing) if the ancestor reader is unconfigured, the live-root set can't be queried, or any live root's header can't be read — strictly no worse than leaving artifacts in place.Live-root set completion:
GetLiveBuildRootsForTeamExcludingTemplatenow UNIONs the team's in-progress/pending builds (status_group) on top of other active templates' assigned builds — closing the gap where a concurrent build reusing the doomed template's exclusive ancestor has noenv_build_assignmentsrow yet.Reconciliation metrics so orphaned storage is derivable, not guessed:
candidates == deleted + delete_failed + skipped_nil_node + guard_kept(success), orfail_closed > 0when nothing was deleted.fail_closedcarries a low-cardinalityreason(reader_unconfigured|live_roots_query|header_unreadable) and is the alert signal for "storage not being reclaimed".deleted/failedcarry aclusterattribute. team/build IDs stay on logs/trace, not metric attributes (cardinality).Background hardening kept: panic-safe, deadline-bounded (2m), bounded fan-out (8), best-effort; storage errors never affect the already-committed 204.
Supersedes #3740
#3740 had the reachability guard but on the pre-hardening base (raw goroutine, no recover/timeout/limit, no metrics). This PR unifies both: guard and hardening and reconciliation metrics. #3740 is closed in favor of this.
Tests
processExclusiveBuildsguard/skip/fail paths, fail-closed classification, and DB-backed live-root tests (in-progress-without-assignment is a root, soft-deleted/ready-unassigned excluded, team-scoped).go build/vet/gofmtclean; unit tests green; DB tests green against a real migrated Postgres.