Skip to content

fix(api): cache the inserted heartbeat with its id - #223

Open
TimeToBuildBob wants to merge 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/heartbeat-replace-by-event-id
Open

TimeToBuildBob wants to merge 2 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/heartbeat-replace-by-event-id

Conversation

@TimeToBuildBob

@TimeToBuildBob TimeToBuildBob commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem

ServerAPI.heartbeat() caches the last heartbeat so the next one can merge into it. Master already replaces the merged event by id, falling back to replace_last() when the id is unknown (ActivityWatch/aw-watcher-afk#61).

The insert path discards the return value of self.db[bucket_id].insert(heartbeat), so the cached event lacks its assigned id. Subsequent merges take the fallback, which resolves "last" by maximum timestamp.

After a backward wall-clock step, heartbeat_merge() refuses to merge into the past and inserts an older heartbeat. The next merge then overwrites the newest stored event instead of the older event, destroying the pre-step activity and leaving two rows at the older timestamp. The existing FIXME in aw_server/api.py describes this failure.

Fix

Cache the event returned by insert() (including its id), falling back to the original heartbeat only when insert returns None. Later merges replace the row actually merged into, matching the by-id approach in aw-server-rust.

Test

test_backward_clock_step_does_not_overwrite_newer_event sends t+0, t+5, t−3600, t−3590 with pulsetime=10, using an explicitly UTC baseline.

  • Before: both rows land at t−3600; the t event is gone.
  • After: two events remain, t−3600 with duration 10 and t with duration 5.

End-to-end client/server probe: ErikBjare/bob, knowledge/research/2026-11-05-aw-clock-step-recording-probe.md.

Local verification recorded on 2026-11-06 for head cfb7399151b0: tests/test_server.py 21 passed; full existing make test 145 passed (Python 3.13). The initial sandbox's tests/test_client.py ConnectionError occurred without a live server; it is not the current hosted check result.

CI status / merge gate (2026-11-07)

Not CI-green: lint is failing. Linux, macOS and Windows tests, typecheck, format, CodeQL and Greptile pass on cfb7399151b0.

The prior investigation reproduced 150 Ruff 0.17.3 violations on unmodified master after #218 upgraded the action. #224 explicitly retains the previous 58-rule selection while keeping the upgraded action; its lint check passes. This is a reproduced baseline failure, not a claim of disjoint lint workflow path filters.

Merge remains blocked on landing that repair and refreshing this branch/checks against the repaired base. Passing application tests or review scores do not establish passing lint. The heartbeat implementation is unchanged.

@greptile-apps

greptile-apps Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The code changes appear safe to merge, but the separately reported lint gate still needs to pass.

Summary

ServerAPI.heartbeat() now caches the event returned by insert(), keeping its assigned ID for later merges.

  • Heartbeat merges update the row that was just inserted.

Reviews (3) · Last reviewed commit: "test(api): use UTC in backward-clock hea..." · Reviewed by Greptile

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

CI note, not caused by this diff: both Ubuntu jobs fail during Set up Python with The version '3.9' with architecture 'x64' was not found for Ubuntu 26.04, so the Ubuntu leg never reaches the tests. macOS and Windows pass (2m33s / 2m38s), and lint, format, CodeQL and Greptile are green.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Fixed the runner mismatch in 065e9be: both Python 3.9 jobs now use ubuntu-22.04 instead of ubuntu-latest. The Python version, Windows/macOS legs and heartbeat implementation are unchanged.

Validated the workflow matrices and confirmed an Ubuntu 22.04 x64 Python 3.9 build exists in the setup-python manifest. The full local make test target passes: 145 passed (local Python 3.12). Pushed the fix; the new GitHub checks still need to complete.

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

@TimeToBuildBob

TimeToBuildBob commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

ServerAPI.heartbeat() now caches the event returned by insert(), retaining its assigned id, and falls back to the original heartbeat when insert() returns None. A new HTTP regression test sends heartbeats across a backward clock step and checks the timestamps and durations of the two stored events.

Safe to merge — no P0/P1 findings

Confidence 5/5

✅ No findings. The diff looks correct to me on this pass.

Files changed (2) — the diff as I read it
  • aw_server/api.py — Captures insert()'s return value and uses it for last_event[bucket_id], with the original heartbeat as a fallback.
  • tests/test_server.py — Imports timezone and adds test_backward_clock_step_does_not_overwrite_newer_event with four heartbeats and assertions on stored timestamps and durations.
Previous review passes
commit score findings engine when
065e9be8ee54 4/5 1 subscription 2026-11-05 05:43 UTC
8edeab960a91 5/5 0 panel 2026-11-05 06:37 UTC
cfb7399151b0 3/5 1 subscription 2026-11-06 20:23 UTC
cfb7399151b0 5/5 0 subscription 2026-11-07 17:23 UTC

Reviewed 79847368fcc8 · panel: codex:gpt-6.1-sol · subscription engine · 10s · 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 tests/test_server.py Resolved
@TimeToBuildBob
TimeToBuildBob force-pushed the fix/heartbeat-replace-by-event-id branch from 8edeab9 to cfb7399 Compare November 6, 2026 15:41
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Rebased onto current master f2aec9707b63aeb4c6f8dd40430bf5a992a4ea4e (includes the Ubuntu 24.04 pin in 5e0ae8f). Dropped the now-redundant pure runner commit 065e9be; the heartbeat implementation and UTC regression are byte-for-byte unchanged from the previous head. New head: cfb7399151b0cc1ca3568bf7bf224a8ac90b5502.

There is now no .github diff against master; both Linux jobs inherit Ubuntu 24.04 and the existing setup/action versions from master. Local Python 3.13 verification: tests/test_server.py 21 passed; full existing make test 145 passed. Force-with-lease push completed with hooks enabled, and the Greptile helper accepted one fresh review request for this exact head. Latest-head CI/review is pending; no merge performed.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

The remaining failure is inherited from master: #218 upgraded Ruff to 0.17.3, whose expanded default rule selection reports 150 violations on unmodified master. This PR's Linux/macOS/Windows tests, typecheck and format checks all pass.

Opened #224 to make the prior lint selection explicit while retaining the upgraded action. Both Ruff versions pass with that configuration, its effective enabled codes exactly match the previous 58-rule baseline, and an undefined-name control still fails. This heartbeat branch also passes Ruff 0.17.3 using the repair configuration.

No heartbeat code changes or reruns of the deterministically failing check. The CI repair needs a maintainer merge first; then this PR needs a refreshed lint run against the repaired base.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Updated the PR description to replace the initial sandbox-only test note with the recorded full-suite results and an explicit lint is still failing merge gate. The summary-only P1 was triggered by that old wording; its preflight now returns no finding, while a control with the old wording still reproduces it.

Refreshed the existing AI-review comment on unchanged head cfb7399151b0: 5/5, no findings, explicitly labelled one-pass outage coverage, not consensus merge evidence. The previous UTC inline finding already has its fix reply and is resolved; no human threads were changed.

No code push, CI rerun or merge. #224 remains the lint repair to land first, followed by refreshing this branch and obtaining passing exact-head checks. This review refresh does not clear that CI blocker.

The merge path already replaces the event it merged into by id when that id
is known (aw-watcher-afk#61). The insert path discarded insert()'s return
value, so the cached heartbeat had no id and every merge fell back to
replace_last(). That fallback targets whichever event sorts last by
timestamp, so after an out-of-order heartbeat (a backward NTP or manual
clock step) it overwrote and destroyed a newer stored event, leaving two
rows at the same timestamp.

Cache the event as stored and add a regression test that steps the clock
back 3600 s: it fails before this change and passes after.

Git-Session-Id: 2459c02a-4b33-51b6-89e9-623b6e0f4df1
Git-Session-Id: 348b38df-3329-54e6-891f-357385c887c9
@TimeToBuildBob
TimeToBuildBob force-pushed the fix/heartbeat-replace-by-event-id branch from cfb7399 to 7984736 Compare November 7, 2026 19:35
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@greptileai review

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Resolved the merge conflict and force-with-lease pushed 79847368fcc801d86e9e4e64fe23fb870fa203c2, rebased onto master 7af1920. The only manual conflict was the test import block: retained master's quote import and this PR's timezone import. The heartbeat fix and UTC regression are otherwise unchanged; there is no workflow diff.

Verification on the rebased head: full make test 170 passed (Python 3.13, pytest 8.4.2, Peewee 3.17.0 as recorded in poetry.lock); the clock-step regression also passes under UTC, Europe/Stockholm and America/Los_Angeles. Black and git diff --check pass. An initial unconstrained local install selected Peewee 4.5.3 and produced three SQLite fixture errors; using the lockfile's Peewee version cleared those errors without a source change.

GitHub now reports mergeable, but not CI-green: current-head lint fails, format passes, and the remaining checks are running. Local Ruff 0.17.3 still reports the expanded-rule baseline failures, so #224 remains the separate lint repair. Requested a fresh Greptile review for this exact head; the previous head's 5/5 is not current-head evidence. No merge performed.

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.

1 participant