From 003e2a74ea7dd2598022b2ca6ee3784d85214662 Mon Sep 17 00:00:00 2001 From: David Snelling Date: Fri, 24 Jul 2026 16:01:41 -0700 Subject: [PATCH] fix: transaction timeouts are a typed no-hot-retry contract; engine-side non-retry pinned; dead transaction path removed A production incident: a native-provider op ground 38-40s inside a transaction, blew the apply-phase budget, rolled back, and a downstream pipeline hot-retried the identical operation into a 6-minute CPU storm. Brainy itself never auto-retried the timeout; the gap was that TransactionTimeoutError only said "retryable" in prose, with nothing machine-readable for a caller to branch on. - TransactionTimeoutError gains two typed, always-true fields: retryable (a later attempt may succeed once the slowness resolves or the budget is raised) and hotRetryUnsafe (an immediate identical retry re-pays the full cost that just timed out and can cascade into a CPU storm -- callers must latch and back off, never loop). context's existing telemetry fields (timeoutMs, operationIndex, elapsedMs, totalOperations, operationName) are now documented as the caller's backoff inputs. - Updated the "retryable" doc-prose sites (transact()'s timeoutMs option, transactionBudgetFloorMs, Transaction.execute()'s contract) to point at the new fields instead of bare prose. - Regression pin (tests/unit/transaction/timeout-never-internally-retried.test.ts): an execution counter proves the engine never re-drives a timed-out operation, through both the single-op engine TransactionManager/Transaction drives for every single-record write, and add()'s upsert-race retry loop (which must exit on the first TransactionTimeoutError, never treat it like the lost-insert-race signal it retries on). - Removed TransactionManager.executeTransactionWithResult -- zero callers anywhere in the codebase. --- src/db/types.ts | 10 +- src/transaction/Transaction.ts | 7 +- src/transaction/TransactionManager.ts | 29 ---- src/transaction/errors.ts | 45 +++++- src/types/brainy.types.ts | 6 +- .../TransactionManager.unit.test.ts | 37 ----- .../timeout-never-internally-retried.test.ts | 141 ++++++++++++++++++ 7 files changed, 199 insertions(+), 76 deletions(-) create mode 100644 tests/unit/transaction/timeout-never-internally-retried.test.ts diff --git a/src/db/types.ts b/src/db/types.ts index 4c8a4957..a7f86a89 100644 --- a/src/db/types.ts +++ b/src/db/types.ts @@ -121,9 +121,13 @@ export interface TransactOptions { * with the batch: `max(30 000, opCount × 2 000)` — production imports on * network-attached disks measure ~2 s per operation, so a flat 30 s budget * silently capped honest bulk work at ~15 operations. A tripped budget - * rolls the whole batch back and throws a retryable - * `TransactionTimeoutError` naming the operation it stopped at, the batch - * size, and the elapsed/budget times. + * rolls the whole batch back and throws a `TransactionTimeoutError` naming + * the operation it stopped at, the batch size, and the elapsed/budget + * times. That error is retryable-with-latch, never hot-retry: its + * `retryable` field says a later attempt may succeed, its + * `hotRetryUnsafe` field says an immediate identical retry re-pays the + * full cost that just timed out — callers must latch and back off, never + * loop. */ timeoutMs?: number } diff --git a/src/transaction/Transaction.ts b/src/transaction/Transaction.ts index 79b53006..ede65e83 100644 --- a/src/transaction/Transaction.ts +++ b/src/transaction/Transaction.ts @@ -62,9 +62,10 @@ const DEFAULT_BUDGET_FLOOR_MS = 30_000 * NEXT operation may start (see {@link Transaction.execute}), never whether * already-completed work is rolled back after the fact. A trip mid-batch * still rolls back every operation applied so far, atomically, and throws a - * retryable, fully-labeled TransactionTimeoutError — that zero-loss guarantee - * doesn't change; only the point at which the clock stops mattering does (at - * the last operation, not one check later). + * fully-labeled `TransactionTimeoutError` — retryable-with-latch, never + * hot-retry (see its `retryable` and `hotRetryUnsafe` fields) — that + * zero-loss guarantee doesn't change; only the point at which the clock + * stops mattering does (at the last operation, not one check later). * * @param opCount - Number of operations in the batch. * @param override - A full override for this call; wins over everything else. diff --git a/src/transaction/TransactionManager.ts b/src/transaction/TransactionManager.ts index 0f13b6a3..5abf48ba 100644 --- a/src/transaction/TransactionManager.ts +++ b/src/transaction/TransactionManager.ts @@ -19,7 +19,6 @@ import { Transaction } from './Transaction.js' import { TransactionFunction, - TransactionResult, TransactionOptions } from './types.js' import { TransactionError } from './errors.js' @@ -105,34 +104,6 @@ export class TransactionManager { } } - /** - * Execute a transaction and return detailed result - */ - async executeTransactionWithResult( - fn: TransactionFunction, - options?: TransactionOptions - ): Promise> { - const startTime = Date.now() - const transaction = new Transaction(options) - - try { - const value = await fn(transaction) - await transaction.execute() - - const executionTimeMs = Date.now() - startTime - - return { - value, - operationCount: transaction.getOperationCount(), - executionTimeMs - } - - } catch (error) { - // Transaction failed - throw error - } - } - /** * Get transaction statistics */ diff --git a/src/transaction/errors.ts b/src/transaction/errors.ts index c270d0ed..d8382a4b 100644 --- a/src/transaction/errors.ts +++ b/src/transaction/errors.ts @@ -73,14 +73,47 @@ export class InvalidTransactionStateError extends TransactionError { /** * Error for transaction timeout + * + * Machine-readable no-hot-retry contract: {@link retryable} and + * {@link hotRetryUnsafe} are both always `true` on this class — they exist + * so a caller can branch on the *shape* of the error instead of parsing + * message text. Read them together: the operation may eventually succeed, + * but never by looping on it immediately. + * + * `context` (inherited from {@link TransactionError}) carries the caller's + * backoff inputs — see the field docs below. */ export class TransactionTimeoutError extends TransactionError { + /** + * The failed operation MAY succeed on a later attempt — once the + * underlying slowness resolves (e.g. a cold page cache warms up) or the + * budget is deliberately raised (`transactionBudgetFloorMs`, or a larger + * `timeoutMs` override on the batch). This is a statement about eventual + * retryability, not a license to retry now — see {@link hotRetryUnsafe}. + */ + public readonly retryable = true + + /** + * An immediate, identical retry re-pays the FULL cost of the work that + * just timed out — it does not resume partway. Looping on this error + * (hot-retrying) repeats that full cost every attempt and can cascade + * into a CPU/resource storm on the caller's side. Callers MUST latch: on + * this error, record `{ at: Date.now(), error }`, surface one loud + * failure to their own caller, and hold a cooldown window before any + * re-attempt (clearing the latch only on success). Never retry this error + * in a tight loop. + */ + public readonly hotRetryUnsafe = true + constructor( timeoutMs: number, operationIndex: number, telemetry?: { + /** Milliseconds elapsed in the transaction when the budget tripped. */ elapsedMs?: number + /** Total number of operations in the batch that timed out. */ totalOperations?: number + /** Name of the operation the batch was about to start when it tripped, if named. */ operationName?: string } ) { @@ -93,8 +126,16 @@ export class TransactionTimeoutError extends TransactionError { telemetry?.elapsedMs !== undefined ? `${telemetry.elapsedMs}ms elapsed, ` : '' super( `Transaction timed out at operation ${progress}${name} — ${elapsed}budget ${timeoutMs}ms. ` + - `The batch rolled back atomically; retry with a higher timeoutMs or a smaller batch.`, - { timeoutMs, operationIndex, ...telemetry } + `The batch rolled back atomically; retryable after the underlying slowness resolves or ` + + `the budget is raised, but hot-retry-unsafe — latch and back off, never loop.`, + { + // Caller backoff inputs — all present on every instance: + /** Configured budget (ms) that was exceeded. */ + timeoutMs, + /** Index of the operation the batch was about to start when it tripped. */ + operationIndex, + ...telemetry + } ) this.name = 'TransactionTimeoutError' } diff --git a/src/types/brainy.types.ts b/src/types/brainy.types.ts index 8bede8d3..b5f286ed 100644 --- a/src/types/brainy.types.ts +++ b/src/types/brainy.types.ts @@ -1658,8 +1658,10 @@ export interface BrainyConfig { * **start** — never whether already-completed work gets rolled back after * the fact (a single-op write can never time out post-hoc: it either runs * or it commits). A trip mid-batch still rolls back every applied operation - * atomically and throws a retryable `TransactionTimeoutError`; only the - * floor of the formula is configurable here. + * atomically and throws a `TransactionTimeoutError` that is + * retryable-with-latch, never hot-retry (see its `retryable` and + * `hotRetryUnsafe` fields); only the floor of the formula is configurable + * here. * * Raise this when a cold store's first writes after a restart legitimately * take longer than 30s per operation (e.g. page-cache-cold canonical writes diff --git a/tests/transaction/TransactionManager.unit.test.ts b/tests/transaction/TransactionManager.unit.test.ts index 86b1692c..29e7f7ae 100644 --- a/tests/transaction/TransactionManager.unit.test.ts +++ b/tests/transaction/TransactionManager.unit.test.ts @@ -5,7 +5,6 @@ * - High-level transaction API * - Statistics tracking * - Error handling - * - Result wrapping */ import { describe, it, expect, beforeEach } from 'vitest' @@ -84,42 +83,6 @@ describe('TransactionManager', () => { }) }) - describe('executeTransactionWithResult', () => { - it('should return detailed result', async () => { - const result = await manager.executeTransactionWithResult(async (tx) => { - tx.addOperation({ - execute: async () => { - await new Promise(resolve => setTimeout(resolve, 1)) - return async () => {} - } - }) - tx.addOperation({ execute: async () => undefined }) - return 'success' - }) - - expect(result.value).toBe('success') - expect(result.operationCount).toBe(2) - expect(result.executionTimeMs).toBeGreaterThanOrEqual(0) - }) - - it('should measure execution time', async () => { - const result = await manager.executeTransactionWithResult(async (tx) => { - tx.addOperation({ - execute: async () => { - await new Promise(resolve => setTimeout(resolve, 25)) - return async () => {} - } - }) - return 'done' - }) - - // Timer coalescing can fire a setTimeout up to a few ms EARLY under - // load, so assert well below the sleep — this tests that time is - // MEASURED, not the OS timer's precision. - expect(result.executionTimeMs).toBeGreaterThanOrEqual(20) - }) - }) - describe('Statistics Tracking', () => { it('should track total transactions', async () => { await manager.executeTransaction(async (tx) => { diff --git a/tests/unit/transaction/timeout-never-internally-retried.test.ts b/tests/unit/transaction/timeout-never-internally-retried.test.ts new file mode 100644 index 00000000..a43a81a8 --- /dev/null +++ b/tests/unit/transaction/timeout-never-internally-retried.test.ts @@ -0,0 +1,141 @@ +/** + * @module tests/unit/transaction/timeout-never-internally-retried + * @description Regression pin for the no-hot-retry contract (8.10.1). + * + * A production incident: a native-provider op ground 38-40s inside a + * transaction, blew the ~32s budget, was rolled back, and a CONSUMER pipeline + * hot-retried the identical operation into a 6-minute 100%-CPU storm. + * Investigation established brainy itself never auto-retries a + * `TransactionTimeoutError` — the storm was entirely the consumer's hot-retry + * loop, driven by a "retryable" doc-prose claim with no machine-readable + * contract. This file pins the brainy-side half of that story so it can never + * regress silently: + * + * (i) the underlying engine (`TransactionManager.executeTransaction()` → + * `Transaction.execute()`) — the exact machinery every single-record + * write (`add`/`update`/`remove`/...) drives via + * `Brainy.persistSingleOp()` — never internally re-executes a timed-out + * operation, and the error it surfaces carries `retryable === true` and + * `hotRetryUnsafe === true` (see `src/transaction/errors.ts`). + * + * Constructed directly (mirrors the existing + * `tests/unit/transaction/timeout-rollback.test.ts` pattern) rather than + * through a real `brain.add()` call: `transactTimeoutBudget()` floors + * every single-op write's budget at `opCount * 2000`ms with NO override + * seam (`transactionBudgetFloorMs` only RAISES that floor — it cannot + * lower it below the per-op-count term), so getting a real `add()` to + * time out requires a multi-second sleep. The engine-level + * `options.timeout` override used here is the exact same + * `TransactionManager`/`Transaction` code `persistSingleOp` calls — + * pinning it here pins add()'s guarantee without paying that wall-clock + * cost. + * + * (ii) `Brainy.add()`'s upsert-race retry loop (src/brainy.ts, + * `MAX_UPSERT_ATTEMPTS = 10`) — proving the loop's `catch` treats a + * `TransactionTimeoutError` as terminal (immediate rethrow) rather than + * the `InsertPreconditionExistsSignal` it retries on, so a mid-flight + * timeout can never be silently swallowed and re-attempted up to 10 + * times. + */ +import { describe, it, expect } from 'vitest' +import { TransactionManager } from '../../../src/transaction/TransactionManager.js' +import type { Operation, RollbackAction } from '../../../src/transaction/types.js' +import { TransactionTimeoutError } from '../../../src/transaction/errors.js' +import { Brainy } from '../../../src/brainy.js' +import { NounType } from '../../../src/types/graphTypes.js' + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)) + +// Brainy's ValidationConfig fixes vectors at exactly 384 dimensions +// (src/utils/paramValidation.ts) — match it so add() doesn't reject test data. +const DIM = 384 +const V = (): number[] => Array(DIM).fill(0.1) + +/** An operation that counts every `execute()` invocation — the re-drive detector. */ +function countingOp(opts: { delayMs?: number; name: string }): Operation & { calls: number } { + const op = { + name: opts.name, + calls: 0, + async execute(): Promise { + op.calls++ + if (opts.delayMs) await sleep(opts.delayMs) + return async () => {} + } + } + return op +} + +describe('transaction timeouts are never internally re-driven (8.10.1 no-hot-retry contract)', () => { + it('(i) the single-op engine (TransactionManager.executeTransaction / Transaction.execute — what add() drives via persistSingleOp) runs the overrun operation EXACTLY once and surfaces ONE retryable+hotRetryUnsafe error', async () => { + const manager = new TransactionManager() + const op0 = countingOp({ name: 'op0-overruns-budget', delayMs: 30 }) + const op1 = countingOp({ name: 'op1-must-never-start' }) + + let caught: unknown + try { + await manager.executeTransaction( + async (tx) => { + tx.addOperation(op0) + tx.addOperation(op1) + }, + // Tiny explicit override — the same override seam `transact()` + // exposes as `options.timeoutMs`; wins outright over the + // opCount*2000 floor that gates every real single-op write + // (transactTimeoutBudget()'s override semantics). + { timeout: 5 } + ) + } catch (err) { + caught = err + } + + expect(caught).toBeInstanceOf(TransactionTimeoutError) + const err = caught as TransactionTimeoutError + // The machine-readable contract callers branch on instead of parsing + // message text (src/transaction/errors.ts). + expect(err.retryable).toBe(true) + expect(err.hotRetryUnsafe).toBe(true) + + // The re-drive assertion: op0 (the one that overran) executed EXACTLY + // once — nothing inside TransactionManager/Transaction looped back and + // re-ran it — and op1 never started at all (the budget gate stopped it + // before it began, per Transaction.execute()'s per-operation loop). + expect(op0.calls).toBe(1) + expect(op1.calls).toBe(0) + }) + + it('(ii) add()\'s upsert-race retry loop (MAX_UPSERT_ATTEMPTS=10) exits on the FIRST TransactionTimeoutError — attempt counter stays at 1, never mistaken for the lost-insert-race signal it retries on', async () => { + const brain = new Brainy({ + requireSubtype: false, + storage: { type: 'memory' }, + silent: true + }) + await brain.init() + + let persistSingleOpCalls = 0 + const timeoutError = new TransactionTimeoutError(5, 1, { + elapsedMs: 6, + totalOperations: 2, + operationName: 'SaveNounMetadata' + }) + // Stub the private commit seam add() drives (persistSingleOp) to throw + // the exact error type the real engine surfaces on a mid-flight timeout. + // This test pins the upsert loop's EXCEPTION-HANDLING contract (does it + // retry a TransactionTimeoutError like it retries + // InsertPreconditionExistsSignal?), not the timing mechanics of a real + // timeout — those are pinned by test (i) and by + // tests/unit/transaction/timeout-rollback.test.ts. + ;(brain as any).persistSingleOp = async (): Promise => { + persistSingleOpCalls++ + throw timeoutError + } + + await expect( + brain.add({ data: 'a', type: NounType.Thing, vector: V() }) + ).rejects.toBe(timeoutError) + + // The loop's attempt counter: exactly one call, never retried up to + // MAX_UPSERT_ATTEMPTS. + expect(persistSingleOpCalls).toBe(1) + await brain.close() + }) +})