Skip to content
63 changes: 56 additions & 7 deletions resources/DatabaseTransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ export class DatabaseTransaction implements Transaction {
private completeDeferredContextRelease(): void {
if (!this.pendingContextRelease) return;
this.pendingContextRelease = false;
if (this.#context?.transaction === this) this.#context.transaction = null;
if (this.#context?.transaction === this) this.#context.transaction = RELEASED_TRANSACTION;
}

disregardReadTxn(): void {
Expand Down Expand Up @@ -517,19 +517,17 @@ export class DatabaseTransaction implements Transaction {
* alive (a fresh write on the same context must not join a DIFFERENT, already-replayed
* transaction) until doneReadTxn() drains the last one, so the release is deferred to there.
*
* Sets `.transaction` to `null` rather than deleting the property: `Context.transaction` is
* typed `DatabaseTransaction | null | undefined` precisely to document this released state, and
* `delete` would repeatedly force a long-lived, hot context (e.g. an MQTT subscription context
* releasing/reattaching a transaction per message) into V8's slower dictionary-mode property
* storage.
* Leaves RELEASED_TRANSACTION in the slot: the slot must stay callable (see that constant), and it
* must stay a plain assignment — `delete` repeatedly forces a long-lived, hot context into V8's
* dictionary-mode property storage.
*/
private releaseContext(final: boolean): void {
if (!final) return;
if (this.readTxnsUsed > 0) {
this.pendingContextRelease = true;
return;
}
if (this.#context?.transaction === this) this.#context.transaction = null;
if (this.#context?.transaction === this) this.#context.transaction = RELEASED_TRANSACTION;
}

checkOverloaded() {
Expand Down Expand Up @@ -1194,6 +1192,57 @@ export class ImmediateTransaction extends DatabaseTransaction {
}
}

/**
* What `context.transaction` holds once its transaction has completed and released the back-reference
* (see releaseContext()). `commit()`/`abort()` are no-ops and reads through it see the latest
* committed state, so the documented `getContext().transaction.commit()` pattern stays callable after
* completion, reporting the same `txnTime: 0` that re-committing the completed transaction itself did
* (its own timestamp is reset by the commit that completed it).
*
* Not a DatabaseTransaction subclass and not extensible: one process-wide instance shared by every
* released context owns no mutable state, and anything off this surface fails loudly rather than
* inheriting behavior that would write through to every other context.
*/
const RELEASED_TRANSACTION_SURFACE = {
open: TRANSACTION_STATE.CLOSED,
transaction: undefined,
writes: Object.freeze([]),
commit(): CommitResolution {
return { txnTime: 0 };
},
abort(): void {},
getReadTxn(): undefined {
return; // no transaction means read latest
},
useReadTxn(): undefined {
return;
},
doneReadTxn(): void {},
disregardReadTxn(): void {},
hasPendingWrites(): boolean {
return false;
},
addWrite(): never {
throw new Error(
'Cannot write to a transaction that has already completed; start a new one with transaction() or pass a fresh context'
);
},
setContext(): never {
throw new Error('Cannot attach a context to the shared released transaction');
},
};
Object.freeze(RELEASED_TRANSACTION_SURFACE);
export const RELEASED_TRANSACTION = RELEASED_TRANSACTION_SURFACE as unknown as DatabaseTransaction;

/**
* The placeholder means "this context has no transaction". Every reader that would otherwise act on the
* value — claim it for a store, adopt it as a context, treat it as data — must ask first, or it operates
* on the one instance every released context shares.
*/
export function isReleasedTransaction(value: unknown): boolean {
return value === RELEASED_TRANSACTION;
}

let timer;

/**
Expand Down
11 changes: 10 additions & 1 deletion resources/Resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ import {
RequestTargetOrId,
} from './ResourceInterface.ts';
import { randomUUID } from 'crypto';
import { DatabaseTransaction, TRANSACTION_STATE, type Transaction } from './DatabaseTransaction.ts';
import {
DatabaseTransaction,
isReleasedTransaction,
TRANSACTION_STATE,
type Transaction,
} from './DatabaseTransaction.ts';
import { IterableEventQueue } from './IterableEventQueue.ts';
import { _assignPackageExport } from '../globals.js';
import { ClientError, AccessViolation } from '../utility/errors/hdbError.ts';
Expand Down Expand Up @@ -224,6 +229,7 @@ export class Resource<Record extends object = any> implements ResourceInterface<
record = idPrefix;
}
}
if (isReleasedTransaction(context)) context = undefined;
if (context) {
if ((context as any).getContext) context = (context as any).getContext();
} else {
Expand Down Expand Up @@ -589,6 +595,9 @@ function transactional(
function applyContext(idOrQuery: string | Id | Query, dataOrContext?: any, context?: Context) {
let id, query, isCollection;
let data;
// An absent argument, not a context — and not this call's data either.
if (isReleasedTransaction(dataOrContext)) dataOrContext = undefined;
if (isReleasedTransaction(context)) context = undefined;
// First we do our argument normalization. There are two main types of methods, with or without content
if (hasContent) {
// for put, post, patch, publish, query
Expand Down
12 changes: 7 additions & 5 deletions resources/ResourceInterface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,13 @@ export interface Context {
/** Describes the current cookie-based session if it is present and grants the capacity to delete it. authentication.enableSessions must be turned on in the harperdb-config.yaml */
session?: Session;
/**
* The database transaction object. `undefined` means none was ever attached; `null` means one
* was attached and has since completed and released its back-reference (DatabaseTransaction.ts's
* releaseContext()) — kept `null` rather than deleting the property so a long-lived, hot context
* (e.g. an MQTT subscription context releasing/reattaching a transaction per message) doesn't
* repeatedly force V8 to deoptimize it into dictionary-mode property storage.
* The database transaction object. `undefined` means none was ever attached. On the RocksDB path, a
* completed transaction releases its back-reference (DatabaseTransaction.ts's releaseContext()) and
* the slot holds RELEASED_TRANSACTION, a shared completed transaction: always safe to call
* `commit()`/`abort()` on (both no-ops) and to read through (latest committed state, no snapshot),
* so code that commits mid-handler can keep using its context. LMDBTransaction does not release,
* so there the completed transaction itself stays in the slot — also safe to call, but retained.
* `null` was the previous released marker and is still accepted defensively.
*/
transaction?: DatabaseTransaction | null;
/** If the operation that will be performed with this context should check user authorization */
Expand Down
2 changes: 2 additions & 0 deletions resources/Table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
DatabaseTransaction,
ImmediateTransaction,
priorStagedWrite,
isReleasedTransaction,
TRANSACTION_STATE,
} from './DatabaseTransaction.ts';
import * as envMngr from '../utility/environment/environmentManager.ts';
Expand Down Expand Up @@ -5597,6 +5598,7 @@ export function makeTable(options) {
}
function txnForContext(context: Context) {
let transaction = context?.transaction;
if (isReleasedTransaction(transaction)) transaction = undefined;
if (transaction) {
if (!transaction.db && isRocksDB) {
// this is an uninitialized DatabaseTransaction, we can claim it
Expand Down
16 changes: 14 additions & 2 deletions resources/transaction.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import type { Context } from './ResourceInterface.ts';
import { _assignPackageExport } from '../globals.js';
import { DatabaseTransaction, type Transaction, TRANSACTION_STATE } from './DatabaseTransaction.ts';
import {
DatabaseTransaction,
isReleasedTransaction,
type Transaction,
TRANSACTION_STATE,
} from './DatabaseTransaction.ts';
import { AsyncLocalStorage } from 'async_hooks';

export const contextStorage = new AsyncLocalStorage<Context>();
Expand All @@ -25,8 +30,12 @@ export function transaction<T>(
asyncStorageContext = contextStorage.getStore();
context = asyncStorageContext ?? {};
} else {
// The released placeholder is an absent argument, not a context: normalized before the fallback
// chain below so it resolves to the ambient store exactly as the `null` it replaced did, rather
// than to a bare `{}` that would drop the caller's user, session and timestamp.
const contextArg = isReleasedTransaction(ctx) ? undefined : ctx;
// request argument included, but null or undefined, so maybe create a new one
context = ctx ?? (asyncStorageContext = contextStorage.getStore()) ?? {};
context = contextArg ?? (asyncStorageContext = contextStorage.getStore()) ?? {};
}

if (typeof callback !== 'function') {
Expand Down Expand Up @@ -75,6 +84,9 @@ export function transaction<T>(

_assignPackageExport('transaction', transaction);

// Only a context that never had a transaction has none to act on: a completed transaction still in the
// slot must no-op here, as it always did, or a checkpointing loop that commits every Nth row fails on
// its second checkpoint.
transaction.commit = function (contextSource) {
const transaction = (contextSource.getContext?.() || contextSource)?.transaction;
if (!transaction) throw new Error('No active transaction is available to commit');
Expand Down
14 changes: 9 additions & 5 deletions unitTests/resources/lingeringWriteCommit.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const { setupTestDBPath } = require('../testUtils');
const { table } = require('#src/resources/databases');
const { setMainIsWorker } = require('#js/server/threads/manageThreads');
const { transaction } = require('#src/resources/transaction');
const { TRANSACTION_STATE } = require('#src/resources/DatabaseTransaction');
const { TRANSACTION_STATE, RELEASED_TRANSACTION } = require('#src/resources/DatabaseTransaction');
const { setTimeout: delay } = require('node:timers/promises');

// A commit issued while a search iterator still holds the read transaction cannot commit the
Expand Down Expand Up @@ -61,7 +61,7 @@ describe('commit with open read iterators commits writes immediately on a replay
const { context, iterator } = await commitWithOpenIterator('linger-write');
// Captured before the iterator drains: once it does, DatabaseTransaction now releases the
// context's own back-reference too (releaseContext(), deferred here until the last iterator
// finishes — see doneReadTxn()), so context.transaction itself goes null. Assertions below
// finishes — see doneReadTxn()), so the slot stops pointing at this instance. Assertions below
// that need the specific (now-closed) transaction instance use this captured reference.
const closedTxn = context.transaction;
assert.equal(
Expand All @@ -78,7 +78,11 @@ describe('commit with open read iterators commits writes immediately on a replay
while (!(await iterator.next()).done);
await delay(50); // give a released-handle failure a beat to surface as an unhandledRejection
assert.equal(closedTxn.transaction, null, 'the drained iterator must release the native handle');
assert.equal(context.transaction, null, 'the drained iterator must also release the context’s back-reference');
assert.equal(
context.transaction,
RELEASED_TRANSACTION,
'the drained iterator must also release the context’s back-reference'
);
assert.ok(await LingerTable.get('linger-write'), 'releasing the read handle must not disturb the committed write');
// audit/txn-log entries batch on the native transaction they were staged into and are only
// written by its commit; the replay must re-stage them into ITS transaction — the original
Expand Down Expand Up @@ -143,14 +147,14 @@ describe('commit with open read iterators commits writes immediately on a replay
assert.equal(await LingerTable.get('linger-fail'), undefined, 'the failed write is not committed');
// A terminal commit failure is just as final as a success: releaseContext() defers the
// context release the same way (pendingContextRelease) until the still-open iterator drains,
// so capture the wrapper before draining it — draining deletes context.transaction itself.
// so capture the wrapper before draining it — draining replaces context.transaction itself.
const closedTxn = context.transaction;
// the original read handle must survive the replay failure: the iterator finishes normally
while (!(await iterator.next()).done);
assert.equal(closedTxn.transaction, null, 'the drained iterator must still release the native handle');
assert.equal(
context.transaction,
null,
RELEASED_TRANSACTION,
'the drained iterator must also release the context’s back-reference, even after a terminal commit failure'
);
await delay(100);
Expand Down
33 changes: 29 additions & 4 deletions unitTests/resources/operationContextTransactionLeak.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const { table } = require('#src/resources/databases');
const { setMainIsWorker } = require('#js/server/threads/manageThreads');
const serverUtilities = require('#src/server/serverHelpers/serverUtilities');
const { contextStorage } = require('#src/resources/transaction');
const { TRANSACTION_STATE, DatabaseTransaction } = require('#src/resources/DatabaseTransaction');
const { TRANSACTION_STATE, DatabaseTransaction, RELEASED_TRANSACTION } = require('#src/resources/DatabaseTransaction');

// Regression coverage for a transaction-context leak exposed by issue #1591/#1592 (ambient user
// context for operation handlers) and confirmed against a real 2-node cluster-formation scenario
Expand Down Expand Up @@ -200,17 +200,17 @@ describe('Ambient operation context must not couple independent writes (transact
const [afterFirst, afterSecond, afterThird] = transactionsSeenAfterEachWrite;
assert.strictEqual(
afterFirst,
null,
RELEASED_TRANSACTION,
"the first write's transaction must be released from the ambient context once its commit completes"
);
assert.strictEqual(
afterSecond,
null,
RELEASED_TRANSACTION,
"the second write's transaction must likewise be released, not left attached for a later write to observe"
);
assert.strictEqual(
afterThird,
null,
RELEASED_TRANSACTION,
"the third write's transaction must likewise be released, for the same reason"
);

Expand All @@ -237,4 +237,29 @@ describe('Ambient operation context must not couple independent writes (transact
'the third write must not join the second write’s transaction instance, for the same reason'
);
});

// The delivery path: an operations-API handler under processLocalTransaction's ambient context reads
// through that context and then commits it itself. transaction.test.js covers the same shape on a
// bare `{}` context; only this one exercises the real ambient operation context.
it('lets an operation handler commit its ambient context after a static read completed that context’s transaction', async () => {
let committed = false;
const result = await serverUtilities.processLocalTransaction(
{ body: { operation: 'test_registered_op', hdb_user: { username: 'internal_bookkeeping' } } },
async () => {
const context = contextStorage.getStore();
await LeakTable.get('handler-commit-target');
// getUserPermissions()'s shape: bound the transaction the reads above ran in, then carry on.
await context.transaction.commit();
committed = true;
await LeakTable.put('handler-commit-target', { name: 'written after the handler commit' });
await context.transaction.commit();
return { message: 'ok' };
}
);
assert.equal(result?.message, 'ok', 'the handler must not fail on its own mid-handler commit');
assert.ok(committed);
const record = await LeakTable.get('handler-commit-target');
assert.ok(record, 'the write made after the handler’s own commit must persist');
assert.equal(record.name, 'written after the handler commit');
});
});
Loading
Loading