mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
8bd642a9f3bb9efac0a0bfba203c4fdbf7a40a3f
8497
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Neil
|
8bd642a9f3 |
merge: adv2-persistence-fixes (3962c1b4d1)
|
||
|
Neil
|
19d5128678 |
merge: adv2-crossplat-fixes (b2873d0bae)
|
||
|
Neil
|
90620adbfc |
merge: relay-lifecycle-fences (d4f7700062)
# Conflicts: # src/main/runtime/relay/desktop-relay-service.ts |
||
|
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
|
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
|
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
|
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
|
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
|
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. |
||
|
Neil
|
9de4e9dd21 |
fix(runtime): drain a removed environment's handle-gap verdicts on teardown
`expiredGenerationByPane` is pruned only by rules that run when a verdict is RECORDED — the stale-generation sweep here, and the tab-death sweep added separately ( |
||
|
Neil
|
c3d46edfea |
test(relay): pin the mid-mint policy flip under interleaving, not just in sequence
The mid-mint LAN-flip fix (
|
||
|
Neil
|
193119c18d |
fix(relay): scope nextPendingExpiry the way hasDemand is scoped
`hasDemand` requires four things of a standing binding — mobile scope, matching
owner identity, matching relay host, and allowed by the live pairing policy.
`nextPendingExpiry` applied NONE of them: it took the minimum `inviteExpiresAt`
across every device in the registry. So it armed the `demandExpiryTimer` wake in
`refreshDemand` off state that provably cannot produce demand.
Three cases measured returning a wake time where `hasDemand` was already false:
a binding for a different relayHostId, a runtime-scope (non-mobile) device, and
a phone the live LAN policy excludes.
Low severity on its own — the fired timer just reconciles to no demand — but it
is spurious wakeup churn from the class that owns the correct predicate three
lines above, which is the kind of drift that stops being harmless later. The
shared clause is now `demandCandidateBinding`; the owner check stays in
`hasDemand` because `nextPendingExpiry` takes no identity. The `hasDemand` side
is a pure conjunction reorder, confirmed behaviour-preserving by mutation rather
than by eye.
Re-arming after a policy exclusion is safe: `pairingPolicyChanged()` calls
`refreshDemand()`, so a flip back to automatic restores the timer. That round
trip is asserted rather than assumed.
Also records two things next to the code that would otherwise be lost:
- Transient refs carry NO OWNER IDENTITY, so `hasDemand`'s transient loop answers
true for any signed-in identity while the two branches below it filter on
`ownerIdentityKey`. Nothing defends the current behaviour OR that regression:
scoping the loop by owner fails exactly one test across the whole relay suite,
the characterisation test added for it. Deliberately not fixed here, and the
comment says why the in-file version is unsafe —
`withTransientDemand('provision')` calls `setMobileRelayBinding` INSIDE the
operation, so during a re-pair the device still holds the old owner's binding
and an inferred filter would drop demand mid-provision, tearing the broker down
under the operation holding the ref. The real fix threads identity through
`acquireTransient`, whose call site is desktop-relay-service.ts.
- The `if (!current) return` guard in the release closure is unreachable today
and mutating it away breaks nothing. Kept and labelled INERT rather than
deleted: it goes load-bearing the moment the ledger grows a dispose()/clear(),
and nothing in the suite would catch that.
The ref-counting core itself came back clean under every hazard exercised:
policy flip between acquire and release (the release closure is policy-blind by
construction, and the demand filter is a live pull per call rather than an
acquire-time snapshot, so neither a permanent pin nor a dropped-but-needed
demand); double release; opposite-order release of two refs on one key; and
cross-device release.
|
||
|
Neil
|
1673716c6d |
fix(relay): bound the live-broker wait over a chain of superseding reconciles
The budget bounded the armed-retry chain only. A waiter that kept following fresh reconciles — which the previous two commits made it correctly do — rode that chain with no deadline check at all. MEASURED, on a fake clock: budget 1,000 ms, reconciles landing every 200 ms, opens taking 400 ms and rejecting. Pre-fix the waiter NEVER SETTLED within a 300,000 ms horizon. Post-fix it settles at exactly 1,000 ms, the budget the caller asked for. Reachable in production because `withTransientDemand` reconciles on BOTH acquire and release, so a busy host supersedes a waiter's reconcile faster than opens settle, and the caller rides the chain indefinitely while holding its demand ref. The deadline check is gated on `joinedReconcile` so the contract that matters is preserved: the one open the waiter ARRIVED on is still never cut short, because cutting a slow-but-succeeding open short would fail a pairing that was about to work. The existing "slow first open past the budget still wins" test passes unchanged, which is the assertion that proves the gate. This is the third defect in this loop and all three share one root cause: `await pending` had no escape once the reconcile the waiter captured stopped being the one that mattered. |
||
|
Neil
|
6cfc1f2a4a |
fix(relay): release a live-broker waiter parked on an open that stop() abandons
`stop()` fences and closes, but did not wake waiters. A waiter parked on an open that never settles therefore had nothing left to release it — the wait is deliberately unbounded across the open it joined, and the coordinator it was waiting on is gone. Measured: the promise stayed pending for the life of the process. This is promise-shaped, not timer-shaped, which is why it survived the leak checks: `vi.getTimerCount()` reads zero at teardown and always did. The new test asserts on the settled value, and keeps the timer-count assertion so the two are not confused again. `fenceAndCloseNow` now fires the same authority-change signal a fresh reconcile does, which is all a parked waiter needs to re-read the world and return the settled cause. One line, because the wake mechanism it reuses landed in the previous commit. The sibling case — a waiter parked on an ARMED RETRY when the coordinator stops — was already clean; pinned in the same file so the distinction between the two teardown positions stays visible. |
||
|
Neil
|
8904488d3b |
fix(relay): wake a live-broker waiter when a newer reconcile takes the authority
A waiter joined `latestReconcile` and awaited it unbounded, on the reasoning that a reconcile always settles. True, but incomplete: once that reconcile is superseded its result is DISCARDED, so the waiter sits out an open nobody will use — measurably still parked after a newer reconcile had already registered a live broker. The wait now races the joined reconcile against an authority-change signal that `beginReconcile` fires, so a waiter follows the reconcile that actually matters instead of the one it happened to arrive on. The open the waiter arrived on is still never cut short. WORTH KNOWING FOR ANYONE TOUCHING THIS LOOP: the generation check that guards this — `if (pending !== source.reconcile()) continue` — was completely unprotected. Deleting it survived ALL 226 existing relay tests. It is the thing that keeps a waiter from answering out of a reconcile that no longer speaks for the coordinator, and nothing in the suite noticed its removal. The new test "follows a superseding reconcile that is still opening rather than answering from the old one" pins it, and is the only test that fails when it is deleted. The loop moved to relay-live-broker-wait.ts because the fix pushed relay-auth-coordinator.ts past the 300-line cap and AGENTS.md forbids disabling max-lines. The source is a record of getters rather than a snapshot because the wait re-reads all of it after each await. Also pins three interleavings that came back CLEAN, so the scope of "clean" is on the record: two waiters on one armed schedule (the short-budget waiter neither cancels the retry nor strands the long one); a retry firing on the exact tick the budget expires (the armed retry takes the tie deterministically and the waiter returns that attempt's cause, not a null one); and a terminal cause landing while a retryable wait is armed. |
||
|
Neil
|
6f514e4022 |
fix(runtime): isolate one worktree's replay from the mirror-hydration drain
The same fan-out hazard as the handle-gap drain, one module up. Settling an environment drains every worktree parked on it in a single loop, called from the frame apply, with `waiter.run()` unguarded. One replay that throws strands every waiter queued behind it and surfaces in the caller applying the frame. Found by looking for the sibling of a defect rather than by a separate interleaving: both modules park a `run` callback and drain N of them from one event, so both have the same blast radius. Kept as its own commit because the two modules route independently. Mutation: dropping the guard kills exactly the one new assertion. |
||
|
Neil
|
15f3401415 |
fix(runtime): isolate one pane's replay from the handle-gap drain
One store write releases every due pane, and the drain runs synchronously inside
a zustand subscriber. `waiter.run()` was unguarded, so a single pane's replay
reached two things it has no business touching:
- the throw escapes out of `useAppStore.setState`, meaning the mirror apply
that published the PTY handle throws at its own call site;
- every pane queued behind the thrower is stranded — waiter still parked,
deadline still armed — and then decides on a connection whose evidence
landed long ago.
The deadline path fans out the same way, so a throwing replay also escaped the
timer callback.
Reachable: `resumeSleepingAgentSessionsForWorktree` reaches `state.createTab`
with no guard of its own. The panes in a drain are strangers to each other and
to the frame that released them; none of them should be able to see another's
failure.
The new tests live in their own file because
host-mirror-handle-gap-resume.test.ts drives the waiter through the real resume
sweep and so cannot choose what a replay DOES. Note for anyone extending that
file: per its header, "did the waiter release" is not an observable here — a
spurious release is re-parked immediately and reads identically one tick later.
These tests assert on timer count and on the deadline instead.
Also records two findings next to the code, so they are not rediscovered:
`expiredGenerationByPane` is never pruned for a removed environment (bounded and
inert, since removal advances the generation, but it does not drain — and a
DIFFERENT leak in that same map is being fixed concurrently, so reconcile rather
than patch around it); and sustained reconnect churn holding a pane parked
indefinitely is CORRECT, not the latch-that-never-releases defect, because under
churn liveness genuinely is unverifiable and ssh-execution-boundary.md forbids
resolving that to `exited`. It has the shape of the defect and will eventually
be "fixed" by someone who does not know that.
Mutation: dropping the guard kills exactly the three new assertions and leaves
all twelve existing waiter tests passing.
|
||
|
Neil
|
3962c1b4d1 |
docs(session): record why a re-key clobbering an existing target stays unfixed
Not a missing guard -- an unresolvable one. Keeping the target is correct when it holds a real closed-last-terminal tombstone; keeping the source is correct when the target row is a stub; nothing records which is newer. The recency map is the only one that can settle it, because Math.max needs no such ordering. |
||
|
Neil
|
bf497512d8 |
fix(runtime): stop a re-pair freeing an in-flight initial-terminal claim
The three initial-terminal suppression defects already fixed were all exits of `dispatchWebRuntimeInitialTerminalBootstrap`. Those exits ARE exhaustive, and this change is not a fourth one: `WebRuntimeTerminalCreateOutcome` is a closed union of `created | failed`, so the helper's five paths — claim denied, thrown failure, returned failure, success with a mirrored row, success without one — cover every way it can return. Read alongside the three prior fixes this looks redundant; it is not, because the gap is a release from OUTSIDE the helper, which the helper cannot see and cannot guard. `clearWebSessionTabsTrackingForEnvironment` dropped the environment's whole latch map regardless of phase, and it runs on a pairing-revision change — which is simultaneously a dependency of the active session-tabs effect. So a re-pair freed an in-flight claim and re-armed a closure whose `requestedInitialTerminal` is false in the same tick, and the next empty frame owned a second create with the first still unresolved. That is STA-6173 restored through a door one level up from the latch. Teardown now drops a parked claim — nothing will answer an `awaiting-mirror` key once its subscription is gone — but only MARKS an in-flight one, as `creating-after-teardown`. It still blocks, and the create's own dispatch releases it on every exit it has. The inverse hazard is real, so a torn-down claim that resolves without a row is released rather than parked: no frame is coming to release it. Holding is bounded: every RPC on the create path carries `timeoutMs: 15_000`, the placement settle a 10s deadline, and the whole body sits in a try/catch, so the create always settles. Two existing tests are rewritten rather than deleted because they asserted the BUGGY contract — that teardown must free an in-flight claim, to stop "a create RPC that never settles" from suppressing the next bootstrap. That premise is unreachable for the bounds above, and what the free actually did was hand the claim to the closure the same teardown re-armed. Their replacements pin the bounded invariant instead, plus the parked-claim half that teardown must still drop. Mutations: freeing the in-flight claim at teardown kills exactly three assertions; parking a torn-down claim kills exactly one. No survivors. |
||
|
Neil
|
b2873d0bae |
fix(worktree): recognise the Windows profile through WSL's drvfs view
`/mnt/<letter>` under a WSL UNC alias is the distro's drvfs mount of a Windows volume, so `\\wsl.localhost\Ubuntu\mnt\c\Users\bob` is `C:\Users\bob` wearing a Linux spelling. The Windows-profile rule excludes every WSL UNC path by design (the aliases normally front a Linux filesystem) and the POSIX shapes never match a `/mnt/...` tail, so that path fell through both and read back as deletable. The spelling is producible by the product: `resolveWslRepoWorktreeBasePath` maps a `/mnt/c/...` worktree base against a WSL repo into exactly this UNC form, and `getWslFilesystemBoundaryDistro` already treats it as the drvfs crossing. A drvfs tail now takes the Windows rule on its drive form, via the existing `toWindowsWslDrivePath`. Scoped to the UNC branch, where `parseWslUncPath` has proven the path is a WSL alias — a plain Linux host's `/mnt/c/...` is untouched. The lowercase-only `/mnt` match is deliberate: `/MNT` is an ordinary case-sensitive Linux directory, never the automount. |
||
|
Neil
|
3ac138553b |
fix(session): keep a renamed worktree's rows from matching on the id it lost
Three persisted session fields survived a worktree re-key still naming the old identity. Two of them are suppression records, so a stale id does not read as residue -- it silently re-admits state the user removed: - closedTerminalTabTombstonesByTabId: the remote merge only suppresses a host tab when the tombstone's worktree equals the tab's, and no snapshot ever covers the old id, so the tombstone never retires either. - clientHostedBrowserCloseIntentsByEnvironment: the replay targets the intent's worktree, and an unresolvable selector answers selector_not_found -- which the replay reads as definitively gone and uses to DROP the intent. - clientHostedBrowserPagesByWorktree: keyed by worktree and re-checked against the row's own workspaceId, so both halves have to move or the pages are never rehydrated. Fixed on both sides of the rename: the main-process persisted migration and the renderer's live store, which would otherwise write the stale values straight back. The coverage test drives off WORKSPACE_SESSION_WORKTREE_REFERENCE_KIND, the census these three fell out of, with the shipping owner collector as its oracle. |
||
|
Neil
|
be3bce2f5b |
fix(worktree): let the worktree path alone decide whose home it is
A WSL project is registered under its UNC spelling (`\\wsl.localhost\<distro>\...`) while its worktree list is read by running git *inside the distro*, which answers in Linux paths and is handed back untranslated (`toWslExecutionSpace`, `worktree-list-reader.ts`). So the pair the removal guard actually receives is `( /home/neil , \\wsl.localhost\... )` — the normal WSL case, not a corrupted row. `isDangerousWorktreeRemovalPath` derived its path ops from that pair (`getPathOps(worktreePath, repoPath)` uses `.some(isWindowsAbsolutePathLike)`), so one Windows-shaped *repo* path picked win32 and every POSIX home-shape rule for the *worktree* path went quiet. `/home/<user>` — the canonical Linux home — read back as an ordinary deletable directory, on the last guard standing in front of the recursive delete. Measured: `/home/neil` under `\\wsl.localhost\Ubuntu\opt\repo` and under `C:/repo` both returned `dangerous=false`. Whose home a path is, is a property of that path and nothing else, so the home guard now re-derives its ops from `worktreePath` alone. The repo-containment check keeps the pair, which genuinely compares two paths. Why no existing test saw it: the guard's unit-test helper called the single-argument form of the path-ops resolver, so those tests never exercised the two-argument call its only production caller makes. A test helper that paraphrases the caller instead of using it misses exactly the bugs that live in the difference. Pre-existing on main (the `pathOps !== posix` gate predates the rewrite) rather than a regression introduced by the surrounding stack, but it lives inside the function that stack rewrote and its new Windows/WSL rules inherit it. |
||
|
Neil
|
09be96a2b6 |
test(runtime): pin sleeping-agent resume on a failed SSH target
The terminal-state floor in workspace-terminal-host-authority.ts has three consumers: initial-terminal seeding, the startup terminal watcher, and sleeping-agent resume. Seeding is covered end to end by worktree-agent-activation-seam.test.ts. Resume was covered only at the predicate, so nothing failed if the floor stopped reaching it — and the floor's own comment says the cost of losing it is a failed target left terminal-less with unresumable agents for the rest of the app session. Pins the resume half directly: an SSH git worktree on a target whose sync terminated in offline/error with an empty hydrated set resumes its sleeping agent. Two controls keep the floor from widening into "resume whenever we are unsure" — an in-flight 'pulling' sync and no sync status at all both stay unverifiable and resume nothing. Verified by mutation: emptying TERMINATED_WITHOUT_ANSWER_PHASES fails exactly the two floor assertions and leaves both controls passing. Routes independently of the two fixes on this branch: the floor predates this stack (#16750), and this only closes a coverage gap in it. |
||
|
Neil
|
a97c9e2144 |
fix(relay): name the LAN flip that lands mid-mint, not relay_control_not_active
withTransientDemand checks the host pairing policy once, at entry. But RelayDemandLedger.hasDemand filters the in-flight operation's OWN transient ref through the live policy, so a flip to local-only while createPairingRelay or provisionRelay is awaiting withdraws the demand that operation is holding. The coordinator then reaches its no-demand branch and publishes 'standby', publish() clears offlineReason to null for any non-offline status, and the broker wait returns with no cause at all — relay_control_not_active, the generic code this stack exists to remove. The flip is exactly the cause and relay_disabled_for_device is already its word; the entry gate just could not see a flip that had not happened yet. Re-ask the policy on the failure path rather than trusting the thrown code. Reproduced first: the added test failed with "expected ... to throw error including 'relay_disabled_for_device' but got 'relay_control_not_active'". Controls cover the two ways this could over-reach — a failure the policy had nothing to do with is not rewritten, and a grant that succeeded under a flip is left alone. Top follow-up found alongside this and deliberately not fixed here: flushRevoke swallows every error with a bare catch, no attempt cap and no item expiry, while the revoke check in hasDemand is deliberately not filtered through the policy. A revoke that fails permanently server-side therefore holds demand open forever and defeats the LAN pick entirely — the same symptom this change is part of closing, by a different route, with no test coverage. |
||
|
Neil
|
c5a38e6383 |
fix(relay): stop an unreadable session file reporting as a sign-out
The coordinator's own comment states the contract: readContext "throws on transient failures and returns null solely when the cloud session is gone (absent, or cleared by a 401)". The implementation did not honour it. readFreshOrcaCloudSession collapsed readOrcaCloudSession's `unreadable` status into `reconnect-required`, so readRelayAuthContext returned null and the coordinator published RELAY_HOST_CLOSE_REASON.SIGNED_OUT, closed the broker with that wire reason, and armed no retry because a terminal cause arms none. `unreadable` fires on EPERM/EACCES/EBUSY/EMFILE/ENFILE/EIO — descriptor exhaustion on a busy machine, a Windows AV file lock. The phone latches setHostSignedOut and shows "Desktop signed out — sign in to Orca on your desktop to reconnect" for a failure the next read would have cleared. The contrast is the argument: `unreadable` is the one status the session store's own doc says "licenses nothing", and clearCloudSessionIfUnchanged already refuses to delete a session because of it — "a session we were denied is not a session we may delete". The relay taxonomy was the single place treating it as evidence the user signed out. Give it its own arm on FreshCloudSessionResult and throw for it in readRelayAuthContext, which lands it on the retryable auth_unavailable path the coordinator already has. Every other caller tests `status !== 'found'`, so their behaviour is unchanged. Correct the coordinator comment to say what it actually depends on: null-means-gone is a contract readRelayAuthContext owes it, not something that branch can verify. Measured before the fix: offlineReason "signed-out", mint code relay_signed_out. After: auth_unavailable. Not fixed here, and worth a separate look: `decrypt-failed` when safeStorage.isEncryptionAvailable() is false (a Linux keyring still locked at login) takes the same route to SIGNED_OUT, but reclassifying it changes sign-in semantics well beyond the relay. |
||
|
Neil
|
614ea3f6a9 |
fix(session): an emptied workspace still survives the reconnect merge
localActiveWorkspaceSurvives decided whether the workspace the user is standing in still exists by counting its tabs. A workspace they had just closed the last terminal in reads as zero, so it "did not survive", the host's null activeWorktreeId was taken literally, and the reconnect dropped them onto the home screen -- for closing a tab. An explicit empty tabsByWorktree row is the record that the user closed the last terminal, not the absence of a workspace (initial-terminal.ts). hasLocalTabsRow two hundred lines up in this same function already draws that distinction with Object.hasOwn; this line was simply missed. The counterweight is pinned too: presence must not turn into "never follow the host", so a host that does name an active worktree still wins over the emptied local one. |
||
|
Neil
|
6bd12fc3e9 |
fix(terminal): a live pane owns its transcript in any workspace
The resume dedup was scoped to the record's own workspace on both terms -- the entry's tab had to be in worktreeTabIds AND entry.worktreeId had to match -- while the completed-turn widening only relaxed the status term. A cross-workspace record whose peer pane is `done` and still holds a live PTY therefore matched nothing, and the sweep launched a second agent onto a transcript the peer is still writing. The two ids really do drift. canonicalizeTerminalSessionWorktreeId re-keys tabsByWorktree, tabGroups, tabGroupLayouts, activeTabIdByWorktree and activeGroupIdByWorktree onto the canonical worktree id, and does NOT re-key sleepingAgentSessionsByPaneKey, whose records carry worktreeId inside them. So adopting an orphaned terminal is a direct producer of a record naming one workspace while its pane and status row name another. Split into two arms rather than widening the existing condition. The new arm carries no workspace scope but demands hard evidence: a provider session id names one transcript, so a pane whose exact PTY is live right now already owns it wherever that pane sits, and no workspace boundary makes a live PTY less live. The scoped arm keeps its scope and its state !== 'done' term, because a status row with no live PTY is a claim about the past and must not reach across workspaces. Pins both directions: the live peer is not forked, and the same peer without a live PTY still resumes. |
||
|
Neil
|
c2f14d1cd7 |
fix(runtime): void a handle-gap verdict the reconnect made stale
The per-pane park bounds itself with one deadline per connection, but the waiter never recorded WHICH connection it was armed on. A wait armed on generation 0 that fires after a reconnect stamps its expiry against the current generation, so hasHostMirrorHandleWaitExpired agrees, the mirror lookup returns null, and the pane is resumed after 1ms on a connection that has had no chance to publish the handle. That is #19735's fork with an extra step, reached through the guard that exists to prevent it. The module's own doc comment claims the opposite -- "a reconnect bumps the connection generation and arms a fresh wait" -- and that is true only for a wait which had ALREADY expired, which is precisely the case the existing test covered. The test and the comment agreed with each other and both were wrong about the live case. The waiter now carries the generation it was armed on and records no verdict when the generation has moved; the replay re-parks through the existing machinery and the new connection gets its own full budget. Still bounded per connection generation, which is what was documented all along. Also pins the three sibling attacks on the same window: two panes in one environment where only one handle lands, a handle published by a foreign environment, and an environment tearing its rows down mid-park (which leaves no waiter and no scheduled timer). The test file now leads with how to assert on this module at all, because the obvious shape cannot fail. "Did the waiter release" is not an observable here -- a waiter released for the wrong reason is re-parked by the replayed sweep, so the store reads identically one tick later, and a mutation releasing every waiter on any tab's handle survived twelve assertions written that way. What a spurious release costs is the deadline, so the assertions advance the clock and require the pane to decide on the ORIGINAL schedule. |
||
|
Neil
|
798a37bcba |
merge: origin/main (fb85f88d64) into ssh-remote-integration-stack
|
||
  Brennan BensonandMerge Sim
|
fb85f88d64 |
fix(browser): restore the Chrome-shaped browser identity (STA-7147) (#19927)
* fix(browser): restore the Chrome-shaped browser identity (STA-7147) #18749 replaced every browser partition's Chrome-shaped UA with Electron's stock one, so since v1.4.198 the embedded browser announces itself on every non-Google host as: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Orca/1.4.198 Chrome/150.0.7871.224 Electron/43.4.1 Safari/537.36 No browser sends that. Sites that re-check the identity holding a session reject it: users report being signed out of x.com, LinkedIn and "most websites," and at least one was signed out of LinkedIn in their own Chrome and met LinkedIn's "suspicious activity" SMS check -- server-side revocation, which reaches beyond our app. The repo already documented the mechanism in browser-google-auth-ua.ts: copied-in cookies "sent under a UA that doesn't match a real first-party browser get flagged by anti-fraud." That is why the Google auth-host switch exists; #18749 kept it for accounts.google.com and handed every other host an Electron identity. Restore the pre-#18749 session identity: strip the Electron and app tokens, and rewrite sec-ch-ua to match. Nothing in the cookie-import write path changed -- it never did; cookies were always written correctly and servers were refusing them. Deliberately KEPT from #18749, all independent of the UA: - anti-detection.ts stays deleted. Its premises were measured false on Electron 43 and its overrides are themselves published bot signatures. - No Runtime.enable into cross-origin iframes (the documented Cloudflare CDP tell). - No unconditional CDP debugger attach on every browsing guest. Known tradeoff, measured: this re-opens #13822. On the unmerged predecessor branch brennan/sta-3905-cloudflare-ua, commit 9f0a4772fe recorded the stock UA clearing dash.cloudflare.com 5/5 while every rewritten variant failed 12/12, and noted that adding client hints does not rescue it. So Cloudflare-gated sites will show verification failures again until a coherent-identity fix lands. That is a bounded, in-app annoyance; session revocation damages users' real accounts. A CDP Emulation.setUserAgentOverride with full userAgentMetadata -- which drives navigator.userAgentData as well as the headers, and was never tested -- is the candidate that could satisfy both, and is being measured separately. Tests: the real-Electron wire-identity test now asserts the stripped identity on ordinary hosts and Firefox on Google auth hosts. Ablation-verified: neutering cleanElectronUserAgent turns it red on the Electron-token assertion. Its fixture also gained an app name -- without one the raw UA carried no app token, so the Orca/x.y.z half of the cleaner was never exercised. * fix(browser): finish the identity revert in the files CI caught browser-session-registry.persistence.test.ts still asserted #18749's behaviour ("keeps the stock UA", "keeps the engine UA"), so the shipped code and its test disagreed. Caught by CI shard 4/8, not locally: I reverted four test files and went to typecheck without re-running the browser suite. Also restores the accurate wording that #18749 generalised away, now that the behaviour it described is back: - browser-google-auth-ua.ts: names the Electron/Chrome-shaped UA again as what anti-fraud flags, which is the reason the auth-host switch exists at all. - docs/browser/profiles.mdx: documents the cleaned Chrome UA default and the --no-ua-spoof escape hatch, which is real again. - tests/tools/google-signin-ua-probe.cjs: comments name the live handler. Deliberately left at #18749's version, because those changes stay correct with anti-detection.ts deleted: - browser-manager-viewport.ts: its comment no longer cites the retired addScriptToEvaluateOnNewDocument injection. - browser-webauthn-profile-delete.test.ts: its added webRequest mock is REQUIRED by the restored setupClientHintsOverride, so reverting it would break the test. * fix(browser): keep restored UA hints browser-owned --------- Co-authored-by: Merge Sim <sim@local> |