fix(storage): stop a stall verdict vouching for the connection it stalled on (DAB-1177)

[matalina]

Follow-up to #32, which merged before these two landed. Both were caught by @jacwright in review there, and both are real.

Neither is reachable at the shipped defaults — transactionTimeout is 0, so nothing arms the hard timer. That is why they were correctly flagged non-blocking. They bite the moment it is enabled, which is DAB-1177 step 2.

1. A stall verdict vouched for the connection it stalled on

The timeout path called settleReject, which runs the wrapped settle and stamps lastSettleAt — so a guard firing counted as evidence the connection was alive, for every other guard on it.

The abort made it worse than it first looks: abortTarget provokes an abort that arrives back through the ordinary onabort handler and stamped a second time.

On a wedged connection with several operations in flight — the exact case this guard exists for — each timeout vouched for the remaining ones and they kept deferring, up to MAX_DEFER_WINDOWS apiece. Bounded, so never a hang, but it delayed precisely the verdicts the guard is meant to deliver.

Now a timedOut flag, set before the abort so neither the rejection nor the abort it provokes stamps. Only genuine settles vouch.

2. Late-fire misread ordinary scheduling lag as a suspended tab

The heuristic compared elapsed time against the window actually scheduled. After a defer that window is only the remainder of a budget and can be a few milliseconds, so budget * LATE_FIRE_FACTOR was trivially cleared by event-loop lag on a main thread busy with bulk IndexedDB work — misreading jitter as suspension, spending the single re-arm on a full extra window, and stamping lateFire: true on an error measured wide awake. That last part is the damaging one: lateFire is the flag the duration stats get filtered by, so a wrong one quietly corrupts the measurement this whole feature exists to produce.

Took both suggestions from the review rather than either: an absolute LATE_FIRE_FLOOR_MS = 500 and measuring against the full hardBudget, factored into a shared firedLate().

This also removed a latent flake in the existing suite — reports a slow transaction used a 20ms budget and asserted lateFire === false, which 25ms of jitter would have flipped.

budget became write-only after the change and is gone.

Testing

tsc --noEmit   clean
npm test       46 passed | 3 skipped   (was 43)

Both fixes carry a regression test. I verified they are not vacuous by reverting both fixes and re-running — exactly those two fail, and pass again once restored.

[jacwright]

Fix 2 is right - the floor plus full-budget comparison closes the jitter misread, and I verified both regression tests fail without their fixes (tsc clean, 46 passed | 3 skipped locally; repo still has no CI).

Fix 1 is incomplete. timedOut is per-promise-closure, but every chained request runs its own requestToPromise closure over the same shared context. A transaction with any request in flight - a plain put/add/delete, the normal case - still stamps lastSettleAt when the guard's rejection propagates through the chained handlers (and, in a real browser, through the per-request AbortError events the abort provokes). The new regression test uses a request-less transaction, so it passes while the common case keeps vouching. Repro and a suggested fix inline.

Housekeeping: version bump correctly absent (manual Publish releases here) - but post-merge someone needs to land a Publish covering #32 + this before anything can pin the guard; worth tracking on DAB-1177.

[jacwright]

timedOut lives in this promise's closure, but each chained request (put/add/delete pass store.transaction) runs its own requestToPromise with its own timedOut = false over the same shared context. When the guard fires, the rejection propagates into each chained request's handler - and in a real browser the abort also fires onerror (AbortError) on every pending request - and those closures stamp lastSettleAt. So the loophole is closed only for a transaction with no requests in flight; any normal write reopens it, and each timeout still buys the sibling guards another defer window (verdicts serialize at ~hardBudget intervals).

Reproduced on this branch: give the harness fake's objectStore a transaction: trans backref plus a put() returning a bare {onsuccess: null, onerror: null} request, then put + commit - lastSettleAt moves by ~clockOffset after the guard fires.

Suggest marking the timed-out target where every closure can see it, e.g. a module-level WeakSet the guard adds the target to before abortTarget, and settle checks timedOut || (transaction && timedOutTargets.has(transaction)). That covers both the chained rejection and the request-level abort events.

[matalina]

Right on both counts, and the vacuous-test part is the bit that stings — the request-less fake was exactly the shape that hid it.

Fixed in 6013af5 with your suggestion: module-level timedOutTargets WeakSet, marked before abortTarget, checked in settle against transaction ?? request. Keying on the transaction (a chained request is evidence about its transaction) covers both the propagated rejection and the request-level AbortError events. Weak so a finished transaction is not retained. The per-closure timedOut flag is gone — the set subsumes it.

Verified the way you did: reverting settle to per-closure semantics (has(request) instead of has(guardedTarget)) fails only the new chained-write test while the old request-less one keeps passing.

[jacwright]

This transaction has no chained requests, which is why it passes while the put/add/delete path still stamps (see Browserbase.ts comment). Note the trap: the fake's objectStore has no transaction backref, so a naive put() here would take the transaction-less path and arm its own guard, passing vacuously. Once fixed, worth asserting the same invariant with a request in flight - the fake needs objectStore() to carry transaction: trans and a put() returning a bare {onsuccess: null, onerror: null} request.

[matalina]

Added the chained-write test with your recipe — objectStore() carrying a transaction: trans backref and a put() returning a bare {onsuccess: null, onerror: null} request.

Took the trap seriously too: since a missing backref would silently send the write down the transaction-less path to guard itself, the test asserts the write and the commit reject with the same error object, which only holds if it genuinely chained. That way it fails loudly rather than passing for the wrong reason if the fake ever drifts.

Also annotated the original request-less test to say what it does and does not cover, so the next reader does not mistake it for the general case.

47 passed | 3 skipped, tsc clean.

[jacwright]

The WeakSet fix addresses the chained-write case from my last round: keyed on the transaction object, marked before the abort, checked in settle, and the new regression test bites (removing the add fails two tests; keying on the request instead fails the chained test). tsc and the suite are green in a fresh worktree; this repo has no CI.

One gap remains in the same class, and it is the shape dw3 actually uses, so one more round. Two lows alongside; the doc-comment one is trivial.

[jacwright]

A transaction-less read (get/getAll/count) issued inside a start() scope runs on the shared _current transaction but passes no transaction here, so its target is its own request. When the transaction's guard fires and aborts, the browser fires AbortError onerror on that pending read, its closure sees has(request) false, and stamps lastSettleAt. The reverse order has the same hole: the read's guard aborts request.transaction, then trans.onabort and every chained write stamp because only the request was marked. dw3 does exactly this shape (templateCache.ts:34, accountService.ts:474 both read inside a scoped transaction).

Suggest keying on the transaction the request belongs to in both directions:

const guardedTarget: object = transaction ?? request.transaction ?? request;

The comment below ("as an AbortError onerror on every request still pending on that transaction") currently claims this case is covered.

[matalina]

Fixed in 56b8ea1 with your one-liner: transaction ?? request.transaction ?? request. Ryan's reverse ordering is covered by the same change, and I took his check on the fallback being inert elsewhere (IDBTransaction has no .transaction; an unscoped read gets its own implicit one).

The doc comment that claimed this case was already covered is rewritten to describe what actually happens, including the scoped-read shape it was silently excluding.

[jacwright]

The fake's abort() only sets aborted = true; it never fires onabort or an AbortError onerror on pending requests, so this wait (and the one at 293) exercises nothing. Having the fake fire both on abort would cover the paths the code comments describe, and is the natural regression test for the scoped-read gap above.

[matalina]

Fixed. The fake now fires what a browser fires — onabort on the transaction and an AbortError onerror on every pending request — and hands out tracked requests carrying their transaction backref, so the backref is structural rather than something each test has to remember.

