treat snapshot cleanup 404 as success, not timeout - #388
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db32c429e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if seen_during_wait: | ||
| return True | ||
| return ttl_seconds_after_finished is not None and ttl_seconds_after_finished <= _POLL_INTERVAL |
There was a problem hiding this comment.
When ttl_seconds_after_finished is 0 or 1, a first GET can still be the documented cache-lag 404 immediately after POST, and the controller also TTL-deletes failed snapshots because setFailed() sets CompletionTime. In those cases this predicate makes create() fabricate Succeeded before any terminal success was observed, so a missing sandbox or failed upload can be reported as a restorable snapshot instead of surfacing an error or continuing to wait.
Useful? React with 👍 / 👎.
| onNotFound: ({ lastValue, seenDuringWait }) => { | ||
| if (!completedBeforeCleanup(seenDuringWait, ttlSecondsAfterFinished)) return undefined; | ||
| const base = lastValue ?? initial; | ||
| return { ...base, status: "Succeeded" }; |
There was a problem hiding this comment.
With ttlSecondsAfterFinished: 0, the first poll after POST may be an eventual-consistency 404 before the controller has processed the CR, and the same TTL cleanup path also removes failed snapshots. Returning { ...base, status: "Succeeded" } here therefore lets a missing sandbox or failed snapshot upload resolve as success even though no restorable snapshot was created.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2f9aa46e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _completed_before_cleanup(seen_during_wait, ttl_seconds_after_finished): | ||
| return latest.model_copy(update={"status": RootfsSnapshotStatus.SUCCEEDED}) |
There was a problem hiding this comment.
When a 404 arrives after the client deadline has already expired, this early return runs before the timeout check below. For example, with ttl_seconds_after_finished=0 or after any prior poll set seen_during_wait, a snapshot that only gets deleted after max_wait_seconds is exceeded is reported as Succeeded instead of raising IsolaTimeoutError, so the documented client-side budget is not enforced; check time.monotonic() before accepting cleanup 404s. The async branch below has the same ordering.
Useful? React with 👍 / 👎.
| const resolved = opts.onNotFound?.({ lastValue, seenDuringWait }); | ||
| if (resolved !== undefined) return resolved; |
There was a problem hiding this comment.
For rootfsSnapshots.create, pollUntilDone calls onNotFound and returns its synthetic Succeeded value before comparing performance.now() with the deadline. If the snapshot is TTL-deleted only after maxWaitMs has elapsed, or the polling request itself crosses the budget, callers get success instead of IsolaTimeoutError; check the deadline before allowing onNotFound to resolve.
Useful? React with 👍 / 👎.
1 participant
No description provided.