Skip to content

DatabaseTransaction has no supported way to extend the open-transaction budget for one known-long write (LMDB gap + fragile workaround) #2057

Description

@kriszyp

Summary

DatabaseTransaction/LMDBTransaction's open-time budget (.timeout) has no supported way
for a caller to extend it for one known-long write — a caller has to reach in and mutate
.timeout directly, and that mutation is fragile and doesn't propagate to every link that
actually needs it. This was found during cross-model review of
fix/deploy-ingest-txn-timeout (harper-large-deploy-txn-abort — the Large Deploy Payload Test
regression), which needed exactly this capability for the deploy payload-ingest write, and
worked around the gap in components/deploymentRecorder.ts's withExtendedTransactionTimeout.
Two independent review lenses (Codex, an internal Harper-domain pass) and a third experimental
lens (Grok) all converged on the same root cause independently.

The gap

  1. LMDB: the extension lands on the wrong object. resources/transaction.ts's
    transaction() returns the DatabaseTransaction it creates
    (resources/transaction.ts:39). The actual write resolves its transaction through
    txnForContext (resources/Table.ts:5489), which reuses that head object only when
    isRocksDB (resources/Table.ts:5492); for an LMDB-backed store it instead allocates a
    chained new LMDBTransaction() on .next (resources/Table.ts:5505). The chained
    object has its own .timeout, its own getReadTxn() (resources/LMDBTransaction.ts:38),
    and its own trackedTxns set (resources/LMDBTransaction.ts:16) — the long-transaction
    monitor never looks at the head's .timeout. Concretely: on HARPER_STORAGE_ENGINE=lmdb,
    withExtendedTransactionTimeout's 10-minute budget sits on an object nobody aborts, while
    the LMDBTransaction that actually stages the row counts down from the generic default and
    is aborted after ~30-60s — the exact "Transaction was open too long" failure the caller was
    trying to avoid, with the workaround in place and its tests green. The same shape can reach
    RocksDB too, whenever the write lands on a .next link because a different database was
    already written earlier in the same transaction chain.

  2. The workaround (assign-after-call) is a coincidence, not an invariant. getReadTxn()
    unconditionally does this.timeout = txnExpiration (resources/DatabaseTransaction.ts:362,
    resources/LMDBTransaction.ts:41). Setting the budget after calling the write (rather
    than before) only survives today because the write's pre-commit read happens to be
    synchronous for the common case — _loadRecord's synchronous snapshot
    (resources/Table.ts:1003-1004) and _writeUpdate's this.#entry ?? short-circuit for an
    existing row (resources/Table.ts:2188). Concrete break: if the row is absent when the
    write runs (concurrently deleted, or a future refactor that reorders row creation),
    loadLocalRecord takes an async branch (resources/Table.ts:5296) and getReadTxn() fires
    after the assignment — the budget silently reverts to the generic default and a large
    write aborts, invisibly to any test that only exercises the arithmetic.

Proposed root-cause fix

Give DatabaseTransaction (and LMDBTransaction) a timeoutBudget field, defaulting to the
current global txnExpiration:

  • Both engines' getReadTxn() reset .timeout to this.timeoutBudget instead of the global
    constant, so a caller's extended budget survives every subsequent read, not just the first
    one, and survives regardless of read timing (sync or async).
  • txnForContext propagates timeoutBudget down the .next chain the same way it already
    propagates sourceApply/isReplay (resources/Table.ts:5508), so an LMDB-chained
    transaction inherits the caller's budget instead of defaulting to the generic one.
  • A caller then sets the budget once, before the write, on whichever transaction object it
    has a handle to, and it holds for every link and every subsequent read — no ordering
    gotchas, no per-engine special-casing at the call site.

Where this currently matters

components/deploymentRecorder.ts's withExtendedTransactionTimeout (added by
fix/deploy-ingest-txn-timeout) works correctly today for the actual regression it fixes
(RocksDB, the default and only engine the CI test exercises, single-database write, existing
row after create()). It is a known, documented gap for HARPER_STORAGE_ENGINE=lmdb
deployments and for any future caller in a similar situation — DESIGN.md's "Extending the
budget for one known-long write" section documents the workaround and this gap so the next
caller doesn't have to rediscover it. This issue tracks doing it properly at the
DatabaseTransaction/LMDBTransaction/txnForContext level so future callers don't need a
per-call workaround at all.

Related, lower-priority observation from the same review

After a timeout abort, Resource.ts's dispatch deliberately joins the poisoned transaction
so the write fails correctly (by design, for issue #1407's atomicity guarantee) — but this
means the deploy row's terminal finish('failed', ...) write also throws on the same
poisoned transaction, so hdb_deployment.status never leaves pending for a deploy that
definitively failed by timeout. This is pre-existing for any timeout-abort inside an operation
handler, not introduced by fix/deploy-ingest-txn-timeout, but that PR makes a too-large-to-fit
deploy's timeout abort the expected way it ends, so it's a more likely path than before.
Probably belongs in its own issue once someone picks this one up — noting it here so it isn't
lost.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Fields

    Priority

    P2

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions