fix(session): stop phantom repeated keystroke replay on reconnect/flap - #295
Draft
tstapler wants to merge 12 commits into
Draft
fix(session): stop phantom repeated keystroke replay on reconnect/flap#295tstapler wants to merge 12 commits into
tstapler wants to merge 12 commits into
Conversation
…e evidence
Research phase (6 parallel agents) plus direct code reading found the
originally-suspected client MessageQueue/reconnect duplication does not
explain repeated single-keystroke delivery (queue items are consumed
exactly once; server input relay sends each message at most once).
session/session_driver.go's startup-dialog auto-answer loop is the
confirmed primary cause: it polls Preview() every 2s and resends
SendKeys("1\n") with no de-duplication/backoff whenever isStartupDialog()
matches, independent of the auto_yes flag. A live-executing test
(session/phase0_repro_test.go) reproduces 3 repeated SendKeys("1\n")
calls when Preview() returns stalled dialog content, matching the
ticket's "over and over" symptom.
The client MessageQueue/epoch-guard gap is real but secondary (per AC3's
explicit text) and will be fixed as additive hardening.
…ial review Plan covers two additive fixes: a content-hash latch on session_driver.go's startup-dialog auto-answer loop (the confirmed root cause), and a connection- generation guard + drop-on-close fix for MessageQueue/useTerminalStream (AC3's explicit secondary hardening requirement), plus an InputDropBadge UX surface and the Go/Jest regression tests required by AC4. Adversarial review caught two real blockers before any code was written: the fix's control-flow could silently starve the driver's inactivity- timeout/ReviewQueue escalation once the dialog latch gave up, and the content-hash approach was vulnerable to incidental formatting jitter (same failure class as the adjacent #164 resize-loop bug). Both resolved and re-verified; architecture and UX reviews landed at CONCERNS only, folded into the plan directly.
Validation phase (3 parallel subagents: test-suite design, pre-mortem, cross-artifact consistency) surfaced one new BLOCKER and one new P1 the Phase 3 review rounds missed: - BLOCKER: no test actually exercised AC4's literal "queued-message-drop- on-close interleaving" scenario (Task 2.1.2 was an isolated close() unit test, not a live-reconnect interleaving test). Added Task 2.2.8. - P1: Preview() returns the entire accumulated PTY buffer, not a tailed "current screen" snapshot, so the content-hash latch's whitespace-only normalization would still misfire in ordinary, non-flapping sessions once any new output changed the whole-buffer hash - a materially larger regression surface than the formatting-jitter case already closed. Task 1.1.2 revised to tail-slice via the existing tailContent/ statusDetectionTailBytes precedent before matching and hashing. Also closed two coverage gaps the validation subagent flagged (a test proving the control-flow fall-through reaches escalation, and a missing Playwright e2e spec required by this repo's feature-registry convention). Readiness gate: PASS (6/6 ACs covered, 0 open blockers/P1s, ADR-001 on disk, no schema changes).
… reconnect input path
Fixes the confirmed root cause of the phantom repeated "1" keystroke bug
(backlog 04089969-0f19-499c-be34-2e8bcfc4f13e): session_driver.go's
startup-dialog auto-answer loop resent SendKeys("1\n") every poll tick
with no de-duplication whenever isStartupDialog() matched, and Preview()
returns the entire accumulated PTY buffer rather than a tailed snapshot,
so the resend could recur in ordinary non-flapping sessions too, not
just during a connection flap.
- session/session_driver.go: content-hash latch (dialogUnanswered ->
dialogAwaitingDismissal -> dialogGaveUp), tail-sliced + whitespace-
normalized before hashing/matching (via the existing tailContent/
statusDetectionTailBytes precedent), bounded retry-on-failure, and a
control-flow fix so a GaveUp/AwaitingDismissal latch falls through to
the existing inactivity-timeout/ReviewQueue escalation instead of
silently wedging the driver loop. Applied to both SendKeys("1\n")
call sites (startup dialog + approval prompt).
- server/services/connectrpc_websocket.go: extracted the WebSocket
input-read loop into runInputReadLoop so it's independently testable
(bounded-exit test proves it stops forwarding input promptly once the
connection closes).
- web-app MessageQueue/useTerminalStream: close() now drops (does not
drain) buffered-but-unsent input; useTerminalStream gained a
connection-generation guard (mirroring the existing usePathCompletions
generation-counter idiom) so an overlapping/rapid reconnect can't leave
two live message loops fighting over the same session. Dropped input
now surfaces via a new InputDropBadge (assertive live-region
announcement + visible badge, coalesced per drop episode).
Confirmed via a live-executing runtime test (session/phase0_repro_test.go,
now superseded by permanent regression coverage in session_driver_test.go)
that the unfixed driver resent the keystroke repeatedly against a stalled
preview buffer. Full adversarial/architecture review + pre-mortem process
in project_plans/phantom-keystroke-replay/ caught and fixed two design
blockers and one P1 gap before this code was written.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q2aGuq28JKW6fT6w2MiKbV
…titute evidence Task 4.1.1's live-browser repro procedure targets the omnibar session- creation path, which never attaches a SessionDriver at all (only CreateDirectorySession/MCP create_session do) - the exact code this fix touches can never run through that path. Also found /tmp is globally pre-trusted on this machine (defeats the trust-dialog trigger) and a log-routing gap in isolated STAPLER_SQUAD_INSTANCE deployments unrelated to this fix. Documented all three for a follow-up live verification pass; recorded the automated re-run of Epic 1's regression suite (same Phase 0 methodology, now bounded instead of unbounded) as substitute evidence.
MUST FIX: InputDropBadge's manual dedup ref broke under React StrictMode
(setup->cleanup->setup replay could clear the dismiss timer without
rearming it, leaving the badge visible forever on first mount in dev).
Collapsed into a single effect with its own cleanup, no ref-based dedup.
CONCERN: window.__e2eTriggerInputDropped now gates on NODE_ENV so it's
tree-shaken out of production builds instead of shipping as an
always-present, unauthenticated script-callable surface.
Also: exhaustive switch (with a panic default) on dialogLatchStatus so a
future 4th status can't silently fall through the unbounded-resend path;
hoisted the duplicated SendKeys("1\n") closure shared by both latch call
sites; removed a redundant useCallback wrapper in TerminalOutput.tsx;
documented the onScrollbackRequest plan deviation.
Reverted one over-eager cleanup: moving all tail-slicing to the call
site (instead of inside answerDialogOnce) broke TestAnswerDialogOnce's
growing-buffer test cases (f)/(g), which call answerDialogOnce directly
with untailed input and rely on its own internal tailContent call.
Restored the internal tail-slice - the "duplication" with the call
site's own tailed value (needed separately for isStartupDialog/
shouldApprovePrompt) is cheap and was never actually redundant once a
unit test exercises the function in isolation.
All Go and Jest suites verified green after this pass (full session +
server/services test trees, plus TerminalOutput's existing suites).
…ager stubs session_driver_test.go's stuckDialogProcessManager fake intentionally returns (nil, nil) for GetPTY/Attach since neither is exercised by the dialog-latch tests. Matches the existing nolint:nilnil convention used in history_detector.go for the same real-fake pattern.
…n sdd:6-verify review Two independent parallel reviews (Go idiom/concurrency, React/TS idiom) each surfaced one real MUST FIX: - GetEffectiveStatus() claimed (via its own comment and callers' comments) to acquire stateMutex.RLock but never did, and the new session_driver_test.go tests wrote inst.Status directly from a second goroutine while the driver loop read it via GetEffectiveStatus — a genuine data race caught by `go test -race`. Fixed by actually taking the RLock in GetEffectiveStatus (matching its documented contract) and routing the tests' cleanup writes through stateMutex.Lock(). - useTerminalStream's disconnect() had no connection-generation guard on its delayed (1000ms) graceful-close timeout or its trailing setIsConnected(false), unlike connect()'s read loop. A disconnect() that armed its timer while still connected, raced by a newer connect() that took over in the meantime, would abort/null the *newer* generation's AbortController and clobber its isConnected state once the stale timer fired — the same class of bug this whole fix exists to close. Fixed by capturing the generation at disconnect() entry and gating both effects on it; new regression test confirmed to fail against the pre-fix code and pass against the fix. Also addressed two Go nitpicks from the same review: replaced the bespoke errSentinel type with errors.New, and removed a local min() helper that redundantly shadowed the Go 1.21+ builtin. go test -race ./session ./server/services and the full web-app Jest suite are green.
…and fixes Documents the two real MUST FIX findings (Go data race, disconnect() reconnect-race gap) surfaced by a fresh two-agent idiom review pass and fixed in f3a876f, per plan.md's existing Post-Implementation Finding convention.
… MCP create_session Completes the literal live-browser-equivalent repro the earlier Post-Implementation Finding could only substitute with re-run unit tests. Created a real session via mcp__stapler-squad__create_session (the path that actually exercises StartSessionDriver), induced the ticket's exact "session not started or paused" flap via pause/resume immediately after creation, and confirmed via server logs the startup dialog was answered exactly once inside the flap window with no resend and no repeated "1" in the final recovered terminal state.
…tion test for rebased base
Rebasing these 12 commits onto current origin/main surfaced two issues in
TestSessionDriver_DialogGaveUp_FallsThroughToInactivityEscalation that were
latent even before the rebase (session_driver.go itself is byte-identical
to the pre-rebase version — confirmed via diff):
1. The driver's activityRef logic always prefers the *later* of
LastMeaningfulOutput and initialPromptSentAt as the inactivity
reference. Once the dialogGaveUp fall-through reaches the initial-prompt
-send step, initialPromptSentAt becomes "now" and permanently wins over
any artificially-stale LastMeaningfulOutput the test seeds — so the real
10-minute driverInactivityTimeout can never be reached in test time.
2. The initial-prompt-send step itself is gated on claudeAtPrompt (never
true against this test's static fake pane content) or timedOut, which
only becomes true after driverReadyTimeout (30s) — a wait the test
didn't budget for.
Per plan.md's own anticipated fallback for this exact test ("a narrower
unit test on the post-latch branch logic if a full-duration ticker test
proves impractical"), rewrote the test to prove the actual regression
surface directly: dialogGaveUp's fall-through reaches the code *after* the
dialog-answer branch (proven by the initial prompt's SendKeys call actually
firing) rather than being trapped in the `continue` Blocker 1 exists to
close, instead of waiting on the unreachable full inactivity-timeout path.
Verified: fails against the old assertion shape when run standalone,
passes with the rewrite; go test -race ./session clean; make lint clean.
10 tasks
Contributor
✅ Registry ValidationTest Coverage: 22/180 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
Contributor
UX Analysis
|
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
Contributor
🎬 E2E Feature Demos2 shard(s) recorded feature flows for this PR. recordings shard 1 Demo preview opens directly in browser (single-file HTML). Raw WebM recordings in ZIP. Expires after 30 days. |
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.
Summary
Fixes a bug where a single keystroke (observed as
1) got repeatedlyre-delivered to the agent/tmux session while a session's connection was
flapping (connect → "session not started or paused" → reconnect), making
the session unusable.
Two independent, previously-untested code paths contributed:
SessionDriver's startup-dialog auto-answer(
isStartupDialog/shouldApprovePrompt→SendKeys("1\n")) had nobound on how many times it could resend the same answer while the PTY
buffer looked "stuck" (a real flap) or kept growing with unrelated new
output (an ordinary, non-flapping session).
useTerminalStream's input path had no guard againstreplaying/leaking buffered input across a superseded connection
generation during a reconnect.
Supersedes #288, which was opened from a branch built on a badly stale
local
main(~1429 commits behindorigin/main), making its diff show2769 unrelated files. This PR is the same 12 commits, cherry-picked onto a
fresh branch off current
origin/main.What Changed
session/session_driver.go: added a hash-basedDialogAnswerLatch(
answerDialogOnce,maxDialogAnswerAttempts=3) that tail-slices the PTYbuffer before matching/hashing so resends are bounded both during a real
stuck-buffer flap and during ordinary active-session output growth.
Rebasing onto
origin/mainsurfaced that main had independently addedits own simpler fix for the same symptom (a
dialogAwaitingClearbool);this PR's tail-sliced, bounded-retry latch fully supersedes it, since
main's version still read the raw (non-tail-sliced) PTY buffer and would
reproduce the original bug in an ordinary, non-flapping session once new
output pushed the dialog text out of a tailed window.
session/instance_state.go:GetEffectiveStatus()'s data race isfixed differently than originally —
origin/mainhad independentlyfixed the same underlying race via a lock-free
Instance.Snapshot()copy-on-write pattern in the interim, which this PR now uses instead of
a manual
RLock.server/services/connectrpc_websocket.go: extracted the inputread-goroutine (
runInputReadLoop) with a bounded, prompt exit onconnection close.
origin/mainindependently added an unrelated newstreamShellViaControlModefeature (shell-tab streaming) in the samefile; both coexist untouched.
web-app/src/lib/hooks/useTerminalStream.ts: added aconnection-generation guard so an overlapping/stale
connect()ordisconnect()can't mutate a newer generation's live connection state.origin/mainhad independently grown a substantial new backoff/auto-reconnect system (
BackoffState,isHardFailed,handleManualReconnect,visibility/online reconnect listeners) since this branch's fork point;
the generation guard is grafted into that newer structure rather than
replacing it.
web-app/src/lib/terminal/MessageQueue.ts+ newInputDropBadge.tsx/useDropEpisodeCoalescer.ts: input queuedduring a disconnect is dropped (not replayed) when superseded, and the
user is visibly (badge) and audibly (assertive
aria-liveannouncement)signaled when this happens.
rewritten to work with
origin/main's newisConnectingRefguard, whichmakes some of the original tests' synchronous overlapping-
connect()premise unreachable through the public API — see inline comments in
useTerminalStream.test.tsfor the adaptation rationale.Test plan
go build ./...— cleango test ./session/... ./server/...— all greengo test -race ./session -run "TestSessionDriver_StuckDialogAnswersBoundedNotUnbounded|TestSessionDriver_TailSliceBoundsDialogMatchAndHash|TestSessionDriver_DialogGaveUp_FallsThroughToInactivityEscalation|TestAnswerDialogOnce"— clean, no racescd web-app && npx tsc --noEmit— cleancd web-app && npx jest --no-coverage— 3675 tests, only 2 pre-existing failures in files this PR never touches (SessionDetail.embedded.test.tsx,BacklogEmptyState.test.tsx— confirmed zero diff viagit diff --statagainst those paths)make lint— 0 issuesmake build— web UI + Go binary build successfullyTestSessionDriver_DialogGaveUp_FallsThroughToInactivityEscalation: the rebase surfaced that its original assertion (waiting for the real 10-minute inactivity timeout) was unreachable in test time even before the rebase (session_driver.gois byte-identical pre/post-rebase) — per plan.md's own anticipated fallback, rewrote it as a narrower, faster unit test on the actual regression surface (the dialogGaveUp control-flow fall-through), confirmed to fail against the old assertion shape and pass with the fixCloses backlog item
04089969-0f19-499c-be34-2e8bcfc4f13e.🤖 Generated with Claude Code