HarperFast / HarperFast/harper

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

Open
#2,323 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

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

## What happens

`ImmediateTransaction.save()` is the commit trigger:

```js
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. `addWrite` → `save(operation)` → `isCommitting === false` → `this.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.

Contributor guide

Open the contributing guide

Research direction

Start with ImmediateTransaction.save() and commit(), then trace txnForContext and the immediateCommit branch in resources/DatabaseTransaction.ts. Design a test around a second autocommit write that makes the native commit reject, and verify the handler observes the rejection rather than acknowledging early. Done means the nested commit promise is awaited and its failure is handled by the write path.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, node.js
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.