Skip to content

Keep terminal input responsive while restoring history - #26158

Open
nwparker wants to merge 1 commit into
mainfrom
nwparker/perf-daemon-history-seed-yield
Open

nwparker wants to merge 1 commit into
mainfrom
nwparker/perf-daemon-history-seed-yield

Conversation

@nwparker

@nwparker nwparker commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​310 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​310
Prod 8 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​249 $\color{#cf222e}{\Huge{\mathbf{−}}}$​137 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​112

ELI5

Restoring a large terminal history can briefly freeze typing in other terminals. Replay now gives pending input a turn while keeping the restored history ahead of the new shell prompt.

What Changed

Prepare the live terminal emulator with the existing bounded replay writer before spawning the restored child, then transfer it into the synchronous session constructor. The existing creation claim remains held throughout replay. Cancellation, shutdown, failed spawn and failed construction release prepared state; failed unattached construction discards startup buffers without emitting output.

Why

The constructor previously replayed every history chunk synchronously after spawn, blocking the daemon and its other PTYs. Preparing before spawn preserves history-before-prompt ordering and avoids buffering a running child during a long replay. This reuses the existing replay budget rather than adding another parser or changing history limits. Startup gates now share the existing pipeline builder; color-query answering moves into its existing reply owner to keep both modules within the line limit.

In the actual history-reader, seed-transfer and terminal-host creation pipeline on macOS, a 25 MiB seed reduced the replay event-loop pause from 367 ms to 1.5 ms and an independent native PTY's input response from 367 ms to 1.5 ms. Replay elapsed time remained about 366 ms. A 1 MiB seed reduced input delay from 18 ms to 2.0 ms. Snapshots matched in both workloads.

Linked Issue

Performance audit; no existing issue linked.

Visual Proof

N/A — terminal content and prompt ordering are preserved; this changes daemon scheduling and resource ownership.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated

19 new cases cover Unicode/escape boundaries, input modes, prompt ordering, ordinary creation, same-ID claims, cancellation, shutdown and failure cleanup. 554 broader tests and 166 final ingress/session tests passed, with two intentional skips in the broader run. An independent frozen-source comparison matched 108 seeds; the original host path fails the yielding regression. Node typecheck, native static rules and changed-file quality checks passed.

Measurements use real timers and an independent background native PTY on macOS, one warmup and four measured trials per workload. The restored child uses a fixture handle; RPC, Windows, Linux and a live SSH connection were not exercised locally. No wire fields change; the same execution-host daemon path applies to remote and folder workspaces. The complete large restore still includes an unchanged approximately 33 ms checkpoint-reading pause. This does not establish the cause of the reported ordinary Codex typing pauses.

AI Disclosure

Review

Self-reviewed cancellation, exclusive creation, emulator transfer, synchronous prompt delivery and cleanup ordering.

Agent skill upstream boundary

  • Not applicable.

Notes

Native child termination policy and normal ingress teardown remain as before.

Checklist

  • This PR is small and focused
  • Explained the behavior, mechanism and approach
  • Visual proof is N/A with reason
  • Self-reviewed for correctness, security and performance
  • Cross-platform, SSH and folder workspaces considered
  • Relevant tests, typecheck and quality checks pass; remaining platform/build checks are covered by CI

@coderabbitai

coderabbitai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 461f5560-9ca0-4fc0-a6e4-64dbb2bdbbf9
📥 Commits

Reviewing files that changed from the base of the PR and between e6bcc5a and d9fdefb.

📒 Files selected for processing (10)
  • src/main/daemon/cold-restore-replay-writer.ts
  • src/main/daemon/session-history-seed-preparation.test.ts
  • src/main/daemon/session-options.ts
  • src/main/daemon/session-output-pipeline.ts
  • src/main/daemon/session-output-plane.ts
  • src/main/daemon/session.ts
  • src/main/daemon/terminal-host-history-seed-yield.test.ts
  • src/main/daemon/terminal-host-session-create.ts
  • src/shared/pty-owner-color-query-replies.ts
  • src/shared/pty-startup-ingress.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Session creation now prepares history seeds before spawning when history chunks are present. The output plane adopts prepared seeds, and the session pipeline creates and cleans up startup components. Startup ingress can discard queued work and close reply delivery. Tests cover replay equivalence, ownership, construction failures, cancellation, and session-creation behavior.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to d9fde

No issue requiring a fix before merge was identified; normal checks remain appropriate.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 10 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: keeping terminal input responsive during history restoration.
Description check ✅ Passed The description covers the required explanation, changes, rationale, visual proof, testing, review, and checklist. It says no issue is linked, although the template requests an issue link for outside …
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

This branch has not been deployed

No deployments
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.

1 participant