Skip to content

fix(terminal): damp park-verdict churn that is too slow to burst - #20851

Open
OrcaWin wants to merge 1 commit into
mainfrom
fix/park-verdict-sustained-churn-damping
Open

OrcaWin wants to merge 1 commit into
mainfrom
fix/park-verdict-sustained-churn-damping

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​364 $\color{#cf222e}{\Huge{\mathbf{−}}}$​4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​360
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​128 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​122

What this fixes

The cold-park verdict flip damper only engaged on a burst — 3 flips inside 1s, a threshold derived from React's 50-commit nested-update bail. The notice limit (12 flips in 60s) was breadcrumb-only, on the reasoning that churn that slow was never near React's bail and damping it would spend a mounted pane's memory for no crash-safety gain.

That reasoning only weighed crash safety. It never weighed the remounts.

ELI5

Orca decides, per terminal tab, whether to "park" it — unmount the pane to save memory. For one user that decision kept flip-flopping: park, unpark, park, unpark, every few seconds, for 47 minutes straight. Every flip tears the pane down and rebuilds it.

There was already a brake, but it only caught fast flip-flopping — three flips in one second. This user's was slow: one flip every three to five seconds. Fast enough to be miserable, too slow to trip the brake. This PR makes the slow case trip a brake too, and the brake gets longer each time the tab refuses to settle (1 min, 2, 4, up to 8), resetting once the tab calms down.

Fix evidence

Field bundle Nz4kzIG_NwLd8KObgjJDKA (v1.4.201, win32) carries 52 churn crumbs across two episodes in two launches:

launch when crumbs tabs duration
L3 09-14 00:53 35 (34 window + 1 burst) 1 46.9 min
L4 09-14 17:07 17 (17 window) 4 8.9 min

Per-flip cadence is elapsedMs / (NOTICE_LIMIT - 1) — the window opens on a flip, so the 12th sits 11 intervals later. L3: 3.19 / 4.63 / 5.44 s (min/median/max).

Across 321 field bundles, 24 carry this churn and all 24 burst at least once, so the burst damper reached every one. This install is the only one that also sustains window-limit churn — that is the part that ran undamped.

Measured closed-loop (driving the pin back into the verdict the way useTerminalParkVerdictPin does), 47 simulated minutes, counting pane remounts — the user-visible quantity:

per flip transitions before → after ceiling reached
3193 ms 884 → 96 (9.2× fewer)
4627 ms 610 → 96 (6.4× fewer)
4800 ms 588 → 96 (6.1× fewer)
5000 ms 564 → 126 (4.5× fewer)
5442 ms 519 → 156 (3.3× fewer)

These count verdict transitions; a mount/unmount cycle is two, so pane mounts are half each figure.

Against the pre-PR source, 9 of the 33 tests fail.

Trade-offs — none hidden

  • This does not fix the driver. Whatever keeps re-proposing the park is still unidentified. The pin masks the rendered verdict; the driver resumes when each pin lapses. This caps the remount rate of an oscillation; it does not end one.
  • Memory. A pinned pane stays mounted for up to 8 minutes, re-armed while churn persists. One oscillating per-worktree flag flips every tab in that worktree in the same pass, so all of them can pin at once. Worse: the merged verdict includes the retention-budget force-park, so a churn-pinned pane can stay mounted through memory reclamation that was trying to free it. That is the strongest form of the original objection and it is not measured here.
  • Only the notice path backs off; a burst still takes a flat one-window pin. The corpus supports leaving it — repeat bursts arrive a median 732 s apart, and 1 of 30 inter-arrivals falls in the band that would mean re-firing the instant a pin lapsed.
  • A known gap, asserted rather than closed. Churn slower than 60s/11 = 5454.5 ms per flip never reaches the limit. The field's slowest observed episode runs 5441.8 ms — 12.7 ms inside that edge, not comfortably clear of it.
  • Telemetry sensitivity drops ~5×, for the very corpus scan that found this. Surviving crumbs carry sustainedPinCount, which discriminates sustained churn better than raw volume, but the detection method changes.

What this PR does not claim

The user wrote "The connection closes, reconnects and again and again and again." An earlier draft took that as the mechanism. It is not. A remount does not reconnect a terminal — parking deliberately keeps the PTY alive, SSH restores from main's snapshot, and a remote runtime re-subscribes a stream on a per-environment multiplexer that outlives the pane. The bundle carries no remote-terminal stream-stall crumbs and no daemon session churn in either window, and the report was filed 12.3 minutes after that launch's churn had already stopped. The churn is real and worth damping on its own terms; the causal link to that sentence is not established.

Adversarial review: 4 rounds, clean on round 4

Rounds 1–3 each found the round's worst defect inside the previous round's fix. Every lens ran in its own worktree pinned to a commit SHA.

round what it found severity
1 The exponential back-off never engaged in production. While a tab is pinned the hook subtracts it from the rendered verdict, so no flip is recorded and windowStartMs never advances — so the quiet branch wiped the counter on the first pass after every pin. Shipped 3.6× the intended remounts. All 24 tests passed either way, because the harness was open-loop. A second lens found five wrong claims in the message, including a per-flip divisor of 12 where the code uses 11 — making the stated safety margin 37× too generous — and a "reconnect" mechanism the source contradicts. critical
2 Round 1's fix made the back-off ratchet: the flip path cleared it on flips < noticeLimit, and the notice path leaves flips at exactly noticeLimit, so four isolated episodes six hours apart reached the 8-minute ceiling. It was also inert above ~4.7 s/flip — at the field's slowest cadence round 1's fix changed nothing (519 → 346 with and without it). blocking
3 Round 2's fix was defeated by one line. Engaging the pin is itself a rendered verdict transition, so the flip it causes overwrote the future deadline with a current timestamp. Round 3 also showed the round-2 message's "geometric edge" explanation was wrong — it was this bug — and that reverting to round 2's threshold still passed all 31 tests. blocking
4 No defect inside round 3's fix — the streak is broken. Confirmed the clobber fix is provably bounded in both directions, re-ran rounds 2 and 3's findings (ratchet gone, inertness gone, one cliff only and it is the documented geometric one), and swept 2000–9000 ms in 20 ms steps with no regression. Found the eased-flip clause itself untested, now fixed. merge

The test gap mattered more than any single bug. easedFlipCount → noticeLimit — behaviourally round 2's defective revision — passed all 31 tests. The closed-loop test that records a flip inside a live pin is the sole killer of both that and round 3's clobber, and a sparse-churn test now covers the remaining clause. Four mutants that survived round 4 all die.

The cold-park verdict flip damper only engaged on a burst: 3 flips inside a 1s
window, a threshold derived from React's 50-commit nested-update bail. The
notice limit — 12 flips in 60s — was explicitly breadcrumb-only, on the
reasoning that churn that slow was never near React's bail and damping it would
spend a mounted pane's memory for no crash-safety gain.

The field disagrees about the cost, though not for the reason first recorded.

WHAT THE BUNDLE SHOWS

Bundle Nz4kzIG_NwLd8KObgjJDKA (v1.4.201, win32) carries 52 park-verdict churn
crumbs. They are two unrelated episodes across four launches, not one run:

  L3  09-14 00:53   35 crumbs, 1 tab,  46.9 min, 34 window + 1 burst
  L4  09-14 17:07   17 crumbs, 4 tabs,  8.9 min, 17 window + 0 burst

L3's 46.9 min counts a lone trailing burst; the window-triggering run itself is
35.2 min, with an 11.7 min quiet gap before that final burst.

Per-flip cadence is elapsedMs / (NOTICE_LIMIT - 1), not / NOTICE_LIMIT: the
window opens ON a flip, so the 12th sits 11 intervals later. L3 runs 3.19s /
4.63s / 5.44s per flip (min/median/max); L4 runs 3.19 / 3.20 / 5.01.

Across 321 field bundles, 24 carry this churn and all 24 burst at least once —
though those 24 are roughly 17 devices, and 23 of them are burst-only, so the
load-bearing fact is narrower than it sounds: this install is the ONLY one that
ever tripped the window rule, and that is what ran undamped.

WHAT IT COSTS, STATED ACCURATELY

Each flip remounts the pane. A remount does NOT reconnect a terminal: parking
deliberately keeps the PTY alive (terminal-parked-tab-watchers runs a pane-less
byte watcher), SSH restores from main's snapshot, and a remote runtime
re-subscribes a stream on a per-environment multiplexer that outlives the pane.
The cost is a remount each time, indefinitely.

The user who filed wrote "The connection closes, reconnects and again and again
and again all the time. Is a cycle." An earlier draft of this change took that
as the mechanism. It is not: the bundle carries no remote-terminal stream-stall
recovery crumbs, no daemon session churn inside either churn window, and one
pane-recovery remount two hours earlier. And the report was filed in L4, 12.3
minutes AFTER that launch's churn had already stopped. The churn is real and
worth damping on its own terms; the causal link to that sentence is not
established, and this change should not be read as proving it.

THE FIX

The notice limit now engages the same unpark pin the burst does, backing off ×2
per consecutive notice window to an 8-minute ceiling, clearing once a full
window closes below the limit.

Measured closed-loop — driving the pin back into the verdict the way
useTerminalParkVerdictPin does — over 47 simulated minutes, counting pane
remounts, which is the user-visible quantity:

  per flip    transitions before -> after   ceiling reached
  3193 ms     884 -> 96   (9.2x fewer)      8x
  4627 ms     610 -> 96   (6.4x fewer)      8x
  4800 ms     588 -> 96   (6.1x fewer)      8x
  5000 ms     564 -> 126  (4.5x fewer)      8x
  5442 ms     519 -> 156  (3.3x fewer)      8x

These count verdict TRANSITIONS; a mount/unmount cycle is two, so actual pane
mounts are half each figure. The ratios are unaffected. The harness applies the
pin in the same commit cascade as the flip that caused it, which is what the
hook does and what three rounds of review showed an open-loop harness cannot
see.

TRADE-OFFS AND LIMITS, NONE OF THEM HIDDEN

- This does not fix the driver. Whatever keeps re-proposing the park is still
  unidentified; the pin masks the rendered verdict and the driver resumes when
  each pin lapses. This caps the remount rate of an oscillation; it does not end
  one.
- A pinned pane stays mounted and holds its memory, re-armed while churn
  persists. One oscillating per-worktree flag flips every tab in that worktree
  in the same pass, so all of them can pin at once. The merged verdict includes
  the retention-budget force-park, so a churn-pinned pane can stay mounted
  through memory reclamation that was trying to free it — and the reclaimer
  records success either way, because its only anomaly breadcrumb fires on a
  different condition.

  The honest number is a duty cycle, not a ceiling. Measured per cadence:
  89.7% of wall time pinned at 3193ms/flip, 84.3% at 4627, 83.9% at 4800,
  77.8% at 5000, 69.1% at 5442, with a peak contiguous pin of ~480s. Before this
  change none of these cadences pinned at all, so blocked reclamation goes from
  roughly 0% to 69-90%. Repo-stated cost is ~2.5MB per mounted pane at the 5k
  scrollback default and ~19MB at 50k. This is the strongest form of the
  original "memory is not free" objection and the change does not answer it.
- Only the notice path backs off; a burst still takes a flat one-window pin.
  The corpus supports leaving it: repeat bursts arrive a median 673s apart
  (n=29, grouped by bundle/launch/tab; 356s if physical pairs are deduped), and
  1 of those 29 inter-arrivals falls in the band that would mean re-firing the
  instant a pin lapsed. That margin is thin — the next gap above the band is
  75.3s, 0.3s outside it.
- One hard edge, geometric: reaching 12 flips takes 11 intervals, so churn
  slower than 60s/11 = 5454.5ms per flip never reaches the limit at all. The
  field's slowest observed episode runs 5441.8ms, 12.7ms inside it. Past roughly
  4.7s per flip each window only barely makes the limit, so damping engages
  later and the win shrinks from 9.2x to 3.3x — but the back-off reaches the
  8-minute ceiling at every cadence measured.
- Crumb volume drops by roughly 6x, a loss of
  telemetry sensitivity for the very corpus scan that found this. The surviving
  'window' crumbs carry sustainedPinCount, which discriminates sustained churn
  better than raw volume did; 'burst' crumbs do not carry it. The detection
  method changes either way.

REVIEW: TWO ROUNDS, AND BOTH FOUND THE FIX ITSELF

Round 1 found the back-off never left 1x in production. While a tab is pinned
the hook subtracts it from the rendered verdict, so no flip is recorded and
windowStartMs never advances; by the time a pin lapsed, windowStartMs was always
older than a full window, so the quiet branch wiped sustainedPinCount on the
first pass after EVERY pin. Closed-loop, that shipped 3.6x the remounts
intended. The 24 tests then in the file passed either way, because the harness
was open-loop. A second lens found five wrong claims in the commit message,
including a per-flip divisor of 12 where the code uses 11 — which made the
stated safety margin 37x too generous — and a reconnect mechanism the source
contradicts.

Round 2 attacked round 1's fix and found two more:

  - The back-off RATCHETED. The flip path cleared it on `flips < noticeLimit`,
    and the notice path leaves `flips` at exactly noticeLimit, so a window
    reopening after hours of silence did not clear it. Four isolated episodes
    six hours apart reached the 8-minute ceiling — the precise behaviour the
    code comment claimed was impossible.
  - And it was INERT above ~4.7s per flip, because a lapsing pin knocks the next
    window out of alignment so it closes at 11 of 12, which that same
    `flips < noticeLimit` test read as the churn easing. At the field's slowest
    observed cadence round 1's fix changed nothing at all: 519 -> 346 both with
    and without it.

Both come from one wrong question. The reset now asks whether the tab has been
genuinely quiet — a full window with no flip and no live pin, tracked on an
explicit lastFlipMs — or whether the closing window held fewer than half the
notice limit. A window closing at 11 of 12 is neither.

Round 3 found the defect inside round 2's fix, and it was one line. Engaging the
pin IS a rendered verdict transition: the hook drops the tab from the parked
set, the effect re-runs on the changed dep, and that flip lands while the pin is
live — overwriting the future deadline with the current time. The back-off then
cleared one window after the pin STARTED rather than one window after it ended.
Round 3 also showed the residual slow-cadence weakness was this bug and not the
geometry the round-2 message blamed, and that reverting to round 2's threshold
still passed all 31 tests. `lastFlipMs` no longer moves backwards off a live
deadline, and a closed-loop test that records a flip inside a live pin now kills
both that clobber and round 2's threshold.

Round 4 found no defect inside round 3's fix and judged it mergeable. It did
find that the eased-flip clause was itself untested — four mutations of it,
including reverting to round 2's threshold, passed every test — and that the
test whose comment claimed to cover it was satisfied by the other disjunct. A
sparse-churn test (three flips per window, indefinitely: eased but never silent)
now separates the two and kills all four.

Also from the review rounds: two separate dead clauses in this function, one
added by each of the previous two rounds, are gone; the burst-interval median in
the shipped comment was one row off (673s, not 732s) and n is 29, not 30. That
median is grouping-fragile — 702.7s by (bundle,tab), 356.1s deduped — but the
facts it supports are not: 1 value in the 59-75s band under every grouping, and
the next gap above it is 75.3s.

Known and not fixed: a tab churning continuously at 6-10 flips per window sits
in the gap between both reset conditions and holds its back-off indefinitely, so
the next notice after it speeds up pins for the ceiling rather than one window.

Stated rather than fixed: because quiet is measured from the pin deadline, a tab
at the 8-minute ceiling needs roughly nine minutes of real silence before the
back-off clears, not one window. The record's doc says so.
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview 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: 4795b6dd-654f-449c-9e0a-be63c457eaf9

📥 Commits

Reviewing files that changed from the base of the PR and between caa465d and 6ffbfc3.

📒 Files selected for processing (3)
  • src/renderer/src/components/terminal-pane/terminal-cold-park-withheld-tabs.test.ts
  • src/renderer/src/components/terminal-pane/terminal-park-verdict-flip-telemetry.test.ts
  • src/renderer/src/components/terminal-pane/terminal-park-verdict-flip-telemetry.ts

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


📝 Walkthrough

Walkthrough

Terminal park flip telemetry now tracks sustained churn and applies exponentially increasing pin durations up to a configured maximum. Activity uses flip and pin deadlines, and sustained state resets after quiet or reduced-churn windows. Tests cover cadence thresholds, closed-loop churn, back-off, maximum duration, isolated episodes, and reset behavior. A test helper now supplies the required lastFlipMs field.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 6ffbf

The sustained pinning change has no identified unresolved issue in the supplied review context and is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description gives detailed context, rationale, trade-offs, evidence, and test coverage. However, it omits required template sections, including Linked Issue, Visual Proof or an explicit N/A explan… Add the missing template sections. Include a linked issue after “Fixes #”, state “N/A” with a reason if there is no visual change, document testing steps and tested platforms, complete the Agent skill upstream boundary section, address the …
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: damping terminal park-verdict churn that is too slow to trigger burst damping.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files.
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.
Full details: Description check

Explanation

The description gives detailed context, rationale, trade-offs, evidence, and test coverage. However, it omits required template sections, including Linked Issue, Visual Proof or an explicit N/A explanation, Testing details and checklist, Agent skill upstream boundary, Notes, and the final Checklist.

Resolution

Add the missing template sections. Include a linked issue after “Fixes #”, state “N/A” with a reason if there is no visual change, document testing steps and tested platforms, complete the Agent skill upstream boundary section, address the Notes items, and complete the checklist. Add AI Disclosure and Review content when applicable.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — two documentation nits inline. I independently re-ran the changed suites (38 tests, green), typecheck, and lint, and reproduced the closed-loop damping claims below the window cliff.

Reviewed changes

  • Sustained-churn damping — the notice limit (12 flips/60s) now engages the unpark pin instead of only breadcrumbing, with exponential back-off (60s → 8min) tracked by sustainedPinCount.
  • Quiet-activity tracking — new required lastFlipMs records a flip or a suppressing pin's deadline, so the back-off resets only after a genuinely quiet window and cannot ratchet across isolated episodes.
  • Pin-clobber fixlastFlipMs = max(nowMs, pinnedUntilMs) keeps a flip that lands inside a live pin (pin engagement is itself a transition) from overwriting the deadline and clearing the back-off a window early.
  • Test coverage — added an open-loop regression guard plus a closed-loop harness that feeds the pin back into the rendered verdict, pinning the clobber, the alignment cliff, the 8× ceiling, and both reset conditions.

I verified the state machine independently: a 47-minute closed-loop run at the 5442 ms/flip slow end reaches the ceiling (sustainedPinCount 7, max pin 480000), and a 2000-5454 ms sweep in 20 ms steps found no cadence where the back-off stays at the first 60s pin. The documented cliff sits between 5442 and 5460 ms.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

*
* Only the notice path backs off. A burst still takes a flat one-window pin,
* which the corpus supports: repeat bursts arrive a median 673s apart (n=29,
* grouped by bundle/launch/tab; 356s if physical pairs are deduped), and just

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corpus stats here (median 673s, n=29, 1 of those 29) don't match the PR description (median 732 s apart, 1 of 30 inter-arrivals). Both describe the same burst corpus, so one is stale — worth reconciling so the sample size and median a future reader trusts are the real ones.

// looks like no flips at all, so a flip-gated reset would ratchet — a tab
// that churned once at launch would still carry a ceiling pin hours on.
//
// Why no isParkVerdictPinLive check: a live pin puts quietSinceMs in the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quietSinceMs is not a field on ParkVerdictFlipRecord; this refers to lastFlipMs. Looks like a stale name from an earlier revision.

Suggested change
// Why no isParkVerdictPinLive check: a live pin puts quietSinceMs in the
// Why no isParkVerdictPinLive check: a live pin puts lastFlipMs in the

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