Repository navigation
fix(ws): say what the server supports before the snapshot that waits on herdr (#2257) - #2268
Conversation
…on herdr (#2257) A send right after a reconnect was refused with capabilities_unknown when the connection's first snapshot came later than 2 s: the snapshot was the only frame listing the server's features, and it waits on herdr's session.snapshot RPC (10 s after a restart, or never when the read fails). The server now sends a small features frame (features and connection id) synchronously on open, before the snapshot. The client learns its features from the first of the two frames and keeps the guard: nothing is sent before they are known, and an older server behaves as before. The connection's pending scope opens on that same first frame, so a queued send that leaves before the snapshot keeps its live row. Closes #2257
…knows (#2257) Review of the first commit reproduced a double send: the connection's pending scope opened on the features frame while the herdr-lifetime fence still ran on the snapshot, so after a herdr replacement a queued send made in that window stayed live in the pane's queue and was also offered as unsent text to send again. The features frame now names the herdr the server reads (herdr_lifetime, from a stat of herdr's socket, no herdr call). A page acts on it before the snapshot only when it names the herdr this page already proved; otherwise sends, receipts and the scope wait for the snapshot as before. A server restart in front of the same herdr, the common case, keeps the early path.
Re-review of the previous commit reproduced a MEDIUM: the features frame named herdr from a stat of its socket file, and a herdr that crashed can leave that file behind. A page reconnecting then could trust a dead herdr's name and send before the snapshot, with the replacement herdr's reused pane IDs. The status collector now says when its lifecycle subscription to herdr starts and closes, and the server names a herdr in the features frame only while that subscription is open under the same socket identity. Without it the frame names none and the page waits for the snapshot.
A subscription closed by its caller reports no close, so stopping the collector left the last herdr named as live. Also tidies the hub fake's doc comment and drops a test assertion that could not fail.
…empt (#2257) Re-review reproduced a MEDIUM: the herdr identity was read when the lifecycle subscription's acknowledgement arrived, not bound to the connection that answered, so herdr A's acknowledgement could certify a replacement B whose socket file had taken the path meanwhile. The collector now reads herdr's identity before each connection attempt and, on the acknowledgement, names it only if the socket still reads the same; a close, a refusal or a stop names none.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Track the live Herdr lifetime server/collector.ts, server/collector.test.ts, server/core/status.ts, server/core/status.test.ts, server/core/status.fake.ts |
The collector reports the identity of a live lifecycle subscription. The status hub exposes a Herdr lifetime only while that identity matches the current socket identity. |
Send features before the snapshot shared/protocol.ts, server/core/server-core.ts, server/core/socket.ts, server/core/socket.test.ts |
The protocol adds a features frame. The server sends it with the connection ID and available Herdr lifetime before requesting the snapshot. |
Trust feature frames for known sessions src/lib/machineSession.ts, src/lib/machineSession.test.ts |
MachineSession accepts early features only when their Herdr lifetime matches the known generation. A trusted frame can establish session scope and connection before the snapshot. |
Use discovered capabilities before the snapshot src/lib/ws.ts, src/lib/ws.test.ts, scripts/ui-regression.ts, changelog.d/features-before-snapshot.md |
The WebSocket client uses trusted feature frames for capability discovery, waits for discovery before relevant operations, and restores watches once per connection. Tests cover feature timing and reconnect behavior; the changelog records the change. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Severity of issue fixed: Medium
Possibly related PRs
- devswha/ovis#1805: Changes the send flow when capability information is not yet available.
- devswha/ovis#1808: Defines the
capabilities_unknownresult after waiting for capability discovery. - devswha/ovis#1853: Adds terminal-insert capability handling to the server feature list and client snapshot wait.
Suggested reviewers: noobear
Merge Risk: 
588bd
-
No actionable issue remains established for this change; it is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 40.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. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly summarizes the main change: the server sends supported capabilities before the herdr-dependent snapshot. Description check The description explains the problem, implementation, compatibility behavior, risks, tests, and validation results, including WS contract coverage. It uses a "Tests" section instead of the template's … Linked Issues check Issue #2257requires early feature discovery for delayed or failed snapshots, retention of the send guard, and unchanged behavior with older servers.server/core/socket.tssends afeaturesframe b…Out of Scope Changes check The changed protocol, server lifecycle tracking, client trust checks, session scope handling, regression script, tests, and changelog support the reconnect capability-discovery objective in #2257. The…Full details: Description check
-
Explanation
The description explains the problem, implementation, compatibility behavior, risks, tests, and validation results, including WS contract coverage. It uses a "Tests" section instead of the template's "Validation" heading, but the required validation information is present.
Full details: Linked Issues check
-
Explanation
Issue
#2257requires early feature discovery for delayed or failed snapshots, retention of the send guard, and unchanged behavior with older servers.server/core/socket.tssends afeaturesframe before awaitingclientSnapshot(), including the connection ID and optional live herdr lifetime.src/lib/ws.tsaccepts trusted feature frames and keeps the 2-secondcapabilities_unknownguard.src/lib/machineSession.tstrusts the frame only for a herdr lifetime already proved by an earlier snapshot. The reported socket, client, lifecycle, session, regression, and check tests cover the required paths.
Full details: Out of Scope Changes check
-
Explanation
The changed protocol, server lifecycle tracking, client trust checks, session scope handling, regression script, tests, and changelog support the reconnect capability-discovery objective in
#2257. The reported remote roster forwarding and Windows marker concern do not demonstrate an unrelated change or a separate feature. No unrelated code change is established by the reviewed summary or focused source evidence.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 40.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 5 functions across 14 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 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.
Comment @coderabbitai help to get the list of available commands.
Review findings and resolutions (omo-native-code-reviewer + Codex Sol 6.1 fast xhigh)CodeRabbit was rate-limited when this PR opened. These are the adversarial reviews this branch went through. Each ran read-only on a worktree pinned to the reviewed commit.
Each fix came with a test that fails on the commit before it. Open LOWs, not fixed (also listed in the PR body):
Final verification on 588bd5d: |
|
@coderabbitai review |
✅ Action performed
|
The inactive-attach legacy-mount scenario imitates an older bridge by removing "watch" from the snapshot's features. With this branch the server lists its features in a frame of its own first, so the page still learned "watch" from it and watched where an older bridge cannot. An older bridge sends no features frame at all: the fixture now drops it in legacy mode. Reproduced locally: ui-contexts-regression on 588bd5d's fixture fails legacy-mount (1 !== 0) as in CI; chat-size-regression passes with this.
|
Fast checks on b705dd8 (run 40713549863, attempt 2) failed on one unit test of 8255: This PR touches nothing under
So this reads as a timing flake under CI's parallel unit load. I'll rerun the failed job once when the run completes. |
|
Attempt 2 of run 40713549863 on b705dd8: Fast checks passed on rerun (the
Three other scripts failed once and passed on retry (application-paging, chat-virtual-browser-qa, agent-skill-menu-demo). The same job passed on 588bd5d, whose application code is identical: b705dd8 only changes the chat-size test fixture. The merge pass's attempt 3 is rerunning it. |
|
Attempt 3 of "Browser regression (other scripts)" on b705dd8 hit the 50-minute This is CI-wide, not this PR: main's own run on 0ca9c08 timed out in the same job (cancelled at 16:53 UTC after 51 minutes), and |
Deploying ovis with
|
| Latest commit: |
9c739bd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b7f428b8.ovis.pages.dev |
| Branch Preview URL: | https://fix-capabilities-before-snap.ovis.pages.dev |
|
Integration tests on 9c739bd (run 40736088806): one of 846 tests failed. That test drives the herdr client against a fake |
|
Merging 9c739bd (b705dd8 with main merged in, no code change of its own). A WebSocket contract change, so it got a real review; Codex was at capacity, so an omo reviewer on gpt-6.1-sol and an independent second reviewer reviewed it; both pass with no findings. The early frame carries only the feature names, this connection's id and an optional hashed herdr identity, all of which a watching device already got in the snapshot; relayed sockets pass it through MachineRelay unchanged (interact and read-only, byte for byte). The client trusts it only for the herdr generation the page already observed for that PC, so first load and an unproven or replaced herdr keep snapshot-only discovery. Against the real client socket, 10 orderings (features then no snapshot, duplicated, after the snapshot, then a snapshot error, lost, an old server, distrust, wrong identity, a reconnect mid-way, a legacy snapshot) keep the #1591 guard: no message is ever typed raw into a bridge that takes submit. Main's client ignores the new frame. On real servers with isolated herdr sessions, main reproduced #2257 (a reconnect send with the snapshot held or failed returned capabilities_unknown); this head sent through |
Closes #2257
Problem
A send from Chat right after a reconnect failed with "Not sent: this PC has not said yet what it supports. Send it again." whenever the connection's first snapshot was late. The client waits 2 s for that snapshot because it is the only frame that lists the server's features (#1805, #1808). The guard keeps a message from being typed raw into a bridge that takes
submit(#1591).server/core/socket.tssends the snapshot only afterclientSnapshot()resolves, and that call waits on herdr'ssession.snapshotRPC (10 s timeout), the OmO/GJC refresh and the completion tracker. When that read fails, the connection gets only anerrorframe and never learns its features.Measured on this PC (details in #2257):
session.snapshot timed out after 10000msten seconds after a restart. Every browser that reconnected then waited at least 10 s.featuresframe is the first frame of every connection, at about 0 ms after open, in all nine scenarios.Change
shared/protocol.ts: a new server frame,{ type: "features"; features; connection; herdr_lifetime? }.server/core/socket.ts:open()sends it synchronously for a local connection, before awaiting the snapshot. It needs no herdr call. Features depend only on the server's version, andherdr_lifetimecomes from the same socket stat as the snapshot's (server/finishes.ts). A relayed connection (/ws?machine_id=) forwards the remote PC's own frames unchanged.src/lib/ws.ts: the client learns its features from the first trustedfeaturesframe or the snapshot. The guard is unchanged: nothing is sent until the features are known, and after 2 s without them the send is refusedcapabilities_unknown. Watch and chat-interest replays run once per connection. By default nofeaturesframe is trusted (trustFeatures).server/collector.ts,server/core/status.ts: the frame names a herdr only while the server's lifecycle subscription to herdr is open, and only for the herdr whose identity was read before that connection was attempted and still reads the same when it answers. A socket file alone proves nothing: a herdr that crashed can leave its file behind.src/lib/machineSession.ts: trusts afeaturesframe only when itsherdr_lifetimenames the herdr this page already proved from an earlier snapshot of that PC (paneLifetimes.generation). Only then do the connection's pending scope andconnectionid open on it. The herdr-lifetime fence still runs on the snapshot. When the frame names another herdr, or none (a replaced or unproven herdr, or a first page load), the page waits for the snapshot as before, so no send leaves for a pane ID a replaced herdr may have given out again.The common case, a server restart in front of the same herdr, gets the early path. Older servers send no
featuresframe, and their behaviour is exactly as before. Nothing is auto-sent after a disconnect.The design follows a capabilities handshake ahead of the state dump: what a peer supports is told before the costly state, as in LSP's
initializeresponse or the Codex app server's initialize response. It differs in that the early answer is used only for a herdr the page already knows.Tests
Each new test fails on origin/main (or, for the trust rule, on this PR's first commit) and passes here:
src/lib/ws.test.ts:capabilities_unknown, nothing sent);server/core/socket.test.ts: the features frame, with its connection id and herdr lifetime, precedes a held snapshot, and is still sent when the snapshot read fails.server/collector.test.ts: the lifecycle subscription names the herdr that answered it until it closes, names none when the socket was replaced between the attempt and the answer, and none after a stop.server/core/status.test.ts: the named herdr equals the snapshot'sherdr_lifetime, and is named only while that herdr's subscription is open under the same socket.src/lib/machineSession.test.ts:scripts/ui-regression.ts: on a reconnect, holds both frames and releases only the features frame. The message is sent and shows as a live queued row while the snapshot is still held. The full script passes (76 checks).bun run check fast: green on the head commit (workflow syntax, generated types and themes, typecheck, build, unit tests). WS contract suites (submit, terminal-insert, ping, watch, secret, conversation-push) on the isolated herdr: 117 pass, 0 fail. The full ui-regression script also passes on the head commit (76 checks).Review
Adversarial reviews (omo-native-code-reviewer and Codex Sol 6.1 fast xhigh), each on a worktree pinned to the reviewed commit:
Each fix came with a test that fails on the commit before it. CodeRabbit: requested on this PR. If it skips or is rate-limited, the findings and resolutions above stand in and are repeated in a PR comment.
Notes
REMOTE_BUNDLE_VERSIONis not raised here: per docs/development.md the release PR raises it for server and shared changes.pid:startmarker. If a replacement herdr answered on its pipe before rewriting that marker, the frame would name the old herdr for that window, as the snapshot already does on main, since both read the same marker. Once the marker is rewritten, the frame names no herdr until the next resubscribe, and pages wait for the snapshot.server/machines.tsobserve) forwards the new frame to every browser's/api/machines/eventsstream, likerole-ack. There it cancels one roster poll in flight per reconnect, and the roster ignores it otherwise./tmp. With the lane's temp root under~, ui-regression's New-folder case failed: the dialog shows the folder as~/..., while the test expects an absolute path. That code path is not touched here, and the case passes with a/tmproot.