perf(ws): foreground terminal delivery — connection-local interest scheduling - #706
Merged
Conversation
…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 #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).
…tests - update_priorities: drop the weight-SCALED service carry. class_served is one normalized virtual clock across classes, so the direct watermark lift is the correct carry; the scaled version starved a demoted lane for a period proportional to the connection's entire prior service. New regression test: demotion_after_history_neither_steals_nor_starves pins both the promotion window (demoted lane arrives within one 8:1 window) and the steady ratio (exactly 3 of 27 saturated pops) with 1000 frames of real history. - InterestState.attach: stop storing fallback entries once a snapshot is authoritative (the map is never consulted then), prune on detach and on terminal.exit admission, and never kill the connection at the cap: skip the entry (Visible default = pre-feature behavior) with a ws.interest.fallback_cap_reached warn instead of a 4008 close. Removes the NX-attempt reconnect-storm path reviewers found. - Sequenced terminal.exit pushes are zero-weight again (legacy parity): an exit can no longer force eviction or trip OutputCapacityExceeded; they stay count-bounded by the independent metadata limit. The in-flight reservation (cap minus leased frame) is deliberate admission-accounting, now documented. - run_with_interest folds into run() (the legacy entry point had no callers); terminal_interest_v1 flows through build_handshake_with_capabilities as a parameter like the sibling capabilities instead of a get_or_insert patch. - terminal-interest: a refused selector read is now debug-logged instead of silently parked (publisher keeps the last accepted snapshot); pinned by a new publisher test. - inventory drift-guard test renamed to combined_surface_is_97; the unreachable post-eviction MetadataLimit check is documented as an invariant. - Client tests for the previously uncovered behavior: 4 WsClient sendTerminalInterest cases (refusal before ready, refusal without the negotiated capability, monotonic socket-owned revisions, no queueing across disconnects + revision reset on the new socket) and 5 mounted TerminalInterestReporter cases (mount publish, active-pane/zoom republish, eager reflush on ready reconnect, immediate hidden report on visibilitychange, workspace-visibility gating). Note: this suite has no global RTL cleanup — the reporter tests unmount explicitly to avoid leaking visibilitychange listeners across cases. Note: pane_reconcile_freshagent's dead_session WARN-count assertion flakes on pristine main too (1/5 in a control worktree at cd6a15e; unrelated to this branch). Everything else passes: freshell-ws/-terminal/-protocol full suites, fmt, clippy -D warnings, typecheck.
- backpressure.rs: TERM-07 attach priority IS now carried (attach.priority as the pre-snapshot fallback, terminalInterestV1 snapshots after); the old 'not implemented' paragraph was stale. - output_queue.rs doc: exits are zero-weight + count-bounded since the preceding commit; the 4008 path requires exhausting evictable output while over cap. - connection_writer arbitration comment: sequenced exits keep their admission stamp and may leapfrog a control streak (admission order preserved). - term09_output_queue.rs doc: references the delivered delivery queue, not the removed OutputQueue name.
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): foreground terminal delivery — connection-local interest scheduling
Problem
On top of #705's per-connection writer, ALL queued terminal output competed FIFO-equal for a connection's byte budget. On a constrained link, background CLIs (CI output, test runners, long builds) keep the focused pane's bytes behind milliseconds-to-seconds of unrelated backlog. Nothing knew which terminal the user is actually looking at.
Changes
crates/freshell-ws/src/terminal_delivery_queue.rs): three service classes (focused / visible / background) receive approximately 8:3:1 BYTES while all are continuously backlogged; empty classes accrue no credit; lanes within a class share bytes equally; per-terminal FIFO and generation-scoped gap coalescing are preserved;terminal.exitstays sequenced behind final output and is never evicted (a connection whose sequenced controls alone exceed the byte+metadata budget is closed with 4008 rather than growing without bound).terminal_interest.rs): additiveterminalInterestV1capability, negotiated onhello/ready; clients send transient fullterminal.interestsnapshots (monotonic revision, never persisted, never queued across disconnects). Malformed or cross-field-invalid snapshots are rejected withINVALID_MESSAGEand never replace the last accepted state; stale/equal revisions are no-ops. Interest changes scheduling of not-yet-sent bytes only — it never attaches, detaches, resizes, spawns, or kills; per-connection, so one viewer never affects another.terminal.attach.priorityfield seeds a terminal's class until the first accepted snapshot (attach_priority_works_for_clients_without_interest_capabilitypins it); detach drops both queued delivery and the fallback entry (discard_terminal_delivery), attach supersession still discards an old generation's queued frames.Porting notes (draft handoff integration)
The handoff's
connection_writer.rsreference was authored against the ORIGINAL Part-1 candidate. This PR instead ports the scheduling/interest design onto the writer as reviewed and landed in #705, retaining all post-review fixes: join-error reconciliation in teardown (both arms), tick-cycle keepalive (no wall-clock jitter), stray-pong consumption, control-lane byte budget with admit-one-when-empty, stamp-gated leapfrog fairness (CONTROL_STREAK_LIMITkeeps an always-saturated control lane from starving strictly-older output while never reordering a prelude behind its replay), and the drained (never dropped) interactive-create worker semantics.output_frame_meta/DEFAULT_TERMINAL_CLIENT_QUEUE_MAX_BYTESstay shared infreshell-terminal; the now-supersededOutputQueuemachinery (and its superseded scheduling test) is removed.Draft defect found and fixed during porting: the authored
update_prioritiesdid not carry normalized service credit across a lane's class migration, so demoting a long-served lane made its new class falsely "underserved" and the demoted lane would beat the freshly-focused lane on the next lease — the opposite of the feature's purpose.terminal_delivery_queuenow converts the origin class's weighted service into the destination class's units on migration; the authored testfocus_does_not_cancel_the_inflight_frame_or_lose_following_bytes(and reprioritization tests) prove promotion, while the steady-state ratio tests prove the 8:3:1 weighting is unchanged. Admission-time serialization (payloads stored wire-ready) replaced the lease-time re-serialization — required since byte cost and class fairness are admission-time properties — so each frame is now serialized exactly once.Node server:
terminal.interestis accept-and-ignore there (Rust-only feature; it never advertises the capability), mirroring the existing auto-resume precedent.Fresh-eyes review history
Two rounds of independent review (Claude). Round 1 found three majors — a weight-scaled service-carry starved demoted lanes (replaced by the correct direct watermark lift + history regression test), the attach-priority fallback map could close a long-lived socket at the 1024-entry cap (now snapshot-skip + exit/detach prune + warn-and-skip instead of 4008), and missing client tests (now 4 ws-client + 5 mounted-reporter + 1 publisher cases). Round 1 minors also fixed: zero-weight sequenced exits restored (legacy parity), dead
run_with_interestwrapper folded, handshake capability flows through the builder as a parameter, selector refusal is debug-logged, inventory test renamed, stale scheduling docs corrected. Round 2 PASSED with minors/nits only; residual disclosed items are noted here:TERMINAL_CLIENT_QUEUE_MAX_BYTESis raised past roughly 42 MiB (default 32 MiB is unreachable); no deployment sets the override today.Verification
cargo fmt --checkclean; clippy onfreshell-ws/freshell-terminal/freshell-protocolwith-D warningsclean.--no-fail-fast, all targets, host), including the new delivery-queue unit tests (18), interest unit tests, four writer-level interest/reprioritization regressions, and the new real-WebSocketterminal_interest_wire.rs(capability negotiated only on opt-in, accepted snapshot never spawns/attaches, unnegotiated rejection, malformed/stale rejection without disconnect). The pre-existingterm09_output_queuewire suite (real sockets, real PTYs, slow reader) passes unchanged.npm run contract:generaterefreshed (37 client→server types; the protocol crate's inventory drift guard was updated to 37/97).npm run typecheckclean; focusedterminal-interestVitest suite green; coordinatednpm run checkgreen.pane_reconcile_freshagent's dead_session WARN-count assertion is a pre-existing ambient flake: reproduced on a pristine control worktree at the current merge base (cd6a15e), unrelated to this branch.Explicitly out of scope
Constrained-link browser benchmarks (the architectural expectation is byte-fair scheduling; measured A/B wins are a follow-up), foreground/background interplay with future bandwidth fractions, and the self-hosted deployment (no server restart performed).