Garbage Collect snapshots and remove the snapshot resource by laoj2 · Pull Request #1417 · agent-substrate/substrate

Luiz Oliveira (laoj2) · 2026-09-02T21:37:57Z

Fixes #664

This PR implements the idea described in #664 (comment)

It does more than Garbage Collection of snapshots, because we also got rid of the Snapshot resource (from the DB/API).

Now, an external snapshot is owned by a single resource:

An Actor owns the snapshot it writes at suspend
A tag owns a copy taken at tag creation,
An actor cloned from a tag borrows the tag's snapshot until its own first suspend.

Garbage Collection: whoever created/owns the snapshot is the only one who ever deletes them:
i.e., if an actor is deleted and it owns a snapshot. The underlying snapshot is deleted with the actor.

this PR:

Drops table actor_snapshots
Keeps table actor_snapshot_tags
Adds an object copy at tag creation, and an owned versus borrowed distinction on the Actor
Adds synchronous external snapshot deletion at actor suspend, at actor delete, and at tag delete

It's a good idea to open an issue first for discussion.

Tests pass
Appropriate changes to documentation are included in the PR

Julian Gutierrez Oschmann (juli4n) · 2026-09-02T22:40:50Z

-    version            bigint NOT NULL,
-    proto              bytea NOT NULL,
    PRIMARY KEY (atespace, name),
    CONSTRAINT actor_snapshot_tags_atespace_fk


Not for this PR, but I think we should use consistent syntax to define the foreign key on the parent table, as well as use consistent names for the constraint name.

Eitan Yarmush (EItanya)

So this PR is quite large so gonna leave an initial review. My biggest comment is about the storage owner. There's a lot of added code related specifically to giving ate-api access to the storage buckets but I would like to think through that, it may be necessary, but worth discussing.

Here are some correctness issues codex discovered, seem correct on first review from me:

## Review findings

### [P1] Serialize tag deletion with an active copy

`DeleteActorSnapshotTag` can list and delete a partial prefix, then remove the tag row while
`TagActorSnapshot` is still copying objects. Objects written after that listing become permanently
orphaned because the row holding their URI is gone.

Please serialize creation and deletion using a tag-specific lease.

### [P2] Delete the fetched tag instance, not just its name

The handler fetches a tag and cleans its snapshot, but then deletes by `atespace/name` without
checking the fetched UID or version.

With concurrent deletes and name reuse, a stale request can delete the replacement tag while
leaving its newly minted snapshot behind. Please make the store deletion conditional on the
fetched tag UID.

### [P2] Abort multipart uploads when completion fails

`copyMultipart` aborts after a part-copy failure, but returns directly when
`CompleteMultipartUpload` fails. S3 retains those uploaded parts and continues billing for them.

Please route completion failures through the same abort cleanup.

Eitan Yarmush (EItanya) · 2026-09-03T13:00:30Z

+  // cleared once snapshot is set, so a tag left with it set names exactly the
+  // objects its unfinished create stranded, and deleting the tag collects
+  // them.
+  string in_progress_snapshot_uri = 3;


This feels a little bit oddly specific. What about modeling this as a state enum?

We unfortunately need the in progress uri. The current sequence of tagging a snapshot is:

tx1: write in_progress_snapshot_uri to the desired URI

Copy snapshot to the URI

tx2: unset in_progress_snapshot_uri, and set external_snapshot.

So, if we fail in between 2 and 3 for some reason, we can recover from where it was left (and delete the partially copied files). If we only have a state enum we lose that ability

This idea was borrowed from what actor does too for both external and local snapshots:

substrate/pkg/proto/ateapipb/ateapi.proto

Lines 513 to 518 in a9c1bd3

// The name the in-progress durable snapshot will be stored under. Snapshot

// names are server-generated UUIDs, but any resource name is valid here.

//

// +k8s:optional

// +k8s:format=k8s-short-name

string in_progress_snapshot_name = 3;

Eitan Yarmush (EItanya) · 2026-09-03T13:01:27Z

