Skip to content

Concurrent multi-path deliveries of the same write all persist an audit entry: identity-tie dedup is a read-then-check with no serialization #2485

Open

Description

@kriszyp

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:

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:

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 Adopt the dual-clock record model on RocksDB: first word = transaction timestamp/log key, source/origin version in a distinct second word (stages 0b and 2) #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.

Activity

  1. added this to the v5.3 milestone on Sep 3, 2026
  2. added theissue type on Sep 3, 2026
  3. self-assigned this
    on Sep 3, 2026
  4. kriszyp commented on Sep 3, 2026

    @kriszyp
    MemberAuthor

    Phase 1: amplification confirmed, CRDT double-apply refuted — and the mechanism narrows to reason 1 alone

    Unit-level repro on main (0db6cabe6), RocksDB, audit: true. N deliveries of the same
    (record id, origin version, origin nodeId), each on its own sourceApply transaction with the
    origin version as the transaction timestamp, exactly as the subscription apply loop drives them.

    deliveries transaction-log entries at that identity counter advanced by
    3 concurrent 3 1
    15 concurrent 15 1
    5 serial (back to back, no visibility wait) 1 1
    5 serial (with visibility wait) 1 1

    Amplification: confirmed, and deterministically exactly N — matching the field's modal-15-of-15
    rather than a spread. Serial re-delivery dedupes correctly, so the defect is specific to concurrent
    delivery.

    Commutative/CRDT double-apply: refuted for this shape. The counter advanced exactly once at
    both N=3 and N=15. Two independent reasons, both visible in a traced run:

    1. All N twins fold their increment onto the same pre-write base, because none can see another's
      staged record. So even with dedup entirely absent the value converges on one increment — a
      double fold needs a delivery whose base is already the winner's committed record.
    2. Every loser's retry is dropped, by the out-of-order walk's inline identity-tie check
      (Table.ts ~2833, "treat a tie as a duplicate and drop it"). That check is not gated by
      stagedOwnAuditEntry, so it still fires when both keyed guards are disarmed.

    The mechanism is narrower than the issue states

    A traced run shows all N entries come from the N first attempts, and every retry is skipped:

    commit { retry: false, stagedOwnAuditEntry: false, existingVersion: <older> }   x15  -> apply + append
    commit { retry: true,  stagedOwnAuditEntry: true,  existingVersion: <version> } x14  -> skip walk-tie
    

    At the moment the N first attempts run their dedup lookups, nothing is committed at that identity,
    so no read-then-check guard could have fired — the guards behave correctly. Reason 1 (the append
    escaping the transaction's atomicity) is the whole of the observed defect; reason 2
    (stagedOwnAuditEntry) contributes none of the 15 entries.

    Direct consequence for the two suggested directions: the narrower option — tagging the append with
    the attempting writer's identity so a retry can tell its own orphan from a twin's committed entry —
    does not reduce the entry count at all.
    It changes which guard drops the retry (keyed instead of
    walk-tie); the loser's own orphan from attempt 1 still persists. It is worth doing as correctness
    hardening for the window below, but it is not a fix for this issue.

    Why the orphan becomes visible, and why nothing after the append can remove it

    Tracing this through rocksdb-js 2.8:

    • executeLogWork writes the transaction's log batch to the file before executeCommitWork
      runs the RocksDB commit, so the bytes land regardless of the commit outcome.
    • commitFinished (which advances the committed-read watermark past them) runs only on
      status.ok(). committedPosition survives resetTransaction(), and addLogEntry is write-once
      per transaction handle, so the retry never re-appends.
    • The loser's retry re-runs with its write skipped, so it commits an empty batch — successfully —
      and that success is what publishes the batch its failed first attempt wrote.
    • Abandoning instead of committing does not help: commitAborted only stops the position pinning
      the watermark, which advances past the bytes. As the source comment puts it, the bytes
      themselves cannot be unwritten.

    So the enforcement point has to be before the append is staged. Nothing downstream of it —
    a better pre-check, a retry-aware guard, an abort — can retract an entry, and read-side dedup would
    leave the primary stated cost (×N audit bytes) untouched.

    Also worth pinning while we are here: two distinct origins that happen to share a version are two
    writes and must each keep an entry, so any fix has to key on the full (nodeId, version, tableId, id)
    identity, never on version alone. The regression test covers that case.

    Double-apply window that does remain (reasoned, not reproduced)

    isReDeliveredDuplicate() is the only guard on two paths — the depth-cap-exceeded walk
    (Table.ts ~2966) and the fully-superseded early-out (~2913) — and it is gated by
    stagedOwnAuditEntry. A twin whose retry reaches either of those, or whose keyed lookup misses the
    winner's entry through the documented lag (#1137), would re-fold against the winner's record. I could
    not reproduce it: the depth-cap path needs >1000 newer audit entries on the key to arrive inside the
    retry window, an increment is never fully superseded (commutative ops survive rebuildUpdateBefore),
    and the lookup lag did not appear in a back-to-back serial re-delivery. This is the pre-existing
    best-effort gap that #1137/#1148 documented; the concurrency does not create it.

    Severity

    Confirmed cost is the ×(paths) transaction-log bytes and apply CPU, with the downstream compounding
    already described. Value corruption is not reproduced, so this is a storage/CPU amplification
    defect rather than a data-integrity one.

    Repro

    unitTests/resources/concurrentDuplicateApply.test.js on the fix branch — 15 concurrent identical
    applies, asserting one entry and one fold. It fails on main sources with a forced rebuild
    (expected 1, actual 15) and asserts the race actually happened (at least one commit attempt lost
    optimistic concurrency) so it cannot pass by being serialized. RocksDB-only: LMDB keys audit entries
    by local audit time, so it has neither this identity nor the keyed dedup.

  5. kriszyp commented on Sep 3, 2026

    @kriszyp
    MemberAuthor

    Phase 2: enforcement point settled, and the fix is sequenced behind #2412

    Recording the decision and the spec so whoever lands it does not have to re-derive any of this.

    Decision (owner's call): do not land a core-only fix now; build the enforcement on #2412's
    identity model.
    The core-only version below is implementable today and was prototyped, but it can
    only promise "exactly one entry and one apply for concurrent deliveries within a live process" —
    see the crash window at the end — and #2412 is the work that makes the write identity unambiguous
    enough to enforce properly. Cost of waiting is the ×(paths) amplification persisting, which
    harper-pro#809 removes the field trigger for
    independently.

    Relationship to harper-pro#809

    Complementary, not overlapping, and worth reading together. #809 fixes the delivery side: it makes
    qualifiesForMultiHopExclusion cover directional sendsTo peers so an origin's writes stop being
    relayed by every mesh member. That removes the topology that produced the observed ×15. This issue is
    the receive-side invariant that survives any multi-path arrival — overlapping subscriptions across a
    reconnect, a proxied subscription, a mesh with both a direct and a relayed route — and it is the
    sentence #809's own DESIGN.md gap rested on ("these are idempotent, so it is a missed optimization,
    not data loss"). Phase 1 shows the idempotence holds for the record value and not for the log. #809's
    body already refers to this as "concurrent-delivery dedup race, tracked separately"; this is that.

    What the fix has to be (two shapes are ruled out, one survives)

    Both directions this issue suggested are disqualified by the phase-1 evidence:

    What survives is transaction-scoped, outcome-aware single-flight, before anything is staged:

    1. Claim the origin transaction identity — origin nodeId plus origin transaction timestamp, which
      is exactly the pair Adopt the dual-clock record model on RocksDB: first word = transaction timestamp/log key, source/origin version in a distinct second word (stages 0b and 2) #2412 makes authoritative — through the store's existing cross-thread lock
      (store.tryLock/unlock, the per-DBDescriptor map already used per-record on the cache-fill
      path). Cross-thread is required: harper-pro's replicator subscribes on every thread, so the N paths
      land wherever their TCP connections did.
    2. A duplicate delivery waits for the owner's outcome, then runs the existing committed-state
      dedup. It is skipped on committed evidence, exactly as a serial re-delivery is today, and applies
      if the owner failed, so it takes over rather than reporting a write applied that never landed.
    3. Release must be idempotent, exception-contained, attached before any claimed operation can throw,
      and reached from every settlement path — synchronous commit-handler throw, async pre-commit
      rejection, terminal native rejection, timeout abort, chained-store failure and successful reuse all
      settle differently. A leaked claim stalls that origin transaction's delivery for the process
      lifetime.

    Two shapes that look simpler and are wrong, both caught by an independent planning review after I had
    prototyped the first one:

    • Claiming per write identity instead of per origin transaction can split an atomic source
      transaction: staging order across keys is not guaranteed equal between two relays of one transaction
      (a warm key reaches addWrite before a cold key that arrived earlier — DESIGN.md, Secondary index permanently diverges from the primary store, and rewriting the record does not repair it (5.2.1 / rocksdb-js 2.7.1) #2211), so one
      delivery can win write A while another wins write B and each commits a partial subset. It also
      collides with same-key writes chained in one transaction, which share the whole identity.
    • Skipping a contender immediately rather than waiting can lose the only viable copy: the
      contender advances its delivery cursor, and if the owner then fails terminally there is nothing to
      recover from, which contradicts the premise that a source apply has no reliable resubscribe/resume
      path (Table.ts ~903-907).

    Scope when it is built: the audited source-apply paths, _writeUpdate and _writeDelete. The delete
    path has no identity-tie dedup at all today, so duplicated deletes amplify the same way. Local writes,
    audit-off tables, copy-apply frames, replay (which never sets isNotification) and LMDB must pay
    nothing, and that should be asserted rather than assumed. Measured cost of the claim on one box: 1.6 µs
    per acquisition, against roughly 20 µs for a batched replicated apply per write, so one claim per origin
    transaction amortises to about a thousandth of that.

    The crash window belongs to the native change

    Even with single-flight, a process that dies between a winner's log append and its commit leaves an
    orphan that a post-restart redelivery reads as proof of application, and the mutation is lost. That is
    pre-existing today — stagedOwnAuditEntry provenance is in-memory — and closing it needs the
    rocksdb-js change to make the log batch participate in the commit outcome, or keep it unpublished and
    retractable until the data commit succeeds. Worth deciding alongside #811 rather than separately.

    Repro branch

    fix/concurrent-duplicate-apply-audit-entries carries
    unitTests/resources/concurrentDuplicateApply.test.js, which fails on main with a forced rebuild:
    15 concurrent identical applies leave 15 entries where one is expected, and three concurrent deliveries
    of a two-write same-key transaction leave 6 where two are expected. Its other two cases — a re-delivery
    arriving after the first has committed, and two distinct origins sharing a version — pass on main and
    are the behaviours a fix must not regress. No PR: with the fix sequenced behind #2412 there is nothing
    mergeable yet, so the branch is the artifact.

    Also worth a look from whoever picks this up: #2259 is in the same commit path and is about the
    neighbouring identity problem, one version identifying two different stored values after a resequenced
    write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

Fields

Priority

P1

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions