diff --git a/src/brainy.ts b/src/brainy.ts index 7568a6f3..39b604ad 100644 --- a/src/brainy.ts +++ b/src/brainy.ts @@ -767,6 +767,34 @@ export class Brainy implements BrainyInterface { private _persistIdleTimer: ReturnType | null = null private _persistBackgroundFlight: Promise | null = null + /** + * FLUSH IS SINGLE-FLIGHT, AND THE QUEUE IS ONE DEEP. `_flushInFlight` is the + * flush body actually running; `_flushFollowUp` is the AT MOST ONE flush + * queued behind it. Every caller — the write cadence, the cross-process + * flush-request watcher, an application calling `flush()` directly — either + * runs (nothing in flight), or joins the single queued follow-up. + * + * WHY A FOLLOW-UP RATHER THAN JOINING THE RUNNING FLUSH: a caller flushes to + * make ITS writes durable, and those writes may have landed after the + * running flush read its state. Joining would return "flushed" over data + * that was never persisted. Chaining one follow-up costs nothing when there + * is nothing new (a clean brain's flush returns immediately — see + * `_dirtySinceLastFlush`) and is correct when there is. + * + * MEASURED, in the production shutdown this was written for: two + * "Flushing Brainy indexes and caches to disk..." runs overlapping 3s + * apart on one brain, their walls growing 295ms → 4.9s as they contended + * for the same providers. + */ + private _flushInFlight: Promise | null = null + private _flushFollowUp: Promise | null = null + /** Flush bodies that got past the single-flight gate (pinned by tests). */ + private _flushBodyRuns = 0 + /** Flush bodies running right now, and the high-water mark — which the + * single-flight law requires to stay at 1 (pinned by tests). */ + private _flushBodiesActive = 0 + private _flushConcurrencyPeak = 0 + // DEFERRED EMBEDDING (MT5): pending markers are LOG RECORDS — an // embed.pending record rides the deferred write's own commit fact and // embed.landed rides the landing commit; this set is the in-memory @@ -889,6 +917,24 @@ export class Brainy implements BrainyInterface { // applies only to instances that were never closed. private closed = false + /** + * THE ONE CLOSE. Set SYNCHRONOUSLY by the first `close()` call, before that + * call yields, and never cleared — close is terminal. Every later or + * concurrent caller receives this same promise, so a shutdown with two + * callers (a host's pool close and the engine's own signal handler) runs + * ONE teardown, not two. + * + * MEASURED, the day this was added: a host that owns shutdown called + * `close()` on every pooled store at SIGTERM while the engine's signal + * handler flushed the same instances in parallel and released their writer + * locks in its own `finally`. One store took 149s to close (148s of it + * silent) against 24s for its idle siblings, and the same race in a local + * reproduction printed `Writer fence lost … the lock file is gone` — the + * handler observing a lock the close it was racing had already released. + * Two owners of one shutdown; now there is one, whoever calls first. + */ + private _closeInFlight: Promise | null = null + // Index-build-at-open state. `lazyRebuildCompleted` predates the health-gate // law (it named a first-QUERY lazy rebuild) and stays for `getIndexStatus()` // API compatibility, but its truth changed: a needed rebuild now runs @@ -2076,105 +2122,88 @@ export class Brainy implements BrainyInterface { */ private registerShutdownHooks(): void { /** - * The signal-path shutdown. THREE LAWS, each written by a production - * shutdown that looked clean and wasn't: + * The signal-path shutdown. ONE OWNER PER BRAIN, AND THE PATH IS `close()`. * - * 1. PER-INSTANCE ISOLATION. This used to be one `try` around a loop over - * every open brain: the first instance whose flush rejected aborted the - * loop, so every remaining brain kept its writer lock and its unwritten - * markers — and the process still exited 0. A pool of brains failed in - * a batch, not one at a time. - * 2. THE MARKER IS PART OF SHUTDOWN. Flushing the indexes without closing - * the generation store leaves the clean-shutdown marker unwritten, so - * the NEXT open reads the store as crashed and folds the whole - * generation log — measured in tens of seconds on a real store, paid on - * every restart, after a shutdown the operator saw exit 0. - * 3. THE LOCK IS ALWAYS GIVEN UP. In a `finally`, per instance: a process - * on its way out holds nothing. + * WHAT THIS REPLACED, and why. The handler used to run its own shutdown — + * a parallel per-component flush, the generation store's close, a second + * parallel round of component closes, and a `finally` that stopped the + * flush-request watcher and released the writer lock. That is a SECOND + * teardown of the same brain, and a host application with its own SIGTERM + * handler (the shape every pooled deployment has) ran the FIRST one at the + * same moment. MEASURED in production the day this changed: a host closing + * seven pooled stores at SIGTERM printed "Shutdown signal received - + * flushing pending data...", went silent for 148s, printed "Flushed + * successfully (1 instance)", and the host's own close of that same store + * returned 1s later — 149s, against 24s for the six stores with no engine + * work in flight. The same race reproduced locally as + * `Failed to flush one Brainy instance on shutdown: Writer fence lost … + * the lock file is gone`: this handler observing a lock that the close it + * was racing had already released. + * + * SO: defer one macrotask, then per instance either STEP ASIDE (a close + * has begun or finished — its owner owns the flush, the markers and the + * lock) or `await instance.close()` — the one durable path, identical to + * what any caller gets. The three laws the old block carried are all + * satisfied by `close()`, each verified against its code: + * + * 1. PER-INSTANCE ISOLATION — kept HERE, in the per-instance try/catch + * below: one brain's failed close never aborts the loop over the rest. + * (`close()` itself is per-instance by construction.) + * 2. THE MARKER IS PART OF SHUTDOWN — `close()` → `closeDurableSteps()` + * Phase 1 awaits `this.generationStore.close()`, which persists the + * counter, advances the fold checkpoint and stamps the clean-shutdown + * marker LAST. That is the step that decides adopt-vs-fold at the next + * open, and it is the same call the old block made. + * 3. THE LOCK IS ALWAYS GIVEN UP — `close()`'s terminal releases run + * whether the durable steps threw or not (its contract: "TWO PARTS, AND + * THE SECOND IS UNCONDITIONAL"): `stopFlushRequestWatcher()` then + * `releaseWriterLock()`, then the VFS shutdown and the terminal + * `closed` flag, and only then is the original failure rethrown. + * `close()` releases the lock in MORE cases than the old block did — it + * also drains the metadata write buffer first, so no pending write can + * land after a successor writer claims the lock. */ - const flushOnShutdown = async () => { + const closeOnShutdown = async () => { console.log('Shutdown signal received - flushing pending data...') - let flushedCount = 0 + // DEFER ONE MACROTASK. A host application registers its own listener on + // the same signal, and Node runs listeners in registration order — ours + // is usually first, because the brain was opened before the host wired + // its shutdown. Yielding once lets every other listener for this signal + // run its synchronous prologue, so a host that calls close() gets to be + // the owner. It is only a courtesy, never the safety: close()'s own + // single-flight gate is what makes a lost race harmless. + await new Promise((resolve) => setImmediate(resolve)) + + let closedCount = 0 + let deferredCount = 0 let failedCount = 0 // Snapshot: close() splices Brainy.instances while we iterate. for (const instance of [...Brainy.instances]) { if (!instance.initialized) continue + // SOMEONE ELSE OWNS THIS ONE. Not a flush, not a lock release, not a + // component close — nothing. Touching a brain whose close is running + // is the whole defect this handler was rewritten for. + if (instance.closed || instance._closeInFlight !== null) { + deferredCount++ + continue + } try { - // Flush all buffered data (parallel across components, this brain only). - await Promise.all([ - (async () => { - if (instance.storage && typeof instance.storage.flushCounts === 'function') { - await instance.storage.flushCounts() - } - })(), - (async () => { - if (instance.metadataIndex && typeof instance.metadataIndex.flush === 'function') { - await instance.metadataIndex.flush() - } - })(), - (async () => { - if (instance.graphIndex && typeof instance.graphIndex.flush === 'function') { - await instance.graphIndex.flush() - } - })(), - (async () => { - if (instance.index && typeof instance.index.flush === 'function') { - await instance.index.flush() - } - })() - ]) - - // Close the generation store: persists the counter, advances the - // fold checkpoint, and stamps the clean-shutdown marker LAST — the - // one step that decides whether the next open adopts or folds. Law 2. - if (instance.generationStore && !instance.isReadOnly) { - await instance.generationStore.close() - } - - // Close components to stop timers that would prevent clean process exit - await Promise.all([ - (async () => { - if (instance.graphIndex && typeof instance.graphIndex.close === 'function') { - await instance.graphIndex.close() - } - })(), - (async () => { - const index = instance.index as JsHnswVectorIndex & VectorIndexOptionalHooks - if (index && typeof index.close === 'function') { - await index.close() - } - })(), - (async () => { - const metadataIndex = instance.metadataIndex as MetadataIndexManager & MetadataIndexOptionalHooks - if (metadataIndex && typeof metadataIndex.close === 'function') { - await metadataIndex.close() - } - })() - ]) - flushedCount++ + // Law 1: this try/catch is the isolation — the loop continues. + await instance.close() + closedCount++ } catch (error) { failedCount++ - console.error('Failed to flush one Brainy instance on shutdown:', error) - } finally { - // Law 3 — the lock and the watcher go regardless. - try { - if (instance.storage && typeof instance.storage.stopFlushRequestWatcher === 'function') { - instance.storage.stopFlushRequestWatcher() - } - } catch (error) { - console.error('Failed to stop the flush-request watcher on shutdown:', error) - } - try { - if (instance.storage && typeof instance.storage.releaseWriterLock === 'function') { - await instance.storage.releaseWriterLock() - } - } catch (error) { - console.error('Failed to release the writer lock on shutdown:', error) - } + console.error('Failed to close one Brainy instance on shutdown:', error) } } - if (flushedCount > 0) { - console.log(`Flushed successfully (${flushedCount} instance${flushedCount > 1 ? 's' : ''})`) + if (closedCount > 0) { + console.log(`Flushed successfully (${closedCount} instance${closedCount > 1 ? 's' : ''})`) + } + if (deferredCount > 0) { + console.log( + `${deferredCount} Brainy instance${deferredCount > 1 ? 's are' : ' is'} already ` + + `closing — left to the caller that owns that close.` + ) } if (failedCount > 0) { console.error( @@ -2201,19 +2230,29 @@ export class Brainy implements BrainyInterface { * markers unwritten. When the host has its own handler (listener count * above our own), the host owns the exit; Brainy only makes its data * durable and steps aside. + * + * THE COUNT IS TAKEN WHEN THE SIGNAL ARRIVES, not after the shutdown ran. + * "Is anyone else handling this signal?" is a question about the moment + * the signal landed. Asking afterwards reads a process that has already + * torn itself down: the handler now CLOSES its instances, and closing the + * last brain deregisters Brainy's own listeners — so a host application's + * single remaining listener would look like `<= 1` and get force-exited + * out of its own graceful shutdown, precisely the failure above. */ - const exitIfSoleShutdownOwner = (signal: 'SIGTERM' | 'SIGINT'): void => { - if (process.listenerCount(signal) <= 1) { + const exitIfSoleShutdownOwner = (ownersWhenSignalled: number): void => { + if (ownersWhenSignalled <= 1) { process.exit(0) } } Brainy.sigtermListener = async () => { - await flushOnShutdown() - exitIfSoleShutdownOwner('SIGTERM') + const owners = process.listenerCount('SIGTERM') + await closeOnShutdown() + exitIfSoleShutdownOwner(owners) } Brainy.sigintListener = async () => { - await flushOnShutdown() - exitIfSoleShutdownOwner('SIGINT') + const owners = process.listenerCount('SIGINT') + await closeOnShutdown() + exitIfSoleShutdownOwner(owners) } Brainy.beforeExitListener = async () => { // Self-deregister FIRST: Node re-emits 'beforeExit' after every event- @@ -2225,7 +2264,7 @@ export class Brainy implements BrainyInterface { process.off('beforeExit', Brainy.beforeExitListener) Brainy.beforeExitListener = undefined } - await flushOnShutdown() + await closeOnShutdown() } process.on('SIGTERM', Brainy.sigtermListener) process.on('SIGINT', Brainy.sigintListener) @@ -2298,6 +2337,33 @@ export class Brainy implements BrainyInterface { return this.initialized } + /** + * @description Whether `close()` has BEGUN on this instance — in flight or + * already finished. The question a shutdown owner asks: this brain's + * teardown belongs to whoever started it, and a second party must not flush + * its components or release its writer lock underneath it. + * + * True from the synchronous moment `close()` is entered, so a listener that + * yields a tick and comes back reads the truth, not a stale "not yet". + * @returns `true` once a close has started. + */ + get isClosing(): boolean { + return this._closeInFlight !== null + } + + /** + * @description Whether `close()` has FINISHED tearing this instance down — + * durable steps attempted, writer lock released, instance terminal. A + * closed brain never re-initializes; every operation on it throws. + * + * True after a close that FAILED partway, too: such a brain still holds no + * writer lock and still serves nothing (see {@link close}). + * @returns `true` once the teardown has completed. + */ + get isClosed(): boolean { + return this.closed + } + /** * Promise that resolves when Brainy is fully initialized and ready to use * @@ -3271,9 +3337,18 @@ export class Brainy implements BrainyInterface { * toward the next trigger. A failure is LOUD and leaves the writes counted * again — silence is not an option, and neither is a retry storm (the next * trigger re-attempts). + * + * COALESCING LIVES IN {@link flush}, NOT HERE. A kick that arrives while a + * flush is running used to return without doing anything — the writes it + * counted waited for some LATER trigger, and this method's guard also could + * not coalesce the flushes it does not start (the cross-process + * flush-request watcher and application `flush()` calls both go straight to + * `flush()`; two of those overlapping is exactly what production showed). + * The gate in `flush()` covers every caller: this kick now either runs the + * flush or joins the single queued follow-up, so the writes it counted are + * always someone's work, and there is still never a second concurrent run. */ private kickBackgroundFlush(reason: 'threshold' | 'idle'): void { - if (this._persistBackgroundFlight) return const counted = this._persistDirtyWrites this._persistDirtyWrites = 0 this._persistLastFlushAt = Date.now() @@ -12903,7 +12978,58 @@ export class Brainy implements BrainyInterface { * process.exit(0) * }) */ - async flush(): Promise { + flush(): Promise { + // ---- THE SINGLE-FLIGHT GATE ---- + // One flush body runs at a time, with at most ONE queued behind it. See + // `_flushInFlight` / `_flushFollowUp` for the measurement that required + // this. NOT `async`: the gate hands back the very promise the work is on, + // so joining callers share identity, not just an outcome. The gate is + // crossed BEFORE any await, so two callers in the same tick cannot both + // find the field empty. + if (this._flushInFlight) { + if (!this._flushFollowUp) { + // The running flush's failure is not this follow-up's failure: it is + // reported to ITS caller, and the queued work still gets its turn. + this._flushFollowUp = this._flushInFlight + .catch(() => {}) + .then(() => { + this._flushFollowUp = null + return this.flush() + }) + } + return this._flushFollowUp + } + const run = this._runFlush() + // `finally` and not `then`: a failed flush must still open the gate, or + // one rejection would wedge every later flush behind a promise nobody + // will ever settle. + const gated = run.finally(() => { + if (this._flushInFlight === gated) this._flushInFlight = null + }) + this._flushInFlight = gated + return gated + } + + /** + * @description The flush body — everything {@link flush} promises, run + * exactly once at a time by that method's single-flight gate. Private + * because non-overlap is part of the contract: there is no supported way to + * run two of these at once, and the counters here witness that. + * @returns Nothing. + */ + private async _runFlush(): Promise { + this._flushBodyRuns++ + this._flushBodiesActive++ + this._flushConcurrencyPeak = Math.max(this._flushConcurrencyPeak, this._flushBodiesActive) + try { + await this._flushSteps() + } finally { + this._flushBodiesActive-- + } + } + + /** @description The flush steps themselves. See {@link flush}. */ + private async _flushSteps(): Promise { await this.ensureInitialized() // Read-only instances have no buffered writes to flush. close() may call @@ -20150,11 +20276,42 @@ export class Brainy implements BrainyInterface { * * The original failure is never swallowed: it is narrated with what it costs * the next open, then rethrown to the caller. + * + * IDEMPOTENT AND RE-ENTRANT. The teardown below runs ONCE. Concurrent + * callers share the one in-flight promise and settle together; a caller + * arriving after it finished gets that same settled promise (close is + * terminal — there is nothing left to redo, and a failed close has already + * released the lock and set `closed`). This is what makes the shutdown + * ownership question answerable at all: whoever calls first owns the close, + * everyone else — including the engine's own signal handler — joins it or + * steps aside. See `_closeInFlight`. * @returns Nothing. * @throws The first failure from the durable close steps, after the * terminal releases have run. */ - async close(): Promise { + close(): Promise { + // NOT `async`: an async wrapper allocates a FRESH promise per call, so + // callers would hold different handles to the same work. Returning the + // stored promise itself makes "one close" observable identity, not just + // observable behaviour. The gate is crossed with NO await before it, so + // two callers in the same tick — and a signal handler resuming mid-close + // — always see the same answer; `isClosing` is true from this assignment + // onward. (`_closeOnce()` is async, so a failure is always a rejection, + // never a synchronous throw out of this method.) + if (this._closeInFlight) return this._closeInFlight + const run = this._closeOnce() + this._closeInFlight = run + return run + } + + /** + * @description The close body — everything {@link close} promises, run + * exactly once by that method's gate. + * @returns Nothing. + * @throws The first failure from the durable close steps, after the + * terminal releases have run. + */ + private async _closeOnce(): Promise { if (this._pendingEmbedIds.size === 0) await this.writeEmbedLowWater() let closeFailure: unknown = null try { @@ -20243,6 +20400,19 @@ export class Brainy implements BrainyInterface { if (this._persistBackgroundFlight) { await this._persistBackgroundFlight.catch(() => {}) } + // Drain the flush chain itself: the running flush AND the single follow-up + // queued behind it. The cadence's own handle above covers only the flushes + // the cadence started — a flush-request from another process, or an + // application's own flush() racing this close, is on the chain and nowhere + // else, and a flush landing mid-close writes behind the close's work. + // Bounded by construction: at most one follow-up exists, and awaiting it + // awaits its leader too, so the second pass is a no-op unless a writer + // raced this close. + for (let pass = 0; pass < 2; pass++) { + const chain = this._flushFollowUp ?? this._flushInFlight + if (!chain) break + await chain.catch(() => {}) + } // Cancel any pending post-import background deduplication FIRST — it is a // writer (merge-deletes), and no delete pass may start mid- or post-close.