test(js-sdk): keep sending wake requests in the auto-resume test - #1865
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
There was a problem hiding this comment.
TASTE.md review — checked the changed hunks in packages/js-sdk/tests/sandbox/lifecyclePayload.test.ts against the API-shape (T-3, T-9, T-11, T-12), timeout (T-44–T-47), and streaming/lifecycle (T-27, T-36) rules.
Violations on public SDK surface: 0. This PR is test-only and touches no exported API; it removes the one-shot triggerAutoResume helper and reuses the existing waitForHttpStatus poller, which also aligns the JS test with the Python sync/async test_auto_resume_wakes_on_http_request (T-1 parity in spirit).
Advisory (1, non-blocking): the new call site threads a positional undefined through the pre-existing waitForHttpStatus(url, status, init?, timeoutMs) chain — see inline. The helper's signature is not changed by this PR, so it is not counted as a violation.
Package ArtifactsBuilt from 770bcf9. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.49.1-devin-1789052072-flaky-auto-resume-test.0.tgzCLI ( npm install ./e2b-cli-2.19.1-devin-1789052072-flaky-auto-resume-test.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.8.1-devin-1789052072-flaky-auto-resume-test.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.4.1-devin-1789052072-flaky-auto-resume-test.0.tgzPython SDK ( pip install ./e2b-2.49.0+devin.1789052072.flaky.auto.resume.test-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.10.0+devin.1789052072.flaky.auto.resume.test-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.5.0+devin.1789052072.flaky.auto.resume.test-py3-none-any.whl |
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
There was a problem hiding this comment.
Looks good — small, test-only change with sound logic.
What was reviewed: the waitForHttpStatus options-object signature change in tests/setup.ts and its single non-trivial call site update in network.test.ts; the lifecyclePayload.test.ts auto-resume flow rewrite (removed triggerAutoResume, now retries the wake request via waitForHttpStatus before checking state). Confirmed the other four call sites of waitForHttpStatus (network.test.ts, snapshot.test.ts, host.test.ts) only pass (url, status) and remain compatible with the new optional trailing options object. Checked the removed narrow catch in triggerAutoResume versus the helper's bare catch {} — not a fail-fast regression here since the helper's loop is bounded by timeoutMs and surfaces a descriptive timeout error either way.
Extended reasoning...
Overview
This PR touches only three test files in packages/js-sdk: tests/setup.ts (changes waitForHttpStatus's trailing parameters from positional (init?, timeoutMs) to an options object { init?, timeoutMs }), tests/sandbox/network.test.ts (updates its one call site to wrap headers inside the new init key), and tests/sandbox/lifecyclePayload.test.ts (removes the bespoke triggerAutoResume helper and replaces a fragile two-step wake-then-poll sequence with a single waitForHttpStatus(url, 200, { timeoutMs: 60_000 }) call that relies on the helper's existing retry loop to keep re-sending the wake request). No production/SDK-surface code is touched, so per CLAUDE.md no changeset is needed, and there is no Python-side mirroring obligation since no public SDK behavior changed.
Security risks
None. This is test infrastructure only — no auth, crypto, or permission-related code paths are affected, and no new external inputs or secrets are introduced.
Level of scrutiny
Low-to-moderate is appropriate given the small, mechanical, test-only diff, but I did verify call-site compatibility and the correctness of the auto-resume fix's premise (racing/lost wake requests get retried instead of relying on a single fire-and-forget request).
Other factors
The PR conversation shows two rounds of review feedback that were both addressed by follow-up commits from the author (mishushakov's request for an options-object signature was implemented in commit 19029fb, verifiable directly in the diff), and this run's own bug-hunting pass independently examined the concern about the removed fail-fast catch (bare catch {} in waitForHttpStatus) and ruled it out, since the retry loop is time-bounded and produces a clear timeout error regardless. With no outstanding findings and both raised concerns addressed either by a verifiable commit or by independent re-examination, and the change itself being small and low-risk, I'm confident this doesn't need further human review.
Summary
Fixes the flaky
auto-resume wakes paused sandbox on http requesttest inpackages/js-sdk/tests/sandbox/lifecyclePayload.test.ts(failure: https://github.com/e2b-dev/E2B/actions/runs/34489636300/job/102912738492 —expected 'paused' to equal 'running', sandbox stillpausedafter 30s).The JS test sent a single wake request (
triggerAutoResume) and then waited forstate === 'running'. If that one request raced the pause finalizing or was swallowed at the gateway (any non-timeout error response is aResponse, not a throw, so a 5xx was silently accepted), nothing ever re-sent the wake signal and the test timed out.This mirrors what the Python sync/async
test_auto_resume_wakes_on_http_requestalready do (wait_for_http_status(url, 200)beforewait_for_state(RUNNING)).triggerAutoResumeis removed as it is no longer used.Per review, the
waitForHttpStatushelper intests/setup.tsnow takes a trailing options object instead of a chain of optional positionals:Test-only change; no changeset needed. Verified locally against production (the test and a timed probe pass; wake → 200 →
runninghappens within ~0.3s of the pause).Link to Devin session: https://app.devin.ai/sessions/deb9199b540b49a889461e902787b9bd
Open in Devin Desktop: https://app.devin.ai/desktop/session/deb9199b540b49a889461e902787b9bd?variant=devin
Requested by: @mishushakov