fix(js-sdk): treat WatchHandle.stop() as a clean watch end

[devin-ai-integration]

Summary

Squashed version of community PR #1912 by @harshitgavita-07. Fixes #1895.

WatchHandle.stop() aborts the watch stream. The abort surfaced as a Connect cancellation, which was mapped to TimeoutError and passed to onExit, so every user-initiated stop looked like a request timeout. The handle now records the stop, and a stream error that arrives after it ends the watch cleanly:

async stop() {
  this.stopped = true
  this.handleStop()
}

private async *iterateEvents() {
  try { for await (const event of this.events) ... }
  catch (err) {
    if (this.stopped) return
    throw await handleRpcErrorWithHealthCheck(err, this.checkHealth)
  }
}

The suppression covers only the stream iterator, so onEvent rejections still reach onExit, including ones raised after stop(). After stop(), onExit is called asynchronously with no argument, which matches what async Python AsyncWatchHandle.stop() passes (#1480). This is now documented in the JSDoc on stop() and WatchOpts.onExit.

Tests in packages/js-sdk/tests/sandbox/files/watchHandle.test.ts cover three cases:

• a clean exit after stop()
• stream errors when the watch was not stopped
• an onEvent error thrown after stop()

Includes an e2b patch changeset.

Not covered: aborting through opts.signal still reports a TimeoutError. That behaviour predates this PR and is left for a follow-up.

Link to Devin session: https://app.devin.ai/sessions/410788c120d144ed87242ce3b1eb22f7
Open in Devin Desktop: https://app.devin.ai/desktop/session/410788c120d144ed87242ce3b1eb22f7?variant=devin

---

[devin-ai-integration]

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

• Disable automatic comment, CI, and merge conflict monitoring

[changeset-bot]

🦋 Changeset detected

Latest commit: df21256

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
e2b	Patch

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

[devin-ai-integration]

TASTE.md review of #1923 (WatchHandle.stop() as a clean end).

Rules checked against the changed code: T-1 (cross-SDK parity), T-26/T-27/T-30 (handle shape, on<Event> callbacks, stop() naming), T-62 (documenting when errors surface), T-69/T-70 (JSDoc on public surface), plus the changeset/test additions.

1 violation (inline): T-1, a semantic parity gap. JS stop() resolves before onExit fires, but async Python's stop() waits until on_exit has run.

Otherwise the change complies. Using a handle-level stopped flag rather than error-type sniffing keeps the stop path independent of how Connect maps the abort, and the WatchOpts.onExit JSDoc now documents when err is set (T-62/T-69).

Not a TASTE rule, just a heads-up: the if (!this.stopped) guard covers the whole try, so an onEvent rejection that races with stop() is now dropped as well as the abort. That's probably fine, but it's broader than the comment says.

[devin-ai-integration]

T-1 (parity: names and semantics mirror 1:1 across JS / sync Python / async Python).

This PR aligns what onExit receives after stop() with async Python, but not when. AsyncWatchHandle.stop() cancels the task and await asyncio.wait([self._wait]), so on_exit(None) has already run by the time stop() returns. Here stop() only flips the flag and aborts; onExit fires later from the detached handleEvents() loop, so await handle.stop() resolves before onExit (the new test has to vi.waitFor after stop() for exactly this reason). The new JSDoc ("Calls onExit … after the watch stops") reads like the Python guarantee but doesn't hold for callers that rely on ordering after await stop().

Compliant form: keep the loop's promise and await it in stop(), e.g.

private readonly done: Promise<void>
// constructor: this.done = this.handleEvents()

/**
 * Stop watching the directory.
 *
 * Resolves after `onExit` has been called (with no error).
 */
async stop() {
  this.stopped = true
  this.handleStop()
  await this.done
}

(Watch for re-entrancy: a stop() awaited from inside onEvent would wait on itself, since handleEvents is awaiting that onEvent. Guard that case, or if the async ordering is intentional, say so in the JSDoc: onExit is called asynchronously after stop() returns.)

[devin-ai-integration]

I kept the async ordering and fixed the JSDoc in df21256. It now says "onExit is then called asynchronously, with no error." Making stop() await the loop would deadlock in the common case where stop() is called from inside onEvent, because the loop is awaiting that same onEvent. Before this PR stop() didn't wait for onExit either, so its timing is unchanged.

[github-actions]

Package Artifacts

Built from 5d75b45. Download artifacts from this workflow run.

JS SDK (e2b@2.52.1-devin-1790858689-watch-stop-clean-exit.0):

npm install ./e2b-2.52.1-devin-1790858689-watch-stop-clean-exit.0.tgz

CLI (@e2b/cli@2.21.1-devin-1790858689-watch-stop-clean-exit.0):

npm install ./e2b-cli-2.21.1-devin-1790858689-watch-stop-clean-exit.0.tgz

Code Interpreter JS SDK (@e2b/code-interpreter@2.8.1-devin-1790858689-watch-stop-clean-exit.0):

npm install ./e2b-code-interpreter-2.8.1-devin-1790858689-watch-stop-clean-exit.0.tgz

Desktop JS SDK (@e2b/desktop@2.4.1-devin-1790858689-watch-stop-clean-exit.0):

npm install ./e2b-desktop-2.4.1-devin-1790858689-watch-stop-clean-exit.0.tgz

Python SDK (e2b==2.52.0+devin.1790858689.watch.stop.clean.exit):

pip install ./e2b-2.52.0+devin.1790858689.watch.stop.clean.exit-py3-none-any.whl

Code Interpreter Python SDK (e2b-code-interpreter==2.10.1+devin.1790858689.watch.stop.clean.exit):

pip install ./e2b_code_interpreter-2.10.1+devin.1790858689.watch.stop.clean.exit-py3-none-any.whl

Desktop Python SDK (e2b-desktop==2.6.0+devin.1790858689.watch.stop.clean.exit):

pip install ./e2b_desktop-2.6.0+devin.1790858689.watch.stop.clean.exit-py3-none-any.whl

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be2f23a070

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve in-flight onEvent failures after stopping

When an async onEvent is already running and rejects after another task calls stop(), this condition discards that callback error and invokes onExit() as though the watch ended cleanly. That rejection was not caused by the stream cancellation, so callers relying on onExit to detect failed event processing lose the failure during normal teardown. Limit the clean-exit handling to the cancellation produced by handleStop, while preserving errors from callbacks already in flight.

Useful? React with 👍 / 👎.

[devin-ai-integration]

Fixed in df21256. Only stream errors that arrive after stop() are treated as a clean end, so onEvent failures are kept.

[claude]

🔴 Once stop() is called, any error during the remaining watch loop is silently swallowed, not just the cancellation stop() itself causes, so real bugs can now go unreported to onExit (the base at least surfaced them, mislabeled). Calling await handle.stop() from inside onEvent (a common pattern: stop after seeing the target event) then doing more work in that callback means a later throw lands in the catch at watchHandle.ts:141-148 and is discarded, since stopped was already set synchronously at watchHandle.ts:102. Fix: scope suppression to the actual stop-induced cancellation (check the error's Connect cancellation code) instead of any error occurring after stopped flips, so unrelated failures post-stop still reach onExit.

Why this was flagged

Trigger: caller's onEvent handler calls await handle.stop() then continues doing work that throws (e.g. a null-deref bug or a second RPC call failing) in that same invocation. watchHandle.ts:101-104 sets this.stopped = true synchronously before handleStop() even resolves. The throw propagates into the single catch at watchHandle.ts:141-148, where if (!this.stopped) is now false, so iterationError stays undefined. onExit fires with err === undefined, hiding a genuine application error that previously (before this diff) would have reached onExit as an Error. No other safeguard distinguishes the abort-caused cancellation from an unrelated error raised after stop().

Verification: The mechanism is real and reachable. stop() sets this.stopped = true synchronously at watchHandle.ts:102, and the catch at watchHandle.ts:141-148 suppresses all errors once that flag is set. Because onEvent is awaited inside that try (line 133), onEvent throwing after stop() is caught with stopped already true, so iterationError stays undefined and onExit fires with no argument.

[devin-ai-integration]

Fixed in df21256. The guard is now scoped to the stream iterator in iterateEvents, not the whole loop, so a throw from onEvent after await handle.stop() reaches onExit. The new test covers exactly this case: onEvent calls stop() and then throws.

[claude]

🟣 Callers who cancel watchDir via opts.signal (the documented way, per connectionConfig.ts:186-187) instead of handle.stop() still get onExit called with a misleading TimeoutError blaming requestTimeoutMs — the exact bug this PR claims to fix. The new guard at watchHandle.ts:145 only checks this.stopped, set only inside WatchHandle.stop() (line 102); an aborted opts.signal reaches the identical controller.abort() path (connectionConfig.ts:241) and the identical Code.Canceled -> TimeoutError mapping, but never sets stopped. Fix: also flag stopped (or detect the abort reason) when opts.signal aborts, so both cancellation idioms get a clean onExit().

Why this was flagged

Trigger: caller does sdk.files.watchDir(path, onEvent, { signal: myController.signal, onExit }) then calls myController.abort() to stop watching — a standard JS cancellation idiom, explicitly documented as supported in connectionConfig.ts:186-187. That abort flows through onUserAbort -> abortWithReason(controller, ...) -> controller.abort() (connectionConfig.ts:241), the same call WatchHandle.stop()'s handleStop() makes, producing the same ConnectError Code.Canceled, mapped to TimeoutError by handleRpcErrorWithHealthCheck. In watchHandle.ts's catch (line 141-148), this.stopped is false because only stop() sets it (line 102), so iterationError is set and onExit receives the spurious TimeoutError — identical to the bug this diff set out to fix, just reached via opts.signal instead of handle.stop().

Verification: Pre-existing; merging does not make the base worse on this route. A caller aborting opts.signal triggers controller.abort() (connectionConfig.ts:205, 241) on the controller passed to rpc.watchDir, producing a Code.Canceled error mapped to TimeoutError (rpc.ts:80-83). The guard if (!this.stopped) (watchHandle.ts:145) only suppresses when stop() ran (sole setter at line 102); a signal abort leaves stopped===false, so onExit(TimeoutError) still fires on the documented opts.signal path.

[devin-ai-integration]

Agreed, the opts.signal abort path still reports a TimeoutError. That predates this PR, and fixing it means passing the user signal (or the abort reason) into WatchHandle, so I've left it out of scope here. I can follow up separately if you want that path to end cleanly too.

[mishushakov]

can you check the comments and whether we should fix this or close the PR altogether?

[devin-ai-integration]

My recommendation is to fix this PR and merge it, not close it. The underlying bug (#1895, where every stop() reported a bogus TimeoutError to onExit) is real, and the fix is small.

The valid finding was that the stopped guard swallowed any error raised after stop(), including onEvent rejections. In df21256 I moved the suppression into iterateEvents, so it now covers only stream errors that arrive after stop(). onEvent errors still reach onExit. I added a regression test where onEvent calls stop() and then throws. It fails on the previous code and passes now.

Two points I did not change, each answered in its own thread:

• stop() ordering compared with async Python: I documented that onExit runs asynchronously and did not make stop() await it.
• Aborting through opts.signal: this behaviour predates the PR and could be a follow-up.

[mishushakov]

is this bug also in python or only here ?

[devin-ai-integration]

This is JS only. Python doesn't have the bug:

• Async (AsyncWatchHandle): stop() cancels the event task, and _handle_events catches the asyncio.CancelledError and calls on_exit(None). That fix landed in fix(sdk): correct filesystem watch handle callback and timeout behavior #1480, so a stop is already reported as a clean end. stop() also awaits the task, so on_exit has run by the time stop() returns.
• Sync (WatchHandle): this is a polling handle with no on_exit callback, so there's nothing that could report a stop as a timeout.

[mishushakov]

please review and decide if it's mergeable

[devin-ai-integration]

I think it's mergeable.

• Diff: the change is small and scoped to JS. Stream errors that arrive after stop() are treated as a clean end. Everything else still reaches onExit, including onEvent errors and stream failures before stop(). Python already behaves this way, so no parity change is needed there.
• Review findings: I fixed the one valid issue in df21256 (callback errors swallowed after stop()), and Devin Review has marked it resolved. The two remaining notes are design or pre-existing, not blockers. onExit runs asynchronously after stop(), and that is now documented. The opts.signal abort path still reports a TimeoutError and could be a follow-up.
• Tests: there are 7 watchHandle unit tests. The new regression test fails without the fix. Lint, typecheck and generated-files pass.
• CI: everything is green except JS SDK - cloudflare-deploy. That job failed while wrangler was deploying a temporary Cloudflare worker, with fetch failed, which looks like a connectivity problem on the runner. The same job passed on the previous commit, 0dd46b9, and my change doesn't touch this path. I don't have permission to re-run it, so a maintainer needs to click re-run before merging.