Skip to content

fix(api): delete snapshot build artifacts on snapshot deletion (stacked on #3797) - #3798

Open
AdaAibaby wants to merge 6 commits into
e2b-dev:mainfrom
AdaAibaby:fix/snapshot-artifact-cleanup
Open

fix(api): delete snapshot build artifacts on snapshot deletion (stacked on #3797)#3798
AdaAibaby wants to merge 6 commits into
e2b-dev:mainfrom
AdaAibaby:fix/snapshot-artifact-cleanup

Conversation

@AdaAibaby

@AdaAibaby AdaAibaby commented Nov 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Deletes the storage artifacts of a snapshot's exclusively-owned builds when the snapshot is deleted (DELETE /sandboxes/{id}), instead of orphaning them in GCS/S3.

A snapshot is backed by its own template, and deleteSnapshot already soft-deletes that template. This reuses the exact template-deletion cleanup for the snapshot's template, so the same guarantees apply.

Stacked on #3797. Review/merge #3797 first. This PR's own delta is one call site in sandbox_kill.go + a DB-backed exclusivity test.

Inherits the reachability guard

Because it reuses #3797's startTemplateArtifactCleanup, the snapshot path inherits the reachability guard, reconciliation metrics, and panic-safe/bounded/best-effort background behavior. The guard is what makes the base-deleted direction safe: if another live build reaches a base layer now DB-exclusive to this snapshot-template, it is kept, not orphaned. teamID is threaded through to scope the live-root set.

Test

DB-backed TestSnapshotArtifactExclusivity: the snapshot's own build and an extra exclusive build are returned for cleanup; a build shared with another template is excluded; the query resolves after soft-delete; every returned build carries the cluster_node_id needed to route DeleteBuild.

Update — rebased on #3797's checkpoint guard; snapshot-path test added

Rebased onto the updated #3797, so this PR inherits the checkpoint/snapshot live-root fix, the online grace window (TemplateArtifactMinAgeSeconds), and the pluggable fail-closed live-root sources.

Adds a DB-backed test on the snapshot path proving both halves of the checkpoint protection:

  1. deleting a snapshot never treats the base template's build as a deletion candidate (the base build is assigned to the base template, so GetExclusiveBuildsForTemplateDeletion for the snapshot's template excludes it), and
  2. the snapshot's own build is a live root when the base template is deleted, so the base build stays in the keep set instead of being orphaned.

Together with #3797's live-root source, deleting a checkpoint-derived snapshot removes only the snapshot's own COW diff layer and never the base it stacks on.

Stacked on #3797 (rebased onto its new tip). The handlers-package unit tests also require #3800 to run in CI; the DB-backed tests pass independently.

/cc @ryan-sana Looking forward to your code review.

@AdaAibaby
fix(api): delete GCS/S3 build artifacts on template deletion
afe0244 
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.
@cla-bot cla-bot Bot added the cla-signed label Nov 4, 2026
@AdaAibaby
fix(api): guard template-artifact deletion with header reachability +…
86e1451 
… 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
AdaAibaby force-pushed the fix/snapshot-artifact-cleanup branch from 24c8645 to caccd1a Compare November 5, 2026 08:38
@AdaAibaby AdaAibaby mentioned this pull request Nov 5, 2026
Open
@AdaAibaby
AdaAibaby force-pushed the fix/snapshot-artifact-cleanup branch from caccd1a to 03babfa Compare November 6, 2026 06:13
@AdaAibaby AdaAibaby mentioned this pull request Nov 6, 2026
Open
@AdaAibaby
fix(api): protect checkpoint/snapshot bases, add online grace window …
2cdaf3c 
…+ pluggable live-root sources

Three hardening changes to the template-artifact deletion reachability guard,
all aimed at the checkpoint-induced permanent-corruption vector and the
live-root/TOCTOU gaps around it.

1) Snapshot/checkpoint live-root source (corruption fix). A snapshot (paused
   sandbox / checkpoint) is a COW diff layer on top of a base template, so its
   header mapping references the base template builds -- but that edge lives
   only in object-storage headers, never in env_build_assignments. The base
   build therefore looks DB-exclusive and deletable. GetLiveBuildRoots... now
   UNIONs a third source: the team snapshot builds, joined via the snapshots
   table and deliberately NOT gated on active_envs. This keeps a base build in
   the keep set even after the snapshot own env is soft-deleted but its diff
   layer still lives in storage -- exactly the window ExistsTemplateSnapshots
   (which joins active_envs) can let a base deletion slip through.

