Repository navigation
fix(machines): ask to connect herdr's PCs again after their approvals were set aside (#2053) - #2192
Merged
Merged
Conversation
… were set aside (#2053) The Settings hint after a crash recovery now tells apart machines.json (add the missing PCs again) from herdr-profile-state.json (choose Connect on the PCs saved in herdr again). docs/remote-pcs.md describes the recovery and the .corrupt-<time> copies. Tests: the saving test runs persist() itself once saveSoon armed its timer instead of polling the 1 s timer (4 s -> 30 ms); setAsideDamaged gains a test for a copy that cannot be flushed, and the ordering test names the file it synced.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 25 minutes. View limit details
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 |
Owner
Author
|
Merging 4e0c8ee: omo-native-code-reviewer and Codex both pass this head. No approval can be skipped: the set-aside store starts empty, and herdr's PCs show their Connect card. The rewritten save test uses no fixed sleeps, and it fails when the code it covers is broken. The required checks are green, and CodeRabbit did not review this head. The LOW notes go to a follow-up issue. |
This was referenced Oct 28, 2026
devswha
added a commit
that referenced
this pull request
Oct 28, 2026
#2219) ## Problem `scripts/terminal-arrows-clicks-regression.ts` (Browser regression, other scripts) failed in the first CI attempt of PRs that touch no terminal code, and passed on rerun: - #2157, run 40627880039, both tries: `an arrow between two letters arrives between them`, got `a←←bab←aba←ba←b`. - #2192, run 40638802659 attempt 1: first try `every held key received in order` (28 of 69, left over `←←x←…`: two repeats overtook the letter `x`); its retry `timed out: left click received as press and release`. Two causes: 1. **An arrow typed right after a letter.** A plain arrow goes through herdr's `pane.send_keys` RPC, around the attach pty the letter was just typed into. herdr offers no barrier for bytes in that pty, so `server/core/input.ts` holds the arrow `KEY_SETTLE_MS` (40 ms) after the last pty keystroke (`TYPED_SETTLE_MS` after an ESC). #634 documents this as a known limit: "the 40 ms settle is a margin, not a delivery acknowledgement". A stall longer than that on a busy runner (6 browser workers, Node PTY host) lets the arrow arrive first, and both failures in the letter/arrow sections are that. herdr 0.9.5 still has no barrier and no report of the program's cursor-key mode (checked its CLI and the generated API), so the product cannot make this exact. The check asserted an order the product only reaches by a margin. 2. **The first click after the pane opens.** `useTerminalMount` hands a press on as a click only when `term.modes.mouseTrackingMode` is on at mousedown. The check clicked right after `input-ready`, and the attach's first output, which turns xterm's mouse reporting on, can be parsed after that. The press was then a plain selection and nothing was sent. ## Change (test only) - **Letters, then arrows.** The check reads each letter before pressing the arrow that follows it, in the five `a ← b` rounds, after Escape, and after each letter between held repeats. An arrow followed at once by a letter stays back to back: the lane queues typing that arrives while a key waits, so that order is exact. - **New `server/core/key-settle.test.ts`.** It drives `createInputLane` with a fake attach and proves the hold that the browser check no longer races. The key goes out at least `KEY_SETTLE_MS` after a letter typed into the pty, and `TYPED_SETTLE_MS` after an ESC. A letter typed while the key waits goes behind it through herdr, not into the pty. - **No fixed waits for "nothing arrives".** Each `Bun.sleep(400)` is replaced by `mark()`: it types `z` after the action and waits until the program has read it and its frame is reported. Anything the action sent went out first on the same socket and pty (or herdr queue, on a mirror), so what came before the mark is all it sent. The file's entry leaves `scripts/fixed-sleeps.json` (9 → 0). The only sleep left is the autorepeat pacing, marked `// fixed wait`. - **Clicks** wait for `.xterm.enable-mouse-events`, the state the page reads to decide that a press is a click. ## Evidence Mutations, each run through the browser check and the unit test, then restored: | Mutation | Result | | --- | --- | | typing ignores a waiting key (`paneQueues.has(...) \|\| stop` → `stop`) | browser FAIL `an arrow between two letters arrives between them`; unit FAIL (letter behind key) | | no settle before a key | unit FAIL (both hold tests); the browser check passes, since it now waits for the letter | | `forwardClick` dropped | browser FAIL `timed out: left click received as press and release` | | link clicks replayed to the program (`!onLink` removed) | browser FAIL `a click on a link sends the program no button`, actual `\x1b[<0;7;1M\x1b[<0;7;1m`: the mark catches bytes that do arrive | | each arrow sent twice | browser FAIL in application cursor mode | **Reproduction.** A pty stall longer than the settle, simulated with `KEY_SETTLE_MS = 0` on 4 pinned busy cores: main's check failed 1 of 5 runs with CI's exact assertion, `an arrow between two letters arrives between them`. This branch's check passed 5 of 5. **Under load**, main's check and this one alternating (pr-policy's flake rule), each run through `dori heavy` and `bun run check run`: - 12 pairs while a `bun run check fast` loop ran in another checkout: this branch 12/12, main 12/12. - 20 pairs pinned with `taskset` to 4 cores alongside 6 CPU hogs, to imitate a 4-core runner: this branch 20/20, main 20/20. This PC's spare cores rarely reproduce the CI stall unaided, so the pass counts show the check holds up under load. The mutations and the stall reproduction above are what show it can still fail. ## Review Test-only, so the omo reviewer alone (notes/pr-policy.md): omo-native-code-reviewer **APPROVE**, with no CRITICAL or HIGH findings. Its MEDIUM finding was that the unit test ran with its own settle values, so a lowered `KEY_SETTLE_MS` or `TYPED_SETTLE_MS` would have slipped through. Fixed in the second commit: the two constants are now exported from `server/index.ts` (no behaviour change), and the test runs the lane with the shipped values against herdr's measured floors (a ~2 ms pty pipeline and a ~150 ms ESC hold). `KEY_SETTLE_MS = 0` and `TYPED_SETTLE_MS = 100` each fail it. The review's LOW findings are also fixed: a timed-out wait prints what was read, the mark checks that nothing came after it, and the check confirms that nothing follows the Escape and ArrowUp. The browser check also logs, without asserting, whether an arrow pressed right after a letter outran the margin. A third LOW (the queue test names the send_text path) is kept on purpose, because that path is intended. `bun run check fast`: green on 39c26af (typecheck, build, unit tests and actionlint).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change
Closes #2053.
Follow-ups from #2045 (
machines.jsoncrash recovery). Of the issue's eight LOW notes, four still applied and are small; they are fixed here. The other four are explained below.src/components/SettingsDialog.tsx): a set-asidemachines.jsoncopy keeps "add any missing PC again". A set-asideherdr-profile-state.jsoncopy now says "choose Connect on those PCs again", because those PCs come from herdr and only their permission to connect was lost (the "Saved in herdr" card's Connect). When both files were set aside, both lines show, each naming its own copies. The new string is translated in ko, ja and zh.docs/remote-pcs.md): a paragraph on the durable writes, the<name>.corrupt-<time>copies (bytes untouched, 0600, never replacing an earlier one), the hint shown until the next restart, and JSON of the wrong shape still stopping the start.server/machines-refresh.test.ts: the saving test no longer polls the privatesaveTimerfor up to 5 s while the real 1 s timer runs. It records the callback of eachsetTimeoutand runs the callback of the exact timersaveSoonarmed, so the real refresh →saveSoon→ timer callback →persist()path still runs, with no wait (about 4 s → 30 ms).server/state-document.test.ts: a new test checks that a copy whose fsync fails is removed and the original is left. The ordering test now asserts that the file it fsynced was the.corruptcopy (descriptor → path through anopenSyncspy), not merely that some fsync came first.Left, with reasons:
.corrupt-*copies) and a way to dismiss it; that is a product decision, not a small fix. The copies stay on disk and the docs now describe them.writePrivateStatenarrows the mode then. Only a widening made by hand during a run waits for the next change or restart.CHANGELOG.md,#2045 by @mattmarcin).No HTTP/WS contract, server code or remote bundle change.
Validation
bun test server/machines-refresh.test.ts -t "saving a remote PC": 4043 ms on main → 27 ms here, and 10/10 with--rerun-each 10; whole file 41/41. Mutants inserver/machines.ts, each failing the test: the timer callback not callingpersist()(0 writes),persist()always writing (3 writes instead of 1),publishnot callingsaveSoon().bun test server/state-document.test.ts: 8/8. RemovingdropCopyafter a failed flush fails the new test; flushing the original instead of the copy fails the ordering test.OVIS_STATE_DIRholding a 353-NULherdr-profile-state.json, with no herdr, no telemetry and loopback only, then opened/?settings=remotein headless Chrome at 1440 and 390 px. Only the new line shows: "The permission to connect to the PCs saved in herdr was damaged … set aside as herdr-profile-state.json.corrupt-…; choose Connect on those PCs again." The add-again line does not show.GET /api/machines/settingsansweredset_aside: ["herdr-profile-state.json.corrupt-…"].src/lib/i18n.test.tsandscripts/conventions.test.tspass.bun run check fast: green on 4e0c8ee (workflow syntax, generated types and themes, typecheck, build, unit tests).