+  // this Actor's Atespace.
+  //
+  // +k8s:opaqueType
+  ObjectRef actor = 1;


I understand this is not for now, but do we want to at least leave room in the API for actors having > 1 snapshot in their histrory

Eitan Yarmush (EItanya) · 2026-09-03T13:06:36Z

+        # ate-api copies external snapshots for tags and deletes the ones
+        # nothing refers to any more, so it needs the same backend atelet reads


This comment is not so much about this line of code, but rather conceptually allowing more component access to storage, is that something we want to do? Can we somehow make ate-api readonly for the actual actor buckets so it can't mess with those

It also means that the var value should match between the components (maybe obvious, though).

Can we somehow make ate-api readonly for the actual actor buckets so it can't mess with those

Right now, ate-api needs write access to delete buckets. If we move this functionality to atelet, we'll need to make sure an atelet is called during actor deletion. Today, this is not always the case.

If a suspended actor is deleted, we skip calling atelet because there's no worker assignment (so the control plane doesn't know which atelet to call and skips that). This is similar to the issue with GCing local snapshots too that I wrote in: #664 (comment)

So, we can consider moving the external snapshot deletion to atelet after that other problem is fixed, WDYT?

It also means that the var value should match between the components (maybe obvious, though).

Seems like we need a config struct somewhere that's shared at the very least, probably need that anway.

So, we can consider moving the external snapshot deletion to atelet after that other problem is fixed, WDYT?

I'm not sold either way, but definitely worth bikeshedding Julian Gutierrez Oschmann (@juli4n) and Tim Hockin (@thockin)

The control plane is about to start managing the lifetime of external snapshots: copying one when a tag is created, and deleting one when its owner lets go. That needs a handle on object storage, which ate-api has never had. Add internal/objectstore. Its Store interface addresses objects by name only -- list, delete, copy -- and the DeletePrefix and CopyPrefix helpers work a whole snapshot prefix at a time. Copies are server-side and deletes are by name, so a multi-gigabyte memory image never transits ate-api. That is what keeps this package separate from atelet's ategcs, which reads and writes the snapshot content itself. Both operations are safe to retry: deleting an object that is already gone succeeds, and a copy overwrites its deterministic destination rather than leaving a second copy behind. Callers can therefore treat an interrupted prefix operation as resumable. GCS and S3 backends are included, along with objectstoretest.Fake for tests that need to see which objects a flow created and released. smithy-go moves from an indirect to a direct dependency: the S3 backend matches on its API errors to tell "already gone" from a real failure.

Until now atelet was the only component that touched the snapshot bucket, so it was the only Workload Identity principal the bucket bindings named. ate-api is about to copy external snapshots for tags and delete the ones nothing refers to any more, which it cannot do without the same two roles. Grant objectAdmin and bucketViewer to ate-system/ate-api-server alongside atelet, in `create iam` and in the teardown that reverses it, and rework both to loop over (subject, role) rather than spell out four combinations. createIamPolicyBindings also gains hasUnconditionalBinding so the "already correct" check is per binding: the old code returned early as soon as one member had both roles, which with two members would have skipped the second one entirely. Point ate-api at a backend while we are here: ATE_STORAGE_BACKEND=gcs in the shipped manifest, and the rustfs S3 settings in the kind overlay so both ends of an external snapshot's life agree on where it lives.

