Skip to content

Commit 7291339

Browse files
heskewclaude
andcommitted
rework: detect re-delivered out-of-order duplicate up front (#1137)
Per cross-model review (Codex): the prior commit replaced the capped-path keyed audit lookup with an additionalAuditRefs check, which dropped duplicate detection for a write re-delivered on the main (in-order) previousVersion chain. Instead: - Move the additionalAuditRefs duplicate check to the TOP of the out-of-order block, so a re-delivered out-of-order write is skipped reliably before the audit walk — immune to the walk stopping at the #1114 depth cap or breaking early on a not-yet-visible audit entry (both let the prior keyed lookup miss the duplicate). - Restore the capped-path keyed auditStore.get lookup unchanged as the best-effort guard for a re-delivered originally-in-order write (which leaves no ref); its residual unreliability under load is tracked in #1148. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 9484c65 commit 7291339

1 file changed

Lines changed: 42 additions & 19 deletions

File tree

resources/Table.ts

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1741,6 +1741,32 @@ export function makeTable(options) {
17411741
// of the updates to the record to ensure consistency across the cluster
17421742
// TODO: can the previous version be older, but even more previous version be newer?
17431743
if (audit) {
1744+
// A re-delivered out-of-order write (full-copy audit-replay re-delivers writes) must not have
1745+
// its commutative ops re-folded. additionalAuditRefs is the record's own list of folded
1746+
// out-of-order versions, read with read-your-writes consistency, so this skips the duplicate up
1747+
// front — before the audit-log walk below, which can miss it: the walk stops at the depth cap, or
1748+
// breaks early on a not-yet-visible audit entry, before reaching txnTime, and the keyed
1749+
// transaction-log lookup it would otherwise use can lag a back-to-back re-delivery (that lag
1750+
// silently double-applied the increment — #1137). This covers the re-delivery while the ref is
1751+
// still on the record; a later in-order write rewrites the record and drops the ref (it survives
1752+
// only as previousAdditionalAuditRefs on the audit log), so that case falls back to the
1753+
// best-effort keyed lookup in the capped block below — see #1148. precedesExistingVersion(...)
1754+
// === 0 is the identity tie: same version AND same node (the local node is id 0, so an undefined
1755+
// options?.nodeId resolves to the same 0 the ref stored).
1756+
if (
1757+
existingEntry.additionalAuditRefs?.some(
1758+
(ref) =>
1759+
ref.version === txnTime &&
1760+
precedesExistingVersion(
1761+
txnTime,
1762+
{ version: txnTime, localTime: txnTime, key: id, nodeId: ref.nodeId },
1763+
options?.nodeId
1764+
) === 0
1765+
)
1766+
) {
1767+
write.skipped = true;
1768+
return; // out-of-order write already folded into this record
1769+
}
17441770
// incremental CRDT updates are only available with audit logging on
17451771
let localTime = existingEntry.localTime;
17461772
let auditedVersion = existingEntry.version;
@@ -1872,7 +1898,12 @@ export function makeTable(options) {
18721898
// retained window are not layered in — but the authoritative full-copy record restores exact
18731899
// convergence. Because we stopped before reaching txnTime, the inline duplicate detection in
18741900
// the walk never ran; full-copy audit-replay re-delivers writes, and re-applying one would
1875-
// double-apply its commutative ops, so rule that out here before folding.
1901+
// double-apply its commutative ops. A re-delivered out-of-order write is already ruled out by
1902+
// the additionalAuditRefs check at the top of this block; this keyed lookup is the best-effort
1903+
// guard for the remaining case — a re-delivered write that was originally in-order (so it left
1904+
// no ref) and is now deeper than the cap. It is best-effort because the transaction-log lookup
1905+
// can intermittently miss an entry under load (tracked separately); the authoritative full-copy
1906+
// record still restores exact convergence.
18761907
logger.warn?.(
18771908
'Out-of-order audit reconciliation exceeded depth cap; reconciling against most recent updates only',
18781909
{
@@ -1881,24 +1912,16 @@ export function makeTable(options) {
18811912
depth: walkSteps,
18821913
}
18831914
);
1884-
// Detect a re-delivered duplicate via the record's own additionalAuditRefs rather than an
1885-
// audit-log lookup at txnTime. Every out-of-order write folded into this record records its
1886-
// {version, nodeId} ref (added in the walk above, RocksDB-only — the same condition as this
1887-
// cap), and the record is read with read-your-writes consistency. The transaction-log query
1888-
// that a keyed audit lookup would use can lag a back-to-back re-delivery — the just-committed
1889-
// entry is not yet visible — which silently double-applied the commutative op (#1137).
1890-
// precedesExistingVersion(...) === 0 is the identity tie: same version AND same node (the
1891-
// local node is id 0, so an undefined options?.nodeId resolves to the same 0 the ref stored).
1892-
const alreadyApplied = existingEntry?.additionalAuditRefs?.some(
1893-
(ref) =>
1894-
ref.version === txnTime &&
1895-
precedesExistingVersion(
1896-
txnTime,
1897-
{ version: txnTime, localTime: txnTime, key: id, nodeId: ref.nodeId },
1898-
options?.nodeId
1899-
) === 0
1900-
);
1901-
if (alreadyApplied) {
1915+
const duplicate = auditStore.get(txnTime, tableId, id, options?.nodeId);
1916+
if (
1917+
duplicate &&
1918+
duplicate.version === txnTime &&
1919+
precedesExistingVersion(
1920+
txnTime,
1921+
{ version: txnTime, localTime: txnTime, key: id, nodeId: duplicate.nodeId },
1922+
options?.nodeId
1923+
) === 0
1924+
) {
19021925
write.skipped = true;
19031926
return; // duplicate write already applied
19041927
}

0 commit comments

Comments
 (0)