perf(ws): isolate terminal writes and ordinary CLI creates from connection dispatch - #705
Merged
Merged
Conversation
…ction dispatch One supervised socket writer per connection: handlers enqueue to a bounded outbox instead of awaiting socket capacity, so a backlog or slow network flush no longer holds input/attach/liveness dispatch. The writer admits preludes and output under one lock, leases one output frame at a time (with the leased frame still counted in pressure accounting), keeps final-output-before-exit ordering, and closes the connection rather than retrying a possibly partially sent frame. One bounded, serial ordinary-create worker per connection (restore creates keep their gated path): a slow CLI launch no longer delays unrelated panes on the same socket. A create already read off the socket is never dropped on disconnect — admission closes and accepted work drains to settle, preserving the dequeued-create-must-spawn contract the pane-reconcile reconnect path relies on. Ordinary-create dedupe cleanup uses allocation-identity generations so a stale worker cannot erase a newer same-requestId retry. Also: keepalive deadlines start at the ping's actual flush receipt; a superseding attach discards only that terminal's unsent old output/gaps/ exit; gap coalescing respects terminal and attach-generation identity; two pre-existing terminal.rs dispatch test scaffolds now wire the real outbox+pump pair onto their loopback sockets. Verification: cargo fmt --check and workspace clippy -D warnings clean; 969 tests across freshell-ws + freshell-terminal pass locally (incl. the 30 authored scheduling tests and the pre-existing pane-reconcile, restore, dedupe, keepalive, replay and multi-client suites); the real WebSocket regression passes in the repo sandbox; coordinated npm run check passes.
… fairness, dead-code removal Address independent review findings on the writer/create-worker split: - Keepalive major: the flush-anchored deadline doubled dead-peer detection (a flush epsilon after its tick slipped timeout to the following tick and left a full silent interval). The deadline is now anchored at the tick that queued the ping — tick-aligned, so an unanswered ping is detected at exactly the next tick one interval later (legacy one-unanswered-cycle parity). At most one outstanding ping; a pong observed before its flush receipt still counts, and a lost receipt reports the writer as gone instead of etching a wrong SendFailed label. Four tests pin: next-tick detection with a just-after-tick flush, unflushed-for-a-cycle timeout, answered-ping retirement with the new deadline armed, and receipt-loss reporting. - A stop carrying a close code during an in-flight send now first lets the started frame finish (bounded 250ms flush — unambiguous continuation, never a retry), then attempts a whole Close (bounded): shutdown (4009) and failure closes keep their UX instead of an abrupt EOF when the socket was busy. - Controls keep their preemption but a CONTROL_STREAK_LIMIT now forces one output frame through after 8 consecutive controls, so a sustained control stream cannot starve output all the way to the catastrophic monitor. - Dropped the now-dead ConnectionOutputQueue adapter from backpressure.rs (zero callers after the writer took over routing); its exit/sync routing pins are covered by connection_writer tests. Refreshed stale module docs in backpressure (writer-based accounting, no more ticker/starve caveat) and output_queue, and the term09 integration header. - A closed create-worker queue no longer makes handle_client_text report a misleading 'send_error'; the supervision branch's 'create_worker_exited' stands. - New test: a full create queue yields a loud RATE_LIMITED reply, and a same-requestId retry is judged independently (dedupe guard cleared). - Documented the ordinary-create behavior changes at the worker spawn site (limiter stamps dequeue time, post-shutdown dequeue skip without reply, late-lineage reap). Full freshell-ws + freshell-terminal suites: 970 tests pass, all targets; sandbox wire regression green.
…ve, budget/doc/coverage fixes Review round 2 found my CONTROL_STREAK_LIMIT fairness rule violated the ready < modes.sync < replay contract: after 8 consecutive controls (e.g. idle-connection keepalive pings), an attach's replay could be leased ahead of its own prelude. Fairness is now stamp-gated: every control and output frame carries a per-connection admission stamp, and output may leapfrog the control lane only when strictly older than the oldest pending control — strictly-older output still interleaves under control floods, while a prelude can never be overtaken by its newer replay. Pinned by a new test using a saturated streak with a real TerminalAttachReady/TerminalModesSync/ replay sequence (which also restores a real-modes.sync-through-push_server routing pin lost with the deleted adapter). Keepalive now counts tick CYCLES identically to legacy's pong_since_last_ping instead of comparing wall-clock deadlines: a ping must be answered by the next tick, so detection lands at exactly one tick boundary and cannot slip a full interval behind a just-after-tick flush or timer jitter. A pong with no outstanding ping still counts as the next ping's answer (legacy boolean parity). The flush receipt survives only as liveness bookkeeping (pump-gone fast detection); its unused Instant payload is gone. DIAG-01 teardown also upgrades a generic 'send_error' to the writer's precise exit reason/close code when the writer had already finished. Control budget: an empty lane always admits ONE frame, even one larger than the configured budget (a legitimate screenshot-size control must not close an idle connection); continued flooding while it is in flight still overflows with 4008. The output+control worst-case sum is now documented at the budget site. Also: output serialization no longer happens under the queue lock (leased frames serialize after release); spawn_queue_cap documents its dual role (server-wide restore gate + per-connection ordinary-create queue); the 2026-08-17 replay-sync plan's retired-adapter pin points at push_server; discard_terminal restructured so rustfmt actually formats it. Full freshell-ws + freshell-terminal suites: 977 tests pass, all targets; sandbox wire regression green.
…cation nits - The DIAG-01 reason upgrade consumed a completed writer JoinHandle but only marked it finished on the Ok arm; a writer panic left the handle 'finished' and the following .await re-polled a consumed handle, panicking the connection task mid-teardown (skipping remove_connection, the create-worker join, and the lease sweep). Both arms now mark finished; the Err arm reports writer_task_failed. The upgrade additionally covers a stale 'writer_stopped' from a lost keepalive receipt. - Pin the shutdown-edge create worker: a create dequeued after the shutdown latch is skipped without a reply or a PTY, and its guard still releases the dedupe reservation (drives spawn()'s real closure; the WsState test fixture is now shared as test_ws_state()). - push/push_stamped in output_queue now delegate to one inner body. - Keepalive doc no longer contradicts its stray-pong test; the duplicate-supression sliver (reply pushed before the dedupe guard drops) is documented at the drop site with the client-visible reasoning. - The wire regression's header describes how it actually runs (plain cargo test; why its process-wide HOME isolation is safe), and push_server's measure-then-reserialize cost is now documented as deliberately deferred. Full freshell-ws + freshell-terminal suites green (all targets, host); this and prior commits' actual count is 973 (the previous message's ' 977' was a transcription error).
pull Bot
pushed a commit
to HinchK/freshell
that referenced
this pull request
Sep 5, 2026
…heduling Adds an additive terminalInterestV1 capability. After the server echoes it on ready, the browser sends transient full terminal.interest snapshots (focused + visible terminal IDs, socket-owned monotonic revision; invalid snapshots are rejected without replacing state; stale revisions are no-ops). The connection writer's output queue becomes a byte-fair per-terminal delivery queue with three service classes — focused/visible/background, ~8:3:1 bytes while all are continuously backlogged — with per-terminal FIFO, generation-scoped gap coalescing, and sequenced terminal.exit retained (a connection whose sequenced controls alone exceed the cap closes with 4008). Presentation interest only reorders not-yet-sent bytes: it never attaches, resizes, spawns, or kills, and one viewer never affects another. terminal.attach.priority remains the pre-snapshot fallback for un-negotiated clients; detach drops queued delivery and the fallback entry. Ported onto the reviewed danshapiro#705 writer rather than replacing it: keeps the join-error teardown reconciliation, tick-cycle keepalive, control-lane budget with admit-one-when-empty, and stamp-gated leapfrog fairness. The delivery queue additionally carries admission stamps (front_stamp) so the landed control-vs-output arbitration survives the scheduling change. Frames are serialized exactly once at admission (byte cost is an admission-time property of fair scheduling), superseding the typed-message lease-time serialization; the now-unused freshell-terminal OutputQueue machinery is removed, keeping only shared output-frame identity extraction and the default cap. Draft defect fixed during porting: update_priorities did not carry normalized service credit across a lane's class migration, so demoting a long-served lane made the destination class falsely underserved and the demoted lane would beat the freshly-focused lane — the migration now converts the origin class's weighted service into the destination's units (pinned by the reprioritization tests while the steady-state ratio tests still pin 8:3:1). Client: TerminalInterestReporter (mounted once in the workspace) publishes coalesced snapshots off selected tab/active pane/zoom/visibility; the revision lives on WsClient so remounts never rewind it; snapshots are never queued across disconnects. ws mocks in 22 suites gained the new no-op method, and the hello-capabilities shape test includes terminalInterestV1. The Node server accepts-and-ignores terminal.interest (never advertises : Rust-only), matching the auto-resume precedent. Protocol inventory frozen at 37 client/60 server. Rust: 1037 tests pass across freshell-ws/-terminal/-protocol (all targets, --no-fail-fast) including 17 delivery-queue units, interest units, four writer-level reprioritization regressions, and the terminal_interest_wire real-socket suite; term09 slow-reader wire suite unchanged and green. npm run check green on the coordinated gate. Note: pane_reconcile_freshagent's tracing assertion flaked once across three full-suite runs (passes isolated and on file rerun; same deferred class noted pre-existing in that suite).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
perf(ws): isolate terminal writes and ordinary CLI creates from connection dispatch
Problem
The
freshell-wsconnection loop awaited every socket send, so a large drained output backlog or a slow network flush blocked input, attach, and liveness dispatch for every pane on that connection. It also awaited ordinaryterminal.createcalls inline, letting one slow CLI launch stall unrelated panes on the same socket.Changes
WriterSender) instead of awaiting socket capacity; only the writer task owns the realSplitSink. Output and control preludes are admitted under one lock so anattach.readycan never be overtaken by its own replay.terminal.exitstays in the sequenced output lane — final output can never be overtaken by the exit frame.attach.readydiscards only that terminal's unsent old output, gaps, and exit — other terminals' queued data are untouched. A frame already leased to the socket keeps its order (it can't be unsent). Gap coalescing now respects terminal and attach-generation identity and uses saturating arithmetic.CreateProtectConfig.spawn_queue_capbounds the queue; full → loudRateLimitederror). Restore creates keep their existing gated path/permits; dedupe admission stays ahead of queueing.pane_reconcile's interrupted-create regression). The one started create is never aborted (a runningspawn_blockingcannot be safely cancelled). A late server-shutdown reaps only the worker's own late lineage.InFlight.startedis now anArc<Instant>; worker drop guards clear only their own generation, so a stale worker cannot erase a newer same-requestId retry (e.g. with a changed restore flag). Legacyclear_if_in_flightcallers are unchanged. Cleanup panics are contained by an unwind boundary.FrameSinkcallbacks can't fill an orphaned outbox (the pump'sDropalso closes admission).No wire-schema, browser, dependency, provider, replay-retention, compression, or renderer changes. True foreground/background bandwidth scheduling, compression, and viewport snapshots are deliberately out of scope (later steps).
WsSinkis now the outbox: a "flush" means admission, not wire delivery.Tests
31 new native tests + fixtures: 12 writer/keepalive cases, 9 serial-worker/cancellation/cleanup cases, 3 dedupe-generation cases, 5 output-queue cases, 1 real-WebSocket regression (
cli_connection_scheduling, run via the repo sandbox), plus re-wire of two pre-existingterminal.rsdispatch test scaffolds onto the real outbox+pump pair.The wire regression proves the headline contract with the real router: with an ordinary create deliberately parked mid-launch, existing claude/opencode/codex headless terminals on the same connection still accept input, new attaches still complete, and app-level ping still answers — without executing any provider binary. RED control: with ordinary-create dispatch restored inline (scratch worktree, discarded), the same test times out in 30s, as designed.
Verification completed (this environment, this base: 5b38513)
cargo fmt --all --checkclean;cargo clippy --workspace --all-targets -- -D warningsclean (toolchain 1.96.0, matching CI).freshell-ws+freshell-terminal(all targets,--no-fail-fast), including the pre-existing pane-reconcile/restore/dedupe/keepalive/exited-terminal replay/multi-client suites.npm run check(typecheck + full vitest + electron) passes on the branch.Fresh-eyes review history
Four rounds of independent review (Claude; GPT was quota-unavailable that day): rounds 1-3 each surfaced real defects that were fixed in follow-up commits (keepalive off-by-epsilon doubling dead-peer detection; fairness fix that broke ready < modes.sync < replay ordering; a JoinHandle double-poll panic on the writer-panic teardown path). Round 4 review PASSED with only minors/nits (noted inline in code comments). Note: commit
b891c0672's message says "977 tests"; the actual count at that commit was 972 — the correct current count is 973.Not done here (per draft scope): constrained-link browser/provider A/B measurements, foreground/background fairness. During this branch's full-suite runs two distinct single-test ambient flakes appeared (
test-coordinatorholder timing; sidebar DOM-identity) — each passed in immediate isolated re-run and both are unrelated to this Rust-only change; one is already being hunted by the test-flake-hardening track. The repo sandbox's fullfreshell-wslib binary additionally SIGABRTs in anotifyinotify-destructor on both base and this branch (pre-existing container environment issue; the identical suite passes on the host, and all other sandbox targets pass).Follow-up boundary / remaining risks
Controls still outrank output — this is not per-pane bandwidth fairness. Other long-running non-create handlers still run inline. A started non-cancellable spawn can hold its worker (not the socket or queue) until it settles. Constrained-link UX acceptance and real-provider checks are the next gate before any ship claim.