fix(remote): keep the binding a re-point replaces instead of destroying it - #884
Open
fujibee wants to merge 5 commits into
Open
fix(remote): keep the binding a re-point replaces instead of destroying it#884fujibee wants to merge 5 commits into
fujibee wants to merge 5 commits into
Conversation
…ng it (#849) Pointing an existing team at a different server overwrote $.remote_binding wholesale. The sync rows and keys for the old server survive that write -- they are keyed on (server_instance_id, remote_team_id, protocol_version) -- but the endpoint string in the binding is the only pointer back to them, so the overwrite orphaned data that was still on disk, with no way back except remembering the old URL. _remote_write_binding now moves a live binding into $.previous_bindings before replacing it with one that names a different server. One entry per (server_instance_id, remote_team_id, protocol_version): a re-archive of the same identity replaces the older copy, and an identity becoming current again leaves the archive, so the array is bounded by the number of distinct servers the team has ever been bound to. The archived copy drops `capabilities` (refetched on every connect; an archived copy would be the one stale snapshot nobody re-reads) and gains `replaced_at`. A binding with no server_instance_id never completed a registration and is replaced without being archived, as before. Same-identity rewrites -- adopt retries, set-endpoint's same-server move -- archive nothing. `remote status` prints each archived endpoint with when it was replaced; restoring one is a reconnect to that endpoint, which adopts the same server instance and finds the partition's rows where they always were.
…lue, and pin the stored alphabet (#849 review) Two static findings on the previous commit, both on the status/recovery surface. The previous line printed the archived endpoint through _remote_endpoint_display -- which drops the path, and for a hosted endpoint the path IS the capability -- while telling the reader to "reconnect to that endpoint". For any path-bearing endpoint that guidance pointed at the wrong address. Status now shows the display form with no reconnect claim, and one trailing line names where the exact value lives (the team's config, under previous_bindings) and why it is not printed. The endpoint<TAB>replaced_at framing rested on an alphabet nothing enforced: the WHATWG parser deletes raw TAB/LF before parsing, so validateEndpoint's https branch accepted URLs whose RAW string -- the thing actually stored -- still carried them. validateEndpoint now refuses raw control bytes outright, which is the validator's own premise ("what is written in the URL is where the connection goes") applied to bytes the parser silently removes. Also narrows the bound stated for the archive: it is per distinct (server_instance_id, remote_team_id, protocol_version) tuple, not per server. New coverage: a path-bearing (capability-style) endpoint archived verbatim, displayed host-only with no token in the output, and restored by reconnecting with the archived exact value; connect refusing a raw-TAB endpoint with its no-control-byte sibling accepted; five verdict-table rows (one allow for a capability path, four deny for raw control bytes).
…y, exact value in config (#849 review)
…tored endpoint's alphabet (#849 review) Two findings from the declared review, both real. $.remote_binding has two wholesale writers, not one: _remote_write_binding, and cmd_pull's bind-after-bootstrap write. The archive ran in only the first, so a pull into an already-bound empty team from a different server replaced the binding with no way back -- the exact loss this PR exists to end, reachable through the door the PR did not cover. The archive step is now a shared primitive (_remote_archive_replaced_binding) and both writers run their document through it; the "one writer" claim is corrected where it was stated. The status previous-lines read endpoint<TAB>replaced_at and split on the separator, resting on an alphabet only NEW endpoints are held to: a binding written by an older version can carry raw TAB/LF, the archive keeps what the binding held, and the framing then split lines and could bleed path/token fragments into output. Rows now travel as one JSON object per line -- JSON escapes every byte below 0x20, so a row is one line whatever the endpoint contains -- fields are re-extracted from each row as JSON, and the printed values are stripped of control bytes. New coverage: a pull that re-points an already-bound, history-less team archives the replaced binding (drives the pull writer, not connect); a legacy fixture whose stored endpoint carries a raw LF in the host and a raw TAB inside a capability-style path is archived verbatim while status emits exactly one previous line and neither the token, the path, nor any fragment.
fujibee
marked this pull request as ready for review
August 18, 2026 23:54
…cmd' (#849 review) Two of the three negative greps in the legacy control-byte case were non-last '! cmd', which POSIX errexit exempts on every bash: the checks central to this PR's claim — no token, no capability path in status — were green whether or not they held, and raised the enforced-assertions count by 2. refute (tests/test_helper.bash) runs the command and fails the test when it succeeds; all three are converted, and a leak mutation (printing the raw archived endpoint) now turns the exact assertion red.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Declared reviewers: 1
Change class: user-affecting — product behavior changes (connect/pull
binding preservation,
remote statusoutput, endpoint validation), not aCI-only change.
Fixes #849. Pointing an existing team at a different server no longer
destroys the binding it had.
What changes
$.remote_bindinghas two wholesale writers —_remote_write_binding(both connect paths) and
cmd_pull's bind-after-bootstrap write. A sharedprimitive (
_remote_archive_replaced_binding) now moves the current bindinginto
$.previous_bindingsbefore either writer replaces it with one thatnames a different identity (
server_instance_id,remote_team_id,protocol_version), so the invariant holds at the writer boundary. Thearray is a sibling of
$.remote_binding, so the wholesalejson_setnevertouches it.
remote statusprints each archived endpoint (host-only display form — for ahosted endpoint the path is the capability, so the exact value is never
printed) with when it was replaced, and one trailing line naming where the
exact value lives:
Those lines are read as one JSON object per row — JSON escapes every byte
below 0x20, so a row stays one line even for an endpoint a pre-fix version
stored with raw TAB/LF in it — and the printed values are stripped of
control bytes. Going forward
validateEndpointalso refuses raw controlbytes outright (the WHATWG parser deletes TAB/LF before parsing, so its
https branch previously accepted URLs whose raw stored form still carried
them), but the display path does not depend on that: bindings written by
older versions are handled safely too.
The setup doc's connect step now states that trying another server with an
existing team is non-destructive and how to come back.
The shape, and the two questions it has to answer
Per-identity dedup rather than an unbounded log: one entry per
(server_instance_id, remote_team_id, protocol_version). Re-archiving an
identity replaces its older copy; an identity becoming current again leaves
the archive.
distinct (server_instance_id, remote_team_id, protocol_version) tuples the
team has ever been bound to — one per server in the common case, more if
the same server re-registers the team or the protocol version moves —
never by how often the team moved between them. A scripted A→B→A→B… flip
holds it at one entry
(tested:
flipping between two servers keeps one archive entry per server).endpointis theinput to
connect, which takes the 409-adopt path on the original server,verifies it is the same instance, and re-anchors. Tested end to end:
connect A → re-point to B → read the archived endpoint out of the config →
connect to it → the binding holds A's original
server_instance_idand arow seeded in A's partition of the local roster journal is still present
(
a round trip A -> B -> A restores A's binding and its partition rows).Two deliberate exclusions from the archived copy's contract:
capabilitiesis dropped. It is refetched on every connect (the function'sown header is the contract: the snapshot holds nothing that cannot be
fetched again), and an archived copy would be the one stale snapshot nobody
re-reads. Restoring is a reconnect, never a copy of the archived object
back into
$.remote_binding.server_instance_idnever completed aregistration; there is no partition behind it to point back to, so it is
replaced without being archived, exactly as before.
Same-identity rewrites — an adopt retry,
set-endpoint's same-server move —archive nothing (tested).
Verification
tests/test_remote.bats, using a second concurrentlyrunning mock server whose instance id is rotated so the two addresses are
two servers. They include a path-bearing (capability-style) endpoint
archived verbatim, displayed without the token, and restored by
reconnecting with the archived exact value — and a connect refusal for a
raw-TAB endpoint beside its accepted no-control-byte sibling. Two cases
drive the review findings: a pull that re-points an already-bound,
history-less team archives the replaced binding (the pull writer, not
connect), and a legacy fixture whose stored endpoint carries a raw LF in
the host and a raw TAB inside a capability-style path is archived verbatim
while status emits exactly one previous line and no token or fragment.
four deny for raw control bytes).
fail; identity-differs condition removed (archive on every write) → the
same-server case fails; control-byte refusal reverted → 3 verdict rows
disagree and the refusal case fails; the pull-site archive call removed →
the pull re-point case fails; status reverted to TAB framing → the legacy
control-byte case fails. All pass with the fix.
configs (re-point / same identity / no binding / null instance id /
flip-back promotion / same-identity re-archive replace): 6/6.
Scope
cmd_connect's temp-file and trap handling is untouched (connect discards the server's refusal, so nothing downstream can explain a failure #871 owns thatseam).
forgetderives the trust-file path fromthe current binding only, so the trust file of an archived partition is
left behind after a re-point + forget. That gap predates this change (the
old binding was simply gone); with
previous_bindingsrecorded, forgetcould now clean those up too.