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.
This commit is contained in:
parent
22702b81c0
commit
003e2a74ea
7 changed files with 199 additions and 76 deletions
|
|
@ -121,9 +121,13 @@ export interface TransactOptions {
|
||||||
* with the batch: `max(30 000, opCount × 2 000)` — production imports on
|
* 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
|
* 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
|
* silently capped honest bulk work at ~15 operations. A tripped budget
|
||||||
* rolls the whole batch back and throws a retryable
|
* rolls the whole batch back and throws a `TransactionTimeoutError` naming
|
||||||
* `TransactionTimeoutError` naming the operation it stopped at, the batch
|
* the operation it stopped at, the batch size, and the elapsed/budget
|
||||||
* size, and the elapsed/budget times.
|
* 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
|
timeoutMs?: number
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -62,9 +62,10 @@ const DEFAULT_BUDGET_FLOOR_MS = 30_000
|
||||||
* NEXT operation may start (see {@link Transaction.execute}), never whether
|
* NEXT operation may start (see {@link Transaction.execute}), never whether
|
||||||
* already-completed work is rolled back after the fact. A trip mid-batch
|
* 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
|
* still rolls back every operation applied so far, atomically, and throws a
|
||||||
* retryable, fully-labeled TransactionTimeoutError — that zero-loss guarantee
|
* fully-labeled `TransactionTimeoutError` — retryable-with-latch, never
|
||||||
* doesn't change; only the point at which the clock stops mattering does (at
|
* hot-retry (see its `retryable` and `hotRetryUnsafe` fields) — that
|
||||||
* the last operation, not one check later).
|
* 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 opCount - Number of operations in the batch.
|
||||||
* @param override - A full override for this call; wins over everything else.
|
* @param override - A full override for this call; wins over everything else.
|
||||||
|
|
|
||||||
|
|
@ -19,7 +19,6 @@
|
||||||
import { Transaction } from './Transaction.js'
|
import { Transaction } from './Transaction.js'
|
||||||
import {
|
import {
|
||||||
TransactionFunction,
|
TransactionFunction,
|
||||||
TransactionResult,
|
|
||||||
TransactionOptions
|
TransactionOptions
|
||||||
} from './types.js'
|
} from './types.js'
|
||||||
import { TransactionError } from './errors.js'
|
import { TransactionError } from './errors.js'
|
||||||
|
|
@ -105,34 +104,6 @@ export class TransactionManager {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
|
||||||
* Execute a transaction and return detailed result
|
|
||||||
*/
|
|
||||||
async executeTransactionWithResult<T>(
|
|
||||||
fn: TransactionFunction<T>,
|
|
||||||
options?: TransactionOptions
|
|
||||||
): Promise<TransactionResult<T>> {
|
|
||||||
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
|
* Get transaction statistics
|
||||||
*/
|
*/
|
||||||
|
|
|
||||||
|
|
@ -73,14 +73,47 @@ export class InvalidTransactionStateError extends TransactionError {
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Error for transaction timeout
|
* 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 {
|
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(
|
constructor(
|
||||||
timeoutMs: number,
|
timeoutMs: number,
|
||||||
operationIndex: number,
|
operationIndex: number,
|
||||||
telemetry?: {
|
telemetry?: {
|
||||||
|
/** Milliseconds elapsed in the transaction when the budget tripped. */
|
||||||
elapsedMs?: number
|
elapsedMs?: number
|
||||||
|
/** Total number of operations in the batch that timed out. */
|
||||||
totalOperations?: number
|
totalOperations?: number
|
||||||
|
/** Name of the operation the batch was about to start when it tripped, if named. */
|
||||||
operationName?: string
|
operationName?: string
|
||||||
}
|
}
|
||||||
) {
|
) {
|
||||||
|
|
@ -93,8 +126,16 @@ export class TransactionTimeoutError extends TransactionError {
|
||||||
telemetry?.elapsedMs !== undefined ? `${telemetry.elapsedMs}ms elapsed, ` : ''
|
telemetry?.elapsedMs !== undefined ? `${telemetry.elapsedMs}ms elapsed, ` : ''
|
||||||
super(
|
super(
|
||||||
`Transaction timed out at operation ${progress}${name} — ${elapsed}budget ${timeoutMs}ms. ` +
|
`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.`,
|
`The batch rolled back atomically; retryable after the underlying slowness resolves or ` +
|
||||||
{ timeoutMs, operationIndex, ...telemetry }
|
`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'
|
this.name = 'TransactionTimeoutError'
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1658,8 +1658,10 @@ export interface BrainyConfig {
|
||||||
* **start** — never whether already-completed work gets rolled back after
|
* **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
|
* 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
|
* or it commits). A trip mid-batch still rolls back every applied operation
|
||||||
* atomically and throws a retryable `TransactionTimeoutError`; only the
|
* atomically and throws a `TransactionTimeoutError` that is
|
||||||
* floor of the formula is configurable here.
|
* 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
|
* 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
|
* take longer than 30s per operation (e.g. page-cache-cold canonical writes
|
||||||
|
|
|
||||||
|
|
@ -5,7 +5,6 @@
|
||||||
* - High-level transaction API
|
* - High-level transaction API
|
||||||
* - Statistics tracking
|
* - Statistics tracking
|
||||||
* - Error handling
|
* - Error handling
|
||||||
* - Result wrapping
|
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, it, expect, beforeEach } from 'vitest'
|
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', () => {
|
describe('Statistics Tracking', () => {
|
||||||
it('should track total transactions', async () => {
|
it('should track total transactions', async () => {
|
||||||
await manager.executeTransaction(async (tx) => {
|
await manager.executeTransaction(async (tx) => {
|
||||||
|
|
|
||||||
141
tests/unit/transaction/timeout-never-internally-retried.test.ts
Normal file
141
tests/unit/transaction/timeout-never-internally-retried.test.ts
Normal file
|
|
@ -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<void>((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<RollbackAction | undefined> {
|
||||||
|
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<never> => {
|
||||||
|
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()
|
||||||
|
})
|
||||||
|
})
|
||||||
Loading…
Add table
Add a link
Reference in a new issue