Skip to content

fix(android): force-commit setSetting before reporting ok - #817

Merged
ErikBjare merged 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/android-set-setting-force-commit
Oct 14, 2026
Merged

ErikBjare merged 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/android-set-setting-force-commit

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

Follow-up to #816, found while verifying ActivityWatch/aw-android#365 on an emulator.

Why

The datastore worker batches SetKeyValue into its transaction and commits on a ~15 s interval (or after 100 events, or on a forced commit). setSetting returned "ok" as soon as the write was queued.

Its first caller, the aw-notify opt-in migration in aw-android#365, sets hasMigratedNotifyOptIn on "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 after ok and only appeared after the batch commit.

What

setSetting calls force_commit() after set_key_value and 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/settings is unchanged.

Verification

  • cargo fmt --check -p aw-server: clean.
  • The local x86_64-linux-android check could not run here: vendored OpenSSL fails to build with NDK 29. This PR's Android CI job compiles it.
  • The change is behavioral only for the Android JNI path (cfg(target_os = "android")); force_commit is the same call endpoints/query.rs already makes.

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
@greptile-apps

greptile-apps Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; the setting write now commits before Android receives success.

Summary

Android setSetting now writes and commits through one worker command before returning "ok".

  • Android settings now get an ok only after the database commits them.

Diagram

sequenceDiagram
    participant Android
    participant Worker
    participant SQLite
    Android->>Worker: SetKeyValueAndCommit
    Worker->>SQLite: Write setting and commit
    alt Commit succeeds
        SQLite-->>Worker: Success
        Worker-->>Android: ok
    else Commit fails
        SQLite-->>Worker: Error
        Worker-->>Android: Error object
    end
Loading

Reviews (2) · Last reviewed commit: "fix(android): acknowledge setting write ..." · Reviewed by Greptile

Comment thread aw-server/src/android/mod.rs Outdated
@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.80%. Comparing base (656f3c9) to head (dfa4d70).
⚠️ Report is 202 commits behind head on master.

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

Git-Session-Id: 7b4221ca-cf92-5635-b52e-20ffb70fab21
@TimeToBuildBob

TimeToBuildBob commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

Safe to merge — no P0/P1 findings on latest review

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/tests/datastore.rs:953 P2 rejected

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 only

Confidence 4/5

1 finding · ⚠️ 1 P2

⚠️ P2 medium — aw-datastore/tests/datastore.rs:944

The new test setting_is_committed_before_ack does not actually verify that the ack is deferred until commit. It calls ds.set_key_value_and_commit(...).unwrap() and then reads from an independent connection. Because set_key_value_and_commit forces a commit before responding, the read will always see the value. However, the test would also pass if the implementation were changed to a plain set_key_value followed by force_commit, or even if the worker committed before responding for any other reason. The test does not distinguish the new atomic behavior from the old separate set/force_commit pair, so it provides no regression protection for the specific race this PR addresses. The test is not vacuous in the sense that it would fail if the value were not committed, but it does not pin the contract that the ack is only sent after commit; it only pins that the value is committed after the call returns, which is also true of the old set_key_value + force_commit sequence.

How this was verified: The test calls ds.set_key_value_and_commit(...).unwrap() and then immediately queries the independent connection. Since the worker sends the response only after commit, the query always sees the committed value. If the implementation were changed to ack before commit, the test would still pass because the query happens after the ack and after the commit (the commit would happen before the query returns). The test does not attempt to read from the second connection while the write is in flight.

Consensus: 3/3 passes agreed
Distinct keys: 1 (general)

Files changed (3) — the diff as I read it
  • aw-datastore/src/worker.rs — Adds SetKeyValueAndCommit command, set_key_value helper, and Datastore::set_key_value_and_commit method; refactors SetKeyValue handling to use the helper.
  • aw-datastore/tests/datastore.rs — Adds setting_is_committed_before_ack test that writes via set_key_value_and_commit and reads from an independent connection.
  • aw-server/src/android/mod.rs — Changes setSetting to call set_key_value_and_commit instead of set_key_value.
Previous review passes
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.

Comment thread aw-datastore/tests/datastore.rs
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

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
ErikBjare merged commit d0f5074 into ActivityWatch:master Oct 14, 2026
8 checks passed
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