fix: release the context's transaction back-reference on completion

[kriszyp]

What / why

A long-lived context — notably an MQTT subscription context, which stays reachable for the
whole life of a suspended delivery loop (server/DurableSubscriptionsSession.ts starts the
delivery loop without awaiting it, so transaction() runs its onComplete and commits
immediately) — kept its context.transaction reference pointing at the completed
DatabaseTransaction forever, pinning it (and whatever it exclusively retained) in memory.

A V8 heap snapshot from a production node running harper-pro 5.1.26 under a ~127k-connection
MQTT load test found 157,893 live DatabaseTransaction objects on a single worker (vs. 10,620
connections — ~14.87 per connection), 157,886 of them retained by a plain object Object
through a property literally named transaction: resources/transaction.ts:40's
context.transaction = transaction, never cleared. 22.9 MB shallow on one of 16 workers.

DatabaseTransaction now releases the context's back-reference once a transaction is truly
done:

• On the wrapper's own final commit (resources/transaction.ts's { doneWriting: true },
  the commit issued once the caller's callback has fully returned) or on abort().
• Identity-guarded (this.#context?.transaction === this) so a context already re-pointed at a
  different (e.g. reused) transaction is never clobbered.
• Not on an in-callback explicit context.transaction.commit() — the documented "this
  transaction be reused and committed again" pattern intentionally keeps recommitting and
  adding writes to the same instance, so releasing there would strand later writes with
  nothing to join.
• Deferred when a final commit still has outstanding read iterators streaming through the
  transaction (the existing "commit now, keep the handle open for iterators" replay path) —
  those iterators still depend on the same instance, so the release completes once the last
  one drains (doneReadTxn() / releaseReadTxn()).
• Not on a transaction poisoned by abortDueToTimeout() (this.timedOut) — Resource.ts's
  dispatcher deliberately keeps joining a timed-out transaction so the rest of that logical
  operation fails atomically (Abort over-time write transactions instead of force-committing (#1407) #1411), instead of silently starting fresh after a partial
  rollback. Releasing there would have disarmed that guarantee (caught by independent review
  and verified against integrationTests/resources/txn-overtime-atomicity.test.ts).

Scoped to the RocksDB path. LMDBTransaction fully overrides commit()/abort() and
doesn't call into this cleanup — under HARPER_STORAGE_ENGINE=lmdb this retention is
unaffected either way (flagged by independent review; left out of scope for this PR).

Expected win (be honest about the size): ~2.2 KB/connection of directly-freed shallow size
(22.9 MB / 10,620 connections), roughly 2.8% of the ~79 KB/connection measured total, plus
whatever each transaction exclusively retained. This is a clean, root-caused, low-risk win —
not the headline fix for per-connection memory. The larger costs (the per-subscription
async-iterator closure chain, ~27%; socket write-path buffers, ~20%; subscriptions-per-connection,
application-side) are being handled separately and are out of scope here.

Implementation note

The literal fix (release unconditionally at the three existing cleanup points, mirroring the
resourceCache clearing that used to happen at the same three points before 23298663d
removed it as vestigial) broke two existing tests exercising the "commit in the middle"
pattern, because context.transaction would go null the instant any interim write's own
short-lived transaction completed. Fixed by threading the wrapper's own doneWriting flag
through as a final gate, plus deferring the release when read iterators are still
outstanding. See the comments on releaseContext() / completeDeferredContextRelease() in
resources/DatabaseTransaction.ts for the full reasoning.

Test plan

• New unit tests in unitTests/resources/transaction.test.js ("Releasing the context
  back-reference on transaction completion"): commit releases, abort releases, a re-pointed
  context is not clobbered, and a context is safely reused for a second transaction() call
  after the first commits.
• Updated unitTests/resources/operationContextTransactionLeak.test.js's mechanism-level test
  (Audit records lack user attribution for writes from registered operations (no ambient operation context) #1591/Audit records now attribute registered-operation writes to the authenticated user #1592 regression coverage) to assert the new, stronger invariant this change provides.
• Updated unitTests/resources/lingeringWriteCommit.test.js to capture the transaction
  reference before it can be released, and added an assertion that the context's own reference
  is released once the outstanding iterator drains.
• New unit test asserting a timeout-poisoned transaction stays attached to its context.
• npm run test:unit:resources (1346 passing) and npm run test:unit:main (4133 passing, 11
  pre-existing/unrelated failures verified against unmodified main — worktree-sandboxed
  component-loading fixtures and one config-validator path-length test, both untouched by this
  diff) both green relative to this change.
• integrationTests/resources/txn-overtime-atomicity.test.ts and
  overtime-multi-write-atomicity.test.ts both pass (the Abort over-time write transactions instead of force-committing (#1407) #1411 atomicity guarantee this PR
  could have disarmed).
• Independent pre-push review (codex + gemini + grok + Harper-domain adjudication): round 1
  came back CHANGES with a confirmed blocker (the timeout-poison interaction above) and a
  confirmed test-coverage regression (a rewritten test lost its original discriminating power);
  both fixed and re-verified. Round 2 re-check: the graded/independent leg timed out on infra
  grounds; the advisory legs that did complete raised nothing new that held up under inspection.

Scope

Out of scope per the source investigation, not attempted here: the MQTT/subscription code
(server/DurableSubscriptionsSession.ts), the async-iterator closure chain, socket buffers, and
reducing subscriptions-per-connection. A v5.1 backport is likely wanted (affected clusters run
5.1.x) but is left to release management to decide.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request addresses a transaction-context memory leak where long-lived contexts (such as MQTT subscription contexts) kept pointing to completed DatabaseTransaction instances, pinning them in memory. It introduces a releaseContext mechanism to drop the transaction's back-reference from its context upon a final commit or abort, while deferring the release if there are outstanding read iterators still using the transaction. It also updates and adds corresponding unit tests to verify this behavior, including handling of timeout-poisoned transactions and context reuse. There are no review comments, and I have no additional feedback to provide.

[claude]

Reviewed; no blockers found.

[cb1kenobi]

Reviewed 65d6ed92a — no issues found. This PR looks good, nice job!

Traced the release across every path — final wrapper commit (doneWriting: true), abort, the deferred-until-iterators-drain path (doneReadTxn()/releaseReadTxn()), retry recursions, and the multi-store next chain — and the identity guard (#context?.transaction === this) plus the timedOut tombstone exclusion hold up. Confirmed the dispatcher semantics are unchanged: a released (now null) reference takes the same start-fresh branch a CLOSED one did, so #1591 and the #1411 over-time atomicity guarantee are both preserved. Test coverage is thorough.

Note: unit tests were not executed in-environment (worktree deps/native rocksdb build not installed); this is a code-trace review, relying on the reported test:unit:resources/test:unit:main results.

—
Generated by Barber AI

[cb1kenobi]

Re-reviewed 4542a8e1 (2 new commits since 65d6ed92) — no issues found. This PR looks good, nice job!

Incremental re-review of the new terminal-commit-failure release site plus the null/type-widening change:

• The new releaseContext(!!options.doneWriting) on the terminal (non-conflict) native-commit rejection path fills a real gap — that branch does a native-only transaction.abort(), so the wrapper abort() → releaseContext() release never ran there and onComplete() has no rejection handler. Correctly deferral-safe (readTxnsUsed → pendingContextRelease → doneReadTxn()/completeDeferredContextRelease()), identity-guarded, and touches only the JS back-reference (no native-handle lifetime change, no use-after-free).
• Verified no double-release: the 861 failure site and the 872 success site are mutually exclusive branches of the same native commit; releaseContext is idempotent under the identity guard regardless.
• The retry-exhaustion give-up path (abortChainAfterRetries → wrapper abort() → releaseContext at 913) already releases the context, so it is not a companion leak.
• Abort over-time write transactions instead of force-committing (#1407) #1411 timeout tombstone preserved: commit() throws transactionOpenTooLongError() synchronously at the top for a timedOut transaction, so it can never reach the new release site — the doneWriting gate there (vs !this.timedOut in abort()) is safe.
• Context.transaction: DatabaseTransaction | null | undefined is a net behavioral no-op — the slot already held null at the prior SHA; no consumer distinguishes null from undefined (dispatcher keys on ?.open === OPEN).

Test changes are consistent (lingeringWriteCommit captures the wrapper before the drain that nulls context.transaction; the mechanism test drops the disallowed prototype.setContext stub for synchronous observation). Unit tests not executed in-environment (native rocksdb build not installed) — code-trace review.

—
Generated by Barber AI

[cb1kenobi]

Re-reviewed daae9ccd — no issues found. The update since the last review is a merge of main (RocksDB managed-backup feature, v5.2.0 release, Windows atomicWriteFile retry-budget fix); this PR's own files (DatabaseTransaction.ts, ResourceInterface.ts, tests) are unchanged and the transaction back-reference release remains correct. This PR looks good, nice job!

—
Generated by Barber AI