atelet: detach Checkpoint from caller cancellation once underway - #2602

This shape looks right to me - the two failure modes you describe are the classic retry-after-timeout pair, and serializing per actor plus using the manifest as a commit marker is a good way to handle them.

One thing I checked before raising it: the fast path returns an empty CheckpointResponse, which at first looked like a retry getting a degenerate answer. It isn't - the normal path at main.go:842 returns the same empty response, and the proto documents state as deliberately unset for now. So no issue there.

Two things I did wonder about.

The idempotent fast path is gated on UPLOAD_MODE_DEFERRED. That makes sense for the manifest check itself, since the proto says IMMEDIATE keeps no local copy, so there'd be no manifest.json to find. But that leaves IMMEDIATE with only half the protection: the lock still stops the overlap race, while the "first attempt already finished and cleaned up" case would presumably still fail with sandbox.json: no such file or directory. Is that deliberate, or just out of scope here?

Smaller one: if rec, err := unmarshalSandboxRecord(manifest); err == nil drops the error. A manifest that exists but doesn't parse would silently leave op.sandboxClass empty, so the checkpoint metric ends up unattributed with nothing explaining why. Might be worth a log line on that branch.

Thanks for the review!

1. UPLOAD_MODE_IMMEDIATE: Yes, targeting on UPLOAD_MODE_DEFERRED for now. In atelet.proto, UPLOAD_MODE_IMMEDIATE is defined as "Upload the snapshot synchronously before Checkpoint returns, while keeping the local copy" with TODO: Temporarily this deletes the local snapshot after the upload. Once that TODO is resolved and IMMEDIATE keeps its local copy too, this same local manifest.json check can cover both modes.
2. unmarshalSandboxRecord error: Good catch — added a slog.WarnContext when unmarshaling fails.

This shape looks right to me - the two failure modes you describe are the classic retry-after-timeout pair, and serializing per actor plus using the manifest as a commit marker is a good way to handle them.
One thing I checked before raising it: the fast path returns an empty CheckpointResponse, which at first looked like a retry getting a degenerate answer. It isn't - the normal path at main.go:842 returns the same empty response, and the proto documents state as deliberately unset for now. So no issue there.
Two things I did wonder about.
The idempotent fast path is gated on UPLOAD_MODE_DEFERRED. That makes sense for the manifest check itself, since the proto says IMMEDIATE keeps no local copy, so there'd be no manifest.json to find. But that leaves IMMEDIATE with only half the protection: the lock still stops the overlap race, while the "first attempt already finished and cleaned up" case would presumably still fail with sandbox.json: no such file or directory. Is that deliberate, or just out of scope here?
Smaller one: if rec, err := unmarshalSandboxRecord(manifest); err == nil drops the error. A manifest that exists but doesn't parse would silently leave op.sandboxClass empty, so the checkpoint metric ends up unattributed with nothing explaining why. Might be worth a log line on that branch.

Thanks for the review!

1. UPLOAD_MODE_IMMEDIATE: Yes, targeting on UPLOAD_MODE_DEFERRED for now. In atelet.proto, UPLOAD_MODE_IMMEDIATE is defined as "Upload the snapshot synchronously before Checkpoint returns, while keeping the local copy" with TODO: Temporarily this deletes the local snapshot after the upload. Once that TODO is resolved and IMMEDIATE keeps its local copy too, this same local manifest.json check can cover both modes.
2. unmarshalSandboxRecord error: Good catch — added a slog.WarnContext when unmarshaling fails.

Synced with #2677. The path now checks the snapshot tracker state instead of manifest.json, so it covers IMMEDIATE too.

Both answered well, thanks.

The IMMEDIATE reasoning makes sense — the proto's TODO about deleting the local snapshot after upload is what made the manifest check inapplicable there, not an oversight. And syncing with #2677 closes it anyway: checking the tracker state rather than manifest.json covers both modes, and I can see the retry test now runs under UPLOAD_MODE_IMMEDIATE as well.

Good catch adding the WarnContext on the unmarshal failure — that was the bit that would have been silent.

Nothing further from me.

Thanks for the review Abhishek Sharma (@abhisheksharma2411)!

Quick update on the scope here: since #2849 now covers answering retried Checkpoint calls from snapshottracker (across both DEFERRED and IMMEDIATE modes). So I removed the duplicate commit-check logic from this PR.