Repository navigation
Count only a record's residents toward replicatedConfirmation, and refuse a count they cannot meet before writing - #3148
Merged
Conversation
… and fail a count no residents can meet before committing A peer acknowledges a database-wide applied position, so a non-resident peer that received only an invalidation stub (or nothing) for a record was counted as confirming it. The commit now hands the confirmation callback one list of other residents per distinct residency among its confirmable writes; harper-pro's waiter counts a peer only for the lists it belongs to. A count larger than a record's other residents fails before the native commit. The commit handler records the residency and version of the audit entry it appended, kept across coordinated retries, and a write that stored no local record is now confirmed on that version. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…al from skipping commit bookkeeping Pre-push review: the refusal ran in save() after the write had staged, so a caller that caught it could still commit the write; it now runs in the write's commit handler before updateRecord, with the committing transaction passed to the handler. A refusal or callback error after the native commit becomes a rejected completion instead of a synchronous throw, so blob cleanup and lock/context release still run. Residents per residency id are cached (the id-to-list mapping never changes), and only writes placed by residency carry the stamped metadata, set after the audit entry is staged. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
The refusal ran in writeCommit, after updateIndices had already staged the write's index entries, so a caller that caught it could commit a dangling index entry. It now runs right after the write's residency is resolved, before anything is staged, and marks the write skipped so its saved blobs are cleaned up. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…red entry's An update whose residency omits the writer appends its audit entry without replacing the local entry, so the entry still held an older version and a resident's acknowledgement of that older version confirmed the new write. The confirmation target now prefers the version the write recorded. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…s inert, and confirm on the log position Pre-push review round 2: - confirmation requested after a write was saved is checked before native submission and aborts the transaction, so the refused write never lands; - a refused write is made inert (a retry re-save stages nothing; confirmation ignores it), so a caught refusal can neither commit it nor lend its older entry's version to the target; - the recorded position is the later of the record version and the log key, since peers acknowledge log positions, and is recorded only for audit-producing, non-copy writes; - a residency id whose list is not yet published is not cached as empty; - a post-commit refusal's rejected completion is handled at creation. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
… first one found Pre-push review round 3: the target kept the first defined value scanning back from the last write, so a trailing unplaced write's entry version (the record version) masked an earlier placed write's log key, which is higher when an explicit timestamp falls below the origin floor. The target now covers every recorded position. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…onfirmation refusal Pre-push review round 4: getResidencyId published the set-to-id mapping before the id-to-list one, so a worker discovering a just-minted id could find no list and refuse a valid confirmation; the list is now written first. Harper's BigInt.toJSON throws, so the refusal formats keys itself. The DESIGN note now bounds the refusal's guarantee to Harper's stored data: a table source's before hook runs first. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
Pre-push review round 4 (pre-existing gap): an unplaced write with an explicit timestamp below the origin floor was confirmed on its record version, which peers' acknowledged log positions can already exceed. Writes of a transaction that requests confirmation now record their position too; other writes pay nothing. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…n the committed log key Pre-push review round 5: - getResidency built a count-based update's placement as [self, ...previousResidency], and the stored list already starts with the writer, so each update repeated it and shed a replica ([S,A,B] became [S,S,A], then [S,S,S]); the new refusal turned that into a 400. The carried-over list now excludes this node. - The confirmation target is floored at the committed transaction's reserved log key, which covers delete-only, publish-only and late-confirmed writes whose record version sits below it. - getResidencyId writes both mappings with putSync, list first. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
… the replication layer refuse before commit Framing recheck (better-alternative-exists), adopted: - The confirmation target is the committed transaction's reserved log key, the position peers acknowledge. A record version can sit below it (explicit timestamp under the origin floor) or above it (a remote-origin version kept by a merge, which residents would never acknowledge), so per-write position stamps are gone. - getResidencyId never claims an id already in use, since readers cache an id's list for good. - replicationConfirmation takes an optional feasibility check, run before native submission with the resident lists, so the replication layer's deterministic refusals (recipient bound) no longer arrive after the write landed. - Count-based placement carries the previous residency over with a loop instead of filter+slice. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
Run inside the full resources suite, the 'test' database resolves under another name. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…ep a no-op commit off its unused log key Pre-push review round 6: - A commit made while read iterators are open replays its writes onto a fresh handle and submits that; the pre-submission feasibility check now runs on both submission paths (refuseUnconfirmable). - A commit whose writes all lost to newer entries appended nothing at its reserved key, which peers pass only when some later write replicates; such a commit falls back to the stored-entry target. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…nd the residents cache Framing recheck (better-alternative-exists), adopted: - getResidencyId claims a new id in one dbis transaction (retryOnBusy on RocksDB; LMDB's sync transaction is exclusive), so two workers minting the same set or drawing the same candidate id conflict and retry instead of overwriting a list readers have cached. - A delete that loses to a newer entry appends nothing; it is now marked skipped, so a commit made only of such writes confirms on the existing entry rather than on a log key nothing was written at. - The per-database residents cache is cleared past 10,000 residencies. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…feasibility check when nothing replicates Framing recheck (better-alternative-exists), adopted: a delete on a table with neither audit nor delete tracking removes the entry without an audit append, so it is marked skipReplicationConfirmation rather than sending the commit to wait on a log key nothing was written at; a commit made only of exempt writes (that, or a reload marker) no longer runs the replication layer's feasibility check. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…synchronous feasibility check, and reuse the checked lists Framing recheck (better-alternative-exists), adopted: - A write that appended its audit entry in an earlier round and lost on a retry round is skipped, yet its retained log entry still replicates at the reserved key; the target now keys off the sticky appendedAuditEntry flag rather than the final round's skipped flag. - A feasibility check that returns a promise is refused (TypeError) before submission instead of being silently left unawaited. - The resident lists checked before submission are kept for the waiter instead of rebuilt after commit. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…t, and keep a checked empty result Framing recheck (better-alternative-exists, local corrections), adopted: a feasibility check that returns a promise has it observed before the TypeError, so its rejection cannot go unhandled; a checked commit with no resident lists no longer rebuilds them after commit. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…the residency id claim, and freeze cached lists Pre-push review round 7: - The sticky appendedAuditEntry also covers an append on a handle an iterator replay discards, so the target now asks whether a confirmable write appended to the handle that was committed (appendedTo). - getResidencyId throws when the claiming transaction yields no id instead of placing the write as unplaced. - Cached resident lists are frozen, so a consumer cannot corrupt them for later commits. - The DESIGN note states when the target falls back to an entry version, and the bounded cache. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…ation targets the log key Pre-push review: appendedTo was recorded only when confirmation was already requested at save, so confirmation requested afterwards fell back to an entry version, which for an omitted-writer update is the older entry it left behind. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements changes to ensure that replicatedConfirmation counts only the actual residents of each written record. It introduces caching of residents by residency ID, adds a feasibility check before native transaction submission to refuse unconfirmable writes early, and updates the transaction commit process to track the exact log keys and resident peer lists. A high-severity issue was identified in DatabaseTransaction.ts where accessing this.writes[0] directly could throw a TypeError if the first write was detached; it is recommended to safely find the first non-null write instead.
…he transaction A detached first write leaves this.writes[0] null, which skipped the check. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
…ignore residency from a discarded replay round Pre-push review: - A coordinated or backoff retry re-enters the submission path after the first attempt's batch was written; re-running the feasibility check there could abort a submitted transaction. It now runs only before the first native submission of a commit. - Resident lists count only writes that appended to the handle being submitted, so a residency recorded in a round an iterator replay discarded is not required. - A delete resets its confirmation exemption each round, since a replay can now append. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
… handle Pre-push review: skipping the check whenever the transaction had already submitted also skipped it for a write that commits on its own fresh handle while the outer commit is in flight, and that write's waiter reused the outer commit's lists. The skip now applies only to a retry of the same handle (isRetry), and the checked lists are used only for the handle they were checked for. A delete's exemption reset skips retry rounds, which re-append nothing. Refs HarperFast/harper-pro#1031 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AtkQzQLoohXK3gEMzGFRz8 Dispatch-Task: hp-1031-residency-aware-confirmation
kriszyp
added a commit
that referenced
this pull request
Oct 20, 2026
The replayed branch carried nested conflict markers in DatabaseTransaction.ts and DESIGN.md. This commit sets the tree to #3266's squash (30bde37) applied to v5.3, adapted where #3266 edits code that only main has: - v5.3 has no origin-closed floor (#3085): drop the releaseLocalKey, reserveLocalKey, reservedLocalKey and transferLocalKey calls, the remoteOrigin branch, and the committedTxnKey fallback for the returned txnTime; keep v5.3's 4-argument commit handler call. #3266's try/catch in save() is kept, since its immediate-commit cleanup (detachWrite, drainCompletions, aborting the unsubmitted handle) is #3266's own. - v5.3 has no residency confirmation (#3148): confirmationTarget, residentPeerLists, refuseUnconfirmable and the residencyId/appendedTo fields stay absent. #3266's filter that skips unsaved deferred writes moves to lastConfirmableEntry(true) on the DatabaseTransaction commit path; LMDBTransaction, which never sets saved, keeps the unfiltered call. - DESIGN.md keeps v5.3's skipped-write bullet (no #3182) and takes #3266's abort bullet. - Table.ts drops the primaryKeyAttribute reset the replay added: it is #3244, which v5.3 does not have, so the net Table.ts change is #3266's unlock() JSDoc only. - noAbortAfterTxnLog.test.js stops importing originClosedFloor: the explicit-timestamp retry test keeps its assertions without the floor, and the two local-key-reservation assertions go. Dispatch-Task: cherry-resolve-kriszyp_harper_3266-96e295a4 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJYcrDZ7y79TEuwnztRn9z
kriszyp
added a commit
that referenced
this pull request
Oct 21, 2026
…he transaction log (conflicts → v5.3) (#3309) * Never abort a transaction once durably written to the transaction log Once a transaction or any link in a multi-store chain has durably written its entry to the RocksDB transaction log (at the first native commit attempt, where hasStagedLog and isRetry are set), it must be carried out unconditionally across conflict retries so the primary store and the audit log cannot diverge. - Add hasDurableLog(transaction) and DatabaseTransaction.hasLoggedChainEntry() to query durable log state across single-store and multi-store chains. - Exempt durably logged transactions from request-path retry caps (MAX_RETRIES on ERR_BUSY/ERR_TRY_AGAIN and RETRY_NOW) and commit-conflict deadlines, logging periodic / one-shot overdue diagnostics (via budgetCarrier so unchained ImmediateTransaction saves track per-handle clocks) while continuing to retry until the commit lands. - Retain record-lock holds (retainForCommit(holdDeadlineMono) / releaseCommitHold) on the native transaction across commit attempts and conflict retries, bounded by the commit-conflict budget so a wedged native commit or logged retry loop cannot hold a key forever, and release them as soon as the local store commit lands before chained stores or replicationConfirmation settle. - Scope ImmediateTransaction writes, retry-round re-staging, lock-hold retention, and abort cleanup to each write's native transaction so a post-lease sibling write on another key cannot abort an in-flight logged retry or re-stage an in-flight sibling write. - Keep logged ERR_BUSY backoff windows registered in outstandingCommits with per-attempt / per-backoff-window start timestamps so suspendDatabaseCommits() drains across backoff sleeps without tripping 503 load shedding on unrelated databases. - Add regression coverage for every post-log retry path and preserve pre-first-submit abort coverage. Fixes #3236 Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Clarify retainForCommit post-expiry deferral bound in DESIGN.md Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * fix(resources): isolate per-handle ImmediateTransaction state and cover terminal post-log failure Address Round 3 review findings: - Isolate ImmediateTransaction timestamp and retry state per native handle (`52469d122ee0`): - Track conflict retry count on `transaction.retries` (`RocksTransactionWithRetry`) alongside `this.retries` and reset `this.timestamp = 0` and `this.retries = 0` unconditionally when any handle commits or aborts, as well as immediately after staging `txnTime` on an unpinned `ImmediateTransaction`. - Keep `this.validated` synchronized with `this.writes` in `clearWrites(onlyForTransaction)`. - Add a regression test verifying that sequential plain writes `put(A, 1, ctx)` then `put(A, 2, ctx)` sharing an `ImmediateTransaction` while a sibling write `B` is parked in native commit each mint distinct monotonically increasing timestamps instead of tying and dropping `A=2`. - Add test coverage in Section 7 of `unitTests/resources/noAbortAfterTxnLog.test.js` driving the real `performCommit` terminal non-conflict catch branch (`aborting transaction after failed commit`) on a handle whose transaction log entry has already flushed (`f933af77479c`). - Fix JSDoc placement on `budgetCarrier` and `retainTransactionLockHolds` and tighten the `harper#3236` invariant note in `resources/DESIGN.md` (`cd730e81a041`, `bc74f1185a19`, `57308daa4e45`). - Document in `resources/DESIGN.md`, #3236, and #3253 the explicit #3236 maintainer design decision (@kriszyp) to keep the caller awaiting an uncapped retry once logged (matching `sourceApply`, with no 503-pending or detached background commit), while native `rocksdb-js` changes (validating conflicts before `executeLogWork` so conflicting first attempts on audited tables do not flush the log batch, exposing `committedPosition > 0`, and refusing/reconciling post-log aborts) are tracked in #3253 (`9dcabe0ba98a`, `e0c5e60a1aaf`, `07a62c6fd60d`, `20be4feb68b1`). Refs #3236 Refs #3253 Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Guard retry re-staging and preserve explicit timestamps on reused ImmediateTransactions - Key the `performCommit` retry re-save condition on the native handle's `options.transaction.isRetry` rather than `this.retries > 0`, and preserve writes with `stagedOn` in `clearWrites` / `noteCommittedLockVersions` / synchronous blob cleanup whenever `commitsInFlight > 1` even if an outer no-handle `performCommit` finishes synchronously while an `innerCommit` is still in flight. - Track `explicitTimestamp` on `DatabaseTransaction` and re-apply `context.timestamp` in `txnForContext` when reusing an `ImmediateTransaction`, so clearing `this.timestamp = 0` after staging does not clobber an explicit `transaction.timestamp` or `context.timestamp` across sequential or concurrent writes. - Release `releaseTransactionLockHolds(transaction)` on the synchronous throw path of `performCommit`. - Document the `hasStagedLog && isRetry` durable-log approximation in `resources/DESIGN.md` and `hasDurableLog()`, and strengthen tests in `unitTests/resources/noAbortAfterTxnLog.test.js` (also addressing 5b468774092c). Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * fix(resources): preserve explicit ImmediateTransaction timestamps and sibling blobs - Reset ImmediateTransaction.timestamp after save() only when save() minted the timestamp itself (mintedTimestamp = !txnTime), and reset this.timestamp at commit/abort settlement only once the last in-flight commit drains ((this.commitsInFlight ?? 0) <= 1), removing the shadowed prototype accessor on ImmediateTransaction. - Track lastContextTimestamp on DatabaseTransaction and only re-apply context.timestamp on ImmediateTransaction reuse while a commit is in flight or when context.timestamp changed, so post-settlement same-key rewrites mint a fresh version instead of tying at context.timestamp. - Preserve sibling writes' staged blobs during untargeted abort() when commitsInFlight > 1. - Update resources/DESIGN.md for ImmediateTransaction.save() retry forwarding. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Reset transaction timestamp when no sibling writes remain after clearWrites Gating `this.timestamp = 0` on `(this.commitsInFlight ?? 0) <= 1` skips the reset whenever a commit retries at least once, because recursive retry entries increment `commitsInFlight` before `performCommit` runs and decrement only as each outer promise unwinds. Gate the reset on `this.writes.length === 0` after `clearWrites(...)` instead, and only re-apply `context.timestamp` on an ImmediateTransaction when `context.timestamp` itself changes. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Address Round 8 review findings on logged commit retry and lock holds - Track active native commit handles in activeCommitHandles rather than relying on commitsInFlight > 1, which is inflated by recursive conflict retries. - Retain commit-phase lock holds immediately after transaction.commit() starts so synchronous throws from transaction.commit() cannot strand lock holds. - Propagate error.hasLog in the terminal commit catch block so rocksdb-js onAbort releases uncommittedPositions without deleting blobs of a durable log. - Document local transaction-log watermark pinning during overdue logged retries in DESIGN.md and the overdue warning log. - Add setMaxRetryDelayMs test seam to replace 42-second backoff waits in tests. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Apply context.timestamp when creating a chained store transaction link Ensure a new transaction.next link created in txnForContext adopts an explicit context.timestamp regardless of saveCommits, matching the root claim path. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Address plan and round-8 review findings for post-log commit retries - Revert Table.ts lastContextTimestamp change so it stays out of scope - Keep activeCommitHandles monomorphic and allocate only when saveCommits is true - Abort detached retry handles and release lock holds on pre-commit retry failures (C1) - Guard headTransaction.id and harperLogger.error in overdue/deadline logging (E1) - Add resetStuckCommitLogCooldownForTests and expand noAbortAfterTxnLog unit tests (C1-C5, replayLogs) Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Address plan review round 2 feedback (L1, A1, D1, N1) - Add jittered backoff (capped by MAX_RETRY_DELAY_MS and tracked via trackOutstandingCommit) on RETRY_NOW_VALUE after retries > 2 so symmetric crossing-writes transactions break lockstep (L1). - Guard abortNativeTransaction and abortAfterCommitError with committed / commitInFlight so an already-committed head handle is never aborted or logged as abandoned if a chained link rejects (A1). - Check typeof error?.hasLog === 'boolean' in hasDurableLog so pre-log rejections annotated by rocksdb-js return false directly (D1). - Trim narrative comments in DatabaseTransaction.ts and test suite (N1). - Add crossing-writes liveness test, LWW/patch outcome assertions, unsubmitted-holder monitor assertions, and chained head non-abort assertions. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * fix(resources): keep hasLog sticky across retries and revert Table.ts diff - Preserve hasLog monotonically across ERR_BUSY / ERR_TRY_AGAIN retry rounds with ||= so a later conflict carrying error.hasLog === false never retracts durability - Track activeCommitHandles and commitInFlight so untargeted abort() and clearWrites() protect in-flight and durably logged commits even when only a single commit handle is active on a shared ImmediateTransaction - Arm and clear holdTimer at the retainCount 0<->1 boundary and guard expireHoldDeadline against racing a settled commit - Revert resources/Table.ts to merge-base and track #saveOperation cross-store cleanup under #3253 Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * fix(resources): scope clearWrites scan and pre-submit abort handle tracking - Gate the clearWrites() write-ownership scan on (hasSiblingInFlight || !committedTransaction) so single-commit ImmediateTransactions skip the extra pass over writes while untargeted clearWrites() calls still protect active sibling commits. - Pass activeCommitHandles into isWriteOwnedByActiveCommit() in both clearWrites() and abort(), and capture the active native handle before abortAfterCommitError() and in performCommit's save/replay error paths so an audit:false sibling commit sleeping in ERR_BUSY backoff is not wiped by a failing sibling or pre-handle failure. - Document deferred-hold semantics on RecordLockHandle in resources/recordLock.ts. - Add regression test covering an audit:false sibling commit parked in ERR_BUSY backoff when another sibling exhausts retries or fails pre-handle in save(). Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * fix(resources): avoid per-commit Set allocation and protect outer retry from late commit abort - Replace unconditional activeCommitHandles Set allocation with single-handle activeCommitHandle fast path, allocating a Set only when concurrent commits overlap on the same DatabaseTransaction (~15.6k context-less Table.put/s, ~15.3k transaction Table.put/s over 100k ops). - Guard abort(), abortChainAfterRetries(), and performCommit catch paths when a sibling/outer commit is in flight so a late immediate commit failing after the outer transaction closed its scope only cleans up its own handle and writes without wiping the outer commit's retry state. - Replace fixed delay(1100) cooldown wait in commitConflictDeadline.test.js with resetStuckCommitLogCooldownForTests(). - Add unit tests for late immediate commit failure during outer request transaction retry and pre-commit completion failure. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Complete cleanup after abortAfterCommitError on logged retry and keep single-handle budget on ImmediateTransaction - Release retained lockHolds and clear writes/locks in abortAfterCommitError and abort() when a retry handle with hasDurableLog(retryTxn) is explicitly aborted after a pre-commit completion or re-staging error (treating abortedByWrapper/committed handles as inactive). - Keep commitStartedAt on the ImmediateTransaction instance for single-handle commits and promote to the native handle only when a sibling commit overlaps, avoiding an extra expando property on the native RocksTransaction handle in the common uncontended path. - Benchmark (audit: true, 20,000 sequential writes per run, 3 runs each): - origin/main (5ed056a): context-less Table.put 1,139 ms / 1,097 ms / 1,131 ms (median 17,679 ops/s); transaction(ctx => Table.put(..., ctx)) 1,434 ms / 1,202 ms / 1,200 ms (median 16,634 ops/s). - branch: context-less Table.put 1,119 ms / 1,127 ms / 1,211 ms (16,510–17,867 ops/s); transaction(ctx => Table.put(..., ctx)) 1,250 ms / 1,268 ms / 1,309 ms (15,275–16,003 ops/s). Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Avoid native handle expandos on single-handle commits and preserve unsaved deferred writes - Track single-handle commit state (activeCommitHandle, activeHandleStagedTxnTime, activeHandleInFlight, activeHandleCommitted) on DatabaseTransaction fields instead of attaching expando properties onto the C++ RocksTransaction wrapper in the common single-handle commit path, falling back to per-handle expandos only when multiple commit handles are concurrently active on an ImmediateTransaction. - Clear deferSave when ImmediateTransaction.save() enters and skip unsaved deferred writes (deferSave && !saved) in performCommit, abort, and clearWrites so a sibling holder's unsaved set() is neither swept nor dropped when another holder retries or settles. - Do not rotate a scope back to OPEN in completeMidScopeCommit after an explicit abort() closed the scope while a logged mid-scope commit was retrying. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Isolate deferred writes and concurrent closed writes across commit and abort * Isolate late immediate commits from rotated mid-scope segment 2 state * Preserve chained next on logged head abort and release locks after late immediate commits on CLOSED scopes * Isolate per-handle completions and cascade abort past late immediate siblings * fix(resources): handle sync commit throws, immediateCommit handle tracking, and deferred write unlock cleanup - Set handle commitInFlight after transaction.commit() returns so a synchronous throw on a retry or immediate handle leaves commitInFlight false and allows abortAfterCommitError to abort the detached handle and release its local key reservation. - Keep immediateCommit handles in activeCommitHandles rather than activeCommitHandle so single-handle fast-path checks never misclassify a late immediate handle after the main handle settles. - Detach unsaved, unstaged deferred writes when an ImmediateTransaction lock handle is unlocked or expires without save(). - Use declare for optional handle-tracking fields and avoid completionHandles/some() allocations on the single-commit fast path. Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * fix(resources): gate commitOutcome lock-hold wrapper and deduplicate detachWrite stagedIn cleanup - Gate commitOutcome.then lock-hold release wrapper in performCommit on hadLeaseProtectedWrite so non-locked commits return commitOutcome directly without an extra promise allocation. - Clear operation.stagedIn inside detachWrite so Table.ts:detachScopedUpgradeWrite on unlock() and ImmediateTransaction.releaseRecordLocks() on lease expiry share one detach path, and reset hasLeaseProtectedWrite when no locked writes remain. - Assert reservedLocalKey(txn) === undefined directly in synchronous commit throw tests on both retry and closed-transaction immediateCommit handles, and test expired lock lease deferred write cleanup on ImmediateTransaction. - Single-record audited put A/B microbenchmark (3 rounds x 10,000 iterations, each iteration calling await BenchTable.put({ id: 'bench-' + i, name: 'val', count: i }) with no context so txnForContext(undefined) allocates a fresh ImmediateTransaction per iteration): branch: 14,453-15,683 ops/sec (p50: 0.058-0.062 ms, p99: 0.161-0.197 ms) origin/main: 14,862-15,602 ops/sec (p50: 0.058 ms, p99: 0.151-0.222 ms) Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Gemini <noreply@google.com> * Keep clearWrites as single owner of expired deferred write cleanup (#3236) - Remove duplicate writes loop and latch reset from ImmediateTransaction.releaseRecordLocks() and redundant stagedIn pre-clears before detachWrite() - Document ImmediateTransaction.clearWrites() retention/detachment of unsaved deferred writes in resources/DESIGN.md - Bound mainCommitEntered wait and drive expired/unlocked deferred write cleanup test through the same ImmediateTransaction context - Single-record audited put A/B benchmark (3x10,000 puts with fresh ImmediateTransaction per iteration): 14,453-15,683 ops/s (p50 0.058-0.062ms, p99 0.161-0.197ms) vs origin/main 14,862-15,602 ops/s (p50 0.058ms, p99 0.151-0.222ms) * Leave a durably logged retry alone in the stalled-commit monitor and report deferred unlocks - deferForCommitInFlight() no longer poisons the context or logs the unknown-outcome restart error while a chain's handle with a durable log entry is still retrying a conflict; it still releases stalled read resources and defers the tick - KeyLockHandle.release() returns false while a commit hold defers the native unlock, so unlock() no longer resolves true for a key a contender cannot yet take - takeCompletions() returns a shared frozen empty array instead of allocating one per commit - the abandoned-entry diagnostic tests exercise the real native abort and assert its ERR_TRANSACTION_ABANDONED throw; the retry-exhaustion test counts only the retried handle's aborts; the unlock half of the deferred-write test is dropped (passes on main) - DESIGN.md states the invariant, its operational cost and the native follow-up as three bullets; record-locks.md drops private field names and states the unlock() contract Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Lap1gu2Gkw7hNkEVP9bUK * Report a logged retry whose counter stops advancing as the stall it is - deferForCommitInFlight() spares a chain with a durably logged retry only while its retry counter moved since the monitor's last tick (loggedRetriesSeen, reset with the attempt state); a native attempt that never settles is still poisoned and logged as before - DatabaseTransaction.activeDurableLogRetries() reports that counter; hasActiveDurableLogCommit() derives from it - the monitor test bounds its waits, releases its gates on failure, and drives three conflicting attempts so it covers both the advancing and the stopped case - issue tags and a restating comment removed from the retry path Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Lap1gu2Gkw7hNkEVP9bUK * Decide a logged retry's progress once per monitor tick, not once per visited link - deferForCommitInFlight() stores the retry-progress decision with the tick that made it, so the second link of a multi-store chain visited in the same tick reads that decision instead of comparing against the count the first visit just stored and flagging an advancing retry as stalled - resetStuckCommitLogCooldownForTests() also clears the stalled-commit warning and error cooldowns, so the monitor test's "must not log" assertions cannot pass vacuously - hasActiveDurableLogCommit() dropped; activeDurableLogRetries() is the only caller-facing form Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Lap1gu2Gkw7hNkEVP9bUK * Give a logged retry two native park timeouts before its stopped counter counts as a stall - deferForCommitInFlight() records when the logged retry counter last changed and treats an unchanged counter as a stall only after LOGGED_RETRY_STALL_MS (twice ROCKSDB_JS_PARK_TIMEOUT_MS), so a monitor tick shorter than a native park no longer poisons a retry that is merely parked - the wedged-commit lock test widens its hold budget from 350 ms to 1.5 s, so the 423 assertion is not racing the hold-deadline timer on a loaded runner Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Lap1gu2Gkw7hNkEVP9bUK * Harden the logged-retry stall window against a malformed park timeout and a moving maximum - LOGGED_RETRY_STALL_MS accepts only a finite positive ROCKSDB_JS_PARK_TIMEOUT_MS, falling back to rocksdb-js's 5000 ms default like the binding does, so a negative or infinite value cannot turn every logged retry past the deadline into a reported stall - the progress counter sums the live logged handles' retries across the chain instead of taking the maximum, so a maximum that moves to another handle and lands on the last seen value still reads as progress Dispatch-Task: harper-no-abort-after-txn-log Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Lap1gu2Gkw7hNkEVP9bUK * Restore the primaryKeyAttribute reset that replaying the branch dropped An earlier commit on this branch added `primaryKeyAttribute = undefined;` to updatedAttributes() and a later one reverted it. Main merged the identical line (#3244) in between, so replaying the revert onto main deleted main's line. Put it back; the branch's net change to Table.ts is the unlock() JSDoc only. Dispatch-Task: pr-maint-58cd7cf2b3da535511530a97eaf5d988 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Resolve the cherry-pick conflicts of #3266 onto v5.3 The replayed branch carried nested conflict markers in DatabaseTransaction.ts and DESIGN.md. This commit sets the tree to #3266's squash (30bde37) applied to v5.3, adapted where #3266 edits code that only main has: - v5.3 has no origin-closed floor (#3085): drop the releaseLocalKey, reserveLocalKey, reservedLocalKey and transferLocalKey calls, the remoteOrigin branch, and the committedTxnKey fallback for the returned txnTime; keep v5.3's 4-argument commit handler call. #3266's try/catch in save() is kept, since its immediate-commit cleanup (detachWrite, drainCompletions, aborting the unsubmitted handle) is #3266's own. - v5.3 has no residency confirmation (#3148): confirmationTarget, residentPeerLists, refuseUnconfirmable and the residencyId/appendedTo fields stay absent. #3266's filter that skips unsaved deferred writes moves to lastConfirmableEntry(true) on the DatabaseTransaction commit path; LMDBTransaction, which never sets saved, keeps the unfiltered call. - DESIGN.md keeps v5.3's skipped-write bullet (no #3182) and takes #3266's abort bullet. - Table.ts drops the primaryKeyAttribute reset the replay added: it is #3244, which v5.3 does not have, so the net Table.ts change is #3266's unlock() JSDoc only. - noAbortAfterTxnLog.test.js stops importing originClosedFloor: the explicit-timestamp retry test keeps its assertions without the floor, and the two local-key-reservation assertions go. Dispatch-Task: cherry-resolve-kriszyp_harper_3266-96e295a4 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJYcrDZ7y79TEuwnztRn9z --------- Co-authored-by: Kris Zyp <kriszyp@gmail.com> Co-authored-by: Gemini <noreply@google.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Oct 21, 2026
Cherry-pick of #3366, adapted to v5.3, which lacks two interfaces main's fix relies on: - committedBy: save() passes the DatabaseTransaction as the commit handler's 5th argument, as main does (#3148, v5.4). - appendedTo: ported; save() records the native handle that staged the write's audit entry (#3187, v5.4). - receivedLogKey/appendedLogKey: v5.3 has a single log key, so the duplicate identity check is txnLogKey === txnTime. (cherry picked from commit 1555252) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Oct 21, 2026
Cherry-pick of #3366, adapted to v5.3, which lacks two interfaces main's fix relies on: - committedBy: save() passes the DatabaseTransaction as the commit handler's 5th argument, as main does (#3148, v5.4). - appendedTo: ported; save() records the native handle that staged the write's audit entry (#3187, v5.4). - receivedLogKey/appendedLogKey: v5.3 has a single log key, so the duplicate identity check is txnLogKey === txnTime. (cherry picked from commit 1555252) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Oct 21, 2026
…3) (#3377) Cherry-pick of #3366, adapted to v5.3, which lacks two interfaces main's fix relies on: - committedBy: save() passes the DatabaseTransaction as the commit handler's 5th argument, as main does (#3148, v5.4). - appendedTo: ported; save() records the native handle that staged the write's audit entry (#3187, v5.4). - receivedLogKey/appendedLogKey: v5.3 has a single log key, so the duplicate identity check is txnLogKey === txnTime. (cherry picked from commit 1555252) Co-authored-by: Kris Zyp <kriszyp@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
⊙ Problem
replicatedConfirmation: Nresolves once N peers report an applied position for the database that is at or past the write. On a table with a residency list, that position is also passed by peers that do not hold the record: a non-resident peer receives only an invalidation stub, and a peer outside the list may receive nothing at all for that record. Either one still counts. A write can therefore report "confirmed by N" while no other node holds a complete copy (Count only peers that hold a complete copy toward replicatedConfirmation — harper-pro#1031). Core is the layer that knows each record's residency, but it never passed that to the replication layer.💡 Solution
The commit now gives the confirmation callback a 4th argument,
residentPeerLists: one list of other residents per distinct residency among the transaction's confirmable writes, with the writer and duplicate names removed. Harper-pro's waiter counts a peer only toward the lists it belongs to, and every list needs N confirmed members. A write with no residency adds no list, because N confirmations from any list already come from N distinct peers. When no write has a residency, the argument isundefinedand the waiter counts as before.setResidencyById, a write that excludes the writer stores no entry at all, so it was never confirmed.residencyIdof the audit entry each write appended. A coordinated retry appends nothing and keeps it, so the residents counted are the ones the replicated entry names. Residents are resolved once per residency id.getResidencyIdnow never claims an id already in use, and it writes the id-to-list mapping before the set-to-id one, so an id that is discoverable always resolves to its one list.skipReplicationConfirmation). A commit made only of exempt writes neither waits nor runs the feasibility check. A commit whose writes all lost to newer entries in every round falls back to the stored-entry target.Cannot confirm replication to 2 node(s) for record "x" in database "data": it is resident on 1 other node(s) (node-b). The refused write turns inert, so a caller that catches the error cannot commit it, even on a retry. Just before native submission, the commit rechecks the lists (confirmation may have been requested after a write was saved). It then runs a feasibility check the replication layer can now register withreplicationConfirmation(callback, feasibility). Harper-pro uses it for its recipient bound, which used to answer 400 for a write that had already landed. An error after the native commit becomes a rejected completion, so the commit's bookkeeping (blob cleanup, lock and context release) still runs.replicateTo: Nbuilt an update's placement as[self, ...previousResidency], and the stored list already starts with the writer. Each update therefore repeated the writer and shed a replica:[S,A,B]became[S,S,A], then[S,S,S]. This was a pre-existing data-placement bug, which the new refusal surfaced as a 400. The carried-over list now leaves this node out.⚖️ Alternatives
better-alternative-existsfive times; each was adopted, and none cleared. The review CLI required a recheck after round 5. Each verdict named a smaller or safer variant inside the same design, and I adopted every one: target the committed log key, not record versions; claim residency ids atomically (the resolved lists are cached); refuse through a pre-submission feasibility hook on both submission paths; exempt entry-less deletes and marker-only commits; key the target off the stickyappendedAuditEntry; refuse and observe an asynchronous feasibility check; reuse the checked lists. The last verdict said its points "do not require replacing the design", so I adopted them without a sixth round. The original plan review cleared withFraming-Verdict: chosen-approach-sound.residencyIdby then. The entry also does not exist when the writer stored no local record.logEntries. Rejected:RocksTransactionLogStorefills that array only when anaftercommitlistener exists. It also skips appends on a retry, so it can be absent or describe a different attempt.🔧 Changes
resources/DatabaseTransaction.tsconfirmationTargetpicks the committed log key, else the stored-entry fallback, deciding by whether a confirmable write appended to the committed handle (appendedTo).residentPeerListsandconfirmingResidentsbuild the per-residency lists. Writer and duplicates are removed, and the lists come from a bounded per-database cache of frozen lists.refuseUnconfirmableruns before both native submissions: the iterator-replay path and the ordinary path. It runs the feasibility check registered withreplicationConfirmation(callback, feasibility), and aborts on refusal.resources/Table.ts_writeUpdatecommit handler refuses an unmeetable count before staging and makes the refused write inert. It records the audit entry'sresidencyId, kept across coordinated retries.getResidencyIdclaims a new id and both mappings in one transaction.skipped, and an entry-less delete is exempt from confirmation.resources/DESIGN.mdgets the invariant note, indexed from the rootDESIGN.md.✅ Verification
unitTests/resources/residencyConfirmation.test.js(19 tests, RocksDB) covers:replicateTo, and mixed/duplicate residencies;The new tests discriminate: they fail on the base sources (7 of 8 originally) and on each superseded intermediate target.
npm run test:unit:resources: 4151 passing.npm run test:unit:main(isolated HOME): 6877 passing. UnderHARPER_STORAGE_ENGINE=lmdb,writerResidencyandreloadMarkerpass, covering the atomic id claim on LMDB.End to end: harper-pro's new three-node
integrationTests/cluster/residencyConfirmation.test.mjsagainst this core passes 3/3, and fails on base. The existing confirmation suitesfullyConnectedReplication,replicationTopology,nonReplicatedDatabase,selectiveTableSubscription,removeNodeBlastRadiusandreplicationWorkerPoolpass.After the PR opened, two more fixes landed. The pre-submission check now runs only before a handle's first submission: a retry of that same handle (
isRetry) is not re-checked, and a fresh handle committing while the outer commit is in flight is. Resident lists count only writes that appended to the submitted handle. The suites below were re-run on the final head.A pre-existing flake,
unitTests/security/userRecordLookups.test.js("onUserChange re-subscribes…"), fails about 1 in 3–6 runs on base too.Refs HarperFast/harper-pro#1031. Companion: harper-pro PR (bumps
coreto this branch). After merge, documentation#723 needs to describe this contract.🤖 Generated by Claude (Anthropic, Opus 5.5); posted via @kriszyp.
Related PRs: #2918 overlaps (DatabaseTransaction save() arguments and the pre-submission abort block), #3092 overlaps (Table source-hook ordering before the commit handler), 3 others independent
Complexity: complicated
Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex,cursor-muse; adjudicated=domain; declined=cursor-grok,cursor-kimi; rounds=11; full=5 @ a8c51af
Review-Attention: deep ~40m (critical: DatabaseTransaction.ts, Table.ts; decisions: do-less-alternative, caught-refusal-semantics, refuse-before-landing, sync-only-feasibility, lmdb-unchanged) @ a8c51af