fix(js-sdk): treat WatchHandle.stop() as a clean watch end - #1912
Conversation
A user-initiated stop aborts the stream, which surfaced as a Connect cancellation mapped to a TimeoutError blaming requestTimeoutMs, so onExit fired with a spurious error on every stop(). Track the user-initiated stop and treat it as a clean end: onExit now fires with no argument, matching the Python SDK (PR e2b-dev#1480) and the documented WatchOpts contract. Fixes e2b-dev#1895
|
We require contributors to sign our Contributor License Agreement, and we don't have @harshitgavita-07 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' |
🦋 Changeset detectedLatest commit: 3d6cd3f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review. Checked: T-1 (JS/Python parity), T-26/T-27/T-28/T-30 (watch handle, callbacks, and stop()), T-57–T-62 (error handling), T-69/T-70 (JSDoc).
1 violation (docs only). The behavior change is good. It fixes a T-1 parity gap: async Python AsyncWatchHandle.stop() already calls on_exit(None), and after this PR JS stop() leads to onExit() with no argument. Sync Python uses a polling handle with no on_exit, which T-28 allows. The public contract changed, but the JSDoc doesn't say so yet (inline comment on stop()).
Not on a changed line: WatchOpts.onExit in packages/js-sdk/src/sandbox/filesystem/index.ts ("Callback to call when the watch operation stops.") should also say that err is undefined after stop() or a clean stream end, and is set only when the watch failed (T-62: document when an error is surfaced).
| @@ -91,6 +97,7 @@ export class WatchHandle { | |||
| * Stop watching the directory. | |||
There was a problem hiding this comment.
T-69 (docstrings are part of the API; document defaults and failure modes) / T-70 (JSDoc tags). This PR makes stop() guarantee a clean end: onExit is called with no error, not a TimeoutError. Callers who branch on err depend on that guarantee, so the public JSDoc should state it instead of leaving it only in a private field comment.
| * Stop watching the directory. | |
| * Stop watching the directory. | |
| * | |
| * Stopping is a clean end: `onExit` is called with no error. |
|
looks good, sign the CLA and check the comment above |
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@mishushakov the CLA check is green, and I've addressed the JSDoc feedback for |
|
/accept |
be2f23a
into
e2b-dev:devin/1790858689-watch-stop-clean-exit
|
CI is green on #1923. Lint and Generated files first failed on formatting in the new watchHandle test file, so I pushed a commit that reformats it with prettier. |
|
I fixed the valid review finding on #1923 and replied on the PR that it's mergeable: callback errors were being swallowed after One check is red: |
## 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: ```ts 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-review-badge-begin --> --- <a href="https://app.devin.ai/review/e2b-dev/e2b/pull/1923" target="_blank"><picture><source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-devin-review-dark.svg?v=4"><img src="https://static.devin.ai/assets/gh-devin-review-light.svg?v=4" alt="Devin Review"></picture></a> <!-- devin-review-badge-end --> --------- Co-authored-by: harshitgavita-07 <harshit.gavita@gmail.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![https://static.devin.ai/assets/gh-devin-review-dark.svg?v=4"><img](https://static.devin.ai/assets/gh-devin-review-dark.svg?v=4"><img){kind=link}
2 participants
Summary
Fixes #1895
WatchHandle.stop()is the documented way to end a directory watch, but every user-initiated stop firedonExitwith aTimeoutErrorblamingrequestTimeoutMs:stop()aborts the stream, the abort surfaces as a Connect cancellation, and the error mapping treats it as a request timeout.This tracks the user-initiated stop in
WatchHandleand treats the resulting stream end as clean:onExitnow fires with no argument, matching theWatchOptscontract ("Callback to call when the watch operation stops") and the Python SDK behavior from #1480.Changes
packages/js-sdk/src/sandbox/filesystem/watchHandle.ts: set astoppedflag instop(); skip reporting the stream error toonExitwhen the end was user-initiated (13 lines incl. comments)packages/js-sdk/tests/sandbox/files/watchHandle.test.ts: two new tests -onExitfires with no argument when the watch is stopped by the user (fails before the fix with the reportedTimeoutError, passes after)onExitwhen the watch was not user-stoppede2bpatchVerification
TimeoutErrorfrom the issue, passes with the fixwatchHandletests passtsc --noEmitclean