fix(remote): pull a team larger than a command line, and say what it is doing (#882) - #895
Merged
Merged
Conversation
…mmand line A Windows machine could not pull a team past about four hundred messages. The apply put one wire id per message into the SQL that reads the outcomes back, embedded it twice, and handed the result to sqlite3 as an argument -- 78 characters per message against a command line that caps at 32,767. Measured on Windows: 413 messages pull, 418 do not. The team in the report had 2,079. Three sites carry a list that grows with the data and all three move to stdin, which has no such limit: the pull outcomes, the push acknowledgements (one row per acked message, a thousand in catch-up), and the roster read members. Fixing only the reported one would leave the same defect in the two beside it. The apply itself already used stdin and is untouched.
A pull of 2079 messages runs for 312 seconds and prints nothing. A verifier running it against a real server on Windows saw the silence, concluded the command had hung, and killed it -- a working command was indistinguishable from a stalled one, and the diagnosis went thirty minutes in the wrong direction (#882). Progress goes to stderr only. cmd_pull captures this process's stdout as the result channel and greps it for pull_bootstrap_result, so a line written there would ride in the stream the caller parses. The lines separate fetching from applying because applying spawns a child process per batch and fetching does not: when the output stops moving, which line it stopped on says which half to look at.
A terminal does not timestamp its own scrollback, so "it printed this and then stopped" cannot say whether the stop was ten seconds or ten minutes. The Linux run that finished took 312 seconds; without elapsed, a slow phase and a stuck one look identical to whoever is watching.
fujibee
marked this pull request as ready for review
August 19, 2026 16:08
A hosted endpoint is https://host/t/<token> and that token is the capability: anyone who reads it off a terminal, a screen share or a pasted log can connect as the team. The progress line added for #882 printed the whole URL -- and that line exists precisely because people sit in front of a silent command and then paste its output into an issue. hostOf() rather than anything new: it is new URL(...).host, which drops path, query, fragment and userinfo, and is already the rule this file uses for the refusal record status prints. remote.sh holds the same rule in shell. The cursor on the second line is the server's and this path does not put it through sequence() first, so only a canonical sequence is printed as itself.
The existing case only reached the first cursor, which is ours -- the mock ended after one page, so a server-chosen next_after never reached a progress line and the guard was covered by nothing. Two pages, the first ending with a control sequence where a sequence belongs.
…lly arrive The first cursor is the server's -- teamSnapshot.min_available_seq, which publicSnapshot never validates -- so it reaches the first progress line as sent. The previous case put the malformed value on the second page instead, where the real sqlite driver refuses a non-numeric sync_pull_cursor (sqlite-sync.sh:891-897) before the loop comes round: a state only a stubbed driver produces. Keeps a canonical value as the control, because a guard decayed into replacing everything would pass every assertion about the malformed one.
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
Closes #882 — both halves of it.
The one thing the reporter actually asked for, twice, was progress output. They
lost 79 minutes to a command that printed nothing, misdiagnosed it as a hang,
and carried that misdiagnosis into the report. Landing the size fix alone would
answer the failure they hit and not the reason they could not investigate it, so
the progress lines (previously #883, now closed) ride here.
Change class: user-affecting. A Windows machine cannot pull a team past
about four hundred messages today. This is the fix for that, not a CI change.
What was wrong
storage_sync_apply_pullbuildsoutcome_ids— one'<wire-id>',per messagein the page — and embeds it twice in the SQL that reads the outcomes back,
which went to
sqlite3as an argument:Windows caps a command line at 32,767 characters. 78 characters per message
— 39 for the id, twice — so the page stops fitting at a few hundred:
Measured on a Windows machine by pm, against the real
sqlite3:Nothing in the product chose that number. The team in the report had 2,079
messages and a default page of 1,000, so it failed on the first page — while
the same data on Linux (≈2MB limit) completed in 312 seconds, which is why
"same path, only the volume differs" had two explanations that fit.
The fix
Stdin has no such limit, and
storage_sync_apply_pullalready applies its pagethat way (
sqlite-sync.sh:1082) — that line is correct and is untouched. Thethree statements whose length grows with the data move to stdin:
_sqlite_data_stdin)Only the reported one was failing in the field. Fixing it alone would leave the
same defect in the two beside it, on paths that are one busy team away from the
same size.
Measured
*
mainis one over its baseline today; that is #887, not this branch.The new case measures the length of the command line, not whether an
operating system refuses it:
A test that waited for the limit to be hit would be green on every machine that
runs CI, because the limit that broke this is Windows'. What must not happen is
growth per message, and that is observable anywhere. It also asserts the
larger page really imported (120 rows), so the comparison cannot pass by both
runs having applied nothing.
Reverting the driver turns it red:
The other half: it says what it is doing
stderr only.
cmd_pullcaptures this process's stdout as the result channeland greps it for
pull_bootstrap_result, so a line written there would ride inthe stream the caller parses.
Two separate claims, with what backs each, because an earlier revision of this
body ran them together as "verified end-to-end":
remote-sync.sh:47execs node with no redirection, andcmd_pullredirects only stdout. That is read from the source, not executedby a test.
pullBootstrapwith injected dependencies and capture both streams. That is aunit measurement of the output, not of the pipeline it travels down.
I did also run the real CLI once with the two streams captured to separate
files, and it agreed. That was a one-off measurement and is not what any of the
green here rests on.
Fetching and applying are named separately because applying spawns a child
process per batch and fetching does not — when the output stops moving, the line
it stopped on says which half to look at. Elapsed seconds are stamped because a
terminal does not timestamp its own scrollback: without them, "it printed this
and then stopped" cannot say whether the stop was ten seconds or ten minutes.
What the progress line must not carry
A hosted endpoint is
https://host/t/<token>and the token IS the capability.The first version of this line printed the whole URL — into the output someone
pastes into an issue when a pull is taking too long, which is the entire reason
the line exists. It prints
hostOf()now:new URL(...).host, already thisfile's rule for the refusal record
statusshows, and alreadyremote.sh'srule in shell (
_remote_endpoint_display). Nothing new was written to redact.The cursor on the second line is the server's, and
publicSnapshotvalidatesonly the team id and the server instance id —
min_available_seqreaches thefirst progress line exactly as sent. Only a canonical sequence prints as itself.
The malformed cursor is planted as
min_available_seqrather than as a secondpage's
next_after: the real sqlite driver refuses a non-numericsync_pull_cursor(sqlite-sync.sh:891-897) before the loop comes round again,so a fixture built there exercises a state only a stubbed driver produces.
The test captures both streams, because the regression worth catching is a
progress line landing in stdout, and that is invisible if only stderr is
measured. It asserts stdout is exactly one line and parses as
pull_bootstrap_result.Still open
Field verification on a real Windows machine — pm has the hardware, and the
reproduction is now a size rather than an environment.