Repository navigation
perf(persistence): persist a matching terminal binding without re-applying it - #31033
AmethystLiang wants to merge 1 commit into
Conversation
…lying it A reattach whose binding already matches memory but is not yet on disk now only marks the selected session domain dirty and relies on the writer to persist it, instead of cloning the whole session twice for rollback and re-applying equal values. Adds a defaults-guard miss to the fast-lane predicate and a binding.path span attribute.
968a3b9 to
c49d758
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main performance change: persisting an already-matching terminal binding without re-applying it. Description check The description is complete and relevant. It covers the user-facing behavior, implementation, rationale, measured impact, testing, platform considerations, linked-issue exception, and checklist items.… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and relevant. It covers the user-facing behavior, implementation, rationale, measured impact, testing, platform considerations, linked-issue exception, and checklist items. The omitted manual test checkbox and lack of a separate Review section do not prevent approval because automated coverage and maintainer context are documented.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · 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.
Comment @coderabbitai help to get the list of available commands.
ELI5
When a terminal reattaches and Orca already knows the right answer, it was still making two full backup copies of your whole saved session "just in case it had to undo", then saving. Now it skips the pointless copies and just saves.
What Changed
Before: a terminal reattach whose binding already matched memory, but whose session had not yet reached disk (for example, right after an unrelated settings edit), was treated like a real change. It scanned for conflicts, cloned the whole selected-host session, re-applied equal values, and cloned it again to keep for rollback.
After: that case is recognised as "already correct, only needs saving". It marks the selected session domain dirty and lets the existing writer persist it. No apply, no conflict scan, and no binding snapshots. Anything that actually changes a binding takes the old path unchanged.
Mechanism:
pty-binding-fast-lane.ts: newdefaults_not_appliedmiss, so a binding without the defaults guard still goes through the repair path (clean and dirty requests alike).pty-binding-persistence.ts+ newpty-binding-durability-only.ts: when the only miss isnot_durable, markworkspaceSession(local) orworkspaceSessionsByHostId(SSH/runtime) dirty and return a success result with a no-op rollback. The no-op rollback keeps the writer's failure publication to listeners without restoring anything.pty-binding-owner-conflict-report.ts: the existing report-only conflict scan, moved out unchanged to keeppty-binding-persistence.tsunder the line limit.pty-binding-span.ts: boundedbinding.pathattribute (durable_match,durability_only,mutation); successful dirty matches share the existing sampling budget with clean fast-lane hits.Unchanged: the writer, queue, generation semantics, serializers, notification order and scope, finalization, and actual-mutation rollback.
Why
The cost scales with total session size, not with the one terminal being reattached, because both snapshots clone every workspace and every saved scrollback buffer, and both stay alive for the whole disk wait.
Alternatives not taken: awaiting
pendingWrite(it swallows errors and may cover older work, so it could acknowledge a binding that was never saved) and enqueueing a flush from inside the operation (it would wait behind itself). Reusing the existing in-operation capture keeps the writer's failure and acknowledgement result.Measured impact
Dirty-match reattach, real writer worker and SQLite, baseline
a282409d20fvs this change, alternating runs, median of runs, 20 trials each:Linked Issue
N/A. Maintainer performance work, no pre-existing issue.
Visual Proof
N/A. No UI or interaction change; this is persistence-internal.
Testing
pnpm tc:node,pnpm lint, andpnpm run check:code-quality:changedpass.Tests pass for the fast-lane predicate, flush and save scheduling, durable writes (including the new matching-durability suite), maintenance contracts, host-partitioned SSH bindings, SSH replay, and the binding span.
New coverage: defaults-guard miss on clean and dirty requests; a dirty match on local and SSH hosts clones nothing and ends up durable; getter edits with settings-only dirty intent are captured under paused acknowledgement; later state and failure publication are preserved; span sampling keeps failures and refusals.
Platforms: persistence logic only, with no path or process handling, so it is platform-independent. SSH and runtime partitions are covered by tests. I did not launch the app.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck, and the relevant tests pass locally