fix(iroh-v2): check the team socket cap before opening the session

[teamleaderleo]

The failure

TeamControl.fetch checked TEAM_SOCKET_LIMIT after await broker.open(...) had
already returned, and after scheduleChanges had already queued the broadcast.
So for a team sitting at 4096 sockets, every further /socket attempt ran the
whole open path and then threw the result away:

• verifyDeviceSignature, an Ed25519 verification over caller-supplied bytes
• consumeDeviceProof, a storage write
• issueChallenge for a device that is not enrolled, another storage write
• observeAuthority, which records fresh authority and increments the team
  revision
• scheduleChanges, which waitUntils a directory.changed.v1 frame to every
  socket in the object plus a dashboard.broadcast to every dashboard socket

and only then returned 429.

The cost does not stop at the rejected caller. Each refused attempt woke every
client already connected to that team with a revision invalidation, and each of
those clients answers a revision invalidation by re-requesting the directory.
A cap that exists to shed load was amplifying it, worst exactly when the team
was already at capacity.

Why this is a defect and not a design choice

The function's own stage tracking says the order was meant to be the other way
around: stage = "open" is set before broker.open, stage = "accept" after
it, and the cap check sat in the "accept" stage. Admission was intended to
precede the session open.

Neither relocated check reads anything broker.open produces. The cap reads
this.ctx.getWebSockets().length; the upgrade check reads a request header.
result is used only from the line after them onwards. The sibling dashboard
path in dashboard-control.ts already checks its cap before it reserves or
accepts anything, so the native path was the outlier.

The rejected caller sees exactly what it saw before: 429, retryable, 5000 ms
backoff. Failure telemetry still reports route: "socket", stage: "accept", so
existing queries keep working.

Changes

• src/team-control.ts: the upgrade and cap checks move ahead of
  broker.open, scoped to incoming.path === "/socket" so /session
  admission is untouched.
• src/team-control.ts and src/dashboard-control.ts: the cap becomes one
  overridable value that both admission paths read. dashboard-control.ts had
  the literal 4096 copied by hand, so the two caps could drift apart silently.
  It is passed through the existing Services hooks rather than imported,
  because importing it would make the module graph cyclic.
• e2e/control-runtime.test.ts and e2e/control-worker.ts: a runtime case that
  holds a socket open, lowers the cap through a test-only fixture override, and
  pins both side effects that must not happen on a refused connection. The
  fixture lowers the cap because 4096 live sockets are not reachable under
  Miniflare; production never subclasses TeamControl, so it always reads the
  4096 constant.

Red and green

Same command both times: bun test e2e/control-runtime.test.ts in
workers/iroh-v2.

Red, on the test commit (7ad6689):

bun test v1.4.0 (34cbb9a40)

e2e/control-runtime.test.ts:
456 |       headers: { upgrade: "websocket", authorization: `IrohTicket ${aheadTicket}`,
457 |         "x-cmux-v2-setup": setupHeader(await setupFor("over-cap", undefined)) },
458 |     });
459 |     expect(refused.status).toBe(429);
460 |     expect(refused.webSocket).toBeNull();
461 |     expect(await control.teamRevision()).toBe(before);
                                               ^
error: expect(received).toBe(expected)

Expected: 3
Received: 4

      at <anonymous> (/workers/iroh-v2/e2e/control-runtime.test.ts:461:42)
(fail) a team at its socket cap sheds the next socket before it mutates team state [40.80ms]

 12 pass
 1 fail
 98 expect() calls
Ran 13 tests across 1 file. [2.64s]

The second assertion is red on that commit too. With the revision assertion
removed so execution reaches it, the frame that a rejected connection pushed to
the socket already connected is:

461 |     // The broadcast is scheduled with waitUntil, so give it room to arrive.
462 |     await Bun.sleep(500);
463 |     expect(frames.filter(frame => frame.schemaId === "directory.changed.v1")).toEqual([]);
                                                                                    ^
error: expect(received).toEqual(expected)

- []
+ [
+   {
+     "revision": 4,
+     "schemaId": "directory.changed.v1",
+     "teamId": "team-control",
+   },
+ ]

Green, on the fix commit (28341dd):

bun test v1.4.0 (34cbb9a40)

e2e/control-runtime.test.ts:

 13 pass
 0 fail
 100 expect() calls
Ran 13 tests across 1 file. [2.95s]

Also green on the fix commit:

• bun run check in workers/iroh-v2 (boundary:check, contracts:check,
  types:check, typecheck, test): 79 pass, 0 fail, Ran 79 tests across 17 files
• bash scripts/test-runtime.sh, all three Miniflare suites: 13 pass,
  8 pass, 22 pass, 0 fail

No macOS or Xcode build was run; this is a Cloudflare Worker only, and nothing
here ships in the macOS or iOS app.

Changelog

Fixed: a cmux team already at its control-socket limit no longer makes every
connected client resynchronize its device directory each time a further
connection is refused.

🤖 Generated with Claude Code

---

Summary by cubic

Fixes the team socket cap so refused connections no longer run the session-open path or wake every connected client.

Previously TeamControl.fetch checked TEAM_SOCKET_LIMIT after broker.open had already returned and scheduleChanges had queued a broadcast. A team at 4096 sockets ran the whole open path — signature verification, storage writes, authority observation, team revision bump — for every rejected attempt, then pushed that revision invalidation to every socket already connected, and each client re-requested the directory in response. The load-shedding cap was amplifying load.

• Moves the upgrade and cap checks ahead of broker.open, scoped to /socket so /session admission is untouched. Failed requests still return 429 with the same telemetry stage.
• Unifies the cap across team-control.ts and dashboard-control.ts as one overridable value supplied through the existing Services hooks, since the dashboard had the literal 4096 copied by hand.
• Adds a runtime e2e case that pins both side effects that must not happen on a refused connection: the team revision must not move, and no directory.changed.v1 frame may reach connected sockets. The test fixture lowers the cap since 4096 live sockets aren't reachable under Miniflare.

^{Written for commit 28341dd. Summary will update on new commits.}

[coderabbitai]

Warning

Review limit reached

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: aab2df12-feb5-4237-af61-bd1c20ab45fd

📥 Commits

Reviewing files that changed from the base of the PR and between 0c753fe and 28341dd.

📒 Files selected for processing (4)

• workers/iroh-v2/e2e/control-runtime.test.ts
• workers/iroh-v2/e2e/control-worker.ts
• workers/iroh-v2/src/dashboard-control.ts
• workers/iroh-v2/src/team-control.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[teamleaderleo]

Cross-model review (Codex gpt-5.6-sol)

• workers/iroh-v2/src/team-control.ts:84-103 — the shared 4,096-socket cap is checked before several non-storage awaits (broker.open, WebCrypto, DO RPCs), which reopen the Durable Object input gate. With one slot left, concurrent native/dashboard opens can both observe 4,095 and both accept; dashboard-control.ts:48-58 has the same gap, and the later opening set is not included in the cap. Reserve a TeamControl-owned pending admission synchronously with the cap check, share/count it for both paths, release it on every failure/after accept, and add a paused concurrent last-slot test proving only one open mutates state.

[teamleaderleo]

Review: a review subagent went through this at 28341dd7220. Nothing blocking, three nits, safe to merge as-is.

The question that mattered: does moving the cap check ahead of broker.open mean the cap is now evaluated before authentication? No. The caller is fully authenticated in the Worker before this Durable Object is reached: src/routing.ts:78 verifies either an HMAC Iroh ticket or a Stack bearer token (which checks team membership and throws team_access_revoked 403), then chargeOpen consumes the per-user budget, and readInternalRequest refuses anything without the verified-authority header. What broker.open adds is device authorization, not caller authentication, and it never calls verifyTeamMember, so no membership check that used to precede the cap is skipped. Refusing after authentication but before authorization is the right placement, and the work now skipped on a refused socket is exactly the amplification a load-shedding path should not pay for: an Ed25519 verify over caller-supplied bytes, a consumeDeviceProof write, an observeAuthority transaction that bumps the team revision and appends an audit row, then a waitUntil fan-out of directory.changed.v1 to every socket plus a dashboard broadcast, each of which makes clients re-request the directory.

Independently verified rather than taken on trust: the red/green receipt is exact. On the parent commit the test fails at teamRevision() with Expected: 3, Received: 4, and with that assertion removed it still fails on the frame assertion with a directory.changed.v1 frame, so both postconditions discriminate on their own. The relocation is also confirmed safe (the moved block reads only incoming.path, headers, getWebSockets() and socketLimit(); result is a later const, so TDZ plus a clean tsc proves it was unused), the /socket scoping is equivalent since /request and /session both returned earlier, and the broker(teamId) identity check still runs first. The test-only setSocketLimit/teamRevision surface is unreachable in production: only e2e/control-wrangler.jsonc binds the subclass, every deploy script uses the default config, and no env var or binding can move the 4096 literal. stage: "accept" telemetry is unchanged for existing queries, and better than the PR claims, since the parent also emitted a phantom status: 200 success event for a refused socket.

Left, all nits, none needing a code change:

• Error precedence at the cap changes, and this is worth stating plainly: a caller whose device was Forgotten or whose ticket expired used to get a terminal 403 device_revoked, and at a full cap now gets a retryable 429 instead, so the client keeps reconnecting rather than surfacing that the Mac was forgotten. I think the new behavior is right, load shedding must not do authorization work to pick a prettier error code, the revocation is also delivered as device.revoked.v1 on a live socket, and the terminal error returns as soon as capacity frees. But it only bites at 4096 concurrent sockets on one team, which is exactly where backoff is what you want.
• The cap-overshoot window is wider: the check is not atomic with ctx.acceptWebSocket and this.opening is counted by neither version, so concurrent opens can overshoot. The race pre-existed (the old position still had an await and a cross-DO RPC between check and accept); the window now also spans broker.open. The cap is a coarse safety valve well under workerd's socket ceiling and the per-user reservation is the real per-user bound, so nobody should read 4096 as a hard ceiling either way.
• protected is erased at runtime, so socketLimit() is RPC-callable on the production object. It returns a constant and cmux's own Worker is the only holder of the binding, so the guarantee is the doc comment's "nothing deployed subclasses this object", not the keyword.

Licensing: the diff stays inside workers/iroh-v2/, no dependency added, nothing moved into or out of a BUSL directory, no app-shipping code introduced there.

Verification the reviewer ran on 28341dd7220: bun test e2e/control-runtime.test.ts 13 pass 0 fail, bun run check 79 pass 0 fail across 17 files, bash scripts/test-runtime.sh 13 / 8 / 22 pass 0 fail.

[teamleaderleo]

Merging on green under the standing rule for fix PRs (skip team review, dogfood, merge on green).

No fleet dogfood evidence on this one, and the reason is structural rather than a skipped step: #8029 turned off Vercel branch previews while keeping main deployments, so an unmerged change to a deployed worker or service has no preview URL an app build could talk to. A fleet build would exercise main, not this branch, so the clicks would prove nothing about the diff. The evidence here is the executed red/green plus the full check suite, recorded in the review comment above.

[github-actions]

Merge receipt for 28341dd722: every check was green at merge (14 verified; 21 skipped by policy). Full suite runs on main after merge.