feat: conditional controller ownership for terminal attachments - #84
feat: conditional controller ownership for terminal attachments#84jiashuoz wants to merge 50 commits into
Conversation
At most one controller per session, everyone else a viewer, handoff explicit and atomic, and every input fenced by a generation at the PTY rather than at the plane. Records the durable lease, the negotiated handshake, the compatibility rules for the three separately deployed parties, and what each of them is verified by. Co-Authored-By: Claude <noreply@anthropic.com>
…port The controller generation exists but nothing conditions on it: every attach advances it and no caller reads it back, so control follows whoever attached last. This is the durable half of fixing that, and it changes nobody's behaviour yet. control.SessionRepository gains two methods. CompareAndAdvance- ControllerGeneration advances a session's generation from an expected value in ONE predicated statement, which is the whole handoff guarantee: two devices claiming from the same generation cannot both match, so one gets the advance and one gets ErrStale. It vacates the lease in the same step, which is why there is no separate release primitive — a controller that leaves advances the generation, and that both frees the lease and fences the bytes it had already sent. RenewControllerLease installs or extends the lease, fenced by generation and holder, and is both a claim's first act and its heartbeat afterwards. The lease itself is two columns beside the generation (0013): an opaque per-attach holder from crypto/rand — never a user, device or account identifier — and an expiry. Expiry is passive and there is no sweeper: a lease whose expiry has passed is simply not live, so a client that dies without releasing cannot wedge control. Existing rows come out vacant and claimable from the generation they already had. controlapp/repotest gains the case that is worth having: eight concurrent claims from generation 7, exactly one 8, seven stale answers, and a row that is still at 8 afterwards. It is concurrent because the sequential version passes for a read-then-write implementation every time — checked by writing one, which fails this case on every adapter. Also fixes freshDB in the pgstore suite, which rewrote the database in RAINIER_TEST_PG_DSN by replacing the literal "/postgres?". Anyone following the README and pointing it at a database of their own got the admin DSN back for every "fresh" store, so unrelated cases shared one database and failed on a perfectly good server. Co-Authored-By: Claude <noreply@anthropic.com>
…e pty The attach command can now ask for control conditionally, and the answer travels all the way down to where input executes. Nothing negotiates yet — the plane still grants no binding — so this changes nobody's behaviour; it builds the thing the next commit switches on. protocol/terminal gains the conditional-ownership vocabulary: the three attach-URL parameters a client advertises with, the claim/release and attached/stale/control_changed messages, and the control/control_ack pair that installs a binding in a sandbox and confirms it. Generations travel as decimal strings, because a uint64 past 2^53 is silently wrong in the browser that renders the next consumer of this protocol. Every field is omitempty and pinned by a test: a peer that negotiates nothing writes the bytes it has always written. control.AttachTerminal gains Negotiated and ExpectedGeneration, both optional. An unnegotiated controller attach still advances the generation unconditionally — a client that cannot be told it is a viewer cannot be made one without breaking it — and that advance is exactly what fences and notifies a negotiated client attached at the same time. A negotiated attach claims only when control is free, or only from the generation it presents, and lands in AttachmentViewer otherwise, because "somebody else is typing" is an answer and not a failure. AttachTarget carries the granted mode and a ControllerLeaseKeeper, so a broker can run a handoff without ever being handed a repository. The binding rides the frame that OPENS an attachment — dial_attach, then the relay FrameOpen — so a sandbox installs it before it queues a byte of screen, with no acknowledgement to order against. A mid-attach handoff arrives as a control message and is answered with a control_ack, which is what will let the plane refuse to tell a taker it has control until the fence protecting it is actually in place. The fence itself is in internal/session, at the process, because the plane is not where input executes: a keystroke accepted from a controller that was displaced a moment later can already be past it. A bound attachment writes only while it is the controller at the session's current generation and its frame was stamped with that same generation; an unbound one writes unconditionally, which is the compatibility rule for an older plane and is safe because under the old message set only one client could have been sending. The pty follows the controller, so a phone watching a laptop's session no longer squeezes it to phone width. Co-Authored-By: Claude <noreply@anthropic.com>
…the rest
The plane now carries the decision the application makes. A client that
advertises the capability on its attach URL is told its mode and
generation before the first snapshot or output byte, and can claim and
release control on the stream it already has. A client that advertises
nothing receives exactly today's message set.
A claim is not answered until the sandbox has confirmed the new
generation. Two attachments reach one sandbox over one conn but on
independent paths, so the plane cannot order a displacement against a
keystroke the previous controller has already sent; waiting for the
control_ack means every frame from the old controller that arrives after
the answer is fenced, and every frame that arrives before it arrived
while the handoff had not happened for anybody. A sandbox that predates
this protocol never acks, so the wait is bounded and the handoff
proceeds without it — a new plane must require nothing an old one cannot
supply.
The displaced controller is told twice over. On this replica the plane
pushes control_changed at once; anywhere else — and as the backstop
everywhere — its own heartbeat renewal stops being accepted within one
interval and it demotes itself. Its fencing is immediate either way,
because the generation moved before either notice was sent. A clean
detach releases, which advances the generation, so the next attach
claims with no click and the leaver's already-sent bytes stop being
executable.
The plane also stamps every client frame it forwards with the generation
that attach holds — a legacy client's included, since the client cannot
stamp its own. That is what makes an unstamped frame reaching a sandbox
mean an older PLANE and nothing else, which is the compatibility rule
the sandbox's fence relies on.
The read model gains one additive object, `controller {generation,
held}`. It names nobody: a client learns that somebody has control,
never who or on what device. The generation is a decimal string there
for the same reason it is one on the wire.
Co-Authored-By: Claude <noreply@anthropic.com>
`rainier attach` keeps its default and its zero clicks: on one laptop it claims control and behaves byte for byte as it always has. On a second device it attaches as a viewer and prints one line naming that another device has control and the key that takes it. --view never claims; --take claims once on attach, and once is the whole of it. Ctrl-\ takes control, beside the existing Ctrl-] detach, and is intercepted ONLY inside an attach a server actually answered. Against a plane that does not speak conditional ownership it is forwarded as an ordinary byte, so a user attached to an older deployment keeps every key they had. Ctrl-] releases as it detaches, so the next attach needs no key at all. Reconnect presents what this device actually held. A controller presents its generation and resumes only while nobody took it; a device that was superseded while it was away comes back a viewer and says so. A viewer stays a viewer rather than quietly acquiring control because a network blip happened to free it. Nothing claims on its own anywhere: --take is spent by the attempt that used it, and a refusal is answered with one line rather than another claim. A client that asks for conditional ownership and is never answered settles as an ordinary attach the moment terminal traffic arrives — it types, stamps no generation, and stops eating Ctrl-\. That is the new client + old plane pairing, and it is a test. `rainier info` gains a Controller row: this device, another device, or none. It needs no new wire field and discloses nobody: the API says whether somebody holds control, never who, and "this device" is worked out from the generation this CLI was last granted — nobody else can hold a generation without advancing past it. Co-Authored-By: Claude <noreply@anthropic.com>
docs/terminal-controller-ownership.md is the operator- and integrator- facing note: the rule, the parameters and why each is the number it is, the handshake and the message set, where the fence lives and why it is at the pty rather than at the plane, and the four compatibility pairings in a table — including the rule that input with no generation is treated as the current controller's, which is safe only because under the old message set no second client could have been sending. The CLI contract gains the same facts additively, in the sections that already own them: attach's ownership behaviour and its two new flags, info's Controller row, and the session document's new `controller` object. Nothing already documented changed. Co-Authored-By: Claude <noreply@anthropic.com>
Review found that the plane's client pump forwarded any message type it did not recognise verbatim onto the sandbox socket — and `control`, the verb that installs a mode and a generation at the pty, is one of them. A viewer could therefore promote itself, or name a generation past every generation the session will ever reach and leave nobody able to type for the life of the process, since the pty's fence only ever goes up. The plane now drops a client's `control` and `control_ack` rather than carrying them; its own handoff writes them straight onto the runner socket and never goes through that pump, so this costs the handoff nothing. Behind it, at the pty: an attachment that was OPENED unbound is never bound later. A binding travels on the frame that opens an attachment — that is what leaves no window between the size and the binding — so a handoff arriving on an attachment that never had one did not come from a plane at all. It came from whatever is on the other end of an unmanaged attach socket, such as a runner's local debugging endpoint, which has no control plane above it. Both fences have a regression test that fails without them. Co-Authored-By: Claude <noreply@anthropic.com>
Four things review found in what one attach is granted. A dropped connection cost the controller its own session. The plane releases whenever an attach ends — however it ended — and a release advances the generation, so a device dialling back presented a generation the row had already moved past and came back a VIEWER of a session nobody was controlling, told that another device had control. It also latched: the client then asked for `mode=view` on every later retry. Presenting a generation now decides only whether to claim under a live lease, not what to claim from; a lease that is vacant or expired is claimed by whoever asks, which is the same rule a first attach gets. A claim whose first lease renew came back stale was answered with success. A stale renew means the generation the claim just won has already been advanced past, so two devices were told at once that they had control. It is now a lost claim. A renew that fails for any OTHER reason still stands: it failed at a generation that is still this attach's, and the generation is the authority while the lease is only the hint. A negotiated attach is authorized for the mode it can REACH, not the one it opens in. It can claim control at any moment on its own stream — that is the take-control key — so a host whose policy separates viewing from controlling would otherwise find `mode=view` a way around it. An unnegotiated viewer is still authorized as a viewer: it has no way to claim. And a broken entropy source fails the attach before anything is claimed, rather than advancing the generation and displacing whoever had control on its way to reporting the failure. Each has a regression test that fails without it. Co-Authored-By: Claude <noreply@anthropic.com>
A viewer was never told the generation had moved. It kept the number it saw when it attached, so its first press of the take-control key claimed from a generation that no longer existed and was answered "somebody else got there first" — about a session nobody was using, after the controller had simply walked away. Only the second press worked. `displace` now announces the generation to every attach behind it: a controller is demoted, and a viewer is told the number, which its client reads silently. A release and a departing attach announce it too. A take-over that IS an attach answered the taker before it had displaced anybody, where a mid-attach claim deliberately waits for the sandbox to confirm the new binding first. The narrow window between them is exactly the one the whole acknowledgement exists to close, so an attach now keeps the same order: displace, wait, then answer. Two handoffs in flight on one attach — a claim from the client pump and a demotion from the heartbeat — shared one acknowledgement channel and could consume each other's answer, leaving one of them to wait out the whole timeout. They are serialised now, and a leftover acknowledgement is dropped rather than mistaken for an answer. A demotion whose generation read fails falls back to the generation the attach already held rather than to zero, so the common case, a read that merely timed out, still leaves the client able to take control in one press. Also adds the plane-side test for a pairing the docs claim and nothing exercised: a legacy attach displacing a negotiated controller, which is what makes admitting a legacy client as a take-over safe for the device it took control from. Co-Authored-By: Claude <noreply@anthropic.com>
…ts size Review found `--view` unenforced on the client: the flag reached the attach URL but not the client's own idea of its mode, which stayed empty until a server answered. Against a new plane that was a window of wasted bytes. Against a plane that predates conditional ownership — the pairing the compatibility matrix exists for — it was the opposite of what the flag says: that plane admits every attach as an unconditional controller, so `--view` took control from whoever had it and its keystrokes executed. The mode is now held locally from the first byte, and an attach that asked to watch is not told "another device has control", which on that plane might not even be true. `--take` spent its one claim only if the attach opened as a viewer. An attach that opened holding control left it unspent, so it fired minutes later, when somebody else took control — a snatch-back nobody pressed a key for. The first answer spends it, whatever the answer said. And gaining control now says how big this terminal is. A viewer's resizes are suppressed on the way out, so the size the session holds for a watching attachment is the one it had when it attached; without this, taking control snaps the pty to a size several window changes old. Each has a regression test that fails without it; the size one drives a real pty, because a pipe has no size to report. Co-Authored-By: Claude <noreply@anthropic.com>
`controller.held` on the session view asked the wall clock while the attachment service measured the same lease against the composed one, so nothing could drive an expiry through the JSON in a test — and a replica whose clock the services trust would have had a read model that did not. The clock is now the Server's, shared by both. Adds the missing test for the one hop between "what the client asked for" and "what the application granted": the attach handler reading the three negotiation parameters off the URL and putting them on the command. Both ends were well covered and the wire between them was not, so a regression that dropped a field would have returned every attach to unconditional semantics silently. It walks a real websocket through a real runner to a real relay, and pins the mode and generation on both the client's answer and the frame that opens the sandbox's attachment. Co-Authored-By: Claude <noreply@anthropic.com>
Review's mutation probes found three places where the code was right and nothing would have noticed if it stopped being. The generation on an incoming FRAME, as distinct from the generation on the attachment's binding, was load-bearing in exactly one scenario and had no test for it: a controller that was displaced and then took control back holds a binding at the current generation, so the rule about its binding says yes, and the only thing between the shell and a keystroke it typed two generations ago is that the frame carries the generation it was sent under. The additive wire promise was pinned for `protocol/terminal` and for nothing else, though the binding also crosses two hops whose ends ship in different artifacts: the `dial_attach` a control plane sends a runner, and the relay frame that opens an attachment inside the sandbox. Both now have a golden for the unbound shape and a round trip for the bound one. Which of the two generations on a forwarded frame the sandbox reads — the client's or the plane's — was neither documented nor tested. The plane's wins, and a fence a client could write its own value into would be no fence at all. Also: a create no longer keeps a caller-supplied lease in the in-memory store, where the Postgres one always dropped it, and the conformance suite now says so on both; and the plane's test logger stops writing to a finished test's `t`, so a leaked pairing's fifteen-second TTL cannot panic the binary out from under every other test in a long run. Co-Authored-By: Claude <noreply@anthropic.com>
…something Review found the design doc describing an earlier shape of the wire — the capability as a field on the first `resize` rather than three query parameters — and stating the pty's compatibility rule as a conjunction that, implemented, would have broken the pairing it exists for. Both now match the code, along with the fourth method on the lease keeper and the package that actually owns the reconnect tests. "Its fencing is immediate either way" was stronger than the truth. The generation moves the instant a take-over commits, and on this replica the plane waits for the sandbox to confirm the new binding before telling anybody they have control; across replicas the sandbox learns it from the taker's own opening frame or the displaced controller's next heartbeat. All three are now written down separately, because they are different guarantees. Three things a reader would otherwise have had to discover: a legacy client displaced by a negotiated one is fenced with no notice it can render and no key that takes control back, which is the same rule that protects the negotiated client running in the other direction and is what the CLI being tagged last is for; presenting a generation is a take-over request rather than only a resume; and a negotiated attach is authorized for the controller it can become, so `mode=view` is not a way around a policy. Co-Authored-By: Claude <noreply@anthropic.com>
|
A third, independent review of this head (Opus, read-only, fresh clone, mutation probes on a throwaway copy). The fence at execution, adapter atomicity and monotonicity, the four compatibility pairings, and wire additivity all verified clean; details in the session that will apply the fixes. The findings, verbatim: 1. should-fix — Consequence on the adapter that actually ships to Cloud: a legacy (unnegotiated) attach — the one path that still uses this primitive, and the whole point of the "old client + new plane" row — advances the generation and leaves the displaced controller's holder and future expiry on the row. For up to Fix: add 2. should-fix — a claim that is still waiting for its sandbox ack is not protected against a second claim, so two attaches are both told Interleaving: A claims (CAS 1→2) and blocks in (probe: two viewer attaches with a non-acking sandbox and Fix: replace 3. should-fix — Fix: authorize the attach at the mode it opens in, and move the controller check to the claim path — i.e. a second 4. nit — a negotiated controller attach that dies at the pairing stage has already displaced the incumbent. 5. nit — two dead fields. 6. nit — no bound on claim frequency. A client with control can loop Merge recommendation: merge after fixes 1, 2 and 3.** The fence at execution is correct, genuinely independent of the plane, and proven so by mutation; the adapters are atomic and monotonic; the compatibility matrix is honestly tested. But #1 makes the two shipped adapters disagree about the primitive the legacy-client pairing runs on, with the conformance suite — the thing the Cloud port will be graded against — silent on it; #2 lets two devices be told they hold control at once, which is the invariant the branch exists to establish, and is reproducible in a few hundred milliseconds; #3 denies A fix session has been dispatched with these findings and will post its dispositions here. The Cloud draft (rainier-cloud#90) will be re-pinned to the fixed head. |
A third independent review of the pushed head found three should-fix defects and three nits. All six were verified against the branch before anything was written; none is refuted. The design records what each one costs — a legacy attach that leaves the displaced holder's lease on the row in Postgres and nowhere else, a claim that can announce control it has already lost, a view-only principal locked out of a session it may watch — and what closes it, including the two behaviours that are documented rather than changed and why. Co-Authored-By: Claude <noreply@anthropic.com>
NextControllerGeneration is the unconditional take-over — the primitive a
legacy attach uses, the "old client + new plane" row of the compatibility
matrix — and it displaces whoever held control. memstore cleared the
holder and the expiry; pgstore advanced the counter alone. The port said
nothing either way, and the conformance suite exercised the primitive and
the lease but never together, so both adapters passed it with opposite
behaviour.
On the adapter that ships, a legacy attach therefore left the DISPLACED
holder's identity and future expiry on the row. For the rest of the 30s
TTL the session view reported a live lease held by somebody with no
authority, and the next negotiated controller attach read that lease and
was admitted a viewer — where memstore gave it control.
The port now says vacating is part of the contract, pgstore's UPDATE
clears both columns, and controlapp's test fake does the same.
repotest S13 gains the case that would have caught it: install a live
lease, advance unconditionally over it, and require the row back vacant
and the displaced holder's heartbeat refused. It FAILS on pgstore without
this fix ("an unconditional advance left the displaced holder's lease
behind") and passes on memstore, which is the divergence itself.
Co-Authored-By: Claude <noreply@anthropic.com>
…control
A claim advances the generation in the store and then waits, for as long
as the acknowledgement timeout allows, for the sandbox to confirm the new
binding. A second claim can win inside that wait. The first one then woke
up, wrote (control, its own generation) over its own displacement, and
told its client `attached mode=control` — so two devices printed
[you have control] and one of them had none.
The pty fence held, so nothing double-executed; what broke is the rule the
whole design has, that at most one client is the controller at any moment.
Until the next heartbeat (<=5s) the displaced client's keystrokes were
forwarded, stamped with a superseded generation, and discarded by the pty
while its screen said it was in charge.
`set` was an unconditional write and `displace` read a peer with `get` and
wrote it with `set` outside one critical section. Three named transitions
replace it, each doing its read and its write under one hold of the lock
it belongs to:
advance monotonic; refuses a generation older than the one held. A
refused claim answers `stale` with the generation that exists,
and re-points the binding it installed at what this attach
actually is, so the sandbox agrees with the plane.
demoteTo always a viewer, at the newer of the two generations. It
cannot refuse: being wrong about the number is survivable,
believing you still have control is not.
displaceTo the "already at or past this generation" check and the write,
together, under the peer's own lock.
TestAClaimSupersededWhileItWaitedIsNeverToldItHasControl makes the
interleaving deterministic rather than raced — the first attach's sandbox
predates the protocol and never acknowledges, the second's answers at once
— and FAILS without this fix with "a claim superseded while it waited was
told it has control at generation 2".
Co-Authored-By: Claude <noreply@anthropic.com>
authorizedAs returned AttachmentController for ANY negotiated attach,
including mode=view. A host policy that grants viewing without granting
driving — the Cloud collaboration policy this seam exists for — therefore
refused `rainier attach --view` outright, locking a view-only principal
out of a session it may perfectly well watch.
It also left the edge asking a different question from the service: the
pre-upgrade check in handleClientAttach uses the mode the client asked
for, so under such a policy the caller would get a 101 upgrade followed by
a policy-violation close instead of the clean 403 that function exists to
give. Nothing failed in tree only because ownerOrAdmin ignores the mode.
An attach is now authorized for the mode it opens in, and the privilege it
might REACH is asked for where it is exercised: controllerKeeper.Claim
asks AuthorizeAttachment(..., AttachmentController) before it touches the
store, live rather than cached, so a grant revoked mid-attach is honoured
at the next press of the take-control key. The escalation that check
replaced stays closed — a mid-attach claim still cannot reach control the
caller was never authorized for.
That makes `Controller != nil` the wrong carrier for "this client
negotiated", because a view-only client must still be TOLD its mode and
generation while never being admitted a controller. AttachTarget carries
the two facts separately, Negotiated and MayClaim, and the plane refuses
an unauthorized claim without leaving the replica.
Two regression tests, both FAILING without this fix:
TestAViewOnlyPrincipalWatchesAndMayNotClaim ("a negotiated view attach
under a controller-denying policy: control: denied")
TestAViewOnlyAttachIsToldEverythingAndClaimsNothing ("no stale message
reached the client; saw [attached]")
Co-Authored-By: Claude <noreply@anthropic.com>
`ownership.legacy` was written by settleLegacy and never read anywhere. Removing it leaves settleLegacy with nothing to do, which is correct rather than surprising: never having settled IS the legacy state, and it is what every behaviour that state implies already reads — stamp returns zero, keyActive is false, mayType is true. The function and its two call sites go with the field, and the comment that explained the state stays where the traffic arrives. `fakePlane.speaks` was never consulted in serve, so newFakePlane(false, …) and newFakePlane(true, …) built identical planes and the flag could not fail a test. The "old plane" in TestNewClientOldPlane is expressed by its script containing no ownership message, which is faithful — that is exactly and only what an old plane is on the wire. No regression test is constructible for removing code nothing reads. The behaviour the field pretended to record is already pinned end to end by TestNewClientOldPlane, which drives a real socket and asserts that an unanswered client types, stamps no generation, sends no claim, prints no ownership notice, and forwards Ctrl-\ to the remote application. Co-Authored-By: Claude <noreply@anthropic.com>
Three behaviours the code has and the operator doc did not say. A take-over commits before the terminal exists: the binding rides the dial_attach, so a negotiated controller attach displaces the incumbent before the runner has been asked to dial back, and an attach that dies at its pairing TTL leaves the previous controller a viewer of a session nobody controls. One press of the take-control key takes it back, because the displacement announced the generation. Reversing the order is not available — until the runner dials back there is no socket to install a binding on — so it is written down rather than restructured. Nothing bounds claim frequency. A client with control can loop claim/release on its own stream, and each claim waits serially on every peer's sandbox. It is an authorized user's nuisance against their own session, and it is deliberately not rate limited: a limiter would also refuse the legitimate rapid hand-back after a mis-press. And what a host policy that separates viewing from driving now gets: the attach authorized at the mode it opens in, the claim authorized on its own and live. Co-Authored-By: Claude <noreply@anthropic.com>
Review found the same lie one step further along than the last fix
reached. A claim answers itself only after it has displaced every peer on
this replica, and that loop waits on each displaced peer's SANDBOX, once
per peer and up to the acknowledgement timeout each. A second claim can
win inside it, and the mode read before the loop is not the mode this
attach holds after it. The attach path does the same thing: the broker
reads the granted mode, displaces, and then sends the value it read.
The result is worse than the window closed last commit, because nothing
corrects it. A client that believes it is the controller sends no claim of
its own, so Ctrl-\ does nothing, and this attach's heartbeat renews
nothing because the plane knows it is a viewer — so no stale renewal ever
demotes it. Its screen says [you have control] for the life of the attach
while the plane drops every keystroke.
Every message that tells a client what it is now goes out under one
`announce` hold that also does the state read: announceClaim,
announceOpening, announceViewer, announceStale. A claim asks one question
once, at the end — does this attach still hold the generation it won? —
which answers both waits, its own and the displace loop's.
announce is only ever taken alone: a claim finishes displacing its peers
before it announces anything about itself, and a displacement takes the
peer's announce and no other, so two attaches cannot each hold one and
wait for the other's.
Two regression tests, each FAILING without its half:
TestAClaimSupersededWhileItDisplacedIsNeverToldItHasControl ("a claim
superseded while it displaced its peers was told it has control at 2")
TestATakeOverAtAttachTimeIsToldWhatItIsAfterItDisplaced ("an attach
displaced before it was ever told anything was told it has control")
Co-Authored-By: Claude <noreply@anthropic.com>
finish read the mode and the generation in two separate critical sections,
with a context being built between them. release, ten lines above, already
reads both in one step; that inconsistency was the tell.
This attach's own heartbeat demotion can complete between the two reads.
A demotion holds `control` for the whole of its bounded wait on the
sandbox and only then writes the CURRENT generation — the one belonging
to whoever took over, possibly on another replica. So the first read says
"still the controller" and the second returns the winner's generation, and
the release advances past a device that legitimately has control, on its
own generation, from an attach that is walking out of the door. That
device's screen says [you have control] while every keystroke is fenced at
the pty, until its next heartbeat notices.
One read, as release does.
TestFinishReleasesOnlyTheGenerationItActuallyHeld races the demotion
against the disconnect 500 times and asserts both halves: no release at a
generation this attach never held, and the live controller still holding
its lease. It FAILS without the fix ("a departing attach released
generation 2, which it never held").
Co-Authored-By: Claude <noreply@anthropic.com>
sendStale started at zero and only moved off it if the store read
succeeded. demote does the opposite on purpose, and the design doc spends
a paragraph on why: it falls back to the generation this attach already
holds, because being told zero is worse than being told a stale number.
Zero is a generation no row is ever at. A client told it presents zero on
every later claim, and a predicated advance from zero can never match a
non-zero row — so every subsequent press of the take-control key is
refused, for the life of the attach, because one store read timed out.
That is the store being briefly unusable, which the heartbeat's own
comment calls expected and survivable.
sendStale now seeds from this attach's own state, like demote and like the
policy refusal a few lines above it.
TestARefusedClaimIsNeverToldGenerationZero drives a claim whose generation
read fails and FAILS without the fix ("a refused claim whose generation
read failed was told 0, want the 7 this attach holds").
Co-Authored-By: Claude <noreply@anthropic.com>
Review raised three consequences of the new AttachTarget fields.
A composer that predates Negotiated sets the keeper and nothing else, and
still compiles. Reading Negotiated literally would silently stop telling
that client its mode, its generation and every handoff — indistinguishable
from an older plane, from the client's end. A keeper has always meant
exactly "this attach negotiated", so the broker still reads it that way.
MayClaim is read the other way, because it is a privilege that needs a
keeper to exercise: never inferred, never wider than the target carries.
The contract now states both invariants and how a target that breaks one
is resolved.
A negotiated claim is also always ANSWERED. Returning in silence, which is
what a negotiated target with no keeper used to get, leaves the
take-control key doing nothing at all — the one outcome a client cannot
tell from a broken connection.
And the attach-time grant no longer re-asks the policy. It is the one
claim whose question was answered microseconds earlier on the very same
mode, so asking again doubles a Cloud collaboration-grant lookup on the
attach path, and a backend that blinks between two identical calls would
fail the attach ErrDenied — reporting a dependency outage as "not
authorized to attach to this session" after the door said yes. The claims
a client makes on its own stream are still asked every time, which is what
honours a grant revoked mid-attach.
Three regression tests, each FAILING without its fix:
TestATargetThatPredatesTheNegotiatedFlagIsStillToldEverything
TestANegotiatedClaimIsAlwaysAnswered
TestAnAttachAsksTheHostsPolicyOnceAtTheDoor ("the policy was asked
[controller controller]; want exactly one controller question")
Co-Authored-By: Claude <noreply@anthropic.com>
…annot fail Adversarial review ran mutation probes over the new code and found five that survived the whole tree. Each one is a property this change was written to establish, and each is now pinned: advance accepts the generation this attach ALREADY holds. A departing peer's release announces the current generation to everybody behind it, which can set a still-waiting claimant to view at exactly the generation it just won; refusing that would tell the winner "somebody else got there first" about a generation it owns and holds the lease on. demoteTo takes the max. A demotion reads the current generation and can be overtaken before it writes; walking the number down would leave the client claiming from a superseded generation, refused every time. displaceTo refuses a generation this attach has passed, and does its check and its write as ONE step. The second is the half of the original defect that had no test at all: split apart, a claim landing between the read and the write is clobbered back to a viewer at an OLDER generation while its client has already been told it has control. It is asserted as an invariant over 2000 rounds rather than as one reproduced interleaving, and it fails immediately against the split version. A refused claim re-points the binding it installed. The existing test read the client stream only and never looked at the loser's sandbox. The fifth mutation — displace's `gen == 0` early return — could not be killed, because it is unreachable: displaceTo now refuses zero for every attach, since every attach is at or past it. The guard is removed and the reason written down; the behaviour it guarded is pinned by TestDisplaceAtGenerationZeroTouchesNobody, which passes either way and says so. Test fixtures renamed to the repository's four-letter form (att_aaaa). Co-Authored-By: Claude <noreply@anthropic.com>
…nting one bit twice Four things review found written down wrongly or not at all. ControllerLeaseKeeper.Claim enumerates its errors and did not mention the ErrDenied it now returns when the host refuses this attach the controller privilege. That is the same divergence class the pgstore lease fix was about — an adapter behaviour the port does not state — so the port states it, including that a broker treats it as "still a viewer" and not as a transport failure. The neighbouring sentence about a nil keeper meaning "the host does not negotiate" now points at the field that carries it. Two more SessionRepository fakes still advanced the counter and left the lease behind: controlapp's session stub, which models the lease correctly in its other two methods and was internally inconsistent in exactly the way pgstore was, and the fleet fake. No test observes the difference today; the next one to try would. The operator doc claimed a view-only client's take-control key is "answered 'you are still a viewer'". It is answered `stale`, which the CLI renders "[somebody else got there first; press Ctrl-\ to try again]" — wrong about the cause and suggesting a key that cannot succeed for that principal. The doc now says what actually happens, and why the two refusals share one message: telling them apart needs a message the wire does not have, and the compatibility matrix is the reason not to add one in a fix. Operators are told to say "read-only" themselves. And attachio's `settled` and `answered` were provably the same bit — one assignment sets both, nothing else writes either — with two comments asserting they mean different things. One bit, both meanings. The extended conformance case also drops a follow-up renew that read like a second vacancy check and was not one (it failed on the generation mismatch alone). What replaces it is the operational consequence of vacancy: a different holder can install a lease at the new generation at once, which the holder predicate only permits over an empty holder. Co-Authored-By: Claude <noreply@anthropic.com>
The design said `advance` closed the window a claim opens on itself. It closed the first of two: the displace loop is the longer one, and the attach path has it as well. It also did not know about `finish`'s split read or `sendStale`'s fallback to a generation no row is ever at. The approach section now describes the one place that says what an attach is, and the verification list names the cases that pin it — including the five mutations that survived the whole tree before this round. Co-Authored-By: Claude <noreply@anthropic.com>
|
A fourth independent review (Opus, read-only, fresh clone, 14-mutant battery: 12 caught) of head 42475de. The state machine itself is verified correct under every interleaving it could construct; what remains is the layer around it. Findings, verbatim, and the verification table: Findings1. BLOCKER — one peer that stops reading freezes every handoff on the session; no write is bounded anywhere in
|
| Transition | Where | What makes it safe |
|---|---|---|
| own claim accepted | ownership.go:303-318 |
store CAS CompareAndAdvanceControllerGeneration (controlapp:212) — exactly one winner per expected; advance under o.mu (:116-124); answer read inside o.announce (:328-339) |
| own claim stale | :306, :385-393 |
sendStale falls back to o.gen, never 0 (pinned; d273a9f mutant caught). Hole: no mode check → finding 2 |
| displaced by peer claim | :563-587, :151-159 |
displaceTo does check+write under one hold of the peer's o.mu; refuses a generation the peer has passed (TestDisplaceToAndAdvanceAreEachOneStep, 2000 iterations) |
| displaced by legacy attach | plane.go:174, controlapp:305 |
NextControllerGeneration advances and vacates the lease (61624cb); the legacy client gets no notice — documented tradeoff, docs/terminal-controller-ownership.md:122; probed and confirmed |
| own release | :403-413 |
Release is a CAS (controlapp:252), so a release that lost changes nothing; generation advance is what makes the fence real |
| own finish (socket close) | :477-498 |
one o.get() at :485 + CAS release; TestFinishReleasesOnlyTheGenerationItActuallyHeld catches the real regression |
| heartbeat renew / expiry | :442-468, controlapp:229-242 |
RenewControllerLease fenced on generation and holder; only ErrStale demotes, transient errors retry inside the 6-renews-per-lease budget |
| reconnect with expected generation | controlapp:332-343 |
one read, then CAS from that read; racing attaches produce one winner (TestTwoDevicesRacingFromOneGenerationHaveOneWinner) |
| sandbox ack timeout | :239-266 |
handoff serialises the pairs; bounded wait; proceeds fenced at the plane alone. Dropping the wait fails 5 tests (mutated and confirmed) |
| two claims, either order | probe | 4 attaches racing one generation, -race -count=4: always exactly one controller, at the store's generation, no attach above it |
| close racing a peer claim | probe | 30 rounds -race: the taker is always told something true, never a generation above the store's |
| pty fence (sandbox side) | internal/session/session.go:mayWriteLocked, observeLocked |
bound attachment writes only when bind.Mode==control && bind.Generation==controllerGen && frameGen==controllerGen; controllerGen only rises; Bind refuses an attachment opened unbound |
| lock discipline | — | owners.mu is released before the displace loop; every critical section holds one attach's lock at a time; order is always announce→mu or handoff→mu, never inverted — no cross-attachment cycle. Exception: announce across a socket write (finding 1) |
| lease keeper lifetime | splice.go:62-64 |
one heartbeat goroutine per splice, one context.CancelFunc, no Stop API to double-call; no goroutine leak (-race clean) |
Answer to the question asked: the state machine itself is correct — the store CAS, the single-step advance/displaceTo, the handoff serialisation and the pty fence hold under every interleaving I could construct or stress. The residue is not the state machine; it is the layer around it. Two of the four announcement paths still assert a mode instead of reading it (the exact defect class 418a8aa fixed on the other two), demoteTo is the one transition with no backwards guard, and the handoff does unbounded serialised I/O while the taker waits. That last one is not fragility, it is a liveness bug that any single paused client triggers.
Merge recommendation: do not merge until 1 and 2 are fixed; 3 and 4 should land with them. Not a redesign — the state machine does not need one — but finding 1 means one paused browser tab freezes control handoff for an entire session, which is precisely the multi-device scenario this PR ships, and finding 2 is the same review defect recurring on the paths the last round did not touch. Folding the four announcement paths into one state-reading announceAs and bounding the displace loop removes the class rather than the instance, which is what the repeat findings in this module are telling you to do.
A fix session has been dispatched with the instruction to remove the defect class (one state-reading announce function, a bounded fan-out displace, a backwards guard on demote, viewer fallback for view-only principals) rather than patch instances. It will post dispositions here and re-pin rainier-cloud#90.
A fourth independent review found one liveness bug (a single peer that stops reading freezes every handoff on the session), the third round's announcement defect recurring on the two paths it did not touch, the one transition with no backwards guard, and an authorization split the first-party CLI cannot reach. This records what each one is, what it costs, what closes it, and the one behaviour that changes on purpose: a view-only principal's plain `rainier attach` becomes a viewer instead of a 403. Co-Authored-By: Claude <noreply@anthropic.com>
…t first Four paths announced a client's ownership and two of them asserted a mode instead of reading it: announceViewer hard-coded `view`, announceStale hard-coded `stale`. Both could therefore tell a live controller it is a viewer. That is the defect 418a8aa fixed on the other two paths — the third time this shape has been found in this module — so this removes the class rather than the instance: there is now ONE announcement function, announceAs, it takes the announce hold, reads (mode, gen) under it, and reports what it read. No caller supplies a mode. announceClaim, announceOpening, announceViewer and announceStale are gone. Two decisions move inside that hold with it. A claim that no longer holds what it won is answered the way a lost race is, and a `stale` that would reach a LIVE controller is sent as `attached control` instead — the case that was reachable with no race at all: a client reading controller.generation and claiming from a value it no longer held was refused by the store, told stale, and stopped typing, while the plane kept forwarding for it and its heartbeat kept renewing its lease. Nobody could type for the rest of the 30s lease. `claim` now returns early for the current controller, which is the other half of that: re-claiming from yourself only advances the generation, fences your own in-flight keystrokes and costs every peer an acknowledgement timeout. The first-party CLI already refuses to send one; nothing at the plane did. Reading the mode at send time also fixes the release path's reordering, where a peer mid-claim was told "another device took control" a moment before "you have control". Tests, each failing without the change (mutations run and confirmed): TestAControllerThatClaimsFromAStaleGenerationKeepsControl and TestAClaimFromTheCurrentControllerNeverAdvancesTheGeneration drive it through the real plane; TestAStaleAnswerNeverTellsALiveControllerItIsAViewer and TestADisplacementNoticeReportsTheModeItReads pin the two announcement decisions directly. TestThePlaneStampsItsOwnViewOverTheClients now claims from a viewer, because a controller's claim no longer advances anything. Co-Authored-By: Claude <noreply@anthropic.com>
displace walked the session's peers serially and did two unbounded writes to each — the binding to that peer's sandbox and the notice to that peer's client — on an unbounded context, with no write deadline anywhere in the plane. One viewer that stops READING its socket (a closed lid, a TCP zero window, a paused browser tab; no RST, so nothing closes it) was therefore enough to freeze every handoff on the session: the taker's claim had already advanced the store, so nobody could type, and the taker was never told it had won, so its client stayed a viewer while its heartbeat renewed a lease it did not know it held. On the attach path it was worse — the displacement runs before the client socket is parked and before the dial_attach is sent, so one stuck viewer stopped a new controller attach from ever reaching its runner. The fix is the class, not the case. displace is now a bounded fan-out: each peer on its own goroutine, so a peer that cannot be reached is never in front of one that can, and each peer-facing step under Plane.step — one acknowledgement timeout, the same budget for a binding, its acknowledgement and a notice. install bounds its runner write with it, and o.send bounds its client write with it, so the announce mutex can no longer be held across a socket that is not draining. Nothing in the package writes to a peer without a deadline now; that is the property, not the individual call sites. The ordering the contract promises is unchanged: displace still returns only when the fan-out is done, so the taker is not told it has control until the displaced controllers' sandboxes have the new binding. What changed is that the bound is two acknowledgement timeouts in total rather than none at all, once per peer, in a queue. A displaced peer that cannot be reached inside it loses only the notice: its pty fence was in force the moment the store advanced, the plane stops forwarding for it before the fan-out starts, and its own heartbeat demotes it within one interval. ClientStream gets a write deadline of its own, so a socket that has taken nothing at all is eventually closed rather than held with a writer parked on it. It is deliberately far above any frame a live client on a slow link could be slow with; what it catches is a peer that is not draining. Tests, verified against the pre-fix shape: TestAStuckPeerDoesNotStallAClaim, TestAStuckPeerDoesNotStallANewControllerAttach and TestAStalledExControllerIsFencedEvenThoughItsNoticeCouldNotBeDelivered all fail (the claim is never answered, the dial_attach never leaves) and TestOneStalledPeerDoesNotHideAnothersDisplacement fails four runs in six, which is the map order deciding whether a stalled peer is walked first. TestAWedgedClientIsClosedRatherThanHeld does not fail without the deadline, it hangs — which is the production symptom. Co-Authored-By: Claude <noreply@anthropic.com>
… others
advance and displaceTo both refuse a backwards move; demoteTo was the
exception, and the justification written above it ("being wrong about the
number is survivable, believing you still have control is not") is true of the
NUMBER and was applied to the MODE.
A heartbeat demotion is decided at the generation this attach held when its
renewal was refused, and it then spends a store read and a bounded sandbox
wait in flight. An attach that wins a newer generation inside that window — a
peer takes control, this device takes it straight back — was demoted anyway:
the plane then says viewer while the store says this attach holds the lease,
so nobody can type, this attach's heartbeat keeps renewing a lease nobody can
use, and every negotiated attach behind it is admitted a viewer, for the rest
of the 30s lease. Which is the outcome the unconditional write was there to
prevent, arrived at from the other side.
demoteTo takes the generation the demotion was decided at and no-ops when this
attach has moved past it. demote reads that generation before the store call,
and now writes the plane's half of the fence FIRST (one locked write that
cannot fail, and it stops this attach being forwarded for at once), telling
the sandbox only when the demotion still stands — installing a viewer binding
for an attach that has since won a newer generation would fence the controller
it just became.
TestADemotionThatWasSupersededDoesNotDemote drives the five-step interleaving
through the real plane, holding the demotion still in the store read with a
gated keeper; TestDemoteToRefusesAGenerationThisAttachHasLeft pins the
transition. Both fail without the guard (mutation run: the controller is told
it is a viewer at the generation it holds the lease on).
Co-Authored-By: Claude <noreply@anthropic.com>
The mode-aware attachment policy was right at the service — an attach is authorized for the mode it opens in, and taking control later is a separate question — and unreachable from the first-party CLI. A plain `rainier attach` asks for `mode=control`, because zero-click on one laptop is what it has always been, so a principal a host grants viewing and not driving got "not authorized to attach to this session" for a session it may perfectly well watch, and had to know to type `--view`. That is the exact principal mayClaim exists for, and `grant`'s own rule — every refusal lands the attach in AttachmentViewer, never in an error — was not applied to the policy refusal. AttachTerminal now asks the policy for the mode the client asked for and, when a NEGOTIATED controller is refused, asks whether it may watch instead; if it may, the attach is admitted as a viewer and everything below asks about the mode it was admitted in. mayClaim then asks the controller question separately and gets the same no, so the store is never touched by its take-control key. The client is told `attached, view` before it paints a screen and prints the viewing notice, which is the thing the operator documentation could previously only ask hosts to tell such users out of band. An unnegotiated controller attach is still ErrDenied: it cannot be told it is a viewer, so admitting it as one would leave a terminal that silently does not type. internal/controld's pre-upgrade check mirrors it, so the two halves still ask the same question and a caller the service would admit is not answered 403 before the upgrade. It is not observable in self-hosted — ownerOrAdmin answers both questions the same way — which is why it is pinned at the service, where a narrower policy can be expressed. TestAViewOnlyPrincipalWatchesAndMayNotClaim previously pinned the refusal; it now pins the admission, the viewer grant, MayClaim staying false, the keeper still being handed over, the order the policy is asked in, and the unnegotiated attach still being refused. docs/terminal-controller-ownership.md says all of it, plus what a bounded parallel handoff means for an operator. Co-Authored-By: Claude <noreply@anthropic.com>
…sleeping Three soft spots the fourth review found in the suite rather than in the code. installAndWait's stale-acknowledgement drain survived deletion: no test covered it. It exists because the acknowledgement channel holds exactly one, so a previous handoff's late answer displaces the next handoff's — which then waits out the whole timeout for an answer it has already been given, while the person who pressed the take-control key looks at a terminal that will not type. TestAStaleAcknowledgementNeverCostsTheNextHandoffItsWait drives it with a sandbox that answers the instant the binding is written, which is what a fast socket does; deleting the drain makes it take the full two seconds. fakeSandbox.order was written and never read — the same kind of field b8916e2 removed from attachio. The two "no stdin arrived" assertions slept 100ms after a real synchronisation point, so a forwarding regression would only have failed probabilistically. awaitPumpCaughtUp replaces the sleep with a round trip through the same client pump: a claim from generation zero, which no row is ever at, so it is refused, nothing moves, and its answer proves everything queued before it has been processed. Co-Authored-By: Claude <noreply@anthropic.com>
Two regressions this round introduced, both found by review, both the same mistake in opposite directions: a bound that belonged to one caller was applied to everybody. o.send wrapped EVERY ownership message in one acknowledgement timeout, and ClientStream closed the socket whenever any context expired. A websocket serialises its writes, so a handoff's courtesy notice queued behind a snapshot the plane was replaying to that same client expired waiting for the write lock — and killed the attach. One take-over anywhere on the session disconnected a client that was merely reading a scrollback over a slow link. Now the caller's context decides: a message this attach is OWED (its opening `attached`, the answer to its own claim) is bounded only by the socket's write deadline, because dropping it leaves a client believing something untrue, while a message about somebody else's handoff carries one acknowledgement timeout, because it is a courtesy. ClientStream closes on its own budget and on nobody else's. The announce hold became a channel for the same reason: acquiring it has to be bounded too, or a peer's displacement queues behind a message this attach is writing to a client that stopped reading. And a claim whose binding never reached its sandbox took control anyway. The store CAS had succeeded, so the generation and the lease were this attach's; the pty discarded every keystroke, because that attachment's binding still said what it said before; and nothing could repair it — the heartbeat renewed happily, and this round's own idempotency guard then answered the client's next press out of the same wrong state. A claim that cannot be fenced at the sandbox is not a claim: the generation goes back, and the client is told what exists now. (A sandbox that merely never acknowledges is not this case — that is an older sessiond, and the handoff proceeds fenced at the plane alone.) The idempotency guard also checks its belief before answering from it. A controller displaced by an attach on ANOTHER replica reads `control` here until its own heartbeat is refused, up to one interval later, and the take-control press is its user's only way out of that; answering it from memory closed that door. One store READ, which advances nothing, reopens it. Tests: TestACourtesyNoticeNeverClosesAHealthyClient and TestAClientThatNeverDrainsIsClosedFromBehindTheWriteLock pin the two halves of the close rule; TestAClaimWhoseBindingNeverLandedGivesTheGenerationBack and TestAControllerDisplacedElsewhereRecoversOnOnePress pin the two claim rules. All four verified against the mutant they exist for. Co-Authored-By: Claude <noreply@anthropic.com>
…e at once Two more from review, both about a window rather than a value. demote re-read the generation it was demoting FROM, at the top of itself. The window the backwards guard exists for opens earlier than that — the moment keeper.Renew is refused — so a claim landing between the refusal and that read produced from == o.gen, the guard did not fire, and the attach that had just won a newer generation was demoted anyway. The heartbeat now passes the generation its renewal was refused for, and release passes the one it gave up. The test holds the refusal itself in flight rather than the store read inside demote, which covers the whole window, and it now also pins that a superseded demotion installs NOTHING in the sandbox: a viewer binding written for an attach that has since won fences the controller it just became, and the heartbeat renews happily because the lease really is its own. And the broker registered an attach in the owner table only after reading the client's opening resize. For a controller attach the application has already advanced the generation, so for the whole of that read — up to attachFirstMsgTimeout, fifteen seconds, on a client that need only be slow — this replica was serving a controller no peer could see. A claim on another attach took its peer list without it, nothing displaced it, and its own opening announcement then told it it had control at a generation somebody else had passed. Two devices, both printing [you have control], until the loser's next heartbeat. Registration now happens before the read, and what the opening announcement says is read after it. Both mutants confirmed caught (the demotion tells the controller it is a viewer; the attach is invisible to its peers). Co-Authored-By: Claude <noreply@anthropic.com>
…notice The fan-out's own parallelism was not covered: with the per-step deadlines in place a serial walk still finishes, so nothing would have noticed the goroutines going away — and a serial walk is what makes a session with several paused devices pay one deadline per device before the taker hears anything, which is the session this feature ships for. TestTheFanOutReachesEveryPeerAtOnce costs six deadlines when serialised and one when not. The deadline on the sandbox-facing half had no test either; TestABindingWriteThatNeverLandsDoesNotHoldTheHandoff waits ten seconds without it. The CLI prints [you have control] when it learns it from a `control_changed`. A claim this client won can be answered by a peer's displacement first — the plane reads what the attach IS at send time, so that notice says `control` — and the `attached` behind it then finds a client that already holds control and says nothing. The person took control and was told nothing at all. Also: one fewer policy call at attach time (a principal the policy has just refused the controller is not asked again), two comments that had drifted from what the code does, and helper deadlines that no longer report a busy machine as a bug — 15s for "the plane should have done this by now", and the two handoff timings these tests measure raised to 1.5s, which is where a loaded machine was dropping a binding write and failing an unrelated assertion. Both documents say what changed: the design doc records what the two reviews of this round found and why a displaced peer's binding is best-effort (the taker's own binding is what raises the sandbox's fence), and the operator-facing page says what a self-claim costs, what a bounded parallel handoff means, and that a take-over that cannot be installed is given back. Co-Authored-By: Claude <noreply@anthropic.com>
…ffer Both socket tests arranged "another writer owns this socket" by writing eight megabytes at a peer that was not reading, which is true only while the kernel's buffers stay smaller than the payload. Under -count=10 they did not: the write landed, the test that depended on a queue behind it asserted against nothing, and its still-running goroutine then raced its own cleanup restoring clientWriteTimeout. coder/websocket's Writer takes the conn's write lock and holds it until it is closed, which is exactly the state being described — so the tests take it. Both are now deterministic, and one of them kills a mutant that survived the adversarial battery: dropping the explicit CloseNow leaves the socket open behind a client that has taken nothing for its whole budget, because the library's own teardown only fires for a write that is in flight. Co-Authored-By: Claude <noreply@anthropic.com>
The fan-out wrapped its own acknowledgement timeout around the install pair, which caps at one timeout what install and installAndWait already cap at one between them: install bounds its write, and a write that fails takes the acknowledgement wait with it. Nothing could tell the wrapper's presence from its absence — a mutation confirmed the whole suite passes either way — so it is a bound that reads as load-bearing and is not. The notice's deadline stays at the loop, because the write it bounds is the one that reaches a client, and four tests fail within seconds without it. Co-Authored-By: Claude <noreply@anthropic.com>
Sixteen tests started work the moment the fake sandbox's dial-back returned, which says the socket was accepted and not that the plane has bound it: the splice does that a few calls later. A displacement landing in between installs nothing — install returns errAttachNotSpliced at once and never retries — so the binding a test was waiting for never arrived, and it waited out its whole deadline. The suite's own awaitSpliced comment said this; the tests predate it. It bit under load and it now bites harder: a claim whose binding cannot be installed gives its generation back rather than taking control the sandbox cannot honour, so a test whose attach was not spliced yet gets a `stale` where it expected `attached`. That is the production behaviour working; the tests have to say what they mean. Every one of them now waits for a frame to cross the splice (awaitSpliced for an attach that may type, awaitViewerSpliced for one that may not), which is only true once the plane holds the socket. Three full `go test ./...` runs and `./attachplane -race -count=10` clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
A fifth independent review (Opus, read-only; 22-mutant battery, 20 caught; twelve stress scenarios through the real plane under -race). Verdict: converged — merge after F1, a three-line fix with a verified reproduction. Verbatim: Findings — most severe firstF1 — Medium‑High. A displaced controller can be skipped and never told, while the taker is told it has control
was, moved := other.displaceTo(gen)
if !moved {
continue // <- asserts "already at gen" == "already told". It is not.
}
Interleaving (deterministic, 6/6 runs, probe
The pty fence holds throughout (A's binding is This is the round‑four defect class arriving through the one door the round‑four fix did not close: Fix (verified): was, moved := other.displaceTo(gen)
wg.Add(1)
go func() {
defer wg.Done()
if moved && was == terminal.ModeControl {
if wait { _ = other.installAndWait(ctx, terminal.ModeView, gen) } else { _ = other.install(ctx, terminal.ModeView, gen) }
}
nctx, cancel := p.step(ctx)
defer cancel()
other.announceAs(nctx, terminal.TypeControlChanged, 0)
}()
F2 — Low. A claim that gives its generation back tells nobody
Probe S12: after the give‑back the store is at generation 3 with a vacant holder, while the attach that actually held control is still F3 — Low. The ownership vocabulary is filtered client→sandbox but not sandbox→client
The client pump explicitly drops F4 — Low.
|
| Invariant | Status | What makes it hold |
|---|---|---|
| One controller once handoffs settle | Holds | CompareAndAdvanceControllerGeneration is one predicated statement (memstore under the store mutex; pgstore one UPDATE), so two claims from one expected produce one advance. advance/displaceTo each do check‑and‑write under one hold of o.mu and refuse backwards moves. installAndWait keeps the taker unanswered until the displaced controller's sandbox has the new binding. Stress: 70 runs of 10 attaches × 60 claims — at most one attached control per generation, 0 exceptions; the plane settled with exactly one controller at the store's generation every run. |
| A client is only ever told its true mode and generation | Holds per announcement; fails at the decision to announce | One announceAs is the only sender; it reads state under the hold and makes both downgrades there (M8, M9 killed). But F1 skips the announcement entirely. |
| A generation announced never exceeds the store's | Holds | Every number in a message comes from o.get() inside the hold; every write to o.gen comes from a store read (grant, Claim, State) or from a peer's store‑derived number. Pinned by S1 (no announced generation exceeded the lease maximum, 70 runs) and TestARefusedClaimIsNeverToldGenerationZero. |
| No unbounded I/O under a lock or while a taker waits | Holds | o.mu is released before every write (install copies runner first; announceAs uses get()); announce is a channel so acquiring it is bounded too; every peer‑facing step carries p.step (M2, M5 killed); displace is a parallel fan‑out that wg.Wait()s (M1, M3 killed). Stress: a claim and a new controller attach behind a peer that had stopped reading were answered in 206 ms and 403 ms at a 200 ms acknowledgement timeout. |
| Every displaced controller is told, or its stream ends | Does not hold as stated | F1. The true weaker statement: every displaced controller is fenced at its sandbox and at the plane before the taker is told, and is told within one heartbeat interval. |
| A departing attach releases only what it held | Holds | finish takes mode and generation in one get() (M21 killed); Release(0) is a no‑op; the repository Release is predicated on the generation. |
| No goroutine or socket leak | Holds | S11: 30 attach/claim/release/close cycles, goroutines 3 → 3, 0 pairings parked, 0 ownerships left. attachTable.claim makes the TTL/dial‑back race exclusive so done closes once; splice drains both pumps before returning. |
| Lease keeper stopped exactly once | Holds | One heartbeat per splice, one splice per pairing, stopped by the hbCtx splice cancels on return. It can outlive finish by one bounded demotion; those writes land on a closed socket, and demoteTo(from, …) stops it contradicting a newer claim (M11, M12 killed). |
Stress results (all -race, throwaway tests in scratchpad/probe/attachplane/zprobe_stress_test.go)
| # | Scenario | Result |
|---|---|---|
| S1 | 10 attaches × 60 claims racing, 70 runs | store reached gen 61 every run; 0 double‑attached control per generation; 0 over‑the‑store announcements; ≤1 settled controller matching the store every run; 5 of 70 runs showed a simultaneous‑belief overlap → F1. With the F1 fix: 0 of 45. |
| S2 | claim during a peer's displacement (controller's sandbox acks 250 ms late) | 12 runs: exactly one client believing control, matching plane and store |
| S3a/S3b | peer stops reading during a claim / during an attach | answered in 206 ms / 403 ms (bound 4×200 ms) |
| S4 | socket close racing claim + release | 40 interleavings × 12 repeats: every broker returned, 0 ownerships survived, no race reports |
| S5 | sandbox acks late / never / acks the previous generation | claim answered in 307 / 607 / 607 ms at a 300 ms timeout; one controller; taker told the store's generation; the previous‑generation ack correctly ignored |
| S6 | heartbeat renewal refused during the attach's own claim | 60 interleavings × 12 repeats: plane and store never disagreed about who holds control (demoteTo(from, gen)) |
| S7 | reconnect with a stale expected generation | answered stale at the store's generation, never promoted |
| S8 | snapshot held 8× the ack timeout by a slow client | arrives; splice survives |
| S9 | attach that has not sent its opening resize | registered, displaced to view at the new generation, taker the only controller |
| S10 | F1 reproduction | 6/6, 1.70 s overlap at production defaults |
| S11 | leak census, 30 cycles | 3 → 3 goroutines, 0 parked pairings |
| S12/S12b | F2 and F3 reproductions | both confirmed |
The PR's own suites are green at this head: attachplane, controlapp, internal/session, internal/relay, internal/attachio, protocol/..., control, cmd/rainier all pass -race -count=2; attachplane also passes -count=20. go build ./... and go vet ./... clean.
Mutation battery — 22 mutants, 20 caught, 2 survived
Caught by the PR's own tests: M1 serial fan‑out (TestTheFanOutReachesEveryPeerAtOnce) · M2 unbounded courtesy notice (4 tests) · M3 no wg.Wait() (3 tests) · M5 unbounded install write (suite hangs → timeout; genuine kill) · M6 no client write deadline (suite hangs; the production symptom) · M7 close on the caller's deadline (TestACourtesyNoticeNeverClosesAHealthyClient) · M8 announceAs trusts the caller's type (2 tests) · M9 no stale→attached rewrite · M10b announce hold released before the write · M11 unconditional demote (2 tests) · M12 demoteTo off‑by‑one (4 tests) · M13 late peer registration (TestAnAttachStillReadingItsFirstMessageIsDisplacedLikeAnyOther) · M14 no generation give‑back · M15 controller's claim answered from memory · M16 advance walks backwards (2 tests) · M17 displaceTo split into two holds · M18 no stale‑ack drain · M19 plane trusts the client's stamp · M20 client control verbs forwarded · M21 finish reads mode and generation separately.
Survivors:
- M4 — move
displaceTointo the fan‑out goroutine. Survives the PR suite and my probes. Not a real gap: it weakens a defensive ordering ("every displaced peer stops being forwarded for at once, ahead of the fan‑out") whose guarantee is already carried bywg.Wait()and by the taker's owninstallAndWaithaving raised the session'scontrollerGenbeforedisplaceruns. I could not construct an observable difference. - M10′ — state read outside the announce hold. Survives the PR suite; killed by S1. Real coverage gap on the property the announce mutex exists for → F6.
(M10 as first written was an inert twin and was replaced by M10′. M5/M6 kill by suite timeout rather than an assertion, which is the correct signal: without the deadline the write never returns.)
Round‑four fixes vs. the four pairings and the CLI — no regression
All green at -count=3 -race: new client + new plane + new sessiond (TestJourney1And2…, TestJourney3And4AClaimIsAnsweredOnlyAfterTheSandboxHasTheFence, internal/relay/TestAMidAttachHandoffIsAcknowledged); old client + new plane (TestAnUnnegotiatedAttachGetsTodaysMessageSetAndStampedFrames, TestALegacyAttachDisplacesANegotiatedControllerAndSaysSo, TestThePlaneStampsItsOwnViewOverTheClients); new client + old plane (internal/attachio/TestNewClientOldPlane, TestViewNeverTypesEvenWhenNothingAnswers, TestAnAttachThatAsksForNothingDialsExactlyAsBefore); new plane + old sessiond (TestAnOldSandboxNeverAcksAndTheHandoffStillHappens, internal/relay/TestAnOldPlanesAttachmentIsUnconditional, TestAnUnboundFrameIsTheBytesItAlwaysWas). CLI: --view/--take flags and their mutual exclusion, reconnect (TestJourney6ReconnectAsksForWhatItActuallyHad), info's Controller row including the older‑server case, [you have control] via TestControlWonInsideSomebodyElsesHandoffIsStillAnnounced and TestTheTakeKeyClaimsFromTheGenerationItWasTold, and TestGainingControlSaysHowBigThisTerminalIs. S9 independently confirms the early‑registration fix; S8 confirms the write‑deadline fix did not bound the snapshot.
Verdict
Converged. The evidence for convergence, as distinct from rounds 2–4: 20 of 22 mutants die to the PR's own tests, including every round‑four fix; the one surviving semantic mutant is a coverage gap rather than a live bug; 11 of 12 stress scenarios were clean on first write; and the single defect found is not a new path but one residual instance of the class round four named — reached through the decision whether to announce rather than through an announcement — traceable to one sentence of design reasoning whose premise the round‑four fix itself invalidated. That is a module whose defect surface has collapsed to a single identifiable seam, not one still generating new ones.
Merge after fixing F1 (three lines, fix verified above, plus S10 as the regression test). F2 and F3 are one‑liners worth taking in the same commit; F4–F7 are documentation, coverage and a comment and can follow. Reason: F1 is the one invariant the feature exists to protect, it reproduces deterministically at production defaults with a 1.7 s window in which two users' screens both say they have control, and leaving it means the next round finds it — which is exactly the pattern this review was asked to decide about.
A final fix session has been dispatched for F1–F7 and the Cloud re-pin; it will post dispositions here.
A fifth independent review ran a 22-mutant battery and twelve stress scenarios through the real plane and found one residual instance of the announcement class — reached through the decision whether to announce rather than through an announcement — plus two one-liners and four small items. This writes down what each is, what changes, and what was considered and rejected. Co-Authored-By: Claude <noreply@anthropic.com>
displace read `displaceTo` returning moved=false as "this peer is already at gen, so it has already been told". Those are different facts. Two paths move an attach's own state and then spend real time before announcing it — demote, which holds installAndWait for a whole acknowledgement timeout against a sandbox that never answers, and sendStale, which can wait out a client's write budget — and a take-over landing in either gap skipped that peer with `continue`, returned from the fan-out at once, and told the taker it had control. The displaced device was told nothing until its own wait timed out: 1.7s at the production default, with two screens both saying "you have control". `moved` now decides only whether that peer's SANDBOX needs a new binding. The announcement goes out unconditionally, and it is safe unconditionally because announceAs reads the mode and the generation under the announce hold and reports what it read — so a peer already at gen is told its own real state, and a client already there folds it to no notice. That premise is the one the fourth round's fix created and the design note it invalidated had predated. The regression test is the reviewer's S10, and it fails on c6d16cb 5/5. Co-Authored-By: Claude <noreply@anthropic.com>
Two findings, both one-liners, both about somebody not being told. A claim whose binding never reached its sandbox gives the generation back rather than becoming a phantom controller. It then told only the client that asked. The store was two generations on with a vacant holder, while the attach that had been the controller was still `control` in the plane and still forwarded for until its own heartbeat renewal was refused — up to one interval later — and every viewer's next press was refused once too, about a session nobody was driving. The give-back now fans the resulting generation out, with the number sendStale already reads. The runner pump forwarded `attached`, `stale` and `control_changed` verbatim, so a sandbox could tell a client it has control. That client prints [you have control], stops sending claims — a client that believes it is the controller never claims — and types into a plane that drops every frame, with detaching as its only way out. Those three are announceAs's to send, exactly as `control` and `control_ack` are the plane's to write, and the runner pump now drops them the way the client pump drops the other two. Dropped rather than fatal: ending the attach would hand a buggy sandbox a way to disconnect every client watching. Co-Authored-By: Claude <noreply@anthropic.com>
`rainier attach --view` is documented as "watch without ever claiming control". The attach was genuinely admitted a viewer and genuinely typed nothing, but Ctrl-\ still sent a claim and the plane honoured it: a view-mode attach whose principal may drive IS authorized to take control mid-attach, because that is what a reconnecting controller admitted as a viewer depends on, so the service cannot tell the two requests apart. The flag is this user's instruction to their own client, so it is held in the one place that decides what this client claims. That covers the take-control key and --take's single claim alike — the CLI already refuses the two flags together, so the second is belt and braces. The key is swallowed rather than forwarded: a --view attach sends no input at all, so forwarding it would reach the same nowhere with more moving parts, and nothing is printed, which is what the key already does on a device that has control. Co-Authored-By: Claude <noreply@anthropic.com>
… hold The write budget was one number for every frame, so it was a WHOLE-write deadline: at 60s and a 16MiB read limit, the largest frame this stream can carry demanded ≈273 KB/s sustained, and a client making perfectly steady progress over a slower link was CloseNow'n mid-snapshot. It is now a base plus the time the payload needs at the slowest rate this stream will keep a client for — 64 KiB/s — so that frame gets 60s + 256s and a message with no payload gets exactly the base, which is what the budget is actually for: closing a socket that has taken nothing. Both knobs move onto the stream. A package variable three tests write is safe only for as long as nothing in the package calls t.Parallel(). And a test for the announce mutex under contention, which no test created deliberately: the fifth review's one surviving mutant reads (mode, gen) before acquiring the hold and reports that, which is the literal shape of the defect the hold exists to prevent. It now dies. Co-Authored-By: Claude <noreply@anthropic.com>
The previous commit made the take-control key inert by reading `askedView`, which does not mean --view. It means "this attach is requesting view mode", and cmd/rainier's reconnectOwnership asks for exactly that on every reconnect whose previous attach ended as a viewer — so a device superseded while its network was out does not take control back on a blip. Those Options are otherwise byte-identical to --view's. So a plain `rainier attach` that lost control, reconnected, and came back a viewer lost Ctrl-\ for the rest of the process: silently, with no line printed, and with no way back, because the plane promotes an attach only in answer to a claim. The one way out was detaching and re-running the command. NeverClaim is set by the flag and by nothing else, and claim() reads it. The regression test builds the Options a reconnect builds and presses the key; it fails on the askedView version. Found by the adversarial review of this round. Co-Authored-By: Claude <noreply@anthropic.com>
…first word Two defects the reviews of this round found in the two fixes before it. The give-back fan-out did not wait. It is the only fan-out in the module that can demote a peer which genuinely holds control — release and finish both demote the caller first, so their peers are already viewers — and displaceTo flips that peer to `view` in the plane before its sandbox has the matching binding. Fire-and-forget there disarms the NEXT taker: displaceTo hands it `was == view`, it skips installAndWait, and it is answered with the old binding still on the wire. The pty's generation fence still holds, so nothing executes twice; the ordering this module promises does not. It waits now, like a take-over, and the test asserts the binding has landed when the claim returns rather than polling until it does. And announcing to every peer reached attaches that are registered but have not been told what they are yet — registration happens before the first message is read, which is bounded only by attachFirstMsgTimeout. A courtesy notice arriving first IS that client's opening answer, and a client reads its first answer differently from a later one: a plain viewer printed "[another device took control]" twice instead of "[viewing — another device has control]" once, and a --view attach, which should hear nothing when it opens, heard it too. A peer whose state did not move AND which has been told nothing is left to its own opening answer, which reads the same state and says the same thing. A peer whose state moved is still told, because there it is news. Also: the S10 regression test now runs with five seconds of acknowledgement margin and fails a run that stalls past it, because a displaced attach's OWN demotion emits a byte-identical notice when its wait gives up. Co-Authored-By: Claude <noreply@anthropic.com>
…everywhere The write budget scaled on the payload, but Data is a []byte and JSON carries it base64, so four wire bytes leave for every three of payload: the largest frame demanded ~8% more throughput than the rate promises. wireSize rounds the payload up to its encoded length. The Send-level test that was supposed to pin the scaling passed with the scaling reverted, because json.Marshal of a 2MiB payload under -race costs most of a second — vacuous at exactly the -race -count the gates run it under. It buys the same half second from a 384-byte payload at 1 KiB/s now, where marshalling is noise, and it has an upper bound as well as a lower one. The largest-frame assertion beside it was a tautology for any base; it checks the wire rate against the promised floor. Documentation the last five commits falsified, in the four places it lives: the v0 contract said Ctrl-\ still works under --view, the ownership note said that flag's key is answered `stale`, the help text did not mention the key at all, and two prior design-doc sentences the change invalidates now say in place what superseded them — which is the thing this round's own finding was about, and which it had repeated. And TestRunNoRaceOnFloodedOutputDuringDetach's five-second wait for the flood to start failed about one run in four on an idle machine, at c6d16cb as well as here. It is waiting for a precondition, not asserting a throughput, so it waits generously. Co-Authored-By: Claude <noreply@anthropic.com>
Both new --view tests pressed Ctrl-\ the moment the attach started. Ctrl-\ is not this client's key until a plane has proved it speaks conditional ownership, so a press that beat the opening `attached` was forwarded as an ordinary byte and never reached the key handler: the reconnect test then failed about one run in fifteen, and the --view test would have passed on a press that asked nothing. A --view attach prints nothing when it opens, so the snapshot behind the answer is what says the answer has landed. Both tests wait for it, and both still fail when their guard is reverted. Co-Authored-By: Claude <noreply@anthropic.com>
… write Reading the state inside the announce hold closed every interleaving with another announcement; it did not close the one with a state change during the write. A client that stopped draining for longer than a handoff's deadline had the notice about its displacement give up on its hold, then took the message already being written, which named the old state, and nothing corrected it afterwards: a viewer gets no heartbeat, its stdin is dropped at the plane, and a client that believes it controls sends no claim. After the write, announceAs now re-reads, and if the state moved it says so as control_changed, until the state has stopped moving. Bounded, because the state only moves forward. With it, the "not yet told" skip in displace asks only whether the peer has been told anything. A peer granted control and displaced before it finished opening was told "somebody took control from you" about control its client never heard it had; its opening answer reads the state at send time and names the moved state as attached view, which is the true first word. The test that pinned the moved peer being told first pinned that symptom and now pins the opening word. Regression tests are the sixth review's probes, with their harness, and fail on the previous head. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q3ty8xDSnUhwAwnoEAHEFu
The opening viewer notice was suppressed on askedView, which a reconnect also sets, so a plain attach superseded while its network was out came back a viewer and printed nothing, against the help text and the CLI contract. The fact that means "asked to watch" is neverClaim, which only --view sets. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q3ty8xDSnUhwAwnoEAHEFu
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q3ty8xDSnUhwAwnoEAHEFu
|
A sixth independent review of head 2fa15d0 (25 mutants, 20 caught; the full stress set clean, F1/F2/F3 reproductions passing). Three findings, fixed directly in c9a0a98, 2533edd and ef0e901 with the review's own probes as regression tests: an announcement now reports the state at the END of its write (re-read after the write, correct with control_changed until stable); a reconnected viewer is told it is viewing (neverClaim, not askedView); the not-yet-told skip in displace asks only whether the peer has been told, so a peer displaced before it finished opening gets |
1 participant
Conditional controller ownership for terminal attachments
At most one attached client may type into a session. Everyone else watches:
they get the screen and every byte of output, and nothing they send is
executed — not stdin, not a resize, not the bytes a terminal writes back in
answer to a query. Control is a lease on the session row (a monotonic
controller_generation, an opaque per-attach holder, an expiry), thegeneration is the authority, and the fence is at the pty, in the sandbox,
because the plane is not where input executes.
docs/design/2026-09-10-conditional-controller-ownership.mddocs/terminal-controller-ownership.mdWhat a person sees
rainier attachclaims control when it is free and watches when it is not —zero-click on one laptop, honest on two devices.
Ctrl-\takes control;--viewnever claims;--takeclaims once. A displaced device prints[another device took control; press Ctrl-\ to take it back]and stopstyping. A reconnect presents the generation it held, so it resumes what it had
and comes back a viewer, saying so, when somebody took it.
Compatibility
The change reaches three separately deployed parties — client, plane, sandbox
— and all four pairings are supported and tested in both directions. A client
that negotiates nothing gets exactly today's message set; a new plane stamps
every frame it forwards, so an unstamped frame reaching a sandbox means an
older plane and nothing else. Generations travel as decimal strings
everywhere, because a
uint64past 2^53 is silently wrong as a JSON number ina browser. The matrix is in
docs/terminal-controller-ownership.md.Fourth review — dispositions
An independent Opus review of
42475defound the state machine correct (storeCAS, single-step
advance/displaceTo,handoffserialisation, pty fence)and one liveness bug, three should-fixes and three nits in the layer around
it. The instruction for this round was to remove the class, not the
instance.
displaceis boundeddisplaceis a bounded fan-out: peers in parallel, every peer-facing step underPlane.step(one ack timeout).installbounds its runner write;ClientStreamgets a write deadline so a wedged client is closed rather than held. The corrective install in the claim answer moved out from under theannouncehold.attachplane/ownership.go,plane.go,stream.goTestAStuckPeerDoesNotStallAClaim,TestAStuckPeerDoesNotStallANewControllerAttach,TestAStalledExControllerIsFencedEvenThoughItsNoticeCouldNotBeDelivered,TestOneStalledPeerDoesNotHideAnothersDisplacement,TestTheFanOutReachesEveryPeerAtOnce,TestABindingWriteThatNeverLandsDoesNotHoldTheHandoff,TestAWedgedClientIsClosedRatherThanHeldannounceViewer/announceStaleassert a mode without reading itannounceAs, holds the announce lock, reads(mode, gen)under it and reports what it read; no caller supplies a mode. The four per-path variants are deleted. Astalethat would reach a live controller is sent asattached control.attachplane/ownership.goTestAControllerThatClaimsFromAStaleGenerationKeepsControl,TestAStaleAnswerNeverTellsALiveControllerItIsAViewer,TestADisplacementNoticeReportsTheModeItReadsdemoteTois the one transition that cannot refusedemoteTo(from, gen)no-ops when the generation has moved pastfrom;demotetakes that generation from the renewal that was refused, and installs the viewer binding only when the demotion still stands.attachplane/ownership.goTestADemotionThatWasSupersededDoesNotDemote,TestDemoteToRefusesAGenerationThisAttachHasLeftMayClaimfalse;internal/controld's pre-upgrade check mirrors it. An unnegotiated attach is still refused — it cannot be told it is a viewer.controlapp/attachments.go,internal/controld/attach.goTestAViewOnlyPrincipalWatchesAndMayNotClaim(rewritten: it used to pin the 403)claimis not idempotent and is client-triggeredattachplane/ownership.goTestAClaimFromTheCurrentControllerNeverAdvancesTheGeneration,TestAControllerDisplacedElsewhereRecoversOnOnePress[you have control]when it learns it that way.attachplane/ownership.go,internal/attachio/ownership.goTestControlWonInsideSomebodyElsesHandoffIsStillAnnouncedfakeSandbox.orderis gone; bothtime.Sleepassertions are now a round trip through the same client pump.attachplane/ownership_test.goTestAStaleAcknowledgementNeverCostsTheNextHandoffItsWaitWhat the two reviews of this round then found
Two further Opus reviews (one independent, one adversarial with a 21-mutant
battery) were run on the diff above. They found that the bound from #1 had
been applied one level too wide, and that two of the fixes composed into a new
failure. Both are fixed, with tests:
control_changedqueued behind a snapshot killed the attachClientStreamcloses on its own budget only. The announce hold became a channel so acquiring it is bounded too.TestACourtesyNoticeNeverClosesAHealthyClient,TestAClientThatNeverDrainsIsClosedFromBehindTheWriteLockTestAClaimWhoseBindingNeverLandedGivesTheGenerationBackdemotere-read the generation it was demoting from, leaving the first part of its own window uncoveredTestAnAttachStillReadingItsFirstMessageIsDisplacedLikeAnyOtherCloseNowon expiry survived the mutation batteryMutation battery. The adversarial review ran 21 mutants against the first
version of this round: 14 caught, 7 survived. Those seven, and what happened
to each:
TestTheFanOutReachesEveryPeerAtOnceinstall's deadlineTestABindingWriteThatNeverLandsDoesNotHoldTheHandoffdemote(install before the plane-side write)TestADemotionThatWasSupersededDoesNotDemotenow reads the sandbox tooCloseNowon expiryTestAClientThatNeverDrainsIsClosedFromBehindTheWriteLockinstallandinstallAndWaitalready cap at one between them. A bound that reads as load-bearing and is not is worse than no bound; the notice's deadline, which is the one that reaches a client, stays and four tests die without itmayAttachviewer mirrorownerOrAdminanswers both mode questions identically, so both branches behave the same. It is pinned at the service, which is where a narrower policy can be expresseddisplaceTointo the fan-out goroutineEvery fix in both tables above was verified by reverting it and watching its
test fail; the failures are quoted in the commit messages.
One more thing the gates found on their own: sixteen tests started work at the
fake sandbox's dial-back rather than at the splice, which is a few calls later
— so a displacement landing in between installed nothing (
installreturnserrAttachNotSplicedat once and never retries) and the test waited out itsdeadline. It bit under load before this round and bites harder now, because a
claim whose binding cannot be installed gives its generation back instead of
taking control. They all wait for a frame to cross the splice now.
Fifth review — dispositions
A fifth independent Opus review of
c6d16cbran a 22-mutant battery (20 died)and twelve stress scenarios through the real plane under
-race, and calledthe module converged: the state machine and every fourth-round fix hold,
the one surviving semantic mutant is a coverage gap rather than a live bug,
and the single defect it found is one residual instance of the class the
fourth round named — reached through the decision whether to announce rather
than through an announcement.
Design note:
docs/design/2026-09-11-controller-ownership-fifth-review.md.displacereaddisplaceToreturningmoved=falseas "already at this generation, therefore already told". Two paths move an attach's own state and then spend real time before announcing it (demoteholdsinstallAndWaitfor a whole ack timeout against a non-acking sandbox;sendStalecan wait out a client's write budget), and a take-over landing in either gap skipped that peer and answered the taker. 1.7 s at production defaults with two screens both saying "you have control"movednow decides only whether that peer's SANDBOX needs a new binding. The announcement is unconditional, and unconditionally safe becauseannounceAsreads the mode and generation under the announce hold — the premise the fourth round's own fix created, and which the design note it invalidated predatedattachplane/ownership.goTestAPeerAlreadyAtTheNewGenerationIsStillToldAboutIt(the reviewer's S10; 5/5 fail onc6d16cb)controlin the plane and still forwarded for, until its own heartbeat — and every viewer's next press is refused once toosendStalealready reads, and waits: this is the only fan-out that can demote a peer which genuinely holds controlattachplane/ownership.goTestAClaimThatGivesItsGenerationBackTellsThePeersToo{"type":"attached","mode":"control","gen":"99"}reached the client. That client prints[you have control], stops claiming, and types into a plane that drops every frameattached/stale/control_changedthe way the client pump dropscontrol/control_ack. Dropped rather than fatal: ending the attach would hand a buggy sandbox a way to disconnect every client watchingattachplane/splice.goTestASandboxsOwnershipMessagesNeverReachTheClient--viewis documented as never claiming, and Ctrl-\ still claimedOptions.NeverClaimis set by the flag and nothing else; the key is swallowed, silently, as it already is on a device that has controlinternal/attachio/ownership.go,cmd/rainier/main.goTestViewNeverClaimsWhateverTheUserPressesbase + wire(payload)/rate, the rate measured against the base64 the socket actually carries. At 60 s + 64 KiB/s the largest frame gets 60 s + 256 sattachplane/stream.goTestTheWriteBudgetScalesWithTheFrame,TestALargeFrameIsGivenTimeInProportionToItself(mode, gen)before acquiring the announce hold) survived the whole suiteSend, queues a second behind it, and wins control while it waits. M10′ dies 5/5attachplane/ownership_test.goTestAnAnnouncementReportsTheStateItFoundWhenItGotTheHoldclientWriteTimeoutwas a mutable package var three tests wroteClientStream; nothing shared is mutatedattachplane/stream.goWhat the two reviews of this round then found
Two further Opus reviews of this diff — one independent, one adversarial,
both with mutation batteries — found four defects it had introduced, plus
documentation it had falsified. All fixed here, each with a test that fails
without its fix.
askedView, which means "this attach is requesting view mode".reconnectOwnershipsets exactly that on every reconnect whose previous attach ended a viewer, so a plainrainier attachthat lost control and reconnected lost Ctrl-\ for the rest of the process — silently, with no way back but detachingOptions.NeverClaimis a separate fact from the mode being requested.TestAReconnectedViewerKeepsItsTakeControlKeyviewin the plane with its binding still in flight, and the next taker skipped its owninstallAndWaitand was answered earlywait=true. The test asserts the binding has landed when the claim returns, rather than polling until it doesattachFirstMsgTimeout), making a courtesy notice their opening answer: two[another device took control]lines for a plain viewer, one for a--viewattach that should hear nothingTestAPeerThatHasNotBeenToldWhatItIsIsNotToldAboutSomebodyElseFirstjson.Marshalof a 2 MiB payload under-racecosts most of a second, so the test was vacuous at exactly the-race -count=30the gates run it underwireSize, and the assertion checks the wire rate against the floor with the base subtracteddocs/cli-v0-contract.mdsaid Ctrl-\ still works under--view;docs/terminal-controller-ownership.mdsaid that flag's key is answeredstale; the help text did not mention the key; two prior design-doc sentences this change falsifies were left standing — which is the thing F1's own finding was aboutControlAckTimeout; a comment attributed generation zero tofinish, which never passes it--viewtests pressed Ctrl-\ before the opening answer, when it is not yet this client's key: the reconnect test flaked ~1 run in 15, and the--viewtest would have passed on a press that asked nothingTestRunNoRaceOnFloodedOutputDuringDetach's 5 s wait for the flood to start failed ~1 run in 4 on an idle machine, atc6d16cbas well as here, and now waits generouslyDeliberately not taken: a wedged client is now held ~5 minutes rather than
1 for the largest frame, and a new attach's first byte can wait one
Plane.stepbehind a wedged peer — both are the price of not disconnecting aclient that is making steady progress, and both cost resources rather than
correctness. A
control_changedcan still precede a peer's openingattachedwhen that peer's state did move, and the client prints its notice twice;
that is pre-existing, the notice is true there, and folding it is a change to
client notice policy with its own design note. The give-back's
control_changed viewreads as "another device took control" when the leaseis vacant — closest word the vocabulary has, and a new type is a wire change.
Verification
All with
RAINIER_TEST_DATABASE_URL/RAINIER_TEST_PG_DSNset to a realPostgreSQL 17, so nothing skipped silently.
make verifyrepoteston memstore (-v)repoteston pgstore (-v)go test ./internal/e2e/ -racego test ./attachplane/ -race -count=20go test ./internal/attachio/ -race -count=5,./cmd/rainier/ -race-race -count=30Every fix in the two tables above was verified by reverting it and watching
its test fail;
TestAPeerAlreadyAtTheNewGenerationIsStillToldAboutItfails5/5 on
c6d16cbitself, and mutant M10′ dies 5/5.One thing the gates found that is not in this PR:
controlapp'sTestPullWorkspaceOverflowtakes 217 s under-raceon its own and blows the10-minute package timeout when the machine is busy. It is untouched by this
branch (
make verifyrunsgo test ./...without-race, and passes), andit wants its own change.
Not in this PR
The state machine itself was not redesigned — three reviews now agree it holds
under every interleaving they could construct. Rate limiting is deliberately
absent (a limiter would also refuse the legitimate rapid hand-back after a
mis-press). A displaced peer that cannot be reached inside one acknowledgement
timeout loses its notice and nothing else: its pty fence is in force the
moment the store advances, the plane stops carrying what it types before the
fan-out starts, and its own heartbeat demotes it within one interval.