Skip to content

fix(aw-sync): skip duplicate device_id folders that truncate history on pull - #686

Merged
ErikBjare merged 3 commits into
ActivityWatch:masterfrom
TimeToBuildBob:bob/aw-sync-dedupe-device-id
Sep 16, 2026
Merged

ErikBjare merged 3 commits into
ActivityWatch:masterfrom
TimeToBuildBob:bob/aw-sync-dedupe-device-id

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

Problem

When one physical device has written under two hostname folders in the sync directory, pull_all imported both. Provenance is the bucket hostname (or $aw.sync.origin), so both folders land in the same destination bucket. Resume-from-newest then silently drops everything older than the first folder's newest event.

Real-world case from #683: POCO F8 Ultra/ (9.4 MB, Jul 2026) vs poco_f8_ultra/ (273 MB, 2021–2026). If the small folder is walked first, ~1M events never import and the log says ✓ Already up to date!.

The duplicate folders come from ActivityWatch/aw-android#272 (hostname sanitization without migration). Any hostname change or folder rename reproduces it.

What changed

  1. pull_all deduplicates by device_id. Discover {hostname}/{device_id}/*.db, group by device_id, keep the largest file, warn! the skipped folders. This is the hazard removal from the issue.
  2. find_remotes_nonlocal applies the same collapse. So a later 3-level walker (aw-sync: daemon (the default subcommand) never pulls — two incompatible sync-folder layouts #682) does not reintroduce the bug on the daemon path.
  3. Loud warning in sync_one. If a resume-from-newest pull reports up-to-date but the source has more events than the destination, log that older history was not imported and that recovering it requires deleting the destination bucket and re-pulling.

Not in this PR

Already-truncated destination buckets are not repaired by this PR. Delete the dest bucket and re-pull after this lands.

Testing

  • cargo test -p aw-sync --offline: 17 lib tests + 10 tests/sync.rs + 3 tests/sync_roundtrip.rs
  • Unit tests cover listing, largest-per-device_id selection independent of discovery order, and unique device_ids kept

…on pull

When one device_id appears under two hostname folders, pull_all imported
both. Provenance is the bucket hostname, so they share a destination
bucket and resume-from-newest then dropped the older history.

Group remotes by device_id and keep the largest db. Warn when a
resume-from-newest pull leaves the destination with fewer events than
the source.

Git-Session-Id: ede65257-42c4-5386-a084-23031fc09a31
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Heads-up on merge order: this PR and #685 conflict in aw-sync/src/util.rs. I checked by merging the two heads locally.

The overlap is in the design, not just the text:

Suggested order: merge #685 first, since it is the smaller layout fix and CI is green. Then rebase this PR so list_remote_dbs and the device_id dedupe sit on top of #685's single walker. That avoids ending up with two directory walkers. I'll do the rebase once #685 lands.

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.82927% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.31%. Comparing base (656f3c9) to head (f191c5f).
⚠️ Report is 113 commits behind head on master.

Files with missing lines Patch % Lines
aw-sync/src/sync_wrapper.rs 0.00% 15 Missing ⚠️
aw-sync/src/util.rs 95.23% 3 Missing ⚠️
aw-sync/src/sync.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #686       +/-   ##
===========================================
+ Coverage   70.81%   81.31%   +10.49%     
===========================================
  Files          51       71       +20     
  Lines        2916     6460     +3544     
===========================================
+ Hits         2065     5253     +3188     
- Misses        851     1207      +356     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

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.

@ErikBjare

Copy link
Copy Markdown
Member

@TimeToBuildBob This is the strongest of the three, and I think it should merge first rather than after #685.

RemoteDb { hostname, device_id, path, size } is exactly the typed-peer abstraction #684 item 5 asked for — one walk producing structured peers instead of Vec<PathBuf> that every caller re-derives meaning from. #685 and #687 both add their own walkers (collect_db_files/hostname_from_db, and scan_sync_dir/classify_top_dir respectively); if this lands first, both can build on RemoteDb instead, which removes the duplication and the util.rs merge conflict that #685 currently has with each of them.

select_remote_dbs_keeps_largest_per_device_id reversing the discovery order before selecting is the right test for this bug specifically — the trigger in #683 was fs::read_dir ordering, so order-independence is the property that actually needs proving, not just "picks the big one".

The sync_one warning is well placed. Worth noting for anyone reading later: it fires on an already-damaged destination, so it is a detection mechanism, not a repair one — as your PR description says, recovery still means deleting the bucket and re-pulling. On the setup in #683 that is a 273 MB / ~1M event re-import, which is fine but not instant.

Two smaller notes:

  1. list_remote_dbs is 3-level-only, so once pull_all stops going through get_remotes(), legacy 2-level root dbs are no longer pull candidates. That is the correct outcome — the 1.19 GB root orphan in aw-sync: daemon (the default subcommand) never pulls — two incompatible sync-folder layouts #682 should never be pulled by a peer — but it is a real behaviour change relative to fix(aw-sync): make default daemon use host-layout pull/push #685's broadened walker, and the two PRs disagree about it. Please make whichever wins explicit in a comment.

  2. This PR removes the only caller of get_remotes() (sync_wrapper.rs:8). fix(aw-sync): make default daemon use host-layout pull/push #685 concurrently rewrites get_remotes() fairly extensively. One of those two is wasted work — another reason to sequence this one first and rebase fix(aw-sync): make default daemon use host-layout pull/push #685 on top.

list_remote_dbs is the pull_all walker and does not see leftover
{device_id}/test.db files at the sync root. That is intentional:
the ActivityWatch#682 root orphan must not be imported by a peer. Document that
find_remotes stays 2-level, and add a regression test.

ActivityWatch#686

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
Rebased onto ActivityWatch#686 so the daemon switch and
pull-only staging fix sit on RemoteDb / list_remote_dbs instead of a
second walker. The default daemon (no --start-date/--buckets/--sync-db)
now uses pull_all+push, the same 3-level layout as `aw-sync sync` and
Android.

list_remote_dbs stays 3-level-only: leftover `{device_id}/test.db` at
the sync root (the ActivityWatch#682 orphan) is not a pull candidate. A test locks
that so it does not get "fixed" later. The advanced sync_run path is
unchanged.

Fixes ActivityWatch#682

Git-Session-Id: d963a5fd-8243-53d2-b4e5-7d020965e48b
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
The default `aw-sync`/`aw-sync daemon` path drove `sync_run` against the
sync root, writing `{device_id}/test.db` and scanning two levels — so it
never saw `{hostname}/{device_id}/*.db` remotes from Android or
`aw-sync sync`. Route the default daemon through `sync_wrapper` like
the simple `sync` command, and only stage a local db on push.

Walker rewrite dropped: `pull_all` already uses `list_remote_dbs`
(3-level-only) after ActivityWatch#686. Broadening
`find_remotes` at the sync root would import the ActivityWatch#682 orphan.

Fixes ActivityWatch#682

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
`aw-sync daemon` can walk a configured sync dir and stay silent when it
finds nothing to pull. This does not change which remotes are pulled
(ActivityWatch#682 / ActivityWatch#685). It makes the miss diagnosable:

- 3-level peers come from list_remote_dbs + select_remote_dbs_by_device_id
  (same pair pull_all uses), so duplicate device_id reporting matches pull
- status-only overlay on top: 2-level leftovers, unrecognised entries,
  own-staging vs peer, SyncLayout
- warn! on pull when zero remotes are found, with skip reasons
- `aw-sync status` inspects peer dbs read-only (no WAL sidecars)

Stacked on ActivityWatch#686.

ActivityWatch#684

Git-Session-Id: 1673d3c9-c19e-5588-bfa5-96436fd680e8
@ErikBjare

Copy link
Copy Markdown
Member

Targeting correction to my note on #685: the dead get_remotes trio should be removed here, not there.

get_remotes becomes unreachable in 4c9fac6 itself — the moment pull_all switches to list_remote_dbs, its last caller is gone. So it is dead in #686 standalone, and since #687 is also based on 4c9fac6 it inherits the same three dead functions (util.rs:274, 290, 402 at that PR's head). Deleting in #685 would leave whichever of the other two merges first carrying unreachable code.

Also worth dropping the // TODO: share logic with find_remotes and find_remotes_nonlocal that sits just above it — that is what this stack just did.

Separately, the #687 rework looks right: remote_db_to_entry means status now classifies the same RemoteDb list pull_all consumes rather than re-walking, which is what makes its output trustworthy as a preview of what a pull would actually do. All three now merge clean in any pairing.

TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
`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
pull_all switched to list_remote_dbs, so get_remotes and its two
helpers (contains_db_file, contains_subdir_with_db_file) had no
callers. Clippy would not flag them because get_remotes was pub.
Also drop the stale TODO that item 5 of ActivityWatch#684 already resolved.

Two walkers remain, on purpose:
- list_remote_dbs + select_remote_dbs_by_device_id — pull_all, 3-level
- find_remotes + find_remotes_nonlocal — advanced sync_run, relative

ActivityWatch#686

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
The default `aw-sync`/`aw-sync daemon` path drove `sync_run` against the
sync root, writing `{device_id}/test.db` and scanning two levels — so it
never saw `{hostname}/{device_id}/*.db` remotes from Android or
`aw-sync sync`. Route the default daemon through `sync_wrapper` like
the simple `sync` command, and only stage a local db on push.

Walker rewrite dropped: `pull_all` already uses `list_remote_dbs`
(3-level-only) after ActivityWatch#686. Broadening
`find_remotes` at the sync root would import the ActivityWatch#682 orphan.

Fixes ActivityWatch#682

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
`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
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@ErikBjare Dropped the trio here: get_remotes, contains_db_file, contains_subdir_with_db_file, plus the stale TODO.

Two walkers left, with a comment saying why they differ:

  • list_remote_dbs + select_remote_dbs_by_device_idpull_all, 3-level
  • find_remotes + find_remotes_nonlocal — advanced sync_run, relative to a given root

f191c5f on this PR. #685 and #687 restacked onto it so they do not reintroduce the dead functions.

@ErikBjare
ErikBjare merged commit 876709e into ActivityWatch:master Sep 16, 2026
7 checks passed
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
The default `aw-sync`/`aw-sync daemon` path drove `sync_run` against the
sync root, writing `{device_id}/test.db` and scanning two levels — so it
never saw `{hostname}/{device_id}/*.db` remotes from Android or
`aw-sync sync`. Route the default daemon through `sync_wrapper` like
the simple `sync` command, and only stage a local db on push.

Walker rewrite dropped: `pull_all` already uses `list_remote_dbs`
(3-level-only) after ActivityWatch#686. Broadening
`find_remotes` at the sync root would import the ActivityWatch#682 orphan.

Fixes ActivityWatch#682

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
`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
ErikBjare pushed a commit that referenced this pull request Sep 16, 2026
…arnings (#687)

* feat(aw-sync): add `status` doctor command and fail-loud empty-pull warnings

`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 (#682
/ #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

#684

Git-Session-Id: 375e1ec2-04d0-5884-9beb-cc5cd9c704c6

* refactor(aw-sync): build status scan on RemoteDb walker

`aw-sync status` was a third directory walk beside list_remote_dbs
(#686) and collect_db_files (#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.

#687

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 16, 2026
The default `aw-sync`/`aw-sync daemon` path drove `sync_run` against the
sync root, writing `{device_id}/test.db` and scanning two levels — so it
never saw `{hostname}/{device_id}/*.db` remotes from Android or
`aw-sync sync`. Route the default daemon through `sync_wrapper` like
the simple `sync` command, and only stage a local db on push.

Walker rewrite dropped: `pull_all` already uses `list_remote_dbs`
(3-level-only) after ActivityWatch#686. Broadening
`find_remotes` at the sync root would import the ActivityWatch#682 orphan.

Fixes ActivityWatch#682

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
TimeToBuildBob added a commit to TimeToBuildBob/aw-server-rust that referenced this pull request Sep 17, 2026
The default `aw-sync`/`aw-sync daemon` path drove `sync_run` against the
sync root, writing `{device_id}/test.db` and scanning two levels — so it
never saw `{hostname}/{device_id}/*.db` remotes from Android or
`aw-sync sync`. Route the default daemon through `sync_wrapper` like
the simple `sync` command, and only stage a local db on push.

Walker rewrite dropped: `pull_all` already uses `list_remote_dbs`
(3-level-only) after ActivityWatch#686. Broadening
`find_remotes` at the sync root would import the ActivityWatch#682 orphan.

Fixes ActivityWatch#682

Git-Session-Id: eca21d8a-f6da-5096-b28f-783778f4c09f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants