[Epic] Snapshot GC improvements

[juli4n]

Parent issue to track follow up improvements for snapshot management / GC.

[EItanya]

While reviewing the workflow races in #1443 I found another one related to snapshot cleanup. Suspend deletes the old snapshot before saving the reference to the new one. If worker deletion crashes the actor in between, that update fails, leaving it pointing to the deleted snapshot and the new one unreferenced.

Should this epic be limited to features, or do we want to include potential bugs as well?

[laoj2]

Should this epic be limited to features, or do we want to include potential bugs as well?

Let's include potential bugs too Eitan Yarmush (@EItanya)

[laoj2]

Suspend deletes the old snapshot before saving the reference to the new one.

suspend saves a reference to the new snapshot here:

substrate/cmd/ateapi/internal/controlapi/workflow_suspend.go

Line 154 in 4003ad4

toUpdate.Status.InProgressSnapshotName = name

This runs before we delete the old snapshot:

substrate/cmd/ateapi/internal/controlapi/workflow_suspend.go

Line 417 in 4003ad4

err = w.releaseReplacedSnapshot(ctx, latestActor, externalSnapshot)

If worker deletion crashes the actor in between, that update fails, leaving it pointing to the deleted snapshot and the new one unreferenced.

The new snapshot uri is referenced in Actor.in_progress_snapshot_name, and the old snapshot (if any) is referenced in Actor.external_snapshot.snapshot_uri.

The worker delete workflow does clean Actor.in_progress_snapshot_name up in the actor:

substrate/cmd/ateapi/internal/controlapi/workflow_worker_delete.go

Lines 216 to 218 in 4003ad4

	// Both in-progress checkpoints die with the worker: the durable one was
	// never uploaded, the local one lived on the node that went away.
	toUpdate.Status.InProgressSnapshotName = ""

If an actor is CRASHED, it can't be resumed/paused/suspended anymore, it can only be deleted. That deletion will clean up everything under the prefix of Actor.external_snapshot.snapshot_uri and Actor.in_progress_snapshot_name (the latter will be empty if the underlying worker was deleted). So, we risk leaking a snapshot if the actor crashes due to a worker deletion workflow during the first suspend of an actor. I'll send a fix.