aw-sync: per-peer and per-bucket errors abort the whole sync pass (and burn the supervisor's restart budget)

[ErikBjare]

(Rewritten — the original framing claimed aw-sync exits with no supervision to catch it. That was wrong: exiting is the intended failure signal and both bundles implement the supervisor side. The real defect is narrower and is described below. See the comment thread for what changed.)

Exiting non-zero on failure is a deliberate and reasonable contract — the module signals failure, the supervisor surfaces it — and both bundles implement it properly:

• aw-qt (aw_qt/trayicon.py): check_module_status() polls manager.get_unexpected_stops() every 5s, auto-restarts up to MAX_AUTO_RESTARTS = 3 within RESTART_WINDOW_SECONDS = 600, shows a tray notification per restart, and past the budget opens show_module_failed_dialog() with the module's log in the detailed text plus a manual Restart button.
• aw-tauri (src-tauri/src/manager.rs): same idea with exponential backoff — restart_count < 3, delay 2^(restart_count+1) seconds, and an alert window that updates in place when the module recovers.

So for a whole-run failure — sync dir missing, config unreadable, aw-server unreachable — the current behaviour is right and should stay.

The actual defect: no error classification

aw-sync promotes every error to a whole-process failure, including errors scoped to a single peer or a single bucket.

loop {
    if let Err(e) = sync::sync_run(client, &sync_spec, mode) {
        error!("Error during sync cycle: {}", e);
        return Err(e);          // any error, whatever its scope
    }

and one rung down, pull_all propagates per host:

for host in hostnames {
    pull(&host, client)?
}

so a single unreadable peer aborts the pass and every peer after it in iteration order is skipped. #686 preserves this shape (for remote in selected { pull_db(...)?; }).

Why supervision makes this worse, not better

A persistent per-peer error becomes a crash loop. On aw-tauri the restart budget is spent in about 14 seconds (2 + 4 + 8), on aw-qt in three polls; then the user gets a modal and sync stops entirely. One permanently-bad peer database — a Syncthing conflict file, a db written by a newer schema, a truncated file — therefore takes down sync for all peers, permanently, rather than costing you that one peer's data.

Without a supervisor the same bug just stops the daemon once. With one it escalates to "sync is broken and disabled, here is a dialog". The contract is fine; routing peer-scoped errors into it is what hurts.

A concrete trigger: #669 replaced an unwrap() with a refusal when a bucket has hostname = "unknown" and there is no source device ID to substitute — correct in itself, since src_did is None on every pull. But that is a per-bucket condition returning Err from the whole cycle. (Scanned 60+ staging dbs on a real mesh and found no such bucket, so this particular trigger is currently latent — the shape is the point.)

Note that Android already does this correctly

The JNI path never exits a process; syncBoth returns {"success": false, "error": ...}, and aw-android#251 surfaces last-completion status in the UI. The CLI is the odd one out.

Suggested fix

Classify errors by scope rather than treating them uniformly:

• Fatal / whole-run — bad sync dir, unreadable config, server unreachable past a retry budget. Keep exiting; the supervisors handle it well. Ideally detect these before entering the loop so the failure is immediate and legible rather than arriving one cycle in.
• Per-peer — unreadable, corrupt, or schema-incompatible peer db. Log, skip that peer, continue the pass, and include it in the end-of-pass summary.
• Per-bucket — the various TODOs, FIXMEs #669 provenance refusal and similar. Skip the bucket, sync the rest.
• Report the non-fatal ones somewhere durable so "sync is running but two peers are failing" is visible: aw-sync status (feat(aw-sync): add status doctor command and fail-loud empty-pull warnings #687) is the natural home, and a last-completion-status file would let aw-qt/aw-tauri show it without the module having to die to be heard.

The distinction that matters: the daemon should only exit for conditions that a restart could plausibly fix. A corrupt peer file is not one of those, and restarting into it three times before giving up is strictly worse than skipping it.

Related: #682, #684 (silent failures), #686 (shares the per-peer ?), ActivityWatch/aw-android#220.

cc @TimeToBuildBob

[ErikBjare]

Rewrote the issue body. The original claimed aw-sync exits with nothing to catch it — that was wrong, and @ErikBjare pointed out the exit is the intended way to surface failures to aw-qt/aw-tauri.

Verified: it is, and both sides implement it. aw-qt does 3 auto-restarts in a 10-minute window then a dialog with the module log (trayicon.py:78-79, 248-278); aw-tauri does 3 with exponential backoff and an alert window (manager.rs:590-606).

The finding that survives is narrower but sharper: there is no error classification. Peer-scoped and bucket-scoped errors get promoted to whole-process failures, and under supervision that is worse than it would be without — a permanently-bad peer db burns the restart budget in ~14 seconds and then disables sync for every peer behind a modal. Exiting for whole-run failures stays correct.

Title updated to match.

[TimeToBuildBob]

Agreed on the rewrite — exiting is the right whole-run contract. The bug is routing peer/bucket errors into it. A permanently-bad peer that burns the supervisor budget and then disables sync for everyone is strictly worse than skipping that peer.

v0.14.0 slice already in flight:

• fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697 makes per-bucket errors non-fatal (warn + continue in the sync_datastores loop) and does the same at peer level in pull_all, so one unopenable peer no longer skips every peer after it. That is the aw-sync: per-peer and per-bucket errors abort the whole sync pass (and burn the supervisor's restart budget) #688 blast radius from Pulling a peer whose bucket hostname contains whitespace 400s and aborts the whole sync pass (activated by #685) #692 / whitespace hostnames.
• fix(aw-sync): open peer databases read-only; skip version mismatch #700 opens peer dbs read-only and skips version mismatch instead of migrating or aborting.

Still open after those land:

• other per-peer failures (corrupt / unreadable file, not just open/version) need the same continue shape
• skipped peers/buckets should show up in aw-sync status (feat(aw-sync): add status doctor command and fail-loud empty-pull warnings #687, already merged) so the daemon does not have to die to be heard

Tracked on ActivityWatch/activitywatch#1445. Not closing this on #697 alone — that PR covers the current crash class, not the full classification.

[TimeToBuildBob]

Correction on coverage, and the remaining daemon-path slice is now a PR.

#697's per-peer continue is in pull_all. The supervised daemon does not use that path — #685 (host-layout switch) is parked for v0.14.0 — so a permanently-bad peer still aborts sync_run, exits non-zero, and burns the aw-qt/aw-tauri restart budget. That is the original #688 trigger for users.

#703 is that daemon-path skip: warn+continue per remote, total failure still Err. Does not close this; per-bucket skip stays on #697, and skipped peers still need to show up in aw-sync status.

#700's open_peer_datastore Err arm still return Err for corrupt/unreadable files. When that lands, that arm should continue the same way, or the daemon-path hole reopens.