fix(terminal): stop stray newline injection on session switch (portable-pty EOF-on-drop) + connect at measured size#28
Merged
Conversation
… on reconnect Kill the stray-newline-on-switch/reconnect artifact (claude reflowing and stacking blank lines when a CLI pane attaches at xterm's 80x24 default and is then refit to the real size). - XTermInstance: never open the WS at an unmeasured size. Resolve the endpoint from FitAddon.proposeDimensions() after fit(); if the container is hidden/ 0-height, defer the initial connect until the first non-zero ResizeObserver tick instead of baking cols=80&rows=24 into the URL. Never tears down a live connection (hidden mobile tabs keep theirs by design). - TerminalProvider: store a getEndpoint() callback instead of a frozen endpoint string; connectWebSocket rebuilds it fresh on every (re)connect so reconnects attach at the pane's CURRENT grid, and skips+reschedules when unmeasured. - Observability: log cols/rows on the tmux attach (pty.rs) and warn on a mode=cli attach at the default 80x24 (terminal.rs) as a permanent regression tripwire. - Extract resolveTerminalEndpoint() as a pure, node-testable helper; add vitest covering unmeasured->null, measured->real grid, and reconnect re-fit.
…njection; add attach-window input tripwire
miguelrisero
changed the title
… logs Control bytes (the \n+EOT injection / escape-sequence signature) stay verbatim; printable keystrokes are masked so passwords or pasted secrets never reach server logs.
…backs Council review round 1: - pty.rs: a failure between take_writer() and PtySession construction (e.g. try_clone_reader under fd pressure) dropped the writer with VEOF still armed, injecting \n+EOT into the just-spawned child — cover the window with a VeofDisarmOnDrop guard (defused once PtySession owns the writer) + unit test; log tcsetattr failure in disarm_master_eof; state the real Drop guarantee (device-scoped termios persistence) in the comments instead of overselling field order. - XTermInstance: the stored connection callbacks captured this mount's refs, which cleanup nulls — after a gate-flip remount (executorRunning toggle, hidden side tabs) a surviving connection silently dropped all output; resolve terminal/fitAddon through the provider registry (same lifetime as the connection) and read sessionId via a latest-value ref so later reconnects join the session the UI currently shows. - TerminalProvider: expose hasTerminalConnection (established OR live in-flight generation) and gate ensureConnection on it so two quick calls can't stack a duplicate backend PTY/tmux attach inside the async open window (real on slow relay transports). - terminal.rs: lock the tripwire redaction invariant with an exhaustive 0x00-0xff mask test.
…hidden-pane retry budget /simplify review (reuse / simplification / efficiency / altitude): - pty.rs: VeofDisarmOnDrop now owns a dup(2) of the master fd and rides the spawn_blocking return tuple (declared first — tuple fields drop in order), extending creation-window coverage to the request future being cancelled at the .await, where the detached task's tuple previously dropped the writer VEOF-armed with no guard alive; shared disarm_eof_on_fd helper; writer-field doc deduped; open_test_pty test helper; guard test updated for the owned-fd form. - terminal.rs: attach-window tripwire state/bounds co-located in an AttachInputTripwire struct — identical 2s/128-byte semantics; expiry now latches the spent budget so post-window frames cost one integer compare. - TerminalProvider: an unmeasurable pane is a wait state, not a connect failure — poll at 1s without spending the retry budget, so a pane hidden longer than the backoff ladder doesn't burn its 6 retries and give up before the user ever shows it; getEndpoint contract comments deduped to point at resolveTerminalEndpoint. - XTermInstance: shared fitInstance() helper for the registry-resolved callbacks; onClose read via latest-ref so its unstable prop identity stops churning ensureConnection/fitTerminal and the ResizeObserver effect on every parent render.
…ting and healing Phase-3 review battery (codex xhigh + 10-angle finder pass), all fixes behavior-verified: - pty.rs: construct the PtySession INSIDE create_session's blocking task and move the child kill into Drop for PtySession. Every teardown — close_session, service shutdown, creation failure after spawn, and the caller's future being cancelled at the .await (which previously orphaned the tmux client: the VEOF guard stopped the injection but nothing killed the child or unblocked the reader thread) — now funnels through the one Drop. The VeofDisarmOnDrop guard, its dup(2), and the tuple-order/defuse-placement conventions are deleted outright; the disarm stays teardown-only. New test drives a real pty child through session drop (VEOF disarmed on the device, child killed and reaped). - terminal.rs: cols/rows become Option<u16> so the stray-newline tripwire warns on true ABSENCE instead of false-positiving on panes that genuinely measure 80x24 (a literal-default regression stays visible in pty.rs's per-attach size log); unit tests for the attach-window tripwire's byte budget and expiry latch. - terminalWsUrl: reject non-finite dims — FitAddon.proposeDimensions() can yield NaN for detached/unstyled containers, and cols=NaN in the URL would 400 and burn the reconnect ladder; vitest added. - XTermInstance: gate measurability on the terminal element's real DOM box (display:none containers pass proposeDimensions' >=1-cell clamp with a garbage 2x1 grid — the tiny-then-resize bounce in new clothes); heal-on-session-switch effect; fitTerminal reuses fitInstance. - TerminalProvider: createTerminalConnection is now ensure-style idempotent — while a live/in-flight connection owns the tab it only refreshes the stored callbacks + endpoint source, fixing the remount staleness the ref indirection alone missed (a remount creates NEW ref cells; the stored closures kept reading the old mount's frozen ones) — and the public hasTerminalConnection gate is deleted in favor of enforcing the invariant where the state lives. Backend-error and clean-close paths now release the tab's entries (if still theirs) so a later mount/session switch can heal instead of finding the slot occupied by a dead socket forever. Already-open sockets from pluggable transports get the size sync onopen would have sent. UNMEASURED_POLL_MS named to keep the wait-state cadence out of the retry ladder.
…on reader-clone failure Council round 2 (mixed engine, 4x codex + claude cross-check): - TerminalProvider: an open started before a remount/session switch could resolve AFTER the endpoint source was refreshed and still register — binding the visible pane to the PREVIOUS session's tmux (all four codex seats, P1). Post-open the attempt is now validated against the current getEndpoint(): size-only drift passes (the post-open resize corrects it), a material change (session/mode/workspace) closes the socket and re-kicks through the single-timer path without spending retry budget. terminalEndpointsEquivalent lives in terminalWsUrl.ts with vitest. - TerminalProvider/XTermInstance: the measurability gate moved from the component into createTerminalConnection, so an occupied-but-hidden tab still refreshes its stored callbacks on remount (the component-side gate skipped the refresh exactly when the predecessor closures were already dead). - XTermInstance: the DOM gate also checks the PARENT box — the one FitAddon actually measures — so a splitter collapsed to 0-height can't slip a ~Nx1 clamped grid past the element-box check. - pty.rs: try_clone_reader failure now kills AND reaps the spawned child inline (was: kill only, one zombie per failed create); the dead PtySession.closed field is gone; close_session drops the removed session outside the registry lock; the Drop comment states the real (accepted, pre-existing, few-instruction) load-then-kill window instead of overclaiming.
…t epoch Final backstop sweep: the stale-open guard skipped validation when the pane was unmeasurable at resolve time (getEndpoint() null), so a session switch racing a slow open could still register the previous session's socket if the pane hid mid-open. Every getEndpoint refresh now bumps an epoch the attempt snapshots before awaiting: unchanged epoch = untouched source, safe to register even hidden; changed epoch = revalidate (size drift passes, material change discards, unmeasurable discards conservatively and re-polls). Discards never spend the retry budget.
…after paint Backstop round 2: a websocket open resolving in the commit-to-passive- effect gap could still read the previous session's ref value and an unbumped endpointEpoch, registering the old session's socket. The ref sync and the heal/refresh effect are now useLayoutEffect — they run synchronously inside the commit, before any promise continuation can interleave, so an in-flight open can never observe a half-switched session.
…e CONNECTING attach The endpoint-epoch check ran only right after openLocalApiWebSocket() resolved, but the default transport resolves with a still-CONNECTING socket. A session switch landing after that socket was registered but before it opened only bumped the epoch (occupied path) without closing or revalidating the connecting socket, so it could become the tab's live connection and bind the pane to the previous session's conversation on a first-attach / post-reap create. Re-validate at onopen (and make the socket's own message/close handlers inert once superseded) via a shared, unit-tested terminalAttemptIsStale() helper that also DRYs the existing post-await check; discard + re-kick through the single timer without spending the retry budget. Never tears down a live/hidden connection — it rejects only a not-yet-live stale handshake.
…ed-child stale bind The onopen re-validation gated on the epoch delta, but the epoch is bumped only by a MOUNTED terminal child. When the host stops rendering the terminal (executorRunning / !sessionsReady) during the CONNECTING window and the session changes while gated, no mounted effect bumps the epoch, so the epoch delta stays false and the stale socket (carrying the previous session_id) was accepted at onopen — binding the pane to the previous conversation on a first-attach / post-reap create. Pass epochChanged=true at the onopen site (the point of no return) so the endpoint is ALWAYS re-inspected; a gated pane's endpoint resolves to null, so the stale socket is discarded and the poll reconnects with the current session once the pane is shown again. The post-await site keeps the epoch-delta gate to avoid churn when a pane is briefly unmeasurable without a switch. Never tears down a live/hidden connection.
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.
Problem
Switching CLI sessions (or otherwise tearing down a terminal WebSocket) injected a stray newline into the newly-attached pane. Live byte-level instrumentation found this has two independent root causes, and the resize fix alone did not stop it.
Primary:
portable-ptywrites\n+ EOF into the tty when the writer dropsportable-pty0.8.1'sUnixMasterWriter::drop(registry sourceunix.rs:351-363) reads the master's termios and, ifc_cc[VEOF]is non-zero, writes[b'\n', VEOF]— LF + Ctrl-D — into the pty. This is the crate's documented "dropping the writer sends EOF to the slave" behavior.In
local-deployment/src/pty.rs,close_session()SIGHUPs the tmux client and drops thePtySession; the writer's Drop then fires that 2-byte write into the tmux client's tty. The tmux server reads it as client keyboard input and forwards it as keystrokes to the active pane (confirmed intmux -vvserver logs:keys are 2 (\n\004) … writing key 0xa (C-j) … 0x4 (C-d)):0x0ainserts a stray newline (never submits);0x04is a no-op → the visible bug.vk_session — the more severe consequence.Reproduced on 4-6 of 6 teardowns with correctly-sized attaches. SSH sessions are immune (no
portable-ptyin that path).Secondary: wrong-size attach reflow
A CLI attach at an unmeasured/default 80×24 grid makes claude reflow and stack blank lines on the follow-up resize (the original PR content).
What changed
Primary fix — disarm VEOF at teardown (
pty.rs):disarm_master_eof(&dyn MasterPty)clearsc_cc[VEOF]on the master fd viatcsetattr. The writer holds adup(2)of the master fd and termios is shared across dup'd fds, soportable-pty'sif eot != 0guard then sees0and skips the injection.impl Drop for PtySession, which runs before thewriterfield drops. This is the single point every teardown path funnels through —close_session(mapremove+ drop), process/service shutdown (sessionsHashMapdropping), and any future cleanup path — so the disarm can never be forgotten.Dropnever fires at creation, so a live shell-mode terminal keeps working Ctrl-D/VEOF semantics; VEOF is only cleared as the session is being torn down.libc = "0.2"as a direct dependency oflocal-deployment(already in the lock tree).Secondary fix — connect at measured size + rebuild WS endpoint on reconnect (prior commit): never connect xterm at an unmeasured size; rebuild the WS endpoint on reconnect. A
tracing::warn!tripwire fires if a CLI attach ever arrives at default 80×24.FIX 4 — attach-window input tripwire (
terminal.rs): for the first 2 seconds after attach OR first 128 input bytes (whichever first),tracing::info!a hex dump of client→pty bytes tagged with the pty session name (vk_<uuid>in CLI mode), per-attach id, and ms-since-attach. Strictly bounded (no allocation growth after the window), permanently safe to leave on. Purpose: production canary that confirms the kill is gone and catches any future client-side injector by distinguishing client-origin bytes from server-origin ones.How verified (honest)
disarm_master_eof_zeroes_veof(opens a pty via the sameopenptypath, asserts a fresh master hasVEOF != 0— guarding against a silent no-op — then assertsVEOF == 0after the helper);cargo test -p local-deployment57 passed,cargo test -p server36 passed;cargo clippy -p local-deployment -p serverclean;cargo fmtclean.Risks
disarm_master_eofis Unix-only (#[cfg(unix)], no-op elsewhere); the injection path only exists on Unix ptys.infofor a bounded post-attach window; bytes are hex, bounded to 128, and the window closes after 2s, so there is no unbounded logging or allocation.ship-it battery
Final ship-it review battery (round 2) completed on head
783a2543. Phases before this checkpoint (council R1, /simplify, codex+xhigh, round-2 FIX BATCH) landed commits through8845db3c.Council round 2 — mixed engine (Codex panel + 1 Claude cross-check), on
8845db3cPanel: security-analyst, performance-engineer, dx-advocate, maintainability-architect.
Backend concerns all cleared by the panel:
Drop-ordering guarantee (VEOF disarm runs before field drops; outlivingArc<writer>clones covered via shared device termios), unsafe termios correctness (POD init, checked syscalls, non-blockingTCSANOW, no panic inDrop), teardown funneling (no fallible step strands a VEOF-armed writer;close_sessiondrops outside the lock), and tripwire boundedness + redaction (2s/128-byte latch, all 256 byte values masked except C0+DEL).Backstop — codex xhigh (CodeRabbit substitute)
CodeRabbit CLI cannot authenticate non-interactively in this environment (deterministic
environment_unsupported), so codex xhigh is the substitute for every sweep below.8845db3c): DO-NOT-SHIP — one P1, independently matching the dx-advocate finding (cross-engine convergence).e8223aa1, after fix P1-a): DO-NOT-SHIP — confirmed P1-a closed, surfaced the sibling P1-b.783a2543, after fix P1-b): SHIP — "Findings: none. No P0/P1 defects found." Both races closed; no new blocker (false visible-attach discard, double reconnect, hidden-live teardown, VEOF disarm, tripwire, tmux attach all checked).Findings fixed (2 × P1, same wrong-session-bind class)
P1-a — stale CONNECTING socket could bind the pane to the previous session (
e8223aa1). The endpoint-epoch validation ran only right afterawait openLocalApiWebSocket()resolved, but the default transport resolves with a still-CONNECTINGsocket. A session switch landing after that socket was registered but beforeonopenonly bumped the epoch (thecreateTerminalConnectionoccupied path) without closing/revalidating the connecting socket, so it could become live bound to the previous session on a first-attach / post-reap create. Fix: re-validate atonopen(not just after the open resolves) via a new pure, unit-testedterminalAttemptIsStale()helper that also DRYs the existing post-await check; on stale, latch asupersededflag, drop the tab's connection entry iff it is this socket, close it, and re-kick through the single timer without spending the retry budget. The socket's ownonmessage/onclosego inert once superseded.P1-b — gated-child variant (
783a2543). Theonopenre-validation gated on the epoch delta, but the epoch is bumped only by a MOUNTED terminal child; whenCliMainPanestops renderingXTermInstance(executorRunning / !sessionsReady) during the CONNECTING window and the session changes while gated, no effect bumps the epoch, so the stale socket was accepted. Fix: at theonopensite passepochChanged = trueunconditionally so the endpoint is ALWAYS re-inspected; a gated pane'sgetEndpoint()resolves tonull(element detached), so the stale socket is discarded and the poll reconnects with the current session once the pane is shown. The post-await site keeps the epoch-delta gate to avoid churn when a pane is briefly unmeasurable without a switch.Both fixes are frontend-only and never tear down a live/hidden connection (locked decision preserved) — they reject only a not-yet-live stale handshake.
Findings rejected
None. No finding was dismissed; the two P1s were fixed and every other seat/sweep found no P0/P1. The maintainability nits raised by the Claude seat (portable-pty 0.8.x
Dropversion coupling; the effect-ordering invariant being correct-but-refactor-fragile; the Drop test hand-building the struct) are P2/P3 by their own author and out of scope for this correctness fix.Verification on
783a2543cargo test -p local-deployment -p server— pass (incl. the 2 new PTY Drop/VEOF tests + 4 tripwire tests);cargo clippy -p local-deployment -p serverclean;cargo fmt --checkclean (backend unchanged by the round-2 fixes).npx vitest run terminalWsUrl.test.ts— 15/15 (addedterminalAttemptIsStalecoverage incl. the onopen force-inspect contract); frontendtsc(local-web, web-core, remote-web, ui) clean; prettier clean.783a2543: frontend-checks, backend-test, backend-clippy, backend-schema-checks, tauri-checks all success; backend-remote-checks skipped (nocrates/remotechanges). (pnpm run checkcannot complete in the review sandbox only because the GTK/glib system libs thetauri-appcrate needs are absent there; CI, which has them, is green.)