…pshot An ActorSnapshot was a row of its own, addressable by (atespace, name), recording where in object storage a suspend had written and who it came from. Nothing ever needed that indirection: exactly one thing points at a snapshot at a time, and the row's provenance fields were never read back. What the indirection did buy was ambiguity about who is responsible for the objects, which is the thing standing in the way of ever collecting them. Collapse it into an ExternalSnapshot message -- a snapshot_uri and the content_scope it captured -- stored directly on whoever holds it: Actor.status.external_snapshot ActorSnapshotTag.status.snapshot ActorTemplate.status.golden_snapshot_status.golden_snapshot GetActorSnapshot goes away and ListActorSnapshots becomes ListActorSnapshotTags: tags are now the only named, listable handle on a snapshot. CreateActorSnapshotTag consequently names the Actor to tag rather than a snapshot resource that no longer exists. Two ActorStatus fields go with it. source_snapshot recorded which snapshot a clone was seeded from, as immutable provenance; it becomes current_snapshot_tag, which is cleared at the clone's first suspend and so answers the question the lifecycle actually asks -- does this Actor still borrow someone else's snapshot? The other, in_progress_snapshot_source_actor_version, existed only to stamp the ActorSnapshot row and has no reader left. The actor_snapshots table and the foreign key actor_snapshot_tags held into it are dropped from the initial migration. This is a breaking change to stored state; Substrate has no compatibility guarantee yet. No behavior changes here: snapshots are still never deleted, and a tag still just points at the snapshot its Actor holds. Collecting them is the following commits.

ate-api is about to copy and delete external snapshots, which it cannot do without a handle on object storage. Build one at startup and thread it through RPCService into ActorWorkflow, where the snapshot lifecycle steps live. The backend is selected from ATE_STORAGE_BACKEND the same way atelet selects the one it reads and writes snapshots through, so both ends of a snapshot's life agree on where it lives. GCS via ADC stays the default; "s3" is what the kind overlay points at rustfs with. The store is allowed to be nil, in which case the lifecycle steps skip and leave external snapshots in place. That is for the unit tests that never reach those steps -- ate-api itself always builds one, and fails startup if it cannot. Nothing consumes the field yet.

A tag used to be a pointer at whatever snapshot its Actor happened to hold. That is only sound while nothing ever deletes a snapshot, which stops being true in the next commit: the Actor's next suspend would collect the objects out from under every tag naming them. Give the tag a copy. CreateActorSnapshotTag becomes a workflow, because it now spans two transactions around an object copy: 1. Reserve -- mint a destination and record it on the tag row in status.in_progress_snapshot_uri, before a single object is written. 2. Copy -- copy the Actor's snapshot prefix, server-side. 3. Finalize -- move the destination into status.snapshot and clear the in-progress field. The reservation is what makes the whole thing recoverable. A tag left with in_progress_snapshot_uri set names exactly the objects its unfinished create stranded, so deleting that tag collects them; and a retry adopts the pending row and re-copies to the URI it already names rather than minting a second destination. Adoption is gated on status.source_actor_uid matching, because two different Actors resuming into one prefix would interleave their objects. Destinations are minted, never derived: NewSnapshotNameForTag returns a fresh `tag-`-prefixed UUID recorded on the row and only ever read back. Nothing can compute its way onto another tag's objects, and a recreated tag never inherits what its predecessor stranded. The prefix keeps the namespace disjoint from actor snapshots, whose names are bare UUIDs. A pending tag is not usable: CreateActor refuses to seed from one, and UpdateActorSnapshotTag refuses to publish one, since both would hand out content that is still being written or may never be. DeleteActorSnapshotTag releases the copy before dropping the row -- the row is the only handle on those objects, so the reverse order would leak them. The whole workflow holds the Actor's lease, which serializes it against a suspend of the same Actor.

Nothing has ever deleted a snapshot. Every suspend wrote a new prefix and abandoned the last, and deleting an Actor left all of them behind, so a bucket grew without bound and only ever shrank by hand. Now that every external snapshot has exactly one owner, collect them when that owner lets go: * A successful suspend releases the snapshot it replaces, in ensureSuspendedFinalized, before the commit overwrites the record that names it. * DeleteActor releases the snapshot the Actor holds, plus the one an interrupted suspend left mid-write, before the row is removed. Both run while the record still points at the objects, so an interrupted release is rediscoverable and the retry deletes whatever is left. The opposite order would drop the only handle on them. The cost is that a crash between the release and the commit can leave objects no row names; that is strictly better than losing the ability to ever delete them. An Actor that borrowed its snapshot from a tag releases nothing: status.current_snapshot_tag says the tag owns those objects and outlives the Actor. It stops borrowing at its own first suspend. One case still leaks, and is marked TODO: an in-progress snapshot whose ActorTemplate no longer resolves. The template's storage location is the only place its URI can be derived from, so without it the choice is between leaking the objects and wedging the Actor in DELETING forever.

