mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 08:02:43 +00:00
6e84424529f2d7262ec3a912a42e20ccce5c19f7
10786
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Neil
|
6e84424529 |
perf(relay): index client request aborts instead of scanning every controller
abortClient runs on every closeClient and every setWrite. Under the flat map
keyed `${clientId}:${requestId}` it had to walk every controller in the relay to
find one client's, so a full churn of N clients each holding K in-flight requests
cost K*N*(N+1)/2 key visits: measured 50 -> 5,100, 100 -> 20,200, 200 -> 80,400,
400 -> 320,800, exactly 4x per doubling.
Do not "optimise" this back to a scan with an early break. It cannot work: the
matching keys are scattered through the map, so any correct loop still visits
every entry before it can know it is done. Only an index makes teardown
proportional to what the client owns.
`create` now returns an opaque handle carrying the owner, so a release finds its
bucket without parsing a composite string key, and no call site changes.
Also stop building the low-water key array eagerly. `belowLowWater` decides on
the aggregate ceiling first and returns without reading the keys, but the caller
had already allocated an N-element array and N template strings to pass them --
paying most in the loaded case, which is when that short-circuit fires. It takes
a thunk now.
The hot-path test becomes a guard rather than a characterisation: it asserts a
teardown visits only the target client's K controllers and never enumerates the
client index at all, since enumerating it is the old scan. Verified by mutation:
restoring the scan shape fails it with "expected 40 to be +0". It asserts the
maps really hold 160 controllers first, so it cannot pass by never filling them.
|
||
|
Neil
|
e3a701822e |
test(persistence): measure the downgrade direction for worktree identity
The stack widens migrateWorktreeIdentity to repoint worktreeId inside session rows. That changes what lands on disk with no wire change, which is Rule 3's shape applied to persistence, so it is measured against v1.4.199 rather than reasoned about. Result: new-build state does not break the old build. The old build renames over it without throwing and loses no row; the two row kinds it cannot repoint stay stale, which is exactly what its own renames already produce. The numbers are measured. A first draft asserted the old build repointed no inner rows at all; it repoints two of four, and the probe is what caught that. |
||
|
Neil
|
c7ba983d68 |
test(relay): measure per-connection teardown and hot-path costs by counting
Both suites replace a would-be duration with the structural fact the duration was a proxy for, so neither depends on machine load. The census pins that attach/publish/detach churn returns every per-connection container to baseline, and asserts the containers actually filled first so a green cannot come from a probe that never loaded them. It also pins the one container with no per-client teardown: a publication-ledger entry is reclaimed only by its own lease, never by closeClient. The operation counts pin that notifyLegacyCapacity costs one ledger lookup per active client, that a broadcast costs a fixed number per subscriber, and that abortClient enumerates every controller rather than the target client's -- which is what makes a full client churn quadratic. |
||
|
Neil
|
527abeb382 |
fix(runtime): bound the remote-runtime connect against an unreachable host
A host that is powered off or firewalled black-holes the TCP SYN, so the remote-runtime WebSocket neither opens nor errors. The Node-side transports set no connect bound, leaving the caller's whole-request timeout as the only one: every `orca <cmd> --environment <unreachable>` sat silent for 60s before failing with a generic `runtime_timeout`. Measured on an unreachable paired host (win-lowspec, SYNs dropped): terminal list / worktree list / repo list / status each took 60.19-60.26s; the same command against a reachable host answered in 0.24s. So this was the shared transport, not one command. Pass `handshakeTimeout` at the three shared remote-runtime WebSocket construction sites, which `ws` applies across TCP connect and the HTTP upgrade. The value matches the bound the browser transport already used. The failure keeps code `remote_runtime_unavailable` so the existing transport-loss classification in terminal-process-inspection still applies, and the message names the endpoint and stops at "unverifiable" — per docs/reference/ssh-execution-boundary.md, loss of contact is never evidence that the host's work stopped. |
||
|
Neil
|
3b71fee483 |
test(wire): pair the session-tabs retirement proof across two builds
The stack makes a host start sending a retirement proof on its own frame when no surface removal carries one. The change argues Rule 1; Rule 3's fourth bullet covers a frame the host starts sending on an existing path, so the claim is measured against v1.4.199 rather than accepted. Neither existing cross-version suite reaches session-tabs: the terminal one covers the binary stream, the agent-session one covers agentSession.*. Result: the old client acts on the proof-only frame, because the whole client half of this surface is unchanged. The old-host cells are pinned to a release that cannot publish the frame at all, which is what makes the new-host cells mean something. |
||
|
Neil
|
ec0a2c19db |
fix(relay): drop the outbox method the lifecycle merge orphaned
The union of adv3-map-composed and relay-lifecycle-fences kept a flushRevokeOutbox that reads this.revokeOutbox/this.flushRevoke, both of which relay-lifecycle-fences moved into the coordinator. Typecheck-only break, invisible on either branch alone. |
||
|
Neil
|
d3006f7466 | Merge remote-tracking branch 'origin/nwparker/paired-close-retirement-proof-publication' into nwparker/j4-upgrade-downgrade-stack-v2 | ||
|
Neil
|
c0431a541d |
Merge remote-tracking branch 'origin/nwparker/adv3-failure-fixes' into nwparker/j4-upgrade-downgrade-stack-v2
# Conflicts: # src/renderer/src/lib/host-mirror-handle-gap-wait.ts # src/renderer/src/runtime/host-session-mirror-hydration.ts |
||
|
Neil
|
fcb19138a1 |
Merge remote-tracking branch 'origin/nwparker/adv3-journey-fixes' into nwparker/j4-upgrade-downgrade-stack-v2
# Conflicts: # src/renderer/src/lib/host-mirror-handle-gap-wait.ts |
||
|
Neil
|
c64d43b31d | Merge remote-tracking branch 'origin/nwparker/adv2-persistence-fixes' into nwparker/j4-upgrade-downgrade-stack-v2 | ||
|
Neil
|
610629b98c | Merge remote-tracking branch 'origin/nwparker/adv2-crossplat-fixes' into nwparker/j4-upgrade-downgrade-stack-v2 | ||
|
Neil
|
5fbad9039f |
Merge remote-tracking branch 'origin/nwparker/relay-lifecycle-fences' into nwparker/j4-upgrade-downgrade-stack-v2
# Conflicts: # src/main/runtime/relay/desktop-relay-service.ts |
||
|
Neil
|
ed38308333 |
test(runtime): pin the removal frame retiring a still-live publisher
KNOWN RED (`it.fails`), no product change. Found while verifying the close retraction fix: once the emptying actually reaches paired clients — a state the previous behaviour never allowed, because nothing propagated — re-adoption of a later create is flaky. Measured 1 failure in 6 runs of the two-client journey. `decideWebSessionTabsSnapshot` treats the host's synthetic `removed:<t>` retraction as a publisher handover: it retires the still-live renderer epoch and installs the retraction as current, while the removal also clears the live freshness record. The next frame from that same running publisher then matches no lineage and reads as a retired generation, so it is outranked and the publisher is locked out of the worktree until its generation changes. `local-structured-session-tabs-sync/snapshot-apply.ts` documents this exact scenario and has a revive escape; the mirror path has none. The suffix case explains the 1-in-6: `hasRetiredValue` is an exact string match, so a republication carrying `:headless-merge:` walks past the fence and only a bare same-epoch republication is locked out. Not fixed here on purpose. Dropping the retirement makes the red case pass but breaks `web-session-tabs-sync.test.ts > keeps a removed worktree fenced against delayed predecessor epochs`, which asserts a same-epoch higher-version frame after a removal must be rejected. At this layer those are the same frame — this function holds no `receivedFrame`, so it cannot separate a delayed predecessor from the live publisher speaking again. The fix belongs in `shouldApplyRecoveredWebSessionTabsSnapshot`, which does hold that ordering and currently defers to the same epoch fence. That is a contract change across two functions and an existing invariant, not a one-liner. |
||
|
Neil
|
982d539df8 |
fix(runtime): publish a terminal retirement proof on the exit's own evidence
A paired client may drop a mirrored terminal on exactly two kinds of host evidence: a `retiredTerminalSurfaces` proof naming the handle, or two authoritative `terminal.list` inventories that omit it. The second needs two host publications, and a quiet workspace publishes one, so the proof is the only evidence that rides the frame carrying the retraction. That proof was minted only as a byproduct of persistence *accepting a change*, which made one value carry two meanings: "a change was accepted" and "the PTY exited". The host renderer's close transaction de-persists the surface and republishes without it, so when it got there first the exit found nothing left to accept and the attestation died with it. Measured on a real paired client: the host retracted in under 500ms, published no proof, then froze its snapshotVersion for 60s while the client kept a dead pane in its tab bar. Persistence still gates *removal* — publishing absence before the membership fence is durable would let a crash resurrect the surface. It no longer gates the proof: the observed exit is itself the attestation. The exit-first ordering already had a passing test; the renderer-first ordering had none, and that is the one users hit. Both orderings are now pinned, with exit-first as the control that makes the renderer-first failures mean something. Wire: `retiredTerminalSurfaces` is an existing optional field on an existing path, already negotiated as `session-tabs.retirement-proof-delta.v1`. This is Rule 1 — an old client that ignores it degrades to the two-inventory route it already uses today, so no capability gate is needed. The sentence "the host starts sending a frame it did not send before" reads like Rule 3; it is not, because the frame shape, the field, and the reader contract are all unchanged. |
||
|
Neil
|
d4f7700062 |
docs(relay): address the teardown ratchet's docstring to whoever it goes red on
The reader who decides this test's fate is not the reviewer — it is someone hitting a red ratchet during an unrelated refactor, weighing whether to relax the pattern. Say the trade to them directly: a ratchet that fails loudly on a benign change beats one that passes silently on a harmful one, and the literal match is deliberate because a refactor that obscures which teardown runs at quit is exactly what should get a second look. |
||
|
Neil
|
762e1faa80 |
test(relay): ratchet which teardown each lifecycle event calls
Closes the gap the cross-reference comments could not: a reader arriving at stop() or fenceAndCloseNow() now finds the other, but nothing failed if someone edited main-process-quit.ts back to the fence, and that is the edit that reintroduces the defect. No harness needed — this reads the source text, like the other ratchets in src/main. Both methods take an optional argument and return void, so swapping them compiles clean and passes every unit test; the difference is that stop() is terminal and the fence is re-armable, and it only surfaces in timer state minutes later. This is the compile error that swap cannot produce. Both directions are asserted, because collapsing them either way destroys the distinction the defect came from: quit must not regain the re-armable fence, and sign-out/relaunch must not lose it. The regexes are qualified by receiver on purpose. The quit handler stops five unrelated services, so a bare `.stop()` would match rateLimits, starNag and the rest. Verified by mutation, both directions and a control: reverting quit to fenceAndCloseNow fails the first test only, collapsing sign-out onto stop() fails the second only, and renaming an unrelated service's stop() fires neither. |
||
|
Neil
|
ae4c130422 |
docs(relay): cross-reference the terminal stop() and the re-armable fence
main-process-quit.ts has no test harness — single export, ~20 heavy imports including electron — so the quit call site's choice between these two is pinned only by the service-level contract. The risk is not that the contract is untested; it is that someone editing the quit handler has no way to find it. Each method now names the other, says which lifecycle events call it, and says what the other one is for. Grep from either end lands on the counterpart, which is the only protection available when one side cannot be tested. |
||
|
Neil
|
9a8cc8c96e |
docs(runtime): the reused-tab-id class is closed at read time, not still open
The `ExpiredHandleGapVerdict` docstring told the next reader that a retracted tab id republished under the same id still inherits its predecessor's verdict, and that closing it "needs a fourth trigger, on row retraction". The test it names as its own pin says the opposite: class D in host-mirror-handle-gap-verdict-union.test.ts asserts the verdict does not answer, and explains it is closed at READ time rather than by any prune. Provenance, since two sources disagreeing is what made this expensive: the paragraph was last written in |
||
|
Neil
|
83369d0ffc |
test(relay): pin RelayControlClient teardown before, and twice after, connect
Characterisation only — no defect found, which is the result worth recording, because the same audit found the service-level fence was not hard. Covers closeNow() at 'idle' before any socket exists, connect() refused after teardown, a control request issued before connect, and closeNow() twice from both 'opening' and 'active'. Asserts the specific fields rather than absence of errors: socket handle dropped so the second pass cannot re-terminate, onClose not re-fired, pendingRequestCount back to 0, and vi.getTimerCount() 0 so neither the connect deadline nor the silence watchdog outlives the close. Mutation-verified: dropping the idle guard in connect(), `this.socket = null`, requests.rejectAll, or silenceWatchdog.stop() each fails one of these. |
||
|
Neil
|
ed35db892b |
fix(relay): stop a mid-read profile switch reporting as a sign-out
Same taxonomy error |
||
|
Neil
|
f1eb8037c8 |
fix(relay): drain the revoke outbox on one broker, not one open per item
flushAll runs unawaited from inside the coordinator's openBroker, so the broker it is draining is not registered yet. Refreshing demand after every removal therefore reconciled against a null ownership: each reconcile opened a fresh director assignment and abandoned the broker still working through the loop. Measured with three queued revocations and a real RelayRevokeOutbox on disk: 3 items produced 3 RelaySessionBroker.connect calls and 2 abandoned brokers, against a relay that rate-limits assignment. After: 1 connect, 3 revokes, one `connecting` transition, the draining broker never closed, outbox empty. Reconcile once after the loop instead, and only when something was actually removed. flushItem keeps its own refresh because onDeviceRevokeQueued calls it directly on an already-registered broker, where retiring the last demand is exactly what should reach standby. Tests use the durable file as the assertion rather than a mock, and cover the fence landing before the open resolves, the fence landing between two items, the multi-item drain, the last-demand revoke reaching standby, and the same reqId queued twice (dedup is the control layer's job — duplicate_relay_request_id — so the service must only settle both without stranding the item). Mutation note: moving onDrained one await later, inside the loop after flushItem returns, survives. The extra microtask lets the coordinator register the broker first, so no extra open occurs and the variant is genuinely inert here; the assertion does catch the real pre-fix placement, synchronous inside the revoke after remove. |
||
|
Neil
|
0671e9eca8 |
fix(runtime): a published handle retires the verdict it answered
The fourth eviction trigger on `expiredGenerationByPane`, and the reason it is not redundant with the three already there or with the two other agents' guards on this same map. A verdict records that a pane's 15s handle-gap wait ran out. Nothing retires it when that pane subsequently publishes its handle, so the NEXT gap on that pane gets no wait at all — #19735 with the bounded wait removed rather than merely shortened. Measured on the reconciled union tree ( |
||
|
Neil
|
0fa72f336a |
fix(runtime): an outage is not a handle-gap verdict
The per-pane handle-gap wait releases at a 15s deadline and records that
expiry as a verdict, which authorises the sleeping-agent resume. The
connection generation was the only thing voiding that verdict, and a plain
disconnect never advances it — runtime-status.ts advances on the reconnect,
under a new runtime id. So a network drop mid-turn expired the wait with a
generation that still matched, and the replay forked a second `--resume`
onto the transcript the host was still writing: #19735 through the
disconnect door.
Suppress the verdict while the client positively knows it is out of contact,
reusing the shared runtime-host connection derivation. The waiter still
releases and re-parks, so contact returning gets a full fresh budget and the
pane is still decided on real silence.
Not redundant with the landed-handle drain that follows this commit, nor with
the read-time pane identity from adv2-skew (
|
||
|
Neil
|
4733b21cdc |
fix(relay): make the quit fence hard, and route quit to the terminal stop()
DesktopRelayService.stop() has zero production callers. Quit, relaunch and
pre-sign-out all go through fenceAndCloseNow() (main-process-quit.ts,
main-window-core-services.ts). That matters because the two are not
equivalent: stop() sets `stopped` and clears both timers, while the fence
cleared only `livenessTimer`, left `demandExpiryTimer` armed, and left
`stopped` false. Its comment — "the next auth mutation re-arms via
refreshDemand" — is right for sign-out and exactly wrong for quit. The path
production takes was the one that permitted a resurrection, and the suite was
exercising the other one, which is why this survived.
Three doors re-armed the fence, each measured against a fresh service:
- a pending invite-expiry wake: getTimerCount() is 1 immediately after the
fence, and the timer fires refreshDemand and reopens the broker
- a mint settling after the fence, through withTransientDemand's finally:
0 timers at the fence, 1 once the mint settles, and connect runs again
- a power-resume ensureLive(), gated only on `stopped`: connect called twice,
a new relay control session opened after quit
Two intents, kept apart rather than unified. fenceAndCloseNow gains a `fenced`
latch cleared only by start()/authMutated(), so sign-out and relaunch keep the
re-arm the old comment protects while the dangerous window between the
pre-sign-out fence and the profile wipe stays shut. Quit calls stop() instead,
which is terminal; the wire behaviour is unchanged, since coordinator.stop()
fences reasonlessly exactly as before. The outbox flush gets the same latch,
because it runs unawaited from inside the broker open and a fence can land
between two revokes.
Deliberate on a vetoed quit: Relay stays down for the session. before-quit
already nulls the mobile pairing provider unconditionally, so pairing is dead
there regardless — a re-armed broker would be a zombie holding a control
session nothing can use.
Not fixed here: `fenced` is not set by stop(), so the two latches stay
independent and a future caller can still pick the wrong one.
|
||
|
Neil
|
cd175054c5 |
refactor(relay): extract the revoke-outbox flush into its own module
Behaviour-preserving move, no logic change: flushRevokeOutbox and flushRevoke leave DesktopRelayService as RelayRevokeOutboxFlusher.flushAll/flushItem. The two fixes that follow each add lines to this file, and it has no headroom under the 300-line cap; a max-lines disable is not an option. Why a new file rather than reusing something: nothing existing drains the outbox. RelayRevokeOutbox is the durable store — making it drain itself over a control would give the persistence layer a dependency on a broker. RelayControlRequests dedups in-flight reqIds on a single socket, not queued items across reconnects. RelayDemandLedger reads the outbox for demand and never mutates it. The drain only ever existed inline here, so this is an extraction, not a parallel implementation; the only new code is the options type and the class shell. |
||
|
Neil
|
1b621b6b13 |
docs(runtime): the two guards on the park-time binding are not redundant
Recording a reconciliation result that existed only in a review thread, and
correcting it in the process — measuring the claim changed it.
The claim under review was that the `?? ''` fallback in `recordExpiredWait` is
unreachable by two independent guards, either sufficient alone: the caller's
generation gate (a missing waiter fails `undefined === number`) and the
record-before-release ordering. That is not what the code does.
Measured, by removing each in turn:
- ordering removed, generation gate kept: the gate does NOT carry it. With the
waiter already deleted, the gate is false on every expiry, so nothing is ever
recorded — five failures, and the door is shut by breaking the mechanism rather
than by refusing ''.
- generation gate removed, ordering kept: 736 files green, one failure, and it is
`does not let a wait armed on the previous connection decide the new one` in
host-mirror-handle-gap-resume.test.ts — a different property entirely.
So the ordering alone makes `''` unreachable, and the generation gate is not a second
guard on it at all: it pins reconnect-void. Both are load-bearing, for different
reasons, which is a stronger argument against removing either than redundancy would
have been — redundancy invites deleting one.
Worth writing in the file because the two sit three lines apart and read as belt and
braces on the same thing. The `''` comment next to them already exists because an
unexplained guard on an unreachable value gets deleted as dead code in a year; a guard
that looks redundant is deleted sooner.
No behaviour change. One comment, corrected against measurement rather than against the
thread it came from.
|
||
|
Neil
|
fc794784cd | test(e2e): pin the close retraction a paired host does not publish | ||
|
Neil
|
a5dfddf6fa |
test(runtime): supply the host home the SSH orphan-cleanup gate now requires
The scenario registers an SSH filesystem provider directly. In production that provider is minted by the relay session that also reports the host's `$HOME`, so the test has to supply the other half rather than rely on the recursive-delete gate falling open on an unknown home. |
||
|
Neil
|
a3512fcd69 |
fix(runtime): stop the epoch-history cap evicting a fence that can still be beaten
The LRU cap added in
|
||
|
Neil
|
cdafc90d8f |
fix(runtime): a handle-gap verdict answers for its pane, not for the tab id
Folds adv2-skew's
|
||
|
Neil
|
20fb0b3f6e |
perf(runtime): index session-tabs tracking keys by environment, and bound the epoch history
Two defects that only appear at scale or over time, in one commit because the fix for the second calls into the structure the first introduces — see the coupling note at the end. 1. THE PER-ENVIRONMENT TEARDOWN SWEEP WAS QUADRATIC IN ENVIRONMENT COUNT. `clearWebSessionTabsTrackingForEnvironment` prefix-scanned every key of eight module maps, each holding E*W entries, plus one global walk — so a catalog update clearing E environments cost 8*E^2*W + E*W. It runs on every pairing-revision change. Measured with a counting probe on the real function (exact key visits, no clock, so a loaded machine cannot contaminate it): E=10 W=50 500/map 17,000 visits -> 500 (34x) E=20 W=50 1000/map 58,740 visits -> 1,000 (59x) E=40 W=50 2000/map 202,220 visits -> 2,000 (101x) E=50 W=20 1000/map 141,600 visits -> 1,000 (142x) Doubling E at fixed W: 17,000 -> 58,740 -> 202,220, i.e. 3.45x and 3.44x per doubling — quadratic, not linear-but-large. The indexed column over the same sweep is exactly 2x per doubling. Doubling W at fixed E is 1.84x and 1.78x, linear as expected. These are freshly-populated fixtures with nothing stale in them, so the quadratic term is a property of the scan itself and not of a leak. Wall clock for cross-reference only: 1.92ms -> 0.64ms at 20x50, 37.1ms -> 4.8ms at 100x50. The fix is the index shape this file already uses for `hostSessionTabMappingKeysByEnvironmentAndWorktree` — that precedent is why this is worth indexing rather than tolerating. The regression assertion is STRUCTURAL, not a timing threshold. A wall-clock ratio bound was tried first and flaked at 22.3x against a <20x limit under suite load, which is exactly the failure mode a timing assertion has on this machine. It now spies on `.keys()` for the nine per-worktree maps and asserts teardown never enumerates any of them — enumerating them IS the sweep, so it pins the property directly and is load-independent. Mutation-checked: restoring the prefix sweep kills it. 2. `sessionTabsPublicationEpochHistoryByWorktree` WAS UNBOUNDED IN KEY COUNT. Only its inner retired array was capped (at 8); the map itself grew 1:1 with worktree lifecycles, since the entry is deliberately retained as a tombstone fence after the live record is dropped. 10,000 create/remove cycles left 10,000 entries. Now LRU-capped at 512, re-inserting on every accepted frame so a still-publishing worktree is never the eviction victim — the tombstones, which are what actually accumulate, are evicted first. WHY ONE COMMIT: the eviction added by (2) calls `releaseSessionTabsEnvironmentKeyedWorktree`, the index added by (1), because an evicted tombstone must also release its index entry. Split, the first commit would leak the index on eviction or the second would not build. The coupling is real rather than incidental, so they land together. Verification: 191 files / 1611 tests pass, log grepped for ELIFECYCLE and failures = 0. pnpm tc and oxlint clean. |
||
|
Neil
|
456ed8a98b | test(e2e): keep the two-client journey spec type-clean | ||
|
Neil
|
6594dccf4d |
test(e2e): journeys for a reopened client and two clients on one host
Two gaps this suite had no coverage for, both driven end to end against a real paired desktop client rather than at a seam. A relaunched client holding a live remote terminal: every paired restart spec here restarts around a browser pane, none around the terminal the user is actually mid-work in. The host-side fixture's on-disk sink is the oracle — one READY for the whole run proves the host never re-spawned the session, and a recorded line for input sent after the relaunch proves the restored pane is wired to that same process rather than painted with its scrollback. Two clients on one host across an emptied workspace: the tombstone is client-local on the runtime path, so a client that never held a row still seeds into a workspace another client deliberately emptied. That asymmetry is by design; a client falling out of step with the host and staying there is not. Phase 0 is the control — without it a later divergence cannot be attributed to the emptying rather than to mirroring never having worked. The input probe goes through `pane.terminal.input`, not `window.api.pty.write`: a mirrored pane's handle is a `remote:` id that no local PTY answers to, so a direct write is swallowed and the assertion passes on nothing. The pre-restart control exists to catch exactly that, and did. |
||
|
Neil
|
46ad377ceb |
fix(test): repair two suites the union broke, neither visible on its own branch
Both were green on their own branch and failed only once the branches were combined. This is the interaction class the union exists to find. 1. relay-concurrency-policy-flip-mid-mint.test.ts stubbed `getRelayRevokeOutbox` with `pendingFor` alone. `hasDemand` now reads `demandingFor` (the revoke-demand expiry), so the stub threw INSIDE reconcile, the mint never settled, and the only symptom was a 30s test timeout with the real cause buried in stderr. A stub that implements part of an interface fails this way whenever the production caller moves to another method. 2. host-mirror-handle-gap-teardown.test.ts REPLACED `tabsByWorktree` on every park, so each pane unpublished the one before it. With the tab-death rule in the same loop those earlier verdicts were legitimately swept before teardown was reached, and the suite counted 2 where it expected 3. The fixture, not the rule, was wrong: this suite is about the class no recording-driven prune can reach, so its panes must all stay published. Rows now accumulate. |
||
|
Neil
|
794089794a |
fix(relay): keep the diagnostic the refusal path exists to produce
Two error paths that destroy their own evidence.
`parseHandshakeMessage`'s unknown-type refusal interpolated `String(t)` on a
peer-supplied value: `{"type":{"toString":1}}` makes String() throw "Cannot
convert object to primitive value", so the refusal arrives without naming what
was refused. `describeRelayProtocolVersion` guards this exact hazard two files
away; the sibling was missed.
`runRelayOrcaCliChannel`'s new `onDecodeError` wrote to stderr and then exited
synchronously. stderr is async on a pipe transport, so the one line recording
why the command died could be dropped — the reason relay-handshake.ts already
exits inside its write callback.
|
||
|
Neil
|
2051f16008 |
fix(mobile): a relay status we could not read is not "offline"
Two problems in the same newly-extracted collector, both in the artifact a user pastes into a bug report. `.catch(() => 'offline' as const)` reported the definite neighbour for a lookup that observed nothing, so a support engineer could not tell a host that reported offline from one that never answered. It now reports `unreadable`. And the collector could reject: `window.api.mobile.getRelayStatus()` throws synchronously when the bridge has no `mobile` — before `.catch` is attached — while both call sites fire this as `void copyRelayDiagnostics()` with the await sitting AHEAD of their try/catch. Measured: the click wrote no clipboard and showed no toast at all, where the pre-extraction inline payload build could not fail. The collector is total now, and the await moved inside the try so a future addition cannot silently kill the toast again. |
||
|
Neil
|
25096b39d8 |
fix(relay): the demand wake signal must not fail the grant it wakes for
`withTransientDemand` called `refreshDemand()` bare on both sides of the operation. It reaches the device registry (`nextPendingExpiry` -> `listDevices`) and the settings store (`hasDemand` -> `isRelayAllowedForDevice`), so it can fail on its own — and measured, each call site failed the operation instead: - the pre-call threw with the transient ref already acquired, so the ref was never released and the operation never ran. Transient refs have no expiry, so that ref holds relay demand for the rest of the process. - the teardown call replaced the operation's own result in both directions: a named mint failure arrived as `listDevices exploded`, and a SUCCESSFUL mint arrived as a rejection. Both are wake signals; the liveness tick and the next refresh re-ask, so a lost signal is recoverable where a stranded ref is not. The containment lives beside the ledger that owns the ref, because desktop-relay-service.ts is at its max-lines ceiling and this must not cost it a line. Also carries the original error as the `cause` of the mid-operation policy flip rewrite, which was discarding it — the one place in a commit about naming causes that destroyed one. |
||
|
Neil
|
94ba0b9f05 |
docs(relay): say plainly that the protocol negotiation is not reachable yet
Two claims that read as live and are not. `relay-protocol-version.ts` documents a negotiation that no live path can reach, and reads as the fix for #13852. It is not. The deploy path namespaces the relay directory by the CLIENT'S OWN build hash — `remoteInstallDirName` is `relay-<fullVersion>` — and the daemon socket lives inside it; the short-socket fallback derives its segment from the same directory, and every `runConnectHandshake` caller takes its path from that one deploy result. So a bridge can only ever meet a daemon of its own build, `msg.version === launchVersion` short-circuits, and `relayProtocolOfferAdmits` never decides anything. The stranded incumbent the feature exists for sits in `relay-<otherVersion>/`, which nothing dials. What ships is three log lines; `MIN_RELAY_PROTOCOL_VERSION` and the `minProtocolVersion` wire field have no live reader. Keeping it is right — the mechanism is correct and hostile-input-safe (20 malformed and hostile offers refused against a real daemon, which stayed up and still admitted a valid cross-build offer afterwards), and it is the part that has to exist first. What was missing is the statement of what else has to land with it, which is now written where someone editing this file will see it: route the bridge to the incumbent's socket, and stop `validateGrant` refusing on `serverBuildId`. That second one matters because its premise, "client and relay ship in one build", is still TRUE today and becomes false the instant the first lands — it is a second gate that would refuse what the handshake just admitted, and shipping only the routing change would look like a regression in the negotiation rather than a missed dependency. `lingerMs` on the coordinator contract reads as a policy knob. No production path sets it: the only `new RelayAuthCoordinator` is in `desktop-relay-service.ts` and passes neither it nor `random`, so every shipped linger is the hardcoded ten-minute default. Labelled a test seam so the next person tunes the default, which is the only thing a user can feel. No behaviour change. Both found by a dead-code and unread-field sweep over the stack. |
||
|
Neil
|
9ff0af8b43 |
test(session): make three data-loss test names match what their bodies check
Each of these named a guarantee it did not check. A test called "never replaces the host snapshot with an empty list" buys more confidence than it earned, which is worse than no test on a path where the failure is deletion. `ssh-host-partition-session-export` — WEAK, and now strengthened. The assertion was `not.toEqual([])`, which `undefined` satisfies. Under `replace-session` the exported projection is authoritative in whole, so an omitted path key destroys the host's copy exactly as an empty list does; the test named both failures and caught one. Measured A/B: with the export mutated to never write the path key, the old assertion PASSES and the new one (`Object.hasOwn` plus the exact id list) FAILS. `remote-workspace-session-merge-local-survival` — INERT, renamed. It was called "drops a tab closed locally rather than resurrecting it from the snapshot", but the tab is not dropped — the host still lists it, the body's own comment concedes it, and the only assertion is that every merged id came from one of the two inputs. Disabling `isSuppressedByClose` entirely leaves every test in the file green; the mechanism is covered by `remote-workspace-session-merge-close-tombstones.test.ts`. Renamed to the real invariant, which is worth keeping, with a pointer to where the close behaviour is actually pinned. No new assertion, because duplicating that file would be the second mistake. `workspace-session-ssh-partition-round-trip` — WEAK, renamed, and this one was actively misleading. "does not adopt a stale populated ssh row over a tombstone in the owning partition" describes a guarantee the code deliberately does NOT provide: the populated row would sit in the legacy `local` partition, and there `workspacesTheBaseOwns` (keyed on `tabs.length > 0`) makes the workspace un-adoptable, so the legacy tabs win and the tombstone is dropped. That trade is deliberate — a resurrected tab is recoverable, a deleted one is not — and is pinned in workspace-session-stranded-partition-adoption-tab- rows.test.ts. No fixture under the old name could have caught the case it named, so the name was a standing promise against the exact data loss the trade accepts. Found by a test-name-versus-body sweep; the first two verdicts were each confirmed by a mutation that survived, with a control mutation that did not. |
||
|
Neil
|
0b1b0eec36 |
fix(runtime): key the wake-respawn latch per environment, like its twin
The wake-respawn latch is the initial-terminal bootstrap latch's twin — both stop one focus from issuing two creates for a workspace, both are consulted from the same subscription closure, and they sit one line apart in the same teardown. The bootstrap latch got two fixes this stack; this one got neither, and still carried both defects: 1. NOT KEYED BY ENVIRONMENT. It was a bare `Set<worktreeId>`. A worktree id is `repoId::path` with no host component, so the same id can be live on two paired runtimes at once (STA-4343) — which is exactly why the bootstrap latch was re-keyed. Keyed by worktree alone, one runtime's respawn suppressed the other runtime's, and one runtime's `end` released the other's claim. 2. A CLEAR-EVERYTHING INSIDE A PER-ENVIRONMENT TEARDOWN. `clearWebSessionTabsTrackingForEnvironment` called `clearAllWebRuntimeWakeTerminalRespawn()`, so tearing one environment down freed every other environment's in-flight claim and a fresh closure for the sibling could issue a second respawn while the first create was still running. That is STA-6173, and the bootstrap latch's own docstring names it: "clearing every environment's latch would release a sibling environment's pending create and let a new subscription for it seed a duplicate, which is this very bug through another door." The correctly-scoped bootstrap clear is the very next line. Found by reading the teardown function as a list rather than as prose — a clear-everything is a one-line call that looks identical to a scoped one at a glance, which is how it sat next to the fix for its own defect. Both latches now have the same shape: `Map<environmentId, Set<worktreeId>>`, a per-worktree release, and a per-environment clear that touches only its own keys. The existing wake-respawn test is threaded with an environment id rather than rewritten: its contract was correct, only the signature moved. The new file pins the two defects themselves. Also documents `clearWebSessionTabsTrackingForEnvironment` as what it has become — the per-environment teardown registry. It now carries seven module clears, and a new per-environment map belongs in that list rather than behind a trigger of its own. The doc says each clear must be scoped to THIS environment and names the wake-respawn latch as the one that was not, because the next clear-everything will look just as harmless. Mutations: reverting the per-environment clear to a clear-all kills exactly the sibling-claim assertion; making the skip check ignore the environment (the old worktree-only keying) kills exactly the two-environments assertion and the sibling-claim one. No survivors. |
||
|
Neil
|
416b35732e |
fix(runtime): reconcile three branches' handle-gap verdict rules into one loop
Three agents changed `recordExpiredWait` on three branches and each verified only their own. This is the union, resolved into the agreed shape and proved on one tree. The rules are NOT alternatives — they have different safety properties, and flattening them to one scope is wrong in both directions. Both wrong shapes were independently written before this was reconciled, so the comments say why. GENERATION rule, per key across EVERY environment (adv2-skew's class). `hasHostMirrorHandleWaitExpired` compares a row against its own environment's CURRENT generation, so a row whose generation has moved can never return true for anybody; retiring it cannot cost a reader a verdict, whoever owns it. Scoped to the recording environment, an environment that reconnects and then goes quiet strands its rows forever. TAB-DEATH rule, recording environment ONLY (my class). Row absence is transient where a generation is not: a sibling mid-republish has no rows for a frame and would lose a verdict its pane still needs — reproduced before it was narrowed. Teardown drain (adv2-races' class) is unchanged and orthogonal: it is the only trigger that fires for a REMOVED environment, whose rows no rule above reaches because such an environment records no further verdict. Right predicate, wrong trigger. The union suite proves all four orphan classes simultaneously, plus the two properties none of the three rules may break: the verdict stays sticky enough to break the park/expire/replay loop, and no rule evicts a verdict a live pane still needs. It uses three environments throughout, because with two at one generation the candidate rules are indistinguishable and the naive fix survives. THE FOURTH CLASS IS UNOWNED AND ASSERTED AS A HAZARD. A retracted tab id that is republished inherits the old pane's verdict and skips its own wait. Unlike every other gap on this map it is NOT conservative: the others drop a verdict and re-park, holding longer, while this one retains a verdict and resumes on a handle that has not landed — the #19735 direction. No rule reaches it: the tab-death predicate stops matching once the id is republished, the teardown drain fires on environment teardown rather than tab retraction, and no waiter exists to observe the retraction because a pane holding a verdict never parks. Closing it needs a fourth trigger, on row retraction. The suite pins the current behaviour so it cannot be quietly forgotten. Union finding, recorded rather than merged: adv2-skew's `docs(relay): the live-broker wait budget does not bound the call` ( |
||
|
Neil
|
1bb7489a92 |
fix(relay): a throwing retry step must not kill the retry chain
`RelayRetrySchedule` ran the caller's recovery step bare inside its timer and
resolved `armed` only on the line after it. Injected a synchronous throw:
the error escaped the timer as an uncaughtException, `armed.resolve()` never
ran, and the schedule was left with no armed timer — so every waiter on
`settled` parked on a promise nothing would ever settle and nothing re-entered
the chain.
The reachable trigger is the origin pool's drain recovery, which calls
`onStatus('draining')` straight through to `webContents.send`. `state.mainWindow`
is nulled only on `'closed'`, so between destroy and that event the send throws
`Object has been destroyed` — every other `webContents.send` under
`src/main/startup/` already guards with `isDestroyed()`. Guarded here too, so
the trigger is removed as well as contained.
|
||
|
Neil
|
dd5f4376f7 |
test(runtime): carry the bootstrap-latch retention suite onto the union
Teardown semantics here are adv2-concurrency-fixes': an in-flight create survives teardown as `creating-after-teardown`. The suite seeds PARKED claims rather than in-flight ones so it pins the unambiguous half and passes against both shapes. Accessors added to their module rather than a second spelling of them. |
||
|
Neil
|
cbf18d46d4 |
fix(runtime): key the wake-respawn latch per environment, like its twin
The wake-respawn latch is the initial-terminal bootstrap latch's twin — both stop one focus from issuing two creates for a workspace, both are consulted from the same subscription closure, and they sit one line apart in the same teardown. The bootstrap latch got two fixes this stack; this one got neither, and still carried both defects: 1. NOT KEYED BY ENVIRONMENT. It was a bare `Set<worktreeId>`. A worktree id is `repoId::path` with no host component, so the same id can be live on two paired runtimes at once (STA-4343) — which is exactly why the bootstrap latch was re-keyed. Keyed by worktree alone, one runtime's respawn suppressed the other runtime's, and one runtime's `end` released the other's claim. 2. A CLEAR-EVERYTHING INSIDE A PER-ENVIRONMENT TEARDOWN. `clearWebSessionTabsTrackingForEnvironment` called `clearAllWebRuntimeWakeTerminalRespawn()`, so tearing one environment down freed every other environment's in-flight claim and a fresh closure for the sibling could issue a second respawn while the first create was still running. That is STA-6173, and the bootstrap latch's own docstring names it: "clearing every environment's latch would release a sibling environment's pending create and let a new subscription for it seed a duplicate, which is this very bug through another door." The correctly-scoped bootstrap clear is the very next line. Found by reading the teardown function as a list rather than as prose — a clear-everything is a one-line call that looks identical to a scoped one at a glance, which is how it sat next to the fix for its own defect. Both latches now have the same shape: `Map<environmentId, Set<worktreeId>>`, a per-worktree release, and a per-environment clear that touches only its own keys. The existing wake-respawn test is threaded with an environment id rather than rewritten: its contract was correct, only the signature moved. The new file pins the two defects themselves. Also documents `clearWebSessionTabsTrackingForEnvironment` as what it has become — the per-environment teardown registry. It now carries seven module clears, and a new per-environment map belongs in that list rather than behind a trigger of its own. The doc says each clear must be scoped to THIS environment and names the wake-respawn latch as the one that was not, because the next clear-everything will look just as harmless. Mutations: reverting the per-environment clear to a clear-all kills exactly the sibling-claim assertion; making the skip check ignore the environment (the old worktree-only keying) kills exactly the two-environments assertion and the sibling-claim one. No survivors. |
||
|
Neil
|
ae77dea3f3 |
fix(relay): stop a revoke that can never land from pinning the relay online
Reproduced first: enqueue a revoke, reject it 50 times the way a deleted device
is rejected, and `hasDemand` is still true. It stays true for the life of the
install.
Two deliberate properties combine into the bug. `flushRevoke` removes the outbox
item only on a SUCCESSFUL flush and swallows every failure in a bare `catch {}`
with no attempt cap and no expiry, so a permanently-rejected revoke is pending
forever. And the revoke arm of `hasDemand` is deliberately NOT filtered by the
host's pairing-connection policy, because a credential must be killed on the relay
whatever the user has since chosen. Forever-pending plus policy-exempt means the
relay is held online permanently and a `local-only` pick is defeated outright —
the regression path for what #19870 closes, previously with no test at all.
THE CHOICE, stated because the convenient one is wrong. The revoke is NOT
abandoned. Failing to revoke leaves a live credential on the relay, so discarding
the intent to tidy up demand would trade a visible stuck relay for an invisible
live credential. The item stays in the outbox and keeps retrying on every future
connection, unchanged.
What expires is only its CLAIM ON DEMAND, via `demandingFor` alongside the
unchanged `pendingFor`: after 24h an unlanded revoke stops being a reason to force
the relay online BY ITSELF. The reasoning is that holding the relay open is not
what makes the revoke succeed — it has had a day of attempts — so holding it open
buys nothing and costs the user the setting they chose. A revoke that can still
land is unaffected inside the window, and one that lands is removed as before.
Uses the existing `createdAt`, so nothing new is persisted and the window survives
restart.
Mutation-tested: putting `pendingFor` back in `hasDemand` fails the new test at
the post-window assertion. The success path is pinned separately so the fix cannot
be mistaken for aging out an item that should have been removed.
Audited the rest of the file for the same shape, since a bare `catch {}` on a
queue is rarely alone: `flushRevoke` is the only one in desktop-relay-service.ts.
|
||
|
Neil
|
6bd997b2cc |
fix(relay): clear the demand-expiry timeout on a fence, not only in stop()
Why the existing lifecycle test missed this: it stubs
`demandLedger: { nextPendingExpiry: () => null }`, so the second timer this class
owns is never armed and the fence is only ever asserted against the liveness
interval. Change that one stub to a future expiry and the test fails.
`fenceAndCloseNow` cleared `livenessTimer` and left `demandExpiryTimer` armed.
`stop()` cleared both — but `stop()` has no production caller, while all three
fence sites (quit, sign-out, SIGNED_OUT) go through the path that cleared only one.
The consequence is the sharp part. The orphaned timeout fires `refreshDemand`,
which reconciles *and* re-installs the liveness interval the fence just tore down.
A fence during an outstanding pairing invite therefore undid itself minutes later
— the post-fence resurrection the comment two lines above forbids in so many words.
Both teardown paths now share one `disarmTimers()`. That also resolves the
`max-lines` pressure the extra clause created: the two teardown blocks were byte
identical, so deduplicating them is the right fix rather than a `max-lines`
disable, which this repo forbids and which the next person under that cap will be
tempted by.
The new test asserts `vi.getTimerCount() === 0` after the fence rather than
inferring from an absence of errors, and pins that a fence is not a stop — the
next auth mutation still re-arms both timers.
Pre-existing, not introduced by this stack; verified identical at the base SHA.
Mutation-tested: removing the `demandExpiryTimer` clause from `disarmTimers`
fails the new test at the timer-count assertion.
Also carries a note at `waitForLiveBrokerResult`: the `pending !== latestReconcile`
arm re-enters the loop without re-checking the deadline, so under continuous
reconcile churn the documented 20s budget is not strictly enforced and the
caller's transient demand ref is held past it. Not a hang — every iteration awaits
a settling promise — so it is bounded by how long churn lasts.
|
||
|
Neil
|
1095e360c0 |
fix(runtime): release the bootstrap latch when the post-create read throws
The `tabsByWorktree` row read that decides between release and park sat outside the try. Measured with `useAppStore.getState` throwing once after a successful create: the dispatch rejects with the latch still in `creating`, and `releaseWebRuntimeInitialTerminalBootstrapOnMirrorFrame` only ever clears `awaiting-mirror` — so no mirror frame can rescue it and every later dispatch for that workspace returns false without creating, until environment teardown. |
||
|
Neil
|
58fac71ba0 |
fix(runtime): a landed handle retires the handle-gap timeout verdict
`expiredGenerationByPane` was cleared only by a later expiry on the same environment with a different connection generation. The module's escape hatch was "a reconnect bumps the connection generation and arms a fresh wait" — and the #19647 change in this same stack stops recording `status: null` for an unreachable host, so `connectionChanged` no longer fires across an outage on the same runtime. Measured: park, let the deadline fire, land the handle, republish rows ahead of handles on the same generation — `findUnhydratedHostMirrorForPane` returned null and the sweep resumed immediately with zero panes parked. That is #19735 with the bounded wait removed entirely rather than merely shortened. A published handle is positive host evidence and ends the gap episode the deadline was about, so it now retires the verdict. The store subscription outlives the waiter for exactly as long as an expiry needs watching. Also carries the worktree across a re-park: adopting an orphaned terminal re-keys `tabsByWorktree` without re-keying the record, so a live wait kept releasing on retraction evidence about the workspace it was no longer about. And contains the same throwing-replay shape in the sibling drain (`host-session-mirror-hydration`), which runs from the frame-apply path. |
||
|
Neil
|
e0bc16a8a1 |
test(session): pin which partition's terminal row survives adoption
workspace-session-stranded-partition-adoption.ts had no test anywhere in src/ or tests/, and its result is fed straight into a wholesale `replace-session` upload by persistedSessionForTarget — so whichever row wins here becomes the host's whole answer for that workspace, and every other client reads it. That is a lot of consequence resting on behaviour nothing records. Six cases, covering both directions the module can be asked about, the contested carve-out, and the no-host-partition short circuit. Two things are written down rather than left to be rediscovered: The known trade. `workspacesTheBaseOwns` keys on `tabs.length > 0`, which makes the direction the module argues for work (an empty base must not block adoption) and makes its mirror image unreachable (an empty HOST row cannot retract a populated base row). During the upgrade window a pre-partition build's rows sit in `local` while this build writes the owning `ssh:<targetId>` partition, so an emptied workspace's tombstone loses to the legacy row and the publish re-offers tabs the user closed. Bounded — a clean quit's `api.set` clears the legacy row. Deliberately not changed: letting the owning partition's empty row win trades a recoverable resurrection for a possible deletion, which is the direction this module's header argues against and the one that let `replace-session` delete the host's copy (#12721). The inert first draft. The contested case was first written with a populated base row and passed — for the wrong reason. A populated row makes the workspace un-adoptable before the contested check is consulted, so mutating that check to `if (true)` left it green. Rewritten with an empty base row, which is the only state where the check is load-bearing, the mutation now fails it. The comment keeps the warning, because "the assertion holds" and "the assertion could have failed" are different claims and reading only the first is how a data-loss bug survives review. |
||
|
Neil
|
9b74b61d21 |
fix(mobile): a scope refusal is not a missing method on the pairing probes
A phone decides whether a desktop can serve Relay by calling pairing.getEndpoints and pairing.provisionRelay and reading the failure. Both call sites treated only `method_not_found` as "this desktop is too old for Relay, stay on LAN" — and that code is the one answer a genuinely old desktop can never give. runtime-rpc-websocket-dispatch.ts runs the mobile allowlist gate BEFORE the RPC dispatcher. So for a mobile-scoped device `method_not_found` is reachable only for a method that IS allowlisted but unregistered; a method an older desktop predates is absent from both lists and the phone is refused with `forbidden`. The fallback could never fire against the exact desktop it exists for. In pre-profile-pairing-coordinator.ts that is the first-time QR pairing flow, so the failure is not a degraded mode — it throws and the phone ends up with no host profile at all: AssertionError: promise rejected "Error: forbidden: forbidden" instead of resolving ❯ requireSuccess src/transport/pre-profile-pairing-coordinator.ts:288 Both local `isMethodNotFound` predicates are replaced by one shared `isPairingRelayRpcUnavailable` that accepts either code. Safe in both directions: a desktop that serves the probes allowlists them, so `forbidden` on one can only mean "this desktop will not expose Relay pairing to a phone", which is exactly what the caller falls back for. See docs/reference/remote-wire-compatibility.md. The desktop-side test pins the mechanism rather than restating it: a mobile-scoped device gets `forbidden` for an unregistered method while a runtime-scoped peer gets `method_not_found` for the same one. Measured: mobile `vitest run src/transport` 98 files / 723 passed. Mutating away the `forbidden` arm fails both new cases; mutating away the `method_not_found` arm fails both pre-existing cases; mutating the desktop gate to emit `method_not_found` fails the new desktop case. No survivors. |