Skip to content

Distinguish replication confirmation failure from a local write failure - #3187

Merged
kriszyp merged 14 commits into
mainfrom
fix/replication-unconfirmed-commit
Oct 20, 2026
Merged

kriszyp merged 14 commits into
mainfrom
fix/replication-unconfirmed-commit

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

⊙ 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.

❓ Your call: As approved by @kriszyp on October 12, expose replicationUnconfirmed: true in programmatic and REST/operations errors and finish the linked local commits before reporting confirmation failure. Keep the authorization change in #3084 separate, and keep both changes on main without a backport.

💡 Solution

Confirmation-hook errors now carry replicationUnconfirmed: true after the linked local commits complete. Their HTTP status, message and public error code are preserved; cause retains the original error, and retryable is 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

❓ Your call: Planning review returned Framing-Verdict: better-alternative-exists; I adopted its sequencing alternative. Await local completions first, then confirmation, instead of allSettled over everything. That would make a local failure wait for a peer timeout, possibly forever. A child's confirmation failure is deferred until the parent's other local completions settle, then reported without waiting on an unrelated stalled parent waiter.

  • Adding the flag only in the serializers cannot tell a native failure from a confirmation failure and leaves programmatic callers unprotected.
  • Pre-submission feasibility checks already refuse impossible counts, but cannot prevent peer timeouts after submission.
  • Mutating the hook's error can contaminate a shared error later reused by a local operation, and fails on frozen errors. The wrapper retains its cause and public wire fields.

❓ Your call: Confirmation coverage is unchanged. This PR does not propagate the count to later databases, pick up a context change after transaction() starts, or register harper-pro's missing LMDB confirmation hook. LMDB tests explicitly register its hook and set the count on the actual write-bearing child. The flag means committed locally, with the same durability as a successful response; it adds no peer or fsync guarantee.

❓ Your call: The flag attests to the reported commit’s issued writes. It cannot attest to later application steps that never ran after the rejection. A nested flag can remain when the enclosing scope has neither pending nor previously discarded writes; the same limitation applies to an explicitly awaited commit inside a callback. Confirmation counts remain root-only in production; manually setting a later link’s private count beneath an unconfirmed root is outside the supported producer scope and is left with the count-propagation follow-up.

❓ Your call: Review-depth checkpoint after three full passes: continue with the demonstrated JSON metadata-getter guard and documentation repair. The first three full review passes took about 90 minutes and found the ordering/scope/serializer defects listed below; diff growth is chiefly real-store regression coverage and stream symmetry. Keep the recorded scope and wrapper choices; the last repair adds no state owner. A downstream harper-pro identity audit was not done in this Harper-only worktree, so callers must use the flag and inspect cause for the original error.

🔧 Changes

DatabaseTransaction.ts:510

const localCompletions = completions.map((completion) =>
  Promise.resolve(completion).catch((error) => {
    if (error instanceof ReplicationConfirmationError) deferredConfirmationError ??= error;
    else throw error;
  })
);
// A confirmation failure must not abort a chained local commit that has yet to land.
return Promise.all(localCompletions).then(() => {
  if (deferredConfirmationError) throw deferredConfirmationError;
  return confirmation;
});
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.all path. 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

  1. Start confirmation — Invoke and observe the hook immediately after native commit.
  2. Wait for local work — Finish linked commits and flushes, deferring confirmation failure while preserving prompt local failures.
  3. Report the outcome — Await confirmation or report the deferred child confirmation failure.

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 ReplicationConfirmationError for programmatic callers, retaining the original error in cause and 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 and retryable: false as 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 ReplicationConfirmationError and inspect cause for 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, preserving retryable: false; stripped object details also retain the public code when their details omit it. Ordinary array errors retain their shape.

❓ Your call: A fault while constructing confirmation targets after native submission remains a pre-existing unmarked failure path outside this hook-outcome change. The design note is specific to confirmation-hook failures; this PR does not assert that every post-commit internal fault has the new contract.

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.

❓ Your call: The required round-five framing recheck returned Framing-Verdict: chosen-approach-sound. Continue with total best-effort diagnostic reads, guarded HTTP extensions and a structured-body fallback; commit ordering, count coverage and state ownership stay unchanged. The final accessor cases modify the rejected inner wrapper in the application callback, rather than relying on unsafe properties surviving constructor snapshotting. Existing HTTP base diagnostic reads remain a separate pre-existing limitation.

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 commit ac4ccb5f5 without the serializer repair, then pass with it. Four JSON terminal metadata getter cases fail at 1207e75ce with malformed JSON, then pass with the guard. Nine metadata wrapping/rollback cases fail on b64a43321 on both engines, then pass after the repair. The final modified-wrapper/coercion/HTTP cases fail at 7197132c2: 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_MISSING for systemSchema.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.

❓ Your call: Independent review covers the rebased transaction lifecycle, rollback history and final documentation. The closing full review is complete; the final documentation delta converged. The docs companion is dispatched separately and will merge after both Harper PRs.

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

🤔 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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread utility/errors/hdbError.ts Resolved
kriszyp and others added 9 commits October 20, 2026 08:14
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>
kriszyp and others added 2 commits October 20, 2026 08:41
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>
kriszyp and others added 2 commits October 20, 2026 11:41
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
Co-Authored-By: OpenAI Codex GPT-6.1 <noreply@openai.com>
@kriszyp
kriszyp force-pushed the fix/replication-unconfirmed-commit branch from ddbb3b8 to 2913e3c Compare October 20, 2026 18:15
@kriszyp
kriszyp marked this pull request as ready for review October 20, 2026 22:02
@kriszyp
kriszyp merged commit 9d8e590 into main Oct 20, 2026
55 of 57 checks passed
@kriszyp
kriszyp deleted the fix/replication-unconfirmed-commit branch 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REST honors a client-supplied X-Replicate-To (confirm count and residency) with no authorization check

1 participant