Repository navigation
feat(queue): merge queued heartbeats per bucket before dispatch - #127
TimeToBuildBob wants to merge 7 commits into
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…, per-request retry Greptile review feedback on ActivityWatch#127: - **Order**: group by bucket, not by full endpoint, and only merge a contiguous run that shares one endpoint. Grouping by endpoint reordered a bucket's heartbeats when their pulsetimes differed (e.g. 10, 20, 10 was sent as 10, 10, 20), which changes the server-side timeline. - **Malformed endpoint**: `pulsetime=1..2` matched the regex but `float()` raised inside coalescing, outside the dispatch error handler, killing the worker. Parsing is now fallible and a malformed pulsetime falls back to sending the request verbatim. - **Partial retry**: dispatch one coalesced request per call and track the index, so a transient error retries only the failed request instead of replaying heartbeats that already reached the server. - **Shutdown**: because one request is dispatched per call, `disconnect()` observes `stop()` between requests instead of blocking on a whole batch. Tests: 21 passed (new: pulsetime-order preservation, malformed pulsetime, partial-retry-does-not-replay). Git-Session-Id: b337
|
Addressed all four findings in f7e463c:
|
|
@greptileai review |
|
Round 2 addressed in 9c0899a:
21 passed; ruff/mypy clean. |
|
@greptileai review |
|
Converged at
All Greptile review threads are now resolved. Head is |
|
Status: converged — all review threads resolved and the PR is mergeable against its stack base. The only blocker is its parent, #115, which has gone CONFLICTING against Once #115 is rebased/merged (or handed to me), I'll rebase this branch onto it immediately — it should be mechanical, since the only overlap is |
…, per-request retry Greptile review feedback on ActivityWatch#127: - **Order**: group by bucket, not by full endpoint, and only merge a contiguous run that shares one endpoint. Grouping by endpoint reordered a bucket's heartbeats when their pulsetimes differed (e.g. 10, 20, 10 was sent as 10, 10, 20), which changes the server-side timeline. - **Malformed endpoint**: `pulsetime=1..2` matched the regex but `float()` raised inside coalescing, outside the dispatch error handler, killing the worker. Parsing is now fallible and a malformed pulsetime falls back to sending the request verbatim. - **Partial retry**: dispatch one coalesced request per call and track the index, so a transient error retries only the failed request instead of replaying heartbeats that already reached the server. - **Shutdown**: because one request is dispatched per call, `disconnect()` observes `stop()` between requests instead of blocking on a whole batch. Tests: 21 passed (new: pulsetime-order preservation, malformed pulsetime, partial-retry-does-not-replay). Git-Session-Id: b337
9c0899a to
a960a3a
Compare
…, per-request retry Greptile review feedback on ActivityWatch#127: - **Order**: group by bucket, not by full endpoint, and only merge a contiguous run that shares one endpoint. Grouping by endpoint reordered a bucket's heartbeats when their pulsetimes differed (e.g. 10, 20, 10 was sent as 10, 10, 20), which changes the server-side timeline. - **Malformed endpoint**: `pulsetime=1..2` matched the regex but `float()` raised inside coalescing, outside the dispatch error handler, killing the worker. Parsing is now fallible and a malformed pulsetime falls back to sending the request verbatim. - **Partial retry**: dispatch one coalesced request per call and track the index, so a transient error retries only the failed request instead of replaying heartbeats that already reached the server. - **Shutdown**: because one request is dispatched per call, `disconnect()` observes `stop()` between requests instead of blocking on a whole batch. Tests: 21 passed (new: pulsetime-order preservation, malformed pulsetime, partial-retry-does-not-replay). Git-Session-Id: b337
|
Addressed the remaining summary-body note in 5f1a2af: the 10k-drain test now asserts that the merged events tile the queued range contiguously. The first event starts at the first queued heartbeat, each one starts where the previous ended, and the last ends at the end of the backlog. That catches a gap or misplacement, not just a total-duration mismatch.
|
|
@greptileai review |
Comments Outside DiffThese findings could not be posted inline.
|
|
Fixed lint: black formatting fix (887dcf8). |
|
On the remaining outside-diff P1 (legacy testing heartbeats stranded, Status at |
When aw-server is unreachable the request queue accumulates one heartbeat per commit interval. On reconnect they were dispatched one HTTP request at a time, so a long outage drained slowly and hammered the server just as it came back (issues ActivityWatch#32 and ActivityWatch#7). RequestQueue now pops queued requests in batches and pre-merges consecutive heartbeats for the same bucket client-side with aw_transform.heartbeat_merge before sending. The merged events still go to the heartbeat endpoint, so the server-side result is unchanged - a run of identical heartbeats that the server would have merged into one event now arrives as one request instead of dozens. Requests are held in an in-memory batch until the whole batch has been handled; a transient error retains and retries the batch, and re-sending an already-delivered heartbeat is a no-op on the server. A non-retryable client error drops only that request and keeps dispatching the rest. Test fills the queue with 200 mergeable heartbeats and asserts the drain uses a single request while the merged event still spans the whole range (no data loss). Git-Session-Id: b337
Git-Session-Id: b337
…, per-request retry Greptile review feedback on ActivityWatch#127: - **Order**: group by bucket, not by full endpoint, and only merge a contiguous run that shares one endpoint. Grouping by endpoint reordered a bucket's heartbeats when their pulsetimes differed (e.g. 10, 20, 10 was sent as 10, 10, 20), which changes the server-side timeline. - **Malformed endpoint**: `pulsetime=1..2` matched the regex but `float()` raised inside coalescing, outside the dispatch error handler, killing the worker. Parsing is now fallible and a malformed pulsetime falls back to sending the request verbatim. - **Partial retry**: dispatch one coalesced request per call and track the index, so a transient error retries only the failed request instead of replaying heartbeats that already reached the server. - **Shutdown**: because one request is dispatched per call, `disconnect()` observes `stop()` between requests instead of blocking on a whole batch. Tests: 21 passed (new: pulsetime-order preservation, malformed pulsetime, partial-retry-does-not-replay). Git-Session-Id: b337
…timing bound Review feedback: the 10k test only checked the first request's duration, so it would pass even if later batches lost heartbeats; assert the summed span across all posted events instead. Remove the wall-clock limit, which made a functional test sensitive to runner load. Git-Session-Id: b337
…ge contiguously Git-Session-Id: 3b002f8a-7967-5ade-8e69-6c86e374a155
Git-Session-Id: 7c62adb8-40f8-58a7-82c2-170ab3f2baaa
…vityWatch#125/ActivityWatch#130 - tests: pass tmp_path to _fresh_queue (ActivityWatch#130 made queues isolated per test) - RequestQueue.__init__: keep _queue_write_failing (ActivityWatch#124) after the batch state - wait_for_queue_empty (ActivityWatch#125): track the in-flight batch instead of _current - drop an import left unused by the rebase Git-Session-Id: 68f2e145-fcff-459d-8e16-c3e43269f5fa
887dcf8 to
151f55b
Compare
|
Rebased onto master now that #130 has landed, so the carried #115 commits are gone. Conflicts with #124 and #125 were resolved by keeping both sides. |
|
Checked both summary-body findings against current head
Updated the PR description to remove the obsolete stack/CI instructions and document current retry and crash-recovery semantics. No code change or Greptile re-trigger: there is no reproduced batching/dependency defect to fix in these findings. CI remains queued; this is not a claim that the 3/5 score or CI has cleared. |
1 participant
Addresses #32 and #7.
Rebased onto
masterafter #130 merged. This is no longer stacked on #115/#130. The current diff changes onlyaw_client/client.pyandtests/test_requestqueue.py; profile, config, CLI, query helpers, and the expanded Makefile test target are already on master.Problem
When aw-server is unreachable, the persistent queue accumulates one heartbeat per commit interval. Sending them individually on reconnect creates thousands of HTTP requests.
Fix
Pop up to
BATCH_SIZE=1000queued requests and merge consecutive compatible heartbeats per bucket usingaw_transform.heartbeat_merge. Send merged events to the heartbeat endpoint, preserving each bucket's order, including changes in pulsetime.wait_for_queue_empty()includes the in-flight batch, not just the queue's unread rows.Scope
No bulk-insert endpoint, profile migration, or dependency changes. Those are independent of heartbeat batching. This does not claim to solve duplicate delivery across a process crash.
Verification
The expanded Makefile test selection passes: 100 tests on Python 3.13, and 100 tests in a fresh Python 3.9 environment installed from this package's declared runtime dependencies plus pytest, explicitly using the locked
aw-core==0.5.17.Tests cover 200 mergeable heartbeats becoming one request, 10,000 becoming ten requests with contiguous coverage of the complete time range, per-bucket ordering, malformed endpoints, permanent errors, partial retries, and waiting for an in-flight batch.
GitHub CI is pending; local passing tests are not a claim that CI has completed.