Repository navigation
Preserve actors when stale snapshot requests are rejected - #2819
Zoe Zhao (zoez7) merged 3 commits into
Conversation
bdf1fb7 to
aabca34
Compare
|
|
||
| // A stale snapshot request conflicts with atelet's newer state, without | ||
| // indicating that the actor failed. Other RPCs keep their existing policy. | ||
| func snapshotRPCConflict(rpc string, err error) bool { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – Please drop snapshotRPCConflict and keep ateletErrorCrashesActor/handleAteletError as they were. Matching on RPC name strings is fragile, because a typo or rename silently brings back the crash. It also adds per-RPC policy to a helper every caller shares, and the check runs twice. The "PromoteSnapshot" branch is dead too, since ateapi never calls PromoteSnapshot.
Checkpoint has a single caller, so the check can go inline where its error is handled in ensureAteletSuspended (workflow_suspend.go):
resp, err := client.Checkpoint(ctx, req)
if err != nil {
// atelet already holds a newer snapshot of the actor.
if status.Code(err) == codes.Aborted {
return wireFidelity, nil, apierror.Aborted("while calling atelet Checkpoint: %w", err)
}
return wireFidelity, nil, handleAteletError(ctx, w.store, actorRef, ateattr.OperationSuspend, "Checkpoint", false, err)
}There was a problem hiding this comment.
Removed snapshotRPCConflict and restored the shared helpers. Aborted is now handled at the Checkpoint call site, preserving caller cancellation. Main's new PromoteSnapshot caller uses its dedicated error mapper.
| wantState: ateapipb.ActorState_ACTOR_STATE_RUNNING, | ||
| }, | ||
| { | ||
| name: "wrapped PromoteSnapshot conflict leaves the actor as it was", |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The PromoteSnapshot cases here cover a path production never takes, because no ateapi code passes "PromoteSnapshot" to handleAteletError. Once the check moves into ensureAteletSuspended, these new cases no longer belong in TestHandleAteletError. Please replace them with a table-driven unit test in workflow_suspend_test.go that calls SuspendActor through a capturing atelet. It should check that Aborted leaves the actor SUSPENDING with its worker assignment, that FailedPrecondition still crashes it, and that a retry sends the same snapshot and finishes.
There was a problem hiding this comment.
Replaced these cases with table-driven SuspendActor tests through the capturing atelet, covering sync/async conflicts, crash behavior, cancellation, and retries with the same snapshot. The race tests pass.
| // TestCheckpointFailureCrashes: an atelet Checkpoint error while pausing or | ||
| // suspending a running actor crashes it, records atelet's error text, and | ||
| // releases its worker. | ||
| func TestSuspendActor_SnapshotConflictPreservesState(t *testing.T) { |
There was a problem hiding this comment.
🤖 should-fix 🟡 – This test is inserted between TestCheckpointFailureCrashes's doc comment and its function. The "...crashes it" comment now sits above a test that checks the actor is not crashed, and TestCheckpointFailureCrashes has no comment. Please move the comment back and give this test its own.
There was a problem hiding this comment.
Moved the crash-test comment back and added a separate comment for the conflict regression test.
| t.Errorf("state after retry = %v, want SUSPENDED", got) | ||
| } | ||
| if got := finished.GetActor().GetStatus().GetLastAssignedGeneration(); got != pending.GetGeneration() { | ||
| t.Errorf("generation after retry = %d, want %d", got, pending.GetGeneration()) |
There was a problem hiding this comment.
🤖 should-fix 🟡 – The retry is only checked through LastAssignedGeneration, which a retry with a new UUID at the same generation would also pass. Please assert that tc.fakeAtelet.CheckpointRequest.GetSnapshot() carries the pending snapshot's UUID and generation. For the repeated conflict, Reset() and then check CheckpointCalled, so the test shows the second Aborted came from atelet.
There was a problem hiding this comment.
The repeated conflict and successful retry now reset the fake, assert CheckpointCalled, and compare the wire snapshot's UUID and generation with the pending snapshot. The functional regression passes.
Keep RPC-specific retry policy out of shared crash handling. Exercise checkpoint errors through suspend workflows and verify retries reuse the persisted snapshot identity.
Retain the snapshot conflict retry guidance alongside paused snapshot uploads. Map conflicts at the tag upload boundary while keeping shared crash policy unchanged.
Late Checkpoint and PromoteSnapshot requests can conflict with a newer snapshot on atelet. Return
Abortedfor stale generations and same-generation UUID conflicts, and have ateapi preserve the actor and its worker assignment while reporting the conflict to the caller. OtherFailedPreconditionerrors retain their existing crash policy.Checkpoint conflicts are handled at the suspend call site. CreateTag maps PromoteSnapshot conflicts in its own upload error handler.
Both ateapi and atelet must include this change to avoid crashing actors on snapshot generation conflicts; mixed versions retain the previous crash behavior.
Regression tests cover the atelet error-conversion boundary, in-flight and background-operation conflicts, caller cancellation, and synchronous/asynchronous SuspendActor recovery. The API test keeps the last completed snapshot and its objects through repeated conflicts, then retries the same pending snapshot after the conflict clears. The API guide describes the retry behavior and its limits.
Fixes #2808.
Validated
13e53825, including mainddb46651, on Linux/amd64 with Go 1.27.2 as a non-root user withCI=true.go test -race -p=4 -count=1 ./...passed: 167 packages, 10,695 passing test and subtest results, 43 skips, and no failures.ateapiandateletpassed.hack/verify-all.shpassed all 16 checks. Regeneration and verification left the source tree clean.Root-gated tests and real runtime cluster E2E were not run.
AI assistance: Codex helped implement and test this change.