Don't poison a transaction parked in its commit phase (it destroyed the deploy payload blob)

[kriszyp]

Fixes #2062.

What was wrong

A blob is saved in the commit's pre-commit phase, so a write carrying one stays inside commit() until the file has landed. For a multi-tens-of-MB deploy payload that is well past storage.maxTransactionOpenTime, and the long-transaction monitor poisoned the transaction right there.

That was worse than the issue's original diagnosis. abort() cleared the write set and unlinked the write's pre-saved blobs — and then the in-flight commit resumed, found nothing left to write, and resolved as success. So:

• the caller was told its write landed when it had been dropped (silent write loss, and no error for ingestPayload's cleanup path to catch), and
• it was left holding a blob instance whose file was gone but whose fileId was still set. saveBlob short-circuits on a set fileId, so the deploy recorder's next put of the same record object silently minted a reference to a destroyed file — surfacing at the peer as Blob file not found, unrecoverably.

Reproduced end to end in integrationTests/database/blob-commit-over-time.test.ts: on main the HTTP write returns 200 and the record is a 404.

The fix

The open-transaction limit polices an application holding a transaction open with an unfinished write set (#1407). Once commit() is entered the write set is sealed and the caller is awaiting the commit, so the time spent in the pre-commit phase is core's own I/O — not the thing the limit exists to stop.

1. The monitor spares a transaction parked in its commit phase rather than poisoning it. Bounded: COMMIT_PHASE_GRACE over-limit ticks (~10 min at the 30s default, since sparing re-arms timeout), because the transaction still pins a read snapshot — a source that stalls rather than finishing falls through to the normal abort. sourceApply/isReplay are spared without a bound: they may be neither aborted (harper-pro#348) nor force-committed mid-write, and their blob sources are bounded by the receive-side idle watchdog.
2. Resuming from the pre-commit await re-checks that the transaction is still alive — timedOut (monitor poison, including via the multi-store chain) or a write set cleared with the handle released (a plain abort()). Either throws instead of resolving as a phantom commit.
3. A blob whose file has been unlinked is tombstoned (storageInfo.discarded), set when the deletion is decided rather than when the unlink is issued, and saveBlob refuses to re-store it. Any residual path that would have minted a second reference to a destroyed file now fails loudly, with the cause, instead of at some later read.

LMDB carries the same three changes.

Also fixed along the way

sourceApply/isReplay transactions parked in the pre-commit phase were previously force-committed on every monitor tick — durably committing a replica record while its blob file was still being written. That predates this change (it's the monitor's else branch); the new unit test fails on main for exactly that reason.

Verification

• unitTests/resources/txn-tracking.test.js — five cases: the commit completes and the blob survives; poisoned-while-parked throws; plain-abort-while-parked throws; a source apply is neither poisoned nor force-committed; the grace is bounded. Pass on both engines (test:unit:resources and test:unit:lmdb).
• unitTests/resources/blob.test.js — a discarded blob cannot be silently re-stored.
• integrationTests/database/blob-commit-over-time.test.ts — end to end over REST with the limit lowered and the blob source trickled; asserts the record commits, the blob reads back complete, no abort line, and the "in its commit phase past the open-transaction limit" line (so the test cannot pass vacuously). Fails on main.
• npm run test:unit:resources (1494 passing), the deploy integration suite (59/59), and the two existing Force-committing an over-time transaction leaves orphaned secondary-index entries (atomicity violation) #1407/Abort over-time write transactions instead of force-committing (#1407) #1411 anchors — longtxn-secondary-index (QA-176) and aborted-update-index-migration (QA-616) — all pass, i.e. the abort guarantee for application-held transactions is unchanged.
• test:unit:main can't run on this machine (the local Harper install holds ~/harper/database/system/LOCK); it's untouched by this change and covered by CI.

Where to look hardest

• The grace bound. COMMIT_PHASE_GRACE = 10 is a judgment call. Too low and a genuinely huge payload on a slow link gets aborted (cleanly now, but aborted); too high and a stalled pre-commit source pins a read snapshot for longer. The unbounded sparing of sourceApply rests on the replication receive path arming blobStreamIdleTimeoutMs — worth a second opinion.
• DatabaseTransaction.ts resume guard (stagedWrites > 0 && this.writes.length === 0 && !this.transaction). It must not false-positive on the retry / immediate-commit / replay paths. I could not find a caller that plain-abort()s a transaction already parked in commit, so that half is a latent trap being closed rather than a live bug — the test drives it directly.

Open / not addressed

• No end-to-end coverage through the actual deploy_component path: the deployment put only registers with the monitor when a read precedes the write in the same transaction (getReadTxn early-returns once a write created the handle), which the clustered field case did and a single-node harness does not. The integration test drives the same mechanism through a component resource instead.
• Two field initializers per transaction construct (committing, commitPhaseTicks) — deliberate; adding them dynamically would cost a hidden-class transition on the commit path.

Generated with Claude Opus 5. Cross-model review: codex + gemini + Harper-domain adjudication, four rounds — round 4 verdict COMMENTS, no majors outstanding.

Human-Review-Need: 4 @ 22e34ad

🤖 Generated with Claude Code

[kriszyp]

Reading order

1. integrationTests/database/blob-commit-over-time.test.ts first. It is the shortest statement of the bug. On main the HTTP write returns 200 and the record is a 404 — the caller is told its write landed and it wasn't. Everything else in the diff exists to make that impossible.

2. resources/DatabaseTransaction.ts:609-625 — the pre-commit await, which is where the whole thing happens. commit() parks here on Promise.all(completions) (the blob's durable file write). Two things were true and neither was obvious: the monitor could abort the transaction while it was parked, and the resumed continuation didn't notice — it walked on with an empty writes array and resolved successfully. That is the silent write loss, and it is also why ingestPayload's cleanup path never ran in the field: it only fires on a rejection.

3. resources/DatabaseTransaction.ts:1074 — the monitor branch. This is the design decision worth arguing with, so it's the one I'd read most carefully.

4. resources/blob.ts:869 / 930 / 1717 — the tombstone. Third line of defence; see below.

Decisions a reviewer should question

Exempting the commit phase at all. #1411 made over-time write transactions abort rather than force-commit, and this walks part of that back. My argument: the limit's premise is "the application is holding a transaction open with an unfinished write set" — once commit() is entered that premise is false. There is no partial write set to protect, the caller is already awaiting, and the abort's only effects were to drop a sealed write and unlink blobs the write still referenced. The #1407/#1411 anchors (QA-176 longtxn-secondary-index, QA-616 aborted-update-index-migration) both still pass, which is the evidence that the application-held case is untouched.

Why the grace is bounded, and why sourceApply isn't. A parked transaction still pins a read snapshot, so an unbounded exemption turns a stalled upload into a snapshot-pinning vector — hence COMMIT_PHASE_GRACE. sourceApply/isReplay are the exception because both of the monitor's other options are wrong for them: aborting drops a canonical write with no resume path (harper-pro#348), and the force-commit branch they used to land in durably commits a replica record whose blob file is still being written. That force-commit is pre-existing — the new source-apply test fails on main for that reason, not because of anything else in this PR.

Why the blob tombstone exists at all, given 1 and 2. With the monitor no longer poisoning a committing transaction and the resume guard catching the rest, the destroyed-blob path should be unreachable. The tombstone is there because "should be unreachable" is exactly what the pre-#2062 code believed. It converts any residual path from a silently-committed reference to a destroyed file into a loud error naming the cause. It is deliberately set when the deletion is decided, not when the unlink is issued — cleanupUnusedBlobs waits for an in-flight save to settle first, and a re-store in that window was still winning.

What the tests do and do not prove

They prove: the write commits and its blob is readable when the save outruns the limit (both engines, plus end to end over REST); a poisoned or plain-aborted parked transaction throws instead of phantom-committing; a source apply is neither poisoned nor force-committed; the grace is bounded; a discarded blob cannot be re-stored.

They do not prove: anything through the real deploy_component path. The deployment put only registers with the long-transaction monitor when a read precedes the write in the same transaction (getReadTxn early-returns once a write has created the handle) — the clustered field case did, a single-node harness does not. The integration test drives the identical mechanism through a component resource instead. Also untested: orphaned partial-file cleanup after the grace-exhausted abort (existing abort-cleanup behaviour, unchanged here).

Coverage

Four rounds of cross-model review (codex graded leg + gemini + Harper-domain adjudication). Rounds 1–3 each found something real and each is a commit in this branch: the unbounded exemption, the plain-abort phantom commit, the sourceApply force-commit, the too-late tombstone, and three unit tests that would have gone red on the LMDB CI leg (they waited on the head of the transaction chain, which on LMDB never owns the table's writes). Round 4 returned COMMENTS with no majors. Outstanding and accepted: two field initializers per transaction construct, deliberate over a dynamically-added property.

— KrAIs (Claude Opus 5)

[gemini-code-assist]

Code Review

This pull request addresses issue #2062, where transactions parked in their commit phase (such as during a large blob write) were incorrectly poisoned by the long-transaction monitor for exceeding the open-transaction limit. The changes introduce a bounded grace period (COMMIT_PHASE_GRACE) for transactions in the commit phase, add checks to prevent phantom commits if a transaction is aborted while waiting on pre-commit work, and tombstone discarded blobs to prevent silent references to deleted files. The review feedback recommends replacing fixed delays in the new unit tests with condition-based waitFor polling to prevent test flakiness in resource-constrained CI environments.

[gemini-code-assist]

Avoid using fixed sleeps or real-time delays (like delay(150)) in tests to prevent flakiness in slow or resource-constrained CI environments. Instead, use a condition-wait helper like waitFor to poll for the expected state (e.g., waiting for the transaction's commitPhaseTicks to increment).

Suggested change

	await delay(150); // the monitor fires repeatedly while the commit waits on the blob write
	await waitFor(() => committingTxn(context)?.commitPhaseTicks > 2, {
	message: 'should wait for monitor to tick several times',
	});

References

1. Prefer using condition-waits (e.g., a helper like waitFor that polls for a condition) instead of fixed sleeps or real-time delays (like setTimeout) in tests to avoid flakiness caused by coarse clock resolutions or slow environments.

[gemini-code-assist]

Avoid using fixed sleeps or real-time delays (like delay(600)) in tests to prevent flakiness in slow or resource-constrained CI environments. Instead, use a condition-wait helper like waitFor to poll for the expected state (e.g., waiting for the transaction's commitPhaseTicks to exceed COMMIT_PHASE_GRACE).

Suggested change

	await delay(600); // many ticks, well past COMMIT_PHASE_GRACE
	await waitFor(() => committingTxn(context)?.commitPhaseTicks > COMMIT_PHASE_GRACE, {
	message: 'should wait until commitPhaseTicks exceeds COMMIT_PHASE_GRACE',
	});

References

1. Prefer using condition-waits (e.g., a helper like waitFor that polls for a condition) instead of fixed sleeps or real-time delays (like setTimeout) in tests to avoid flakiness caused by coarse clock resolutions or slow environments.

[claude]

Reviewed; no blockers found.

[kriszyp]

CI status: 37 pass, 4 fail — all four failures are pre-existing on main, not from this change.

• Integration Tests 6/6 (Node 24 / Windows / uWS) — txnlog-purge-stale-read-blast.test.ts:683, "purge job should COMPLETE, got ERROR: Table-level transaction log deletion is not supported for RocksDB tables". The identical assertion fails on main's own run (30956731137, shard 5/6). Adding a test file shifted which shard it lands in, hence 6/6 here vs 5/6 there.
• Integration Tests 2/6 (Bun) — redeploy-runtime-equivalence.test.ts:392. Also failing on main (Bun shard 1/6), along with three sibling cases in the same file.

This PR's own coverage is green: all three Unit Test legs pass, and the new integration test ran and passed on Linux and Windows —

▶ Blob save that outruns the open-transaction limit (#2062)
  ✔ the write commits and its blob survives (2034ms)

Staying in draft until the pre-existing main breakage clears (or a reviewer is happy to look past it).

— KrAIs (Claude Opus 5)