Skip to content

cmux-tui: each terminal gets a fair share of the terminal connection lane (on top of #27719's 48-of-64 cap) - #28343

Merged
lawrencecchen merged 15 commits into
feat-cmux-nextfrom
fix/pa-b1
Nov 1, 2026
Merged

lawrencecchen merged 15 commits into
feat-cmux-nextfrom
fix/pa-b1

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #27698 (the per-terminal part)

Summary

Rescoped (lead decision 2026-11-01 05:20Z: "build on it"). #27719 (fbb9e39) is on feat-cmux-next. It caps connections from processes inside the session's terminals at 48 of the daemon's 64, so 16 stay for the app. This PR keeps that cap and adds only a fair share per terminal inside the terminal lane. Before this, one terminal (an agent's hooks in a loop, a script running cmux in a pane) could take all 48 terminal connections, so every other terminal's agents were refused server.busy.

Out of this PR now (the earlier design, see the history): the app's own socket, the RLIMIT_NOFILE budget, and the removal of the 2 s write stall. #27719 covers the app lockout. The others can come back as their own PRs if the A/B data asks for them.

History: 21646a4 merges feat (conflicts take feat's side). 6d91a64 reverts this branch's earlier design to feat. Then the three commits below are the change (red test, fix, test and doc follow-up from the review).

Changelog

Fixed: one terminal opening many connections to the session daemon no longer takes every terminal connection; other terminals' agents still connect.

Testing

server/tests/outbound_and_scheduler.rs::one_terminal_cannot_take_the_terminal_lane_from_the_others: terminal 101 claims connections in a loop; it must stop at most at half the lane, terminals 102 and 103 must each get one, and the app's slots must be untouched.

  • Red at 66808c6 (the client id threaded through with no behavior change, plus the test): fleet step 74e90a163f9834b0cc464cf0 (cmux-tui-rust-check.sh test one_terminal_cannot_take_the_terminal_lane, macOS).
  • Green at 20f7f9c (the test also claims again after the release and must reach the same count, so a leaked share fails it): results in a comment. At 8fc1e8a: compile 4fb9c091cd1a5ccb9eb2d9a6 passed; test server:: c74aa48508c5 and clippy b7e7a3f2c43e in a comment.

Observed / Not run

  • Observed: the fleet steps above (results in a comment on this PR when they end).
  • Not run: an Austin's-day A/B of this narrower change. The earlier A/B (comment above) measured the app-socket design.

No capture: daemon change, no UI.

Owner: PL-daemon (session cmux-b6) owns cmux-tui-core.

…oad (red, #27698 B1)

Austin's nightly-next (2026-10-29): about 80 terminal agents held all 64
daemon connections, 55,162 were refused, the app's among them.

Red tests, with behavior-neutral seams (ConnectionBudget at the old 64,
ConnectionLane/ConnectionSlot, app_lane_socket_path) so they compile on
the base:
- an_app_connection_is_served_while_the_session_socket_is_full
- one_terminal_opening_many_connections_cannot_starve_another
- the_connection_budget_follows_the_open_file_limit
- a_client_that_stops_reading_for_a_while_keeps_its_connection
- AppLaneConnectionTests (Swift): the app connects on <socket>.app
…res; budget from RLIMIT_NOFILE; no 2 s write kill (#27698 B1)

Austin's nightly-next (2026-10-29): about 80 terminal agents held all 64
daemon connections; 55,162 were refused in 2.5 h, the app's among them.
The Austin's-day A/B on #28159 showed 64/64 with ~3,700-4,250 refusals
per 6-min run.

- The app's own socket, `<session socket>.app` (app_lane_socket_path),
  bound under the same start lock. Each socket has its own budget, so
  terminals, agents, hooks and the CLI (session socket) can never take
  the app's connections. The app (LineTransport) connects there first and
  falls back to the session socket (older daemon, stale file, long path).
  Chromium: the browser's own IPC channels never compete with renderer
  connections (render_process_host_impl.cc gives each renderer its own).
- A fair share per client on the session socket: the client is the peer's
  process session (getsid), so a terminal's shell, agent, hooks and cmux
  calls count as one. A client may take a connection only while it then
  holds fewer than the socket has left free: one terminal stops at half
  the lane, and every new client still finds room. Remote entry and
  WebSocket clients keep the lane budget only (no identity at accept).
- The budget comes from the soft RLIMIT_NOFILE the owner already raises at
  start (owner_start, raise_open_file_limit: hard limit, at most
  kern.maxfilesperproc / OPEN_MAX): half of the descriptors left after
  start, 8 per slot (4 descriptors per connection, 2 connections per slot
  with the hung-up pool), half per socket; on macOS also bounded by
  kern.num_taskthreads (2 threads per connection). 64 and
  MAX_SERVER_CONNECTIONS are gone; the parked-connection bound follows it.
- No write deadline: STREAM_WRITE_TIMEOUT (2 s) closed a connection whose
  client read late (a stalled app main thread lost its connection and
  resynced). Now a client is ended only when its replies overflow the
  bounded control queue (QueuedSink evicts and shuts the socket); the
  WebSocket ping never waits on a peer that does not read, and the ping
  clock shuts a dead peer's socket.

Replaces the stopgap split of #27719 (48/64 for terminals).
…not a 3 s sleep (R5, #27698 B1)

StalledSocket models the kernel in memory: a daemon write waits while the
client's buffer is full, and one that waits when the test declares the
stall fails as SO_SNDTIMEO does if a write deadline is set (red on the base:
2 s STREAM_WRITE_TIMEOUT), else keeps waiting (green). The app-lane test
reads with no deadline: the daemon answers or refuses with one line.
@lawrencecchen lawrencecchen added the risk:high Risk tier (docs/ci/risk-tiers.md) label Oct 30, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

PL-daemon (owner of cmux-tui-core, OWNERS.md): this PR changes connection admission (diagnostics/connection_budget.rs, diagnostics/connection_leases.rs, server/listen.rs, server/restoring.rs) and the outbound write path (bounded_outbound.rs, line_connection.rs, websocket_listener.rs: no write deadline; overflow of the control queue ends the connection). R9 rows addressed: 3 (R-L1), MAX_SERVER_CONNECTIONS 64 / MAX_PARKED 32 (R-L1, R-L2), 4 (R-L5). It replaces the #27719 stopgap.

@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review d0db479: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr28343 -f ref=fix/pa-b1 -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); base red: the same failure on the base 9b4e5a5; does not block

Shipped in:

@blacksmith-sh

blacksmith-sh Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Found 4 test failures on Blacksmith runners:

Failures

Test View Logs
main.DeveloperIdAppTests.test_download_prints_the_active_profile_only/
test_download_prints_the_active_profile_only
View Logs
stubs: /System/Library/Frameworks/Security.framework/Versions/A/Security has no stub/
Security has no stub
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_CFRetain']/
CoreFoundation, missing 1: ['_CFRetain']
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_log2']/
CoreFoundation, missing 1: ['_log2']
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

… prints its own prefix first) (#27698 B1)

the_connection_budget_follows_the_open_file_limit failed on both the red
and the green fleet runs (16894a02, 3830e38f) at connection_lanes.rs:123:
the child printed "connection-budget=30" after libtest's "test <name> ... "
on the same line, and the parser only took a line that starts with it.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent review subagent, Claude Opus 5.5): FIX at 6aaecf6
reviewer-session: c1891b34-9b98-4010-89a3-95fb5ae4cdbd
author-session: pa-b1
R9: R-L2 (the app socket shares the session's outbound byte budgets with the session socket; with no write deadline a stalled terminal client can hold them indefinitely, see finding 2). R-L3 (no retry_after_ms on server.busy) is deferred to pa-b2 as the PR says.
perf claim: none

Findings

  1. blocker: the PR causes its own CI red. clippy (changed cmux-tui packages) fails on this head (run 41064429499, job 124092017529). cmux-tui.yml keeps clippy -D warnings blocking. Every error is in a file this PR changed:

    • cmux-tui/crates/cmux-tui-core/src/server/bounded_outbound.rs:601: SynchronizedTcpStream::set_write_timeout is now dead code, because the WebSocket path no longer calls it. Delete it.
    • cmux-tui/crates/cmux-tui-core/src/diagnostics/connection_budget.rs:239: limit.rlim_cur as u64 casts u64 to u64 (unnecessary_cast).
    • cmux-tui/crates/cmux-tui-core/src/server/tests/connection_lanes.rs:170,178,180,196: unnecessary qualification (std::sync::Mutex etc.).
    • connection_lanes.rs:146: unnecessary cast.
    • connection_lanes.rs:349: redundant clone.

    The fleet test job 652657654d38999d5d336cb0 shows the same warnings. The compile check passed only because it does not deny warnings.

  2. should-fix (R-L2, R-L5 interaction): the app's lane is not isolated in outbound memory, and a stalled client now holds shared memory for as long as it stalls.

    • server/restoring.rs:146 makes one RenderService per session. ServedSession::serve (server/listen.rs:456) uses it for both lanes. So the app socket and the session socket share one pair of byte budgets: OUTBOUND_GLOBAL_BYTE_CAPACITY and OUTBOUND_GLOBAL_CONTROL_BYTE_CAPACITY, 4 x 32 MiB each (server.rs:2587-2588).
    • Every queued message holds its bytes in that pool until it is written (render_service.rs BudgetedText).
    • Before this PR, the 2 s write deadline ended a non-reading connection, and its queued bytes went back to the pool within about 2 s.
    • Now, a terminal client that stops reading but keeps its socket open holds its queued bytes until it reads or exits. Examples: a cmux/cmux-tui frontend stopped with Ctrl-Z, a client in a debugger. The bytes are the in-flight item, two items per backpressured stream, up to 256 per non-backpressured stream, and initial vt-state.
    • One connection's own cap (OUTBOUND_CONNECTION_BYTE_CAPACITY, 8 x 32 MiB) is larger than the whole session pool.
    • When the pool is used up, BudgetedJsonWriter fails with WouldBlock (render_service.rs:182). MessageWriter::send_control then closes the requesting connection (message_writer.rs:289-300), and that can be the app's connection. Streams overflow and the app resyncs. This is the cmux NEXT: agents in terminals can fill the daemon's 64 connections and lock the app out #27698 symptom (the app loses its connection because of terminal clients), but caused by a different mechanism, and it now lasts for the whole stall instead of 2 s.
    • evict_on_overflow (bounded_outbound.rs:722) ends a connection only when that connection's own control queue overflows. It does not cover regular-queue bytes. It also does not cover the terminate_stream_locked path (bounded_outbound.rs:363): there, a full control reserve sets closed but does not shut the socket down, so a blocked writer stays blocked and keeps its bytes.
    • Smallest principled fix in scope: give the app lane its own RenderService (its own budgets; ServedSession keeps one per lane; remote entry and WebSocket already have their own instances). Follow-up: when the pool refuses, evict the connection that holds the most bytes and is not being drained, not the requester.
  3. should-fix (cheap): the cleanup order can delete the next owner's app socket. server.rs:2869-2872 cleanup removes the session socket first and <socket>.app second. release_ownership runs it without the start lock.

    • The race: after the first unlink, a waiting successor (server ensure) can bind both files (listen.rs:402,416-432 removes the old .app and binds a new one). Then the predecessor's second unlink deletes the successor's live .app file.
    • The app then silently stays on the session socket, under the fair share, for that daemon's whole life. On a loaded Mac (the exact case this PR targets), the predecessor can be preempted between the two unlinks.
    • Fix: remove .app first, then the session socket. A successor cannot bind until the session file is gone.
  4. should-fix (PR description reds, not product code):

    • pr-capture-check: the check classifies the Swift change as native UI. Add No capture: <why>, for example "transport-only change, no UI".
    • pr-perf-section-check: add a ## Performance heading with No perf numbers: <why>, or the Austin's-day numbers when they exist.
    • These are own reds, and next-autoland keeps the PR out while they are red.
  5. note: the app socket is a convention, not a reserved class. Any same-user process can connect to <socket>.app. That includes a terminal agent that lists the socket directory. Such a process takes the app's budget with no fair share. Authority is unchanged (see the security line below), so this is resource isolation, not a privilege bypass. A cheap hardening for later: on the app lane, refuse a peer that origin_gate::classify_local_peer classifies as Terminal. Do this on the connection thread, never on the accept thread, which must not take the state lock.

  6. note: the Swift fallback happens on any connect error (LineTransport.swift:126). On macOS, a unix connect to a full backlog also gives ECONNREFUSED, the same error as a stale file. So a burst of app connects larger than the backlog lands on the session socket under the very load this PR is for. This is unlikely with a backlog of 128 and a fast accept loop. One way to handle it: log a fallback when the .app file is a live socket.

  7. note: the fair share is "half of what is left", not equal shares (connection_budget.rs:153). The first greedy session keeps 50% of the lane, the next 25%, and so on. New clients always find room while held <= limit - 2, which meets the stated goal. Also, at the floor lane = 1 (connection_budget.rs:79), no client with an identity is ever admitted on the session socket (1 < 1 is false). This cannot happen with a realistic RLIMIT_NOFILE.

  8. note: the hung-up pool is one pool for both lanes, bounded by one lane's limit (connection_budget.rs:188, connection_leases.rs).

    • Hung-up session-socket connections can fill the pool, and then hung-up app connections cannot be reclaimed.
    • A refused app claim also reclaims session-socket leases. This is harmless.
    • Every over-share refusal now runs the O(leases) poll sweep on the accept thread. Before, the sweep ran only when the lane was full.
  9. note: the WebSocket read loop can still wait on a peer that does not read. websocket_listener.rs:447: when a Ping arrives, tungstenite flushes the Pong through SynchronizedTcpStream::write (and read() flushes any pending Pong too). That write takes write_lock with a blocking lock(). If the writer thread holds the lock while it is blocked on that peer, the read loop blocks too, and the dead-peer clock does not run until TCP gives up. The ping change itself (try_lock) is correct.

  10. note: Windows. bind_app_lane checks the path length only #[cfg(unix)]. On Windows, or for an .app path that cannot be removed (for example a directory), the ? at listen.rs:425,429 stops the whole daemon from starting. The app socket is optional, because the app falls back, so a bind failure should fall back to None too.

  11. note: test seams in production source.

    • ConnectionBudget::for_test (connection_budget.rs:84) and ConnectionStats::set_budget_for_test (:221) are pub, and only in-crate tests use them. Make them #[cfg(test)].
    • budget: Mutex<ConnectionBudget> (diagnostics.rs:486) exists only so a test can replace the budget. Production takes that lock on every claim.
  12. note: test strength.

    • an_app_connection_is_served_while_the_session_socket_is_full fills only one identity's share (4 of 8), not the whole lane. It also passes on Linux even when the peer identity is None.
    • The Rust green ran on Linux (/home/runner). The macOS-only paths (kern.num_taskthreads, /dev/fd counting, getsid on the macOS peer pid) were only compiled.
    • Recommendation: run connection_lanes and connection_budget::tests once on a fleet Mac.

Checked and fine

  • Lane and identity accounting: claim_slot and give_back run under one lanes lock, so they are consistent. A slot is released once only: ConnectionLease::drop releases it unless hung_up, and reclaim releases it only after a CAS on hung_up. Counters cannot underflow (saturating lane count; the identity entry is removed at 0). active and peak are updated after the claim.
  • Fair-share arithmetic matches its doc and tests: one client alone stops at about L/2. With L=16 the counts go 8, 4, 2, 1, and the app lane is untouched.
  • Budget derivation: from_limits = (soft − held)/2/8 slots, bounded on macOS by threads/2/4, split in half between the lanes, with a floor of 1. It is read after owner_start::prepare raises the limit (main.rs:2261 comes before bind at :2418 and before Mux construction at :2439+). The worst case is total + one lane of connections (pool bound) times 4 fds, which is ≤ half of the fds left. The unit test arithmetic is correct.
  • Restore-time parked bound: computed from the raised limit. Each lane parks its own connections, so probes cannot evict the app's parked connections. On attach, the app's parked connections are served first, and the total stays ≤ the lane budgets.
  • App socket lifecycle:
    • It is bound under the start lock, after the session socket is known not to be live, and a stale file is removed.
    • It gets restrict_file.
    • Every shutdown path removes it through cleanup: SocketFile drop, PendingServer drop, RestoringServer drop, the serve_restoring spawn-failure path, and release_ownership.
    • Both accept loops are woken on PendingServer and RestoringServer drop.
    • If the app accept loop fails to spawn, the session loop is woken.
    • A path that does not fit sun_path falls back to None on unix.
  • Security: the app socket adds no method and no authority. Its connections go through the same handle_connection_with_permit. origin_gate::set_peer_identity_at_accept and verified_app (audit token) classify by peer, not by socket. Accept still refuses other uids. Remote entry and WebSocket cannot reach it. No relay allowlist changed, so the relay-authorization analysis does not apply.
  • Removing the write timeout does not leak slots from dead clients. A client that hangs up gives EPIPE or POLLHUP, and reclaim still works. A live client that does not read holds its slot like an idle client. Daemon shutdown and handoff do not join connection threads. The shutdown-ack flush and the detach flush keep their own bounded waits.
  • evict_on_overflow triggers only on WouldBlock from the connection's own control reserve. The old code already closed the writer in that case, so no healthy client is newly evicted.
  • WebSocket dead-peer shutdown now wakes a writer that is blocked on the socket. The ping skips while the writer holds the lock.
  • Swift: only the app links CmuxNextDaemon (the server helper does not). Every non-bridge LineTransport caller passes a cmux-tui session socket. A missing or stale file costs one failed connect syscall (ENOENT or ECONNREFUSED; AF_UNIX connect does not block on macOS). The bridge path is unchanged.
  • Tests: no timer decides correctness. StalledSocket models SO_SNDTIMEO as a signal, the Swift test polls with a 0 timeout, and the budget test runs child processes. The red seams in 4637c8e are behavior-neutral (a budget at the old 64, the old try_claim). The outbound_and_scheduler.rs changes keep the same 64 cap and the same pool-bound semantics with for_test(64). The server_stats limit assertion now follows the derived limit, which is appropriate.
  • Fleet evidence:
    • Rust green 652657654d38999d5d336cb0: 4/4 passed (Linux).
    • Red c9998cd24bcce70bf4caee0e: per the PR.
    • Swift green d8defea597ba534c7ed62b28: done, 3/3 passed.
    • Swift red 5310919572130ece41519c78: failed as expected. theAppConnectsOnItsOwnSocketWhenTheDaemonHasOne failed at AppLaneConnectionTests.swift:62/63. The two fallback tests pass on the base, as they should.
  • Other CI reds not caused by this PR: protocol contract and inventory fail on cmux-tui/src/cli.rs:767-771 (resource-word boundary); this PR does not touch that file. tooling-tests fails on tests/test_cmux_tui_owned_mini_caches.py, which has no workflow lane; this PR did not add that file. release-notary fails on the highlight chief-name-in-agent-chats.md, which this PR did not add.

… .app first; clippy (review of 6aaecf6, #27698 B1)

Review FIX (landing/reviews/6aaecf6a9207...md):
- Blocker: clippy -D warnings on changed files: SynchronizedTcpStream::
  set_write_timeout is dead with no write deadline; same-type casts,
  unnecessary qualifications and a redundant clone in the tests.
- The two sockets shared one RenderService, whose outbound byte budgets
  a session-socket client that stops reading now holds until its queue
  overflows (no 2 s write kill). The app's replies could then fail on a
  full pool and the app's connection close: #27698 by another path. The
  app socket now has its own RenderService.
- cleanup removes <socket>.app before the session socket, so a daemon
  that binds right after the first removal keeps its fresh .app.
… outbound bytes (review of 6aaecf6, #27698 B1)

Regression test for the app socket's own RenderService (0a0796c):
spend the session socket's outbound and control budgets, as clients that
stopped reading hold them now that no write deadline frees them; a
session-socket identify then gets no reply (the test checks it proves
something), and the app socket's identify is still answered.
ServedSession::new builds the session's two budget sets in one place.
Red proof: cp-scratch/pa-b1-red-budget (6aaecf6 + this test, with
ServedSession::new serving both sockets from one RenderService).
…on starts without its app socket (red, #27698 B1)

Lead review of 5631798, two defects for Austin's load:
- every_lane_admits_a_client_at_the_smallest_budget: at the smallest
  budget (one connection per lane) the fair share refuses every client.
- the_session_socket_serves_when_the_app_socket_cannot_be_bound: a
  directory nobody may remove at <socket>.app makes the whole daemon fail
  to start.
…ane has room; the daemon starts without its app socket (#27698 B1)

- The fair share admitted a further connection only while the client
  then held fewer than the lane had left free, so a lane of one (the
  smallest budget an fd limit derives) refused every client. A client's
  first connection now gets in whenever the lane has room.
- A failure to bind <socket>.app (a path that can't be removed, Windows)
  failed the whole start. The app socket is an addition: the daemon now
  logs why and serves the session socket alone; the app's LineTransport
  already falls back to it.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent review subagent, Claude Opus 5.5): SHIP at bfff9fd
reviewer-session: f5f685ae-e35a-403d-a461-c58e03b7da84
author-session: pa-b1
R9: pass
perf claim: none

Status of the first review's findings (6aaecf6)

  1. Clippy red: fixed.
    • The dead SynchronizedTcpStream::set_write_timeout is removed.
    • The u64 casts are removed (connection_budget.rs:243, connection_lanes.rs:232).
    • The qualifications and the redundant clone are fixed.
    • Fleet clippy e7ae69bcb2bbf14dea1d75e4 at this head passed. The PR check clippy (changed cmux-tui packages) passes.
  2. App outbound budgets shared with the session socket: fixed.
    • ServedSession::new (server/listen.rs:136-142) builds a separate RenderService for each socket. serve picks the right one by lane (listen.rs:494-497). restoring.rs:144 uses ServedSession::new.
    • A stalled session-socket client can no longer spend the bytes that the app's replies need.
    • The new test the_apps_replies_never_wait_for_the_session_sockets_outbound_bytes exhausts both budgets of the session socket. It first checks that the precondition holds (a session-socket identify is not answered). It then requires the app lane to answer.
    • The test is red at e3b0e14 (job 91231c51aa4b0a48c342a014, :143) and green on Linux and on a Mac.
  3. Cleanup race: fixed. cleanup (server.rs:2873-2876) now removes .app before the session socket. A successor cannot bind until the session file is gone, so the predecessor never deletes a successor's .app.
  4. Description reds: fixed. pr-capture-check and pr-perf-section-check pass at this head. The body now has ## Performance with No perf numbers: <why> and a "Known limits" section.

The lead's two delta fixes are correct:

  • A lane of one admits a client (connection_budget.rs:157, mine == 0 || mine + 1 < limit - held).
    • A client's first connection is admitted whenever the lane has room.
    • The greedy limit is unchanged: one session still stops at about half of the lane.
    • The lane bound held >= limit is still checked first, so a lane can never go over its budget.
    • Trade-off: many distinct one-connection sessions can now fill the lane to its last slot. Before, the last slot was kept for clients with no identity. This matches the stated setsid limit and does not affect the app socket.
    • The test every_lane_admits_a_client_at_the_smallest_budget covers a lane of one, including the refusal of a second client.
  • The app socket cannot stop a start (listen.rs:404-415).
    • Any bind_app_lane error is logged, and the daemon serves the session socket alone.
    • If the error comes after the listener is bound (from restrict_file), the SocketFile drop removes the partly created .app. It cannot remove an object it does not own, such as a directory; the test checks that keep survives.
    • Drop and cleanup still call connect and remove_file on that path. Both fail harmlessly.
    • The test the_session_socket_serves_when_the_app_socket_cannot_be_bound covers this. It is red at 703547c (job add8a471bd9554026ec90e48, :133).

R-L2 judgment: pass

  • The app has a reserved class: its own socket, its own connection budget, and now its own outbound byte budgets.
  • Each session-socket client gets a share (its process session).
  • The PR body states the remaining limits (setsid helpers, remote entry and WebSocket without identity, an .app that any same-user process can open, the fallback on any connect error, the shared hung-up pool, the WebSocket Pong wait, pub test setters). None of them lets a terminal client lock the app out of its connections or its reply memory, which was the cmux NEXT: agents in terminals can fill the daemon's 64 connections and lock the app out #27698 failure.
  • Fire-and-forget hook traffic on its own channel, and R-L3/R-L4 (retry_after, jittered retry), are assigned to pa-b2. That is acceptable for this batch.

New notes (none block)

  • note: within the session socket, a stalled client still holds its queued bytes until it reads or exits. Example: a cmux/cmux-tui frontend stopped with Ctrl-Z. While those bytes are held, they are taken from the other session-socket clients' budget (CLI, hooks, terminal frontends), and this lasts longer than the old 2 s. The "Known limits" section does not state this. Add it there, or plan the follow-up: when the budget refuses, evict the connection that holds the most bytes and is not being drained, not the requester. The app is not affected.
  • note: the Swift app does not check who owns <socket>.app (LineTransport.swift:126). It also does not check the session socket. The trust comes from the private runtime directory, so .app gets the same trust as the session socket in the derived path.
    • A gap exists only for an explicit socket path in a shared directory such as /tmp. There, another user could own a live <socket>.app that the daemon cannot remove. The daemon now logs and continues, and the app would still try .app first.
    • Cheap hardening for later: accept .app only when getpeereid returns this uid, as ProviderConnection.swift:86 already does.
  • note: R5. The identify_on test helper (connection_lanes.rs:246-257) sets a 5 s read deadline. In green runs, the session-socket check gets end of stream at once (send_control fails and the writer shuts the socket). The app check gets its reply at once. The Mac run took 0.229 s, so no timer decides a result. The deadline only ends a red run, as its comment says.

Checked and fine

  • No change in the delta touches the accounting from the first review: the lanes lock, single release, and the lease CAS.
  • Budget derivation, the restore-time parked bound, and app socket binding under the start lock with restrict_file are unchanged apart from the fallback.
  • Fleet evidence at this head:
    • Rust connection_lanes on Linux, f90c529a6e7a84aa37841141: passed.
    • Fleet Mac aws-m4pro-4, f4a34935b708a89cd8229f90: 7/7 passed. This run executes the macOS-only kern.num_taskthreads and /dev/fd code. My first review's request is met.
    • Compile check 0100b7d5e2ddbdc647cbd2d5: passed.
    • Reds: add8a471bd9554026ec90e48 (:109, :133) and 91231c51aa4b0a48c342a014 (:143).
    • Swift is unchanged since 6aaecf6: green d8defea597ba534c7ed62b28 passed 3/3; red 5310919572130ece41519c78 failed at :62/:63.
  • The remaining PR check reds are base reds in files this PR does not touch, the same as at 6aaecf6:
    • protocol contract and inventory: cmux-tui/src/cli.rs:767-771, the resource-word boundary.
    • tooling-tests: the registry lane of tests/test_cmux_tui_owned_mini_caches.py.
    • release-notary: highlight chief-name-in-agent-chats.md.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Oct 30, 2026
…26-11-01 05:20Z)

The app socket, the RLIMIT_NOFILE budget and the removal of the 2 s write
stall leave this PR: #27719 (fbb9e39) already keeps 16 of the 64
connections for the app by capping terminal clients at 48, and the lead chose
to build on that cap. The tree is feat-cmux-next again here; the next commits
add only the per-terminal fair share inside the terminal lane.
…thers (red)

Builds on #27719's 48-of-64 terminal cap (lead decision 2026-11-01 05:20Z).
The accept loop already knows which terminal a peer runs in (its process
session); this commit carries that id to the terminal claim and the lease,
with no behavior change, and adds the test. Red here: terminal 101 takes all
48 terminal connections and terminal 102 finds none.
…nal lane

On top of #27719's 48-of-64 terminal cap (lead decision 2026-11-01 05:20Z): a
terminal's first connection gets in whenever the lane has room; a further one
only while that terminal then holds fewer than the lane has left free. One
terminal opening connections in a loop stops at 24 of the 48, and every other
terminal still finds room. The share and the count change together under one
lock; a hung-up client's reclaim gives back its terminal's share too. A
connection whose terminal is unknown keeps the lane bound alone.
@github-actions github-actions Bot added risk:normal Risk tier (docs/ci/risk-tiers.md) and removed risk:high Risk tier (docs/ci/risk-tiers.md) labels Nov 1, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-review-subagent-cw): SHIP at 8fc1e8a

Scope: git diff 6d91a64778a 8fc1e8aba71 (7 files, +131/-38). git diff origin/feat-cmux-next...8fc1e8aba71 shows the same 7 files and nothing more. 6d91a64 has the same tree as the merge base 9b4e5a5, so the revert leaves no old-design code. I read the code only. I did not compile or run tests.

Correctness (pass)

  • Share math. With limit L = 48, a terminal that holds h > 0 gets one more only if h + 1 <= L - active - 1. One terminal alone stops at h = 24 (2h <= 46 lets it go from 23 to 24; at 24 it stops). The test bound first.len() < 48/2 + 1 asks for <= 24, so the bound is exact. On the base the loop takes 48, so the test is red there, as the fleet step shows.
  • Atomicity. Each increment of terminal_active (with client and without client) occurs under terminal_clients, after a check against limit. Thus terminal_active can never go above limit. A release decrements the map, unlocks, then decrements the count. A claim that runs between these two steps sees a count that is too high and refuses. It never admits too many.
  • Release exactly once. The lease keeps its client. Drop releases only when hung_up is false. reclaim_hung_up releases only after its compare_exchange(false, true) on hung_up succeeds. Drop has &mut self, so it cannot run at the same time as a reclaim that holds an upgraded Arc. Both paths give the same client to release_terminal.
  • Lock order. The leases lock and the terminal_clients lock are never held together. reclaim_hung_up collects the upgraded leases in a block and releases leases before it calls release_terminal. claim_terminal_slot drops its guard on return, so try_claim_terminal calls reclaim_hung_up when terminal_clients is not held.
  • No map leak. A refused claim returns before entry(), so it adds no entry. A release removes the entry at 0. The map can hold at most 48 entries.
  • client == None: the result is the same as the old fetch_update (active < limit, then +1). The difference is that the claim now also takes the mutex, which is a short hold on the accept path. cmux-tui: agents in terminals hold at most 48 of the daemon's 64 connections #27719's tests pass None and still describe the lane cap correctly.
  • Panic ratchet: the non-test lines add no unwrap, expect, indexing or unchecked arithmetic. Poison is recovered with PoisonError::into_inner, and the arithmetic is saturating.

Findings that do not block

  1. Make the PR title correct before it lands. The title still says "the app gets its own daemon socket; ... connection budget from RLIMIT_NOFILE; no 2 s write kill". This PR removes all three. The squash commit subject comes from the title, so as it is, the feat history will be incorrect. A title edit does not change the head, so this SHIP stays valid.
  2. The description says too much in one sentence. "every other terminal still finds room for its first connection" is true only when one terminal fills its share. If several terminals loop, the shares halve: 24, 12, 6, 3, 1, 1, 1 fill the lane, and the eighth terminal is refused. Write "one terminal cannot hold more than half the lane". Also, the code admits while the terminal then holds no more than (not "fewer than") the free places. Correct this in the description, in the doc comment on try_claim_terminal and in the test doc comment.
  3. The text about the unknown terminal is not correct for production. "A connection whose terminal is unknown (no peer pid) is bounded by the lane alone": in listen.rs, a peer with no pid or no session gets terminal_of_peer == None. That connection goes to the app lane (claim_watched_connection) as in cmux-tui: agents in terminals hold at most 48 of the daemon's 64 connections #27719, not to the terminal lane. Production does not call try_claim_terminal(.., None). Only the tests do.
  4. The last assertion of the test does not prove what its message says. After drop, claim_terminal_connection_from(&connections, 101).is_some() passes also if the map leaked 101's 24 entries, because a held count of 24 with active 0 still admits (25 <= 47). To prove the release, run the loop again and assert that terminal 101 again gets first.len() (24) connections. With a leak, it gets 12. This is REVIEW.md "Assert the behavior the PR adds".
  5. Fairness depends on the arrival order. The first terminal that loops gets 24, the next one 12, and so on. This is a bound, not an equal share. That is acceptable for this fix, but do not call it "fair share" in user-visible text.
  6. Tier and evidence. The PR has the risk:normal label, but the brief said risk:high. I did not change the label. This receipt is on the exact head, so it is valid for both tiers. The description shows the fleet steps at 8fc1e8a as "results in a comment when they end". Put the green result of the test, clippy and compile steps on the PR before ready-to-land.

…e is cleared; the share is "no more than" the free lane (review 6553892057)
@lawrencecchen lawrencecchen changed the title cmux-tui: the app gets its own daemon socket; per-terminal fair share; connection budget from RLIMIT_NOFILE; no 2 s write kill (#27698) cmux-tui: each terminal gets a fair share of the terminal connection lane (on top of #27719's 48-of-64 cap) Nov 1, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-review-subagent-cw): SHIP at 20f7f9c

Scope: git diff 8fc1e8aba71 20f7f9c17fd (2 files, +11/-10). This change has only doc comments and the test. git diff origin/feat-cmux-next...20f7f9c17fd still shows only the 7 files of the change. The admission code has not changed since my SHIP at 8fc1e8a (comment 6553892057), so all the correctness points in that comment apply to this head. I read the code only. I did not compile or run tests.

The earlier findings are now closed

  • Title: it is now about the fair share only. That is correct.
  • "No more than": the doc comment on try_claim_terminal, the test doc comment and the description now agree with the code (held + 1 <= limit - active - 1).
  • Fill: the doc comment and the description now say that several terminals in a loop can fill the lane (about 7). This agrees with the halving 24, 12, 6, 3, 1, 1, 1.
  • No pid: the description now says that a peer with no readable pid never enters the terminal lane. This agrees with listen.rs.
  • Test: after the release, terminal 101 claims again in a loop, and the test asserts again.len() == taken (24). If the release leaked, 101's held count would start at 24 and the loop would stop at 12, so the test would fail. Now the assertion proves its message.

Small points, which do not block

  1. The Testing section says "it must stop below half the lane". The test permits 24 of 48, which is exactly half. Write "at most half the lane".
  2. The History line says "the two commits below are the change". There are now three commits (66808c6, 8fc1e8a, 20f7f9c).
  3. The doc comment on try_claim_terminal still says "A connection whose terminal is unknown is bounded by the lane alone". This is correct for the function. Only the tests call it with None.
  4. Evidence: the compile and the test run at 20f7f9c are queued. Put their green results on the PR before ready-to-land. The PR has the risk:normal label and the brief said risk:high. I did not change the label. This receipt is on the exact head, so it is valid for both tiers.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Fleet evidence at 20f7f9c (rescoped: a fair share per terminal on top of #27719's cap; see the description):

  • Red at 66808c6 (the client id threaded through, no behavior change, plus the test), step 74e90a163f9834b0cc464cf0 (aws-m4pro-8): failed on its assertion, "terminal 101 took 48 of the 48 terminal connections" (outbound_and_scheduler.rs:318).
  • Green at 20f7f9c, step bbe1d25a318e90ce89292068 (aws-m4pro-6, test outbound_and_scheduler): 45 tests run, 45 passed, including the new test and cmux-tui: agents in terminals hold at most 48 of the daemon's 64 connections #27719's cap tests.
  • At 8fc1e8a (the same admission code): test server:: step c74aa48508c54a42db34d59a ran 1361 tests: 1360 passed, 1 failed. The failure is acpmux server::ws_peek::tests::a_connection_that_sends_no_request_is_closed_after_the_timeout, a timing test in acpmux, a crate this PR does not change.
  • Compile (cmux-ci compile-check --package CmuxNext --app, job 3a27e48a4d29946ce279073e): passed.
  • Review: SHIP at this head (comment 6553906274).

@lawrencecchen lawrencecchen added ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds and removed ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds labels Nov 1, 2026
…accept loop asks terminal_of_peer)

clippy step b7e7a3f2c43e at 8fc1e8a: method contains_peer is never used.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-review-subagent-cw): SHIP at d0db479

Scope: git diff 20f7f9c17fd d0db4795215 (1 file, +2/-1). It adds #[cfg(test)] and a doc line on TerminalSessions::contains_peer. The admission code has not changed since my SHIP at 8fc1e8a (comment 6553892057) and at 20f7f9c (comment 6553906274). I read the code only. I did not compile or run clippy.

Dead-code check

The fix is correct: at this head, only the tests in terminal_sessions.rs (mod tests, lines 80-112) call contains_peer, and the accept loop (listen.rs:420) calls terminal_of_peer.

I examined each item that the PR adds or changes. I found no other item that has become dead:

  • terminal_of_peer: listen.rs:420 and contains_peer call it. terminal_of_peer calls current and origin_gate::peer_pid / peer_session_id, so those are still used.
  • claim_watched_terminal_connection: listen.rs calls it.
  • try_claim_terminal, claim_terminal_slot, release_terminal and the field terminal_clients: claim_connection_watching, ConnectionLease::drop and reclaim_hung_up use them.
  • ConnectionLease.terminal (now Option<Option<u32>>): drop and reclaim_hung_up read it.
  • claim_terminal_connection and claim_terminal_connection_from: both are #[cfg(test)]. The #[cfg(test)] import block in server.rs imports them, and the tests use them, so the import has no unused name.
  • PoisonError (new import in diagnostics.rs): claim_terminal_slot and release_terminal use it.

Small points, which do not block

  1. The new doc line writes ``terminal_of_peer`` with double backticks. The file uses single backticks in other places. Rustdoc shows both the same way.
  2. Before ready-to-land, put the green clippy, compile and test results at d0db479 on the PR. This head was changed because clippy found an error, so a green clippy is the evidence for this change. The earlier small points about the description (the Testing line says "below half"; the History line says "two commits") are still open. The PR has the risk:normal label and the brief said risk:high. I did not change the label. This receipt is on the exact head, so it is valid for both tiers.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

3 issues found across 7 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="cmux-tui/crates/cmux-tui-core/src/server/bounded_outbound.rs">

<violation number="1" location="cmux-tui/crates/cmux-tui-core/src/server/bounded_outbound.rs:106">
P2: `client: Option<u32>` lets a caller pass `None` and take a terminal slot with no fair-share limit, which contradicts the rule that peers without a readable PID stay outside the terminal lane. Make `client` a `u32`, so the no-PID case has to use `claim_watched_connection`, and update the test at outbound_and_scheduler.rs:393-401 accordingly.</violation>
</file>

<file name="cmux-tui/crates/cmux-tui-core/src/diagnostics.rs">

<violation number="1" location="cmux-tui/crates/cmux-tui-core/src/diagnostics.rs:707">
P2: This condition strands terminal-lane capacity once multiple terminals have leases: with two terminals holding 23 each, neither can claim either remaining slot. Compare the client count with its half-lane share so two terminals can use their fair shares.</violation>

<violation number="2" location="cmux-tui/crates/cmux-tui-core/src/diagnostics.rs:718">
P2: `release_terminal` updates the client map under its mutex but decrements `terminal_active` after unlocking, so a concurrent reconnect can observe the old lane count and be refused spuriously. Keep both updates under the same lock for known clients.</violation>
</file>

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

pub(super) fn claim_watched_terminal_connection(
connections: &Arc<crate::diagnostics::ConnectionStats>,
stream: &dyn transport::Stream,
client: Option<u32>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: client: Option<u32> lets a caller pass None and take a terminal slot with no fair-share limit, which contradicts the rule that peers without a readable PID stay outside the terminal lane. Make client a u32, so the no-PID case has to use claim_watched_connection, and update the test at outbound_and_scheduler.rs:393-401 accordingly.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/cmux-tui-core/src/server/bounded_outbound.rs, line 106:

<comment>`client: Option<u32>` lets a caller pass `None` and take a terminal slot with no fair-share limit, which contradicts the rule that peers without a readable PID stay outside the terminal lane. Make `client` a `u32`, so the no-PID case has to use `claim_watched_connection`, and update the test at outbound_and_scheduler.rs:393-401 accordingly.</comment>

<file context>
@@ -91,25 +91,37 @@ pub(super) fn claim_watched_connection(
 pub(super) fn claim_watched_terminal_connection(
     connections: &Arc<crate::diagnostics::ConnectionStats>,
     stream: &dyn transport::Stream,
+    client: Option<u32>,
 ) -> Option<ConnectionPermit> {
-    claim_connection_watching(connections, watch_peer(stream), true)
</file context>

Comment on lines +718 to 728
pub fn release_terminal(&self, client: Option<u32>) {
if let Some(client) = client {
let mut clients = self.terminal_clients.lock().unwrap_or_else(PoisonError::into_inner);
if let Some(held) = clients.get_mut(&client) {
*held = held.saturating_sub(1);
if *held == 0 {
clients.remove(&client);
}
}
}
self.terminal_active.fetch_sub(1, Ordering::AcqRel);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: release_terminal updates the client map under its mutex but decrements terminal_active after unlocking, so a concurrent reconnect can observe the old lane count and be refused spuriously. Keep both updates under the same lock for known clients.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/cmux-tui-core/src/diagnostics.rs, line 718:

<comment>`release_terminal` updates the client map under its mutex but decrements `terminal_active` after unlocking, so a concurrent reconnect can observe the old lane count and be refused spuriously. Keep both updates under the same lock for known clients.</comment>

<file context>
@@ -686,7 +693,38 @@ impl ConnectionStats {
+        true
+    }
+
+    pub fn release_terminal(&self, client: Option<u32>) {
+        if let Some(client) = client {
+            let mut clients = self.terminal_clients.lock().unwrap_or_else(PoisonError::into_inner);
</file context>
Suggested change
pub fn release_terminal(&self, client: Option<u32>) {
if let Some(client) = client {
let mut clients = self.terminal_clients.lock().unwrap_or_else(PoisonError::into_inner);
if let Some(held) = clients.get_mut(&client) {
*held = held.saturating_sub(1);
if *held == 0 {
clients.remove(&client);
}
}
}
self.terminal_active.fetch_sub(1, Ordering::AcqRel);
pub fn release_terminal(&self, client: Option<u32>) {
if let Some(client) = client {
let mut clients = self.terminal_clients.lock().unwrap_or_else(PoisonError::into_inner);
if let Some(held) = clients.get_mut(&client) {
*held = held.saturating_sub(1);
if *held == 0 {
clients.remove(&client);
}
}
self.terminal_active.fetch_sub(1, Ordering::AcqRel);
} else {
self.terminal_active.fetch_sub(1, Ordering::AcqRel);
}
}

Comment on lines +707 to +708
let free_after = limit.saturating_sub(active.saturating_add(1));
if held > 0 && held.saturating_add(1) > free_after {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: This condition strands terminal-lane capacity once multiple terminals have leases: with two terminals holding 23 each, neither can claim either remaining slot. Compare the client count with its half-lane share so two terminals can use their fair shares.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At cmux-tui/crates/cmux-tui-core/src/diagnostics.rs, line 707:

<comment>This condition strands terminal-lane capacity once multiple terminals have leases: with two terminals holding 23 each, neither can claim either remaining slot. Compare the client count with its half-lane share so two terminals can use their fair shares.</comment>

<file context>
@@ -686,7 +693,38 @@ impl ConnectionStats {
+        }
+        if let Some(client) = client {
+            let held = clients.get(&client).copied().unwrap_or(0);
+            let free_after = limit.saturating_sub(active.saturating_add(1));
+            if held > 0 && held.saturating_add(1) > free_after {
+                return false;
</file context>
Suggested change
let free_after = limit.saturating_sub(active.saturating_add(1));
if held > 0 && held.saturating_add(1) > free_after {
if held > 0 && held.saturating_add(1) > limit / 2 + limit % 2 {

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Fleet evidence at d0db479 (#[cfg(test)] on contains_peer, after clippy at 8fc1e8a failed with dead_code):

  • clippy (step 03d6de5be9828767ce5571e7, macOS): green.
  • cmux-tui-rust-check.sh test server::t (step f248eace73706d2a0f9d829a): 334 tests run, 334 passed, including one_terminal_cannot_take_the_terminal_lane_from_the_others, cmux-tui: agents in terminals hold at most 48 of the daemon's 64 connections #27719's cap tests and terminal_sessions.
  • Compile (cmux-ci compile-check --package CmuxNext --app, job db50c566822c2c2e79d07186): passed.
  • Red (unchanged): step 74e90a163f9834b0cc464cf0 at 66808c6.
  • Review: SHIP at this head (comment 6553994603).

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 1, 2026
@lawrencecchen
lawrencecchen merged commit 96428aa into feat-cmux-next Nov 1, 2026
179 of 184 checks passed
@lawrencecchen
lawrencecchen deleted the fix/pa-b1 branch November 1, 2026 06:09
@lawrencecchen lawrencecchen added risk:high Risk tier (docs/ci/risk-tiers.md) and removed risk:normal Risk tier (docs/ci/risk-tiers.md) labels Nov 1, 2026
@github-actions github-actions Bot added risk:normal Risk tier (docs/ci/risk-tiers.md) and removed risk:high Risk tier (docs/ci/risk-tiers.md) labels Nov 1, 2026

This branch was previously deployed

3 inactive deployments
artifacts — d0db4795 Deployed Nov 1, 2026 by lawrencecchen via add the Windows daemon to the tree #93961
sentry-dsyms — d0db4795 Deployed Nov 1, 2026 by lawrencecchen via upload cmux-tui dSYMs to Sentry #93961
ci-routing-agent — d0db4795 Deployed Nov 1, 2026 by lawrencecchen via cmux-tui tree preflight #93961
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant