feat(api): create the snapshot record at capture start and mark it error on failure#4999
feat(api): create the snapshot record at capture start and mark it error on failure#4999mu-hashmi wants to merge 7 commits into
Conversation
…cepted Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
…pletion Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
|
View your CI Pipeline Execution ↗ for commit 369e6df
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Pull request overview
This PR changes the snapshot-from-sandbox capture flow to create a Snapshot record immediately when the capture is accepted (state PENDING), then transition it to ACTIVE on success or ERROR (with errorReason) on failure/timeout. This aligns capture behavior with existing pull/build snapshot lifecycles so failures are observable and quota settlement follows the snapshot record’s state transitions.
Changes:
- Added utilities to (1) create the pending capture record at accept-time, (2) activate it on success (or fall back to legacy insert), and (3) mark it errored on failure.
- Updated v0 (background) and v2 (
SNAPSHOT_SANDBOXjob) paths to activate/fail the upfront record instead of only inserting after success. - Updated the snapshot state cron (
check-snapshot-state) to treat capture-shapedPENDINGsnapshots specially and time them out toERROR.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/api/src/sandbox/utils/persist-snapshot-from-sandbox.util.ts | Splits persistence into accept-time PENDING insert, success activation, and failure marking; adds capture-signature predicate. |
| apps/api/src/sandbox/utils/persist-snapshot-from-sandbox.util.spec.ts | Adds unit coverage for pending insert, activation (incl. legacy fallback), failure marking, and capture predicate. |
| apps/api/src/sandbox/services/snapshot.service.ts | Exposes new snapshot-from-sandbox lifecycle helpers via SnapshotService. |
| apps/api/src/sandbox/services/sandbox.service.ts | Creates pending snapshot record before dispatching capture; v0/v2 failure paths mark record ERROR and remove legacy pending-usage rollback logic. |
| apps/api/src/sandbox/services/job-state-handler.service.ts | On v2 job completion, activates pending record; on FAILED, marks record ERROR with job.errorMessage. |
| apps/api/src/sandbox/managers/snapshot.manager.ts | Prevents pull/build pending-state handler from corrupting capture records; adds timeout-to-error behavior for capture-shaped pending snapshots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ACTIVE/ERROR), so the outer catch's rollback must not fire. | ||
| pendingSnapshotCountIncrement = undefined | ||
|
|
||
| const runnerAdapter = await this.runnerAdapterFactory.create(runner) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…on throws Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
vedranjukic
force-pushed
the
main
branch
2 times, most recently
from
2c15a7f to
b40f732
Compare
rajpratham1
left a comment
rajpratham1
left a comment
There was a problem hiding this comment.
This is a solid improvement to the snapshot lifecycle.
Creating the snapshot record at capture start gives users immediate visibility into pending work, surfaces duplicate-name conflicts before any runner work begins, and ensures failures/timeouts are recorded on the snapshot itself instead of silently disappearing. I also like that quota ownership is transferred to the snapshot lifecycle, with consistent settlement through the PENDING → ACTIVE/ERROR transitions.
The implementation covers both v0 and v2 runners, includes sensible fallback behavior for pre-existing jobs, restores sandbox state on initiation failures, and adds comprehensive regression tests for pending creation, activation, failure handling, and timeout scenarios.
I don't see any blocking issues. Looks good to merge.
TL;DR: snapshot-from-sandbox captures now create the snapshot record at accept time and mark it
error(+errorReason) on every failure path, so failed captures are visible instead of silently vanishing.Description
Capture previously inserted the Snapshot record only AFTER success. Any failure (error, timeout, transport abort) left zero evidence: sandbox state silently reverted, the SDK reported success, the snapshot 404'd forever (#4990).
After this PR:
pendingwhen the capture is accepted (v0 and v2), so duplicate names 409 before any capture work runs.active(existing propagation preserved); failures (v0 background error, v2 FAILED job, cron timeout, initiation throw) mark iterrorwith a realerrorReasonwhile the sandbox still reverts to its previous state.pending -> errorstate event (closes a quota leak).sizeGBis persisted for v0 records; deploy-window fallback keeps jobs from pre-rollout APIs working (legacy insert-ACTIVE).Design decision: capture records reuse
SnapshotState.PENDING, discriminated byisPendingCaptureSnapshot(pending+ nobuildInfo+ emptyimageName+ emptyref), instead of a new enum value - avoids a PostgresALTER TYPEmigration and an OpenAPI enum change regenerating six client libraries. The!refterm exists because activation setsrefbut neverimageName, so reactivated rows (INACTIVE -> PENDING) would otherwise match the signature - found by review, fixed in 194a03a.Behavior notes for reviewers:
errorrecord until deleted - consistent with pull/build failures, but previously failures left no trace. Release-note worthy.SNAPSHOTTINGresolves via the cron timeout (generic reason) instead of the job's message - rare corner, accepted.Review guide
Suggested order:
apps/api/src/sandbox/utils/persist-snapshot-from-sandbox.util.ts- the record lifecycle helpers and the capture signature (risk center: theisPendingCaptureSnapshotpredicate gates everything).apps/api/src/sandbox/services/sandbox.service.ts- accept-time insert + v0 failure path + initiation try/catch.apps/api/src/sandbox/services/job-state-handler.service.ts- v2 FAILED handling + quota settle.apps/api/src/sandbox/managers/snapshot.manager.ts- cron guard (risk center: interaction with the pull flow).Generated files: one additive client regen documenting the 409 (response-map and doc entries across the go/java/python clients); guarded by the CI regen-drift gate.
Commit map
Related PRs
Part of #4990. Merge order: this PR first; #5004 (v0 async capture) is stacked on this branch and gets retargeted to main after merge; #5000 (SDK record polling) pairs with this and merges independently.
Validation
persist-snapshot-from-sandbox.util.spec.ts(21 assertions: insert fields/event/409 mapping, activation incl. legacy fallback and non-capture refusal, failure idempotency, signature predicate incl. reactivated rows) + newsandbox.service.spec.ts(3 tests on the initiation-throw path).nx test api: 52 suites / 571 tests green;nx build api+ lint green.--skip-nx-cacheand verified by the CI drift gate.Closes #4991