fix(bench): kill a TTY exchange on inactivity instead of total time

[Denver2828]

🔗 Linked Issue

Closes #3971

---

🏷️ PR Type

What kind of change does this PR introduce?

• type:bug — Bug fix (non-breaking change that fixes an issue)
• type:feature — New feature (non-breaking change that adds functionality)
• type:docs — Documentation only
• type:refactor — Code refactoring (no functional changes)
• type:chore — Build, CI, or tooling changes
• type:breaking-change — Breaking change (fix or feature that changes existing behavior)

---

📝 Summary

On Linux /dev/ptmx under transition-axis load (#3971), waitForReviewModeTTY kills an exchange on a fixed total deadline even while the TUI banner is still actively trickling in, producing the CI failure read TUI before "RDD is currently ENABLED globally." ... context deadline exceeded.

This PR replaces the total-time budget with an inactivity watchdog: the exchange dies only after ttyTimeout (10s) without receiving a single PTY byte, and every received byte resets the timer. A new ttyOverallTimeout (2min) caps the whole exchange so a forever-chattering terminal still dies. Both kill causes unwrap to context.DeadlineExceeded, so caller classification is unchanged.

A blind total-budget bump was considered and rejected (rationale recorded in the commit body): any fixed total still loses under enough load, and it slows detection of every genuine hang.

---

📂 Changes

File / Area	What Changed
bench/runner.go	Replace the total-time PTY read deadline with an inactivity watchdog (ttyTimeout, reset on every byte) plus an overall exchange cap (ttyOverallTimeout); both unwrap to context.DeadlineExceeded
bench/runner_test.go	Regression test replaying a slow trickling first frame through the real waitForReviewModeTTY (RED on merge base, GREEN on head), plus two tests pinning the overall cap and the inactivity budget

---

🤖 AI Assistance

Select exactly one option. Do not check both options.

• None — No material AI assistance was used.
• Material assistance used — Complete all applicable declaration fields below.

Tool/model (if known): Claude Code (Fable)

Material scope: Implementation drafting, test authoring, and verification orchestration, under the contributor's direction and review.

Verification performed: Regression test observed failing on the merge base (1863432) with the CI failure shape and passing on the head (including -count=5); gofmt -l clean; go run ./internal/gofmtcheck OK; go vet ./... (bench module) OK; go build ./... (root module) OK; git diff --check clean; bench full-module failing set compared byte-identical before/after the change.

Trivial formatting, spelling, minor autocomplete, search/navigation, and trivial, non-substantive mechanical transformations do not need to be itemized. See AI_POLICY.md for the canonical policy.

---

🧪 Test Plan

Unit Tests

go test ./...

Run in the bench module. New tests:

• TestRunTTYToleratesASlowFirstFrameUnderLoad — scripted terminal replays the j121 banner in trickling frames (silences of 400/400/700ms under a 1s stand-in budget, 1.5s total, over it), read through the real waitForReviewModeTTY. FAILS on merge base 1863432 with the CI failure shape; PASSES on head, also at -count=5.
• TestRunTTYOverallCapKillsAForeverChatteringExchange and TestRunTTYInactivityBudgetStillKillsASilentExchange — pin the overall cap and the inactivity budget.

The bench full-module failing set is byte-identical before and after this change: 12 pre-existing failures (the #3934 ConPTY pair plus ten POSIX-fixture failures matching #2603). This PR fixes none of them and adds none. #3934 (native Windows ConPTY) is a different defect from #3971 (Linux /dev/ptmx under load) in the same runner; it is used here only as a cross-lane non-regression bar.

Go Format

go run ./internal/gofmtcheck

OK (gofmt -l also clean).

E2E Tests (Docker required)

cd e2e && ./docker-test.sh

NOT RUN locally: Docker is unavailable in the local environment; deferred to CI. The change is confined to the bench module.

Benchmark Validation

This changes the benchmark runner, so benchmark validation applies. Unit-level validation is covered by the three tests above. The full driven-journey corpus run is NOT RUN locally (it needs the product binary and a long transition-axis run) and is deferred to CI.

Also run locally: go vet ./... (bench module) OK, go build ./... (root module) OK, git diff --check clean.

• Unit tests pass (go test ./...) — new/changed tests pass; the 12 pre-existing bench failures are unchanged byte-identical before/after (see above)
• Go format passes (go run ./internal/gofmtcheck)
• E2E tests pass (cd e2e && ./docker-test.sh) — deferred to CI (Docker unavailable locally)
• Manually tested locally

---

🤖 Automated Checks

The following checks run automatically on this PR:

Check	Status	Description
Check PR Cognitive Load	⏳	PR should stay within 400 changed lines (additions + deletions) or use size:exception
Check Issue Reference	⏳	PR body must contain Closes/Fixes/Resolves #N
Check Issue Has status:approved	⏳	Linked issue must have been approved before work began
Check PR Has type:* Label	⏳	Exactly one type:* label must be applied
Unit Tests	⏳	go test ./... must pass
Go Format	⏳	go run ./internal/gofmtcheck must pass
E2E Tests	⏳	cd e2e && ./docker-test.sh must pass

---

✅ Contributor Checklist

• PR is linked to an issue with status:approved
• PR stays within 400 changed lines, or I have requested/obtained maintainer-applied size:exception with rationale documented
• I have added the appropriate type:* label to this PR
• Unit tests pass (go test ./...)
• Go format passes (go run ./internal/gofmtcheck)
• E2E tests pass (cd e2e && ./docker-test.sh) — deferred to CI (see Test Plan)
• Benchmark validation completed, or this change is not applicable to the benchmark (explain why in the Test Plan) — unit-level done; full corpus run deferred to CI (see Test Plan)
• I have updated documentation if necessary
• My commits follow Conventional Commits format
• I understand, reviewed, and take responsibility for the complete submission
• I selected exactly one AI-assistance option and, if material assistance was used, completed all applicable declaration fields
• My commits do not include Co-Authored-By trailers

---

💬 Notes for Reviewers

• Distinct causes, deliberately: bug(bench): j121-rdd-tui-controls-global-mode flakes with a pty deadline in the CI transition-axis run #3971 (Linux /dev/ptmx under transition-axis load) and bug(bench): rc.3 PTY runner fails two deterministic native Windows tests #3934 (native Windows ConPTY) are different defects in the same runner. This PR addresses only bug(bench): j121-rdd-tui-controls-global-mode flakes with a pty deadline in the CI transition-axis run #3971; bug(bench): rc.3 PTY runner fails two deterministic native Windows tests #3934 appears here only as a non-regression bar (its failing pair is byte-identical before/after).
• Both new timeouts unwrap to context.DeadlineExceeded, so existing caller classification and error handling are untouched.
• A native 4-lens review (risk/resilience/readability/reliability) ran on the frozen candidate and closed approved with no blocking findings; gates are informational/unmanaged and this is offered as evidence only, not as delivery authority.

For production Go changes in internal/cli, internal/reviewtransaction, or internal/sddstatus:

Not applicable — this PR changes only the bench module; no qualifying guard population is touched.

• Identify any qualifying security, integrity, admission, repair, or governance guard and challenge its legitimate input population against real-world evidence.
• Confirm its guard:population direction and claim are adjacent and accurate, and that .guard-population-baseline.txt changed only when the guard contract intentionally changed.
• Do not treat a passing declaration/registry check as proof that no qualifying guard was omitted or that the population claim is semantically complete.

Summary by CodeRabbit

• Bug Fixes

  • Improved terminal interaction timeouts to tolerate slow output while still stopping exchanges that become inactive or run indefinitely.
  • Preserved successful handling of delayed terminal responses, continuous output, terminal closure, and quit actions.
  • Prevented an update notification from interrupting the related terminal workflow.

• Tests

  • Added coverage for slow, silent, continuously active, and automatically terminated terminal exchanges.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 79907b9f-38b8-4c77-8f93-cf92e12b4c3b

📥 Commits

Reviewing files that changed from the base of the PR and between 7a5b914 and 662aaf8.

📒 Files selected for processing (1)

• bench/journeys_issue3766.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

Changes

The TTY runner now enforces separate inactivity and overall exchange deadlines. PTY byte reads refresh the inactivity deadline. Tests cover slow, continuous, and silent output. The issue #3766 journey disables the update modal with a sandbox fixture.

TTY exchange watchdog

Layer / File(s)	Summary
Progress-aware TTY watchdog bench/runner.go	runTTYWithDeadlines tracks PTY progress with ttyProgressReader. ttyWatchdog enforces inactivity and overall budgets. Timeout handling uses context.Cause(ctx).
Deterministic deadline tests bench/runner_test.go	Fake PTY infrastructure and tests validate delayed progress, overall timeout, inactivity timeout, transcript capture, quit handling, and cleanup.
Issue #3766 journey setup bench/journeys_issue3766.go	The journey seeds a current last_update_check timestamp in sandbox state before the repository and TUI steps.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 662aa

The PR changes benchmark TTY timeout handling to tolerate active but slow output while retaining inactivity and overall limits; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant runTTYWithDeadlines
  participant ttyProgressReader
  participant ttyWatchdog
  runTTYWithDeadlines->>ttyWatchdog: Create inactivity and overall budgets
  ttyProgressReader->>ttyWatchdog: Record each PTY byte
  ttyWatchdog->>runTTYWithDeadlines: Cancel with timeout cause
  runTTYWithDeadlines->>ttyWatchdog: Read context cancellation cause

Loading

Suggested reviewers: decode2

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the primary change: TTY exchanges now terminate on inactivity instead of a fixed total-time deadline.
Linked Issues check	✅ Passed	The changes satisfy issue [#3971]. The TTY watchdog refreshes the 10-second inactivity budget on received bytes, enforces a 2-minute overall cap, preserves timeout classification, and adds regression …
Out of Scope Changes check	✅ Passed	All changes are related to the linked issue [#3971]. The runner changes, regression tests, and update-check fixture directly support reliable TTY benchmark journeys and do not introduce unrelated scop…

Full details: Linked Issues check

Explanation

The changes satisfy issue [#3971]. The TTY watchdog refreshes the 10-second inactivity budget on received bytes, enforces a 2-minute overall cap, preserves timeout classification, and adds regression tests for slow, continuous, and silent exchanges. The update-check fixture also prevents the modal from obscuring the affected TUI journey.

Full details: Out of Scope Changes check

Explanation

All changes are related to the linked issue [#3971]. The runner changes, regression tests, and update-check fixture directly support reliable TTY benchmark journeys and do not introduce unrelated scope.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Denver2828]

The Unit Tests red on commit 1 (7a5b914) is expected and diagnostic, not a regression.

#3971 turns out to have two distinct manifestations in the same fragile journey. The original occurrences are load slowness mid-exchange, which commit 1's inactivity watchdog addresses. Running under that watchdog, CI then exposed the second: the bench sandbox gives every journey a fresh HOME with no update-check cooldown, so the launch update check runs, and when upstream main is ahead of the CI-built binary the "Update Available" modal covers the menu j121 waits on — its first expected text, "Start installation", never arrives. Where this death previously surfaced only as an opaque read /dev/ptmx: input/output error, the watchdog now appends no PTY output for 10s: context deadline exceeded and the captured screen showing the covering modal, which is what made the cause legible.

Commit 2 (662aaf8, +15/−0, j121's journey file only) seeds a current last_update_check into the sandbox state before the exchange, mirroring the #3561 journey fixture that is green in CI for the same reason. It deliberately does not seed installed_agents: j121 exercises the not-installed state. j120 and j122 share the same latent exposure; that is outside this issue's scope and may warrant a follow-up. The green proof landed: this PR's CI run on 662aaf8 completed with j121 green — 61 journeys, 0 failed.