fix(sales-summaries): stop days falling out of balance #18
Reference in New Issue
Block a user
Delete Branch "sales-summary-balancing-v2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Three faults were leaving restaurant days out of balance — one in the data, two in the arithmetic. A fourth was found late and turns out not to be a balancing fault at all; it is deliberately left visible, and it is the most important thing in this PR.
Measured over ninety days on a restored copy of production (210 clients, 18,900 client-days), running the real nightly job twice: once with the fixes off, once with them on.
979 days repaired, none knocked out of balance, and not one already-balanced day altered — verified line by line (category, debit/credit side, amount to the cent, account), not just on each day's bottom line.
Of the 279 days left, 171 are not arithmetic faults — see The fourth problem below.
📊 Full report: https://claude.ai/code/artifact/58910b7e-45e0-40c9-956d-a6932d548dea
📋 Rollout plan:
docs/2026-08-15-sales-summary-rollout-plan.md— steps to execute, in orderNothing changes on deploy
The one behaviour change that alters a client's books sits behind a per-client feature flag that is off by default:
summary-service-charges.Merging and deploying changes no client's books until a client is opted in. Removing the flag is the rollback and restores today's behaviour exactly.
The data fault
Ten Square locations were configured against two client records each. Sales orders scoped their identifier by client; refunds, card payments, payouts and cash-drawer shifts used the bare Square id. Those attributes are
:db.unique/identity, so both clients' imports resolved to a single entity and the last writer won — 3,387 refunds, 4,069 payouts and 2,628 cash-drawer shifts changed hands over time, across 19 client pairs of which only 10 are visible in today's configuration.Worse, one payment could belong to two orders.
:sales-order/chargesis:db/isComponent, so removing a voided order cascaded into payments the other client still needed. Seedocs/2026-08-15-remove-voided-orders-risk.md.Three fixes: client-scope the four key schemes; look the record up under both schemes so the change can deploy before the migration finishes; and a migration giving every order its own payment.
Run over the whole database: 19,040,785 orders walked, 9,100,314 payments re-keyed, 200,027 copied, ending with 17,047,142 payments scoped, none left to rename, none unscopable, and no payment owned by more than one order. Idempotent and resumable; about thirteen minutes.
The arithmetic faults
Refunded tips stayed on the books.
get-tipsummed tips by joining through:sales-order/charges, so a return-only order — which has no tender to join through — contributed nothing while its reversal sat unread on:sales-order/tip. The fix is additive rather than substitutive on purpose: where an order does have a tender, the tender is the correct source.Service charges were collected but never earned. Nothing read
:sales-order/service-chargeat all. Now credited for Square orders only, both signs.The fourth problem — refunds with no sales behind them
Read this one. After the three fixes, 156 of the 279 remaining days are a record carrying refunds on a day it recorded no sales at all.
The obvious reading is that the refund settled on a closed day. It is wrong. Checking each day against the date its client first recorded any order: 132 of 156 fall before that client had a single order in the system, and for seven of the nine records affected, every one does. These are periods whose sales were never imported.
The refunds are not theirs either. Ownership history settles it — a $35.35 refund dated 26 February belonged to
NGDGthat same day and was taken over byNGDUon 12 August, with others flipping between the two several times a day across 12–15 August.NGDU's first order is 2 August; it holds 94 refunds dated before it existed as a trading record. Unscoped refund keys meant whichever import ran last took ownership — the data fault above, seen from the other end.Across nine records, 659 refunds worth $15,225.24 sit on a record dated before its own first order.
NG4SandNGPSare not among the ten shared locations, so this is wider than the duplicate records.A fix for this was written, measured, and removed. Booking a return equal to the day's refunds whenever the client recorded no sales closes 156 days and $4,820.19, is incapable of touching a trading day, and breaks nothing. It came out anyway: an unbalanced day is the only visible signal that a restaurant's sales are not being imported, and closing it would remove the alarm and leave the fire. What remains is a comment explaining why and a test asserting the day stays out of balance, so the next person to notice the arithmetic finds the reasoning first.
Step 9 of the rollout plan is the real fix and needs a business decision: either import the missing sales, or move the refunds to the record that has them.
Supporting changes
schema.ednis built from an attribute incloud-migration-schema.edn, so every test fixture died in setup — very likely why sales summaries had no tests before thiscompare-sales-summariesmoved totest/cljasauto-ap.tools.*One correction worth reading
d/as-ofcannot be used to compare summary amounts.:ledger-mapped/amount,ledger-sideandaccountare all:db/noHistory, so superseded values are discarded — a recomputed summary reads back throughas-ofwith its categories intact and its amounts absent, which is indistinguishable from a legitimate all-zero balanced day. Every figure in the report comes from live captures taken straight after each pass. The tool's docstring now says so where someone will actually hit it.Needs a decision before rollout
What this does not fix
279 client-days over ninety days, $7,790.54 — but only 108 of them are arithmetic:
Testing
lein test auto-ap.jobs.sales-summaries-test auto-ap.square.core3-test auto-ap.jobs.rekey-square-external-ids-test— 26 tests, 62 assertions, 0 failures.Beyond unit tests, the whole analysis was rebuilt from a fresh restore of the production backup and reproduced the previous run to the cent at the point the two are comparable — 279 days and $7,790.54, with not one client-day differing by half a penny.
🤖 Generated with Claude Code
sales-summaries-v2 recomputes every dirty summary, but nothing set the dirty flag on a schedule: mark-all-dirty was only ever called by hand from the comment block, and the job was registered in neither server.clj's INTEGREAT_JOB dispatch nor terraform/deploy.tf, so -main was dead code that could never run in production. Summaries were only recalculated when someone remembered to do it in the REPL, and POS data keeps arriving after a business day closes, so a summary computed once on the day was routinely wrong and stayed wrong. Add a daily job that marks the trailing 7 days dirty and recomputes them, leaving finished work alone. "Finished" is the condition the app already calls Balanced -- debits equal credits and every line is mapped to an account. Since that is derived rather than stored, a summary that later falls out of balance is picked up again on the next run. Extract the Balanced predicate into auto-ap.datomic.sales-summaries so the grid's pill and the job share one definition, rather than a background job requiring an SSR namespace. total-debits/total-credits resolve the ledger side from either a plain keyword or the {:db/ident ...} map a pull returns, and accepted? requires every item to declare a side: un-normalized pulled items otherwise sum to 0.0 on both sides, read as balanced, and get skipped silently and permanently. Also fix sales-summaries-v2 destroying user-entered line items. It filtered for :sales-summary-item/manual? to preserve them, but dirty-sales-summaries' index-pull selector never fetched :sales-summary/items, so manual-items was always empty. Because items is a component attribute upserted via [:reset-rels ...], every recompute deleted the hand-entered lines -- often the very lines that make a summary balance. Harmless while nothing ran on a schedule; destructive the moment this does. Register the job in the admin Background Jobs dropdown too, with a days field: schedules are prod-only, so the admin page is the only way to run it on staging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Code review — 8 reviewer lenses + empirical verification
Verdict: Not ready. One confirmed P0 that doubles money under the rollout order this PR documents. Everything else is fixable in place; the migration logic itself held up well under attack.
I verified the load-bearing claims by executing them against Datomic (in-memory databases and the restored production copy) rather than by reading. Three things that looked like bugs turned out not to be, and are not reported below:
{:db/id nil}is accepted by Datomic (treated as absent → tempid), thetake-whileboundary indirty-sales-summariesis sound (falsesorts beforetruein the tuple, verified), andupsert-cash-shifts' new 2-arity is behaviour-preserving.P0 — Deploy window doubles tender for the second client on a shared location
src/clj/auto_ap/square/core3.clj:595-596(payout path) and:305-306(tender path)Confirmed end to end against a real database using the actual functions. Sequence:
Xexists under the legacy keysquare/charge/P, referenced by client B's order.existing-idresolvesXvia its legacy-key fallback and renames it tosquare/charge/AAA-LA-P— A renames a charge it does not own, and writes no:charge/client/:charge/location.existing-idnow misses both schemes (scoped-B never existed; the legacy key was consumed in step 2), returns nil, and a second chargesquare/charge/BBB-LB-Pis minted.:sales-order/chargesis:db.cardinality/manyand orders are transacted as plain maps (core3.clj:652), so nothing retracts the stale ref.Result: order B holds two charge entities for one Square payment — $200 of tender for a $100 payment.
get-payment-itemsandtendered-tipboth sum them.It then compounds. Running the migration on that state produces:
That double-scoped shape is exactly the failure the report describes as having already "doubled the tender on five clients by $3,000–$7,000 each".
raw-square-id's owner-prefix guard cannot strip A's scope because step 2 left the charge with no:charge/client.Why the 90-day validation missed it: that measurement ran after Phase 0 deactivated the duplicate locations — i.e. after the two-writer condition was removed. The rollout plan puts deploy (Step 1) before deactivation (Step 3), so it re-creates the very condition the validation had already eliminated. Ten client pairs are live in that window.
Suggested resolutions (either closes it):
:charge/clientis a different client, and have the order import retract stale:sales-order/chargesrefs.Found by: adversarial (P0) + correctness (P1), then reproduced empirically.
P1 — The go/no-go gate samples the oldest 2% of orders
docs/2026-08-15-sales-summary-rollout-plan.md:113all-order-idsstreams:aevt, which is ascending entity id — sotake 400000is the oldest 400k of 19,040,785 orders. Measured on the restored copy, that sample spans 2019-12-31 → 2021-06-03. The shared-location contention and the entire analysis window are in 2026. This gate is systematically blind to the risk it exists to detect, and Step 7 re-enablesremove-voided-orders— a:db/retractEntitycascade — on the strength of it.This also corrects something I reported earlier in this branch's history: "0 shared across 400,000 orders checked" was measured on that wrong slice. I re-ran it on the newest 400,000 orders (2026-01-14 → 2026-08-18, covering the whole contention period): still 0. The conclusion holds; the evidence originally given for it did not.
Fix: drop the
take, or label it explicitly as a progress sample and make the unsampled call the actual gate.Found by: data-migration + adversarial (both P1), confirmed empirically.
P1 — The prescribed pre-flight collision check reads a key
plannever returnsdocs/2026-08-15-sales-summary-rollout-plan.md:91-92The plan tells the operator to "check
:collisionsis empty for every attribute" and hands them(dissoc (rk/plan ...) :new-keys). Butplan's accumulator is{:total :to-migrate :already-scoped :unscopable :new-keys}(rekey_square_external_ids.clj:84) — there is no:collisionskey, and the commanddissocs the only inputcollisionsaccepts. The check silently passes on every database.Fix:
(rk/collisions (:new-keys (rk/plan db attr prefix))).Found by: adversarial (confidence 100).
P2 — Findings with multi-reviewer agreement
collisionsguard the other three attributes get — on the largest, most collision-prone attribute (17M rows)rekey_square_external_ids.clj:303-304vs:298-300rekey_square_external_ids.clj:220-226; plan:172unscoped-report's docstring contradicts itself. It says "once every count reads zero" the legacy fallback can be removed, but:no-ownercan never reach zero for charges (~283k payout stubs lack:charge/clientby design).migrate-all!'s docstring correctly says the signal is:legacy = 0. The two disagree about the same "completeness gate"rekey_square_external_ids.clj:261-262vs:292-293split-and-rekey-charges!— its siblingmigrate!logs every 20 batches, yet the function that walks all 19M orders logs only on completion. A mid-run death leaves no trail of how far it gotrekey_square_external_ids.clj:198vs:106-109existing-idDB call sits outside the try/catch that protects the sibling payouts path. A transient failure aborts the whole client's import chain (and, since refunds run first, silently skips that client's cash-drawer shifts for the cycle)core3.clj:620,:739vs:533-601rekey_square_external_ids.clj:225-226docs/…reconciliation.html:574-577P2 — A documentation claim that is false, and provably harmful
docs/2026-08-15-sales-summary-reconciliation.html:574-577states that for two orders of the same client sharing a payment, "run across nineteen million orders in batches of two thousand, such pairs almost always fall in different batches and the second order now takes a copy," and flags it as unmeasured risk to check before production.I ran the real migration at batch-size 1 (forcing separate batches):
The charge is not cloned. Once batch 1's
:keepwrites:charge/client/:charge/locationalongside the rename, batch 2'sraw-square-idtakes its owner branch,new-keyreconstructsold, and the guard(not= old new-key)atrekey_square_external_ids.clj:174drops the row before:actionis ever read. Batch size is irrelevant for same client + location.This is worth fixing beyond tidiness: the false claim manufactured a P1 in an independent reviewer, which cited the document as its evidence. It also creates a pre-production action item that would cost real time to chase. Two reviewers independently traced the code and reached the same conclusion I measured.
P3 / smaller
:clonedcounts intent, not entities. The counter increments while building the tx (:206,:219), and two same-client clones in one batch collapse onto one string tempid — so the report's "the record count rose by exactly 200,027, which is the check that it created what it meant to" is a weaker check than stated. (correctness):ledger-mapped/account nilon its Service Charges line, soaccepted?never passes andmark-stale-dirtyre-marks that day nightly, forever. Worth a pre-flight check in Step 6. (correctness)split-and-rekey-charges!never updates:expected-deposit/charges, and deposit imports only add refs, never retract — a deposit may keep pointing at a charge reassigned to another client. (data-migration, unconfirmed)Withdrawn after verification
Reported by a reviewer, then disproved — recording these so they don't get re-raised:
cons'd intorefresh-client!. It was not —git show f8ef7918showsrefund-only-returnswired insideget-returnsvia theorat line 449, which the test calls directly. The test does catch the documented reintroduction shape. Downgraded to a hardening suggestion: an end-to-endrefresh-client!test would be strictly stronger.Coverage
aider, which is not a review route). Its findings therefore carry no independent-model corroboration; the P0 above earns its confidence from reproduction, not from agreement.AGENTS.mdcovers only Gitea/PR workflow, so it has no bearing on this diff. The new tests follow the project's testing conventions (:referimports,wrap-setup, DB-state assertions) — no violations.untendered-tip,service-charge-total) follow the file's existing per-item pattern and are narrowindex-rangescans.943bc188.tmp/restore_checks.cljis untracked and excluded.migrate-all!has no end-to-end test.Biggest test gaps
:sales-order/chargesref. (This would have caught the P0.)migrate-all!end to end: pass ordering, the collision gate, idempotence of a second full run.migrate-all!throws as documented.🤖 review generated with Claude Code
Two clients on one Square location double a day's tender in the window between deploying and finishing the migration. Reproduced end to end: 1. charge X carries the legacy key square/charge/P and belongs to client B's order 2. client A's payout import resolves X through existing-id's legacy fallback and renames it into A's scope 3. client B's next order import matches neither scheme, so it mints a second charge 4. :sales-order/charges is cardinality-many and orders transact as plain maps, so nothing retracts the first B's order ends up holding two charges for one payment — $200 of tender for a $100 payment — and running the migration on that state produces square/charge/BBB-LB-AAA-LA-P, the same double-scoped shape that already doubled tender on five clients once during this work. existing-id's legacy branch now declines any record already owned by a different client, reading the owner attribute and, for charges that predate :charge/client, the client of the referencing order. Declining is also correct on its merits: the write then lands on this client's own copy, which is what the scoped keys exist to create. The payout path also writes :charge/client/:charge/location alongside the key, so a charge's scope and its owner can no longer disagree. The guard is transitional and gets deleted with the legacy branch it protects, at rollout step 9. Rollout resequenced for the decision to leave duplicate client records active: no deactivation, no "which record survives" call, and the risk window closed by pausing the importer across deploy + migrate rather than by removing one of the two writers. Both records converge to independent stable histories once every key carries its owner. Also from review: - migrate-all! now collision-checks the charge pass like the other three attributes instead of discovering a clash mid-run over 17M rows - split-and-rekey-charges! logs progress every 200 batches; an interrupted 19M-order run left no trail - unscoped-report's docstring no longer promises a zero its :no-owner column cannot reach; plan is named as the authoritative signal - the rollout's pre-flight asked for a :collisions key plan never returns, so it silently passed on every database - the multi-parent gate sampled (take 400000 (all-order-ids db)), which streams :aevt — ascending entity id — and so read the OLDEST 2% of orders: 2019-12-31 to 2021-06-03, before any of the contention it looks for. Now every order of the last year via the client+date index, 5,159,787 on the restored copy, reading 0 - the report claimed same-client pairs get copied once batches split them. They do not, at any batch size; verified at batch-size 1 and now pinned by a test 30 tests, 72 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Review feedback applied —
10d0d01bDesign decision that supersedes part of the review above: duplicate client records stay active. Client config is left as-is and the re-key is what resolves the pairs — each record ends up with an independent, stable history under its own keys. So "retire the duplicate client records" is gone from the plan entirely, along with the "which record survives" business decision.
That changes what the P0 fix has to be. Deactivation is no longer available as the thing that ends the two-writer condition, so the exposure is narrowed to what it actually is: the interval between deploying and finishing the migration, while legacy keys still exist for a client to resolve.
P0 — fixed and proven
existing-id's legacy branch now declines any record already owned by a different client, reading the owner attribute and — for charges predating:charge/client— the client of the referencing order. The payout path also writes:charge/client/:charge/locationalongside the key, so a charge's scope and its owner can no longer disagree.Re-ran the exact reproduction:
BBB-LB-AAA-LA-P(double-scoped)BBB-LB-P, migration a no-opPinned by
deploy-window-does-not-double-a-second-clients-tender.Declining is also right on the merits — the write then lands on this client's own copy, which is what the scoped keys exist to create. The guard is transitional and gets deleted together with the legacy branch at step 9, which is now a scheduled step rather than "no hurry": with both records permanently live, that branch is the only code path that can ever cross a client boundary.
Rollout resequenced
Imports are paused across deploy + migrate, which closes the window rather than narrowing it — the migration is ~13 minutes, so the maintenance window is short.
remove-voided-ordersremove-voided-ordersAlso fixed
:collisionspre-flight asked for a keyplannever returns, so it passed on every database. Now(rk/collisions (:new-keys (rk/plan ...))), andmigrate-all!collision-checks the charge pass too instead of discovering a clash mid-run over 17M rows.(take 400000 (all-order-ids db))streams:aevt, so it read the oldest 2% — 2019-12-31 → 2021-06-03, entirely before the contention. Replaced with every order of the last year via theclient+dateindex (5,159,787 on the restored copy, reads 0), scoped that way deliberately becauseremove-voided-ordersonly deletes recent orders. Completeness across all 17M isplan's:to-migrate 0, and the doc now says which check does which job.unscoped-report's docstring no longer promises a zero its:no-ownercolumn cannot reach.split-and-rekey-charges!.Deliberately not done
try/catcharound the per-record import fns. Real asymmetry, but wrapping it silently skips records — missing money reads worse than a failed cycle that retries.git show f8ef7918confirms the removed rule was insideget-returns.Unchanged: the 659 refunds sitting on a record with no sales behind them (step 10). Leaving config as-is fixes the future, not the past.
30 tests, 72 assertions, 0 failures.
🤖 Claude Code
The report's figures were taken with one client record of each shared pair deactivated and a live Square import run afterwards. That is no longer how this deploys — both records stay live and the re-key is what separates them — so the numbers described a configuration that will not exist. Re-run from scratch: fresh restore of backup point 209608347 (verified back to 16,545,495 charges and legacy keys before starting), the new month-wise migration, then two full ninety-day recomputes. before 1,451 days out of balance / $81,023.96 after 542 days out of balance / $20,239.00 915 into balance, 6 out, 17,916 summaries untouched Two corrections to claims that no longer hold: - "zero days knocked out of balance" is now six. All six are the same shape — the Tip line falls by a round amount and the day breaks by exactly that — and all six are on shared-location records. Each is a tip reversed on one record whose refund went to its twin: the fix makes a hidden mis-attribution visible rather than causing one. - the migration takes ~38 minutes, not ~13. The month-wise walk adds per-month index overhead, and the earlier figure predated it. What did NOT move is the part that should not. For the 190 clients that do not share a Square location the residue is 119 days and $1,730.61 in both runs, with the same five restaurants accounting for it (NG4S, NGMV, NGEB, NGPS, N-30012). The arithmetic fixes behave identically whatever is done to the duplicates, which is a stronger check than either run alone. $18,508.39 of the $20,239.00 — 91% — sits on the twenty shared-location records. The report now states plainly what retiring the duplicates would be worth (~$12,000 of variance across ~260 client-days per ninety days), while noting the comparison is not perfectly isolated because the earlier run also included a live import. Migration re-measured: 16,236,839 re-keyed, 500,438 cloned, plan reports {:total 17045933 :to-migrate 0 :already-scoped 17045933 :unscopable 0}, and the multi-parent gate reads 0 across all 5,158,470 orders of the last year. Step 10's refunds re-measured too: 660 worth $15,237.02 dated before their own record's first order, 140 of the 171 no-sales days falling before it. 31 tests, 76 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Re-keying stops two client records on one Square location fighting over a record, but it does not make their books equal. Sales orders have always been keyed by client, so each record built its own order history from the start. Refunds, payouts and cash-drawer shifts were not, so only ONE record holds each of them — whichever imported it last. The migration freezes that ownership rather than evening it out. The record left without them shows returns from its own orders and no refunds against them. NGBK held 158,535 orders and five refunds. On 2026-05-11 both NGBK and NGBR held the same 221 orders; NGBK had no refunds, NGBR had two worth $2,232.29, and NGBK was out by $2,232.29 to the cent. Rather than manufacture copies, ask Square again. Client-scoped keys mean each record now creates its own copy of whatever it reads, so replaying a window converges the two histories with no code inventing a duplicate. `backfill-history` does that for a date range across orders, payouts, refunds and shifts. After it, all ten pairs held matching order and refund counts. Fixes a capped read found by doing this: the refunds import asked Square for a location's refunds and read only the first page — no cursor, no date range. Square pages at a hundred, so a location with more refunds silently returned a hundred and the response looked complete. That is why each twin held almost exactly 100 refunds and why an earlier import added exactly 1,000 across ten locations. `refunds`/`upsert-refunds` now follow the cursor and accept a window. Re-measured from the same fresh restore, duplicates left active: before 1,191 days out of balance / $70,276.50 after 122 days out of balance / $2,379.45 (32 material) 1,069 into balance, 0 out, 0 already-balanced days altered The shared records went from 423 days / $18,508.39 to 3 days / $648.84 — NGBK and NGBR at $299.42 each (the known tender-versus-order gap) and NGDA at $50.00 (auto-gratuity as a service charge). The zero-regression guarantee is restored too: the six days that broke without the backfill were tips reversed on one record whose refund sat on the twin, and all six closed once both sides had their own copy. This beats retiring the duplicate records, which left 279 days and $7,790.54, and it needs no decision about whose history to abandon. Unchanged across every run: for the 190 clients that do not share a location, 119 days and $1,730.61, same five restaurants. Cost: 5.9 hours for ninety days across twenty records. Every Square call shares one 25 req/s throttle, refunds and shifts cost one API call each, and backfill-history imports three clients at a time. Reads are not the limit — existing-id measures 32 microseconds. 31 tests, 76 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.