fix(desktop): retain allocation ownership when startup cleanup fails - #1810
fix(desktop): retain allocation ownership when startup cleanup fails#1810danielgwilson wants to merge 2 commits into
Conversation
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Humanish operator (Codex).
|
🦋 Changeset detectedLatest commit: 74080fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
98fa8c7 to
9c3a25d
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have @danielgwilson on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
There was a problem hiding this comment.
TASTE.md compliance review (rules from e2b-dev/sdk-harness/TASTE.md, judged only on the lines this PR changes).
Checked: T-1/T-1c parity of the new JS/Python surface, T-54 flat entry-point exports, T-57/T-58 error hierarchy, T-32 kill semantics, T-16/T-19 type shapes, T-62/T-63/T-64 error-message quality, T-69/T-70/T-71 docs.
5 violations, all in the new error surface — none in the control-flow change itself:
- T-62/T-64: the new message states what failed but not what to do, and omits the one datum the error exists to carry (the sandbox ID) — in both languages.
- T-69 (+T-70/T-71):
Sandbox.creategained a new failure mode in both SDKs and neither the JSDoc nor the docstring documents it. - T-1: the Python constructor sets
__cause__itself and the raise site usesraise ... from error.
What holds up well: DesktopStartupError ↔ DesktopStartupException naming is exactly T-1c; both extend the domain base per T-57; both are re-exported from the flat entry point per T-54; the changed catch keeps re-raising the original startup error when cleanup succeeds, so T-32's "kill returns false for an already-absent sandbox" path is unaffected.
Not line-anchorable:
- T-54 asks for public names to be listed in
__init__.py's__all__.packages/desktop-python/e2b_desktop/__init__.pyhas no__all__today (pre-existing), soDesktopStartupExceptionis only implicitly public — worth adding an explicit__all__while touching this file. - Type parity nit (T-1): JS types both payloads as
unknownwhile Python types themException.unknownis idiomatic for a JScatchbinding, so this is acceptable, but the JS test passescleanupError: null, so the field genuinely can hold a non-Error— the JSDoc should say so rather than leaving readers to assume anError.
|
Addressed the error-surface review in
I left the pre-existing Pinned repository format, lint and typecheck passed, along with 10 JavaScript and 9 Python readiness tests and package builds/import checks. No live sandbox failure was induced. — Humanish operator (Codex), affiliated with Humanish |
|
We require contributors to sign our Contributor License Agreement, and we don't have @danielgwilson on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
|
September 9 update from the Humanish operator (Codex), affiliated with Humanish: this ownership gap is reproducible with the published The package-level reproduction below executes the installed SDK's real constructor, import { Sandbox } from '@e2b/desktop'
import assert from 'node:assert/strict'
const startup = new Error('synthetic startup failure')
const cleanup = new Error('synthetic cleanup failure')
let killCalls = 0
class Probe extends Sandbox {
static async createSandbox() {
throw new Error('provider allocation forbidden')
}
constructor(...args) {
super(...args)
this.commands.run = async () => { throw startup }
this.kill = async () => { killCalls++; throw cleanup }
}
}
const error = await Probe.create({
debug: true,
apiKey: 'synthetic-not-a-provider-key',
requestTimeoutMs: 1000,
timeoutMs: 1000,
}).catch(error => error)
assert.equal(error, startup)
assert.equal(killCalls, 1)
assert.equal(error.sandboxId, undefined)
assert.equal(error.cleanupError, undefined)We merged a downstream guard that keeps one bounded cleanup receipt during creation and refuses automatic retries when cleanup is unconfirmed. Its installed-SDK regression tests also cover original error identity, four startup phases, an unresolved cleanup method, and restoration of normal kill behavior after successful creation. The timeout test injects a pending local kill method. It demonstrates that SDK-internal cleanup can run before a caller's outer cleanup deadline begins; it does not establish an indefinitely hanging provider request. E2B 2.49.0's normal kill path inherits the configured request timeout and supports cancellation. We separately ran two hosted sandboxes with controlled command failures after Xvfb launch and after XFCE launch. Both preserved the injected startup error, called the normal SDK kill exactly once, and confirmed the specific owned sandbox was absent afterward. Those checks support successful real teardown after these startup faults; they do not measure ambiguous cleanup outcomes or provider failure prevalence. The structured allocation ID and cleanup cause proposed here remain useful to downstream callers after the 2.4.0 release. Maintenance update, September 9: rebased this contribution onto |
8cb60f6 to
74080fa
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have @danielgwilson on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
When desktop initialization fails and killing its allocated sandbox also fails, both desktop SDKs currently discard the cleanup error and return only the startup error. The caller loses the sandbox ID it needs for targeted reclamation.
This change exports
DesktopStartupErrorin JavaScript andDesktopStartupExceptionin Python for that dual-failure path. Both retain the allocation ID, the original startup exception as the standard cause, and the cleanup exception. Their messages name the allocation and its targeted cleanup call without including the underlying failure payloads. BothcreateAPIs document the new failure mode; JavaScript documents arbitrary rejection values, and Python sets the cause once in the constructor. Successful cleanup—including an already-absent sandbox—still rethrows the original startup exception unchanged. Successful initialization and base-allocation failures keep their existing behavior.Fixes #1808.
The new errors inherit from
SandboxError/SandboxException. On the dual-failure path, callers previously matching a specific startup-error subclass should inspectcause/__cause__instead. The patch adds no cleanup retry and does not claim the allocation is still alive: cleanup could have completed despite a transport failure. Only the previously allocated sandbox ID is exposed for the caller to reconcile.Example handling:
Validation: repository format, lint and typecheck passed. The targeted readiness suites passed 10 JavaScript and 9 Python tests, using mocked allocation/startup/cleanup boundaries. Restoring the prior catch blocks makes the dual-failure regressions fail in both languages; restoring this patch returns both suites green. The JS suite covers frozen errors and non-Error rejections as well. Compiled CJS/ESM exports and the built Python wheel include the new public errors. No live sandbox failure was induced; occurrence rates and ambiguous provider cleanup outcomes remain unmeasured.
The changeset covers both desktop packages. This contribution was prepared by the Humanish operator (Codex), affiliated with the project.