Skip to content

fix(relay): settle the live-broker wait under superseding reconciles, stop and churn - #20061

Open
nwparker wants to merge 12 commits into
fix/18211-lan-mode-applies-to-paired-devicesfrom
nwparker/relay-broker-wait-concurrency
Open

nwparker wants to merge 12 commits into
fix/18211-lan-mode-applies-to-paired-devicesfrom
nwparker/relay-broker-wait-concurrency

Conversation

@nwparker

@nwparker nwparker commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​768 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​768
Prod 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​158 $\color{#cf222e}{\Huge{\mathbf{−}}}$​48 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​110

Rewritten smaller (2026-09-17)

The first version reconciled four parallel signals for "which reconcile is current" — an integer authEpoch, a Promise compared by identity (latestReconcile), a deferred woken by wakeWaiters(), and RelayRetrySchedule.settled. The wait loop parked on promise identity while authority was an integer, and the two were reconciled by hand at each turnover site. Nothing structurally enforced "every turnover wakes the waiters"; it was a call you had to remember.

It is now one field, authority, and one mutator, turnover(). Ending the outgoing authority is what releases waiters, and it happens inside the same operation that installs the replacement — so a superseded wait is impossible rather than prevented. Object identity is the epoch, so isEpochCurrent becomes an identity check and the integer is gone.

Net effect of the rewrite: +124 / −210 against the previous version. Production across the whole PR is now +127/−84; the remainder is tests, which are the behavioural contract and were kept.

What fell out of the shape rather than being added

  • A caller arriving after a fence gets the cause immediately. Previously the fence left latestReconcile pointing at the open it had just abandoned, so a waiter arriving afterwards parked behind that open until readContext's own 60s ceiling. The fence now installs an idle authority. New test.
  • RelayRetrySchedule.settled is subsumed. A retry fires through beginReconcile, which turns the authority — the thing the waiter is already parked on. The armed deferred is deleted. cancel() still never resets the attempt counter.
  • relay-live-broker-wait.ts and its six-closure LiveBrokerWaitSource seam are deleted. No test ever imported them; the seam existed to keep the coordinator under max-lines, which the smaller shape no longer needs.

Corrections to the earlier analysis

A previously-reported "fence hole" — the claim that fenceAndCloseNow left a woken waiter re-parking on the same fenced reconcile — was wrong, and is retracted. Measured on the old shape: the waiter did settle, because after the wake it fell through to armedRetry(), which the fence's retry.cancel() had nulled. Removing wakeWaiters() from the fence made it hang, which is what proved the wake load-bearing. No one-liner was applied and the header comment was accurate as written. What was missing was coverage, so the fence regression test written during that investigation is kept here.

Two independent design passes converged on the same shape and, separately, on the same missing test (post-fence arrival). That agreement is why the idle-authority case is pinned rather than assumed — worth stating, since a later reader cannot reconstruct it from the diff.

Dropped

The characterisation test pinning the transient-ref owner-scoping gap is deleted, not carried forward. Its own comment admitted the behaviour was a bug, and scoping the loop correctly would have failed exactly that one test — a test you must delete before you can fix a bug is worse than no test. The gap stays documented on hasDemand, in four lines rather than fourteen.

Explicitly still out of scope

The underlying churn — beginReconcile invalidating every in-flight open unconditionally, plus withTransientDemand calling refreshDemand() on both acquire and release of every RPC, so phones polling faster than an open completes can starve it — is not fixed here, deliberately. It is a behaviour change rather than a restructuring: relay-auth-coordinator.test.ts pins hard-restart semantics for same-identity re-reconciles, and adopting an in-flight open would invert that. It also needs a distinction the coordinator does not have (auth mutations must hard-restart; demand refreshes need not). Follow-up PR, roughly +40 prod / +60 test. This shape makes it a one-line choice in beginReconcile, because the wait keys on authority identity rather than on how many opens were started.

Mutation results (measured, not reasoned)

Mutation Result
turnover() no longer ends the outgoing authority 10 tests fail
fence installs a running authority instead of idle 5 fail, incl. the new post-fence-arrival test
fold retry.reset() into turnover() 1 fails — the five-minute backoff-cap test
bound the arrived-on open with the budget 2 fail — wait-budget and slow-first-open

Suites: src/main/runtime/relay plus src/main/startup — 87 files / 752 passed, 8 skipped, 0 failures. pnpm tc exit 0. oxlint clean (coordinator under max-lines with the loop inlined). Changed-code quality gate passed, casting and SAFETY-rationale both 0 new.

Stacked on #19870 (fix/18211-lan-mode-applies-to-paired-devices). Review the seven commits above it.

Five relay-concurrency defects that lived only on un-PR'd integration branches. They were the relay half of nwparker/adv2-concurrency-fixes, which has no PR of its own and is not going to get one — so every one of these would have been lost when that branch was abandoned.

The live-broker wait (three fixes, one file)

waitForLiveBrokerResult is what every caller blocks on to get a broker. Three separate ways it failed to settle:

commit defect
f8ab8f5c693 a newer reconcile takes the authority and the parked waiter is never woken — it waits for a reconcile that is no longer the current one
9070031ed13 stop() abandons the open the waiter is parked on, leaving the waiter on a promise nothing settles
0549bd19a65 a chain of superseding reconciles is unbounded — the pending !== latestReconcile arm re-enters the loop without re-checking the deadline, so the documented 20s budget is not enforced and the caller's transient demand ref is held past it

The third is not a hang — every iteration awaits a settling promise — so it is bounded by how long the churn lasts, which is exactly why it needed a bound rather than a fix.

Demand scoping

86781b85514 — nextPendingExpiry was not scoped the way hasDemand is. The two read the same ledger and disagreed about which devices count, so an expiry could be armed for demand that the other half did not consider live.

Interleaving coverage

e016ec1f27f — the existing mid-mint policy-flip test drove its steps in sequence, which cannot reach the interleaving the defect needs. This pins it under real interleaving.

Plus one documentation fix

78b274b55dd — lingerMs on the coordinator contract reads as a policy knob and is not one. The only production 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.

Lifted from a dead-code/unread-field sweep (94ba0b9f052) stranded on an un-PR'd branch. The rest of that sweep is a protocol-reachability docstring and goes to #19861, not here.

Plus a correction this stack made necessary

61dd0e62b30 docs(relay): the live-broker wait budget does not bound the call.

LIVE_BROKER_WAIT_BUDGET_MS carried two claims. One is true and load-bearing — 20s bounds how long a waiter sits through armed retries. The other was false: "stays inside the phone's 30s request budget."

It cannot. The deadline is consulted only after the await, and a reconcile's own ceiling is readContext's cloud-refresh timeout (60s, plus one retry for a definitive 5xx) followed by the broker open. Measured: with a reconcile in flight, a wait at the default 20s budget had not settled at 45s.

Not academic — pairing.provisionRelay reaches this through requireActiveBroker, so it is phone-facing. The phone gives up and retries while the desktop still holds a transient demand ref for the call it abandoned.

Why it belongs in this PR specifically. f8ab8f5c693 extracts the wait into a new file, relay-live-broker-wait.ts — and carries both comments along verbatim. Without this commit, this stack moves a measurably false claim into a brand-new file, where it reads as freshly written and authoritative rather than as something inherited.

The same applies to the inner comment: "a reconcile always settles (opens carry HTTP deadlines)" is true, but the parenthetical names only the smaller of its two deadlines and omits readContext's, which dominates. "Settles" is not "settles soon", and reading it as the latter is how the 30s claim survived in the first place.

Behaviour unchanged. Bounding the reconcile would fail a slow-but-succeeding open — the worse trade, and what the adjacent comment already argues.

Carried from 6b029820cc9 on the un-PR'd nwparker/j4-skew-matrix. That commit targets relay-auth-coordinator.ts, where this code no longer lives after the extraction, so it does not apply as written; the corrections are ported to their new home. The mutation was re-run after the port rather than trusted: bounding the await with the deadline fails relay-auth-coordinator-wait-budget.test.ts, so the test still pins the property in its new location.

Not in here, deliberately

6bd997b2cca "fix(relay): clear the demand-expiry timeout on a fence, not only in stop()" shares this lineage and is superseded by #19963 — see that PR. Re-applying it would be a regression, not a duplicate.

Its near-twin fcea70fe3c6 (same subject, different content — the pair is not patch-identical, despite looking it) is worse than a duplicate here. Its one unique addition is a four-line comment documenting that the pending !== latestReconcile arm skips the deadline check — which is exactly the gap 0549bd19a65 fixes in this PR. Landing it would ship a comment describing a defect that no longer exists.

Base

main does not work as a base: f8ab8f5c693 conflicts there. The dependency is #19870's stack, onto which all six apply clean and in order. Merge order: #19870 → this.

Verification

tsc --noEmit -p config/tsconfig.node.json clean · src/main/runtime/relay + src/main/startup — 79 files / 683 tests passed, 2 files / 8 tests skipped, 0 failures, 0 infra errors · oxlint clean · oxfmt --check clean.

(src/main/startup/ensure-virtual-display.test.ts › reports failure when a stale socket blocks the Xvfb rebind is excluded — it is red on the untouched base, Xvfb on macOS.)

Rebase record (2026-09-11)

Stack preserved: base stays fix/18211-lan-mode-applies-to-paired-devices (#19870), rebased onto main at 5fa62feda7c264c6eda99ba4788ffebab9aae26c first. Replayed with git rebase --onto 13eee7eacf8a6fcd4bb77ff680d18f727fca1171 9ce190247edbdb508b452e8c62a63c4c2cdd0089.

SHA
old head 61dd0e62b30f4700010f769bdedc7cdcd1e55a88
new head 80fcca2551252c95f4964378b4913ff18321913a
new base (#19870) 13eee7eacf8a6fcd4bb77ff680d18f727fca1171

No conflicts across all seven commits.

nwparker added a commit that referenced this pull request Sep 11, 2026
…le yet

relay-protocol-version.ts documents a negotiation 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. 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 first, and relayProtocolOfferAdmits never decides
anything on any live path. The stranded incumbent this exists for sits in
`relay-<otherVersion>/`, which nothing dials.

Keeping it is right -- the mechanism is correct and hostile-input-safe, and it
is the part that has to exist first. What was missing is the statement of what
else has to land with it, written where someone editing this file will see it.

Carried from a dead-code and unread-field sweep stranded on the un-PR'd branch
nwparker/adv2-map-reconciled (94ba0b9). The lingerMs half of that sweep is
a coordinator concern and went to #20061 instead.

No behaviour change.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Live-broker wait extraction — waitForLiveBrokerResult moves into relay-live-broker-wait.ts as runLiveBrokerWait, driven by a LiveBrokerWaitSource of re-read closures so every loop iteration sees current coordinator state.
  • Authority-turnover wake — a new authorityChange resolver wakes waiters on every fresh reconcile and fence, so a waiter parked on a superseded open re-joins the newer reconcile instead of holding behind an open nobody will use.
  • Stop/fence release — stop() and fenceAndCloseNow() now wake parked waiters, settling a waiter stranded on an abandoned open (previously a process-lifetime leak).
  • Budget over superseding chains — the deadline is re-checked after a supersession (not only over armed retries), bounding a chain of superseding reconciles while still never cutting the initial open short.
  • Demand-expiry scoping — nextPendingExpiry now shares demandCandidateBinding with hasDemand, excluding foreign-host, non-mobile, and policy-excluded devices from wake-timer arming.
  • Documentation — lingerMs labelled a test seam (the only production construction passes neither it nor random); the budget comment corrected to stop claiming it bounds the call.
  • Tests — three new test files covering interleaving, stop/fence settlement, churn-bounded budget, and demand-ledger lifecycle/scoping, all using an observe() helper that turns a leaked waiter into an assertion failure rather than a runner hang.

I traced the authorityChange wake/Promise.race logic against the reconcile/fence ordering (swap-then-resolve means a reader never sees a stale resolved promise; latestReconcile is updated before wakeWaiters), and verified the budget semantics against settledResult's live-broker re-read. Each bug-pinning test fails against the pre-fix implementation. The hasDemand refactor through demandCandidateBinding is behavior-preserving, and the nextPendingExpiry scoping change is correct (owner filtering is intentionally left to hasDemand; a foreign owner's expiry at worst causes a harmless spurious wake).

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker force-pushed the fix/18211-lan-mode-applies-to-paired-devices branch from 9ce1902 to 13eee7e Compare September 11, 2026 08:10
…e 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.
…) 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.
…conciles

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.
`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.
…t in sequence

The mid-mint LAN-flip fix (a97c9e2, "name the LAN flip that lands mid-mint, not
relay_control_not_active") was pinned with the flip sequenced BEFORE the mint.
That leaves the case it was actually written for untested: the flip landing at
an await point inside the mint.

Both interleavings now run — the flip landing while the mint is parked on an
in-flight broker open, and landing mid `create_pairing_relay` after the broker is
already live. Both name `relay_disabled_for_device`, so the fix holds under
interleaving and not merely in sequence.

No production change. Committed separately because it verifies an existing fix
rather than carrying one, and routes to whoever owns that fix.
No production path sets it: the only `new RelayAuthCoordinator` is in
desktop-relay-service.ts and passes neither `lingerMs` nor `random`, so every
shipped linger is the hardcoded ten-minute default. Labelled so the next person
tunes the default, which is the only thing a user can feel.

Lifted from a dead-code and unread-field sweep that was stranded on an un-PR'd
integration branch (94ba0b9); the rest of that sweep belongs to #19861.
LIVE_BROKER_WAIT_BUDGET_MS carried two claims. One is true and load-bearing: 20s
bounds how long a waiter sits through armed retries. The other was false, and
this stack moved it verbatim into a brand-new file where it reads as freshly
written: "stays inside the phone's 30s request budget".

It cannot. The deadline is consulted only AFTER the await, which is unbounded on
purpose, and a reconcile's own ceiling is readContext's cloud-refresh timeout
(60s, plus one retry for a definitive 5xx) followed by the broker open. Measured:
with a reconcile in flight, a wait at the default 20s budget had not settled at
45s.

The consequence is not academic. pairing.provisionRelay reaches this through
requireActiveBroker, so it is a phone-facing call: the phone gives up and retries
while the desktop is still holding a transient demand ref for the request it
abandoned.

Behaviour unchanged. Bounding the reconcile would fail a slow-but-succeeding
open, which is the worse trade and is what the adjacent comment already argues.
What changes is that the false half of the claim is removed and the real ceiling
is written down, with a test pinning it so the claim cannot drift back.

Also corrected the inner comment: "a reconcile always settles (opens carry HTTP
deadlines)" is true, but the parenthetical named only the smaller of its two
deadlines and omitted readContext's, which dominates. "Settles" is not "settles
soon", and reading it as the latter is how the 30s claim survived.

Carried from 6b02982 on the un-PR'd nwparker/j4-skew-matrix. That commit
targets relay-auth-coordinator.ts; both comments moved to relay-live-broker-wait.ts
in 'wake a live-broker waiter' earlier in this stack, so the corrections are
ported to their new home rather than applied where they no longer exist.

Re-verified after the port rather than assumed: mutating the await to bound it
with the deadline fails the new test, so the test still pins the property in its
new location.
@nwparker
nwparker force-pushed the nwparker/relay-broker-wait-concurrency branch from 61dd0e6 to 80fcca2 Compare September 11, 2026 08:11
nwparker added a commit that referenced this pull request Sep 11, 2026
…le yet

relay-protocol-version.ts documents a negotiation 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. 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 first, and relayProtocolOfferAdmits never decides
anything on any live path. The stranded incumbent this exists for sits in
`relay-<otherVersion>/`, which nothing dials.

Keeping it is right -- the mechanism is correct and hostile-input-safe, and it
is the part that has to exist first. What was missing is the statement of what
else has to land with it, written where someone editing this file will see it.

Carried from a dead-code and unread-field sweep stranded on the un-PR'd branch
nwparker/adv2-map-reconciled (94ba0b9). The lingerMs half of that sweep is
a coordinator concern and went to #20061 instead.

No behaviour change.
fenceAndCloseNow wakes waiters, but nothing covered it: every existing test
fences only after the wait already resolved. Removing the wake left the
waiter parked on the abandoned open and no test noticed.

Measured: with wakeWaiters() removed from fenceAndCloseNow this test fails
(alongside the stop() case, which shares the mechanism); with it restored the
relay suite is 28 files / 226 tests green.
Clean auto-merge; verified the result differs from the base only in this
branch's own files.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Fence settlement test — adds one test pinning that fenceAndCloseNow() wakes a waiter parked on an open that never returns. It targets the direct fence callers (quit, sign-out) rather than stop(), so the wake stays in fenceAndCloseNow instead of migrating into stop() and silently leaking the direct path.

The rest of the PR is unchanged since the prior review (the 80fcca2 replay was a byte-identical rebase onto #19870). I confirmed the new test passes, that it fails if the wakeWaiters() call in fenceAndCloseNow is removed, and that oxfmt/oxlint are clean.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Picks up the base's changed-code casting-gate fixes.
Four of the eleven findings were removable rather than suppressible: annotate
the variable instead of asserting the literal. The rest carry a line-specific
SAFETY rationale on the line that opens the cast expression, which is where the
diagnostic points -- not the `} as Foo` that closes it.

Measured: gate exit 0, tc exit 0, relay 32 files / 251 tests passed.
The coordinator tracked "which reconcile is current" four ways: an integer
authEpoch, a Promise compared by identity (latestReconcile), a deferred woken
by wakeWaiters(), and RelayRetrySchedule.settled. The wait loop parked on
promise identity while authority was an integer, and the two were reconciled by
hand at each turnover site. Nothing structurally enforced "every turnover wakes
the waiters" -- it was a call you had to remember.

Now there is one field, `authority`, and one mutator, `turnover()`. Ending the
outgoing authority is what releases waiters, and it happens inside the same
operation that installs the new one, so a superseded wait is impossible rather
than prevented. Object identity is the epoch, so isEpochCurrent becomes an
identity check and the integer goes.

Behaviour is unchanged. Falling out of the shape:

- The fence installs an idle authority, so a caller arriving after a fence gets
  the cause now instead of parking behind the open the fence abandoned. Before,
  the fence left latestReconcile pointing at that open. New test.
- RelayRetrySchedule.settled is subsumed: a retry fires through beginReconcile,
  which turns the authority, which is what the waiter is parked on. cancel()
  still never resets the attempt counter.
- relay-live-broker-wait.ts and its six-closure LiveBrokerWaitSource seam are
  deleted. No test ever imported them; the seam existed to keep the coordinator
  under max-lines, which the smaller shape no longer needs.

Also drops the characterisation test that pinned the transient-ref owner-scoping
gap. A test that must be deleted before a bug can be fixed is worse than no
test; the gap stays documented on hasDemand, in four lines rather than fourteen.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

The delta since the prior pullfrog review is the single head commit e3dd6ae687, a refactor of the live-broker wait's authority model plus its test/comment fallout.

  • One authority object — Replaced four parallel signals for "which reconcile is current" (the integer authEpoch, the latestReconcile promise, the authorityChange deferred, and RelayRetrySchedule.settled) with a single authority field mutated only through a new turnover(). Object identity is now the epoch, so isEpochCurrent(epoch) became isCurrent(authority).
  • Wait parks on the authority — waitForLiveBrokerResult awaits the captured RelayAuthority (settled for completion/supersession, ended for an armed retry) and re-joins by identity, instead of the LiveBrokerWaitSource closure seam.
  • Fence installs an idle authority — A caller arriving after fenceAndCloseNow() is now settled with the current cause rather than parking behind the open the fence abandoned; pinned by a new test.
  • Extraction deleted — relay-live-broker-wait.ts and LiveBrokerWaitSource are gone; LIVE_BROKER_WAIT_BUDGET_MS moves to relay-auth-coordinator.ts.
  • Retry schedule simplified — RelayRetrySchedule loses armed/settled (a firing retry now wakes waiters by turning over the authority); cancel() still does not reset the backoff ladder.
  • Coverage trimmed — The transient-ref owner-scoping characterisation test is removed and the hasDemand KNOWN GAP comment compacted to four lines.

I traced the new state machine against every mutation site (stop, fenceAndCloseNow, beginReconcile, the retry timer callback, ensureLive, refreshAccessToken): turnover() is the only writer of authority and the only caller of RelayAuthority.end()/RelayRetrySchedule.cancel(), so any turnover that supersedes a parked authority resolves the exact promise the waiter holds — no lost wake, no spin past the deadline check, and no stale result. The retry-schedule suite dropped settled coverage in step with the API it removed, and the coordinator-level wake paths remain pinned in relay-concurrency-broker-wait.test.ts. Verified at head: full relay suite (32 files / 249 tests) passes, typecheck:node clean, check:code-quality:changed and check:max-lines-ratchet clean.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant