fix(auth): stop heartbeating a lock that is no longer ours
Found on a re-read of the round-4 liveness work, not by review. The heartbeat refreshed the lock directory's mtime unconditionally. After a steal — possible when this process looked dead, or across hosts where the pid check cannot apply — the directory belongs to someone else, and touching it keeps *their* lock looking alive. If they then die, the stale timestamp nobody is maintaining is being maintained by us, and a third writer can never break in: a stale lock turned unbreakable, which is the permanent outage the stealing exists to avoid. The heartbeat now re-reads the owner stamp and clears itself when the token no longer matches, so it only ever refreshes a lock we still hold. The release path already checked the token; this closes the same hole on the other side. Tested by stamping a different owner onto a held lock and aging it: the mtime stays old across several heartbeat intervals, and release leaves the thief's lock intact. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
050556e1a7
commit
0cb59cf3fd
2 changed files with 45 additions and 4 deletions
|
|
@ -879,6 +879,32 @@ describe("acquireLock", () => {
|
|||
await release();
|
||||
});
|
||||
|
||||
it("stops heartbeating a lock that is no longer ours", async () => {
|
||||
// After a steal, the directory belongs to someone else. Continuing to
|
||||
// touch it would keep *their* lock looking alive after they died, which
|
||||
// turns a stale lock into an unbreakable one.
|
||||
const target = path.join(tmpDir, "stolen.json");
|
||||
const lockDir = `${target}.lock`;
|
||||
const release = await acquireLock(target, { heartbeatMs: 10 });
|
||||
// Someone judges us dead and takes it.
|
||||
await fs.writeFile(
|
||||
path.join(lockDir, "owner"),
|
||||
JSON.stringify({ token: "thief", pid: process.pid, host: os.hostname() }),
|
||||
"utf-8",
|
||||
);
|
||||
const afterSteal = new Date(Date.now() - 30_000);
|
||||
await fs.utimes(lockDir, afterSteal, afterSteal);
|
||||
// Give the heartbeat several chances to (wrongly) refresh it.
|
||||
await new Promise((r) => setTimeout(r, 80));
|
||||
const mtime = (await fs.stat(lockDir)).mtimeMs;
|
||||
expect(Date.now() - mtime).toBeGreaterThan(1_000);
|
||||
// And our release leaves their lock alone, since the token no longer
|
||||
// matches.
|
||||
await release();
|
||||
expect(existsSync(lockDir)).toBe(true);
|
||||
await fs.rm(lockDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it("cleans up and reports when it cannot stamp ownership", async () => {
|
||||
// mkdir succeeded, the owner write did not. Returning a no-op release
|
||||
// here would enter the critical section unlocked *and* leave the
|
||||
|
|
|
|||
|
|
@ -336,10 +336,25 @@ export async function acquireLock(
|
|||
// never keeps the process alive on its own — the interval is a
|
||||
// side-channel, not work anyone is waiting for.
|
||||
const heartbeat = setInterval(() => {
|
||||
const now = new Date();
|
||||
void fs.utimes(lockDir, now, now).catch(() => {
|
||||
// Lock already released or stolen; the release below copes.
|
||||
});
|
||||
// Confirm the lock is still ours before refreshing it. If it was
|
||||
// stolen (only possible when we looked dead, or across hosts where
|
||||
// the pid check cannot apply), the directory now belongs to someone
|
||||
// else — and touching it would keep *their* lock looking alive after
|
||||
// they died, which is how a stale lock becomes an unbreakable one.
|
||||
void (async () => {
|
||||
try {
|
||||
const held = parseOwner(await fs.readFile(ownerFile, "utf-8"));
|
||||
if (held?.token !== token) {
|
||||
clearInterval(heartbeat);
|
||||
return;
|
||||
}
|
||||
const now = new Date();
|
||||
await fs.utimes(lockDir, now, now);
|
||||
} catch {
|
||||
// Released, stolen, or unreadable. The release below copes, and
|
||||
// there is nothing useful to do from a timer.
|
||||
}
|
||||
})();
|
||||
}, heartbeatMs);
|
||||
heartbeat.unref?.();
|
||||
return async () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue