feat(fleet): accept a paired client's own earnings history - #256
Conversation
A client that has been reading a provider account on its own -- CashPilot Desktop before it was paired -- had no way to hand that history to the server, so the fleet view began on the day of pairing and every earlier day was simply lost from the total. POST /api/workers/earnings-import takes those readings and stores them under the importing client's own source, which the source-aware schema added in the previous change makes possible. Separate series matter here: earnings are clamped deltas between consecutive readings of the same balance, so interleaving two samplers of one account makes every apparent drop clamp to zero and understates the total. Each series is differenced on its own and the results are summed. Two properties carry the security of it: * The source comes from the AUTHENTICATED worker, never the request body, so no client can write into another's history or into the server's own. * Only a fully enrolled worker may import. A caller still presenting the shared enrollment key gets 403 with instructions to heartbeat first -- every worker holds that key, and this writes durable money data. Re-sending a day updates it rather than appending, so a retried or repeated import is safe by construction. Refs: CashPilot-Desktop-xjr
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded a worker-authenticated earnings import endpoint with validated dates, finite numeric fields, bounded batches, source isolation, catalog filtering, and idempotent upserts. Added safe validation-error serialization, transactional bulk storage, tests, documentation, and Docker image tag updates to version 1.15. ChangesWorker earnings import
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Two ways the new endpoint could be handed input it would store without complaint. The date was free text. Both delta readers ORDER BY it, so a reading dated 2026-1-2 or 01/02/2026 sorts into the wrong place in its own series and the readings either side then difference against the wrong neighbour. It fails silently, only for the client that sent it, and only in the earned figure -- never in the balance the dashboard shows. It is now required to be YYYY-MM-DD and a real calendar day, so 2026-02-30 is refused rather than stored. The readings list was unbounded. One authenticated client could hand the server an arbitrarily large body to parse and then write row by row; a single compromised worker is enough. Capped at 2000, comfortably above an honest import (the server keeps 400 days and a client chunks at 1000). Validation runs before authentication, so a malformed body cannot be used to probe which client ids exist. Refs: CashPilot-Desktop-xjr
v1.15.0 released and the example compose files still pinned 1.14, so anyone following the quickstart deployed a series behind. The pin test caught it -- it exists because a stale pin is what gave issue #188 a version with a first-run bug that had been fixed for months. Unrelated to this branch's change, but it fails CI on every branch until it is fixed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
==========================================
+ Coverage 95.45% 95.50% +0.05%
==========================================
Files 47 47
Lines 6202 6274 +72
==========================================
+ Hits 5920 5992 +72
Misses 282 282
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Found by re-reading the endpoint rather than from a report.
JSON has no NaN or Infinity. Python's parser accepts them anyway, so
{"balance": NaN} was stored verbatim. One such reading poisons every
delta taken from that series -- NaN - x is NaN, and every comparison
against it is False, so the clamp silently misbehaves -- the account
total becomes NaN, and serialising that back out emits a bare NaN that
JSON.parse rejects. A single bad reading from one client breaks the
dashboard for everyone. Both float fields now refuse non-finite values.
That exposed a second problem underneath: FastAPI's 422 body echoes the
offending input, so the REJECTION could not be serialised either and the
client got a 500 for what is squarely a bad request. A
RequestValidationError handler now renders non-finite floats as their
names -- keeping the message diagnostic rather than dropping the field --
and fixes the whole class instead of the one endpoint that takes a float
today.
My first version of that handler broke every OTHER validation error: a
custom validator's error carries the raised ValueError OBJECT in ctx,
which is not serialisable, and skipping jsonable_encoder turned each one
into a 500. Caught by the date tests, and now pinned by its own
regression test.
Also: the skipped list is deduplicated. A client pushing 400 days of a
platform this server does not know got the same name back 400 times --
a response that grows with the request, echoing client-supplied strings,
and saying nothing the set does not.
A negative control showed the sanitiser's bool guard was dead (bool
subclasses int, not float), so it is gone along with the comment that
justified it incorrectly.
Refs: CashPilot-Desktop-xjr
|
Self-review of this diff found two real defects, both now fixed in A Rejecting one was then a 500. FastAPI's 422 body echoes the offending input, so the rejection itself could not be serialised. A My first version of that handler broke every other validation error (a custom validator's error carries the raised Also deduplicated One negative control passed, which meant the code was wrong to have the guard rather than the test being wrong: the sanitiser's 3607 tests pass, coverage 95.53%. Five new negative controls, each failing the test that claims to catch it. |
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_beads_batch_63.py (2)
156-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the
(blank)placeholder in the response.The endpoint reports a blank slug as
"(blank)"inskipped. This test asserts onlyimported == 0. A change that dropped blank slugs silently would still pass.♻️ Proposed addition
assert resp.json()["imported"] == 0 + assert resp.json()["skipped"] == ["(blank)"] upsert.assert_not_awaited()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_beads_batch_63.py` around lines 156 - 163, Update test_a_blank_slug_is_skipped to also assert that the response JSON contains "(blank)" in its skipped results, while preserving the existing imported count and upsert.assert_not_awaited checks.
462-473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead the import cap from
EarningsImportinstead of hardcoding 2000.
assert chunk <= 2000tests two literals, so it passes even ifEarningsImport.readings.max_lengthchanges. Store the limit in alimitvariable fromEarningsImport.model_fields["readings"]metadata and assertchunk <= limitso the test fails when the model cap changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_beads_batch_63.py` around lines 462 - 473, The test_a_real_sized_import_still_fits assertion hardcodes the import cap instead of checking the EarningsImport model configuration. Read the limit from EarningsImport.model_fields["readings"] metadata into a limit variable, then assert chunk <= limit while preserving the existing catalog-size validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/main.py`:
- Around line 3861-3876: Add an upsert_earnings_bulk helper in the database
layer that accepts the filtered readings, opens one connection, reuses the
existing upsert SQL via executemany, commits once, and preserves connection
cleanup/error behavior from upsert_earnings. In the request handler’s
reading-processing flow, retain slug validation and skipped counting, collect
valid rows, then call upsert_earnings_bulk once and update written using the
batch result instead of invoking upsert_earnings per reading.
In `@docs/fleet.md`:
- Around line 44-52: Update the request documentation around the fleet payload
example to show the required Authorization header using the per-worker key, and
document that a blank or missing client_id produces a 400 response.
In `@tests/test_beads_batch_63.py`:
- Line 59: Replace the tuple-expression assertions at
tests/test_beads_batch_63.py lines 59-59 and 460-460 with explicit assertions on
upsert.await_count == 0, preserving the messages “a shared-key holder wrote
earnings” and “the server wrote rows from a body it should have refused”
respectively, so the failure messages are effective and Ruff B018 is satisfied.
---
Nitpick comments:
In `@tests/test_beads_batch_63.py`:
- Around line 156-163: Update test_a_blank_slug_is_skipped to also assert that
the response JSON contains "(blank)" in its skipped results, while preserving
the existing imported count and upsert.assert_not_awaited checks.
- Around line 462-473: The test_a_real_sized_import_still_fits assertion
hardcodes the import cap instead of checking the EarningsImport model
configuration. Read the limit from EarningsImport.model_fields["readings"]
metadata into a limit variable, then assert chunk <= limit while preserving the
existing catalog-size validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c358c88c-317c-411f-9be0-f7f9361beabd
📒 Files selected for processing (5)
app/main.pydocker-compose.fleet.ymldocker-compose.ymldocs/fleet.mdtests/test_beads_batch_63.py
|
@coderabbitai review |
|
A thousand-reading import committed a thousand times. Every commit is an fsync that takes SQLite's write lock, so one import serialised a thousand disk syncs against this server's own collector and request latency tracked sync cost rather than row count. upsert_earnings_many does the same upsert with executemany and one commit. Only half the reported cause was real, and the distinction matters for anyone reading this later: _get_db hands out a borrowed handle on a SHARED per-loop connection whose close() is a documented no-op, so the loop was never opening and closing a thousand connections. It was committing a thousand times. Batching then introduced a bug of its own, which the tests caught: the connection is shared and outlives the request, so a failed batch left an abandoned transaction holding the write lock and the NEXT write blocked for twelve seconds before timing out. It now rolls back. Two more from the same review: * docs/fleet.md gains the Authorization header and every status the endpoint can answer with. A reader integrating against that page alone could not previously construct a valid request. * Two assertions were tuple expressions -- `mock.assert_not_awaited(), "why"` builds a tuple and discards the message, so a red build showed the mock's generic text instead of the reason. Fixed here and in test_optional_runtime.py. The review said ruff's B018 catches that pattern when bugbear is enabled. Bugbear IS enabled here and B018 is not ignored, and ruff 0.15.14 passes it clean -- checked against a minimal probe rather than assumed. So nothing in CI would have caught a recurrence, and there is now an AST-based test that does. Structural, not a string search, because a string search would match the pattern inside its own docstring. One negative control PASSED, which meant the test was wrong: the transactionality test raised while BUILDING the rows, before any SQL ran, so it proved only that the row build validates first and it passed against a writer that committed after every row. It now fails inside the statement, and both it and the wedged-connection test fail under their controls. Reported by CodeRabbit on PR #256.
|
All three addressed in Batching — implemented as Batching then introduced a bug of its own, which the new tests caught: the shared connection outlives the request, so a failed batch left an abandoned transaction holding the write lock and the next write blocked for twelve seconds before timing out. It now rolls back. Docs — Tuple assertions — fixed at both sites, plus a third the diff didn't touch ( One correction there too, and it changes what the right fix is. The finding says Ruff's m = Mock()
m.assert_not_called(), "this message is dead"→ So nothing in CI would have caught a recurrence. There is now an AST-based test that does — structural rather than a string search, because a string search would match the pattern inside its own docstring. One negative control passed, which meant my test was wrong, not the code. The transactionality test raised while building the rows, before any SQL ran — so it proved only that the row build validates first, and it passed against a writer that committed after every row. It now fails inside the statement. Both it and the wedged-connection test fail under their controls. 3619 tests pass, coverage 95.55%. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What
POST /api/workers/earnings-importlets a client that has been reading a provider account by itself hand that history to the server. CashPilot Desktop is the case that prompted it: run standalone for months, pair, and the fleet view started on the day of pairing — every earlier day was gone from the total.This is the second half of
CashPilot-Desktop-xjr. The first half (#255) added thesourcecolumn and made both delta readers key on(platform, source); this adds the write path that uses it.Why separate series, rather than merging into one history
Earnings are stored as cumulative balance readings, and an earned figure is the clamped delta between consecutive readings. Interleaving two samplers of the same provider account is therefore not harmless: sampler A reads 12.0, sampler B reads 11.8 an hour later, and the negative step clamps to zero. Every crossing loses a real gain and the total comes out systematically understated.
Storing each client's readings under its own
sourcemeans each series is differenced on its own, and the results are summed. Nothing merges, nothing is double-counted, and unlinking a client leaves its own series intact — which is what makes "show only what this machine earned alone" possible later.Security
Two properties, both tested with negative controls:
cid), never from the request body. The model carries nosourcefield at all; a test sends one anyway, so a future model change that started honouring it fails here rather than silently letting a client overwrite the server's own series.state == "ok").enrollandreissueboth mean the caller presented the shared key, which every worker holds — accepting it would let anyone with that token write a history for anyclient_idthey named. The 403 says how to proceed rather than just refusing.Unknown slugs are skipped and reported rather than stored: storing one would create a platform the catalog cannot name, render, or ever collect for again, and a silent drop looks identical to a successful import.
Idempotent by construction — the
(platform, source, date)unique index means a re-pair or a retried import updates a day rather than adding a second reading for it, which would difference against itself and read as zero.Verification
source="server", allowing shared-key holders, storing unknown slugs, dropping the currency normalisation, coercing an absent FX rate to0.0, and accepting a blankclient_ideach fail the tests that claim to catch them.ruff check+ruff format --checkclean;node --check,currency_check.mjs,fleet_staleness_check.mjsall pass.Docs:
docs/fleet.mdgains the endpoint, the payload shape, and the three deliberate constraints.Summary by CodeRabbit
New Features
Bug Fixes
Documentation