docs(square): record how remove-voided-orders can delete another client's payments
:sales-order/charges is a component attribute, so [:db/retractEntity order] cascades into the charges. Where two clients were configured on one Square location, both clients' orders resolved to the SAME charge entity, because charge keys carried no client scoping and :charge/external-id is :db.unique/identity. Retracting a voided order therefore deletes a charge the other client's order still references, leaving that order with sales and no tender: the day goes out of balance and the payment disappears from the current database value. Measured on the restore: 35,870 of 56,829 charges (63%) in the contended clients' recent window have more than one parent order. Phase 0 stops new sharing and the re-key makes it structurally impossible going forward, but neither splits the charges that are already shared, so the hazard outlives both. Records the options, including guarding the retraction so it detaches shared charges rather than deleting them. Pre-existing risk, not introduced here. The validation run skipped this step. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
115
docs/2026-08-15-remove-voided-orders-risk.md
Normal file
115
docs/2026-08-15-remove-voided-orders-risk.md
Normal 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.
|
||||||
Reference in New Issue
Block a user