fix(sales-summaries): stop days falling out of balance

Three faults were leaving restaurant days out of balance — one in the
data, two in the arithmetic — plus a fourth that turned out to be a
missing-data problem and is deliberately left visible. Measured over ninety days on a restored
copy of production (210 clients, 18,900 client-days): 1,258 days out of
balance and $69,560.10 becomes 279 days and $7,790.54 — of which 171 are
not arithmetic faults at all, but days whose sales were never imported.

979 days repaired, none knocked out of balance, and not one
already-balanced day altered — verified line by line (category, side,
amount to the cent, account), not just on each day's bottom line.

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/charges is
:db/isComponent, so removing a voided order cascaded into payments the
other client still needed.

Fixes: client-scope the four key schemes; look the record up under both
schemes so the change deploys before the migration finishes; and a
migration that gives every order its own payment. Run over the whole
database that is 19,040,785 orders walked, 9,100,314 payments re-keyed
and 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-tip summed tips by joining
  through :sales-order/charges, so a return-only order — no tender to
  join through — contributed nothing while its reversal sat unread on
  :sales-order/tip. Additive, not substitutive: 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-charge. Now credited for Square orders only, both
  signs, behind summary-service-charges.

The flag is off by default, so deploying this changes nothing until a
client is opted in. docs/2026-08-15-sales-summary-rollout-plan.md has the
steps.

WHAT IS DELIBERATELY NOT FIXED

156 of the 279 remaining days carry refunds on a record that recorded no
sales at all that day, and 132 of those fall before that client's first
ever order. The refunds are not theirs: ownership history shows a $35.35
refund dated 26 February belonging to NGDG that day and taken over by
NGDU on 12 August, flipping between the two several times a day. Across
nine records, 659 refunds worth $15,225.24 sit on a record dated before
its own first order — unscoped keys let whichever import ran last take
ownership.

A rule closing those days was written and measured (156 days, $4,820.19,
nothing broken) and then removed. An unbalanced day is the only visible
signal that a restaurant's sales are not being imported; balancing it
would remove the alarm and leave the fire. A comment and a test hold that
decision in place. Step 9 of the rollout plan is the real fix, and it
needs a business decision.

SUPPORTING

- Install schema attributes before the tuples that compose them. A tuple
  in schema.edn is built from an attribute in cloud-migration-schema.edn,
  so every test fixture died in setup — very likely why sales summaries
  had no tests before this.
- Log each day's imbalance and its suspect lines.
- Bound the dirty-summary scan to one client: 1,321 ms to 5.6 ms.
- compare-sales-summaries lives in test/clj as auto-ap.tools.* — it is a
  verification harness, not part of the running application. Its
  docstring now warns that d/as-of cannot be used to compare summary
  amounts: :ledger-mapped/amount, ledger-side and account are
  :db/noHistory, so a recomputed summary reads back with its amounts
  absent and looks like a legitimate balanced day.

26 tests, 62 assertions.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-15 20:17:58 -07:00
parent 943bc18842
commit 57a84dae11
12 changed files with 2207 additions and 34 deletions

View File

@@ -0,0 +1,115 @@
---
title: remove-voided-orders can delete another client's payments
type: risk
date: 2026-08-15
status: open — decide before merging the re-key
---
# `remove-voided-orders` can delete another client's payments
Measured on the restored backup, 2026-08-15. This risk is **pre-existing** — nothing in the
sales-summary work created it — but it is live right now, and the re-key work touches the same
data, so it should be understood before merging.
## The mechanism, in four steps
**1. Charges are component entities of an order.**
```clojure
;; resources/schema.edn
{:db/ident :sales-order/charges
:db/valueType :db.type/ref
:db/isComponent true ;; <- this is the load-bearing bit
:db/cardinality :db.cardinality/many}
```
`:db/isComponent true` tells Datomic the charges *belong to* the order. It is what lets you
transact an order with its tenders nested inside, and it means the charges have no independent
existence as far as Datomic is concerned.
**2. `retractEntity` on a component parent deletes the children too.**
That is the documented behaviour of `:db/retractEntity`: it recursively retracts component
values. `square.core3/remove-voided-orders` ends with exactly that:
```clojure
(s/map (fn [[o]]
[[:db/retractEntity [:sales-order/external-id (:sales-order/external-id o)]]]))
```
It asks Square for the last 10 days of orders, keeps the ones that should *not* be imported —
voided and cancelled orders — and retracts any of those we already stored. That is correct and
desirable on its own: a voided order should not sit in the books.
**3. But one charge can be shared by two orders.**
When two clients are configured on the same Square location, both import the same Square data.
Order keys embed the client, so each client gets its own order entity. Charge keys did **not**
embed the client, and `:charge/external-id` is `:db.unique/identity`, so both clients' orders
resolved to *the same charge entity*:
```
NGCD order 17592395490523 ──┐
├──> charge 17592490524 ← one entity, two parents
NGCC order 17592395511722 ──┘
```
**4. So retracting one order deletes a charge the other order still points at.**
Datomic sees a component and removes it. The surviving order keeps its line items — its sales —
but its tender is gone. The day then shows revenue with no payment against it, the summary goes
out of balance, and the payment is gone from the current database value. (History retains it, so
it is recoverable by someone who knows to look, but nothing in the app will show it again.)
## How exposed are we
Measured over the 10 contended clients across 2026-07-13 → 08-14:
| | |
|---|---|
| Charges examined | 56,829 |
| **Referenced by more than one order** | **35,870 (63%)** |
So this is not a theoretical corner. Roughly two thirds of the charges in that population have
two parents, and any voided order among them takes a charge down with it.
The exposure window for *new* damage is the rolling 10 days `remove-voided-orders` searches, but
the shared charges themselves span the whole period the locations were double-configured.
## What changes after Phase 0 and the re-key, and what doesn't
- **Phase 0 (done on the restore)** stops new sharing: only one client per location imports now,
so no new order pairs form.
- **The re-key (done for refunds and the contended clients' charges)** makes sharing structurally
impossible going forward, because a charge key now contains the client code.
- **Neither retroactively splits the 35,870 charges that are already shared.** They still have two
parents. Until they are split, `remove-voided-orders` remains capable of deleting a payment
belonging to the other client.
This is why plan §3.3 forbids retracting anything — including any historical cleanup of the
duplicate clients' data — until a verification query shows zero charges with more than one parent.
## Options, roughly in order of preference
1. **Split the shared charges, then let removal run normally.** Re-import the affected window now
that keys are client-scoped, so each client creates its own charge entity. This reuses the
import path rather than hand-constructing component entities. Verify with a query for charges
having more than one referencing order; it must reach zero.
2. **Guard the retraction.** Before retracting an order, check whether any of its charges are
referenced by another order; detach those (retract the `:sales-order/charges` ref rather than
the charge) and retract the rest. Small, contained change, and it makes the operation safe
regardless of what shape the data is in — worth doing on its own merits even after a split.
3. **Do nothing and accept it.** Only defensible once every location has a single client *and*
the historical shared charges are gone. Not true today.
## What I did about it during the validation run
I ran the import on the restore with `remove-voided-orders` **skipped**, and ran the other steps
(`upsert-locations`, `upsert`, `upsert-payouts`, `upsert-refunds`) normally. That kept the
validation faithful to how the import behaves without risking silent payment loss in the data
the measurements were about to be taken from.
**Nothing in the production system has been changed.** This note is about a risk that already
exists there.