HarperFast / HarperFast/harper
Concurrent multi-path deliveries of the same write all persist an audit entry: identity-tie dedup is a read-then-check with no serialization
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
## Summary
Replication's identity-tie duplicate detection is a read-then-check guard with no
serialization against a *concurrent* identical delivery. When the same write reaches a node
N times at once over N different paths (same record id, same version, same origin node id),
all N applies pass the guard and all N persist a transaction-log/audit entry. The record
converges (the value is identical for a `put`), but the audit log is amplified ×N, and for a
commutative/CRDT patch the re-apply looks like a genuine double-fold rather than a dropped
duplicate.
This invalidates a documented design assumption. `harper-pro` `replication/DESIGN.md` item 11
knowingly leaves multi-path delivery in place for directional peers:
> The multi-hop dedup exclusion (`replicationConnection.ts` `qualifies` checks feeding
> `SUBSCRIPTION_UPDATE excludeNodes`) tests only `replicates === true || replicates?.sends`,
> so a directional peer whose record is `{ sendsTo: [...] }` (no `.sends`) … is NOT added to
> the exclusion list. The result is that a subscriber may receive some records via more than
> one path; **these are idempotent (replication applies by sequence), so it is a missed
> optimization, not data loss.**
The fan-out is the known gap and is fine to leave; the idempotence it rests on is what does
not hold. That is what this issue is about.
## Observed
A v4→v5 migration topology: writes enter a 15-node v5 mesh through a single one-way
(directional) bridge peer. Because the bridge origin never gets into `excludeNodes`, every
v5 peer relays those writes to every other peer, so each node receives each write ~15×.
10s audit sample on one v5 node:
- 228/228 sampled write-versions appear as duplicate audit entries
- modal duplicate count **exactly 15** (= peer count), not a spread
- same record id + same version + identical timestamp per group (e.g. one upsert stored as
15 separate audit entries)
Cost in that cluster: ~15× audit bytes and ~9 cores/peer of apply work. Disk was not a
constraint there, and the topology is temporary — but the dedup race is core-side and
outlives the bridge. Any directional or otherwise multi-path topology reproduces it.
The exact 15/15 ratio is the part worth noting: a plain "all arrived before the first one
committed" race would produce a spread. A deterministic N-of-N says the guard is disarmed
structurally, not occasionally.
## Mechanism
Two independent reasons a concurrent twin is not recognized. Both are in
`resources/Table.ts` / `resources/DatabaseTransaction.ts` on `main` (checked at `0db6cabe6`).
**1. The audit/transaction-log append escapes the transaction's atomicity.** From
`resources/DatabaseTransaction.ts:934`:
> log entries batch on the native transaction and are durably written by its commit attempt
> — even a failed one — so they survive the abort-after-failed-commit of the retry paths
So of N concurrent identical applies, the N−1 that lose optimistic concurrency on the record
still leave their audit entries behind. The record write is correctly conflict-detected; the
log append is not, and there is no uniqueness constraint on the log key it would conflict
against.
**2. `stagedOwnAuditEntry` cannot distinguish "my own orphaned append" from "a concurrent
twin's committed append", and disarms dedup for exactly the latter.**
`resources/Table.ts:2591`:
```ts
const stagedOwnAuditEntry = retry && write.appendedAuditEntry === true;
```
It gates both keyed dedup guards — the up-front lookup (`Table.ts:2743`) and
`isReDeliveredDuplicate` (`Table.ts:2804`). Its purpose is correct: a write that appended its
own entry in a failed attempt must not read that orphan back as "already applied" and drop
itself. But a concurrent twin is in the same state — it also appended, also conflicted, also
retried — so on its retry it assumes the entry it would find is its own and skips the check,
when the entry it would find is the *winner's* and the write really was already applied.
The existing comment at that line reasons about a duplicate co-batched in the *same*
transaction ("the transaction-wide retry flag would also suppress dedup for a genuine
re-delivered duplicate co-batched with the conflicting write"). The cross-transaction,
cross-worker twin — different `write` object, different transaction, possibly a different
worker thread, so no read-your-writes visibility either — is the case not covered.
Broken invariant, stated plainly: the guards assume *audit entry for (version, nodeId, key)
exists ⟺ this write was already applied*. Under concurrent multi-path delivery both
directions fail — an entry can exist for a write that never committed (1), and a write can
be applied twice while an entry for it already exists (2).
## Impact
Confirmed:
- Audit/transaction-log amplification ×(paths). Retention purges it eventually, so it is
bounded in time but proportional for as long as the topology exists.
- Apply CPU ×(paths) per node.
- Compounding downstream: duplicate audit entries are what a full-copy audit replay re-ships,
so a node that copies from an amplified node inherits the amplification.
- Silent. Nothing alarms; cluster status, connection truth and convergence all look healthy.
It shows up only as audit growth and CPU.
Derived from the code, **not yet observed** — worth confirming first thing:
- A commutative/CRDT patch (increment) delivered over N paths should be double-applied by
mechanism 2, since the retry re-runs the fold with dedup disarmed. That is value
corruption, not just waste. The field case was upserts (full `put`s, value-idempotent), so
it would not have shown there. Related history: #1137, #1148.
## Suggested direction
Enforce the identity rather than pre-checking it:
- The log key already *is* the identity — `auditStore.get(version, tableId, id, nodeId)` is
the existing keyed lookup. Making the append conditional-on-absent (so a twin's append
conflicts and retries, instead of a read-then-write check both twins pass) puts enforcement
at the store, where concurrency is actually resolved. This lines up with the dual-clock
direction in #2412, where the first word is the transaction timestamp / log key and is
identity per origin log.
- Failing that, make mechanism 2's guard precise: tag the append with the attempting writer's
identity so a retry can tell its own orphan from a twin's committed entry, instead of
keying off the boolean "I appended something".
## Repro sketch
Three v5 nodes where B and C both receive from A over directional (`sendsTo`) routes and also
relay to each other, so B receives A's writes twice. Write a record on A; assert exactly one
audit entry per (id, version, nodeId) on B. Then repeat with a `patch` carrying an increment
and assert the counter advanced once. A unit-level version is cheaper: two concurrent applies
of the same (id, version, origin nodeId) through the replication apply path, on separate
transactions, and assert one audit entry and one fold.
## Versions
Observed on a 5.1.x mesh. Both guards and the escaping log append are present on `main`
(`0db6cabe6`), so the defect is not release-specific.
Contributor guide
Assessment
This issue has not been assessed yet.