fix: release drains in-flight writer-lock heartbeat — no phantom lock after unlink
clearInterval() stops future heartbeat ticks but not one already in flight: a straggler tick past its ownership guards could land its atomic lock rewrite AFTER releaseWriterLock()'s unlink, re-creating the lock file as a phantom that blocks the next writer until the stale TTL expires (~60s) — the pool-eviction reopen case. Found as an ENOENT heartbeat warning during benchmark teardown; the quiet variant is the harmful one. releaseWriterLock() now awaits the in-flight tick (tracked per tick, self-clearing) before reading/unlinking, so a straggler's write always lands BEFORE the unlink and gets removed with everything else. The heartbeat's ENOENT is also now benign-by-contract (lock or directory removed under us — the next acquire recreates it); other errors stay loud. Pin: straggler-past-guards simulation — lock file absent after close, directory immediately claimable (fails on the undrained code).
This commit is contained in:
parent
300d9f2a16
commit
70e4bc8a79
2 changed files with 69 additions and 2 deletions
|
|
@ -96,6 +96,14 @@ export class FileSystemStorage extends BaseStorage {
|
||||||
private static readonly WRITER_STALE_THRESHOLD_MS = 60_000
|
private static readonly WRITER_STALE_THRESHOLD_MS = 60_000
|
||||||
private writerLockHeartbeat?: NodeJS.Timeout
|
private writerLockHeartbeat?: NodeJS.Timeout
|
||||||
private writerLockInfo?: WriterLockInfo
|
private writerLockInfo?: WriterLockInfo
|
||||||
|
/**
|
||||||
|
* The currently-executing heartbeat refresh, if any. `releaseWriterLock()`
|
||||||
|
* awaits it before unlinking: clearInterval() stops FUTURE ticks but not a
|
||||||
|
* tick already in flight, and a straggler landing after the unlink would
|
||||||
|
* RE-CREATE the lock file — a phantom lock blocking the next writer until
|
||||||
|
* the stale TTL expires (the pool-eviction reopen case).
|
||||||
|
*/
|
||||||
|
private writerHeartbeatInFlight?: Promise<void>
|
||||||
|
|
||||||
// Flush-request RPC state. The writer polls `locks/_flush_requests/` for
|
// Flush-request RPC state. The writer polls `locks/_flush_requests/` for
|
||||||
// new `.req` files and emits `.ack` files in `locks/_flush_responses/` after
|
// new `.req` files and emits `.ack` files in `locks/_flush_responses/` after
|
||||||
|
|
@ -1880,8 +1888,18 @@ export class FileSystemStorage extends BaseStorage {
|
||||||
// Heartbeat — rewrite lastHeartbeat every WRITER_HEARTBEAT_MS so other
|
// Heartbeat — rewrite lastHeartbeat every WRITER_HEARTBEAT_MS so other
|
||||||
// processes can tell a live writer from one that crashed without releasing.
|
// processes can tell a live writer from one that crashed without releasing.
|
||||||
this.writerLockHeartbeat = setInterval(() => {
|
this.writerLockHeartbeat = setInterval(() => {
|
||||||
this.refreshWriterLockHeartbeat().catch((err) => {
|
const tick = this.refreshWriterLockHeartbeat().catch((err) => {
|
||||||
|
// ENOENT = the lock (or its directory) vanished mid-refresh — the
|
||||||
|
// store was released or removed under us; the next acquire recreates
|
||||||
|
// it. Benign by construction; anything else stays loud.
|
||||||
|
if ((err as NodeJS.ErrnoException)?.code !== 'ENOENT') {
|
||||||
console.warn('[brainy] Failed to refresh writer lock heartbeat:', err)
|
console.warn('[brainy] Failed to refresh writer lock heartbeat:', err)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
this.writerHeartbeatInFlight = tick.finally(() => {
|
||||||
|
if (this.writerHeartbeatInFlight === tick) {
|
||||||
|
this.writerHeartbeatInFlight = undefined
|
||||||
|
}
|
||||||
})
|
})
|
||||||
}, FileSystemStorage.WRITER_HEARTBEAT_MS)
|
}, FileSystemStorage.WRITER_HEARTBEAT_MS)
|
||||||
if (typeof this.writerLockHeartbeat.unref === 'function') {
|
if (typeof this.writerLockHeartbeat.unref === 'function') {
|
||||||
|
|
@ -1913,6 +1931,15 @@ export class FileSystemStorage extends BaseStorage {
|
||||||
clearInterval(this.writerLockHeartbeat)
|
clearInterval(this.writerLockHeartbeat)
|
||||||
this.writerLockHeartbeat = undefined
|
this.writerLockHeartbeat = undefined
|
||||||
}
|
}
|
||||||
|
// Drain an in-flight heartbeat tick BEFORE unlinking: clearInterval stops
|
||||||
|
// future ticks only, and a straggler write landing after the unlink would
|
||||||
|
// re-create the lock as a phantom (blocking the next writer until the
|
||||||
|
// stale TTL). After the drain, any refresh is either fully landed (we
|
||||||
|
// unlink its output below) or not started (it sees writerLockInfo
|
||||||
|
// undefined and returns).
|
||||||
|
if (this.writerHeartbeatInFlight) {
|
||||||
|
await this.writerHeartbeatInFlight
|
||||||
|
}
|
||||||
if (!this.writerLockInfo) {
|
if (!this.writerLockInfo) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -153,6 +153,46 @@ describe('Multi-process safety + read-only mode', () => {
|
||||||
expect(err.lockInfo?.pid).toBe(otherPid)
|
expect(err.lockInfo?.pid).toBe(otherPid)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('release drains an in-flight heartbeat — no phantom lock re-created after unlink', async () => {
|
||||||
|
// The race (8.9.0): clearInterval stops FUTURE heartbeat ticks, but a
|
||||||
|
// tick already in flight could land its lock rewrite AFTER release's
|
||||||
|
// unlink — re-creating the lock as a phantom that blocks the next
|
||||||
|
// writer until the stale TTL. Simulate the in-flight tick explicitly
|
||||||
|
// and prove release waits for it.
|
||||||
|
writer = new Brainy({ requireSubtype: false, storage: { type: 'filesystem', path: dir } })
|
||||||
|
await writer.init()
|
||||||
|
const storage: any = (writer as any).storage
|
||||||
|
|
||||||
|
// An in-flight refresh that is ALREADY PAST its ownership guards
|
||||||
|
// (captured the lock info before release ran) and lands its atomic
|
||||||
|
// rewrite slowly — the exact straggler shape; absent the drain it
|
||||||
|
// writes after the unlink.
|
||||||
|
const { join: joinPath } = await import('node:path')
|
||||||
|
const capturedInfo = { ...storage.writerLockInfo }
|
||||||
|
const lockPath = joinPath(dir, 'locks', '_writer.lock')
|
||||||
|
const slowTick = (async () => {
|
||||||
|
await new Promise((r) => setTimeout(r, 100))
|
||||||
|
await storage.writeFileAtomic(
|
||||||
|
lockPath,
|
||||||
|
JSON.stringify({ ...capturedInfo, lastHeartbeat: new Date().toISOString() })
|
||||||
|
)
|
||||||
|
})()
|
||||||
|
storage.writerHeartbeatInFlight = slowTick.catch(() => {})
|
||||||
|
|
||||||
|
await writer.close() // → releaseWriterLock must drain slowTick first
|
||||||
|
await slowTick.catch(() => {}) // both paths fully settled either way
|
||||||
|
writer = null
|
||||||
|
|
||||||
|
const { existsSync } = await import('node:fs')
|
||||||
|
const { join } = await import('node:path')
|
||||||
|
expect(existsSync(join(dir, 'locks', '_writer.lock'))).toBe(false)
|
||||||
|
|
||||||
|
// And the directory is immediately claimable — no stale-TTL wait.
|
||||||
|
const next = new Brainy({ requireSubtype: false, storage: { type: 'filesystem', path: dir } })
|
||||||
|
await expect(next.init()).resolves.toBeUndefined()
|
||||||
|
await next.close()
|
||||||
|
})
|
||||||
|
|
||||||
it('allows a second in-process writer with a warning (same PID)', async () => {
|
it('allows a second in-process writer with a warning (same PID)', async () => {
|
||||||
// Two Brainy instances in the same Node process: not the dangerous
|
// Two Brainy instances in the same Node process: not the dangerous
|
||||||
// cross-process case. Should succeed (with a console warning).
|
// cross-process case. Should succeed (with a console warning).
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue