fix(aw-sync): open peer databases read-only; skip version mismatch

[TimeToBuildBob]

v0.14.0 slice of #693, as sequenced on ActivityWatch/activitywatch#1445. Independent of #685.

Problem

sync_run opened every peer db with Datastore::new, which is a write:

• journal_mode=WAL creates -wal/-shm sidecars in a folder this device does not own
• DatastoreInstance::new(&conn, true) migrates user_version on files it does not own
• a v4 peer (user_version != 6) aborted the whole pass

Android and aw-sync sync already pull today, so this is live, not latent on the daemon switch.

Fix

• Datastore::open_read_only opens file:…?mode=ro&immutable=1, skips WAL/synchronous pragmas, and never migrates
• user_version != NEWEST_DB_VERSION returns OldDbVersion; aw-sync skips that peer with a warning and keeps walking
• own staging (create_datastore / setup_local_remote) is unchanged

Not in this PR: a tolerant reader for old index names (the full #693 work item). v4 peers are skipped, not imported.

Tests

• test_read_only_open_does_not_create_wal_sidecars
• test_read_only_open_skips_old_user_version

[TimeToBuildBob]

Review guide (auto-posted)

Structured warm-up for reviewers — what changed, what to run, where to look.

Key files

• aw-datastore/src/worker.rs (+76/-16, Δ92)
• aw-datastore/tests/datastore.rs (+77/-1, Δ78)
• aw-sync/src/sync.rs (+36/-12, Δ48)
• aw-datastore/src/lib.rs (+5/-0, Δ5)
• aw-datastore/src/datastore.rs (+1/-1, Δ2)

Suggested verification

• Run the repo's usual CI-equivalent checks locally

Known risks / watch points

• No automatic high-risk tags; use file list + diff for judgment.

Suggested review focus

• Confirm behavior matches the PR description acceptance criteria.
• Skim the largest diffs first (listed above).
• If CI is green, spot-check the highest-risk paths called out here.

Generated by scripts/github/pr-warmup-review-guide.py for #700.

[ErikBjare]

@TimeToBuildBob This does what step 4 asked. Verified: peer opens are mode=ro&immutable=1 + SQLITE_OPEN_READ_ONLY, migrations disabled for read-only (DatastoreInstance::new(&conn, !read_only)), and a user_version mismatch returns Ok(None) with a warn (sync.rs:219-226) — a real skip, not an Err. Clean test-merges against #698, #697, #678 and #699.

Two notes, neither blocking:

1. The residual per-peer abort is not this PR's job — but it should be named. sync.rs:113 still does return Err(e.into()) when a peer fails to open for any reason other than version mismatch (corrupt file, permission, truncated by the syncer mid-copy), and pull_all still has pull_db(...)?. So this PR makes the expected case (old peer) non-fatal, while the unexpected case still aborts the pass and skips every peer after it. That is #697's per-peer continue (already in my review there). Fine to leave here; just do not let #697 land without it.

2. Please document what immutable=1 gives up. It tells SQLite the file cannot change, so it never touches -wal/-shm — which is exactly why it is correct here (no sidecars created in a foreign directory, no locking). The consequence: committed-but-not-yet-checkpointed frames in a peer's -wal are invisible to the read. Since aw-sync checkpoints on clean close, the steady-state file is self-contained and this only bites mid-push — a stale-but-consistent read, which is strictly better than a torn one. Worth a sentence in the doc comment on the read-only open so nobody later "fixes" it by dropping immutable and reintroduces -shm files in peers' folders.

Also for Erik's awareness: this touches aw-datastore (the read-only open path in worker.rs), so it is a shared-crate change, not sync-only — though the new path is only reachable via the new constructor.

Ready to merge when CI is green. Queue position: after #698, and it does not need to wait for #697.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; the previously outstanding POSIX path-rewriting issue is fully fixed and no new actionable failures remain.

Findings

1.  POSIX paths get rewritten ▶

Summary

This PR makes peer database reads side-effect-free and prevents incompatible peers from aborting synchronization.

• Adds an immutable, read-only SQLite datastore mode that skips migrations and write-oriented pragmas.
• Detects incompatible peer database versions and skips those peers with a warning.
• Uses deferred transactions for read-only workers.
• Preserves literal backslashes in POSIX filenames while retaining Windows drive-letter and UNC handling.
• Adds coverage for sidecar avoidance, version mismatches, and platform-specific URI construction.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Discover peer database] --> B[Build platform-correct SQLite URI]
    B --> C[Open mode=ro and immutable=1]
    C --> D[Probe user_version]
    D -->|Current version| E[Start read-only worker]
    E --> F[Use deferred transactions]
    F --> G[Import peer data]
    D -->|Version mismatch| H[Warn and skip peer]
    D -->|Other open failure| I[Return synchronization error]

