fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal

[TimeToBuildBob]

Fixes #692.

Problem

Pulling a peer whose bucket hostname contains whitespace (e.g. POCO F8 Ultra from Android devices before ActivityWatch/aw-android#272 sanitized device names) fails with a 400 from create_bucket (#658 rejects new buckets with whitespace hostnames). The 400 propagated via ? and aborted the entire sync pass, leaving every subsequent peer unseen (#688 blast radius).

Fix

Part 1 — Sanitize on import (Option 1 from #692, with the legacy-ID lookup):

get_or_create_sync_bucket now checks for an existing unsanitized legacy bucket first (so any device already pulled before this fix keeps its established ID and avoids a full re-import fork per ActivityWatch/activitywatch#1373). If no legacy bucket exists, the hostname and derived ID are sanitized with aw-android's exact algorithm (sanitize_hostname(), byte-identical to DeviceHostname.kt: lowercase, [^a-z0-9_-]+ → _, trim _) before calling create_bucket. "POCO F8 Ultra" → "poco_f8_ultra", so Android's hostname-column migration lands on the same ID. $aw.sync.origin keeps the raw hostname. The sanitizer also runs when the hostname field has whitespace even if the derived ID is already clean.

Part 2 — Per-bucket and per-peer non-fatal errors (Option 3 from #692 / #688):

• The ? propagation in the sync_datastores bucket loop is replaced with warn + continue. A broken or invalid bucket is skipped; all healthy buckets still sync.
• pull_all does the same at peer level: a peer that fails to open no longer aborts the pass and skips every peer after it (needed by aw-sync writes to peer databases on every pull: WAL flip + schema migration on files it does not own #693's skip-on-mismatch).

Tests

sanitize_hostname("POCO F8 Ultra") == "poco_f8_ultra"          ← unit, matches DeviceHostname.kt
test_whitespace_hostname_pull_creates_sanitized_bucket
test_whitespace_hostname_pull_reuses_legacy_unsanitized_bucket
test_whitespace_hostname_sanitizes_even_when_id_is_clean       ← origin clean, hostname dirty
test_unusable_datastore_does_not_panic                         ← renamed + updated
test_non_string_sync_origin_does_not_panic                     ← renamed + updated

Related: #658, #685, #688, #693, ActivityWatch/aw-android#272, ActivityWatch/activitywatch#1373.

[ErikBjare]

@TimeToBuildBob The legacy-ID lookup is right — existing unsanitized → existing sanitized → create sanitized is exactly the resolution order #692's correction asked for, $aw.sync.origin keeps the raw hostname, and both paths are tested. One blocking item and two small ones.

Blocking: the sanitizer must match aw-android's, or this defers the fork rather than closing it

This PR sanitizes with replace(char::is_whitespace, "_"):

"POCO F8 Ultra"  →  "POCO_F8_Ultra"

aw-android's sanitizeDeviceHostname (DeviceHostname.kt) is lowercase(Locale.ROOT).replace(Regex("[^a-z0-9_-]+"), "_").trim('_'):

"POCO F8 Ultra"  →  "poco_f8_ultra"

Trace what happens the day ActivityWatch/aw-android#272 lands and the phone migrates its bucket hostname column to poco_f8_ultra:

1. sync_origin = "poco_f8_ultra", so new_id = …-synced-from-poco_f8_ultra
2. get_bucket(new_id) → NoSuchBucket (the existing one is …-synced-from-POCO_F8_Ultra)
3. new_id.contains(char::is_whitespace) → false → else branch → create …-synced-from-poco_f8_ultra
4. Resume-from-newest starts from nothing → full re-import → every event twice in /timeline

That is the discussions#1373 symptom again, just moved to the Android migration date — and by then every desktop that ran this PR has a POCO_F8_Ultra bucket that no future ID will match.

Fix is small and only possible before merge: use Android's exact algorithm (lowercase, [^a-z0-9_-]+ → _, trim _), so today's desktop creates …-synced-from-poco_f8_ultra and Android's migration lands on the same ID with no lookup needed. Worth a shared sanitize_hostname() in aw-sync with a doc comment saying it must stay byte-identical to DeviceHostname.kt, plus a test asserting "POCO F8 Ultra" → "poco_f8_ultra".

Small: per-peer isolation is still missing, and step 3 depends on it

sync_wrapper.rs, pull_all:

for remote in selected {
    pull_db(client, &remote.hostname, &remote.path)?;
}

This PR makes bucket-level errors non-fatal but a peer that fails to open still aborts the pass and skips every peer after it. Plan step 3 (#693 minimal: open peers read-only, skip on user_version mismatch) needs this to be warn + continue so a skipped peer has somewhere to go — otherwise "skip peer" becomes "abort pass". Three lines; since this is already the non-fatal-errors PR, it belongs here rather than waiting for #688.

Small: sanitize on the hostname, not only the ID

The branch condition is new_id.contains(char::is_whitespace). If sync_origin came from $aw.sync.origin metadata (clean) while bucket_from.hostname still has whitespace, new_id is clean, the else branch runs, final_hostname keeps the whitespace, and create_bucket 400s anyway. Narrow, but the guard should be bucket_from.hostname.contains(char::is_whitespace) || new_id.contains(char::is_whitespace).

With the first item fixed I have nothing else on this one.

[codecov]

Codecov Report

❌ Patch coverage is 76.56904% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.32%. Comparing base (656f3c9) to head (bff7a2a).
⚠️ Report is 121 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-sync/tests/sync.rs	77.41%	35 Missing ⚠️
aw-sync/src/sync_wrapper.rs	0.00%	16 Missing ⚠️
aw-sync/src/sync.rs	92.53%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #697      +/-   ##
==========================================
+ Coverage   70.81%   79.32%   +8.50%     
==========================================
  Files          51       74      +23     
  Lines        2916     7821    +4905     
==========================================
+ Hits         2065     6204    +4139     
- Misses        851     1617     +766     

☔ 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]

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]

Addressed all three.

Blocking — sanitizer now matches aw-android. Shared sanitize_hostname() is byte-identical to DeviceHostname.kt (trim, lowercase, [^a-z0-9_-]+ → _, trim _). "POCO F8 Ultra" → "poco_f8_ultra", not "POCO_F8_Ultra". $aw.sync.origin still keeps the raw hostname. Tests lock the Android DeviceHostnameTest.kt cases plus the POCO example, and assert the whitespace-only form is not created.

Per-peer isolation. pull_all is now warn + continue on pull_db failure, so a peer that fails to open does not abort the pass. Step 3 (#693 skip-on-mismatch) has somewhere to go.

Sanitize on hostname, not only ID. Guard is bucket_from.hostname.contains(whitespace) || new_id.contains(whitespace). Added a test where $aw.sync.origin is already poco_f8_ultra while hostname is still POCO F8 Ultra.

Pushed as 67eeee7. The earlier "waiting on a maintainer click" note is superseded — this needed the review items first. CI is running.

[ErikBjare]

@TimeToBuildBob review

[TimeToBuildBob]

All three items confirmed in 67eeee7. CI is green on all platforms.

Blocking — sanitizer matches aw-android: sanitize_hostname() does trim → lowercase → [^a-z0-9_-]+→_ → trim_, byte-identical to DeviceHostname.kt. "POCO F8 Ultra" → "poco_f8_ultra" not "POCO_F8_Ultra". $aw.sync.origin still keeps the raw hostname. Tests lock all the Android DeviceHostnameTest.kt cases plus the POCO example, and assert the whitespace-only form is never created.

Per-peer isolation: pull_all uses warn + continue on pull_db failure; a peer that fails to open does not abort the pass. #693 skip-on-mismatch has somewhere to go.

Sanitize on hostname not only ID: guard is bucket_from.hostname.contains(whitespace) || new_id.contains(whitespace). test_whitespace_hostname_sanitizes_even_when_id_is_clean covers the exact case where $aw.sync.origin is already poco_f8_ultra while hostname is still POCO F8 Ultra.

One trivial nit (not blocking): in test_unusable_datastore_does_not_panic the first assert !result.is_err() || result.is_ok() is a tautology (!is_err() == is_ok()), so it never fails. The second assert result.is_ok() is the live check. Harmless, but worth knowing so it doesn't confuse a future reader.

Ready to merge.

[TimeToBuildBob]

P1 from the AI review: a hostname like " * " sanitizes to unknown and would have created -synced-from-unknown, mixing every such remote. That's the same provenance hole the existing hostname == "unknown" guard exists to close, but the guard never saw it because the source hostname wasn't the sentinel yet.

7d6cfd9 refuses that pull; per-bucket skip keeps the rest of the pass. Test: test_whitespace_hostname_that_sanitizes_to_unknown_is_skipped_on_pull.

P2 (Locale.ROOT vs Unicode lowercase) left as-is — ASCII is what DeviceHostname.kt tests, and that's the fork this PR has to close.

CI running on 7d6cfd9. Still pull-only here; needs a maintainer click once green.

[ErikBjare]

@TimeToBuildBob Now CONFLICTING against master — #698 landed and rewrote the ds_localremote block in sync_run that this touches. Needs a rebase; since it needs a push anyway for the sanitizer (match DeviceHostname.kt byte-for-byte) and the per-peer continue in pull_all, fold all three into one push. Note #678 may go in ahead of this now that it is green and mergeable — rebase over both if so.

[ErikBjare]

@greptileai review

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; the remaining timestamp issue only affects diagnostics for uncommon pre-epoch data.

Summary

This PR sanitizes imported bucket hostnames using the Android-compatible algorithm, preserves legacy destination IDs where possible, and isolates bucket and peer failures while continuing healthy synchronization work.

• Unconditionally normalizes hostname case, punctuation, and whitespace before creating new sync buckets.
• Reuses legacy unsanitized destination buckets to avoid unnecessary history forks.
• Returns an error when every attempted bucket or peer fails, while allowing partial success.
• Adds regression coverage for hostname variants, legacy reuse, unknown origins, and total-failure reporting.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Remote bucket] --> B[Derive raw destination ID]
  B --> C{Legacy raw bucket exists?}
  C -->|Yes| D[Reuse legacy bucket]
  C -->|No| E[Sanitize hostname and origin]
  E --> F{Sanitized bucket exists?}
  F -->|Yes| G[Reuse sanitized bucket]
  F -->|No| H[Create sanitized bucket]
  D --> I[Sync events]
  G --> I
  H --> I
  I --> J{Bucket sync succeeds?}
  J -->|Yes| K[Count success and continue]
  J -->|No| L[Warn and continue]
  K --> M{Any bucket succeeded?}
  L --> M
  M -->|Yes or none attempted| N[Return success]
  M -->|All attempted failed| O[Return error]

Loading

_{Reviews (2) · Last reviewed commit: "fix(aw-sync): sanitize hostnames uncondi..."}

[TimeToBuildBob]

Rebased onto master (8037907) over #698's maybe_setup_local_remote rewrite. MERGEABLE. CI in flight.

Folded into this one push as asked:

• sanitizer still byte-identical to DeviceHostname.kt ("POCO F8 Ultra" → poco_f8_ultra)
• pull_all per-peer continue (warn, do not abort the pass)
• refuse pull when hostname sanitizes to unknown

Did not restack onto #678: hunks still do not overlap (reconcile_updated_events in sync_one vs sanitizer/pull_all), and stacking would dump #678's commits into this PR. If #678 merges first this should stay mergeable; I will rebase again only if it does not.

[ErikBjare]

@TimeToBuildBob Rebase confirmed — on current master, sanitizer/per-peer continue/unknown-skip/all four tests intact. Greptile's re-review (maintainer-triggered, so that path is confirmed working) raised three things; two need changes before this merges.

Required 1 — sanitize always, not only on whitespace (sync.rs:338)

Greptile is right and it is the same fork I blocked on earlier, in case-only form. The guard is hostname.contains(whitespace) || new_id.contains(whitespace), but DeviceHostname.kt also lowercases and replaces punctuation. So PIXEL8 (no whitespace) is left as PIXEL8 today; when ActivityWatch/aw-android#272 migrates that phone's bucket hostname to pixel8, new_id becomes …-synced-from-pixel8, the legacy lookup finds nothing, and the history forks — exactly what the whitespace fix was written to prevent.

Fix: compute sanitized = sanitize_hostname(raw) unconditionally. Lookup order stays raw ID → sanitized ID; create under the sanitized ID whenever it differs from raw. Then Android's migration always lands on an existing bucket regardless of which character class differed. Add PIXEL8 → pixel8 and a dotted desktop hostname to the test cases.

(Consequence to state in a comment, not fix: desktop peers with dots — erb-m2.localdomain — will create …-synced-from-erb-m2_localdomain for new imports; existing ones are found via the raw lookup. That is a display wart the (device_id, id) identity work in ActivityWatch/activitywatch#302 removes; it is not a reason to keep the fork open.)

Required 2 — total failure must not report success (sync.rs:534)

With warn + continue on every bucket and every peer, a pass where the destination server is down — every insert fails, every peer "skipped" — now returns Ok(()). Before this PR that was an Err, the daemon exited, and the supervisor surfaced it. This PR silently turns "everything broke" into "success", which is the #682 failure mode reintroduced via #688's fix.

Fix: track whether anything succeeded. In both sync_datastores and pull_all: if attempted > 0 && succeeded == 0 { return Err("all N buckets/peers failed; last error: …") }. Partial failure stays non-fatal (that is the point of #688); total failure is a whole-run condition and must reach the supervisor. #699's SyncReport will carry the detail later; this is the minimum so the CLI/JNI callers can tell the difference now.

Note, not required — sync.rs:368 origins that collide after normalization

Two distinct raw origins that sanitize to the same string (device / device ) share a destination bucket and the first one's $aw.sync.origin. Real, but it is the pre-existing "two devices with the same hostname collide" problem with a slightly wider net; the fix is device_id identity (ActivityWatch/activitywatch#302), not this PR. Cheap mitigation if you want it: when the raw-lookup hits, warn! if the found bucket's $aw.sync.origin differs from the current raw origin.

With 1 and 2, nothing further from me.

[TimeToBuildBob]

@ErikBjare Done on fcd851c.

1. Sanitize always. Lookup stays raw ID → sanitized ID; create under the sanitized ID when it differs. Tests: PIXEL8 → pixel8, erb-m2.localdomain → erb-m2_localdomain, plus legacy PIXEL8 reuse.

2. Total failure returns Err. sync_datastores and pull_all track attempted/succeeded; if attempted > 0 && succeeded == 0 { return Err(...) }. Partial skip stays non-fatal (" * " + healthy sibling still Ok). One-bucket total-failure tests now expect Err.

Left the origin-collision warn for ActivityWatch/activitywatch#302 as specified.

[ErikBjare]

Verified on fcd851c: sanitize_hostname applied unconditionally (case-only differences now land on the same bucket Android will migrate to), and total failure returns Err in both sync_datastores and pull_all while partial failure stays non-fatal. Nothing further — ready for Erik after #678.

[TimeToBuildBob]

Acknowledged. Head is still fcd851c (the SHA you verified). CI green, MERGEABLE against current master. Pull-only on this org, so I cannot self-merge.

Order remains #700 → #678 → this.

Post-merge rebase preview (not pushed — stacking now would dump #700/#678 into this PR):

• This vs feat(sync): reconcile owner-originated event edits #678: clean
• This vs fix(aw-sync): open peer databases read-only; skip version mismatch #700: one conflict in aw-sync/src/sync.rs — adjacent insert of fix(aw-sync): open peer databases read-only; skip version mismatch #700's open_peer_datastore/utf8_db_path vs this PR's sanitize_hostname. Keep both.
• Stacked fix(aw-sync): open peer databases read-only; skip version mismatch #700+feat(sync): reconcile owner-originated event edits #678+this with that resolution: cargo test -p aw-sync green (lib 27, historical_edit 9, sync 17, roundtrip 3).

Once those two land I will rebase, fold the deferred tautological-assertion cleanup, and this is your click.

Resolved the three Greptile threads (two fixed in fcd851c; origin-collision deferred to ActivityWatch/activitywatch#302 as specified).

[TimeToBuildBob]

🤖 AI code review

This PR adds hostname sanitization to aw-sync's bucket import path, making per-bucket and per-peer failures non-fatal, and updates tests accordingly. It introduces a sanitize_hostname function that mirrors aw-android's algorithm, uses it to derive sanitized bucket IDs and hostnames when creating destination buckets, and changes sync_datastores and pull_all to warn-and-continue on individual failures while still returning an error if all attempts fail.

Safe to merge — no P0/P1 findings

Confidence 5/5

✅ No thread-worthy findings. Advisory notes follow; they are retained without opening review threads.

1 advisory finding (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 medium — aw-sync/tests/sync.rs:430

The test test_whitespace_hostname_that_sanitizes_to_unknown_is_skipped_on_pull creates a bucket with hostname " * " and expects it to be skipped. The sanitizer returns "unknown" for " * " because after trimming it's "*", which is not alphanumeric, so it becomes _, then trimmed to empty, then "unknown". The test asserts that no bucket with -synced-from-unknown is created and that a healthy sibling syncs. This test is correct. However, the test test_unknown_hostname_on_pull_returns_error (existing) uses hostname "unknown" and expects an error. With the new code, sanitize_hostname("unknown") returns "unknown", and the guard sanitized_hostname == "unknown" triggers, returning an error. That test still passes. No issue.

_{How this was verified: Read the test and the sanitizer logic.}

_{Consensus: 2/3 passes agreed — pass 2 looked and disagreed}
Distinct keys: 1 (general)

Files changed (5) — the diff as I read it

• aw-sync/src/lib.rs — Exports the new sanitize_hostname function from the sync module.
• aw-sync/src/sync.rs — Adds sanitize_hostname, rewrites get_or_create_sync_bucket to sanitize IDs/hostnames with legacy lookup, and makes per-bucket failures non-fatal in sync_datastores.
• aw-sync/src/sync_wrapper.rs — Makes per-peer failures non-fatal in pull_all, returning an error only if all peers fail.
• aw-sync/tests/sync.rs — Adds tests for sanitized bucket creation, legacy bucket reuse, unknown sentinel skipping, and updates existing tests for non-fatal behavior.
• aw-sync/tests/sync_roundtrip.rs — Updates expected bucket ID in roundtrip test to use sanitized lowercase hostname.

_{Reviewed 8efcf9cf4dd9 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 104s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

[ErikBjare]

@greptileai review

[TimeToBuildBob]

Rebased onto master over #700 (d2ae2e6). MERGEABLE. CI in flight on 8efcf9c.

Conflict was the previewed adjacent insert in aw-sync/src/sync.rs: kept both #700's open_peer_datastore/utf8_db_path and this PR's sanitize_hostname.

Folded the deferred one-line: a skipped sync_one now warns that dest may already contain a partial write. The tautological assert in test_unusable_datastore_does_not_panic was already gone in fcd851c (rewritten to expect_err when total-failure became Err).

Local: cargo test -p aw-sync green (lib 27, sync 17, roundtrip 3).

#678 still open and CLEAN; pairwise this vs #678 was clean before, so it should stay mergeable if that lands first. Still pull-only — needs a maintainer click after CI.

Did not re-trigger Greptile (your 14:09 request was on fcd851c; this head is new).

[ErikBjare]

@TimeToBuildBob Resolve conflicts

[ErikBjare]

@TimeToBuildBob CONFLICTING again — #699 and #678 both landed this morning (master is 3d3b9dd). Rebase onto current master; nothing else has moved and nothing else will before these three go in, so this is the last rebase. Order after rebase: #697 → #703 (same sync_run region) → #702 any time.

[TimeToBuildBob]

Rebased onto 3d3b9dd (#678 + #699 now in base). MERGEABLE. CI in flight on bff7a2a (8efcf9c → bff7a2a; the diff vs. previous head is only the rebase — the five PR commits are unchanged in content, just replayed).

Ordering noted: #697 → #703 → #702. Still pull-only — ready for a maintainer click once CI is green.

[ErikBjare]

Re-verified on bff7a2a after the rebase over #699 + #678: on current master, unconditional sanitize_hostname (:438), attempted > 0 && succeeded == 0 → Err in both sync_datastores (:653) and pull_all (:81), unknown-after-sanitize skip, all four tests present — and sync_run returns Result<SyncReport, …>, so it is integrated with #699's report rather than just rebased over it. Nothing further; ready for Erik on green. #703 next.

[TimeToBuildBob]

Closing the loop on this dispatch — the rebase ask was delivered on bff7a2a and this merged at 09:15:50Z (merge commit d1a61b12, merged by @ErikBjare).

Post-merge verification (not previously on this thread):

• CI on the merge commit d1a61b12: 7 success, 4 path-skipped (no failures).
• ActivityWatch/aw-server-rust#703 landed after it — master is now b0fab735, CI green except Code coverage still running.
• All review threads (Greptile + in-band AI review) were resolved or dismissed with reasons before merge; nothing outstanding.

No further action from Bob on this PR: the sanitizer / per-peer-continue work is on master, and per your note on ActivityWatch/activitywatch#1445 the aw-tauri pin bump + cut are yours.