It fires synchronously where a browser queues the events. That is deliberate: it is the stricter ordering, and it immediately surfaced a real bug. abortTarget ran before settleReject, so onabort's bare Error('Abort') beat the typed error to the promise — four tests went red on it. Browsers queuing the abort asynchronously is the only reason callers ever saw StorageTimeoutError. The guard now settles before aborting, so the outcome is the same in either ordering.

[jacwright]

nit: the WeakSet block landed between the LATE_FIRE_FACTOR doc comment and the constant it documents. Move this above that comment.

[matalina]

Fixed — the WeakSet block now sits above the LATE_FIRE_FACTOR doc comment, and guardedTransactions is declared alongside it.

[RyanHavoc]

Second reviewer, reading the code fresh rather than the thread. Not approving: @jacwright's scoped-read gap is real, I reproduced the reasoning independently, and it's still open on this head (6013af5).

Confirming the blocking finding, and it's worse in one direction than described. ObjectStore.get/getAll/count (and Where.getAll/getAllKeys/count) all pass null as transaction while _transStore hands them db._current inside a start() scope, so guardedTarget resolves to the request. Two orderings, both leaking:

• Transaction guard fires first → timedOutTargets.add(trans) → trans.abort() → AbortError onerror on the pending read → that closure checks has(request), false → stamps. As described.
• The read's own guard fires first → timedOutTargets.add(request) → abortTarget(request) walks to request.transaction and aborts the shared transaction → trans.onabort and every chained write settle against has(trans), false → each stamps. So a single scoped read's deadline can hand out fresh defer windows to every sibling on the connection it just killed.

transaction ?? request.transaction ?? request closes both, because both orderings then mark and check the same object. I checked the fallback holds elsewhere: requestToPromise(trans, null, db) from start() passes the transaction as request, and IDBTransaction has no .transaction, so it still keys on itself; a read outside any scope gets the implicit per-request transaction _transStore just created, which is unique to it. No behaviour change for either.

Fix 2 I have no concerns about. The floor plus full-hardBudget comparison is right, and folding both into firedLate keeps the soft path honest. Worth noting the deliberate consequence: after a defer, Date.now() - scheduledAt is the remainder, so proportional lateness is effectively unreachable and only genuine multi-second suspension trips it — which is the intent, but it means late-fire detection is much rarer post-defer than the name suggests.

Two lows inline (a comment whose premise doesn't hold for the scoped-read shape, and a settle test that runs with guards disabled). Both non-blocking and neither needs a round of its own.

---

Generated by Claude Code

[RyanHavoc]

Low, pre-existing, and adjacent to @jacwright's scoped-read finding rather than a separate bug — but this comment's own premise doesn't hold for the shape he names.

"Only the transaction-less shape owns a deadline" reads as transaction-less request ⇒ no transaction to already be guarded. A get/getAll/count issued inside a start() scope is transaction-less by parameter while running on _current, which was armed back in start(). So that read arms a second guard on an already-guarded transaction, and the stall it is watching is the same stall — precisely the "two deadlines on one stall" this comment says the branch avoids. They differ only in start time, so the read's deadline lands later and can abortTarget a transaction whose own guard already deferred.

Keying on request.transaction fixes the stamping, but not this — the second timer stays armed. Worth either widening the arming condition to skip when request.transaction already has a guard, or narrowing the comment to say what it actually means (this branch arms because nothing else in this closure will).

---

Generated by Claude Code

[matalina]

Agreed, and I went with widening the arming rather than narrowing the comment — a guardedTransactions WeakSet registered before arming, so a request whose transaction already has a deadline takes a skip branch.

One argument for widening that neither round raised, and the one that decided it for me: this is not only a hard-timeout problem. armStorageGuard bails only when BOTH statics are zero, so at the measurement defaults dw3 is about to ship (slowTransactionTimeout = 1000, transactionTimeout = 0) a scoped read still arms and still gets its own soft timer. One slow transaction then emits two storage_transaction_slow events, keyed on different store names — double-counting the distribution the whole instrument exists to produce, on exactly the templateCache/accountService path. Narrowing the comment would have left that in.

Skipping is safe: the transaction's guard still fires and aborts, and the AbortError reaches the read's onerror, so it rejects rather than hanging. One wrinkle worth a look — the onsuccess wiring lived inside the arming branch, so a skipped read would have been left unable to resolve; it is now outside, covering both transaction-less paths.

If you think the double-report argument does not hold, the arming skip is the piece to push back on.

[RyanHavoc]

Low: with both statics at 0, armStorageGuard returns null before arming anything, so no guard exists, timedOutTargets is never consulted for a mark that could not have been added, and this passes identically with or without the whole WeakSet change.

It's a fine test of settle in the abstract, but the comment above calls it "the other half of the invariant" — and the invariant at issue is a target that was guarded and did not time out still stamps. That needs a live guard the settle beats: transactionTimeout = 1000, then fire oncomplete as it already does. That version fails if a future change ever marks on arm rather than on fire, which is the regression this half is meant to hold.

---

Generated by Claude Code

[matalina]

Right — it passed with the whole WeakSet change removed. Now runs at transactionTimeout = 1000 so a guard is genuinely armed and the settle beats it, which makes it hold the invariant you named.

Checked it bites: marking on arm instead of on fire (adding to timedOutTargets alongside guardedTransactions) fails this test and only this test.

Since vacuous tests have been the running theme here, I reverted each fix on its own rather than trusting a green suite — keying fix 2 failures, arming skip 1, abort ordering 4, mark-on-arm 1.

[jacwright]

Round three closes everything from rounds one and two, and the two additions are right.

Guard target is transaction ?? request.transaction ?? request, so a scoped read's abort-driven AbortError no longer stamps, and the reverse ordering cannot happen because a scoped read no longer arms its own deadline. The skip branch is keyed on a WeakSet of guarded transactions, added before arming and never removed, so it can never block a fresh transaction, and the read still settles because onsuccess/onerror are installed on the skipped branch too. That skip also fixes a real dw3 shape: accountService calls start() twice on the same versionchange transaction, and round two fired a phantom StorageTimeoutError plus abort after a clean oncomplete there. Round three does not. The reorder to mark, reject, then abort means the typed error reaches the caller and the later onabort/AbortError settles are no-ops on an already-settled promise, with dispatchError firing once. Fix 2 is unchanged.

Repo has no CI: 49 passed, 3 skipped, tsc --noEmit clean in a fresh worktree. Mutations bite as claimed: reverting the guard target fails the once-per-transaction and scoped-read tests, disabling the skip branch fails once-per-transaction, restoring abort-before-reject fails four tests, and mark-on-arm fails only the one test Ryan predicted. One non-blocking test note inline. package.json still 3.1.2, so a Publish covering #32 and #33 is still needed after merge.

[jacwright]

Non-blocking. This assertion and the comment above it pin the fake's synchronous abort order, not browser behaviour. Browsers queue abort events as a task, so the chained write's rejection arrives first through the transaction promise's microtask chain: request.error throws while pending, the catch falls through to err, and the write sees StorageTimeoutError, identical to commitErr. Switching the fake's abort() to setTimeout(..., 0) fails only this line. The production reorder is correct under both orderings, so this is about what the test proves: the round-two identity check expect(writeErr).toBe(commitErr) was the anti-vacuity guard, and this replacement only catches a missing backref. Queue the fake's abort as a task and restore the identity assertion, or correct the two comments (here and the "strictly stricter" one on the fake).

[RyanHavoc]

Approved at 56b8ea1. My round-two finding is closed and both orderings of the scoped-read gap are shut.

Verified the mechanism against the real IDB shapes rather than the commit message. IDBTransaction.prototype has no transaction, a scoped read's request.transaction is identically the scope's transaction object, and two unscoped reads get distinct implicit transactions — so transaction ?? request.transaction ?? request keys every shape on the thing that actually wedged, and the fallback is inert everywhere else, exactly as the comment claims.

The arming skip is safe for a reason the description undersells: start() arms eagerly at L414, when the transaction is created, so the scope's transaction always owns the deadline and a later read can never win that race and leave the transaction guarded by a request-scoped timer. Unscoped reads can't collide either, per the distinct-implicit-transaction check above. I went looking for a way to strand a transaction with no deadline and didn't find one.

Reject-before-abort is right, and the reasoning generalises: settling first makes the caller's error the typed one under both the synchronous ordering the fake now forces and the asynchronous one browsers actually use.

Reproduced locally at 56b8ea1: tsc --noEmit clean, 49 passed | 3 skipped. Re-ran every mutation claim in the commit message and all four bite at the stated counts — keying fix 2, arming skip 1, abort ordering 4, mark-on-arm 1 — plus two the message doesn't claim: dropping LATE_FIRE_FLOOR_MS fails 1, and breaking the onsuccess wiring for guard-skipped reads fails the main suite broadly. That last one is worth noting since the wiring move is the sort of change that usually ships unpinned; it is covered, just from Browserbase.spec.ts rather than from here.

Two lows inline, neither blocking and neither this branch's to fix: the double-start(trans) promise that now never settles at all (pre-existing, but it revises what "removes a phantom timeout" bought us), and guardedTransactions recording an attempt to arm rather than a live deadline.

Agreed with Jacob that a Publish covering #32 and #33 still needs to follow the merge.

---

Generated by Claude Code

[RyanHavoc]

Low, non-blocking, and not yours to fix here — but worth recording, because it slightly revises the credit this branch has been given.

This skip branch does remove the phantom timeout on a double start(trans). What it leaves behind is a promise that never settles at all.

start() at L414 always calls requestToPromise(trans, null, db), so a second start() on the same IDBTransaction re-enters here, takes this branch, and arms nothing. It then can't wire handlers either: request.oncomplete === null and request.onerror === null are both false, since the first call already set them. Nothing ever calls that closure's settleResolve/settleReject. Then transactionPromise.set(trans, promise) publishes it, so every later chained op on that transaction (if (transaction) branch, L1002) awaits it too.

Confirmed against this head with a hand-driven fake, transactionTimeout = 20: the second commit() was still pending at 150ms, transaction aborted.

The shape is real — dabble-writer-3.0/src/services/accountService.ts:471,476 double-starts one versionchange transaction, and the putAll at :479 is exactly a chained op on the published promise. It doesn't bite today because neither is awaited and both timers ship at 0, so armStorageGuard returns null and the second promise never settled before this branch existed either. The change is what happens once the hard timer is on: round two rejected it (wrongly, but it terminated) and this terminates nothing.

The actual defect is the === null wiring not being re-entrant, which predates all of this and belongs in its own change. Flagging it because DAB-1177 step 2 is what makes the difference observable.

---

Generated by Claude Code

[RyanHavoc]

Low, non-blocking naming/precision nit.

This runs unconditionally, but armStorageGuard returns null when both budgets are <= 0 — the shipped default. So at defaults the set records "a closure passed through here", not "this transaction carries a deadline", and every scoped read in the app already takes the skip branch above rather than the arming one. Harmless as it stands: the skip is a no-op when nothing would have armed anyway, and onsuccess is wired outside the branch so the read still resolves.

It only diverges if a timeout static is raised while a transaction is open — the transaction registered with no guard, and its reads skip, so it stays unguarded for its whole life. Narrow, and dw3 would set these at boot, so probably never.

guard = armStorageGuard(...); if (guard) guardedTransactions.add(guardedTarget); makes the set mean what its name says. The "registered before arming" ordering the comment relies on is preserved either way — armStorageGuard only reads the statics and calls setTimeout, so no other request can be issued in between.

---

Generated by Claude Code