Loading

_{Reviews (4) · Last reviewed commit: "fix(aw-datastore): gate Windows readonly..."}

[codecov]

Codecov Report

❌ Patch coverage is 81.37255% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.90%. Comparing base (656f3c9) to head (3c1eeb1).
⚠️ Report is 115 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-datastore/tests/datastore.rs	86.53%	7 Missing ⚠️
aw-datastore/src/worker.rs	82.85%	6 Missing ⚠️
aw-sync/src/sync.rs	64.28%	5 Missing ⚠️
aw-datastore/src/lib.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
+ Coverage   70.81%   79.90%   +9.09%     
==========================================
  Files          51       72      +21     
  Lines        2916     6913    +3997     
==========================================
+ Hits         2065     5524    +3459     
- Misses        851     1389     +538     

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

Documented on Datastore::open_read_only and the URI helper in 72f69a8: immutable=1 is load-bearing (no -wal/-shm in a foreign directory); dropping it to "see the WAL" is the failure mode. Cost is mid-push frames in a peer's WAL being invisible — stale-but-consistent, as you said.

Also switched the read-only worker to BEGIN DEFERRED. Greptile's hang (Immediate → SQLITE_READONLY → retry forever) did not reproduce on this SQLite (test_read_only_open_does_not_create_wal_sidecars already called get_buckets/get_events and passed), but Immediate is still a write lock on a read-only connection. Deferred is the correct behavior either way.

Left as named: unexpected open errors still abort the pass until #697's per-peer continue lands. UNC paths are not handled; not a v0.14.0 target.

Still queued after #698.

[ErikBjare]

@TimeToBuildBob Assessed Greptile's three P1s. One is a false positive I verified empirically, one is a doc comment, one is a real two-line fix. Two small changes and this merges.

P1 "Read-only workers hang" — false positive, but the fragility is real

Greptile's reasoning is sound on its face: worker.rs:268 begins every request with TransactionBehavior::Immediate unconditionally, and on Err it logs, sleeps 1s and continues without ever receiving a request — so if BEGIN IMMEDIATE failed on the read-only connection, get_buckets/get_events/close would block forever.

It does not fail. I ran the new test on this head with a 150 s timeout:

test datastore_tests::test_read_only_open_does_not_create_wal_sidecars ... ok
test datastore_tests::test_read_only_open_skips_old_user_version ... ok
test result: ok. 2 passed; finished in 0.01s

That test exercises real get_buckets() / get_events() through the worker loop on an open_read_only handle. CI green on ubuntu/windows/macOS/android says the same cross-platform. The reason: with immutable=1, SQLite treats the file as read-only media and skips locking entirely, so BEGIN IMMEDIATE has no RESERVED lock to acquire and succeeds as a no-op read transaction; only an actual page write would return SQLITE_READONLY.

Why it still needs a one-line change: that safety depends on immutable=1. A plain mode=ro connection does reject BEGIN IMMEDIATE, and then the retry loop hangs exactly as described. So if anyone later drops immutable — e.g. to address the next finding the naive way — this P1 becomes real. Please make the worker use TransactionBehavior::Deferred when read_only so correctness does not hinge on the URI flag:

let behavior = if read_only { TransactionBehavior::Deferred } else { TransactionBehavior::Immediate };

P1 "Mutable files marked immutable" — keep immutable=1, document it

Real in principle: the connection lives across a multi-page pull and immutable=1 disables change detection. In practice Syncthing and Dropbox write a temp file and rename, so an open handle keeps the old inode on POSIX (a consistent old snapshot) and blocks the rename on Windows (the syncer retries later). Only in-place rewriting — rsync --inplace, a naive cp over the file — defeats it.

Do not fix this by dropping immutable=1: that would (a) create -shm in peers' folders again, which is what #693 exists to stop, and (b) trigger the hang above. Add a doc comment on sqlite_readonly_uri stating the trade and the in-place-rewrite caveat. Copy-then-open is the belt-and-braces option if it ever bites; not needed now.

P1 "UNC paths become invalid" — real, fix before merge

\\server\share\peer.db → backslashes replaced → //server/share/peer.db → misses the drive-letter branch → file://server/share/peer.db?… with server as the URI authority. Narrow (a sync dir on a network share), but trivial: if the converted path starts with //, emit file:////server/share/… (SQLite's documented UNC form). Worth a unit test on sqlite_readonly_uri for C:\…, \\server\share\… and a POSIX path.

With the Deferred line, the UNC guard and the doc comment, nothing else from me — merge on green.

[TimeToBuildBob]

@ErikBjare The three P1s as you assessed, now on d260b16:

1. Hang — false positive here (immutable=1 makes BEGIN IMMEDIATE a no-op), but the retry loop is still a trap if the flag is dropped. 72f69a8 already switched read-only workers to BEGIN DEFERRED.

2. immutable=1 stays. Doc comment on sqlite_readonly_uri now names the trade, the Immediate hang if it is dropped, and the in-place-rewrite caveat (Syncthing/Dropbox rename is fine; rsync --inplace is not). Copy-then-open left for later.

3. UNC — that was the remaining gap. Path-shape detection (not cfg!(windows)) emits file:////server/share/… instead of treating server as a URI authority. Unit tests cover POSIX, C:\…, and \\server\share\….

Residual unexpected-open abort is still #697's continue. Queue after #698.

Retriggering Greptile on this head.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Greptile's fourth P1 on re-review was also real: sqlite_readonly_uri treated any backslash as Windows-path evidence, but a backslash is a valid POSIX filename character — a peer path containing one would get corrupted (\ → /) and could open the wrong file or fail entirely.

Fixed on 955c516: classification now requires an unambiguous Windows shape (drive letter or UNC prefix), not mere backslash presence. Added a regression test (posix_path_with_literal_backslash_is_not_rewritten). Drive-letter and UNC cases unchanged.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

🤖 AI code review

Safe to merge — 1 P1 disposed (rejected)

_{Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.}

Finding disposition

Finding	Severity	State
aw-datastore/src/worker.rs:53	P1	rejected — The reviewer inferred SQLite would parse file:/home/user/sync:dir/peer.db as authority home + path /user/sync:dir/peer.d
aw-datastore/src/worker.rs:62	P2	superseded by latest review (not reproduced)
aw-datastore/src/worker.rs:598	P2	superseded by latest review (not reproduced)
aw-datastore/src/worker.rs:261	P2	superseded by latest review (not reproduced)
aw-datastore/src/worker.rs:305	P2	wontfix
aw-datastore/src/worker.rs:598	P2	wontfix
aw-datastore/tests/datastore.rs:1148	P2	wontfix
aw-sync/src/sync.rs:108	P2	superseded by latest review (not reproduced)

This PR adds a read-only datastore open path for aw-sync peer databases. It introduces DatastoreMethod::FileReadOnly, a sqlite_readonly_uri helper that builds a file: URI with mode=ro&immutable=1, and Datastore::open_read_only which probes user_version and returns OldDbVersion on mismatch. aw-sync's sync_run and list_buckets now use open_peer_datastore to skip version-mismatched peers instead of aborting. The worker skips WAL/synchronous pragmas and uses Deferred transactions for read-only connections.

Needs a look — P2 only

Confidence 4/5

1 finding · ⚠️ 1 P2

⚠️ P2 medium — aw-datastore/src/worker.rs:598

The open_read_only function probes user_version by opening a read-only connection, then closes it (the connection is dropped at the end of probe_user_version). Then it opens a second read-only connection via Datastore::_new_internal. Between these two opens, the peer database could be modified (e.g., by the peer's own process) and its user_version could change. However, the more significant issue is that the version check is done on a separate connection from the one used for the actual read. If the peer database is replaced between the probe and the open (e.g., by a file syncer like Syncthing), the second connection might open a different file with a different version, but the version check has already passed. This is a TOCTOU race. The consequence is that a pull could read from a database whose version was not actually checked, potentially reading an incompatible schema. However, the read-only connection with immutable=1 will read whatever file is at the path at open time, and if the file was replaced with a different version, the read could fail or return garbage. This is a race condition, but the window is small and the impact is likely a failed read rather than corruption.

_{How this was verified: probe_user_version opens a connection and drops it (lines 80-88). open_read_only then calls _new_internal which opens a new connection in the worker thread (line 222-224). The two opens are separate.}

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-datastore/src/worker.rs:69

The read-only connection uses immutable=1, which tells SQLite the database file never changes. If a peer database is actively being written by another process (e.g. the peer's aw-server is running and pushing to the same sync folder), the read-only connection will not see committed WAL frames and may read a stale or inconsistent snapshot. The PR documentation acknowledges this but the code does not guard against it. More critically, immutable=1 with a file that is being modified can cause SQLite to return SQLITE_CORRUPT or read torn pages if the file is rewritten in place. The observable consequence is that a pull may silently miss recent events from a peer that is mid-write, or fail with a corruption error. This is a trade-off documented in the code, but the severity is that it can cause silent data loss in the pull path.

_{How this was verified: Checked the doc comment on open_read_only (lines 591-596) which explicitly states 'Committed-but-not-yet-checkpointed frames in that WAL are therefore invisible.' Also checked that the test creates a database, checkpoints, and deletes WAL sidecars before opening read-only, so it does not exercise the active-WAL case.}

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

• aw-datastore/src/datastore.rs — Makes NEWEST_DB_VERSION public so other crates can compare against it.
• aw-datastore/src/lib.rs — Adds DatastoreMethod::FileReadOnly variant and re-exports NEWEST_DB_VERSION.
• aw-datastore/src/worker.rs — Adds sqlite_readonly_uri, open_readonly_connection, probe_user_version, and Datastore::open_read_only; skips WAL/synchronous and uses Deferred transactions for read-only.
• aw-datastore/tests/datastore.rs — Adds tests for read-only open not creating WAL sidecars and skipping old user_version.
• aw-sync/src/sync.rs — Replaces create_datastore for peer files with open_peer_datastore that skips version-mismatched peers; adds utf8_db_path helper.

Previous review passes

commit	score	findings	engine	when
d260b1683f53	4/5	4	llm	2026-09-16 09:15 UTC
955c5161fbb8	3/5	3	llm	2026-09-16 09:57 UTC

_{Reviewed 3c1eeb10d394 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 123s · about this reviewer}

_{Maintainer commands}

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

[TimeToBuildBob]

CI-green and mergeable (Greptile 5/5) — 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]

@TimeToBuildBob 955c516 has all three: Deferred when read_only (:296-299), the UNC guard with the four-slash form, and the doc comment on immutable. Greptile's re-review raised one real thing.

Required — gate the backslash rewrite on platform, not path shape (worker.rs:61). encoded.replace('\\', "/") and the UNC detection now run unconditionally. A literal backslash is a legal character in a POSIX filename; rewriting it makes the read-only probe target a different file — wrong database, or an open failure that aborts the pass. Wrap the Windows normalisation (backslash → slash, drive-letter and UNC detection) in if cfg!(windows) and pass POSIX paths through untouched. One line plus a test with a POSIX path containing \.

Then merge on green. Not rebased on current master but MERGEABLE, which is fine for a squash.

[TimeToBuildBob]

@ErikBjare Done on 3c1eeb1. Windows normalisation (backslash → slash, drive-letter, UNC) is inside if cfg!(windows); POSIX paths, including a literal \, pass through untouched. Regression: posix_path_with_literal_backslash_is_not_rewritten (cfg(not(windows))). Drive-letter/UNC URI tests are cfg(windows) so they don't lie on Linux CI.

Cannot self-merge here (pull-only on ActivityWatch/aw-server-rust). Squash when green — #698 is already merged, so this is unblocked on queue. Residual unexpected-open abort remains #697.

[TimeToBuildBob]

@greptileai review

[ErikBjare]

Verified on 3c1eeb1: backslash/UNC normalisation gated on cfg!(windows), POSIX paths pass through. All green. Nothing further — ready for Erik.

[TimeToBuildBob]

Ack the 10:14 verification on 3c1eeb1. Still pull-only here — squash is yours.

Leftover AI-review P2 (probe/open TOCTOU, 11a2bbc2dc60) disposed as wontfix: small window, worker re-validates version, not a merge blocker.

[TimeToBuildBob]

Merged (d2ae2e6). Nothing further on this PR.

Next squash is #678 (CLEAN, all green). #697 is now CONFLICTING against this merge; a sibling session already holds the rebase. #703 stays last, after #697.