Repository navigation
ateapi accepts snapshot report from atelet for actors in CRASHED state - #3148
Merged
Dmitry Berkovich (dberkov) merged 3 commits intoOct 23, 2026
Merged
Dmitry Berkovich (dberkov) merged 3 commits into
Dmitry Berkovich (dberkov) merged 3 commits into
Conversation
Zoe Zhao (zoez7)
changed the title
Zoe Zhao (zoez7)
force-pushed
the
crashed-snapshot-report
branch
2 times, most recently
from
October 23, 2026 03:29
27db651 to
358ff93
Compare
Zoe Zhao (zoez7)
requested review from
Dmitry Berkovich (dberkov) and
Michelle Au (msau42)
October 23, 2026 03:38
Zoe Zhao (zoez7)
marked this pull request as ready for review
October 23, 2026 03:38
Comment on lines
+69
to
+87
| case ateapi.ActorState_ACTOR_STATE_CRASHED: | ||
| return apierror.FailedPrecondition("actor %s is %s; revert it first", actorRef, st) | ||
| reported := state.GetLatestDurable() | ||
| snap := findSnapshotByGeneration(status.GetSnapshots(), reported.GetGeneration()) | ||
| // A crashed Actor keeps its AssignedNode, so only Locality tells a crashed | ||
| // suspend's snapshot from an upload that finished on node: only a finalized | ||
| // suspend sets Locality. Its upload is not recorded, and a revert deletes | ||
| // it with the rest of the discarded execution. | ||
| if snap != nil && snap.GetLocality() == "" && snap.GetDurableSnapshot().GetStatus() != ateapi.SnapshotStorageStatus_SNAPSHOT_STORAGE_STATUS_COMPLETED { | ||
| slog.InfoContext(leaseCtx, msgDurableCrashedSuspend, | ||
| slog.Int("generation", int(reported.GetGeneration())), | ||
| slog.String("uuid", reported.GetUuid())) | ||
| } else if err := applyLatestDurable(leaseCtx, status, reported); err != nil { | ||
| return err | ||
| } | ||
| if state.GetLatestLocal() == nil && state.GetUploading() == nil { | ||
| if err := applyDetach(status, node, reported.GetGeneration()); err != nil { | ||
| return err | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🤖 Because the snap != nil && snap.GetLocality() == "" && ... check runs before applyLatestDurable, a report naming a crashed suspend's generation bypasses the UUID, DurableSnapshot != nil, upload URI, and fidelity validations in applyLatestDurable. If such a report also has LatestLocal == nil && Uploading == nil with a mismatched UUID or wrong upload URI, ApplyReportedSnapshotState will log msgDurableCrashedSuspend and apply the detach to older snapshots instead of rejecting the report.
If we move this check into applyLatestDurable right after the validation checks (before setting snap.Fidelity and snap.DurableSnapshot.Status):
if status.GetState() == ateapi.ActorState_ACTOR_STATE_CRASHED && snap.GetLocality() == "" {
slog.InfoContext(ctx, msgDurableCrashedSuspend,
slog.Int("generation", int(reported.GetGeneration())),
slog.String("uuid", reported.GetUuid()))
return nil
}then all validations still run (snap is already guaranteed not to be COMPLETED at that point in applyLatestDurable), and ACTOR_STATE_CRASHED can share applyActorSnapshotState with SUSPENDED and RUNNING without duplicating it here.
| msgDurableNotRecorded = "Ignoring a durable snapshot at a generation the actor no longer records" | ||
| msgDurableOlder = "Ignoring a durable snapshot older than the latest completed one" | ||
| msgDurableNotRecorded = "Ignoring a durable snapshot at a generation the actor no longer records" | ||
| msgDurableCrashedSuspend = "Ignoring the upload of a snapshot whose suspend crashed" |
Collaborator
There was a problem hiding this comment.
🤖 Nit: msgDurableCrashedSuspend is defined alongside msgDurableOlder and msgDurableNotRecorded, which are tested in TestApplyLatestDurableLogsStaleReports. If the crashed-suspend check is moved into applyLatestDurable, we can also add a test case for msgDurableCrashedSuspend in TestApplyLatestDurableLogsStaleReports.
A report for a CRASHED actor failed with FailedPrecondition, which atelet does not retry. A revert then deleted an upload that atelet had finished as in progress. A report for a CRASHED actor now records its finished upload and applies a detach. A crashed suspend's snapshot has no Locality, so its upload is not recorded and the revert deletes it. The detach does not require the latest snapshot on the node to be durable, because a revert drops local snapshots before the actor can resume.
ReportActorSnapshotState also accepted a report from a node that held one of the actor's snapshots while the actor was placed elsewhere. A snapshot's locality is set only to the assigned node, and the actor leaves a node only after its snapshots there are detached.
Zoe Zhao (zoez7)
force-pushed
the
crashed-snapshot-report
branch
from
October 23, 2026 16:54
358ff93 to
4a0dcc9
Compare
A report naming a crashed suspend's snapshot skipped the uuid, upload URI and fidelity checks in applyLatestDurable. Move the check into applyLatestDurable after those checks, so a CRASHED actor goes through applyActorSnapshotState like a SUSPENDED or RUNNING one. Check the actor's state before cloning it.
Zoe Zhao (zoez7)
force-pushed
the
crashed-snapshot-report
branch
from
October 23, 2026 17:00
4a0dcc9 to
d083fd3
Compare
Michelle Au (msau42)
approved these changes
Oct 23, 2026
Dmitry Berkovich (dberkov)
approved these changes
Oct 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this needed?
actorOnNodecheck to only check the assigned node. Today, each actor's local snapshots are cleared during Detach, we don't have any actors with old snapshots on a different node.AI use: AI-assisted, I reviewed and tested it.