You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(attach): keep herdr's takeover diagnostic from a displaced tab whose attach exits late - #2630
scripts/take-over-regression.ts:68 failed now and then on CI (main run 41559711148, and run 41477987143 on an unrelated branch). The tab displaced by Open here in another bridge was shown herdr's own line, herdr: server shut down: terminal attach taken over. This is a product race, not a test flake.
Right after an attach settles early, the refusal guard in server/core/terminal-process.ts holds a line that starts with herdr: server shut down: and passes on herdr's teardown before it. AttachOutputTail (server/attach-output.ts) keeps that teardown as the possible start of the takeover diagnostic on an ATTACH_HOLD_MS (100 ms) timer, and the guard's own timer ends at the same 100 ms. A trace of the failing case showed the tail's timer firing first: it flushed the teardown alone, the guard then published its line without it, and the tail no longer recognised the diagnostic. A whole held diagnostic was also flushed after 100 ms when herdr's exit came later than that, as on a busy PC.
The tail's timer leaves the tail to the guard while the guard still holds a line. The guard's timer always ends within one hold and republishes the line, which re-arms the tail's timer, so nothing is held without bound.
herdr's teardown before the diagnostic is passed on at once (the terminal gets it either way), and the tail remembers how much of the diagnostic it began, so words arriving in a later read still match. Only herdr's words wait, up to one deadline of ATTACH_TAKEOVER_EXIT_MS (2 s, the bound a late refusal's exit gets) from their first byte, for the exit that confirms them; ended() drops them on that exit.
Words that never complete, or a pane's own identical text with no exit, are passed on at that deadline.
Validation
Failing first: the new test in server/core/terminals.test.ts (real attach lifecycle, mocked PTY, fake timers, the exit driven after 10 holds) failed on main with Expected to not contain: "terminal attach taken over". With the fix, terminals.test.ts, attach-output.test.ts and terminal-process.test.ts pass (35/35). Two more tests cover the partial diagnostic and the pane's own words with no exit.
Browser: checkTakeOver alone through bun run check run (isolated herdr session, Node PTY host as on CI), under dori heavy: 20 consecutive passes on 05f0261 and 21/21 on 84e7638.
Merge-pass review (2 MEDIUM) fixed in 84e7638 with failing-first tests; see the comment below.
Reviews: omo code reviewer APPROVE. Reverting either half of the fix fails the new tests. LOW notes only: the 2 s bound is intended; a pre-existing case is left as a follow-up (the read-race branch of ended() returns without flushTail()). Codex Terra (gpt-6.1-terra, xhigh) APPROVE, no findings.
Skipped: full check, the integration suite, the Windows VM run.
The other three browser flakes from the same main run are fixed in a separate test-only PR.
…ose attach exits late
Right after an attach settles early, the refusal guard holds a line that starts with
"herdr: server shut down:" and passes on herdr's teardown before it. The output tail keeps that
teardown as the possible start of the takeover diagnostic on a timer of ATTACH_HOLD_MS (100 ms),
and the guard's own timer ends at the same 100 ms. When the tail's timer fired first, it flushed the
teardown alone, and the guard's line then reached the tail without it, so the displaced tab was
shown "terminal attach taken over" (take-over-regression.ts:68 on CI, Node PTY host). A whole
diagnostic was also flushed after 100 ms when the exit came later, as on a busy PC.
The tail's timer now leaves the tail to the guard while the guard still holds a line, and a whole
diagnostic waits up to ATTACH_TAKEOVER_EXIT_MS (2 s, as a late refusal's exit does) for the exit
that confirms it. The start of a diagnostic that never completes still flushes after the hold.
Auto incremental reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
Summary
Summary by CodeRabbit
Bug Fixes
Prevented a delayed terminal attach from displaying a takeover message after another browser has taken over the pane.
Incomplete or unconfirmed takeover-like output continues to appear after a delay.
The attach output tail now identifies complete takeover diagnostics. The pending-output timer waits up to 2,000 ms for a confirming exit before releasing them. Regression tests cover delayed, partial, and unconfirmed takeover output.
Changes: Takeover diagnostic handling
Layer / File(s)
Summary
Detect and delay takeover diagnostics server/attach-output.ts, server/core/terminal-process.ts, server/core/terminals.test.ts, changelog.d/takeover-late-exit.md
AttachOutputTail exposes whether its held tail starts with TAKEN. The pending-output timer waits 2,000 ms for complete takeover diagnostics and retains holdFor for incomplete output. Tests cover delayed exits, partial diagnostics, and takeover-like output without a confirming exit. The changelog records the delayed-exit case.
devswha/ovis#2373: Both PRs address takeover timing using a 2,000 ms bound.
devswha/ovis#2255: Both PRs change attach-output hold behavior for takeover cases.
Suggested reviewers:odfalik
Merge Risk: Low · previous review 05f02
The change provides bounded suppression of delayed takeover diagnostics. It is mergeable with a small changelog correction clarifying that exits beyond the timeout can still display the message.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title names the change (keeping herdr's takeover diagnostic from a displaced tab whose attach exits late) and the fix scope (attach). It matches the changeset, which adds timer deferral and a 2 s …
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.
Description check
The description follows the template with both required sections, Change and Validation. The Change section explains the CI failure, the root cause (two 100 ms timers racing), and the fix. The Validat…
Full details: Title check
Explanation
The title names the change (keeping herdr's takeover diagnostic from a displaced tab whose attach exits late) and the fix scope (attach). It matches the changeset, which adds timer deferral and a 2 s hold for complete takeover diagnostics. It is specific and concise, so it passes.
Full details: Description check
Explanation
The description follows the template with both required sections, Change and Validation. The Change section explains the CI failure, the root cause (two 100 ms timers racing), and the fix. The Validation section lists the tests run, the results, and the checks skipped (full check, integration suite, Windows VM run). It also links the CI run IDs and a separate test-only PR. No issue link is given, but the template asks for one only for features, and this is a bug fix. The description is specific and complete.
Full details: Docstring Coverage
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
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.
Reviewing files that changed from the base of the PR and between f1a6332 and 05f0261.
Files selected for processing: 4
changelog.d/takeover-late-exit.md
server/attach-output.ts
server/core/terminal-process.ts
server/core/terminals.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
Merge-pass review (Codex Terra xhigh, fake-timer reproductions). The race is fixed for the traced ordering (teardown and guarded line together: both timers reach the same deadline, the diagnostic is suppressed, the teardown kept), and the new test fails deterministically on main. Two MEDIUMs before merge:
The hold isn't bounded at 2 s. In terminal-process.ts:394 each accepted newline restarts the full timer, so output shaped like herdr's teardown + diagnostic followed by newlines 1.9 s apart stayed hidden for 5.8 s. Please bound the hold from when it starts (one 2 s deadline, not per newline), and add that case to the test. A program printing herdr's own takeover line is unlikely, but a live terminal must never stall longer than the stated bound.
Split chunks still paint the diagnostic. Sending ...terminal attach , advancing 100 ms, then taken over\r\n with an immediate nonzero exit painted the full diagnostic: the partial tail was flushed before the rest arrived; the 2 s wait only applies once the whole line is assembled. If a cheap fix exists (keep holding a prefix of the diagnostic while the guard could still complete it, under the same 2 s bound), please add it with a test; otherwise say so in the PR as a known limit (an exit more than 2 s after a complete line also paints it).
Not covered here: the mirror path's own tests (the sandbox refused their socket listener); it bypasses the changed tail.
… across slow reads
Merge-pass review of #2630:
- each read restarted the 2 s wait, so text shaped like herdr's takeover diagnostic followed by
line ends stayed hidden for seconds. The held words now wait one deadline from their first byte.
- a diagnostic split across reads more than a hold apart was painted: the tail flushed the start
it held (herdr's teardown) after ATTACH_HOLD_MS, and the words that followed no longer matched.
The teardown is passed on at once (the terminal gets it either way) while the tail remembers
how much of the diagnostic it began, and only herdr's words wait, up to the same deadline. A
plain alternate-screen exit is no longer held at all.
Merge-pass review findings, both fixed in 84e7638 (failing-first: the 5 new or changed cases in server/core/terminals.test.ts failed on 05f0261):
MEDIUM, timer restarted on every read: the held words now wait one deadline, ATTACH_TAKEOVER_EXIT_MS from their first byte; later reads (line ends) never move it. Test: waits one bounded time from the diagnostic's first byte, however many line ends follow it.
MEDIUM, diagnostic split across reads more than a hold apart: AttachOutputTail now passes herdr's teardown on at once (the terminal gets it either way) and remembers how much of the diagnostic it began, so only herdr's words wait, up to the same deadline. A split inside the teardown, at its end, or inside the words is dropped once the exit confirms it. Tests: never paints herdr's takeover diagnostic split N bytes in, its reads far apart (3 cuts). A side effect: a plain alternate-screen exit (\x1b[?1049l, as vim or less prints) is no longer held at all.
Validation of 84e7638: terminals.test.ts, attach-output.test.ts, terminal-process.test.ts, take-over.contract.test.ts: 48 pass. bun run check fast: success (tested sha 84e7638). checkTakeOver in an isolated herdr session, Node PTY host, under dori heavy: 21/21 consecutive.
Fixes the occasional take-over-regression.ts:68 failure, which was a product race: a tab displaced by Open here in another bridge could be shown herdr's own herdr: server shut down: terminal attach taken over line. The output tail's 100 ms timer could fire before the refusal guard's, flushing herdr's teardown alone so the diagnostic was no longer recognised.
Merge-pass review (Codex Terra xhigh, fake-timer reproductions, two rounds). Round 1 found that the new hold restarted its 2 s timer on each newline (diagnostic-shaped output hidden 5.8 s) and that a diagnostic split across reads more than 100 ms apart was still painted; 84e7638 fixes both with tests that fail on 05f0261. Checked on the final head: divergent near-matches and following Unicode output are painted in order without loss; close during either hold paints nothing later; a reattach doesn't carry a remembered match; the remembered teardown is bounded per attach attempt; the original race stays fixed.
Left as is (LOW): the 2 s bound is counted from when the output tail takes the line, so the guard's own 100 ms hold comes on top (ordinary output that looks exactly like herdr's takeover line can be held up to 2.1 s). A diagnostic whose last words arrive after the 2 s bound is painted, as the live-output limit requires.
CodeRabbit reviewed it; its one thread (name the 2 s bound in the changelog) was fixed in 6dedc2e and resolved.
)
## Change
Four browser scripts failed now and then in "Browser regression (other
scripts)" on main (run 41558343287 on c0cdc8e, and later runs). The
take-over failure was a product race, fixed in #2630. This PR fixes the
other three, plus the relay-media failure seen on bef2be1. Test-only:
no product code changes.
### `chat-local-send-follow-demo-regression.ts:95`: "the reader moves
away"
`wheelUp` waited for each wheel notch to lower `scrollTop` by more than
20 px. A write trace on the page showed every notch moving the view 120
px. Then, as a row above the view was measured, LegendList's
`ScrollAdjust` called `scrollBy(+103)` to keep the reader's rows in
place, with `scrollHeight` growing by the same 103 px. That undid the
notch in `scrollTop`, but the distance from the end stayed exactly 600
px before and after. The reader had moved away; the wait could not see
it. The wait now measures the distance from the end, which that
adjustment leaves alone.
Not a #2624 regression. An A/B of the unmodified script, alternating
commits under `dori heavy`, gave passes/runs of: 05c9133 (before #2624)
5/7, 9ef34f8 (#2624) 5/6, main 2/6. The step fails before #2624 too;
#2624 changes when rows get measured, so it lands more often. No
`scrollToEnd` or follow write appears in the trace.
### `file-viewer-regression.ts`: "video not visible"
The CI failure came right after `setFileOpen("panel")` reloads the page:
`videoLink.click()` was a pointer click on a transcript link while the
reloaded chat followed to its end, so the click landed on nothing and no
popup opened (0 "locator resolved" lines on CI and locally). The
script's own header comment describes this trap, and its other steps
already use `activateInTranscript`. Under load, three more steps hit the
same trap: after a `goto` (big code), after the panel closes (the
extensionless and unknown-text loop, which keeps the link focused so the
focus-return assertion still holds), and after a reload and Back at 390
px. The click on the framed page's table-of-contents link is dispatched
to the link: the frame's own navigation is under test, not where the
panel lays the frame out.
### `files-prompt-regression.ts:71`: Escape left Files open after a pane
switch
Not reproduced in about 435 local attempts (15 per run, fresh pages, 6x
CPU throttle, load up to 92), and nothing found by reading the code. The
Escape check stays: it is the only place a script checks Escape closing
Files. When it fails now, the script writes what decided it to the log
and to `files-<page>-escape-diagnostics.json` in the browser-evidence
artifact: whether a window keydown probe saw the key, the focused
element, the modal and dialog stack, whether FilesDialog stood down
(file or diff over it, native modal), and any open picker. Tracked as
#2631.
### `relay-media-browser-qa.ts:44`: "Timed out: copied-link clipboard
event"
Seen inside `relay-e2e-regression.ts` on bef2be1 (run 41568570526). The
stubbed `writeText` always resolves, so the click never reached Copy
link. `copy.click({ force: true })` is aimed at where the button was,
under a frozen browser clock, while the viewer may still move it. The
click is now dispatched to the button itself; the clipboard listener is
still subscribed before it.
## Validation
All runs went through `dori heavy`; host load ranged from about 4 to 92
(70 CPU hogs for part of it).
- chat-local-send-follow: 22/22 consecutive.
- file-viewer: 20 consecutive and counting on the final script (`bun run
check run`, isolated herdr session). Earlier rounds under load caught
the other lost-click steps fixed here.
- files-prompt: 22/22 consecutive. A forced failure (key withheld) wrote
the diagnostics JSON and screenshot. The review's try/catch, which keeps
the original timeout as the reported error, came after those runs; it
runs only when the check fails.
- relay-media (fixed): 22/22 consecutive; the unmodified script passed
10/10 locally, so CI is the only evidence for that one.
- `bun run check fast`: success on 6069b0a. An earlier run caught a
regression in this branch: `ci-browser.test.ts` forbids `??` on evidence
dirs (#1554). Fixed with `||`.
- omo code reviewer: APPROVE, LOW notes only. Applied: the diagnostics
never replace the original error. Not changed: browser-other uploads
PNGs only on failure; the 390 px reopen dispatches directly because
`activateInTranscript` waits for a frame that section lacks; the
`wheelUp` callers assert the distance afterwards.
- Skipped: full check, Windows VM.
No changelog fragment: test-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Change
scripts/take-over-regression.ts:68failed now and then on CI (main run 41559711148, and run 41477987143 on an unrelated branch). The tab displaced by Open here in another bridge was shown herdr's own line,herdr: server shut down: terminal attach taken over. This is a product race, not a test flake.Right after an attach settles early, the refusal guard in
server/core/terminal-process.tsholds a line that starts withherdr: server shut down:and passes on herdr's teardown before it.AttachOutputTail(server/attach-output.ts) keeps that teardown as the possible start of the takeover diagnostic on anATTACH_HOLD_MS(100 ms) timer, and the guard's own timer ends at the same 100 ms. A trace of the failing case showed the tail's timer firing first: it flushed the teardown alone, the guard then published its line without it, and the tail no longer recognised the diagnostic. A whole held diagnostic was also flushed after 100 ms when herdr's exit came later than that, as on a busy PC.ATTACH_TAKEOVER_EXIT_MS(2 s, the bound a late refusal's exit gets) from their first byte, for the exit that confirms them;ended()drops them on that exit.Validation
server/core/terminals.test.ts(real attach lifecycle, mocked PTY, fake timers, the exit driven after 10 holds) failed on main withExpected to not contain: "terminal attach taken over". With the fix,terminals.test.ts,attach-output.test.tsandterminal-process.test.tspass (35/35). Two more tests cover the partial diagnostic and the pane's own words with no exit.checkTakeOveralone throughbun run check run(isolated herdr session, Node PTY host as on CI), underdori heavy: 20 consecutive passes on 05f0261 and 21/21 on 84e7638.bun run check fast: success on 05f0261 and on 84e7638.ended()returns withoutflushTail()). Codex Terra (gpt-6.1-terra, xhigh) APPROVE, no findings.The other three browser flakes from the same main run are fixed in a separate test-only PR.