Preserve system-info registrations during stale lifecycle cleanup - #1944

With this guard, a Run/Restore that fails before Register now preserves a pre-existing registration for the same actor. But resetActorDirs(actorUID) at the top of this same request already removed SystemInfoVolumeRootsDir(actorUID), so the preserved entry points at files this request deleted. refreshBundle only rewrites on a hash change, so nothing recreates the bundle until the next CTB rotation, and the entry keeps being iterated by projecting() indefinitely. The old unconditional Deregister cleared this case.

Scenario: stale entry for X (crashed worker, no Terminate) → Restore X runs resetActorDirs → ensureSandboxAssets fails before Register → defer skips → X's stale entry stays live with its trust files gone.

One option: have the pre-Register failure path still drop whatever entry is there (it is stale by construction once the dirs are reset), and only use the owner check after Register has run. Same applies to the Run defer above.

Thanks for the suggestion! Run and Restore remove the old registration before they reset the actor directories.

If Run fails before this step, it keeps the existing registration. If either operation fails after Register succeeds, cleanup removes only that operation’s registration.

This registers the "existing" volume under a separate t.TempDir() rather than ateletpath.SystemInfoVolumeRoot(actorUID, ...), which is where systemInfoVolumesFor would put it. So the test never models resetActorDirs deleting that root during the failed Restore, and the final readProjected(...) == certB check only passes because refreshBundle happens to MkdirAll and rewrite on the hash change. The intermediate state (registration live, files missing) is never asserted. Suggest using the real root and asserting the files right after the failed Restore, before rotating the CTB.

switched to use the actual path with SystemInfoVolumeRoot, moved checks right after failure and before bundle rotation

This never asserts that the failure happened after Register (the Restore twin below checks for while creating pause OCI bundle). If mountExternalVolumes, resetActorDirs, or ensureSandboxAssets starts failing in this fixture, or someone reorders Run so prepareOCIBundles precedes Register, Run still errors, actors["actor-uid-run"] is nil because Register never ran, and the test stays green without ever exercising DeregisterOwned on the Run path. Pin the error, or assert registration was non-nil.

Both tests now require the OCI preparation error and the projected ca.pem file. This ensures that the Register ran before the failure.

Returning a non-nil *registeredActor together with a non-nil error, after this block has already removed it from the map, means the callers' deferred DeregisterOwned(actorUID, registration) takes r.mu only to find a mismatch and return. Either return nil, err here (both callers already guard on registration != nil) and keep this self-cleanup, or drop the self-cleanup and let the defer do it. Doing both is redundant and the value-with-error return is easy to misread.

Related: Deregister, DeregisterOwned, and this branch now hand-roll the same "remove from r.actors under r.mu, then mark stale" sequence, with one of them setting stale under r.mu and the other two after releasing it. A single remove(uid, match func(*registeredActor) bool) helper would keep the three in step.

Register now removes its failed entry and returns nil, err. Kept this cleanup separate because Register already holds the actor lock. The other cleanup paths take that lock themselves.

When the initial write fails after this Register superseded a live prev, prev has already been marked stale and dropped, and nothing restores it. So a retried B for X that fails here leaves X with no registration at all, which contradicts the PR's stated invariant that failure cleanup is scoped to the operation's own registration. Pre-existing in effect, and in production prev's files were wiped by resetActorDirs anyway, so this may just warrant a comment rather than a fix.

Added a comment and test to make this behavior explicit: a failed replacement does not restore the previous registration. Run and Restore remove that registration before resetting its files.

owner.uid already carries the UID, so taking actorUID separately opens two silent failure modes: a mismatched pair (DeregisterOwned("uid-b", regForA)) compares the wrong slot, never matches, and leaves regForA live with no signal; and a nil owner for an absent UID passes the nil == nil guard, no-ops the delete, then panics on owner.mu.Lock(). Suggest DeregisterOwned(owner *registeredActor) that returns early on nil and keys off owner.uid.

DeregisterOwned now takes only the owner, uses owner.uid, and returns immediately for nil. Tests check that cleanup from an old owner preserves the newer registration.

This closes the stale-files case, but it also makes the underlying hazard sharper. Two overlapping activations for the same actorUID still destroy each other: if the control plane times out Run #1 while prepareOCIBundles is still unpacking and issues Run #2, this line marks Run #1's live registration stale and resetActorDirs deletes its half-written bundle. Run #1 then fails on a half-deleted tree, or if its RunWorkload already went out, leaves a running actor whose only registration was built from Run #2's reset.

As far as I can tell atelet has no per-actor lock around Run/Restore/Checkpoint/Terminate (the only keyed mutex in cmd/atelet is in filecache). A per-UID mutex around those bodies would remove the whole class, and Register's supersede branch plus a plain Deregister in the defer would be correct again, so the token API could go away.

Not asking for that in this PR. But if that is the intended end state, it would be good to say so, since otherwise DeregisterOwned reads as the fix rather than a stopgap.

I think we need to track this one in particular for follow-up. It's broader but should-fix.

This is correct today, but only because nothing returns between this defer and g.Wait() further down, and that constraint lives in the comment. A future early return nil, err in between (say a goldenRec validation moved below op.kind) makes the defer read registration while the prep goroutine may still be assigning it: a -race hit, and in the lost-write case a registration that is never cleaned up.

A shape that does not depend on the ordering: have the prep goroutine own cleanup of what it registered (call DeregisterOwned on its own error path), or install this defer only after g.Wait() and handle the Wait-failure branch explicitly.

Nit: registration != nil here (and at the Restore defer) duplicates the nil guard inside DeregisterOwned, which also has its own test. The call-site check is dead and tells the next reader the callee is not nil-safe. Keep one contract, either the callee guard or the call-site check.

Minor, non-blocking: Register can now fail after writing some volumes (say metadata succeeds, trust fails because the ClusterTrustBundle is not synced yet). It self-removes and returns nil, so nothing identifies or cleans the files it left under the freshly reset root. Harmless today because every activation resets first, but if Register ever gains a caller that does not reset, those become stale projections. Simplest hardening would be for Register to remove the roots it created in this call before returning the error.

This comment still motivates supersede-on-collision with the crash-recovery case, but after this PR Run and Restore Deregister before reset, so that path never reaches Register with a prev entry. The only remaining way the superseding branch fires is two activations overlapping for one UID. That is worth saying here, and the Info-level "Superseded a stale system-info volume registration" log probably deserves Warn now, since it signals the race rather than routine recovery.

Understood on keeping this separate because actor.mu is already held here. That said, there are now three copies of "delete map entry under r.mu, then mark stale" (here, Deregister, DeregisterOwned) with slightly different lock shapes. Splitting out just the map part, something like unlink(uid string, owner *registeredActor) *registeredActor that does the r.mu section and returns what it removed, would let all three share it while each caller handles stale under whatever lock state it is in.

This subtest passes a nil owner (the preceding subtest asserts that Register returns nil on failure), so DeregisterOwned returns at the nil guard and the ownership comparison it is named for is never reached. Replacing the r.actors[owner.uid] != owner check with an unconditional delete would not fail this test. The behaviour the name promises is covered by StaleDeregisterPreservesNewerRegistration already. Either drop this one as redundant with DeregisterOwnedNil, or rename it to say it checks that a failed Register yields a nil token whose cleanup is a no-op.