Repository navigation
Distinguish replication confirmation failure from a local write failure - #3187
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to handle replication confirmation failures after local commits have already succeeded. It adds the ReplicationConfirmationError class and helpers like confirmReplicationAfterCommit and settleCommitCompletions to defer confirmation failures until local writes settle, ensuring that local commits are not aborted by replication timeouts. The changes propagate the replicationUnconfirmed and retryable: false flags to REST problem details, JSON streams, and server error handlers, and include comprehensive unit tests. The reviewer suggested improving the ReplicationConfirmationError constructor to safely handle primitive string rejections for the cause parameter.
This was referenced Oct 15, 2026
Wait for linked local commits before reporting replication confirmation errors. Preserve their HTTP status and code, expose replicationUnconfirmed in REST and operations responses, and prevent retryable hints from replaying committed writes. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Copy the original constructor name without relying on instanceof Error, and accept its existing status alias. Verify with a real foreign-realm error through the registered confirmation hook. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Preserve prompt local failures and the ordinary unconfirmed commit path. Strip foreign nested confirmation markers before rolling back pending writes; expose the nonretryable outcome in structured and streaming responses. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Preserve negative retry hints in REST, streams and structured array operations errors when the outer scope removes the inner commit flag. Define the flag at the reported commit boundary, and return the observed waiter directly when no local completions remain. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Guard extension getters on arbitrary iterator rejection reasons so terminal JSON remains valid. Document the terminal metadata and point confirmation-hook ordering to its native-store regression suite. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
…le metadata Guard error inspection before aborting, preserve the committed outcome when cause diagnostics throw, and drop unreadable structured details before removing a nested flag. Pin callback/source rollback and confirmation diagnostics against real stores on both engines. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Use guarded diagnostic reads when stripping a nested outcome, guard the new HTTP extensions, and retain the confirmed outcome when structured operations details cannot be copied. Verify real native writes and rollback, both HTTP serializers, non-string diagnostics, and the no-pending-write control. Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
This was referenced Oct 20, 2026
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
This was referenced Oct 20, 2026
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
kriszyp
force-pushed
the
fix/replication-unconfirmed-commit
branch
from
October 20, 2026 18:15
ddbb3b8 to
2913e3c
Compare
kriszyp
marked this pull request as ready for review
October 20, 2026 22:02
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 was referenced Oct 22, 2026
Closed
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
A replication confirmation can fail after a local write commits. The rejection previously looked like a write failure, and its fail-fast aggregation could abort a later database still waiting to commit. This is the commit-contract follow-up to #3084 and harper#2898.
💡 Solution
Confirmation-hook errors now carry
replicationUnconfirmed: trueafter the linked local commits complete. Their HTTP status, message and public error code are preserved;causeretains the original error, andretryableis false. Local write/flush failures remain prompt. When an enclosing rollback removes the confirmation flag, responses keep its negative retry hint. A missing flag does not mean nothing was written: independent databases can still fail partially.⚖️ Alternatives
🤔 Keep empty aborts free of a capturing rollback scan
Adopt the indexed-loop helper from the framing recheck. A length guard skips the callback but can retain its captured context; the helper removes both allocations while preserving ownership tests. Empty scopes skip the helper. This implementation choice is reversible.
Scope, compatibility and review decisions
🤔 Remember discarded writes on the existing scope
Keep discarded-write history on the existing root; all three funnels consult it after cleanup. Poison flags also cover commits that landed, while serializers miss programmatic callers. The latch records selected discarded writes; empty cleanup skips the ownership scan. Planning:
Framing-Verdict: chosen-approach-sound.Scope and other approaches
🔧 Changes
DatabaseTransaction.ts:510
Product and architecture tour
Product and architecture tour
Local completion comes first
What must finish before a confirmation failure reaches the caller?
DatabaseTransaction.ts wraps synchronous and asynchronous post-commit hook failures immediately, observing the rejection while local work is pending. The local completion wait separates confirmation from the existing local completion list, defers a child's marked failure, and keeps unconfirmed transactions, including linked databases, on their existing
Promise.allpath. LMDBTransaction.ts uses the same completion order, including on its optimistic and exclusive write paths; a synchronous hook throw now reaches bookkeeping before the failure surfaces.Commit and report the outcome
With no local completions left, the helper returns the already-observed confirmation promise directly. The ordering prevents a confirmation failure from reaching the abort funnel while a linked local write can still land. Inspect this boundary most closely: the native/validation/flush error paths remain separate from the confirmation outcome.
Error responses retain the distinction
How do callers distinguish this outcome and preserve the original error information?
hdbError.ts adds
ReplicationConfirmationErrorfor programmatic callers, retaining the original error incauseand its public status/code/message and structured details, including across JavaScript realms. Unreadable cause fields use ordinary fallbacks without replacing the committed outcome; unreadable structured details are omitted when stripping the flag for an outer rollback. REST.ts publishes the flag andretryable: falseas problem-details extensions. serverHandlers.js carries them through ordinary and structured operations error bodies while preserving the public error code. The structured fallback reads diagnostics independently, so unreadable details or modified wrapper message/code getters retain the outcome and readable fields. contentTypes.ts and JSONStream.ts add the same fields to terminal SSE, NDJSON and JSON stream records. SSE/NDJSON outcome fields remain readable even when status metadata throws.transaction.ts and both commit failure funnels guard outcome metadata reads so an arbitrary rejection cannot bypass abort, and remove an independent nested transaction’s flag when the enclosing scope has pending or previously discarded writes. That prevents an inner committed write from attesting to an outer rollback; the inner error remains available through
cause.The root’s rollback history survives write-set cleanup. RocksDB abort selection, terminal native failures and LMDB cleanup record discarded work before it can be erased. All three rejection funnels consult that history; committed or still-submitted owners and self-committing instances retain their own outcome.
🤔 Preserve shared errors through a wrapper
Use an independent wrapper to protect shared and frozen rejection objects. Programmatic and string consumers receive
ReplicationConfirmationErrorand inspectcausefor original identity; object responses retain the public code. The compatibility audit is tracked in #3331.Structured details and scope
Flagged array-shaped operations details gain an
{ error: [...], code, replicationUnconfirmed, retryable }envelope. Array details remain enveloped after an enclosing rollback, preservingretryable: false; stripped object details also retain the public code when their details omit it. Ordinary array errors retain their shape.The invariant and the unchanged confirmation scope are recorded in resources/DESIGN.md, indexed from DESIGN.md. server/DESIGN.md specifies the optional terminal-record extensions. contentTypes.test.js also checks that throwing metadata getters on sync/async iterator errors leave valid JSON, and unreadable status cannot hide SSE/NDJSON outcome fields.
The documentation companion covers #3084's super-user requirement and this error contract. @kriszyp dispatched it separately on October 20; it will merge after #3084 and #3187.
✅ Verification
Build, lint, formatting and design-doc checks pass. Full resources: 4,452 RocksDB / 3,370 LMDB; final focused suites: 144 / 142. Integration: 2,543 passes, six Ollama setup cancellations. The core suite fails deployment retry assertions that also fail on pinned main.
Executed gates, regression proofs, measurement and review limitations
Two limitations remain tracked. A manually counted child can keep a failed parent confirmation pending on its remote waiter (#3341); current public count producers do not create that dependency. A pre-existing LMDB cascade skip can strand writes queued after an unawaited batch lands while a flush or chained completion remains pending. In mixed-engine processes, a nested RocksDB confirmation error can still carry a flag through that scope. #3342 tracks the native batch/cascade ownership work; the rollback latch does not repair it.
🤔 Keep child-confirmation ordering
Retain the shared helper’s per-completion classification for explicitly counted child links. Its measured two-completion cost is 0.260 µs; removing it would narrow the tested ordering contract in both engines for a saving small beside a peer round trip. Production count propagation is unchanged.
Executed gates and regression proofs
measured: true — empty
abort(true)on Node 26.2.0, five warmed batches of 100,000 preallocated transactions. Median µs (range): baseline 0.461 (0.063–0.548), capturing scan 0.240 (0.189–0.549), guard 0.433 (0.313–0.644), indexed helper 0.375 (0.313–0.473); zero observed minor-GC events inside the timed batches. Constructors were outside timing. These noisy component results establish no throughput or end-to-end source-latency win; the indexed form removes the captured context by construction.measured: true — confirmation-aggregation component microbenchmark on Node 26.2.0, five interleaved batches of 20,000 successful aggregates: zero local completions 0.580 → 0.325 µs; two completions 1.014 → 1.274 µs (+0.260 µs). Uses the actual helpers and already-resolved promises; excludes storage, network and failure wrapping. Plain unconfirmed commits retain their existing aggregation.
replicationConfirmationOutcome.test.js uses real tables, real transactions and the production callback-registration API. It covers synchronous/async failure, frozen/primitive/shared reasons, unreadable cause fields, rollback cleanup with throwing metadata, successful confirmation, pre-submit refusal, held linked writes, prompt local failures with a stalled waiter, child-confirmation ordering, independent nested failures, explicitly awaited commits, terminal records in three streaming formats, and REST problem details. Both engines are exercised, including exclusive LMDB writes. Held confirmation gates cover callback/source timeout, request-signal disconnect and explicit abort, with REST output and own-commit controls. A real local commit failure is swallowed before a later independent confirmation failure; the outer scope keeps the negative retry hint and loses the flag.
serverErrorHandler.test.js drives real Fastify error routing and the actual operations insert dispatch, verifies the row committed, and covers ordinary/structured/array errors plus unchanged local errors. A stripped structured-object regression fails before the final public-code copy, then passes with it, retaining the code and negative retry hint without mutating the cause. The two production operations confirmation variants are skipped under LMDB, whose producer remains unregistered; direct LMDB commit-path tests cover the callback outcomes.
On base
f6cf04149, all three boundary regressions fail for the expected reasons: the held chain rejects early, and REST/operations omit the flag. They pass after restoring the fix. The five outer-rollback retry-hint cases also fail at reviewed commitac4ccb5f5without the serializer repair, then pass with it. Four JSON terminal metadata getter cases fail at1207e75cewith malformed JSON, then pass with the guard. Nine metadata wrapping/rollback cases fail onb64a43321on both engines, then pass after the repair. The final modified-wrapper/coercion/HTTP cases fail at7197132c2: 13 on RocksDB and 12 on LMDB, then pass; the no-pending-outer-write control preserves the original flag.Six final serializer regressions (terminal status regression and operations fallback regression, four and two cases respectively) fail at
453782e1c, then pass with independent guarded reads.Boundary route: real REST handler and real Fastify operations dispatch with real storage. Full process bootstrap, authentication middleware and actual peer acknowledgement are not established by these callback-outcome tests.
Rebased onto main
1fd591ba2, preserving its per-native-handle cleanup, timestamp/context lifecycle, durable-log no-abort rule, retry-handle cleanup and lock release before confirmation. The rebase preserves the prior runtime diff; final review then repaired a missing structured error-code copy and rollback history after an earlier abort. The combined tree with Require a super user for X-Replicate-To and the operations API replicateTo/replicatedConfirmation fields #3084 merges cleanly. The design-note whitespace fold preserves every word and fits the note budget both alone and combined.Current Node 26.2.0 gates: build, lint, full Prettier check and design-doc index pass. Full resources before the final structured-code and rollback-history repairs: 4,452 passing / 54 pending on RocksDB, 3,370 passing / 739 pending on LMDB. Final focused suites after the rollback-history repair: 144 RocksDB, 142 LMDB with four expected skips. The reused self-committing control is RocksDB-only: LMDB closes that failed instance on the unchanged baseline. The integration run before those final repairs reports 2,543 passing, zero failed tests, six Ollama setup cancellations, 18 skips and one todo. The setup exception is
ERR_IMPORT_ATTRIBUTE_MISSINGforsystemSchema.json, previously reproduced on base; the integration command exits nonzero.Full core is not green locally. An initial run was interrupted after deployment failures/timeouts and produced no epilogue. The normal fail-fast retry has 1,069 passes / 17 pending, then fails
deployActivation.test.js:957(expected one swap retry, observed zero). With all eight production files replaced by pinned main and rebuilt, the same run has 1,068 passes / 17 pending and fails the sibling move-aside retry assertion at line 924, also observing zero. Source was restored and rebuilt. This demonstrates baseline retry-timing failures; it does not establish the cause of every later timeout in the interrupted run. No component code or tests changed here.CI resource-core failed on Node 24 and 26 (run); Node 22 passed that step. The failures are not diagnosed: GitHub returned 404 for logs while the jobs continued. Integration and platform checks passed.
The real RocksDB swallowed-failure test fails when only the terminal-failure latch is removed, proving that it pins the asynchronous branch. The file was restored and rebuilt. The empty-read abort control passes on both engines. Broader transaction, monitor, durable-log and ownership suites pass: 227 RocksDB / 138 LMDB.
Closes #2898
🤖 Generated by OpenAI Codex; posted via @kriszyp.
Release target: v5.4 — main only; no backport, as requested by @kriszyp.
Related PRs: #3084 overlaps (replication controls), #2918 overlaps (commit/lock bookkeeping), #3248 overlaps (commit/lock bookkeeping), #3266 overlaps (durable-log protection already in base), #3229 overlaps (REST errors and LMDB commit path), #3126 overlaps (REST and stream error handling), 8 others independent
Complexity: medium
Deferred findings
d6a146f7ae8937e6major correctness —utility/errors/hdbError.ts— Confirmation wrapping changes rejection identity and constructor matching — filed Audit confirmation-error class and identity consumers for the new wrapper #33319c03bc90cc60188bmajor reliability —resources/DatabaseTransaction.ts— Parent confirmation failure waits for child remote confirmation — filed Separate child local completion before propagating replication confirmation counts #3341aeae1ffb130afd40major data-integrity —resources/DatabaseTransaction.ts— Submitted LMDB link can strand writes staged after its batch lands — filed Track later LMDB writes across a landed batch and skipped abort cascade #3342428d4c0261177c16minor performance —resources/DatabaseTransaction.ts— Confirmed commits classify local completions for explicitly confirmed child links — filed Evaluate confirmed-commit completion classification overhead if count coverage changes #3332bc5e3e943a0c8a1cminor documentation —resources/DESIGN.md— Public confirmation behavior changes lack an evidenced documentation companion — filed Track the dispatched replication-control and confirmation-outcome documentation companion #3333859074820e86872anit data-integrity —resources/LMDBTransaction.ts— Follow-up needs root-bypass and mixed-engine entry conditions — filed Track later LMDB writes across a landed batch and skipped abort cascade #3342🤔 defer this PR's major correctness finding
d6a146f7ae8937e6(Confirmation wrapping changes rejection identity and constructor matching) to #3331 instead of fixing it here?🤔 defer this PR's major reliability finding
9c03bc90cc60188b(Parent confirmation failure waits for child remote confirmation) to #3341 instead of fixing it here?Review-Coverage: authored=codex; ran=gemini,claude,cursor-muse; adjudicated=domain; blocked=muse(failed); declined=cursor-grok,cursor-composer,cursor-kimi; rounds=13; full=10 @ 2913e3c
Review-Attention: deep ~50m (critical: DatabaseTransaction.ts, LMDBTransaction.ts +1; hot: REST.ts; decisions: do-less-alternative, outcome-boundary, confirmation-wrapper, error-identity, scope-attestation, deferred-d6a146f7, deferred-9c03bc90; raised: degraded review) @ 2913e3c