fix(sdk-typescript): poll the snapshot record in _experimental_createSnapshot#5000
fix(sdk-typescript): poll the snapshot record in _experimental_createSnapshot#5000mu-hashmi wants to merge 5 commits into
Conversation
…shot Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
|
View your CI Pipeline Execution ↗ for commit 91cdee1
💡 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 fixes a correctness gap in the TypeScript SDK’s Sandbox._experimental_createSnapshot by polling the snapshot record (via SnapshotsApi.getSnapshot) to determine success/failure, rather than inferring success from the sandbox leaving SNAPSHOTTING (which could previously mask silent capture failures).
Changes:
- Update
_experimental_createSnapshotto poll the snapshot record to terminal states (active=> success,error/build_failed=> throw witherrorReason), with legacy-server 404 tolerance + one grace re-read. - Plumb a
SnapshotsApiinstance intoSandboxconstruction (viaDaytona.create/get/listand_experimental_fork). - Add focused unit tests for success/failure/legacy behaviors and regenerate the docs page to reflect the new behavior/signature.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| libs/sdk-typescript/src/Sandbox.ts | Implements record-based polling and adds snapshotsApi dependency to Sandbox. |
| libs/sdk-typescript/src/Daytona.ts | Instantiates and threads SnapshotsApi through Sandbox creation and SnapshotService. |
| libs/sdk-typescript/src/tests/Sandbox.test.ts | Adds tests covering record polling and legacy 404/grace behaviors. |
| apps/docs/src/content/docs/en/typescript-sdk/sandbox.mdx | Updates generated docs to reflect constructor signature and updated snapshot behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| constructor( | ||
| sandboxDto: SandboxDto | SandboxListItemDto, | ||
| private readonly clientConfig: Configuration, | ||
| private readonly axiosInstance: AxiosInstance, | ||
| private readonly sandboxApi: SandboxApi, | ||
| private readonly snapshotsApi: SnapshotsApi, | ||
| ) { |
There was a problem hiding this comment.
Fixed in cfc34c4: snapshotsApi is optional; when omitted the constructor default-instantiates SnapshotsApi from the same configuration used for the other APIs (captured before the toolbox-proxy repoint), so direct constructors keep working and still get record polling.
![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 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…fter capture Signed-off-by: Muhammad Hashmi <mhashmi@berkeley.edu>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…lure 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.
Nice improvement. The SDK now tracks the snapshot record instead of relying solely on sandbox state, which aligns with the backend's newer snapshot lifecycle. The implementation also maintains backward compatibility by gracefully handling legacy APIs where the snapshot record appears later, and the optional SnapshotsApi constructor parameter avoids introducing a breaking change.
The polling logic correctly handles successful completion, capture failures, build failures, timeout behavior, and transient "not found" cases. Refreshing sandbox state after record completion keeps the local object consistent without affecting the success path if the refresh itself fails. The accompanying tests cover both modern and legacy flows well.
I don't see any blocking issues. Looks good to merge.
TL;DR:
_experimental_createSnapshotnow polls the snapshot record (not sandbox state), so failed captures throw with the record'serrorReasoninstead of reporting success for a snapshot that doesn't exist.Description
The helper previously polled sandbox state and treated any exit from
SNAPSHOTTINGas success. A silently failed capture (#4990) therefore RESOLVED with a snapshot name that 404s forever.Now it polls the record by name until terminal:
activeresolves;error/build_failedthrow aDaytonaErrorcarrying the record'serrorReason. Terminal-state handling mirrorsSnapshotService.create.SNAPSHOTTING; once the sandbox reverts with no record, ONE grace re-read closes the server-side persist/revert straddle before reporting the legacy silent revert as a failure.Sandboxgains an optionalsnapshotsApiparameter. When omitted (direct constructors in the wild), it is default-instantiated from the same pre-proxyConfigurationthe other APIs use - captured before the constructor repointsclientConfigto the toolbox proxy. No breaking change; everyone gets record polling.Known, labeled limitation: against PRE-#4999 servers, a pre-existing ACTIVE record with the same name yields a false success - pre-existing failure class, fixed server-side by #4999's accept-time 409.
Review guide
Suggested order:
libs/sdk-typescript/src/Sandbox.ts-waitForSnapshotCompleteloop (risk center: terminal mapping + legacy fallback), then the constructor default.libs/sdk-typescript/src/Daytona.ts- wiring of the new param at construction sites.__tests__/Sandbox.test.ts- the six snapshot-wait specs document the contract.Mechanical:
apps/docstypedoc regen (generated; CI diffs it), test-file import fix (nx module-boundary rule counts CJSrequireof a buildable lib as lazy-loading).Commit map
Related PRs
Part of #4990. Pairs with #4999 (record exists from accept time); mergeable independently thanks to the legacy fallback. Python SDK has the same bug plus an unenforced-timeout defect - deliberately out of scope, follow-up issue to come.
Validation
nx test sdk-typescript: 15 suites / 202 tests green, incl.: success gated on the record (polled twice),errorrejects with errorReason,build_failedrejects, 404-tolerance while snapshotting, grace re-read recovery, legacy hard-fail once reverted with no record, defaultSnapshotsApiinstantiation, success + refresh-throws still resolves.tsc --verbatimModuleSyntax) clean; prettier clean; typedoc regenerated in-PR.npm install- infra flake, passed on rerun, tracked in ci: nuxt runtime-compat harness fails silently on transient npm errors #5008.)Closes #4992