2) Online deletion grace window (narrows TOCTOU). A DB-exclusive candidate
   younger than TemplateArtifactMinAgeSeconds (flag, fallback 24h; <=0 disables)
   is not deleted online; it is counted skipped_too_new and left to the offline
   sweeper. This narrows the window between the live-root query and DeleteBuild,
   where a fresh build may have just reused the candidate as a cache-hit
   ancestor before its protecting row/header landed. Reconciliation identity
   extended: candidates == deleted + delete_failed + skipped_nil_node +
   guard_kept + skipped_too_new (or fail_closed>0). New meter
   api.template.artifacts.skipped_too_new. Candidate query selects created_at.

3) Pluggable live-root sources, fail-closed on any source error. The keep-set
   build is refactored behind a liveRootSource interface (dbLiveRootSource is
   the sole production impl today; orchestrator/Redis-pinned roots can be added
   later). collectLiveRoots unions + dedups all sources and fails closed on the
   FIRST source error (classified live_roots_query), so a partial root set can
   never expand into an under-protected keep set.
@AdaAibaby
test(api,db): cover checkpoint live-root protection, online grace win…
1955e52 
…dow, fail-closed source union

DB-backed (get_live_build_roots_test.go): a snapshot build is a live root when
its BASE template is deleted; it stays a root even after the snapshot own env
is soft-deleted (the checkpoint corruption guard); another team snapshot is
never a root.

Unit (template_artifacts_test.go): a candidate younger than the grace window is
deferred (skipped_too_new) while an old one is deleted; grace disabled (0) still
deletes a fresh build. rowWithNode now carries an old CreatedAt so existing
cases are unaffected.

Unit (template_artifact_guard_test.go): collectLiveRoots unions+dedups multiple
sources and fails closed (live_roots_query) on any single source error.
@AdaAibaby
fix(api): delete snapshot build artifacts on sandbox snapshot deletion
2eb2e78 
A snapshot is backed by its own template, which deleteSnapshot already
soft-deletes. Reuse the template-deletion artifact cleanup for the snapshot's
template so its exclusively-owned build artifacts are removed from GCS/S3
instead of being orphaned.

Stacked on the reachability-guarded template cleanup: the snapshot path inherits
the same guard, reconciliation metrics, and panic-safe/bounded/best-effort
background behavior. The guard is what makes the base-deleted direction safe —
if another live build reaches a base layer now DB-exclusive to this
snapshot-template, it is kept, not orphaned. teamID is threaded through to scope
the live-root set the guard consults.

Test: DB-backed TestSnapshotArtifactExclusivity proves the snapshot's own build
and an extra exclusive build are returned for cleanup, a build shared with
another template is excluded, and the query resolves after soft-delete.
@AdaAibaby
test(api): snapshot deletion keeps the base it stacks on (checkpoint …
886fbe2 
…guard)

Complements the snapshot live-root fix on the base-delete direction. Adds a
DB-backed test proving both halves of the checkpoint protection on the snapshot
path: (1) deleting a snapshot never treats the base template's build as a
deletion candidate (the base build is assigned to the base template, so
GetExclusiveBuildsForTemplateDeletion for the snapshot's template excludes it),
and (2) the snapshot's own build is a live root when the base template is
deleted, so the base build stays in the keep set rather than being orphaned.
@AdaAibaby
AdaAibaby force-pushed the fix/snapshot-artifact-cleanup branch from 03babfa to 886fbe2 Compare November 6, 2026 06:26

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ValentaTomas ValentaTomas Awaiting requested review from ValentaTomas ValentaTomas is a code owner
@jakubno jakubno Awaiting requested review from jakubno jakubno is a code owner
@dobrac dobrac Awaiting requested review from dobrac dobrac is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@AdaAibaby