Skip to content

A closed ImmediateTransaction is reused for the next write, and that write's commit promise is dropped #2323

Description

@kriszyp

Found while fixing #2292; independent of it, and present on main today.

What happens

ImmediateTransaction.save() is the commit trigger:

save(...args) {
    const transaction = args[0];
    if (this.isCommitting) {
        super.save(transaction, null, true);   // <- return value dropped
    } else {
        this.isCommitting = true;
        return when(this.commit(), () => { this.isCommitting = false; });
    }
}

Once such a transaction has committed, commit() has left it CLOSED and rotateAfterMidScopeCommit will not reopen it (it is not scopeOwned). It stays in context.transaction, so txnForContext hands it back for the next write to that database. On that write:

  1. addWritesave(operation)isCommitting === falsethis.commit().
  2. commit()'s save loop re-enters the override with isCommitting === true, so it calls super.save(operation, null, true).
  3. this.open !== OPEN, so that inner save() takes the immediateCommit branch (resources/DatabaseTransaction.ts) and returns this.commit({ …options, transaction }) — the promise that owns the native commit. The isCommitting branch discards it, and the commit loop discards it too.
  4. The outer commit() finds this.transaction unset (nothing was attached to a CLOSED instance), so commitResolution is never assigned and it returns a synchronous { txnTime }.

Consequence

await resource.save() (and await table.put(...) on that context) resolves before RocksDB has committed the write:

  • a crash in that window loses a write the handler already acknowledged;
  • a read-back inside the same handler can see the pre-write record;
  • if that commit rejects, nothing handles the rejection — it surfaces as an unhandledRejection in the worker while the client has already been sent 2xx.

The write itself normally lands, so this is a lost guarantee rather than a lost write in the common case, which is also why it is invisible.

Reaching it

Any second write to the same database through a context whose slot holds an ImmediateTransaction that has already committed once — i.e. an autocommit write path with no transaction() scope. txnForContext installs such an instance whenever the slot is empty or holds the released placeholder and something resolves a transaction without going through the static-API wrappers (an instance load is the usual route).

Candidate fix

Propagate the promise instead of dropping it: return super.save(...) from the isCommitting branch, and have commit()'s save loop stageCompletion() a thenable result so the outer commit awaits it. Both are small, but they are in the commit path every autocommit write takes, so this wants its own test design — instrumenting the native commit to reject and asserting the handler sees it, rather than asserting durability (which holds either way).

Not this issue

The atomicity/join-gate defect in #2292. That fix makes this reachable one extra way (a chained link for a second database is now itself an ImmediateTransaction, so it can be re-entered after closing), but the shape above predates it and the head slot has always had it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    Fields

    Priority

    P1

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions