diff --git a/resources/DESIGN.md b/resources/DESIGN.md index 6cf880efe9..1b6b37f001 100644 --- a/resources/DESIGN.md +++ b/resources/DESIGN.md @@ -376,7 +376,7 @@ Blobs flagged with `saveBeforeCommit` (or `saveInRecord`) are written to disk in The mitigations live in three places: - `startPreCommitBlobsForRecord` (`blob.ts`) returns the blob list alongside its completion callback so each `TransactionWrite` can attach a `savedBlobs: Blob[]`. `cleanupUnusedBlobs(blobs)` (`blob.ts`) waits for each blob's `saving` promise to settle, then `deleteBlob`s the file — it clears the input list so it's idempotent across repeated calls (e.g. an early-return that also gets caught by the abort path). -- `Table.ts` commit handlers set `write.skipped = true` (and reset to `false` at the top of each invocation) on early-return paths that don't write the record/audit: duplicate-tie, superseded-by-put, no-audit-fullUpdate-loses, and cache-resolve version-changed. The transaction commit success paths (`DatabaseTransaction.commit` and `LMDBTransaction.commit`) walk writes and call `cleanupUnusedBlobs(write.savedBlobs)` for every still-skipped write. Cleanup is deferred (rather than run inline in the commit handler) because the commit handler runs again on optimistic-lock retries, and a retry can flip a previously-skipped write into a successful one (e.g. the existing record gets deleted between attempts so the older replicated update suddenly wins). Inline cleanup would race the deletion's `setTimeout` against the retry that referenced the blob. +- `Table.ts` commit handlers set `write.skipped = true` (and reset to `false` at the top of each invocation) on early-return paths that don't write the record/audit: duplicate-tie, superseded-by-put, no-audit-fullUpdate-loses, a losing invalidate, relocate or delete, and cache-resolve version-changed. Staging reads the same flag as "appended nothing" (`appendedTo`, below), so an unmarked early return also makes `replicatedConfirmation` wait on a log key nothing was written at (harper#3182). The transaction commit success paths (`DatabaseTransaction.commit` and `LMDBTransaction.commit`) walk writes and call `cleanupUnusedBlobs(write.savedBlobs)` for every still-skipped write. Cleanup is deferred (rather than run inline in the commit handler) because the commit handler runs again on optimistic-lock retries, and a retry can flip a previously-skipped write into a successful one (e.g. the existing record gets deleted between attempts so the older replicated update suddenly wins). Inline cleanup would race the deletion's `setTimeout` against the retry that referenced the blob. - `LMDBTransaction.abort` and `DatabaseTransaction.abort` walk all writes and run the same cleanup unconditionally (regardless of `skipped`), since nothing was committed. `DatabaseTransaction.commit` adds an explicit reject handler so a `Promise.all` failure on `completions` (e.g. a blob save errored) aborts the underlying transaction instead of leaking it _and_ the blob files. Both engines funnel synchronous throws and returned rejections from the whole commit chain through that cleanup once (`abortAfterCommitError`); LMDB captures `next` before its success path detaches the link, so a child's rejection still closes every uncommitted link. The cascade skips a link whose native commit is still outstanding — clearing its writes would unlink blobs that commit still references. **A blob instance outlives its file, so deletion tombstones it.** The `fileId` stays set on a `Blob` whose file has been unlinked, and `saveBlob` short-circuits on a set `fileId` — so a caller still holding the instance (the deploy recorder re-puts the same record object across a deploy) would re-encode a reference to a file that is gone. `cleanupUnusedBlobs` marks the blob's shared file state when transaction cleanup decides the file is no longer usable, before an in-flight save settles; slices share that state with their source. Ordinary supersession keeps its retention and re-reference window; reclamation marks every queued instance only after it claims the unlink. `saveBlob` throws on a discarded instance rather than minting a second reference (issue #2062). @@ -923,7 +923,7 @@ confirmable writes (`residentPeerLists`); the waiter needs the full count from e residency adds no list. The position is the committed transaction's reserved log key (`confirmationTarget`), not a record version, which can sit below the key (an explicit timestamp under the origin floor) or above it (a remote-origin version kept by a merge). Only when no confirmable write appended to the committed handle -(every write lost to a newer entry) does it fall back to the stored entry's version, as before. +(every write lost to a newer entry, which its commit handler marks `skipped`) does it fall back to the stored entry's version, as before. The commit handler stamps `write.residencyId` on each round that appends an audit entry and keeps it on a coordinated retry, which appends nothing, so the residents counted are the ones the replicated entry names. diff --git a/resources/Table.ts b/resources/Table.ts index 12731ccc09..11b7908c03 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -3981,7 +3981,11 @@ export function makeTable(options): TableResourceClass { const txnLogKey = isRocksDB ? (reservedLocalKey(transaction) ?? (context as any)?.timestamp ?? txnTime) : txnTime; - if (precedesExistingVersion(txnTime, existingEntry, options?.nodeId) < 0) return; + write.skipped = false; + if (precedesExistingVersion(txnTime, existingEntry, options?.nodeId) < 0) { + write.skipped = true; + return; + } const residency = TableResource.getResidencyRecord(options.residencyId); let metadata = 0; let newRecord = null; diff --git a/unitTests/resources/residencyConfirmation.test.js b/unitTests/resources/residencyConfirmation.test.js index 224b0b06fc..ac6d0b849b 100644 --- a/unitTests/resources/residencyConfirmation.test.js +++ b/unitTests/resources/residencyConfirmation.test.js @@ -56,6 +56,11 @@ describe('replicatedConfirmation residency (harper-pro#1031)', () => { const confirmed = (context, write) => transaction(context, write); const sorted = (lists) => lists.map((list) => [...list].sort()).sort(); + const logKeyOf = (Table, id, type) => { + for (const entry of Table.primaryStore.rootStore.auditStore.getRange({ start: 1, log: 0 })) + if (entry.tableId === Table.tableId && entry.recordId === id && entry.type === type) return entry.txnLogKey; + assert.fail(`no ${type} audit entry for ${id}`); + }; it('passes the other residents of a record placed by a residency list', async () => { await confirmed({ replicatedConfirmation: 1 }, () => @@ -120,6 +125,55 @@ describe('replicatedConfirmation residency (harper-pro#1031)', () => { assert.equal(calls[0].version, version); }); + it('confirms a commit whose only write is a relocation that lost to a newer entry on that entry (harper#3182)', async () => { + await Unplaced.put('relocated', { id: 'relocated', body: 'kept' }); + const { version } = Unplaced.primaryStore.getEntry('relocated'); + const context = { replicatedConfirmation: 1, timestamp: version - 60_000 }; + await confirmed(context, async () => { + const resource = await Unplaced.getResource('relocated', context); + resource._writeRelocate('relocated', {}); + }); + assert.equal((await Unplaced.get('relocated')).body, 'kept'); + assert.equal(Unplaced.primaryStore.getEntry('relocated').version, version); + assert.equal(calls.length, 1); + assert.equal(calls[0].version, version); + }); + + it('confirms on the log key when a losing relocation shares the commit with an applied write', async () => { + await Unplaced.put('relocated-mixed', { id: 'relocated-mixed', body: 'kept' }); + const { version } = Unplaced.primaryStore.getEntry('relocated-mixed'); + const context = { replicatedConfirmation: 1, timestamp: version - 60_000 }; + await confirmed(context, async () => { + const resource = await Unplaced.getResource('relocated-mixed', context); + resource._writeRelocate('relocated-mixed', {}); + await Unplaced.put('relocated-mixed-new', { id: 'relocated-mixed-new' }); + }); + assert.equal(calls.length, 1); + assert.equal(calls[0].version, logKeyOf(Unplaced, 'relocated-mixed-new', 'put')); + }); + + it('confirms a relocation that lost when staged but applied when re-staged past an open iterator', async () => { + await Unplaced.put('relocated-evicted', { id: 'relocated-evicted', body: 'kept' }); + const { version, value } = Unplaced.primaryStore.getEntry('relocated-evicted'); + const context = { replicatedConfirmation: 1, timestamp: version - 60_000 }; + let iterator; + try { + await confirmed(context, async () => { + // the open iterator makes the commit re-stage every write on a fresh transaction + iterator = Unplaced.search({ conditions: [] })[Symbol.iterator](); + iterator.next(); + const resource = await Unplaced.getResource('relocated-evicted', context); + resource._writeRelocate('relocated-evicted', {}); + await Unplaced.evict('relocated-evicted', value, version); + }); + } finally { + iterator?.return?.(); + } + assert.equal(Unplaced.primaryStore.getEntry('relocated-evicted'), undefined); + assert.equal(calls.length, 1); + assert.equal(calls[0].version, logKeyOf(Unplaced, 'relocated-evicted', 'relocate')); + }); + it('keeps a count-based placement at its count when a record is updated', async () => { Records.setResidency(undefined); const previousNodes = global.server.nodes;