HarperFast / HarperFast/harper
Missing when() rejection arms: isCommitting sticks true, and a save-loop-attached native handle is neither committed nor aborted
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
Split out of the #2288 / #2291 review. Two adjacent gaps in the same path, neither of which loses data.
## 1. `ImmediateTransaction.save` — `isCommitting` sticks true on a rejected commit
`resources/DatabaseTransaction.ts:1410`:
```js
this.isCommitting = true;
return when(this.commit(), () => {
this.isCommitting = false;
});
```
No rejection arm, so a rejected commit leaves `isCommitting === true` for the life of the instance. It is **not** a write loss: later writes then route through `super.save` with `open === CLOSED`, which takes the immediate-commit branch and still commits them. But the flag is wrong from then on, and recovery depends on a second code path happening to be correct rather than on the flag being right.
## 2. `DatabaseTransaction.commit` — a rejecting completion strands the native handle
`resources/DatabaseTransaction.ts:851`:
```js
return when(
completions.length > 0 ? Promise.all(completions) : null,
() => { /* the entire commit resolution */ }
);
```
Also no rejection arm. The staging loop above it runs `save()` for every write, and `save()` **attaches a fresh native handle** when the transaction had none (`:773`, under `this.open === OPEN`). If any staged completion then rejects — a `before`, a `beforeIntermediate`, or a write's commit handler — the whole resolution block is skipped and the rejection propagates:
- the attached handle is neither committed nor aborted;
- `this.open` stays OPEN and `releaseContext()` never runs, so the context keeps pointing at a transaction holding a live read snapshot and staged write intents;
- other writers' coordinated-retry commits park on those intents — the same cost as #2001.
It is **bounded**, not a permanent leak. `save()` registers the chain root in `supervisedWriteRoots` (`:786`) precisely because the handle was adopted outside `getReadTxn()`, so the long-transaction monitor (`:1512`) reaps it at `txnExpiration`. The exposure is up to one expiration, not until GC — which is why this is filed at P2 rather than higher.
This half is **pre-existing and general**, not specific to `ImmediateTransaction`: any transaction whose completions reject strands its handle the same way. The Immediate path merely makes it easy to reach, because its handle is *only* ever attached inside the save loop (its `getReadTxn()` opens none).
## Fix shape
Both want the same treatment: a rejection arm that restores the flag / aborts the handle and releases the context, then rethrows. The terminal-failure arm already inside the commit resolution — `transaction.abort()` in a `try`, then `endScopeOwnership()`, `releaseContext()`, rethrow — is the shape to copy.
Line references are against `main` at the time of filing; #2291 shifts `commit()` by a few lines.
Contributor guide
Research direction
Start in resources/DatabaseTransaction.ts at the save paths around lines 773 and 1410 and the commit completion handling around line 851. Compare the existing terminal-failure arm, then verify that rejected commits restore isCommitting or abort the native handle, release context, and rethrow without leaving the transaction open.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, nodejs
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 66/100