Skip to content

test(terminal): make PTY lifecycle cases deterministic - #8551

Closed
bbingz wants to merge 3 commits into
stablyai:mainfrom
bbingz:fix/process-exit-poll-jitter
Closed

bbingz wants to merge 3 commits into
stablyai:mainfrom
bbingz:fix/process-exit-poll-jitter

Conversation

@bbingz

@bbingz bbingz commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Two independent exact-head CI failures exposed nondeterminism in pty-connection.test.ts:

  1. The replacement-hook-owner case assumes an update lands between two jittered completion polls but did not control Math.random(). Pinning the jitter midpoint makes its fake-timer boundary deterministic, matching the coordinator unit tests.
  2. Most cases create real pane foreground-agent trackers without retaining and disposing the pane binding. Delayed reads could settle after beforeEach replaced the shared store mock. CI for fix(runtime): republish projects after renderer reload #8072 proved the leak: a single-LEAF_1 case received two callbacks for LEAF_2, which that case cannot create.

The fixture now restores mocks after every case and records/disposes foreground trackers before the next store mock is installed.

CI evidence

Testing

  • Jitter-sensitive case passed 25 consecutive runs.
  • pty-connection.test.ts + pane-foreground-agent-tracker.test.ts: 457/457 passed.
  • pnpm run typecheck.
  • oxlint on the changed test.
  • oxfmt --check on the changed test.
  • git diff --check.
  • Exact-head Node 24 PR Checks: full verify, unpacked build, and packaged CLI smoke passed.

Evidence limitation

The cross-case tracker leak did not reproduce locally under Node 26; its diagnosis is based on the authoritative Node 24 CI's impossible cross-pane callback arguments. Exact-head PR Checks are the final verifier.

Scope

Test fixture lifecycle only; no runtime behavior changes.

@bbingz

bbingz commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PTY connection test suite tracks and disposes foreground agent trackers during teardown, restores Vitest mocks after each test, and stubs Math.random to 0.5 in the confirmed idle exit test to make coordinator poll jitter deterministic.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning It covers summary and testing, but it omits required template sections like Screenshots, AI Review Report, Security Audit, and Notes. Add the missing sections, include a screenshots/no visual change note, and fill in the AI review, security audit, and notes details.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main change: making PTY terminal lifecycle tests deterministic.

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

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

@bbingz bbingz changed the title test(terminal): pin process-exit poll jitter test(terminal): make PTY lifecycle cases deterministic Jul 13, 2026
@AmethystLiang
AmethystLiang requested a review from Jinwoo-H July 13, 2026 16:47
@bbingz

bbingz commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded. Against current main@d8629c41c8, this PR produces no remaining merge diff. The deterministic Math.random jitter pin landed in #8534 (45c4a1f61e), and the cross-test async cleanup landed in #9260 (bd7205684a). No unique change remains in this PR.

@bbingz bbingz closed this Jul 18, 2026
@bbingz
bbingz deleted the fix/process-exit-poll-jitter branch August 1, 2026 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants