Skip to content

Drain artifact watcher before closing WS on end_session_message#29

Merged
jackcbrown89 merged 2 commits intomainfrom
worktree-jb-fix-invocation-delay
Apr 17, 2026
Merged

Drain artifact watcher before closing WS on end_session_message#29
jackcbrown89 merged 2 commits intomainfrom
worktree-jb-fix-invocation-delay

Conversation

@jackcbrown89
Copy link
Copy Markdown
Contributor

@jackcbrown89 jackcbrown89 commented Apr 17, 2026

Summary

  • On end_session_message, ws.close() was called immediately and the 3s chokidar drain + pending-request wait ran in the close handler. Any artifact_upload_request_message produced by a late chokidar event during the drain was silently dropped because send() is gated on readyState === OPEN.
  • Moved the drain into a memoized drain() helper invoked before ws.close() on the orderly path. The close handler still calls drain() for unexpected disconnects; memoization prevents double-draining.

Test plan

  • npx vitest run src/session.test.ts — all 29 tests pass

Note

Medium Risk
Changes session shutdown ordering and introduces memoized draining, which can affect artifact upload completeness and session close timing if edge cases exist (e.g., multiple close paths or long-running uploads). Scope is contained to WebSocketSession teardown logic plus a patch version bump.

Overview
Ensures end_session_message drains the artifact watcher and waits for pending artifact uploads before calling ws.close(), so late chokidar events can still enqueue artifact_upload_request_message while the socket is open.

Refactors the shutdown sequence into a memoized drain() helper that is invoked from both the orderly end-session path and the WS close handler (for unexpected disconnects), preventing duplicate drain work.

Bumps runtimeuse version from 0.11.0 to 0.11.1.

Reviewed by Cursor Bugbot for commit 79977a7. Bugbot is set up for automated code reviews on this repo. Configure here.

Previously ws.close() fired immediately on end_session_message and the
3s chokidar drain ran in the close handler. Late artifact_upload_request
messages produced during the drain were silently dropped because send()
is gated on readyState === OPEN.

Move the drain into a memoized helper invoked before close() on the
orderly path; the close handler still drains for unexpected disconnects.
@jackcbrown89 jackcbrown89 merged commit b05bff2 into main Apr 17, 2026
6 checks passed
@jackcbrown89 jackcbrown89 deleted the worktree-jb-fix-invocation-delay branch April 17, 2026 23:40
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 79977a7. Configure here.

this.logger.log("Received end_session_message. Closing session.");
// Drain the artifact watcher *before* closing so any late chokidar
// events still have an open socket to send upload requests through.
await this.drain();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drain failure prevents ws.close() and caches rejected promise

Low Severity

If drain() rejects (e.g., stopWatching() / chokidar watcher.close() throws), ws.close() on the next line is never reached, leaving the WebSocket open. Worse, the memoized drainPromise caches the rejection permanently, so when the close handler later calls await this.drain(), it re-throws — resolve() is never called and run() hangs forever. Wrapping ws.close() in a finally (or catching drain errors) would ensure the socket is always closed. In the old code, ws.close() was called unconditionally before any drain logic.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 79977a7. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant