Extract the session SocketRegistry port - #1513
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
Warning Review limit reached
Next review available in: 53 seconds Limit details: You’ve used all 8 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe session WebSocket manager is replaced by ChangesSocket registry migration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR extracts the session socket lifecycle behind a runtime-neutral interface while preserving existing identity validation and wire behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![open-inspect[bot]](https://avatars.githubusercontent.com/in/2661131?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
PR #1513, "Extract the session SocketRegistry port" by @ColeMurray, changes 14 files with 126 additions and 158 deletions. The change cleanly moves Cloudflare WebSocket mechanics into DurableObjectSocketRegistry, updates session collaborators to consume narrower socket abstractions where practical, and preserves the existing sandbox/client lifecycle behavior.
Critical Issues
None found.
Suggestions
- Architecture
packages/control-plane/src/session/durable-object-session-connections.ts:11-DurableObjectSessionConnectionsstill depends on the concreteDurableObjectSocketRegistrybecause it needspersistClientMapping,configureAutoPing, and upgrade socket creation. This is not blocking, but if the goal is a fully runtime-neutral socket port, consider moving those methods into a dedicated interface or splitting the lifecycle/Cloudflare-only responsibilities so this adapter can depend on a contract rather than the implementation.
Nitpicks
None.
Positive Feedback
- The tagged
ctx.getWebSockets("sandbox")usage is a nice simplification and avoids scanning/classifying unrelated client sockets during sandbox recovery and detach. - Existing hibernation and stale sandbox identity checks were preserved, including terminal sandbox cleanup before re-adopting sockets.
- The control-plane typecheck passes locally.
Questions
None.
Verdict
Approve.
There was a problem hiding this comment.
[deep review] The extraction is moving in the right direction, but the new boundary is not actually runtime-neutral or substitutable yet. The inline findings are architectural blockers because they leave consumers coupled to both WebSocket-specific state and the concrete Durable Object implementation.
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
|
Addressed the fresh architecture findings in 020102e:
Deferred the explicitly scoped follow-ups: removing Local validation passed: control-plane lint, typecheck, build, 2,746 unit tests, and 861 integration tests. |
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
|
Closing: this PR targeted the pre-decomposition tree (the |
…COL-84) (#1745) ## Summary Roadmap **P-2 / COL-84** (Control plane on AWS, epic E1). Follows #1715 (P-1). `SessionConnectionAuthenticator.handleWebSocketUpgrade` performed the whole upgrade for both legs: it created a `WebSocketPair`, accepted the server half, and returned the 101 with `webSocket: client`. Both the pair and `Response.webSocket` exist only on Workers; on Node the HTTP server completes the handshake (`ws.handleUpgrade`) after the same authentication and guards. So the session now **decides** and the host **accepts**. ### What changed - **`UpgradeDecision`** (`session/connection-authenticator.ts`): `authorize(request)` runs every guard and returns either `{ kind: "reject", response }` or `{ kind: "accept", role, attach(ws) }`. Guard order is unchanged and still entirely after token validation: 403 wrong sandbox id → 401 invalid token → 410 session terminal → 410 sandbox stopped → 403 credentials changed. `attach` is a **one-shot capability** closed over the admitted identity and the request-scoped logger, so authorize → host handshake → attach is the only successful path by construction. Sandbox attachment is **prepare-then-commit**: arming the inactivity alarm is the one fallible await and runs first; adopting the socket, the ready status, and the broadcasts follow synchronously, so a failed handshake leaves the previous bridge in place and publishes nothing. The authenticator implements the narrow `SessionUpgradeAdmission` interface hosts program against. - **Cloudflare adapter** `src/cloudflare/websocket-upgrade.ts`: authorize → `WebSocketPair` → attach the server half → 101 with the client half; on attach failure it closes the server half and returns 500. `SessionDO.fetch` routes `Upgrade: websocket` requests to it; everything else still goes through `server.onRequest`. - The HTTP dispatcher no longer has an upgrade branch or dep, and `createUpgradeSockets()` is gone from the manager. `SessionRuntime` gains `upgrades`; the request-correlation child logger moved to `session/request-logger.ts`, shared by the dispatcher and the authenticator. - **Prod → public reconciliation** (first commit): the two manager hunks production has carried since #1586 land on the `SocketHost` port. Accepting a bridge now closes every other live sandbox socket (not only the cached pointer, which hibernation drops), and the cached socket is validated against the persisted sandbox id before the fast-path return. Two tests ported with them. ### Deviations from the issue text - The accepted decision carries the attachment rather than the identity fields (`sandboxId` / `wsId`); the identity is closed over. `ClientInfo` is built at `subscribe`, not at upgrade, so there was nothing else to carry. - `sockets.accept(server, tags)` stays inside the manager (`acceptClientSocket` / `acceptAndSetSandboxSocket`) rather than moving to the adapter: the tags are manager-owned identity and the manager already reaches the host through the `SocketHost` port, so a Node host gets the same tagging for free. ### Done when - [x] No `WebSocketPair` or `webSocket:` outside `src/cloudflare/` (`index.ts:141` is the Worker-level forward, untouched per the issue). - [x] `websocket-sandbox.test.ts`, `websocket-client.test.ts`, the #1577 410 race tests and the credentials-changed 403 test pass unchanged. - [x] `connection-authenticator.test.ts` drives `authorize` with fake collaborators and asserts the decision for each guard, including the mid-hash mutations, and `attach` for both roles, the prepare-then-commit ordering, the nothing-committed failure path, and one-shot attachment. - [x] `index.ts handleWebSocket` untouched. ### Verification - `npm run typecheck -w @open-inspect/control-plane`: clean - Unit: 245 files, 3,619 tests pass - Workerd integration: 96 files, 1,129 tests pass (the two "force eviction" uncaught-exception lines are the eviction test's deliberate abort) ### Follow-up filed, not in this PR The deep review noted that closing replaced sandbox sockets is cleanup, not a dispatch fence: the message router processes frames from any `sandbox`-tagged socket without checking it against the active one. That predates this PR (and the prod hunk it reconciles) and needs its own design for stale-bridge trailing events and hibernation recovery, so it is filed as COL-128 (P-9), blocking N-8. Closes COL-84. PR #1513 closed as superseded. https://claude.ai/code/session_01E3k9fw7GE4HMYHh86vKxXp <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added host-managed WebSocket upgrades for session connections. * Added request correlation using trace and request IDs in logs. * **Bug Fixes** * Improved validation for sandbox WebSocket connections, including stale credentials and changed sandbox identities. * Replaced all active sandbox sockets when a sandbox reconnects, including sockets retained during hibernation. * Improved handling of upgrade authorization failures and attachment errors. * **Tests** * Added coverage for connection authorization, lifecycle events, socket replacement, and stale connection recovery. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
SocketRegistry<Connection>contract for session socket lifecycle and transport operationsDurableObjectSocketRegistryVerification
npm run typecheck -w @open-inspect/control-planenpm run lint -w @open-inspect/control-planenpm run build -w @open-inspect/control-planenpm test -w @open-inspect/control-plane(2,742 tests)npm run test:integration -w @open-inspect/control-plane(861 tests)Closes COL-50.
Created with Open-Inspect
Summary by CodeRabbit
Refactor
Tests