ateapi,atelet: reclaim actor node state when a worker's pod is gone by sairajp-rewind · Pull Request #2797 · agent-substrate/substrate

Sairaj Pokale (sairajp-rewind) · 2026-10-17T06:43:59Z

What is this PR and Why we need it ?

A deleted worker pod is deregistered by workersync, which calls DeleteWorker to mark its assigned actors CRASHED and clear their WorkerAssignment. Those actors are never terminated on the node, so /var/lib/ate/actors/<uid> and /var/lib/ate/snapshots/<uid> stay on disk until the actor is later deleted or reverted, if it ever is. (Note: this covers worker-pod removal via DeleteWorker; tearing down a crashed sandbox on a still-live worker is tracked separately in #2355.)

Changes

atelet workerless / gone-worker Terminate:
- Skips TerminateWorkload when worker_pod_uid is empty, the worker socket is gone, sandbox-assets.json is absent (Run and Restore write it before RunWorkload/RestoreWorkload, and an earlier Terminate removes it with the actor directory), or TerminateWorkload returns Unavailable/NotFound. Any other error from a live worker fails Terminate before touching disk.
- Volume unmount failures in Terminate return Internal without %w-wrapping downstream gRPC errors, so a transient CSI failure does not surface to ateapi as Unavailable (and reaches DeleteActor/RevertActor callers as Internal instead of Unavailable).
ateapi (DeleteWorker):
- DeleteWorker takes each actor's lease (ABORTED if held, requeued by workersync), skips actors that already reached SUSPENDED, and calls best-effort atelet.Terminate with a 30s timeout to reclaim the actor's node state and local snapshots. Once a call times out (DeadlineExceeded) or returns Unavailable, remaining actors on that worker skip the node and leave cleanup to revert/delete.

Testing

Unit & functional tests: go test ./cmd/atelet/... ./cmd/ateapi/internal/controlapi/...
Live-cluster verification (3 × c3-standard-4 gvisor nodes, 3 × c3-highcpu-4 microvm nodes):

Suite	`gvisor` (3 × `c3-standard-4`)	`microvm` (3 × `c3-highcpu-4`)
`durdir_data_baseline` 1m smoke (8 MiB `glutton-durdir-data`)	`20` reqs, `0` fails (`0.00%`)	`15` reqs, `0` fails (`0.00%`)
`DeleteWorker` scale-down reclaim (3 actors, 256 MiB `durable-dir`/actor)	`/var/lib/ate/actors`: `0.00 GB -> 1.41 GB (3 dirs) -> 0.00 GB (0 dirs)`; `-0.25 GB`/node reclaimed (`+0.00 GB` vs baseline)	`/var/lib/ate/actors`: `0.00 GB -> 1.64 GB (3 dirs) -> 0.00 GB (0 dirs)`; `-0.25 GB`/node reclaimed (`+0.00 GB` vs baseline)
`DeleteActor` on paused actor with no worker (`worker_pod_uid=""`)	`local-checkpoint` pruned & `/var/lib/ate/actors/<uid>` removed (`1.75 ms`)	`local-checkpoint` pruned & `/var/lib/ate/actors/<uid>` removed (`9.45 ms`)

Fixes #1688

Tests pass
Appropriate changes to documentation are included in the PR

igooch

The ateapi side looks good to me. Taking the actor's lease, and giving up on a node after one timeout, both make sense. I have two questions inline on the keep branch in Terminate, and three asks on the description.

Please fix the PR description so it matches the code.

The first paragraph says DeleteWorker clears the actor's AssignedNode. It doesn't, and it shouldn't. The crash keeps AssignedNode so a later delete or revert can still clean up the node, and this PR relies on that when it skips a reclaim.
One bullet says delete and revert no longer skip Terminate when worker_pod_uid is empty. Main already sends Terminate in that case. The only change here in ensureAteletTerminated is moving the fallback spec into fallbackWorkloadSpec. Please drop the bullet.
Fixes #<issue_number_goes_here> is still the placeholder. This is #1688.

#2871 moves resetActorDirs into the snapshottracker package. Whichever of the two PRs merges second needs a one-line rename in the keep branch.

Sairaj Pokale (sairajp-rewind) · 2026-10-20T03:37:05Z

Traced both questions against what landed on main over the weekend (#2789, #2795, #2804, #2810, #2819), and I think we should drop keep_local_snapshots and have DeleteWorker send a full Terminate:

Nothing uses the kept snapshot or in-flight upload today: releaseBoundActor holds the actor lease and skips SUSPENDED actors, so every actor it reclaims transitions to CRASHED. While CRASHED, ateapi refuses ReportActorSnapshotState with FailedPrecondition (snapshot_report.go:64-65), which atelet treats as terminal (report.go:345-352). RevertActor then sends a full Terminate (workflow_revert.go:188-195), deletes every PENDING durable snapshot from object storage (workflow_revert.go:209-224), and drops all local snapshots (workflow_revert.go:287).
Q1 (no detach scheduled): Since BeginStart clears DetachAt (lifecycle.go:173-182), keeping the snapshot leaves local-checkpoint/ and sandbox-assets/ on disk indefinitely if the crashed actor is never reverted or deleted. Scheduling a detach here would wait 30m and first upload a snapshot that RevertActor later deletes before freeing local disk.
Q2 (background upload left running): Leaving the upload running is safe for durable-snapshot-gc (since DeleteWorker deletes no objects and RevertActor/DeleteActor send a full Terminate before deleting anything), but pointless because the upload lands in a PENDING prefix that RevertActor or DeleteActor will delete.

Sending a full Terminate from DeleteWorker stops any background upload immediately via Forget, cleans up local-checkpoint/ and the actor dir right away, and drops keep_local_snapshots from TerminateRequest (along with the #2871 resetActorDirs conflict).

Does dropping keep_local_snapshots and running a full Terminate here sound good ? igooch Michelle Au (@msau42)

igooch · 2026-10-20T17:50:02Z

Does dropping keep_local_snapshots and running a full Terminate here sound good ?

Agreed, drop keep_local_snapshots and send a full Terminate. Nothing can use the kept snapshot once the actor is CRASHED.

A few asks:

In case delete or revert sends a second Terminate after DeleteWorker's. Please add a test for both cases:
1. The first Terminate failed partway, for example on a populated volume dir. The second removes what's left.
2. The first Terminate finished. The second succeeds with nothing to do.
The ENOENT comment on sandbox-assets.json says no sandbox was ever started. Restore writes the record after RestoreWorkload, so that isn't always true. Please reword it.
atelet, nodepath: separate per-actor snapshot directories under /var/lib/ate/snapshots #2746 is about to merge and moves the snapshot dirs to /var/lib/ate/snapshots/<uid>/. If that merges first please rebase onto it, and have a test check both trees are gone.
A crash leaves the actor's sandbox running on its worker; a gVisor re-host then loses track of it #2355 (a crash leaves the sandbox running on a live worker) isn't fixed here. Please make sure the description doesn't suggest it is.

Michelle Au (@msau42), with the full Terminate, an async-suspend snapshot whose successful upload wasn't reported before the crash is lost. That's already true on main, since ateapi refuses the report while CRASHED and revert deletes the PENDING prefix. Is that intended, or should it be recoverable?

Sairaj Pokale (sairajp-rewind) · 2026-10-20T19:41:36Z

Thanks @igooch! Rebased onto latest main (872ab207) and updated in 9aaa731f:

Dropped keep_local_snapshots: reverted the proto/validation changes and removed keep_local_snapshots / hasLocalSnapshots so DeleteWorker (workflow_worker_delete.go:358-366) and Terminate (main.go:1705-1757) always run the full teardown.
Repeated Terminate tests: updated TestTerminateCanBeRetried to also checkpoint a local snapshot first and cover both a direct retry (worker_pod_uid set) and DeleteActor/RevertActor after DeleteWorker (worker_pod_uid="") when:
1. the first Terminate fails partway on a populated volume dir and the second removes what's left, and
2. the first Terminate finishes and the second succeeds as a no-op, checking that both ActorPath and LocalCheckpointsDir are gone (lifecycle_test.go:1291-1296).
Reworded ENOENT comment on readSandboxRecord: updated main.go:1800-1808 (and lifecycle_test.go:1141-1144) to note that Run writes sandbox-assets.json before RunWorkload, Restore writes it after RestoreWorkload, and an earlier Terminate removes it with the actor directory.
PR description & #2746: updated the PR description (Fixes #1688, noted #2355 is separate, and applied the line fixes). #2746 hasn't merged to main yet; if it merges first I'll rebase onto it and assert SnapshotsActorPath is gone too.

Michelle Au (msau42) · 2026-10-20T20:49:50Z

Michelle Au (@msau42), with the full Terminate, an async-suspend snapshot whose successful upload wasn't reported before the crash is lost. That's already true on main, since ateapi refuses the report while CRASHED and revert deletes the PENDING prefix. Is that intended, or should it be recoverable?

That is fine for now. RevertActor reverts the actor to its last known COMPLETED durable snapshot in ateapi and discards any node-local state and PENDING uploads. An async suspend only guarantees node-local persistence upon return; durability in object storage isn't committed until atelet finishes the background upload and reports it.

If the upload had already completed on atelet and only the report raced with the crash, we could potentially recover that snapshot (e.g., by accepting latest_durable reports while CRASHED or returning snapshot state on Restore/Terminate before Forget), but we can leave that as a future optimization. Having DeleteWorker run a full Terminate here makes sense.

Sairaj Pokale (sairajp-rewind) · 2026-10-21T16:42:52Z

@igooch Rebased onto #2746 (57443d36) in 9c076b24 and updated both TestTerminateReclaimsStateWithoutALiveSandbox and TestTerminateCanBeRetried to assert that both ateletpath.ActorPath (/var/lib/ate/actors/<uid>) and ateletpath.ActorSnapshotPath (/var/lib/ate/snapshots/<uid>) are removed.

igooch

The ateapi side still looks good to me. I have one behavior fix in Terminate and a few small cleanups inline.

igooch

🤖 All five comments are addressed, thanks. One test now fails in CI, inline. The description's first bullet also still says Restore writes the record after RestoreWorkload, so please update it to match.

igooch

LGTM.

Michelle Au (@msau42) could you please review?

A deleted worker pod is deregistered, and its bound actors are marked CRASHED, but their node state is never terminated, so actors/<uid> and snapshots/<uid> stay on the node until the actor is deleted or reverted, if it ever is. The worker-delete workflow now sends atelet's Terminate before releasing each bound actor. It is best-effort, so an unreachable node cannot wedge deregistration. Each Terminate gets 30s, so a hung unmount cannot run out the delete's deadline. Once one times out or cannot reach atelet, other actors on that node skip Terminate for 30s across subsequent worker deletes and are left to their revert or delete. Each release holds the actor's lease, so it never runs during a suspend. An actor whose lease is taken is left for a retry: DeleteWorker returns ABORTED, which workersync requeues. Terminate now skips workload teardown when the worker is gone (missing socket, no sandbox record, or UNAVAILABLE) while still reclaiming the actor's node state. Restore writes the sandbox record before RestoreWorkload, matching Run, so a missing record means no sandbox was started.

## What this PR does and why we need it ? When detaching an actor (either via background timer detach or explicit `DetachActor` RPC), `atelet` previously only pruned entries inside `/var/lib/ate/snapshots/<uid>/local-checkpoint/`, leaving the actor's snapshot directory (`/var/lib/ate/snapshots/<uid>`) and actor directory (`/var/lib/ate/actors/<uid>`) on the node indefinitely. Because `sandbox-assets/` holds hard links into the sandbox asset cache (`st_nlink > 1`), leaving the actor directory also permanently pinned sandbox binaries in the cache after detach. ### Changes - Move `ResetDir`, `ResetActorDirs`, `ResetActorDirsKeepingVolumes`, `RemoveActorFilesExceptState`, and `RemoveActorDirs` into `cmd/atelet/internal/actordirs` so `Atelet` and `snapshottracker.Tracker` share directory cleanup logic (preserving populated external volume mount directories, removing `ateletpath.ActorSnapshotPath`, and handling read-only bundle upper directories via `imagecache.RemoveAllWritable`). - Update `Tracker.finishDetach` to call `actordirs.RemoveActorFilesExceptState` (removing `/var/lib/ate/snapshots/<uid>` and all entries under `/var/lib/ate/actors/<uid>` except `snapshot-state.json`) while `opDetach` is active, and mark a detach report due (`markReportDueLocked`) for both background and foreground `Detach` calls. - Keep `snapshot-state.json` (with `ReportDueSince`) on disk until the detach report finishes so a retried `DetachActor` RPC still sees `LatestDurable` and a pending detach report survives an `atelet` restart. - Remove `snapshot-state.json` and the empty `/var/lib/ate/actors/<uid>` directory (via `removeDetachedActorState` in `finishReportDueLocked`) once `!Started && LatestLocal == nil && Uploading == nil` and the control plane acknowledges the detach report or refuses it with `NotFound`. On give-up or any other refusal (`Unimplemented`, `FailedPrecondition`, `InvalidArgument`), `snapshot-state.json` stays on disk with `ReportDueSince` cleared so `DetachActor` keeps returning `LatestDurable`. - Require `--snapshot-report-workers >= 1` in `validateSnapshotWorkerFlags` so background detach reports are always sent. - In `cleanUpAfterCheckpoint`, call `actordirs.RemoveActorFilesExceptState` instead of `actordirs.ResetActorDirs` when no local snapshot is kept on the node, so empty subdirectories are not recreated. - Document in `atelet.proto` (`ActorSnapshotState.latest_durable` and `DetachActorResponse.state`) that `DetachActor` queues a background detach report and forgets the actor once acknowledged. > **Merge order**: Land after agent-substrate#2797 so `Terminate` tolerates a missing `sandbox-assets.json` after `cleanUpAfterCheckpoint` calls `RemoveActorFilesExceptState`. - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch from 16fe427 to fcb4525 Compare October 17, 2026 07:13

Luomu (cloudonly) mentioned this pull request Oct 19, 2026

DeleteWorker's bound-actor sweep reads the actor without its lease, so an in-flight resume's fresh claim is skipped and then cascade-deleted #2860

Closed

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch from fcb4525 to 10bed3c Compare October 19, 2026 05:37

Sairaj Pokale (sairajp-rewind) requested a review from igooch October 19, 2026 16:02

Sairaj Pokale (sairajp-rewind) marked this pull request as ready for review October 19, 2026 16:47

Bowei Du (bowei) added area/api-machinery area/node area/reliability kind/bug Something isn't working / bugfixes labels Oct 19, 2026

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch 2 times, most recently from 48e086b to 8b56220 Compare October 19, 2026 20:32

igooch reviewed Oct 19, 2026

View reviewed changes

Comment thread cmd/atelet/main.go Resolved

Comment thread cmd/atelet/main.go Resolved

Sairaj Pokale (sairajp-rewind) requested a review from Michelle Au (msau42) October 20, 2026 16:01

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch from 8b56220 to 9aaa731 Compare October 20, 2026 19:31

Michelle Au (msau42) reviewed Oct 20, 2026

View reviewed changes

Comment thread cmd/ateapi/internal/controlapi/workflow_worker_delete.go Resolved

Comment thread cmd/atelet/lifecycle_test.go Resolved

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch 2 times, most recently from 0a87a28 to 9c076b2 Compare October 21, 2026 16:42

igooch reviewed Oct 21, 2026

View reviewed changes

igooch mentioned this pull request Oct 21, 2026

atelet: Clean up whole actor directory on detach #2871

Merged

2 tasks done

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch 3 times, most recently from 5ba10b4 to 193636e Compare October 21, 2026 19:39

igooch reviewed Oct 21, 2026

View reviewed changes

Comment thread cmd/ateapi/internal/controlapi/workflow_worker_delete_test.go Resolved

igooch approved these changes Oct 21, 2026

View reviewed changes

Sairaj Pokale (sairajp-rewind) requested a review from Michelle Au (msau42) October 21, 2026 20:11

Michelle Au (msau42) reviewed Oct 21, 2026

View reviewed changes

Comment thread cmd/atelet/main.go Resolved

Comment thread cmd/ateapi/internal/controlapi/workflow_worker_delete.go Resolved

Comment thread cmd/ateapi/internal/controlapi/workflow.go Resolved

Comment thread cmd/atelet/main.go Resolved

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch from 193636e to b5f1ebf Compare October 21, 2026 21:41

Sairaj Pokale (sairajp-rewind) requested a review from Michelle Au (msau42) October 21, 2026 21:51

Michelle Au (msau42) previously approved these changes Oct 21, 2026

View reviewed changes

Michelle Au (msau42) added this pull request to the merge queue Oct 21, 2026

github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 21, 2026

Sairaj Pokale (sairajp-rewind) dismissed Michelle Au (msau42)’s stale review via a18d3a6 October 21, 2026 22:36

Sairaj Pokale (sairajp-rewind) force-pushed the durdir-gc-pr1 branch from b5f1ebf to a18d3a6 Compare October 21, 2026 22:36

Michelle Au (msau42) approved these changes Oct 21, 2026

View reviewed changes

Michelle Au (msau42) enabled auto-merge October 21, 2026 22:38

Michelle Au (msau42) added this pull request to the merge queue Oct 21, 2026

Merged via the queue into agent-substrate:main with commit e8a0719 Oct 21, 2026
15 checks passed

Scott Funkenhauser (sfunkenhauser) mentioned this pull request Oct 22, 2026

[Bug]: Actors stay DELETING forever when runsc's _pause state file is truncated or the sandbox record is missing #2011

Closed

Chenyi Wang (chw120) mentioned this pull request Oct 22, 2026

Preserve committed local snapshots when reverting actors #3041

Open

Luomu (cloudonly) mentioned this pull request Oct 23, 2026

ateapi: read the actor under its lease in DeleteWorker's bound-actor sweep #2869

Open

Suresh Attaluri (whysosuresh) mentioned this pull request Oct 25, 2026

KubernetesAgentSubstrate: install Agent Substrate v0.6.0 plantonhq/planton#638

Closed

Conversation

Sairaj Pokale (sairajp-rewind) commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR and Why we need it ?

Changes

Testing

Uh oh!

igooch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sairaj Pokale (sairajp-rewind) commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igooch commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sairaj Pokale (sairajp-rewind) commented Oct 20, 2026

Uh oh!

Michelle Au (msau42) commented Oct 20, 2026

Uh oh!

Uh oh!

Uh oh!

Sairaj Pokale (sairajp-rewind) commented Oct 21, 2026

Uh oh!

igooch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igooch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igooch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Sairaj Pokale (sairajp-rewind) commented Oct 17, 2026 •

edited

Loading

Sairaj Pokale (sairajp-rewind) commented Oct 20, 2026 •

edited

Loading

igooch commented Oct 20, 2026 •

edited

Loading