notid/integreat
1
0
Fork 0
Code Issues Pull Requests 4 Actions Packages Projects Releases Wiki Activity

fix(square): do not resolve a record that belongs to another client

Browse Source 
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>
This commit is contained in:
Bryce
2026-08-16 08:09:53 -07:00
parent 57a84dae11
commit 10d0d01b82
6 changed files with 375 additions and 180 deletions
Download Patch File Download Diff File Expand all files Collapse all files

202
docs/2026-08-15-sales-summary-rollout-plan.md
View File

@@ -9,8 +9,8 @@ Measured on a restored copy of production (backup point `209608347`), 210 client
with zero days knocked out of balance and zero already-balanced days altered.
Of the 279 days left, **171 are not balancing faults** — they are days where a client's sales were
never imported while its refunds were. Step 9 is about those, and it is the most important thing in
this document.
never imported while its refunds were. Step 10 is about those, and it is the most important
thing in this document.
---
@@ -23,16 +23,54 @@ this document.
| Expected migration runtime | ~13 minutes for 19M orders on a warm cache |
| Nothing here touches | invoices, payments, the ledger, or any client without the flag set |
**One prerequisite that is not code.** Ten Square locations are configured against two client records
each. Someone in the business has to decide which record survives at each. The newer record usually
has no history from before the split, so keeping it loses years of that location's books. Do this
before step 3.
**Client configuration is left exactly as it is.** Ten Square locations are configured against two
client records each, and both stay active. The re-key is what resolves them: once every record
carries its owner in its key, each client's import resolves only its own records and the two
records keep independent, stable histories. No "which record survives" decision is needed, and
nothing is deactivated.
The consequence to be aware of: each Square payment, refund, payout and shift at a shared location
becomes **two entities, one per client record** — by design. That is the stable end state, not a
duplicate to clean up. If any report or export aggregates across client records, one restaurant's
takings would be counted twice at that layer. Nothing in this work changes that either way.
**The only window of risk is between deploying and finishing the migration**, while legacy keys
still exist for a client to resolve. Steps 2–5 exist to make that window effectively zero.
---
## Step 1 — Deploy the code
## Step 1 — Guard `remove-voided-orders`
Deploy the branch as normal. The flag is absent from every client, so:
Do this before the migration, not after. `:sales-order/charges` is `:db/isComponent true`, so
retracting an order cascades into its payments. Until step 4 finishes there are still payments with
two parent orders, and deleting one client's voided order can take the other client's payment with
it.
Either leave `remove-voided-orders` switched off until step 4 verifies clean, or change it to detach
a payment that has more than one parent rather than delete it. Detaching is worth doing regardless —
it makes the safety a property of the deletion rather than of the migration having been run first.
See `docs/2026-08-15-remove-voided-orders-risk.md`.
---
## Step 2 — Pause the Square importer
**This is what makes the deploy safe, and it is easy to skip.** Steps 2 through 5 should be one
maintenance action, not separate days' work.
While legacy keys exist, `square.core3/existing-id` falls back to them — and at a shared location
that is the one code path that can reach across client records. Running the migration with imports
paused means no client is resolving keys while the keys are being rewritten, so the window closes
entirely rather than merely narrowing.
The migration itself takes about **13 minutes** for all 19M orders, so the pause is short.
---
## Step 3 — Deploy the code
Deploy the branch. The flag is absent from every client, so:
- tips are calculated exactly as they are today,
- no `Service Charges` line is written.
@@ -41,60 +79,33 @@ The only changes that take effect immediately are the safe ones: imbalance loggi
dirty-summary scan bounded to one client (1,321 ms → 5.6 ms per client), the schema-ordering fix,
and the importer's new client-scoped keys.
**The importer starts writing client-scoped keys straight away, and reads both schemes.** That is
deliberate and is what makes the deploy independent of the migration. Do not remove the legacy
lookup in `square.core3/existing-id` yet — see step 8.
**Verify before moving on.** After one nightly import cycle:
```clojure
;; refunds, payouts and shifts must not have doubled
(count (d/datoms (d/db conn) :aevt :sales-refund/external-id))
(count (d/datoms (d/db conn) :aevt :expected-deposit/external-id))
(count (d/datoms (d/db conn) :aevt :cash-drawer-shift/external-id))
```
Compare against the same counts taken immediately before deploy. Growth should be ordinary daily
volume. A near-doubling means the legacy fallback is not working — **stop and roll back the deploy**.
---
## Step 2 — Guard `remove-voided-orders`
Do this before the migration, not after. `:sales-order/charges` is `:db/isComponent true`, so
retracting an order cascades into its payments. Until step 3 finishes there are still payments with
two parent orders, and deleting one client's voided order can take the other client's payment with
it.
Either leave `remove-voided-orders` switched off until step 3 completes, or change it to detach a
payment that has more than one parent rather than delete it. Detaching is worth doing regardless —
it makes the safety a property of the deletion rather than of the migration having been run first.
See `docs/2026-08-15-remove-voided-orders-risk.md`.
---
## Step 3 — Retire the duplicate client records
Business decision from the top of this document. Deactivate the losing record's Square location so
the importer stops fetching for it. The record itself stays; its history is untouched.
**Verify:** no Square location is configured against two active client records.
**The importer reads both key schemes**, so the deploy does not depend on the migration having
finished. Two protections cover the interval before it does: imports are paused (step 2), and
`existing-id` refuses to resolve a record that already belongs to a different client. Do not remove
the legacy lookup yet — see step 9.
---
## Step 4 — Run the migration
Run it immediately after the deploy, while imports are still paused.
```clojure
(require '[auto-ap.jobs.rekey-square-external-ids :as rk])
;; read-only first — check :collisions is empty for every attribute
(dissoc (rk/plan (d/db conn) :charge/external-id rk/charge-prefix) :new-keys)
;; read-only first — no two entities may want the same key. `plan` does NOT return a
;; :collisions key; you have to hand its :new-keys to `collisions` yourself.
(rk/collisions (:new-keys (rk/plan (d/db conn) :charge/external-id rk/charge-prefix)))
;; => [] (anything else: stop, do not migrate)
;; then the whole thing
(rk/migrate-all! 2000)
```
`migrate-all!` runs this same check itself, on every attribute including charges, and throws
rather than transacting if it finds one. Running it by hand first just means finding out before
the 13-minute walk rather than partway through it.
Runs in about thirteen minutes over 19M orders. It is **idempotent and resumable** — a record that
already carries the right name is skipped, so it can be stopped and re-run without consequence.
@@ -102,25 +113,69 @@ If it appears to crawl, the cause is almost certainly garbage collection in the
not the transactor. That misdiagnosis cost two days of projected runtime during this work. Free
retained memory in the REPL and re-measure before changing anything about the database.
**Verify — all four must read zero to migrate and zero unscopable:**
**Verify.** Two checks, doing two different jobs — run both.
**(a) Completeness, across everything.** `plan` must report nothing left to do, for all four
attributes:
```clojure
(rk/unscoped-report (d/db conn))
(dissoc (rk/plan (d/db conn) :charge/external-id rk/charge-prefix) :new-keys)
;; => {:total 17047142 :to-migrate 0 :already-scoped 17047142 :unscopable 0}
```
;; and the gate that this work exists for
(rk/charges-with-multiple-parents (d/db conn) (take 400000 (rk/all-order-ids (d/db conn))))
Read `:to-migrate 0` **and** `:unscopable 0`. This is the authoritative signal, and it covers all
17M charges.
`unscoped-report` is useful colour but is not the gate: its `:no-owner` column never reaches zero
for charges, because ~283k payout stubs carry no `:charge/client` of their own and it classifies
by attribute rather than by resolving ownership. Judge completeness by `plan`.
**(b) The safety gate for the cascade** — no payment may answer to two orders, or re-enabling
`remove-voided-orders` in step 8 can delete a payment another order still needs. Check **every**
order in the last year, with no sampling:
```clojure
(let [db (d/db conn)
cs (map first (d/q '[:find ?c :where [?c :client/code _]] db))
year (java.util.Date. (- (.getTime (java.util.Date.)) (long (* 365 86400000))))]
(rk/charges-with-multiple-parents
db (map first (iol-ion.query/scan-sales-orders db cs year nil))))
;; => 0
```
Note `unscoped-report`'s `:no-owner` column is not a gap: ~283k payout-stub payments carry no
`:charge/client` attribute of their own, so it cannot verify them by attribute. `plan` resolves
ownership through whatever refers to them and is the figure to trust.
On the restored copy that is 5,159,787 orders — 27% of the table — via the
`:sales-order/client+date` index. A year is chosen deliberately: `remove-voided-orders` only ever
deletes orders Square reports as voided, which are recent, so that is where the destructive risk
lives. Completeness across all of history is check (a)'s job, not this one.
> Do **not** sample this with `(take n (rk/all-order-ids db))`. `all-order-ids` streams `:aevt`,
> which is ascending entity id, so a `take` returns the *oldest* orders — on the restored copy the
> first 400,000 are all from 2019–2021, before any of the contention this gate looks for. It would
> report a confident zero having inspected none of the relevant data.
---
## Step 5 — Recompute summaries, flags still off
## Step 5 — Resume the Square importer
Only once step 4's two checks read clean. The maintenance window ends here.
The first cycle after resuming is the one to watch. Compare these against the same counts taken
immediately before the deploy — growth should be ordinary daily volume:
```clojure
(count (d/datoms (d/db conn) :aevt :sales-refund/external-id))
(count (d/datoms (d/db conn) :aevt :expected-deposit/external-id))
(count (d/datoms (d/db conn) :aevt :cash-drawer-shift/external-id))
(count (d/datoms (d/db conn) :aevt :charge/external-id))
```
A near-doubling of any of them means records are being created rather than matched — **stop and
roll back the deploy.** Charges are included deliberately: they are the one that doubles a client's
takings rather than merely duplicating a row.
---
## Step 6 — Recompute summaries, flags still off
```clojure
(require '[auto-ap.jobs.sales-summaries :as ss])
@@ -128,7 +183,7 @@ ownership through whatever refers to them and is the figure to trust.
```
This is the pass that banks the deduplication. **Capture the result before going further** — you
will need it as the baseline for step 6, and it cannot be reconstructed afterwards:
will need it as the baseline for step 7, and it cannot be reconstructed afterwards:
```clojure
(require '[auto-ap.tools.compare-sales-summaries :as cmp]) ; test/dev classpath
@@ -143,7 +198,7 @@ will need it as the baseline for step 6, and it cannot be reconstructed afterwar
---
## Step 6 — Turn the flag on, a few restaurants at a time
## Step 7 — Turn the flag on, a few restaurants at a time
Needs accounting sign-off first: `summary-service-charges` posts to **49000 Service Income**, chosen
so the work could be measured. It affects reporting, never whether a day balances.
@@ -156,7 +211,7 @@ so the work could be measured. It affects reporting, never whether a day balance
Start with two or three restaurants, confirm, then widen.
**Verify** against the capture from step 5:
**Verify** against the capture from step 6:
```clojure
(def after (cmp/summaries-in (d/db conn) start end))
@@ -168,29 +223,34 @@ The two numbers that matter — both were zero across all 18,900 client-days in
- `:balanced->unbalanced` must be **0**
- previously-balanced days whose lines changed must be **0**
If either is non-zero, retract the flag for the affected clients and re-run step 5. The flag is the
If either is non-zero, retract the flag for the affected clients and re-run step 6. The flag is the
rollback: removing it restores today's behaviour exactly.
---
## Step 7 — Re-enable `remove-voided-orders`
## Step 8 — Re-enable `remove-voided-orders`
Safe once step 4's gate reads zero. Keep the detach-rather-than-delete guard from step 2.
Safe once step 4's gate reads zero. Keep the detach-rather-than-delete guard from step 1.
---
## Step 8 — Remove the legacy key lookup
## Step 9 — Remove the legacy key lookup
Only once `plan` reports `:to-migrate 0` and has stayed there through several import cycles. Drop
the second branch of `square.core3/existing-id`. At that point two clients sharing a location
becomes structurally incapable of producing a shared record, rather than prevented by a convention a
future import could break.
**Schedule this; do not leave it open-ended.** Both client records at a shared location stay active
permanently, so the legacy fallback in `square.core3/existing-id` is the one code path that can ever
reach across them. Deleting it is what turns the guarantee from conventional into structural.
This is the last step and there is no hurry.
Once `plan` reports `:to-migrate 0` and has stayed there through several import cycles, drop the
legacy branch of `existing-id` — and with it `owned-by-other-client?`, which exists only to make
that branch safe while it lives. After this, two clients on one location are structurally incapable
of resolving onto each other's records, and no ordering discipline is required to keep it that way.
Until it is done, the protection is the guard plus the maintenance window, both of which depend on
people doing the right thing. That is the reason not to let this drift.
---
## Step 9 — Deal with the refunds that have no sales behind them
## Step 10 — Deal with the refunds that have no sales behind them
**The most important item in this document, and the only one that is not just execution.**
@@ -256,7 +316,7 @@ tender against $6,059.57 of order totals on one day), the ezCater fee question,
clusters on NGMV and NGEB.
Plus 15 days / $974.99 where a processing fee lands on a day with no trading — the same shape as
step 9 but from the payout side, so it needs the payout modelled rather than a rule in the summary.
step 10 but from the payout side, so it needs the payout modelled rather than a rule in the summary.
---