wait_actortemplate_ready polled status.goldenSnapshotStatus.goldenSnapshot.name, which existed while golden_snapshot was an ObjectRef pointing at an ActorSnapshot row. It is now an inline ExternalSnapshot -- snapshotUri and contentScope, with no name -- so the expression yielded empty no matter what the reconciler recorded, and every caller waited out its full timeout on a template that had been ready for minutes. Poll snapshotUri instead. Both copies of the helper need it: the one in hack that the demos and the e2e job use, and the one the benchmarking workload deploy carries.

Actor-owned snapshots now live under: `<location>/atespaces/<atespace>/actors/<actor-UID>/snapshots/<snapshot-uid>` Tag owned snapshots now live under: `<location>/atespaces/<atespace>/actor-snapshot-tags/<snapshot-uid>` + Added helpers to build these URIs/prefixes + Removed current_snapshot_tag from the Actor (since ownership is now inferred from the snapshot path)

Luiz Oliveira (laoj2) requested a review from Julian Gutierrez Oschmann (juli4n) September 2, 2026 21:38

Luiz Oliveira (laoj2) force-pushed the snapshot-garbage-collection branch 2 times, most recently from 83339d7 to f1dcbb8 Compare September 2, 2026 22:39

Julian Gutierrez Oschmann (juli4n) reviewed Sep 2, 2026

View reviewed changes

Comment thread internal/resources/snapshot.go Outdated

Julian Gutierrez Oschmann (juli4n) reviewed Sep 2, 2026

View reviewed changes

Comment thread pkg/proto/ateapipb/ateapi.proto

Eitan Yarmush (EItanya) self-requested a review September 3, 2026 02:44

Eitan Yarmush (EItanya) reviewed Sep 3, 2026

View reviewed changes

Luiz Oliveira (laoj2) added 7 commits September 3, 2026 09:15

Luiz Oliveira (laoj2) force-pushed the snapshot-garbage-collection branch 2 times, most recently from 37dc297 to 9fdc282 Compare September 3, 2026 17:10

Luiz Oliveira (laoj2) force-pushed the snapshot-garbage-collection branch from 9fdc282 to 34a7a2b Compare September 3, 2026 17:13

Navigation Menu

Uh oh!

Garbage Collect snapshots and remove the snapshot resource - #1417

Garbage Collect snapshots and remove the snapshot resource#1417
Luiz Oliveira (laoj2) wants to merge 8 commits into
agent-substrate:mainfrom
laoj2:snapshot-garbage-collection

Luiz Oliveira (laoj2) commented Sep 2, 2026

Uh oh!

Julian Gutierrez Oschmann (juli4n) Sep 2, 2026

Uh oh!

Uh oh!

Uh oh!

Eitan Yarmush (EItanya) left a comment •

edited

Loading

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Uh oh!

Luiz Oliveira (laoj2) Sep 3, 2026

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Uh oh!

Luiz Oliveira (laoj2) Sep 3, 2026

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// The name the in-progress durable snapshot will be stored under. Snapshot
	// names are server-generated UUIDs, but any resource name is valid here.
	//
	// +k8s:optional
	// +k8s:format=k8s-short-name
	string in_progress_snapshot_name = 3;

		# ate-api copies external snapshots for tags and deletes the ones
		# nothing refers to any more, so it needs the same backend atelet reads

Uh oh!

Conversation

Luiz Oliveira (laoj2) commented Sep 2, 2026

Uh oh!

Julian Gutierrez Oschmann (juli4n) Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Eitan Yarmush (EItanya) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Luiz Oliveira (laoj2) Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Luiz Oliveira (laoj2) Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Eitan Yarmush (EItanya) Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Eitan Yarmush (EItanya) left a comment •

edited

Loading