feat(aw-sync): add status doctor command and fail-loud empty-pull warnings - #687
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #687 +/- ##
==========================================
+ Coverage 70.81% 79.63% +8.82%
==========================================
Files 51 72 +21
Lines 2916 6817 +3901
==========================================
+ Hits 2065 5429 +3364
- Misses 851 1388 +537 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI-green and mergeable — waiting only on a maintainer click. This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted. |
|
@TimeToBuildBob The doctor command is the right shape, and the read-only handling is the detail I would have asked for: opening peer dbs Two things. Merge orderYour note on #684 says this and #685 "touch the same files in different hunks and should merge either order". They do not: Reproduce: git fetch origin pull/685/head:pr-685 pull/687/head:pr-687
git worktree add --detach /tmp/mt pr-685 && cd /tmp/mt
git merge --no-commit --no-ff pr-687
Build the scan on #686's
|
b2aa766 to
7a2cf22
Compare
|
@ErikBjare You're right — Pushed a restack onto #686 (
#685 still needs a rebase after these two land. This PR currently includes #686's commit until that one merges — then GitHub drops it from the diff. Still pull-only on this repo, so not self-mergeable. Ready for a maintainer click; this can land first (carries #686) or after #686. |
`aw-sync status` was a third directory walk beside list_remote_dbs (ActivityWatch#686) and collect_db_files (ActivityWatch#685). 3-level peers now come from the same list_remote_dbs + select_remote_dbs_by_device_id pair pull_all uses, so duplicate-device_id "not pulled" matches the pull path. Leftover 2-level / unrecognised entries sit on top of that list. ActivityWatch#687 Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
7a2cf22 to
b26b635
Compare
`aw-sync status` was a third directory walk beside list_remote_dbs (ActivityWatch#686) and collect_db_files (ActivityWatch#685). 3-level peers now come from the same list_remote_dbs + select_remote_dbs_by_device_id pair pull_all uses, so duplicate-device_id "not pulled" matches the pull path. Leftover 2-level / unrecognised entries sit on top of that list. ActivityWatch#687 Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
b26b635 to
1ad9f8a
Compare
|
Heads-up: this went Also relevant to the |
…arnings `aw-sync daemon` can walk a configured sync dir and stay silent when it finds nothing to pull, which is indistinguishable from a working setup. This does not change which remotes are pulled (ActivityWatch#682 / ActivityWatch#685). It makes the miss diagnosable: - classify both 2-level (`{device_id}/*.db`) and 3-level (`{hostname}/{device_id}/*.db`) layouts without opening sqlite - warn! on pull when zero remotes are found, with skip reasons - `aw-sync status` prints every entry, inspects peer dbs read-only (no WAL sidecars), and flags duplicate device_id, hostname mismatch, unpublished staging, and unimported peers ActivityWatch#684 Git-Session-Id: 375e1ec2-04d0-5884-9beb-cc5cd9c704c6
`aw-sync status` was a third directory walk beside list_remote_dbs (ActivityWatch#686) and collect_db_files (ActivityWatch#685). 3-level peers now come from the same list_remote_dbs + select_remote_dbs_by_device_id pair pull_all uses, so duplicate-device_id "not pulled" matches the pull path. Leftover 2-level / unrecognised entries sit on top of that list. ActivityWatch#687 Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
1ad9f8a to
42d6128
Compare
|
Rebased onto current The branch was New head:
Local check: This is step 1 of the v0.14.0 sequence on ActivityWatch/activitywatch#1445. After this merges, next is the two in-PR additions on #685. |
|
@greptileai review |
|
@greptileai review |
|
@TimeToBuildBob review |
|
@ErikBjare Post-merge review of What is right
Finding:
|
| Path | Walker | Sees |
|---|---|---|
aw-sync / daemon → sync_run |
find_remotes (2-level) |
{device_id}/*.db |
aw-sync sync (simple) → pull_all |
list_remote_dbs (3-level) |
{hostname}/{device_id}/*.db |
status classifies from the pull_all pair. remote_db_to_entry sets not_visible_to_daemon = None for a 3-level winner. db_entry does the same for a 2-level peer.
So:
- A 3-level Android peer prints as
[peer 3-level]with no "not pulled" reason. - The daemon still does not pull it (
find_remotesnever looks at depth 3). pull_discovery_warningsis called fromsync_runwith that scan. The original failure (102 daemon passes, 3-level peers, zero remotes) now dumps those peers under "Found 0 remote db files" without saying why the daemon skipped them.
The field name promises daemon visibility. The implementation reports pull_all visibility, and only own/duplicate. Neither path's layout miss is named.
pull_all itself still info!("No remote databases found") and returns Ok(()) — fail-loud did not land on the simple sync path.
Not a revert. It is the sentence the doctor exists to print. Suggested follow-up, no pull-behaviour change:
- 3-level peer:
not pulled by daemon: 2-level walker (find_remotes); visible to aw-sync sync / pull_all - 2-level peer:
not pulled by pull_all: 3-level-only walker; visible to daemon - Rename the field, or stop calling it
not_visible_to_daemonuntil that is true.
Tests today encode the weak behaviour (classifies_both_layouts_and_flags_duplicate_device_id asserts the dump contains "3-level", not a walker reason).
Smaller notes
- Mixed-layout warning in
collect_warningsstill says "new daemon pushes should use the 3-level host layout (aw-sync:daemon(the default subcommand) never pulls — two incompatible sync-folder layouts #682 / fix(aw-sync): make default daemon use host-layout pull/push #685)". After the Sync: tracking issue and v0.14.0 release triage activitywatch#1445 revision, fix(aw-sync): make default daemon use host-layout pull/push #685 is parked and the daemon stays 2-level for v0.14.0. That sentence is now the wrong advice. status.rswarning collection (unimported, hostname mismatch, unpublished staging, leftover-synced-from-,test.dbname) has no unit tests. Codecov 0% onstatus.rsis real; scan/inspect coverage lives inutil.rs.sync_runstillreturn Errwhen one peer db fails to open (warn, then abort). You already called this on Sync: tracking issue and v0.14.0 release triage activitywatch#1445: per-peercontinueis fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697, not this PR.- Output shape is Entries + Warnings. A later "last pass" section from aw-sync: return and persist a SyncReport — a pass is a side effect, so nothing can report what it did #695
SyncReportfits without redesign.
No code change from this review. The walker labels are the one follow-up I would take next, unless you want them left until the v2 devices/{device_id}/ switch makes both walkers obsolete.
* feat(aw-sync): daemon pull is opt-in via aw-sync config.toml (#714) Erik's b8 decision: 0.14 is push-only everywhere unless the user opts in. Add the first proper config for aw-sync's config dir (dirs::get_config_dir(), previously a TODO stub): # aw-sync config pull = false # default; set true to import peers every pass Daemon subcommand: --mode changes from a defaulted SyncMode to Option<SyncMode> so "not given" is distinguishable from an explicit choice. Effective mode is push-only when the config's pull=false, both when pull=true, and an explicit --mode always overrides the config (dirs::effective_daemon_mode). The one-shot `aw-sync sync` command is untouched -- it keeps its own pull+push default. The file is created with the commented default on first daemon start (or `status`), so users find the switch. `aw-sync status` now prints the effective mode and where the config was read from. No separate suppression was needed for the #687 "zero peers" warning: sync_run() already only captures pull-discovery warnings when mode is Pull/Both, so a push-only daemon pass naturally produces none. Git-Session-Id: 2f40c8f4-6fa5-5c1d-9313-e9e188acb41c * fix(aw-sync): status reads config without creating it; suppress per-peer pull warnings Two fixes from #716 review (Erik, 2026-09-18): 1. `status` is a read-only doctor. Swap `load_or_create_sync_config` for the new `read_sync_config` helper which returns `None` when the file is absent instead of writing the default. When absent, status now prints: daemon mode: push (pull=false, config: <path> — not present, default) 2. When `pull = false`, listing every peer with "! has not been imported locally" is misleading — the daemon is doing the right thing. Replace the per-peer warnings with one top-level note: pull is off in <config>; peers below are visible but not imported by the daemon (set pull = true, or run `aw-sync sync`) Also adds two tests for `read_sync_config` (absent → None, no file created; present + pull=true → Some(config)). Git-Session-Id: d398 * fix(aw-sync): explicit --mode daemon startup no longer depends on config.toml An explicit --mode is documented to always win over config.toml, but the daemon still unconditionally loaded/created the config file first. A malformed or unwritable config.toml would then abort startup before daemon() ran, even with an explicit CLI override. Skip config loading entirely when --mode is given explicitly. Git-Session-Id: 212461ef-20e7-561e-b99b-a8f641b73b9a * fix(aw-sync): status never creates config dir; clarify config-derived mode is a guess - config_dir_path() constructs the aw-sync config path without creating it; get_config_dir() (used by the daemon, which is about to write config.toml there) now delegates to it. status previously called get_config_dir() before read_sync_config(), so the read-only doctor command silently created the config directory as a side effect. - status's 'daemon mode:' line is derived from config.toml alone and cannot see an explicit --mode passed to a running daemon; label it as such and point at the 'Last pass' section, which reports the mode actually used. Git-Session-Id: a7e90efc-d5b6-5d9b-a826-55b00e53d16c * fix(aw-sync): namespace daemon config under [daemon] table Erik: 'Properties should be in some default namespace, not in top-level imo.' Follows aw-server's own convention of nesting a distinguishable sub-concern in its own [section] (cf. [auth] in aw-server/src/config.rs) rather than dumping it at the config's top level. pull now lives at [daemon].pull; SyncConfig wraps a new DaemonConfig. Updated the commented default, all call sites, and the dirs.rs unit tests. Git-Session-Id: bcd5de1a-146f-5d49-a776-ae61c28c2602
Fixes #684 items 1–2 (fail loud +
aw-sync status). Does not change which remotes are pulled — that is #685.Stacked on #686 (
RemoteDbwalker). Suggested merge order: #686 → this → #685 rebased. #685 conflicts with this inaw-sync/src/util.rs; #686 and this compose clean.The problem
Setting up sync and having it silently do nothing is currently indistinguishable from a working setup. The report that prompted this: 102 daemon passes,
Pulling...102 times, zero remotes found, zero warnings.sync_run()only logs remotes when the 2-level walk finds some. Finding zero peers in a directory the user configured for sync is the interesting case.What this PR does
find_remotes_nonlocalreturns nothing (or skips classified peers),warn!with every sync-dir entry and why it was not pulled. Opening a remote db that fails alsowarn!s before returning the error.aw-sync status. Read-only doctor command. Does not create staging files. Prints:SQLITE_OPEN_READ_ONLYso it does not write-wal/-shminto a Syncthing folderdevice_idacross folders (aw-sync: duplicate folders for one device_id silently truncate history on pull #683), folder name ≠ bucket hostname, unpublished own staging, unimported peers, leftover…-synced-from-…buckets,test.dbnameRemoteDbwalker. 3-level peers come fromlist_remote_dbs+select_remote_dbs_by_device_id(the same pairpull_alluses), so duplicate-device_id"not pulled" is what pull would actually skip. Status-only overlay on top: 2-level leftovers, unrecognised entries, own-staging vs peer,SyncLayout.Not in this PR (called out in #684)
manifest.json(item 3) — status currently opens sqlite read-only; a manifest is the cheaper follow-up.aw-webuipeer surfacing (item 4) — different repo.autostart_modules(item 6).Tests
cargo test -p aw-sync --lib:device_idselect_remote_dbs_by_device_id