Repository navigation
Conversation
SetKeyValue is batched by the datastore worker for up to ~15s, so setSetting could return "ok" for a write that is lost if the process dies before the batch commits. The aw-android notify opt-in migration marks itself done on "ok", so a lost write was never retried. Git-Session-Id: 94600cc0-7dec-4a5f-9e17-5e6cf50ce7ad
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #817 +/- ##
===========================================
+ Coverage 70.81% 84.80% +13.98%
===========================================
Files 51 82 +31
Lines 2916 11633 +8717
===========================================
+ Hits 2065 9865 +7800
- Misses 851 1768 +917 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Git-Session-Id: 7b4221ca-cf92-5635-b52e-20ffb70fab21
🤖 AI code reviewSafe to merge — no P0/P1 findings on latest reviewUpdated 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
Adds a new SetKeyValueAndCommit worker command and Datastore::set_key_value_and_commit method that writes a key/value and forces a commit in the same worker batch, so the response is only sent after the transaction commits. The Android setSetting JNI path now uses this method instead of set_key_value, and a new integration test verifies the value is visible to an independent SQLite connection after the call returns. Needs a look — P2 onlyConfidence 4/5 1 finding ·
|
| commit | score | findings | engine | when |
|---|---|---|---|---|
dfa4d700f60c |
4/5 | 1 | llm | 2026-10-14 11:25 UTC |
Reviewed dfa4d700f60c · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 33s · about this reviewer
Maintainer commands
@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.
|
@greptileai review |
|
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. |
Follow-up to #816, found while verifying ActivityWatch/aw-android#365 on an emulator.
Why
The datastore worker batches
SetKeyValueinto its transaction and commits on a ~15 s interval (or after 100 events, or on a forced commit).setSettingreturned"ok"as soon as the write was queued.Its first caller, the aw-notify opt-in migration in aw-android#365, sets
hasMigratedNotifyOptInon"ok". If the process dies before the batch commits, the setting is lost and the migration is never retried. On a fresh install that means the key stays missing, and NotifyWorker treats a missing key as the legacy default-on, which is the opposite of opt-in. On device, the stored row was indeed absent right afterokand only appeared after the batch commit.What
setSettingcallsforce_commit()afterset_key_valueand reports"ok"only once the write has committed. A commit failure returns an error object, so the Android caller retries on the next start.POST /api/0/settingsis unchanged.Verification
cargo fmt --check -p aw-server: clean.x86_64-linux-androidcheck could not run here: vendored OpenSSL fails to build with NDK 29. This PR's Android CI job compiles it.cfg(target_os = "android"));force_commitis the same callendpoints/query.rsalready makes.