Skip to content

save() adopts a read handle without the per-handle bookkeeping getReadTxn() establishes (readTxnsUsed becomes NaN) #2224

Description

@kriszyp

Summary

DatabaseTransaction.getReadTxn() establishes three pieces of per-handle bookkeeping together when it opens a read handle (resources/DatabaseTransaction.ts):

this.readTxnsUsed = 1;
this.baseReadRefConsumed = false;
if (DEBUG_LONG_TXNS) this.stackTraces = [new StartedTransaction()];
...
trackedTxns.add(this);

save() also assigns this.transaction — the branch that opens a handle for a write when the transaction has none — and establishes none of them. Anything that later reads through that adopted handle therefore operates on unseeded bookkeeping.

The stackTraces half of this surfaced as #2222 (a TypeError that 500'd every affected search() when storage.debugLongTransactions was on) and is guarded at the read site in #2225. The refcount half is still there, is independent of that flag, and is on main today.

The refcount half

useReadTxn() does this.readTxnsUsed++. On an adopted handle readTxnsUsed has never been assigned, so the increment is undefined++NaN, and every subsequent comparison against it is false:

const txn = new DatabaseTransaction();
txn.transaction = { openTimer: 0 };   // what save()'s adopt branch assigns
txn.useReadTxn();                     // readTxnsUsed: undefined -> NaN
txn.doneReadTxn();                    // --NaN === 0 is false; handle is never aborted

Observed directly (unit harness, 5.2.3): initial readTxnsUsed = undefinedafter useReadTxn = NaNafter doneReadTxn = NaN, transaction still held = true.

Consequences, all from NaN failing every > 0 / === 0 test:

  • doneReadTxn() never reaches zero, so the native read handle is never abort()ed by the iterator-drain path.
  • abort()'s while (this.readTxnsUsed > 0) this.doneReadTxn() exits immediately, so an aborting request leaves the read snapshot pinned until GC finalizes the handle — blocking compaction for that long.
  • The adopted handle is also never trackedTxns.added, so the long-transaction monitor never sees a write-first transaction at all: it can neither bound its snapshot nor abort it for exceeding maxTransactionOpenTime.

commit() is not affected: its readTxnsUsed-- then > 0 test also goes false on NaN, which takes the normal commit path.

Reachability

Latent as far as I could demonstrate — I could not construct a request that reaches the adopt branch:

  • put() reads the existing entry first, which runs getReadTxn()'s fresh branch and seeds readTxnsUsed = 1, so an ordinary write-then-search transaction is correctly counted (verified: readTxnsUsed = 1 after put, still 1 after a search).
  • After a search drains, doneReadTxn() decrements only to the base reference (1), never to 0, so this.transaction stays set and save() never adopts.

So the adopt branch needs a transaction that writes without any prior read, or one whose base reference was already consumed by a commit. Whether a real path does that (blind writes, publish, a post-commit write) is exactly what wants checking before deciding how to fix it.

Suggested direction

Seed the bookkeeping where the handle is adopted, so "handle exists ⇒ bookkeeping exists" holds however the handle arrived, instead of guarding each consumer. That is deliberately not a drive-by: it puts write-first transactions under the long-transaction monitor for the first time (they become abortable per #1407) and changes what doneReadTxn()'s refcount means for a handle whose base reference was never taken, so it wants an owner who can rule on both.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Fields

Priority

None yet

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions