fix(storage): a clean close is recorded, and the writer lock is always given up
A production restart made this necessary: a service stopped with exit code 0,
having awaited close() on every pooled brain, and its next boot announced
"Overwriting stale writer lock ... appears dead" for every store it owned.
Nothing had crashed. "The recorded pid is gone" is equally true of an orderly
restart and of a crash, so the verdict could not tell an operator which one
they had — and when the OS recycles a pid it fails the other way, refusing to
open a store whose writer died days ago.
Three changes, all at the law:
- close() is two parts, and the second is unconditional. The durable steps
(flush, markers, component close, plugin deactivate, buffer drain) move to
closeDurableSteps(); the terminal releases — the flush-request watcher, the
WRITER LOCK, the VFS timers, the terminal `closed` flag — always run. The
original failure is narrated with what it costs the next open, then rethrown.
- releaseWriterLock() writes a CLEAN-CLOSE RECORD (`locks/_writer.close`)
naming the lock generation it released; the next claim consumes it, so a
record can never vouch for a later crash. An open reads the record instead
of guessing: recorded → nothing to recover; absent → say so, and name the
crash recovery this open will now run.
- The signal path stops failing in a batch. It was one try around a loop over
every open brain, so the first instance whose flush rejected stranded every
remaining brain's lock and markers — at exit code 0. Now: per-instance
isolation, the generation store's close (the clean-shutdown marker, without
which the next open folds the whole log) is part of shutdown, the lock is
given up in a finally, and the handler no longer calls process.exit() when
the host application has its own signal handler — that race truncated the
host's own close() mid-flight.
Pins: tests/integration/writer-lock-clean-close.test.ts — completed close
leaves no lock and a consumed-once record with a silent reopen; a failing
durable step still releases and still rethrows; SIGKILL leaves the lock with
no record and the reopen names the crash; a host SIGTERM handler runs to
completion.
Branch plan (10 lines):
1. writer lock: clean-close record + always-release close [this commit]
2. open narration: an always-on channel; production clamps prodLog to ERROR,
which is why a three-minute open printed nothing
3. open narration: per-phase lines as each phase ENDS, with progress cadence
4. measure both real-store fixtures on the box, before/after
5. move the generation-log fold out of the foreground where the serving law
allows; durable resumable progress marker
6. same for the VFS bootstrap
7. counts: a legacy container-rule ledger must not keep serving wrong
denominators; counts.json written atomically
8. counts pin with scar directories; two copies of one archive agree
9. docs/canonical-layout-ratification.md — 12 facts confirmed/corrected
10. report: MEASURED before/after, findings, and whether this is 10.4.4
This commit is contained in:
parent
38c3397b60
commit
e652162c1f
4 changed files with 641 additions and 108 deletions
|
|
@ -14,7 +14,8 @@ import {
|
|||
StorageBatchConfig,
|
||||
SYSTEM_DIR,
|
||||
STATISTICS_KEY,
|
||||
WriterLockInfo
|
||||
WriterLockInfo,
|
||||
WriterCloseRecord
|
||||
} from '../baseStorage.js'
|
||||
import { getBrainyVersion } from '../../utils/index.js'
|
||||
import { isAbsentError } from '../../utils/errorClassification.js'
|
||||
|
|
@ -99,6 +100,13 @@ export class FileSystemStorage extends BaseStorage {
|
|||
// timer rewrites the lock every 10s so stale-lock detection can tell a dead
|
||||
// writer from a slow one. The constant name matches the file path used.
|
||||
private static readonly WRITER_LOCK_FILE = '_writer.lock'
|
||||
/**
|
||||
* The clean-close record at `locks/_writer.close` (see
|
||||
* {@link WriterCloseRecord}). Written when the lock is released, consumed by
|
||||
* the next claim, so an open can distinguish "the previous writer left" from
|
||||
* "the previous writer died" without inferring either from a pid.
|
||||
*/
|
||||
private static readonly WRITER_CLOSE_FILE = '_writer.close'
|
||||
private static readonly WRITER_HEARTBEAT_MS = 10_000
|
||||
private static readonly WRITER_STALE_THRESHOLD_MS = 60_000
|
||||
private writerLockHeartbeat?: NodeJS.Timeout
|
||||
|
|
@ -1902,11 +1910,24 @@ export class FileSystemStorage extends BaseStorage {
|
|||
rootDir: this.rootDir
|
||||
}
|
||||
await this.writeFileAtomic(lockFile, JSON.stringify(info, null, 2))
|
||||
await this.clearWriterCloseRecord()
|
||||
this.installWriterLock(info)
|
||||
return info
|
||||
}
|
||||
|
||||
const stale = !options?.force && (await this.isWriterLockStale(existing))
|
||||
// THE CLEAN-CLOSE RECORD IS CONSULTED FIRST (see WriterCloseRecord).
|
||||
// A lock file whose release was RECORDED is bookkeeping left behind by
|
||||
// an orderly shutdown, not evidence of a crash — take it over calmly
|
||||
// and say so. Only when no record vouches for this lock do we fall
|
||||
// back to inferring liveness from the pid, and then we say THAT
|
||||
// honestly too: an unrecorded lock means the writer did not complete
|
||||
// its close, so the store was not closed cleanly and this open pays
|
||||
// recovery.
|
||||
const closeRecord = await this.readWriterCloseRecord()
|
||||
const releasedCleanly =
|
||||
closeRecord !== null && this.closeRecordVouchesFor(closeRecord, existing)
|
||||
const stale =
|
||||
releasedCleanly || (!options?.force && (await this.isWriterLockStale(existing)))
|
||||
if (!options?.force && !stale) {
|
||||
// Consumer-facing error contract: callers detect this case via
|
||||
// err.code and read the holder's details from err.lockInfo.
|
||||
|
|
@ -1917,8 +1938,16 @@ export class FileSystemStorage extends BaseStorage {
|
|||
options?.force
|
||||
? `[brainy] Force-overwriting writer lock for ${this.rootDir} ` +
|
||||
`(was held by PID ${existing.pid} on ${existing.hostname}).`
|
||||
: `[brainy] Overwriting stale writer lock for ${this.rootDir} ` +
|
||||
`(PID ${existing.pid} on ${existing.hostname} appears dead).`
|
||||
: releasedCleanly
|
||||
? `[brainy] Clearing the leftover writer lock for ${this.rootDir} — ` +
|
||||
`PID ${existing.pid} on ${existing.hostname} RELEASED it cleanly at ` +
|
||||
`${closeRecord!.closedAt} but could not remove the file. ` +
|
||||
`Nothing to recover.`
|
||||
: `[brainy] Overwriting stale writer lock for ${this.rootDir} ` +
|
||||
`(PID ${existing.pid} on ${existing.hostname} is gone and left NO ` +
|
||||
`clean-close record — that writer did not finish closing, so this ` +
|
||||
`store was not closed cleanly; open will run crash recovery and ` +
|
||||
`report its wall).`
|
||||
)
|
||||
// Takeover: verify the file still holds the lock we judged (a live
|
||||
// successor may have claimed meanwhile), then remove it and fall
|
||||
|
|
@ -1972,6 +2001,12 @@ export class FileSystemStorage extends BaseStorage {
|
|||
await fs.promises.unlink(claimTmp).catch(() => {})
|
||||
}
|
||||
|
||||
// CONSUME the previous writer's clean-close record. It described the
|
||||
// lock generation that just ended; leaving it in place would let it
|
||||
// vouch for OUR lock if this process later dies without closing —
|
||||
// turning a real crash into a "closed cleanly" verdict. One unlink.
|
||||
await this.clearWriterCloseRecord()
|
||||
|
||||
this.installWriterLock(info)
|
||||
return info
|
||||
}
|
||||
|
|
@ -2095,13 +2130,27 @@ export class FileSystemStorage extends BaseStorage {
|
|||
return
|
||||
}
|
||||
const lockFile = path.join(this.lockDir, FileSystemStorage.WRITER_LOCK_FILE)
|
||||
const released = this.writerLockInfo
|
||||
try {
|
||||
// Only delete if we still own it — avoid clobbering a successor that
|
||||
// claimed the lock via force-override.
|
||||
const current = await this.readWriterLock()
|
||||
if (current && current.pid === this.writerLockInfo.pid && current.hostname === this.writerLockInfo.hostname) {
|
||||
const ours =
|
||||
current === null ||
|
||||
(current.pid === released.pid && current.hostname === released.hostname)
|
||||
if (current && ours) {
|
||||
await fs.promises.unlink(lockFile)
|
||||
}
|
||||
// THE CLEAN-CLOSE RECORD (see WriterCloseRecord). Written whenever this
|
||||
// instance gives up a lock nobody else has taken — the unlink above
|
||||
// having succeeded OR the file already being gone. The next open reads
|
||||
// it instead of guessing from pid liveness: a recorded release is an
|
||||
// orderly shutdown, an absent record is a writer that never finished
|
||||
// closing. Not written when a successor holds the lock: our release is
|
||||
// then a no-op and a record would slander their live lock.
|
||||
if (ours) {
|
||||
await this.writeWriterCloseRecord(released)
|
||||
}
|
||||
} catch (err: any) {
|
||||
if (err.code !== 'ENOENT') {
|
||||
console.warn('[brainy] Failed to release writer lock file:', err)
|
||||
|
|
@ -2111,6 +2160,94 @@ export class FileSystemStorage extends BaseStorage {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @description Read the clean-close record at `locks/_writer.close`, or
|
||||
* `null` when it is absent or unparseable. A torn record is treated as
|
||||
* absent — the conservative direction, since an unreadable record can
|
||||
* vouch for nothing.
|
||||
* @returns The record, or null.
|
||||
*/
|
||||
public async readWriterCloseRecord(): Promise<WriterCloseRecord | null> {
|
||||
await this.ensureInitialized()
|
||||
const recordFile = path.join(this.lockDir, FileSystemStorage.WRITER_CLOSE_FILE)
|
||||
try {
|
||||
const raw = await fs.promises.readFile(recordFile, 'utf-8')
|
||||
const parsed = JSON.parse(raw) as WriterCloseRecord
|
||||
if (
|
||||
typeof parsed?.pid !== 'number' ||
|
||||
typeof parsed?.hostname !== 'string' ||
|
||||
typeof parsed?.startedAt !== 'string' ||
|
||||
typeof parsed?.closedAt !== 'string'
|
||||
) {
|
||||
return null
|
||||
}
|
||||
return parsed
|
||||
} catch (err: any) {
|
||||
if (err.code === 'ENOENT') return null
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @description Whether a clean-close record describes the very lock
|
||||
* generation `lock` represents. The match is pid + hostname + `startedAt`:
|
||||
* `startedAt` is the lock generation's identity, so a record can never
|
||||
* vouch for a LATER lock taken by the same pid on the same host (the
|
||||
* same-process re-open path mints a fresh `startedAt`).
|
||||
* @param record - The clean-close record read from disk.
|
||||
* @param lock - The lock file's contents.
|
||||
*/
|
||||
private closeRecordVouchesFor(record: WriterCloseRecord, lock: WriterLockInfo): boolean {
|
||||
return (
|
||||
record.pid === lock.pid &&
|
||||
record.hostname === lock.hostname &&
|
||||
record.startedAt === lock.startedAt
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* @description Write the clean-close record for a lock this instance just
|
||||
* released. Atomic (temp + rename) so a concurrent opener never reads half
|
||||
* a record. A failure here costs the next open nothing but the honest
|
||||
* fallback (pid liveness), so it warns rather than failing the close.
|
||||
* @param released - The lock info this instance held.
|
||||
*/
|
||||
private async writeWriterCloseRecord(released: WriterLockInfo): Promise<void> {
|
||||
const record: WriterCloseRecord = {
|
||||
pid: released.pid,
|
||||
hostname: released.hostname,
|
||||
startedAt: released.startedAt,
|
||||
closedAt: new Date().toISOString(),
|
||||
version: released.version
|
||||
}
|
||||
const recordFile = path.join(this.lockDir, FileSystemStorage.WRITER_CLOSE_FILE)
|
||||
try {
|
||||
await this.writeFileAtomic(recordFile, JSON.stringify(record, null, 2))
|
||||
} catch (err) {
|
||||
console.warn(
|
||||
`[brainy] Failed to write the writer clean-close record for ${this.rootDir} — ` +
|
||||
`the next open will fall back to pid liveness and may report this orderly ` +
|
||||
`shutdown as a crash:`,
|
||||
err
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @description Remove the clean-close record. Called by every successful
|
||||
* lock claim so a record never outlives the lock generation it describes.
|
||||
*/
|
||||
private async clearWriterCloseRecord(): Promise<void> {
|
||||
const recordFile = path.join(this.lockDir, FileSystemStorage.WRITER_CLOSE_FILE)
|
||||
try {
|
||||
await fs.promises.unlink(recordFile)
|
||||
} catch (err: any) {
|
||||
if (err.code !== 'ENOENT') {
|
||||
console.warn('[brainy] Failed to clear the writer clean-close record:', err)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
public override async readWriterLock(): Promise<WriterLockInfo | null> {
|
||||
await this.ensureInitialized()
|
||||
const lockFile = path.join(this.lockDir, FileSystemStorage.WRITER_LOCK_FILE)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue