feat(dirs): content-based DB selection when both Local and Roaming have data

[TimeToBuildBob]

Problem

When both Windows Local and Roaming directories contain databases, always choosing Local can hide months of v0.14.0 history behind a stale Local test database.

Scope: selection only

Partial progress on #810; does not close that issue. This PR chooses which existing database to use. It does not merge histories, rename an occupied Local database aside, persist Python-import recovery, or update the Tauri lock. Those remain in the issue's recovery task.

When Roaming wins and Local already contains a database, the server uses Roaming in place and leaves both originals untouched. It skips the migration copy that would necessarily fail against an occupied Local target. Roaming-only migration remains unchanged.

Selection

• Inspect the current profile's intended database filename (sqlite.db, or legacy sqlite-testing.db), not the first arbitrary .db in a directory.
• Open read-only, preserving visibility of WAL events; require PRAGMA quick_check to succeed.
• Compare (MAX(starttime), COUNT(*)) lexicographically: newest timestamp, then higher count, then Local.
• Keep Local and log a warning if either candidate cannot be inspected. An encrypted/unreadable database is not treated as empty; this inspection does not receive the encryption key.
• Log both successful ranks and the chosen directory.

Verification

Regression coverage includes newer Roaming, newer Local, equal timestamps with unequal counts, complete ties, default versus legacy testing filenames in the same directories, both empty, empty Local versus populated Roaming, corrupt Roaming, unreadable Local, and zero copy calls when Local is occupied.

Three new regressions failed against the original implementation (count tie, unreadable Local, occupied-target copying) before the fix, then passed. All 32 directory tests pass locally.

Production library Clippy and the full server-library test results are recorded in the follow-up review comment. The installed Rust 1.99 test-target Clippy additionally flags a pre-existing field_reassign_with_default in endpoints/extension_cors.rs; that unrelated file is unchanged.

[greptile-apps]

Confidence Score: 5/5

[Critical impact] The PR appears safe to merge; no blocking issues remain.

Summary

The PR chooses between Local and Roaming databases using the current profile’s newest event, then event count. It keeps Local when inspection fails and preserves both databases when Local is occupied.

• The server chooses the newer database when Local and Roaming both have one.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Choose current profile database] --> B{Both candidates exist?}
  B -->|Yes| C[Inspect read-only and compare newest event then count]
  C --> D{Inspection succeeds?}
  D -->|No| E[Keep Local]
  D -->|Yes| F[Choose higher rank; Local wins ties]
  F --> G{Roaming chosen and Local occupied?}
  G -->|Yes| H[Acquire lock and recheck; use Roaming without copying]
  G -->|No| I[Keep Local or migrate Roaming-only data]
  B -->|No| I

Loading

_{Reviews (4) · Last reviewed commit: "test(dirs): observe actual migration loc..." · Reviewed by Greptile}

[codecov]

Codecov Report

❌ Patch coverage is 91.11111% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.76%. Comparing base (656f3c9) to head (068b320).
⚠️ Report is 199 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-server/src/dirs.rs	91.11%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #811       +/-   ##
===========================================
+ Coverage   70.81%   84.76%   +13.95%     
===========================================
  Files          51       82       +31     
  Lines        2916    11560     +8644     
===========================================
+ Hits         2065     9799     +7734     
- Misses        851     1761      +910     

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

🤖 AI code review

Safe to merge — 2 findings disposed (accepted-tradeoff; 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-server/src/dirs.rs:248	P1	accepted-tradeoff
aw-server/src/dirs.rs:351	P1	rejected — Verification says: The lock is acquired in the and_then closure and dropped when the closure returns. This is true but d
aw-server/src/dirs.rs:360	P1	resolved (no disposition on record)
aw-server/src/dirs.rs:1424	P2	fixed
aw-server/src/dirs.rs:211	P2	accepted-tradeoff

Adds content-based directory selection in aw-server/src/dirs.rs: when both Local and Roaming contain a .db, it opens the profile-specific database read-only, runs PRAGMA quick_check, and compares (MAX(starttime), COUNT(*)) to choose between DirChoice::Target and DirChoice::Misplaced. resolve_or_migrate now returns the Roaming path without copying when the chosen database already exists in Local, and a test-only thread-local signals migration-lock contention. rusqlite moves from dev-dependencies to regular dependencies.

Not safe to merge — 1 P1 open

Confidence 3/5

1 finding · ❌ 1 P1

❌ P1 high — aw-server/src/dirs.rs:360

When both dirs hold some .db file but the target lacks the current profile's exact database (e.g. only sqlite-testing.db exists while the default profile needs sqlite.db), choose_module_dir_for_database correctly returns Misplaced via the (Ok(false), Ok(true)) arm. resolve_or_migrate then enters the Misplaced branch and hits the new early return at line 360. Because has_database(target) is true due to the unrelated .db, it returns dir: misplaced, migrated_database: false without ever calling migrate_dir. The user's current-profile database is never copied to Local, so the server keeps using Roaming on every start and the repeated warning about merge recovery is emitted, even though the copy would be safe and the target directory has no conflicting file. The condition should check for the exact database filename the profile uses, not any .db in the directory.

if target.join(db_filename(crate::config::get_profile())).exists() {

_{How this was verified: The early-return condition only looks at the directory, not the filename; a target with only sqlite-testing.db and no sqlite.db triggers it even though the profile's database is absent there. The (Ok(false), Ok(true)) arm in choose_module_dir_for_database returns Misplaced for that case.}

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

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-server/src/dirs.rs:211

database_rank counts rows and picks MAX(starttime) from the profile database without restricting to any particular bucket, but the real aw-datastore events table can contain events whose starttime is 0 or NULL? Actually starttime is NOT NULL in the schema, and the comparison uses (MAX(starttime), COUNT()) across all buckets. A stale Local database that holds event rows for a long-running bucket (e.g. a bucket whose last event is old but which also contains many non-activity events) can be ranked as having a higher count than a newer Roaming database when timestamps tie. The tie-breaker prefers higher COUNT() over Local. If both databases have the same newest timestamp (which happens when one database contains a later event in a rarely-written bucket and the other contains many events all ending at the same time), the choice picks the side with more rows even when the other side's recent activity is more relevant. This does not violate the stated selection contract, but it can select a database that is not the 'newest history' the PR claims to prefer.

_{How this was verified: Traced the (t,m) arm in choose_module_dir_for_database and the test test_content_selection_unreadable_local_is_not_empty which writes a non-SQLite file to target and asserts Target is returned.}

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

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

• aw-server/Cargo.toml — Moves rusqlite 0.30 from dev-dependencies to dependencies so dirs.rs can open SQLite databases at runtime.
• aw-server/src/dirs.rs — Adds database_rank, choose_module_dir_for_database, and content-based selection; changes resolve_or_migrate to skip copying into an occupied target; adds a test-only contention notification and many new tests.

Previous review passes

commit	score	findings	engine	when
6612e1931861	2/5	2	llm	2026-10-10 20:11 UTC
55d01bd36f0e	4/5	1	llm	2026-10-10 21:21 UTC
311a4d837fd0	4/5	2	llm	2026-10-10 22:02 UTC

_{Reviewed 068b3204a8b2 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 742s · about this reviewer}

_{Maintainer commands}

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

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Pushed fe75fcc and 6612e19 through the in-band review wrapper, addressing all four Greptile findings: timestamp/count ties, intended profile filename, unreadable/encrypted Local fallback, and repeated occupied-target copying. Occupied-target selection now also waits for the migration lock and rechecks the choice.

Verification: 91 server-library tests passed, 1 ignored; production-library Clippy and formatting passed. Added regression coverage for uncheckpointed WAL events, corrupt/empty databases, profile filenames, count ties, unreadable Local, zero copy calls, and lock/recheck behavior. Test-target Clippy on installed Rust 1.99 flags a pre-existing initializer lint in extension_cors.rs; that unrelated file is unchanged.

Replied to and resolved all four original Greptile threads, then requested one fresh review via the helper. The two in-band findings were disposed with explicit reasons: conservative inspection failure is an accepted recovery tradeoff, and the claimed post-lock migration ignores the occupied-target guard. No open findings of ours remain; new-head CI/review is pending.

This remains selection-only, not the complete v0.14.2 recovery in #810. Both histories remain intact, but automatic merging, occupied-Local rename-aside, durable Python-import recovery, Tauri pins, and release verification are still outstanding. Corrected the PR body so it no longer claims to close that issue.

[TimeToBuildBob]

@greptileai review

[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.

[TimeToBuildBob]

@greptileai review