Skip to content

Commit 2bb3a52

Browse files
authored
feat: earnings readings record which machine took them (#255)
* feat: earnings readings record which machine took them The server half of CashPilot-Desktop-xjr, and the piece that gates the Desktop side. The goal is the user's: pairing uploads a Desktop's history retroactively, the Desktop then shows the complete picture, and on unlink it falls back to what that machine earned alone. The obvious implementation corrupts the data, and not visibly. The server does not store EARNINGS -- it stores cumulative BALANCE READINGS and derives earnings as clamped deltas between CONSECUTIVE readings. Two samplers of one provider account, differenced as a single series, produce deltas between readings that came from different samplers: the sequence oscillates, every downward step clamps to zero, and the total is SYSTEMATICALLY UNDERSTATED while looking entirely plausible. Worse than double counting, which at least overstates visibly. So a reading now records its `source` -- 'server' for this server's own collectors, a client_id for a paired machine -- and deltas are taken per (platform, source) and only then summed. Both delta readers were updated; the second one was easy to miss. That also makes (platform, source, date) the idempotency key, replacing (platform, date): re-pairing or a retried import overwrites a day rather than appending a second reading for it. Two machines reporting one platform on one day is now the normal case rather than a constraint violation. THE CURRENT-BALANCE QUERY DELIBERATELY STAYS ONE ROW PER PLATFORM. A provider reports a single balance for the whole account, so the newest reading from ANY source IS the current balance; taking the latest per source and summing would multiply it by the number of machines watching. I BROKE THE UPGRADE PATH TWICE, both times by declaring the new index in _SCHEMA. _SCHEMA is replayed on EVERY startup and `CREATE TABLE IF NOT EXISTS` is a no-op on an existing volume, so the index named `source` before the ALTER added it and init_db died with "no such column: source" -- the app does not start at all. The index is created in the MIGRATION, after the column, and a comment says why. The pre-existing fx-migration test caught it both times. The dedupe helper runs BEFORE the column is added, so its key now adapts to the schema actually on disk; referencing `source` unconditionally crashed init_db on exactly the volume that helper exists to rescue. FOUR NEGATIVE CONTROLS FIRE, and one initially did not: keying deltas by platform alone passed against test data where both sources reported the SAME numbers, because the ORDER BY groups them and the single crossover clamps to zero. The fixture now uses far-apart scales (10->12 and 50->52) so the crossover is POSITIVE and would be counted as earnings that never happened. * fix: the legacy index survived, and nothing could write a source Three CodeRabbit findings on #255, all real, and the first is the one that mattered. THE LEGACY INDEX SURVIVED AN UPGRADE. I created idx_earnings_platform_source_date but never dropped idx_earnings_platform_date, so on every existing volume the OLD unique index kept forbidding two sources for one (platform, date) -- silently defeating the entire change for exactly the installs it was written for. Creating a replacement is not removing a constraint. My own upgrade test could not see it: the legacy table it builds had NO indexes, so there was nothing to survive. It now builds the index too, and a second test writes two sources for one day end to end. A control that skips the DROP fails both. NOTHING COULD WRITE A SOURCE. upsert_earnings had no `source` parameter, so SQLite applied the default to every call and a paired client could never create its own series through the storage API. The schema accepted a column that the write path could not populate -- a feature that exists only in the table definition. It takes `source: str = "server"` now, with tests that two sources upsert independently and that one source still dedupes. A TEST THAT DID NOT TEST WHAT ITS NAME CLAIMED. "a legacy row with no source" set source='server' explicitly, so it exercised no fallback and merely repeated the single-source case. The column is omitted now, which is the shape a migrated row actually has.
1 parent 96f789e commit 2bb3a52

2 files changed

Lines changed: 637 additions & 24 deletions

File tree

app/database.py

Lines changed: 115 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -319,8 +319,23 @@ def decrypt_value(value: str) -> str:
319319
-- any accuracy — which is what a net-profit or tax export needs. NULL only when
320320
-- the rate was genuinely unavailable, never a guess.
321321
fx_rate_usd REAL,
322+
-- WHO took this reading. 'server' is this CashPilot's own collectors; a
323+
-- paired client pushing its history uses its own worker client_id.
324+
--
325+
-- Load-bearing, not bookkeeping. A balance is a RUNNING TOTAL and earnings
326+
-- are the delta between CONSECUTIVE readings. Two samplers of one provider
327+
-- account interleaved into a single series oscillate -- server 10, desktop
328+
-- 9, server 11, desktop 10 -- and because a drop clamps to zero, the total
329+
-- comes out SYSTEMATICALLY UNDERSTATED while looking entirely plausible.
330+
-- Deltas are therefore taken per (platform, source) and only then summed.
331+
source TEXT NOT NULL DEFAULT 'server',
322332
created_at TEXT NOT NULL DEFAULT (datetime('now'))
323333
);
334+
-- NOTE: the (platform, source, date) unique index is created in the MIGRATION,
335+
-- not here. On an upgraded volume `CREATE TABLE IF NOT EXISTS earnings` is a
336+
-- no-op, so an index declared here would reference `source` before the ALTER
337+
-- adds it and every upgrade would fail on "no such column: source". The
338+
-- existing fx-migration test models exactly that volume and caught it.
324339
325340
-- updated_at exists so a credential's AGE is knowable. Several collectors use
326341
-- values copied out of a browser and some expire in hours; without a timestamp
@@ -451,8 +466,18 @@ def decrypt_value(value: str) -> str:
451466
revoked_before REAL NOT NULL
452467
);
453468
454-
CREATE UNIQUE INDEX IF NOT EXISTS idx_earnings_platform_date
455-
ON earnings (platform, date);
469+
-- The (platform, SOURCE, date) unique index is created in the MIGRATION, not
470+
-- here, and this must stay that way. _SCHEMA is replayed on EVERY startup, and
471+
-- on an upgraded volume `CREATE TABLE IF NOT EXISTS earnings` is a no-op -- so
472+
-- an index declared here names `source` before the ALTER has added it and
473+
-- init_db dies with "no such column: source", taking the whole app down on
474+
-- upgrade. I made that mistake twice; the fx-migration test catches it both
475+
-- times.
476+
--
477+
-- Source is part of the key because two machines may legitimately report the
478+
-- same platform on the same day -- the normal case once a client pushes its
479+
-- history -- while one machine reporting a platform twice for one day is still
480+
-- a duplicate to be upserted away.
456481
457482
CREATE INDEX IF NOT EXISTS idx_earnings_created
458483
ON earnings (created_at);
@@ -606,7 +631,7 @@ async def _dedupe_earnings_before_indexing(db: Any) -> None:
606631
would have left behind had the index been there all along.
607632
"""
608633
cursor = await db.execute(
609-
"SELECT 1 FROM sqlite_master WHERE type = 'index' AND name = 'idx_earnings_platform_date'"
634+
"SELECT 1 FROM sqlite_master WHERE type = 'index' AND name = 'idx_earnings_platform_source_date'"
610635
)
611636
if await cursor.fetchone():
612637
return # Index already present, so duplicates cannot exist.
@@ -615,7 +640,15 @@ async def _dedupe_earnings_before_indexing(db: Any) -> None:
615640
if not await cursor.fetchone():
616641
return # Fresh install; the table is about to be created cleanly.
617642

618-
cursor = await db.execute("SELECT COUNT(*) - COUNT(DISTINCT platform || '|' || date) AS extra FROM earnings")
643+
# This runs BEFORE the `source` column is added, so the key must match
644+
# whichever schema is actually on disk. Referencing `source` unconditionally
645+
# crashes init_db on exactly the volume this helper exists to rescue.
646+
cursor = await db.execute("PRAGMA table_info(earnings)")
647+
has_source = any(row["name"] == "source" for row in await cursor.fetchall())
648+
key = "platform || '|' || COALESCE(source, 'server') || '|' || date" if has_source else "platform || '|' || date"
649+
group_by = "platform, source, date" if has_source else "platform, date"
650+
651+
cursor = await db.execute(f"SELECT COUNT(*) - COUNT(DISTINCT {key}) AS extra FROM earnings")
619652
row = await cursor.fetchone()
620653
extra = int(row["extra"] or 0)
621654
if extra <= 0:
@@ -627,7 +660,7 @@ async def _dedupe_earnings_before_indexing(db: Any) -> None:
627660
"and the application can start.",
628661
extra,
629662
)
630-
await db.execute("DELETE FROM earnings WHERE id NOT IN (SELECT MAX(id) FROM earnings GROUP BY platform, date)")
663+
await db.execute(f"DELETE FROM earnings WHERE id NOT IN (SELECT MAX(id) FROM earnings GROUP BY {group_by})")
631664
await db.commit()
632665

633666

@@ -745,6 +778,7 @@ async def init_db() -> None:
745778
# When the per-worker key was minted, so the window in which the
746779
# SHARED key still works for that worker can be bounded.
747780
await db.execute("ALTER TABLE workers ADD COLUMN key_issued_at TEXT")
781+
748782
# Backfilled to NOW, not to NULL and not to the distant past.
749783
# Every already-enrolled-but-unconfirmed worker would otherwise be
750784
# instantly past its window the moment this upgrade lands, and a
@@ -762,6 +796,30 @@ async def init_db() -> None:
762796
earnings_cols = {row["name"] for row in await cursor.fetchall()}
763797
if "fx_rate_usd" not in earnings_cols:
764798
await db.execute("ALTER TABLE earnings ADD COLUMN fx_rate_usd REAL")
799+
# Add `source`: WHO took the reading. Existing rows were all taken by
800+
# this server's own collectors, so 'server' is the truthful backfill
801+
# rather than a placeholder -- there was no other sampler before this.
802+
if "source" not in earnings_cols:
803+
await db.execute("ALTER TABLE earnings ADD COLUMN source TEXT NOT NULL DEFAULT 'server'")
804+
# Unconditional, and only AFTER the column is guaranteed to exist. This
805+
# is the ONLY place the index is created, so a fresh install and an
806+
# upgraded volume take the same path -- declaring it in _SCHEMA instead
807+
# broke every upgrade, because `CREATE TABLE IF NOT EXISTS` is a no-op
808+
# there and the index then named a column the ALTER had not yet added.
809+
#
810+
# Makes (platform, source, date) the idempotency key, so re-pairing or a
811+
# retried import overwrites a day rather than appending a second reading
812+
# for it -- which would difference against itself and read as zero.
813+
# The LEGACY index must go FIRST. On an upgraded volume
814+
# idx_earnings_platform_date survives, and it still forbids two sources
815+
# for one (platform, date) -- so the new index would be created and the
816+
# old one would quietly keep rejecting exactly the writes this change
817+
# exists to allow. Creating the replacement is not enough; the old
818+
# constraint has to be removed. (CodeRabbit, PR #255.)
819+
await db.execute("DROP INDEX IF EXISTS idx_earnings_platform_date")
820+
await db.execute(
821+
"CREATE UNIQUE INDEX IF NOT EXISTS idx_earnings_platform_source_date ON earnings (platform, source, date)"
822+
)
765823

766824
# Migrate deployments table: add spec_encrypted so an existing install starts
767825
# remembering what it deployed. Rows written before this stay empty and fall
@@ -814,12 +872,20 @@ async def upsert_earnings(
814872
currency: str = "USD",
815873
date: str | None = None,
816874
fx_rate_usd: float | None = None,
875+
source: str = "server",
817876
) -> None:
818-
"""Insert or update an earnings record for a platform + date.
877+
"""Insert or update an earnings record for a platform + source + date.
819878
820879
``fx_rate_usd`` is the currency -> USD rate at collection time. It is stored
821880
alongside the balance because exchange rates are only cached live: without it,
822881
the USD value of a historical non-USD reading cannot be reconstructed later.
882+
883+
``source`` is WHO took the reading: ``"server"`` for this server's own
884+
collectors, or a paired client's worker id. It is part of the key, so two
885+
machines may report the same platform on the same day -- the normal case once
886+
a client pushes its history -- while one machine reporting a platform twice
887+
for a day still upserts. Without this parameter the schema would accept a
888+
source but nothing could ever write one, so a client's series could not exist.
823889
"""
824890
date = date or datetime.now(UTC).strftime("%Y-%m-%d")
825891
db = await _get_db()
@@ -829,9 +895,9 @@ async def upsert_earnings(
829895
# WHERE guard preserves created_at when the balance is unchanged.
830896
await db.execute(
831897
"""
832-
INSERT INTO earnings (platform, balance, currency, date, fx_rate_usd)
833-
VALUES (?, ?, ?, ?, ?)
834-
ON CONFLICT(platform, date) DO UPDATE SET
898+
INSERT INTO earnings (platform, balance, currency, date, fx_rate_usd, source)
899+
VALUES (?, ?, ?, ?, ?, ?)
900+
ON CONFLICT(platform, source, date) DO UPDATE SET
835901
balance = excluded.balance,
836902
currency = excluded.currency,
837903
-- COALESCE, not a plain assignment: if the rate lookup failed this
@@ -848,7 +914,7 @@ async def upsert_earnings(
848914
WHERE earnings.balance != excluded.balance
849915
OR earnings.fx_rate_usd IS NULL
850916
""",
851-
(platform, balance, currency, date, fx_rate_usd),
917+
(platform, balance, currency, date, fx_rate_usd, source),
852918
)
853919
await db.commit()
854920
finally:
@@ -868,8 +934,16 @@ async def get_earnings_summary() -> list[dict[str, Any]]:
868934
-- at is sitting right here in the row.
869935
SELECT platform, balance, currency, date, fx_rate_usd
870936
FROM earnings
871-
WHERE (platform, date) IN (
872-
SELECT platform, MAX(date) FROM earnings GROUP BY platform
937+
-- Deliberately ONE row per platform, not one per source. A
938+
-- provider reports a single balance for the whole account, so the
939+
-- newest reading from ANY source IS the current balance; taking the
940+
-- latest per source and summing them would multiply it by the
941+
-- number of machines watching.
942+
WHERE id IN (
943+
SELECT id FROM earnings e
944+
WHERE e.date = (SELECT MAX(date) FROM earnings WHERE platform = e.platform)
945+
GROUP BY e.platform
946+
HAVING id = MAX(e.id)
873947
)
874948
ORDER BY platform
875949
"""
@@ -958,31 +1032,39 @@ async def _usd_earned_per_date(db: Any) -> tuple[dict[str, float], int]:
9581032
"""
9591033
cursor = await db.execute(
9601034
"""
961-
SELECT platform, date, balance, currency, fx_rate_usd
1035+
SELECT platform, date, balance, currency, fx_rate_usd, source
9621036
FROM earnings
963-
ORDER BY platform, date
1037+
ORDER BY platform, source, date
9641038
"""
9651039
)
9661040
per_date: dict[str, float] = {}
967-
previous: dict[str, tuple[str, float]] = {}
1041+
# Keyed by (platform, SOURCE) for the same reason as get_earned_by_platform:
1042+
# two machines sampling one provider account interleave into a series whose
1043+
# deltas are meaningless, and every drop clamps to zero, so the total comes
1044+
# out understated while looking plausible.
1045+
previous: dict[tuple[str, str], tuple[str, float]] = {}
9681046
unpriced = 0
9691047
for row in await cursor.fetchall():
9701048
platform = row["platform"]
1049+
# Absent source means a row written before the column existed; those
1050+
# were all this server's own, so they join the 'server' series rather
1051+
# than forming a phantom one.
1052+
series = (platform, (row["source"] or "server"))
9711053
currency = (row["currency"] or "USD").upper()
9721054
rate = _usd_rate(currency, row["fx_rate_usd"])
9731055
if rate is None:
974-
previous.pop(platform, None)
1056+
previous.pop(series, None)
9751057
unpriced += 1
9761058
continue
9771059
balance = float(row["balance"] or 0.0)
978-
before = previous.get(platform)
1060+
before = previous.get(series)
9791061
if before is not None and before[0] == currency:
9801062
# Clamped per platform BEFORE summing: a payout drops one
9811063
# platform's balance, and an unclamped drop would cancel real
9821064
# earnings on another platform in the same day's total.
9831065
gained = max(0.0, balance - before[1]) * rate
9841066
per_date[row["date"]] = per_date.get(row["date"], 0.0) + gained
985-
previous[platform] = (currency, balance)
1067+
previous[series] = (currency, balance)
9861068
return per_date, unpriced
9871069

9881070

@@ -1840,20 +1922,27 @@ async def get_earned_by_platform(days: int = 30) -> dict[str, float]:
18401922
try:
18411923
cursor = await db.execute(
18421924
"""
1843-
SELECT platform, date, balance, currency, fx_rate_usd
1925+
SELECT platform, date, balance, currency, fx_rate_usd, source
18441926
FROM earnings
18451927
WHERE date >= date('now', ?)
1846-
ORDER BY platform, date
1928+
ORDER BY platform, source, date
18471929
""",
18481930
(f"-{max(1, int(days))} days",),
18491931
)
18501932
rows = await cursor.fetchall()
18511933

18521934
earned: dict[str, float] = {}
1853-
previous: dict[str, tuple[str, float]] = {}
1935+
# Keyed by (platform, SOURCE). Keyed by platform alone, two samplers of
1936+
# one provider account interleave into a single series whose deltas are
1937+
# meaningless: each drop clamps to zero, so the total is understated.
1938+
previous: dict[tuple[str, str], tuple[str, float]] = {}
18541939
unpriced = 0
18551940
for row in rows:
18561941
platform = row["platform"]
1942+
# Absent source means a row written before the column existed, and
1943+
# those were all this server's own. Never invent a distinct source
1944+
# for them: that would split one real series in two.
1945+
series = (platform, (row["source"] or "server"))
18571946
currency = (row["currency"] or "USD").upper()
18581947
balance = float(row["balance"] or 0.0)
18591948
# USD is parity by definition. Trusting a stored rate on a USD row
@@ -1866,14 +1955,16 @@ async def get_earned_by_platform(days: int = 30) -> dict[str, float]:
18661955
# contribute earnings nor anchor the next delta. Dropping the
18671956
# baseline is what stops a later reading from being differenced
18681957
# across the gap and counting the unpriced period twice.
1869-
previous.pop(platform, None)
1958+
previous.pop(series, None)
18701959
unpriced += 1
18711960
continue
18721961

1873-
before = previous.get(platform)
1962+
before = previous.get(series)
18741963
if before is not None and before[0] == currency:
1964+
# Summed into the PLATFORM, so each source contributes its own
1965+
# earnings and the combination happens after differencing.
18751966
earned[platform] += max(0.0, balance - before[1]) * float(rate)
1876-
previous[platform] = (currency, balance)
1967+
previous[series] = (currency, balance)
18771968

18781969
if unpriced:
18791970
_logger.warning(

0 commit comments

Comments
 (0)