Repository navigation
feat(config-sync): show what a running sync is doing - #861
Merged
Merged
Conversation
A sync is one request: it captures the local snapshot, reads the remote revision, merges, uploads every resource object, publishes the head, applies the result and cleans up history. The caller hears back only at the end, and the upload is unbatched — one object per resource, two round trips each — so a vault with a few hundred resources looks like a frozen window: the settings page disables its controls and then shows nothing until the call resolves. Add a progress observer to the sync path and install one for the manual run. It reports the phase (capture, download, merge, upload, apply, cleanup), the units that phase counted, and the bytes when transfer knows them: `done`/`total` count resource objects while transferring and entities otherwise, `total == 0` means the phase cannot know its size, and `bytesTotal == 0` means the byte size is unknown, which is the normal case for a download. Reports go out as `configSync.progress` with the same NDJSON notification `configSync.changed` already uses, throttled to five per second with a phase change never dropped, and the state event plus the call's return value stay the terminal signal. The background poll installs the silent observer: only the manual path has a caller watching. The document/progress naming collision is resolved by renaming the RAII flag that marks a running sync, which was also called `SyncProgress`.
The cloud sync card disabled its controls for the whole run and then reported the outcome, so a long upload had nothing to show. Subscribe to the host's `configSync.progress` reports and draw them: the phase, the units that phase has finished, and the bytes when they are known. The report is rendered only while this page is running a sync itself, so the background poll stays quiet, and the request's answer clears it. An announced byte total leads the reading — one large object can dominate a phase whose unit count already looks finished — otherwise the unit pair leads, and a report that announced neither stays indeterminate with the phase name alone rather than a fraction the card would have to invent. The fraction is announced as a real `progressbar` with the figures as its value text. Turn one report into that reading in a pure module, with its own tests, so the page keeps owning only the subscription. `configSync.syncNow` also stops inheriting the 130 s default transport deadline. The host cannot cancel a run under way and keeps going after a deadline expires, so the old budget turned a long sync into a timeout error while the sync was still running; the report stream is now the liveness signal and the deadline only exists to stop a promise hanging forever when a response is genuinely lost.
The settings-workflow section of the configuration-sync specification and the IPC protocol's cloud-sync section described the state event and the call's return value as the only outcome, which no longer covers a run that reports itself while it runs. Record the phase names, what `done`/`total` count, what a zero total and a zero byte total mean, the throttling, and that only the manual path reports.
Two gaps an independent review found in the new coverage. Nothing pinned the throttle interval, so a constant raised to minutes would still pass: report twice in one phase, past the interval, and both must arrive. And the sync assertion only required the first phase to be `capture`, which the closing report alone satisfies: require the pair, the started report and the counted one, so deleting either fails.
The report was gated on `busy === "sync"`, so "enable sync" — which runs the same full `configSync.syncNow` — showed nothing. That path is the worst one to leave blank: a first enable uploads the whole vault, and it runs while `configured` is still false, so the status card that held the progress row had not been rendered yet either. Watch both operations and move the row above the status card, between the connection card and the card a first enable has not built. Also correct the progress type's own description: `merge` reports what it merged, and only `cleanup` reports its phase without counting it.
Reading a compatibility vault walks every device tip, and each tip read the resources below it. With per-object reports inside each of them the count restarted, so a vault with more than one tip walked the bar backwards. Count the tips instead: the reports stay monotonic, and the mode reports one object fewer per resource than it used to.
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.
Cloud sync is one request:
configSync.syncNowcaptures the local snapshot, readsthe remote revision, merges, uploads every resource object, publishes the head,
applies the result and cleans up history. The caller hears back only at the end.
The settings card therefore disables its controls and then shows nothing for the
whole run — which is a poor fit for the shape of the work: the upload is
unbatched, one object per resource with two round trips each, so a vault with a
few hundred resources is minutes of nothing on screen.
Two things made it worse. The card already has a
syncingbadge, but it onlyappears when a state event happens to land. And the transport's default 130 s
deadline expired long before the host was done, so a large sync surfaced as
host RPC timeout: configSync.syncNowwhile the host was still working throughobjects.
Change
reports
configSync.progressover the same NDJSON channelconfigSync.changedalready uses; the background poll installs the silentobserver, because only the manual path has a caller watching.
capture,download,merge,upload,apply,cleanup— the units that phase counted, and the bytes when transfer knowsthem.
done/totalcount resource objects while transferring (the devicetips being read, in append-only mode) and entities otherwise;
total == 0means the phase cannot know its size, and
bytesTotal == 0means the byte sizeis unknown, which is the normal case for a download. Reports are throttled to
five per second and a phase change is never dropped; the state event and the
call's return value stay the terminal signal.
and "enable sync" — as a row above the status card, so a first enable shows
progress while
configuredis still false. An announced byte total leads thereading, because one large object can dominate a phase whose unit count already
looks finished; otherwise the unit pair leads, and a report that announced
neither stays indeterminate with the phase name alone. The figures are
announced as a real
progressbar's value text.configSync.syncNowno longer inherits the 130 s default deadline. The hostcannot cancel a run under way and keeps going after a deadline expires, so the
report stream is the liveness signal and the deadline only exists to stop a
promise hanging forever when a response is genuinely lost.
SyncProgresstoSyncRunGuard, soSyncProgresscan mean what a run reports.Evidence
cargo test -p host-core --locked: 634 passed. New: five observer tests(wire names, a phase change is never throttled, a report is never withheld
longer than the interval, the numbers a card shows, the silent observer), and
the two-device scenario installs a recorder and asserts that a sync reports
capturefirst, that it reports the capture pair (started, then counted),that
capture → merge → upload → applycome in that order, and that the uploadreports a total that is reached with a byte total.
report fails the sync test; a receiver-style 128 KiB ceiling, an upload total
forced to 0, and the throttle interval raised to ten minutes each fail a
different test.
pnpm build:js,pnpm --filter @pi-desktop/desktop typecheck,pnpm lint,scripts/check-style-tokens.mjs,scripts/check-architecture.mjs: pass.apps/desktop/test/config-sync-progress.test.mjscovers the reading(byte total leads, download falls back to units, no total stays
indeterminate, clamping, byte formatting);
config-sync-settings.test.mjscovers the wiring and the placement — one progress row, rendered before the
card a first enable has not built (verified by moving the block behind the
gate and watching the assertion flip);
packages/shared/src/protocol.test.tscovers the new event name and the sync deadline.
pnpm -r --if-present testpasses except two timing-sensitive tests(
npm-executable,remote-host-ssh-transport) that exceeded their owntimeouts under full parallel load and pass when run alone, on this tree and on
main; neither reads config-sync state.docs/scripts/check-docs.mjs(508 pages) andcheck-locales.mjs(80 pairs)pass; the specification and IPC protocol state the contract in both locales.
Risks and what this does not do
updates the counters once, when it lands.
automatic sync that is already in flight still has no visible progress.
coarser than the strict path; it is that or a bar that walks backwards.
already had: the phase measures the work the run does, not the bytes it adds.
and the 30 minute ceiling is a lost-response guard rather than a promise a sync
finishes in 30 minutes.