From 3fe4599251e35eeee416d2954abc00b181f4a92e Mon Sep 17 00:00:00 2001 From: flint Date: Thu, 8 Oct 2026 18:25:33 +0000 Subject: [PATCH 01/10] fix(relay): one cross-process lock covers the record check, publication and acceptance marker Relay acceptance checked for an existing record, published the record and wrote the acceptance marker, but only the check ran under the mailbox lock, so two receivers serving the same branch could each accept the same delivery. One cross-process lock (the mailbox lock, a bounded wait, a named refusal on timeout) now spans the existing-record check, the record publication and the acceptance marker for a branch+id, so exactly one receiver accepts a delivery; the other sees a duplicate or refuses a differing payload. Closes #561 --- .../fixed-561-relay-accept-one-lock.md | 1 + packages/cli/src/utils/mail.ts | 32 ++- packages/cli/src/utils/relay.ts | 123 ++++++++---- .../cli/test/relay-accept-single-lock.test.ts | 186 ++++++++++++++++++ 4 files changed, 290 insertions(+), 52 deletions(-) create mode 100644 .changelog/unreleased/fixed-561-relay-accept-one-lock.md create mode 100644 packages/cli/test/relay-accept-single-lock.test.ts diff --git a/.changelog/unreleased/fixed-561-relay-accept-one-lock.md b/.changelog/unreleased/fixed-561-relay-accept-one-lock.md new file mode 100644 index 00000000..620d5ba2 --- /dev/null +++ b/.changelog/unreleased/fixed-561-relay-accept-one-lock.md @@ -0,0 +1 @@ +- **Relay acceptance runs the existing-record check, the record write and the acceptance marker under the recipient's mailbox lock.** Two receivers serving the same branch can no longer both accept a delivery: the second sees a duplicate (identical payload) or refuses (differing payload). diff --git a/packages/cli/src/utils/mail.ts b/packages/cli/src/utils/mail.ts index 7a43c5a1..d1134d90 100644 --- a/packages/cli/src/utils/mail.ts +++ b/packages/cli/src/utils/mail.ts @@ -134,6 +134,20 @@ function mailboxRoot(agent: string): string { return existsSync(branchMailRoot) ? branchMailRoot : join(mailDirPath(), agent); } +/** + * The mailbox root relay acceptance locks and publishes into: `agent`'s own + * mailbox, or the host-level `.undeliverable` tree when `agent` is not a valid + * id (a relayed delivery to an invalid recipient is dead-lettered there). + */ +export function relayAcceptRoot(agent: string): string { + try { + return mailboxRoot(agent); + } catch (error) { + if (!(error instanceof Error && error.message.startsWith("Invalid agent id"))) throw error; + return join(mailDirPath(), ".undeliverable"); + } +} + export function getInbox(agent: string): { root: string; tmp: string; fresh: string; cur: string; dlq: string } { const root = mailboxRoot(agent); const tmp = join(root, "tmp"); @@ -430,12 +444,8 @@ function carriesRelayDelivery(parsed: unknown, raw: string): boolean { return /"relayDelivery"\s*:/.test(raw); } -export function findRelayedRecord(agent: string, delivery: { branchId: string; id: string }, payload: z.infer, wireRecipient = payload.to): string | undefined { - let root: string; - try { root = mailboxRoot(agent); } catch (error) { - if (!(error instanceof Error && error.message.startsWith("Invalid agent id"))) throw error; - root = join(mailDirPath(), ".undeliverable"); - } +export function findRelayedRecord(agent: string, delivery: { branchId: string; id: string }, payload: z.infer, wireRecipient = payload.to, options: { heldRoot?: string } = {}): string | undefined { + const root = relayAcceptRoot(agent); const roots = new Set([root]); for (const parent of [mailDirPath(), join(process.env.HOME || homedir(), ".tps", "branch-office")]) { if (!existsSync(parent)) continue; @@ -455,8 +465,12 @@ export function findRelayedRecord(agent: string, delivery: { branchId: string; i const recipientRoot = root; for (const root of roots) { mkdirMailDirectory(root); - const lock = acquireMailLockSync(root); - if (!lock) throw new Error(`mailbox busy for relayed message ${delivery.id}`); + // A caller that already holds this mailbox's lock (relay acceptance holds + // the recipient's across check, publication and marker) passes it here so + // the scan does not re-acquire it — a nested acquisition is a hard error. + const held = root === options.heldRoot; + const lock = held ? null : acquireMailLockSync(root); + if (!held && !lock) throw new Error(`mailbox busy for relayed message ${delivery.id}`); try { for (const dir of ["new", "cur", "dlq"]) { const path = join(root, dir); @@ -523,7 +537,7 @@ export function findRelayedRecord(agent: string, delivery: { branchId: string; i } } } - } finally { lock.release(); } + } finally { lock?.release(); } } return undefined; } diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index 96f66059..990bd347 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -3,7 +3,8 @@ import { dirname, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; import { randomUUID } from "node:crypto"; import { sanitizeIdentifier } from "../schema/sanitizer.js"; -import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailSyncError, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; +import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailSyncError, relayAcceptRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; +import { acquireMailLockSync } from "./mail-lock.js"; import { LoopDetector } from "./loop-detector.js"; import { FileSystemTransport, resolveTransport, TransportRegistry, type TransportChannel, type TpsMessage } from "./transport.js"; import { NoiseIkTransport } from "./noise-ik-transport.js"; @@ -379,55 +380,91 @@ function recordAcceptance(acceptedDir: string, marker: string): void { syncMailDirectory(acceptedDir); } +/** How long relay acceptance waits for the recipient's mailbox lock before it + * refuses. Bounded: past it the delivery is refused by name, never skipped and + * never waited for indefinitely. */ +const RELAY_ACCEPT_LOCK_TIMEOUT_MS = 2000; + +function relayAcceptLockTimeoutMs(): number { + const raw = Number(process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS); + return Number.isFinite(raw) && raw > 0 ? raw : RELAY_ACCEPT_LOCK_TIMEOUT_MS; +} + +/** The named refusal relay acceptance raises when the recipient's mailbox lock + * is held past the bound. */ +export class RelayAcceptLockTimeoutError extends Error { + constructor(recipient: string) { + super(`relay acceptance timed out waiting for the mailbox lock for ${recipient}`); + this.name = "RelayAcceptLockTimeoutError"; + } +} + export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): boolean { MailDeliverBodySchema.shape.id.parse(body.id); if (!/^[a-zA-Z0-9_-]+$/.test(branchId)) throw new Error(`invalid branch id for relayed message ${body.id}`); - // The marker path includes the branch: a 64-hex id is deterministic, so two branches can send the same one. - const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId); - const marker = join(acceptedDir, body.id); - const legacyMarker = join(getMailDir(), ".relay-accepted", body.id); - const existingMarker = existsSync(marker) ? marker : existsSync(legacyMarker) ? legacyMarker : undefined; - const delivery = { branchId, id: body.id }; - const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }); - if (existingMarker && existingRecord) { - syncMailFile(existingMarker); - syncMailDirectory(dirname(existingMarker)); - return false; - } - if (!existingMarker && existingRecord) { - recordAcceptance(acceptedDir, marker); - return false; - } - - let delivered: boolean; + // ONE cross-process lock — the recipient's mailbox lock — held across the + // existing-record check, the record publication and the acceptance marker, so + // exactly one receiver accepts a delivery for a branch+id: the other sees a + // duplicate (identical payload) or refuses (differing payload). Every fact the + // accept decision uses is read under this lock, and the recipient's mailbox is + // the one the record is published into, so the lock serializes both receivers. + const recipientRoot = relayAcceptRoot(body.to); + // Create the recipient's mailbox (and register its creation for fsync) before + // the lock, in the same order findRelayedRecord used to, so an unlocked + // mkdir never swallows the parent-directory sync. + mkdirMailDirectory(recipientRoot); + const lock = acquireMailLockSync(recipientRoot, { timeoutMs: relayAcceptLockTimeoutMs() }); + if (!lock) throw new RelayAcceptLockTimeoutError(body.to); try { - sendMessage(body.to, body.content, body.from, delivery, body.timestamp); - delivered = true; - } catch (e: unknown) { - if (e instanceof MailSyncError) throw e; - const reason = e instanceof Error ? e.message : String(e); - const cls: PromoteRejectClass = /inbox full/i.test(reason) - ? "inbox-full" - : /^(Invalid agent id|Message body)/.test(reason) ? "invalid" : "storage-unavailable"; - console.error(`[relay] local delivery failed for message ${body.id} to ${body.to}: ${reason}`); + // The marker path includes the branch: a 64-hex id is deterministic, so two branches can send the same one. + const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId); + const marker = join(acceptedDir, body.id); + const legacyMarker = join(getMailDir(), ".relay-accepted", body.id); + const existingMarker = existsSync(marker) ? marker : existsSync(legacyMarker) ? legacyMarker : undefined; + const delivery = { branchId, id: body.id }; + const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot }); + if (existingMarker && existingRecord) { + syncMailFile(existingMarker); + syncMailDirectory(dirname(existingMarker)); + return false; + } + if (!existingMarker && existingRecord) { + recordAcceptance(acceptedDir, marker); + return false; + } + + let delivered: boolean; try { - deadLetterUndelivered( - body.to, - { id: body.id, from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, - cls, - reason, - delivery, - ); - } catch (dlqErr: unknown) { - console.error( - `[relay] dead-letter failed for message ${body.id} to ${body.to}: ${dlqErr instanceof Error ? dlqErr.message : String(dlqErr)}`, - ); - throw dlqErr; + sendMessage(body.to, body.content, body.from, delivery, body.timestamp); + delivered = true; + } catch (e: unknown) { + if (e instanceof MailSyncError) throw e; + const reason = e instanceof Error ? e.message : String(e); + const cls: PromoteRejectClass = /inbox full/i.test(reason) + ? "inbox-full" + : /^(Invalid agent id|Message body)/.test(reason) ? "invalid" : "storage-unavailable"; + console.error(`[relay] local delivery failed for message ${body.id} to ${body.to}: ${reason}`); + try { + deadLetterUndelivered( + body.to, + { id: body.id, from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, + cls, + reason, + delivery, + ); + } catch (dlqErr: unknown) { + console.error( + `[relay] dead-letter failed for message ${body.id} to ${body.to}: ${dlqErr instanceof Error ? dlqErr.message : String(dlqErr)}`, + ); + throw dlqErr; + } + delivered = false; } - delivered = false; + recordAcceptance(acceptedDir, marker); + return delivered; + } finally { + lock.release(); } - recordAcceptance(acceptedDir, marker); - return delivered; } async function acceptRelayedMail(channel: TransportChannel, branchId: string, msg: TpsMessage, body: MailDeliverBody): Promise { diff --git a/packages/cli/test/relay-accept-single-lock.test.ts b/packages/cli/test/relay-accept-single-lock.test.ts new file mode 100644 index 00000000..32abdaf3 --- /dev/null +++ b/packages/cli/test/relay-accept-single-lock.test.ts @@ -0,0 +1,186 @@ +/** + * relay-accept-single-lock.test.ts — cli#561. + * + * Relay acceptance checks for an existing record, publishes the record and + * writes the acceptance marker. Only the check used to run under the mailbox + * lock, so two receivers serving the same branch could each publish the same + * branch+id: 193 of 200 ids got two inbox records and both calls reported + * delivered. One cross-process lock — the recipient's mailbox lock — now spans + * the check, the publication and the marker, so exactly one receiver accepts a + * delivery for a branch+id. + * + * These tests run two real child processes delivering the same ids at once. + */ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { spawn } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { getInbox } from "../src/utils/mail.js"; +import { RelayAcceptLockTimeoutError, deliverRelayedToLocal } from "../src/utils/relay.js"; + +const BRANCH = "remote"; +const RECIPIENT = "local"; +const FROM = "remote"; +const TIMESTAMP = "2000-01-01T00:00:00.000Z"; +const IDS = 16; + +const RELAY_URL = new URL("../src/utils/relay.js", import.meta.url).href; +const LOCK_URL = new URL("../../agent/dist/lib/mail-lock.js", import.meta.url).href; + +/** The child process: either delivers a list of ids, or holds the mailbox lock. */ +const CHILD_SOURCE = ` +const url = ${JSON.stringify(RELAY_URL)}; +const mode = process.env.RELAY_CHILD_MODE; +if (mode === "hold") { + const { acquireMailLock } = await import(${JSON.stringify(LOCK_URL)}); + const lock = await acquireMailLock(process.env.RELAY_CHILD_ROOT, { timeoutMs: 10000 }); + if (!lock) { process.stderr.write("holder could not acquire\\n"); process.exit(2); } + process.stdout.write("ready"); + process.stdin.once("data", () => { lock.release(); process.exit(0); }); +} else { + const { deliverRelayedToLocal } = await import(url); + const ids = JSON.parse(process.env.RELAY_CHILD_IDS); + const counts = { delivered: 0, duplicate: 0, refused: 0 }; + for (const id of ids) { + try { + const ok = deliverRelayedToLocal(process.env.RELAY_CHILD_BRANCH, { + id, + from: process.env.RELAY_CHILD_FROM, + to: process.env.RELAY_CHILD_TO, + content: process.env.RELAY_CHILD_PREFIX + id, + timestamp: ${JSON.stringify(TIMESTAMP)}, + }); + if (ok) counts.delivered += 1; else counts.duplicate += 1; + } catch { + counts.refused += 1; + } + } + process.stdout.write(JSON.stringify(counts)); +} +`; + +interface Counts { delivered: number; duplicate: number; refused: number; } + +let root: string; +let mail: string; +let childScript: string; +let savedEnv: Record; + +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), "tps-relay-accept-")); + mail = join(root, "mail"); + savedEnv = {}; + for (const key of ["HOME", "TPS_MAIL_DIR", "TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS"]) savedEnv[key] = process.env[key]; + process.env.HOME = root; + process.env.TPS_MAIL_DIR = mail; + delete process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS; + childScript = join(root, "relay-accept-child.ts"); + writeFileSync(childScript, CHILD_SOURCE); +}); + +afterEach(() => { + for (const [key, value] of Object.entries(savedEnv)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + rmSync(root, { recursive: true, force: true }); +}); + +function jsonFiles(dir: string): string[] { + return existsSync(dir) ? readdirSync(dir).filter((file) => file.endsWith(".json")) : []; +} + +/** Run one delivering child and resolve with its counts when it exits. */ +function deliverChild(prefix: string, ids: string[]): Promise { + const child = spawn("bun", [childScript], { + env: { + ...process.env, + RELAY_CHILD_MODE: "deliver", + RELAY_CHILD_BRANCH: BRANCH, + RELAY_CHILD_TO: RECIPIENT, + RELAY_CHILD_FROM: FROM, + RELAY_CHILD_PREFIX: prefix, + RELAY_CHILD_IDS: JSON.stringify(ids), + }, + stdio: ["ignore", "pipe", "inherit"], + }); + return new Promise((resolve, reject) => { + let out = ""; + child.stdout.on("data", (chunk: Buffer) => { out += chunk.toString(); }); + child.once("error", reject); + child.once("exit", (code) => { + if (code !== 0) return reject(new Error(`delivery child exited ${code}`)); + try { resolve(JSON.parse(out) as Counts); } + catch (error) { reject(new Error(`delivery child printed ${JSON.stringify(out)}: ${String(error)}`)); } + }); + }); +} + +/** Spawn a real process that holds the recipient's mailbox lock until released. */ +function holdLock(): Promise<{ release: () => Promise }> { + const child = spawn("bun", [childScript], { + env: { ...process.env, RELAY_CHILD_MODE: "hold", RELAY_CHILD_ROOT: join(mail, RECIPIENT) }, + stdio: ["pipe", "pipe", "inherit"], + }); + const exited = new Promise((resolve) => child.once("exit", resolve)); + return new Promise((resolve, reject) => { + child.once("error", reject); + child.once("exit", (code) => reject(new Error(`holder exited ${code} before ready`))); + child.stdout.once("data", (chunk: Buffer) => { + if (chunk.toString() === "ready") resolve({ release: async () => { child.stdin.end("release"); await exited; } }); + else reject(new Error(`holder printed ${JSON.stringify(chunk.toString())}`)); + }); + }); +} + +/** Map each delivery id to the ids of the inbox records that carry it. */ +function recordsByDeliveryId(): Map { + const byId = new Map(); + for (const file of jsonFiles(getInbox(RECIPIENT).fresh)) { + const record = JSON.parse(readFileSync(join(getInbox(RECIPIENT).fresh, file), "utf8")) as { relayDelivery?: { id?: string } }; + const id = record.relayDelivery?.id ?? "unmarked"; + byId.set(id, [...(byId.get(id) ?? []), file]); + } + return byId; +} + +describe("relay acceptance holds the recipient's mailbox lock across check, publication and marker (cli#561)", () => { + test("two processes delivering identical payloads store one record per id and one delivered", async () => { + const ids = Array.from({ length: IDS }, () => randomUUID()); + const [a, b] = await Promise.all([deliverChild("same-", ids), deliverChild("same-", ids)]); + + expect(a.delivered + b.delivered).toBe(IDS); + expect(a.refused + b.refused).toBe(0); + + const byId = recordsByDeliveryId(); + expect([...byId.keys()].sort()).toEqual([...ids].sort()); + for (const id of ids) expect(byId.get(id)).toHaveLength(1); + }, 60_000); + + test("two processes delivering differing payloads store one body per id and refuse the other", async () => { + const ids = Array.from({ length: IDS }, () => randomUUID()); + const [a, b] = await Promise.all([deliverChild("A-", ids), deliverChild("B-", ids)]); + + expect(a.delivered + b.delivered).toBe(IDS); + expect(a.refused + b.refused).toBe(IDS); + + const byId = recordsByDeliveryId(); + expect([...byId.keys()].sort()).toEqual([...ids].sort()); + for (const id of ids) expect(byId.get(id)).toHaveLength(1); + }, 60_000); + + test("a lock held past the bound refuses by name and writes nothing", async () => { + const holder = await holdLock(); + try { + process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS = "75"; + const body = { id: randomUUID(), from: FROM, to: RECIPIENT, content: "timed out", timestamp: TIMESTAMP }; + expect(() => deliverRelayedToLocal(BRANCH, body)).toThrow(RelayAcceptLockTimeoutError); + expect(jsonFiles(getInbox(RECIPIENT).fresh)).toEqual([]); + expect(existsSync(join(mail, ".relay-accepted"))).toBe(false); + } finally { + await holder.release(); + } + }, 60_000); +}); From 6927562de0d7bb50c7220083025a349d6ee005ce Mon Sep 17 00:00:00 2001 From: flint Date: Thu, 8 Oct 2026 12:51:21 -0700 Subject: [PATCH 02/10] fix(relay): one acceptance deadline bounds every lock in the accept path, with a named refusal (#561) Co-Authored-By: Claude Opus 5.5 --- .../fixed-561-relay-accept-one-lock.md | 2 +- packages/cli/src/utils/mail.ts | 4 +- packages/cli/src/utils/relay.ts | 29 ++++++++------- .../cli/test/relay-accept-single-lock.test.ts | 37 ++++++++++++------- 4 files changed, 42 insertions(+), 30 deletions(-) diff --git a/.changelog/unreleased/fixed-561-relay-accept-one-lock.md b/.changelog/unreleased/fixed-561-relay-accept-one-lock.md index 620d5ba2..2722fcb1 100644 --- a/.changelog/unreleased/fixed-561-relay-accept-one-lock.md +++ b/.changelog/unreleased/fixed-561-relay-accept-one-lock.md @@ -1 +1 @@ -- **Relay acceptance runs the existing-record check, the record write and the acceptance marker under the recipient's mailbox lock.** Two receivers serving the same branch can no longer both accept a delivery: the second sees a duplicate (identical payload) or refuses (differing payload). +- **Relay acceptance runs the existing-record check, the record write and the acceptance marker under the recipient's mailbox lock.** For the same branch, recipient and id, at most one receiver accepts: the second sees a duplicate (identical payload), refuses (differing payload), or gets the timeout refusal. A timeout publishes no mail record or acceptance marker. diff --git a/packages/cli/src/utils/mail.ts b/packages/cli/src/utils/mail.ts index d1134d90..6189d999 100644 --- a/packages/cli/src/utils/mail.ts +++ b/packages/cli/src/utils/mail.ts @@ -444,7 +444,7 @@ function carriesRelayDelivery(parsed: unknown, raw: string): boolean { return /"relayDelivery"\s*:/.test(raw); } -export function findRelayedRecord(agent: string, delivery: { branchId: string; id: string }, payload: z.infer, wireRecipient = payload.to, options: { heldRoot?: string } = {}): string | undefined { +export function findRelayedRecord(agent: string, delivery: { branchId: string; id: string }, payload: z.infer, wireRecipient = payload.to, options: { heldRoot?: string; acquireLock?: (root: string) => MailLock } = {}): string | undefined { const root = relayAcceptRoot(agent); const roots = new Set([root]); for (const parent of [mailDirPath(), join(process.env.HOME || homedir(), ".tps", "branch-office")]) { @@ -469,7 +469,7 @@ export function findRelayedRecord(agent: string, delivery: { branchId: string; i // the recipient's across check, publication and marker) passes it here so // the scan does not re-acquire it — a nested acquisition is a hard error. const held = root === options.heldRoot; - const lock = held ? null : acquireMailLockSync(root); + const lock = held ? null : options.acquireLock ? options.acquireLock(root) : acquireMailLockSync(root); if (!held && !lock) throw new Error(`mailbox busy for relayed message ${delivery.id}`); try { for (const dir of ["new", "cur", "dlq"]) { diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index 990bd347..e78ba084 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -380,9 +380,7 @@ function recordAcceptance(acceptedDir: string, marker: string): void { syncMailDirectory(acceptedDir); } -/** How long relay acceptance waits for the recipient's mailbox lock before it - * refuses. Bounded: past it the delivery is refused by name, never skipped and - * never waited for indefinitely. */ +/** The shared wait budget for relay acceptance's mailbox locks. */ const RELAY_ACCEPT_LOCK_TIMEOUT_MS = 2000; function relayAcceptLockTimeoutMs(): number { @@ -390,8 +388,7 @@ function relayAcceptLockTimeoutMs(): number { return Number.isFinite(raw) && raw > 0 ? raw : RELAY_ACCEPT_LOCK_TIMEOUT_MS; } -/** The named refusal relay acceptance raises when the recipient's mailbox lock - * is held past the bound. */ +/** The named refusal when an acceptance mailbox lock times out. */ export class RelayAcceptLockTimeoutError extends Error { constructor(recipient: string) { super(`relay acceptance timed out waiting for the mailbox lock for ${recipient}`); @@ -402,19 +399,23 @@ export class RelayAcceptLockTimeoutError extends Error { export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): boolean { MailDeliverBodySchema.shape.id.parse(body.id); if (!/^[a-zA-Z0-9_-]+$/.test(branchId)) throw new Error(`invalid branch id for relayed message ${body.id}`); - // ONE cross-process lock — the recipient's mailbox lock — held across the - // existing-record check, the record publication and the acceptance marker, so - // exactly one receiver accepts a delivery for a branch+id: the other sees a - // duplicate (identical payload) or refuses (differing payload). Every fact the - // accept decision uses is read under this lock, and the recipient's mailbox is - // the one the record is published into, so the lock serializes both receivers. + // The recipient's lock spans check, publication and marker. For the same + // branch, recipient and id, at most one receiver accepts; the second sees a + // duplicate (identical payload), refuses (differing payload), or gets the timeout refusal. + const deadline = Date.now() + relayAcceptLockTimeoutMs(); + const acquireLock = (root: string) => { + const remaining = deadline - Date.now(); + if (remaining <= 0) throw new RelayAcceptLockTimeoutError(body.to); + const lock = acquireMailLockSync(root, { timeoutMs: remaining }); + if (!lock) throw new RelayAcceptLockTimeoutError(body.to); + return lock; + }; const recipientRoot = relayAcceptRoot(body.to); // Create the recipient's mailbox (and register its creation for fsync) before // the lock, in the same order findRelayedRecord used to, so an unlocked // mkdir never swallows the parent-directory sync. mkdirMailDirectory(recipientRoot); - const lock = acquireMailLockSync(recipientRoot, { timeoutMs: relayAcceptLockTimeoutMs() }); - if (!lock) throw new RelayAcceptLockTimeoutError(body.to); + const lock = acquireLock(recipientRoot); try { // The marker path includes the branch: a 64-hex id is deterministic, so two branches can send the same one. const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId); @@ -422,7 +423,7 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): const legacyMarker = join(getMailDir(), ".relay-accepted", body.id); const existingMarker = existsSync(marker) ? marker : existsSync(legacyMarker) ? legacyMarker : undefined; const delivery = { branchId, id: body.id }; - const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot }); + const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot, acquireLock }); if (existingMarker && existingRecord) { syncMailFile(existingMarker); syncMailDirectory(dirname(existingMarker)); diff --git a/packages/cli/test/relay-accept-single-lock.test.ts b/packages/cli/test/relay-accept-single-lock.test.ts index 32abdaf3..ef1b2439 100644 --- a/packages/cli/test/relay-accept-single-lock.test.ts +++ b/packages/cli/test/relay-accept-single-lock.test.ts @@ -1,15 +1,9 @@ /** * relay-accept-single-lock.test.ts — cli#561. * - * Relay acceptance checks for an existing record, publishes the record and - * writes the acceptance marker. Only the check used to run under the mailbox - * lock, so two receivers serving the same branch could each publish the same - * branch+id: 193 of 200 ids got two inbox records and both calls reported - * delivered. One cross-process lock — the recipient's mailbox lock — now spans - * the check, the publication and the marker, so exactly one receiver accepts a - * delivery for a branch+id. - * - * These tests run two real child processes delivering the same ids at once. + * For the same branch, recipient and id, at most one receiver accepts; the + * second sees a duplicate (identical payload), refuses (differing payload), + * or gets the timeout refusal. */ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { spawn } from "node:child_process"; @@ -118,10 +112,10 @@ function deliverChild(prefix: string, ids: string[]): Promise { }); } -/** Spawn a real process that holds the recipient's mailbox lock until released. */ -function holdLock(): Promise<{ release: () => Promise }> { +/** Spawn a real process that holds a mailbox lock until released. */ +function holdLock(agent = RECIPIENT): Promise<{ release: () => Promise }> { const child = spawn("bun", [childScript], { - env: { ...process.env, RELAY_CHILD_MODE: "hold", RELAY_CHILD_ROOT: join(mail, RECIPIENT) }, + env: { ...process.env, RELAY_CHILD_MODE: "hold", RELAY_CHILD_ROOT: join(mail, agent) }, stdio: ["pipe", "pipe", "inherit"], }); const exited = new Promise((resolve) => child.once("exit", resolve)); @@ -171,7 +165,7 @@ describe("relay acceptance holds the recipient's mailbox lock across check, publ for (const id of ids) expect(byId.get(id)).toHaveLength(1); }, 60_000); - test("a lock held past the bound refuses by name and writes nothing", async () => { + test("a recipient lock timeout refuses by name, publishing no mail record or acceptance marker", async () => { const holder = await holdLock(); try { process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS = "75"; @@ -183,4 +177,21 @@ describe("relay acceptance holds the recipient's mailbox lock across check, publ await holder.release(); } }, 60_000); + + test("a different mailbox's lock uses the acceptance deadline and refuses by name, publishing no mail record or acceptance marker", async () => { + const holder = await holdLock("other"); + try { + process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS = "75"; + const body = { id: randomUUID(), from: FROM, to: RECIPIENT, content: "timed out", timestamp: TIMESTAMP }; + const started = Date.now(); + expect(() => deliverRelayedToLocal(BRANCH, body)).toThrow(RelayAcceptLockTimeoutError); + expect(Date.now() - started).toBeLessThan(1000); + for (const agent of [RECIPIENT, "other"]) { + for (const dir of ["new", "cur", "dlq"]) expect(jsonFiles(join(mail, agent, dir))).toEqual([]); + } + expect(existsSync(join(mail, ".relay-accepted"))).toBe(false); + } finally { + await holder.release(); + } + }, 60_000); }); From 8d4b19c3700161ec5424be8bb90710f217cf5b74 Mon Sep 17 00:00:00 2001 From: flint Date: Thu, 8 Oct 2026 14:04:35 -0700 Subject: [PATCH 03/10] fix(relay): acceptance locks a stable per-recipient key and publishes to the root it locked (#561) Co-Authored-By: Claude Opus 5.5 --- .../fixed-561-relay-accept-one-lock.md | 2 +- packages/cli/src/utils/mail.ts | 37 +++++---- packages/cli/src/utils/relay.ts | 34 +++++---- .../cli/test/relay-accept-single-lock.test.ts | 76 ++++++++++++++++--- 4 files changed, 112 insertions(+), 37 deletions(-) diff --git a/.changelog/unreleased/fixed-561-relay-accept-one-lock.md b/.changelog/unreleased/fixed-561-relay-accept-one-lock.md index 2722fcb1..20e80fba 100644 --- a/.changelog/unreleased/fixed-561-relay-accept-one-lock.md +++ b/.changelog/unreleased/fixed-561-relay-accept-one-lock.md @@ -1 +1 @@ -- **Relay acceptance runs the existing-record check, the record write and the acceptance marker under the recipient's mailbox lock.** For the same branch, recipient and id, at most one receiver accepts: the second sees a duplicate (identical payload), refuses (differing payload), or gets the timeout refusal. A timeout publishes no mail record or acceptance marker. +- **Relay acceptance serializes receivers with a stable recipient lock and publishes into the selected mailbox.** A timeout publishes no mail record or acceptance marker. diff --git a/packages/cli/src/utils/mail.ts b/packages/cli/src/utils/mail.ts index 6189d999..f8eed796 100644 --- a/packages/cli/src/utils/mail.ts +++ b/packages/cli/src/utils/mail.ts @@ -134,11 +134,7 @@ function mailboxRoot(agent: string): string { return existsSync(branchMailRoot) ? branchMailRoot : join(mailDirPath(), agent); } -/** - * The mailbox root relay acceptance locks and publishes into: `agent`'s own - * mailbox, or the host-level `.undeliverable` tree when `agent` is not a valid - * id (a relayed delivery to an invalid recipient is dead-lettered there). - */ +/** The selected relay mailbox, or `.undeliverable` for an invalid recipient. */ export function relayAcceptRoot(agent: string): string { try { return mailboxRoot(agent); @@ -148,8 +144,21 @@ export function relayAcceptRoot(agent: string): string { } } +export function relayAcceptLockRoot(agent: string): string { + try { + assertValidAgentId(agent); + return join(mailDirPath(), agent); + } catch (error) { + if (!(error instanceof Error && error.message.startsWith("Invalid agent id"))) throw error; + return join(mailDirPath(), ".undeliverable"); + } +} + export function getInbox(agent: string): { root: string; tmp: string; fresh: string; cur: string; dlq: string } { - const root = mailboxRoot(agent); + return inboxAtRoot(mailboxRoot(agent)); +} + +function inboxAtRoot(root: string): ReturnType { const tmp = join(root, "tmp"); const fresh = join(root, "new"); const cur = join(root, "cur"); @@ -444,8 +453,8 @@ function carriesRelayDelivery(parsed: unknown, raw: string): boolean { return /"relayDelivery"\s*:/.test(raw); } -export function findRelayedRecord(agent: string, delivery: { branchId: string; id: string }, payload: z.infer, wireRecipient = payload.to, options: { heldRoot?: string; acquireLock?: (root: string) => MailLock } = {}): string | undefined { - const root = relayAcceptRoot(agent); +export function findRelayedRecord(agent: string, delivery: { branchId: string; id: string }, payload: z.infer, wireRecipient = payload.to, options: { heldRoot?: string; heldAcceptanceRoot?: string; acquireLock?: (root: string) => MailLock } = {}): string | undefined { + const root = options.heldRoot ?? relayAcceptRoot(agent); const roots = new Set([root]); for (const parent of [mailDirPath(), join(process.env.HOME || homedir(), ".tps", "branch-office")]) { if (!existsSync(parent)) continue; @@ -468,7 +477,7 @@ export function findRelayedRecord(agent: string, delivery: { branchId: string; i // A caller that already holds this mailbox's lock (relay acceptance holds // the recipient's across check, publication and marker) passes it here so // the scan does not re-acquire it — a nested acquisition is a hard error. - const held = root === options.heldRoot; + const held = root === options.heldRoot || root === options.heldAcceptanceRoot; const lock = held ? null : options.acquireLock ? options.acquireLock(root) : acquireMailLockSync(root); if (!held && !lock) throw new Error(`mailbox busy for relayed message ${delivery.id}`); try { @@ -542,7 +551,7 @@ export function findRelayedRecord(agent: string, delivery: { branchId: string; i return undefined; } -export function sendMessage(to: string, body: string, from?: string, relayDelivery?: { branchId: string; id: string }, senderTimestamp?: string, wireRecipient = to): MailMessage & { filePath: string } { +export function sendMessage(to: string, body: string, from?: string, relayDelivery?: { branchId: string; id: string }, senderTimestamp?: string, wireRecipient = to, inboxRoot?: string): MailMessage & { filePath: string } { assertValidAgentId(to); const sender = from || "unknown"; assertValidAgentId(sender); @@ -560,8 +569,8 @@ export function sendMessage(to: string, body: string, from?: string, relayDelive ); } - const inbox = getInbox(to); - const quotaCount = countInboxMessages(to); + const inbox = inboxRoot === undefined ? getInbox(to) : inboxAtRoot(inboxRoot); + const quotaCount = readdirSync(inbox.fresh).filter((f) => f.endsWith(".json")).length; if (quotaCount >= MAX_INBOX_MESSAGES) { throw new Error(inboxFullMessage(to, quotaCount)); } @@ -688,10 +697,12 @@ export function deadLetterUndelivered( cls: PromoteRejectClass, reason: string, relayDelivery?: { branchId: string; id: string }, + inboxRoot?: string, ): string { validateMessageId(record.id); let inbox: { tmp: string; dlq: string }; - try { + if (inboxRoot !== undefined) inbox = inboxAtRoot(inboxRoot); + else try { assertValidAgentId(agent); inbox = getInbox(agent); } catch (error) { diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index e78ba084..3d09f4c4 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -3,7 +3,7 @@ import { dirname, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; import { randomUUID } from "node:crypto"; import { sanitizeIdentifier } from "../schema/sanitizer.js"; -import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailSyncError, relayAcceptRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; +import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailSyncError, relayAcceptRoot, relayAcceptLockRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; import { acquireMailLockSync } from "./mail-lock.js"; import { LoopDetector } from "./loop-detector.js"; import { FileSystemTransport, resolveTransport, TransportRegistry, type TransportChannel, type TpsMessage } from "./transport.js"; @@ -396,12 +396,16 @@ export class RelayAcceptLockTimeoutError extends Error { } } +let relayAcceptTestHook: ((root: string) => void) | undefined; + +export function setRelayAcceptTestHook(hook?: (root: string) => void): void { + if (process.env.NODE_ENV !== "test") throw new Error("relay acceptance test hook requires test mode"); + relayAcceptTestHook = hook; +} + export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): boolean { MailDeliverBodySchema.shape.id.parse(body.id); if (!/^[a-zA-Z0-9_-]+$/.test(branchId)) throw new Error(`invalid branch id for relayed message ${body.id}`); - // The recipient's lock spans check, publication and marker. For the same - // branch, recipient and id, at most one receiver accepts; the second sees a - // duplicate (identical payload), refuses (differing payload), or gets the timeout refusal. const deadline = Date.now() + relayAcceptLockTimeoutMs(); const acquireLock = (root: string) => { const remaining = deadline - Date.now(); @@ -410,20 +414,22 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): if (!lock) throw new RelayAcceptLockTimeoutError(body.to); return lock; }; - const recipientRoot = relayAcceptRoot(body.to); - // Create the recipient's mailbox (and register its creation for fsync) before - // the lock, in the same order findRelayedRecord used to, so an unlocked - // mkdir never swallows the parent-directory sync. - mkdirMailDirectory(recipientRoot); - const lock = acquireLock(recipientRoot); + const acceptanceRoot = relayAcceptLockRoot(body.to); + mkdirMailDirectory(acceptanceRoot); + const acceptanceLock = acquireLock(acceptanceRoot); + let mailboxLock: ReturnType | undefined; try { + const recipientRoot = relayAcceptRoot(body.to); + mkdirMailDirectory(recipientRoot); + if (recipientRoot !== acceptanceRoot) mailboxLock = acquireLock(recipientRoot); + relayAcceptTestHook?.(recipientRoot); // The marker path includes the branch: a 64-hex id is deterministic, so two branches can send the same one. const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId); const marker = join(acceptedDir, body.id); const legacyMarker = join(getMailDir(), ".relay-accepted", body.id); const existingMarker = existsSync(marker) ? marker : existsSync(legacyMarker) ? legacyMarker : undefined; const delivery = { branchId, id: body.id }; - const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot, acquireLock }); + const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot, heldAcceptanceRoot: acceptanceRoot, acquireLock }); if (existingMarker && existingRecord) { syncMailFile(existingMarker); syncMailDirectory(dirname(existingMarker)); @@ -436,7 +442,7 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): let delivered: boolean; try { - sendMessage(body.to, body.content, body.from, delivery, body.timestamp); + sendMessage(body.to, body.content, body.from, delivery, body.timestamp, body.to, recipientRoot); delivered = true; } catch (e: unknown) { if (e instanceof MailSyncError) throw e; @@ -452,6 +458,7 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): cls, reason, delivery, + recipientRoot, ); } catch (dlqErr: unknown) { console.error( @@ -464,7 +471,8 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): recordAcceptance(acceptedDir, marker); return delivered; } finally { - lock.release(); + mailboxLock?.release(); + acceptanceLock.release(); } } diff --git a/packages/cli/test/relay-accept-single-lock.test.ts b/packages/cli/test/relay-accept-single-lock.test.ts index ef1b2439..859ef58c 100644 --- a/packages/cli/test/relay-accept-single-lock.test.ts +++ b/packages/cli/test/relay-accept-single-lock.test.ts @@ -1,14 +1,10 @@ /** * relay-accept-single-lock.test.ts — cli#561. - * - * For the same branch, recipient and id, at most one receiver accepts; the - * second sees a duplicate (identical payload), refuses (differing payload), - * or gets the timeout refusal. */ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; -import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { getInbox } from "../src/utils/mail.js"; @@ -34,7 +30,24 @@ if (mode === "hold") { process.stdout.write("ready"); process.stdin.once("data", () => { lock.release(); process.exit(0); }); } else { - const { deliverRelayedToLocal } = await import(url); + const { deliverRelayedToLocal, setRelayAcceptTestHook } = await import(url); + const pause = process.env.RELAY_CHILD_PAUSE; + if (pause) { + const { existsSync, writeFileSync } = await import("node:fs"); + setRelayAcceptTestHook((root) => { + writeFileSync(pause + ".ready", root); + const deadline = Date.now() + 10000; + const wait = new Int32Array(new SharedArrayBuffer(4)); + while (!existsSync(pause + ".release")) { + if (Date.now() >= deadline) throw new Error("test pause timed out"); + Atomics.wait(wait, 0, 0, 5); + } + }); + } + if (process.env.RELAY_CHILD_STARTED) { + const { writeFileSync } = await import("node:fs"); + writeFileSync(process.env.RELAY_CHILD_STARTED, ""); + } const ids = JSON.parse(process.env.RELAY_CHILD_IDS); const counts = { delivered: 0, duplicate: 0, refused: 0 }; for (const id of ids) { @@ -43,7 +56,7 @@ if (mode === "hold") { id, from: process.env.RELAY_CHILD_FROM, to: process.env.RELAY_CHILD_TO, - content: process.env.RELAY_CHILD_PREFIX + id, + content: JSON.parse(process.env.RELAY_CHILD_PREFIX) + id, timestamp: ${JSON.stringify(TIMESTAMP)}, }); if (ok) counts.delivered += 1; else counts.duplicate += 1; @@ -87,7 +100,7 @@ function jsonFiles(dir: string): string[] { } /** Run one delivering child and resolve with its counts when it exits. */ -function deliverChild(prefix: string, ids: string[]): Promise { +function deliverChild(prefix: string, ids: string[], env: Record = {}): Promise { const child = spawn("bun", [childScript], { env: { ...process.env, @@ -95,8 +108,9 @@ function deliverChild(prefix: string, ids: string[]): Promise { RELAY_CHILD_BRANCH: BRANCH, RELAY_CHILD_TO: RECIPIENT, RELAY_CHILD_FROM: FROM, - RELAY_CHILD_PREFIX: prefix, + RELAY_CHILD_PREFIX: JSON.stringify(prefix), RELAY_CHILD_IDS: JSON.stringify(ids), + ...env, }, stdio: ["ignore", "pipe", "inherit"], }); @@ -140,7 +154,49 @@ function recordsByDeliveryId(): Map { return byId; } -describe("relay acceptance holds the recipient's mailbox lock across check, publication and marker (cli#561)", () => { +async function waitForFile(path: string): Promise { + const deadline = Date.now() + 10000; + while (!existsSync(path)) { + if (Date.now() >= deadline) throw new Error(`test child did not create ${path}`); + await new Promise((resolve) => setTimeout(resolve, 5)); + } +} + +describe("relay acceptance (cli#561)", () => { + test.each(["new", "dlq"])("routing change before %s publication", async (destination) => { + const id = randomUUID(); + const hostRoot = getInbox(RECIPIENT).root; + const branchRoot = join(root, ".tps", "branch-office", RECIPIENT, "mail"); + const pause = join(root, "pause"); + const started = join(root, "started"); + const prefix = destination === "new" ? "same-" : "\u0000same-"; + const first = deliverChild(prefix, [id], { RELAY_CHILD_PAUSE: pause }); + let second: Promise | undefined; + let results: [Counts, Counts | undefined]; + try { + await waitForFile(pause + ".ready"); + expect(readFileSync(pause + ".ready", "utf8")).toBe(hostRoot); + expect(existsSync(join(hostRoot, ".mail-lock"))).toBe(true); + mkdirSync(branchRoot, { recursive: true }); + second = deliverChild(prefix, [id], { RELAY_CHILD_STARTED: started }); + await waitForFile(started); + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(jsonFiles(join(branchRoot, "new"))).toEqual([]); + expect(existsSync(join(mail, ".relay-accepted", "by-branch", BRANCH, id))).toBe(false); + } finally { + writeFileSync(pause + ".release", ""); + results = await Promise.all([first, second]); + } + const [a, b] = results; + expect(a.delivered).toBe(destination === "new" ? 1 : 0); + expect(b?.delivered).toBe(0); + expect(b?.duplicate).toBe(1); + expect(a.refused + (b?.refused ?? 0)).toBe(0); + const files = jsonFiles(join(hostRoot, destination)); + expect(files).toHaveLength(1); + expect(JSON.parse(readFileSync(join(hostRoot, destination, files[0]!), "utf8")).relayDelivery).toEqual({ branchId: BRANCH, id }); + for (const dir of ["new", "cur", "dlq"]) expect(jsonFiles(join(branchRoot, dir))).toEqual([]); + }, 60_000); test("two processes delivering identical payloads store one record per id and one delivered", async () => { const ids = Array.from({ length: IDS }, () => randomUUID()); const [a, b] = await Promise.all([deliverChild("same-", ids), deliverChild("same-", ids)]); From 115245da40d1cf51000a060fe87e253b671cb1c4 Mon Sep 17 00:00:00 2001 From: flint Date: Fri, 9 Oct 2026 01:08:26 +0000 Subject: [PATCH 04/10] fix(relay): a resend after ACK is judged against its acceptance receipt, not republished (#573) --- .../fixed-535-branch-receiver-before-ack.md | 2 +- .../fixed-573-relay-resend-after-ack.md | 1 + packages/cli/src/utils/relay.ts | 145 +++++++++++-- packages/cli/test/relay-delivery-loss.test.ts | 204 ++++++++++++------ packages/cli/test/relay-review.test.ts | 14 +- 5 files changed, 285 insertions(+), 81 deletions(-) create mode 100644 .changelog/unreleased/fixed-573-relay-resend-after-ack.md diff --git a/.changelog/unreleased/fixed-535-branch-receiver-before-ack.md b/.changelog/unreleased/fixed-535-branch-receiver-before-ack.md index 6252fab2..0a47e521 100644 --- a/.changelog/unreleased/fixed-535-branch-receiver-before-ack.md +++ b/.changelog/unreleased/fixed-535-branch-receiver-before-ack.md @@ -1 +1 @@ -- **Inbox-routed branch relay deliveries are recorded before ACK and reused or republished on resend.** Inbox payload conflicts are refused without ACK. Reply, forward and drop handler outcomes are outside this guarantee. Promotion rejects repeated signed-envelope messageIds. +- **Inbox-routed branch relay deliveries are recorded before ACK.** Inbox payload conflicts are refused without ACK. Reply, forward and drop handler outcomes are outside this guarantee. Promotion rejects repeated signed-envelope messageIds. diff --git a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md new file mode 100644 index 00000000..d5102823 --- /dev/null +++ b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md @@ -0,0 +1 @@ +- **A relay delivery sent again while its acceptance receipt remains on disk is judged against it.** An identical payload is acknowledged as a duplicate with no second record; a differing payload is refused. Receipts are pruned past a bounded age, after which a resend counts as a fresh delivery. diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index 8245ad3b..2ae8eccd 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -1,4 +1,4 @@ -import { appendFileSync, existsSync, mkdirSync, readdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import { appendFileSync, existsSync, mkdirSync, readdirSync, readFileSync, renameSync, statSync, unlinkSync, writeFileSync } from "node:fs"; import { dirname, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; import { randomUUID } from "node:crypto"; @@ -16,7 +16,7 @@ import { registerServiceProxyHandler } from "./service-proxy-host.js"; import { clearHostState, writeHostState, type HostConnectionState, type ServiceHealth } from "./connection-state.js"; import { listServices } from "./service-registry.js"; import snooplogg from "snooplogg"; -import type { ZodError } from "zod"; +import { z, type ZodError } from "zod"; const { log: slog, warn: swarn, error: serror } = snooplogg("tps:relay"); @@ -371,13 +371,118 @@ export function handleIncomingMail(branchId: string, msg: TpsMessage): void { }); } -function recordAcceptance(acceptedDir: string, marker: string): void { +/** + * The payload an acceptance receipt binds to its branch+id: the relay payload + * of the delivery that was accepted, so a later resend can be judged against it + * after the inbox record is gone. + */ +const RelayAcceptReceiptSchema = z.object({ + from: MailDeliverBodySchema.shape.from, + to: MailDeliverBodySchema.shape.to, + body: z.string(), + timestamp: MailDeliverBodySchema.shape.timestamp, +}); +type RelayAcceptReceipt = z.infer; + +function acceptReceipt(body: MailDeliverBody): RelayAcceptReceipt { + return { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }; +} + +/** Receipts older than this no longer block a resend: a resend that late is a fresh delivery. */ +const RELAY_ACCEPT_RECEIPT_TTL_MS = 7 * 24 * 60 * 60 * 1000; + +function relayAcceptReceiptTtlMs(): number { + const raw = Number(process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS); + return Number.isFinite(raw) && raw > 0 ? raw : RELAY_ACCEPT_RECEIPT_TTL_MS; +} + +/** The most directory entries one prune pass examines, so the prune's work is bounded however large the directory is. */ +const RELAY_ACCEPT_PRUNE_MAX = 4096; + +/** + * Remove acceptance receipts older than `ttlMs` from one branch's directory and + * return how many were removed. The scan examines at most + * RELAY_ACCEPT_PRUNE_MAX entries, so a call's work has a hard bound that does + * not depend on the directory's size. + */ +export function pruneRelayAcceptanceReceipts(acceptedDir: string, now = Date.now(), ttlMs = relayAcceptReceiptTtlMs()): number { + if (!existsSync(acceptedDir)) return 0; + let removed = 0; + let examined = 0; + for (const entry of readdirSync(acceptedDir, { withFileTypes: true })) { + if (examined >= RELAY_ACCEPT_PRUNE_MAX) break; + examined++; + if (!entry.isFile()) continue; + const path = join(acceptedDir, entry.name); + let mtimeMs: number; + try { + mtimeMs = statSync(path).mtimeMs; + } catch { + continue; // gone or undatable: nothing to prune + } + if (now - mtimeMs <= ttlMs) continue; + try { + unlinkSync(path); + removed++; + } catch { + // The resend decision already ignores an expired receipt, so a failed + // unlink does not block a resend; a later pass retries. + } + } + if (removed > 0) syncMailDirectory(acceptedDir); + return removed; +} + +function recordAcceptance(acceptedDir: string, marker: string, receipt: RelayAcceptReceipt): void { mkdirMailDirectory(acceptedDir); const tmp = `${marker}.tmp`; - writeFileSync(tmp, "", { mode: 0o600 }); + writeFileSync(tmp, JSON.stringify(receipt), { mode: 0o600 }); syncMailFile(tmp); renameSync(tmp, marker); syncMailDirectory(acceptedDir); + // Bounded hygiene: whenever a new receipt is written, drop the ones past the + // TTL. + try { + pruneRelayAcceptanceReceipts(acceptedDir); + } catch (error: unknown) { + console.error(`[relay] acceptance receipt prune failed: ${error instanceof Error ? error.message : String(error)}`); + } +} + +/** + * The acceptance marker for a delivery, or undefined when there is none or the + * one present has aged past the receipt TTL (an expired receipt no longer + * blocks). The marker is keyed by branch+id; its receipt records the payload. + */ +function existingAcceptanceMarker(marker: string, legacyMarker: string): string | undefined { + const path = existsSync(marker) ? marker : existsSync(legacyMarker) ? legacyMarker : undefined; + if (!path) return undefined; + try { + if (Date.now() - statSync(path).mtimeMs > relayAcceptReceiptTtlMs()) return undefined; + } catch { + // A stat failure keeps it and lets the verdict read it (fail closed). + } + return path; +} + +/** + * Whether a surviving marker's receipt is for the same payload. A marker with + * no readable receipt (an empty pre-receipt marker, or a corrupt one) is a + * conflict: the delivery's identity is unproven, so publishing again could + * deliver a second copy. A read failure propagates and refuses the delivery. + */ +function acceptanceReceiptMatches(path: string, expected: RelayAcceptReceipt): boolean { + const raw = readFileSync(path, "utf-8"); + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return false; + } + const receipt = RelayAcceptReceiptSchema.safeParse(parsed); + if (!receipt.success) return false; + const r = receipt.data; + return r.from === expected.from && r.to === expected.to && r.body === expected.body && r.timestamp === expected.timestamp; } /** The shared wait budget for relay acceptance's mailbox locks. */ @@ -427,17 +532,33 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId); const marker = join(acceptedDir, body.id); const legacyMarker = join(getMailDir(), ".relay-accepted", body.id); - const existingMarker = existsSync(marker) ? marker : existsSync(legacyMarker) ? legacyMarker : undefined; + const receipt = acceptReceipt(body); + const existingMarker = existingAcceptanceMarker(marker, legacyMarker); const delivery = { branchId, id: body.id }; const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot, heldAcceptanceRoot: acceptanceRoot, acquireLock }); - if (existingMarker && existingRecord) { - syncMailFile(existingMarker); - syncMailDirectory(dirname(existingMarker)); + if (existingRecord) { + // The record is still live: this delivery is already stored. Keep a + // receipt (or upgrade a pre-receipt marker) so a resend after the record is + // ACKed is still recognised. + if (existingMarker && acceptanceReceiptMatches(existingMarker, receipt)) { + syncMailFile(existingMarker); + syncMailDirectory(dirname(existingMarker)); + } else { + recordAcceptance(acceptedDir, marker, receipt); + } return false; } - if (!existingMarker && existingRecord) { - recordAcceptance(acceptedDir, marker); - return false; + if (existingMarker) { + // No record, but this branch+id was accepted before (a normal mail ACK + // removes the record). A matching payload is a duplicate; anything else — + // a different payload, or a marker with no usable receipt — is refused, + // publishing no second record. + if (acceptanceReceiptMatches(existingMarker, receipt)) { + syncMailFile(existingMarker); + syncMailDirectory(dirname(existingMarker)); + return false; + } + throw new Error(`relayed delivery conflict for branch ${branchId} message ${body.id}`); } let delivered: boolean; @@ -468,7 +589,7 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): } delivered = false; } - recordAcceptance(acceptedDir, marker); + recordAcceptance(acceptedDir, marker, receipt); return delivered; } finally { mailboxLock?.release(); diff --git a/packages/cli/test/relay-delivery-loss.test.ts b/packages/cli/test/relay-delivery-loss.test.ts index b1961ec6..24d24164 100644 --- a/packages/cli/test/relay-delivery-loss.test.ts +++ b/packages/cli/test/relay-delivery-loss.test.ts @@ -4,7 +4,7 @@ import * as fs from "node:fs"; import { randomUUID } from "node:crypto"; import { join } from "node:path"; import { tmpdir } from "node:os"; -import { gcMessages, getInbox, MAX_INBOX_MESSAGES, sendMessage } from "../src/utils/mail.js"; +import { gcMessages, getInbox, MAX_INBOX_MESSAGES, sendMessage, ackMessageAtPath } from "../src/utils/mail.js"; import { runBranch, writeBranchConf } from "../src/commands/branch.js"; import { runMail } from "../src/commands/mail.js"; import { syncRemoteBranch, connectAndKeepAlive, deliverRelayedToLocal } from "../src/utils/relay.js"; @@ -309,9 +309,6 @@ for (const entry of ["sync", "connect"] as const) { const source = join(dir, "incomplete.json"); const incomplete = JSON.stringify({ relayDelivery: { branchId: "remote", id: body.id } }); fs.writeFileSync(source, incomplete); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); - fs.mkdirSync(join(marker, ".."), { recursive: true }); - fs.writeFileSync(marker, ""); const errors = spyOn(console, "error").mockImplementation(() => {}); await start(); const send = channel.send; @@ -500,14 +497,106 @@ for (const entry of ["sync", "connect"] as const) { expect(drainOutbox(false)).toEqual([]); }); - for (const state of ["removed-unread", "consumed", "legacy"] as const) { - test(`existing ${state} marker with no record republishes before ACK`, async () => { + async function ackOnlyRecord(): Promise { + const inbox = getInbox("local"); + const [file] = jsonFiles(inbox.fresh); + ackMessageAtPath(join(inbox.fresh, file)); + expect(jsonFiles(inbox.fresh)).toEqual([]); + } + + test("an identical resend after ACK is a duplicate: one record, no second delivered", async () => { + const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "once", SEEDS))); + await start(); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); + expect(acks.map((ack) => ack.body)).toEqual([{ id: body.id, accepted: true }]); + await ackOnlyRecord(); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body }); + expect(acks.map((ack) => ack.body)).toEqual([{ id: body.id, accepted: true }, { id: body.id, accepted: true }]); + expect(jsonFiles(getInbox("local").fresh)).toEqual([]); + expect(jsonFiles(getInbox("local").cur)).toEqual([]); + expect(drainOutbox(false)).toEqual([]); + }); + + test("a differing resend after ACK is refused without a second delivery", async () => { + const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "once", SEEDS))); + const errors = spyOn(console, "error").mockImplementation(() => {}); + await start(); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); + await ackOnlyRecord(); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body: { ...body, content: "changed payload" } }); + expect(acks.map((ack) => ack.body)).toEqual([{ id: body.id, accepted: true }]); + expect(jsonFiles(getInbox("local").fresh)).toEqual([]); + expect(jsonFiles(getInbox("local").dlq)).toEqual([]); + expect(errors.mock.calls.flat().join("\n")).toContain("conflict"); + }); + + test("a receipt older than the prune bound no longer blocks a resend", async () => { + const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "late", SEEDS))); + await start(); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); + await ackOnlyRecord(); + const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS = "1000"; + const old = new Date(Date.now() - 5000); + fs.utimesSync(marker, old, old); + try { + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body }); + } finally { + delete process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS; + } + expect(acks.map((ack) => ack.body)).toEqual([{ id: body.id, accepted: true }, { id: body.id, accepted: true }]); + expect(jsonFiles(getInbox("local").fresh)).toHaveLength(1); + }); + + test("acceptance receipts past the prune bound are removed when a new one is written", async () => { + const first = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "first accepted", SEEDS))); + await start(); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body: first }); + const accepted = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"); + const firstMarker = join(accepted, first.id); + expect(fs.existsSync(firstMarker)).toBe(true); + process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS = "1000"; + const old = new Date(Date.now() - 5000); + fs.utimesSync(firstMarker, old, old); + const second = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "second accepted", SEEDS))); + try { + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body: second }); + } finally { + delete process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS; + } + expect(fs.existsSync(firstMarker)).toBe(false); + expect(fs.existsSync(join(accepted, second.id))).toBe(true); + expect(jsonFiles(getInbox("local").fresh)).toHaveLength(2); + }); + + test("a marker read failure refuses the delivery without an ACK", async () => { + const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "marker read", SEEDS))); + const errors = spyOn(console, "error").mockImplementation(() => {}); + await start(); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); + await ackOnlyRecord(); + const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const read = fs.readFileSync; + const fault = spyOn(fs, "readFileSync").mockImplementation((path, options) => { + if (String(path) === marker) throw Object.assign(new Error("injected marker read denied"), { code: "EACCES" }); + return read(path, options as BufferEncoding); + }); + try { + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body }); + } finally { + fault.mockRestore(); + } + expect(acks.length).toBe(1); + expect(jsonFiles(getInbox("local").fresh)).toEqual([]); + expect(errors.mock.calls.flat().join("\n")).toContain("injected marker read denied"); + }); + + for (const state of ["removed-unread", "consumed"] as const) { + test(`an existing ${state} receipt with no record dedups an identical resend and is ACKed`, async () => { const envelope = buildSignedEnvelope("remote", "local", state, SEEDS); const body = queue(JSON.stringify(envelope)); const inbox = getInbox("local"); - const marker = state === "legacy" - ? join(process.env.TPS_MAIL_DIR!, ".relay-accepted", body.id) - : join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); const client = new MailClient(process.env.TPS_MAIL_DIR!, undefined, "local", { async getAgent(id) { const seed = SEEDS[id as keyof typeof SEEDS]; @@ -517,54 +606,61 @@ for (const entry of ["sync", "connect"] as const) { spyOn(console, "error").mockImplementation(() => {}); await start(); const msg: TpsMessage = { type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }; - if (state === "legacy") { - fs.mkdirSync(join(process.env.TPS_MAIL_DIR!, ".relay-accepted"), { recursive: true }); - fs.writeFileSync(marker, ""); - } else { - const send = channel.send; - channel.send = async () => { throw new Error("injected lost ACK"); }; - await deliverDirect(msg); - channel.send = send; - expect(acks).toEqual([]); - expect(fs.readFileSync(marker, "utf8")).toBe(""); - expect(jsonFiles(inbox.fresh)).toHaveLength(1); - if (state === "consumed") { - expect(await client.checkNewMail()).toHaveLength(1); - expect(hasCommittedMessageId(join(process.env.TPS_MAIL_DIR!, "local"), envelope.messageId)).toBe(true); - expect(envelope.messageId).not.toBe(body.id); - } else { - expect(hasCommittedMessageId(join(process.env.TPS_MAIL_DIR!, "local"), envelope.messageId)).toBe(false); - } - const clock = spyOn(Date, "now").mockReturnValue(Date.now() + 2000); - try { expect(gcMessages("local", "24h", undefined, "1s")).toBe(1); } - finally { clock.mockRestore(); } - expect(jsonFiles(inbox.fresh)).toEqual([]); - expect(jsonFiles(inbox.cur)).toEqual([]); + const send = channel.send; + channel.send = async () => { throw new Error("injected lost ACK"); }; + await deliverDirect(msg); + channel.send = send; + expect(acks).toEqual([]); + expect(jsonFiles(inbox.fresh)).toHaveLength(1); + if (state === "consumed") { + expect(await client.checkNewMail()).toHaveLength(1); + expect(hasCommittedMessageId(join(process.env.TPS_MAIL_DIR!, "local"), envelope.messageId)).toBe(true); } + const clock = spyOn(Date, "now").mockReturnValue(Date.now() + 2000); + try { expect(gcMessages("local", "24h", undefined, "1s")).toBe(1); } + finally { clock.mockRestore(); } + expect(jsonFiles(inbox.fresh)).toEqual([]); + expect(jsonFiles(inbox.cur)).toEqual([]); + const recordsAtAck: number[] = []; - const send = channel.send; + const ackSend = channel.send; channel.send = async (ack) => { if (ack.type === MSG_MAIL_ACK) recordsAtAck.push(jsonFiles(inbox.fresh).length); - return send(ack); + return ackSend(ack); }; await deliverDirect(msg); - expect(recordsAtAck).toEqual([1]); - expect(jsonFiles(inbox.fresh)).toHaveLength(1); + expect(recordsAtAck).toEqual([0]); + expect(jsonFiles(inbox.fresh)).toEqual([]); expect(jsonFiles(inbox.cur)).toEqual([]); expect(jsonFiles(inbox.dlq)).toEqual([]); expect(acks.map((ack) => ack.body)).toEqual([{ id: body.id, accepted: true }]); expect(drainOutbox(false)).toEqual([]); - const promoted = await client.checkNewMail(); - expect(promoted).toHaveLength(state === "consumed" ? 0 : 1); - if (state === "consumed") { - const [file] = jsonFiles(inbox.dlq); - expect(fs.readFileSync(join(inbox.dlq, `${file}.reason`), "utf8")).toContain("class: replay"); - } + expect(await client.checkNewMail()).toEqual([]); + expect(fs.existsSync(marker)).toBe(true); }); } + test("an empty pre-receipt marker with no record refuses without an ACK", async () => { + const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "legacy", SEEDS))); + const inbox = getInbox("local"); + const legacy = join(process.env.TPS_MAIL_DIR!, ".relay-accepted"); + const marker = join(legacy, body.id); + const errors = spyOn(console, "error").mockImplementation(() => {}); + await start(); + fs.mkdirSync(legacy, { recursive: true }); + fs.writeFileSync(marker, ""); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); + expect(acks).toEqual([]); + expect(errors.mock.calls.flat().join("\n")).toContain("conflict"); + expect(jsonFiles(inbox.fresh)).toEqual([]); + expect(jsonFiles(inbox.cur)).toEqual([]); + expect(jsonFiles(inbox.dlq)).toEqual([]); + expect(drainOutbox(false).map((item) => item.id)).toEqual([body.id]); + expect(fs.readFileSync(marker, "utf8")).toBe(""); + }); + for (const signature of ["valid", "invalid"] as const) { - test(`resend with an unrelated consumed id and ${signature} signature republishes before ACK`, async () => { + test(`a delivery whose envelope reuses a consumed message id is refused at promotion (${signature} signature)`, async () => { const original = buildSignedEnvelope("remote", "local", "unrelated", SEEDS); sendMessage("local", JSON.stringify(original), "remote"); const client = new MailClient(process.env.TPS_MAIL_DIR!, undefined, "local", { @@ -578,29 +674,9 @@ for (const entry of ["sync", "connect"] as const) { if (signature === "invalid") envelope.body = "tampered"; const body = queue(JSON.stringify(envelope)); const inbox = getInbox("local"); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); spyOn(console, "error").mockImplementation(() => {}); await start(); - const msg: TpsMessage = { type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }; - const send = channel.send; - channel.send = async () => { throw new Error("injected lost ACK"); }; - await deliverDirect(msg); - channel.send = send; - expect(acks).toEqual([]); - expect(fs.readFileSync(marker, "utf8")).toBe(""); - const clock = spyOn(Date, "now").mockReturnValue(Date.now() + 2000); - try { expect(gcMessages("local", "24h", undefined, "1s")).toBe(2); } - finally { clock.mockRestore(); } - expect(jsonFiles(inbox.fresh)).toEqual([]); - expect(jsonFiles(inbox.cur)).toEqual([]); - expect(jsonFiles(inbox.dlq)).toEqual([]); - const recordsAtAck: number[] = []; - channel.send = async (ack) => { - if (ack.type === MSG_MAIL_ACK) recordsAtAck.push(jsonFiles(inbox.fresh).length); - return send(ack); - }; - await deliverDirect(msg); - expect(recordsAtAck).toEqual([1]); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); expect(acks.map((ack) => ack.body)).toEqual([{ id: body.id, accepted: true }]); expect(drainOutbox(false)).toEqual([]); expect(await client.checkNewMail()).toEqual([]); diff --git a/packages/cli/test/relay-review.test.ts b/packages/cli/test/relay-review.test.ts index 6fd36178..931dcf41 100644 --- a/packages/cli/test/relay-review.test.ts +++ b/packages/cli/test/relay-review.test.ts @@ -98,11 +98,16 @@ describe("relay review regressions", () => { expect(deliverRelayedToLocal("remote", message)).toBe(false); expect(synced).toContain(acceptedRoot); expect(synced).toContain(mail); - expect(fs.readFileSync(join(acceptedRoot, "by-branch", "remote", message.id), "utf8")).toBe(""); + expect(JSON.parse(fs.readFileSync(join(acceptedRoot, "by-branch", "remote", message.id), "utf8"))).toEqual({ + from: message.from, + to: message.to, + body: message.content, + timestamp: message.timestamp, + }); }); for (const dir of ["new", "cur", "dlq"] as const) { - test(`a truncated ${dir} record is preserved and reported while a marked delivery is republished`, () => { + test(`a truncated ${dir} record is preserved and reported; a marked delivery with no usable receipt is refused`, () => { const inbox = getInbox("local"); const message = body(); const corrupt = join(inbox.root, dir, "truncated.json"); @@ -112,7 +117,7 @@ describe("relay review regressions", () => { fs.mkdirSync(accepted, { recursive: true }); fs.writeFileSync(join(accepted, message.id), ""); const errors = spyOn(console, "error").mockImplementation(() => {}); - expect(deliverRelayedToLocal("remote", message)).toBe(true); + expect(() => deliverRelayedToLocal("remote", message)).toThrow(`relayed delivery conflict for branch remote message ${message.id}`); const quarantine = join(inbox.root, "quarantine"); const files = fs.readdirSync(quarantine).filter((file) => file.endsWith(".json")); expect(files).toHaveLength(1); @@ -121,7 +126,8 @@ describe("relay review regressions", () => { expect(fs.readFileSync(`${path}.reason`, "utf8")).toContain(corrupt); expect(errors.mock.calls.flat().join("\n")).toContain(corrupt); expect(fs.readdirSync(join(inbox.root, dir))).not.toContain("truncated.json"); - expect(deliverRelayedToLocal("remote", message)).toBe(false); + fs.rmSync(join(accepted, message.id)); + expect(deliverRelayedToLocal("remote", message)).toBe(true); const records = fs.readdirSync(inbox.fresh).filter((file) => file.endsWith(".json")); expect(records).toHaveLength(1); expect(JSON.parse(fs.readFileSync(join(inbox.fresh, records[0]!), "utf8")).body).toBe(message.content); From 70992dc10776f8979fabafa3c4ef927221901a5d Mon Sep 17 00:00:00 2001 From: flint Date: Thu, 8 Oct 2026 23:51:25 -0700 Subject: [PATCH 05/10] fix(relay): accept a delivery only when record and receipt are both durable; one lock per delivery id; bounded receipt retention Co-Authored-By: Claude Opus 5.5 --- .../fixed-573-relay-resend-after-ack.md | 2 +- packages/cli/src/utils/mail.ts | 15 +- packages/cli/src/utils/relay.ts | 214 ++++++++++++------ .../cli/test/relay-accept-invariants.test.ts | 163 +++++++++++++ .../cli/test/relay-accept-single-lock.test.ts | 65 +++++- packages/cli/test/relay-delivery-loss.test.ts | 82 +++---- packages/cli/test/relay-review.test.ts | 22 +- 7 files changed, 429 insertions(+), 134 deletions(-) create mode 100644 packages/cli/test/relay-accept-invariants.test.ts diff --git a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md index d5102823..8731b41a 100644 --- a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md +++ b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md @@ -1 +1 @@ -- **A relay delivery sent again while its acceptance receipt remains on disk is judged against it.** An identical payload is acknowledged as a duplicate with no second record; a differing payload is refused. Receipts are pruned past a bounded age, after which a resend counts as a fresh delivery. +- **Deduplicate relay resends after local ACK while the receipt remains.** Failed acceptance writes roll back new records; old receipt buckets are pruned on relay start and a timer. diff --git a/packages/cli/src/utils/mail.ts b/packages/cli/src/utils/mail.ts index deb6dfae..2fc2a9b9 100644 --- a/packages/cli/src/utils/mail.ts +++ b/packages/cli/src/utils/mail.ts @@ -394,6 +394,9 @@ export class MailSyncError extends Error { } } +export class MailSendInputError extends Error {} +export class MailInboxFullError extends Error {} + export function syncMailFile(path: string): void { try { const fd = openSync(path, "r"); @@ -568,10 +571,14 @@ export function findRelayedRecord(agent: string, delivery: { branchId: string; i } export function sendMessage(to: string, body: string, from?: string, relayDelivery?: { branchId: string; id: string }, senderTimestamp?: string, wireRecipient = to, inboxRoot?: string): MailMessage & { filePath: string } { - assertValidAgentId(to); const sender = from || "unknown"; - assertValidAgentId(sender); - assertValidBody(body); + try { + assertValidAgentId(to); + assertValidAgentId(sender); + assertValidBody(body); + } catch (error) { + throw new MailSendInputError(error instanceof Error ? error.message : String(error)); + } // Guard: when running in test mode or when the caller has explicitly // opted in, refuse to write to the default ~/.tps/mail/ directory @@ -588,7 +595,7 @@ export function sendMessage(to: string, body: string, from?: string, relayDelive const inbox = inboxRoot === undefined ? getInbox(to) : inboxAtRoot(inboxRoot); const quotaCount = readdirSync(inbox.fresh).filter((f) => f.endsWith(".json")).length; if (quotaCount >= MAX_INBOX_MESSAGES) { - throw new Error(inboxFullMessage(to, quotaCount)); + throw new MailInboxFullError(inboxFullMessage(to, quotaCount)); } const timestamp = new Date().toISOString(); diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index 2ae8eccd..361d4754 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -1,9 +1,9 @@ -import { appendFileSync, existsSync, mkdirSync, readdirSync, readFileSync, renameSync, statSync, unlinkSync, writeFileSync } from "node:fs"; +import { appendFileSync, existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, statSync, unlinkSync, writeFileSync } from "node:fs"; import { dirname, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; import { randomUUID } from "node:crypto"; import { sanitizeIdentifier } from "../schema/sanitizer.js"; -import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailSyncError, relayAcceptRoot, relayAcceptLockRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; +import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailInboxFullError, MailSendInputError, relayAcceptRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; import { acquireMailLockSync } from "./mail-lock.js"; import { LoopDetector } from "./loop-detector.js"; import { FileSystemTransport, resolveTransport, TransportRegistry, type TransportChannel, type TpsMessage } from "./transport.js"; @@ -388,7 +388,12 @@ function acceptReceipt(body: MailDeliverBody): RelayAcceptReceipt { return { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }; } -/** Receipts older than this no longer block a resend: a resend that late is a fresh delivery. */ +function refusalText(error: unknown, content: string): string { + const message = error instanceof Error ? error.message : String(error); + return content ? message.replaceAll(content, "[redacted]") : message; +} + +/** A receipt stops blocking resends after this age. */ const RELAY_ACCEPT_RECEIPT_TTL_MS = 7 * 24 * 60 * 60 * 1000; function relayAcceptReceiptTtlMs(): number { @@ -396,56 +401,85 @@ function relayAcceptReceiptTtlMs(): number { return Number.isFinite(raw) && raw > 0 ? raw : RELAY_ACCEPT_RECEIPT_TTL_MS; } -/** The most directory entries one prune pass examines, so the prune's work is bounded however large the directory is. */ -const RELAY_ACCEPT_PRUNE_MAX = 4096; +function receiptBucket(now = Date.now()): string { + return new Date(now).toISOString().slice(0, 10); +} -/** - * Remove acceptance receipts older than `ttlMs` from one branch's directory and - * return how many were removed. The scan examines at most - * RELAY_ACCEPT_PRUNE_MAX entries, so a call's work has a hard bound that does - * not depend on the directory's size. - */ +export function relayAcceptanceReceiptPath(branchId: string, id: string, now = Date.now()): string { + return join(getMailDir(), ".relay-accepted", "by-branch", branchId, receiptBucket(now), id); +} + +/** Remove expired day buckets. */ export function pruneRelayAcceptanceReceipts(acceptedDir: string, now = Date.now(), ttlMs = relayAcceptReceiptTtlMs()): number { if (!existsSync(acceptedDir)) return 0; let removed = 0; - let examined = 0; for (const entry of readdirSync(acceptedDir, { withFileTypes: true })) { - if (examined >= RELAY_ACCEPT_PRUNE_MAX) break; - examined++; - if (!entry.isFile()) continue; - const path = join(acceptedDir, entry.name); - let mtimeMs: number; + if (!entry.isDirectory() || !/^\d{4}-\d{2}-\d{2}$/.test(entry.name)) continue; + const end = Date.parse(`${entry.name}T00:00:00.000Z`) + 24 * 60 * 60 * 1000; + if (!Number.isFinite(end) || end > now - ttlMs) continue; try { - mtimeMs = statSync(path).mtimeMs; - } catch { - continue; // gone or undatable: nothing to prune - } - if (now - mtimeMs <= ttlMs) continue; - try { - unlinkSync(path); + rmSync(join(acceptedDir, entry.name), { recursive: true }); removed++; - } catch { - // The resend decision already ignores an expired receipt, so a failed - // unlink does not block a resend; a later pass retries. - } + } catch {} } if (removed > 0) syncMailDirectory(acceptedDir); return removed; } +function startReceiptPrune(branchId?: string): () => void { + if (branchId && !/^[a-zA-Z0-9_-]+$/.test(branchId)) throw new Error("invalid branch id for receipt prune"); + const pass = () => { + const root = join(getMailDir(), ".relay-accepted", "by-branch"); + const prune = (dir: string) => { + try { pruneRelayAcceptanceReceipts(dir); } + catch { console.error("[relay] acceptance receipt prune failed"); } + }; + try { + if (branchId) prune(join(root, branchId)); + else if (existsSync(root)) { + for (const entry of readdirSync(root, { withFileTypes: true })) { + if (entry.isDirectory()) prune(join(root, entry.name)); + } + } + } catch { console.error("[relay] acceptance receipt prune failed"); } + }; + pass(); + const configured = Number(process.env.TPS_RELAY_ACCEPT_PRUNE_INTERVAL_MS); + const timer = setInterval(pass, Number.isFinite(configured) && configured > 0 ? configured : 60_000); + timer.unref(); + return () => clearInterval(timer); +} + +class ReceiptRecoveryIncompleteError extends Error {} + +function receiptMayExist(path: string): boolean { + try { lstatSync(path); return true; } + catch (error) { + const code = (error as NodeJS.ErrnoException).code; + return code !== "ENOENT" && code !== "ENOTDIR"; + } +} + function recordAcceptance(acceptedDir: string, marker: string, receipt: RelayAcceptReceipt): void { - mkdirMailDirectory(acceptedDir); + mkdirMailDirectory(dirname(marker)); const tmp = `${marker}.tmp`; - writeFileSync(tmp, JSON.stringify(receipt), { mode: 0o600 }); - syncMailFile(tmp); - renameSync(tmp, marker); - syncMailDirectory(acceptedDir); - // Bounded hygiene: whenever a new receipt is written, drop the ones past the - // TTL. + try { + writeFileSync(tmp, JSON.stringify(receipt), { mode: 0o600, flag: "wx" }); + syncMailFile(tmp); + renameSync(tmp, marker); + syncMailDirectory(dirname(marker)); + } catch (error) { + if (receiptMayExist(marker)) { + try { unlinkSync(marker); syncMailDirectory(dirname(marker)); } + catch { throw new ReceiptRecoveryIncompleteError("relay receipt recovery incomplete; retry may duplicate"); } + } + try { if (existsSync(tmp)) unlinkSync(tmp); } catch {} + throw error; + } try { pruneRelayAcceptanceReceipts(acceptedDir); - } catch (error: unknown) { - console.error(`[relay] acceptance receipt prune failed: ${error instanceof Error ? error.message : String(error)}`); + } catch { + console.error("[relay] acceptance receipt prune failed"); } } @@ -454,15 +488,20 @@ function recordAcceptance(acceptedDir: string, marker: string, receipt: RelayAcc * one present has aged past the receipt TTL (an expired receipt no longer * blocks). The marker is keyed by branch+id; its receipt records the payload. */ -function existingAcceptanceMarker(marker: string, legacyMarker: string): string | undefined { - const path = existsSync(marker) ? marker : existsSync(legacyMarker) ? legacyMarker : undefined; - if (!path) return undefined; - try { - if (Date.now() - statSync(path).mtimeMs > relayAcceptReceiptTtlMs()) return undefined; - } catch { - // A stat failure keeps it and lets the verdict read it (fail closed). +function existingAcceptanceMarker(acceptedDir: string, id: string, legacyMarker: string): string | undefined { + if (receiptMayExist(acceptedDir)) { + for (const bucket of readdirSync(acceptedDir, { withFileTypes: true }).filter((entry) => entry.isDirectory()).map((entry) => entry.name).sort().reverse()) { + const path = join(acceptedDir, bucket, id); + if (!receiptMayExist(path)) continue; + try { + if (Date.now() - statSync(path).mtimeMs > relayAcceptReceiptTtlMs()) continue; + } catch {} + return path; + } } - return path; + if (!receiptMayExist(legacyMarker)) return undefined; + try { if (Date.now() - statSync(legacyMarker).mtimeMs > relayAcceptReceiptTtlMs()) return undefined; } catch {} + return legacyMarker; } /** @@ -485,7 +524,25 @@ function acceptanceReceiptMatches(path: string, expected: RelayAcceptReceipt): b return r.from === expected.from && r.to === expected.to && r.body === expected.body && r.timestamp === expected.timestamp; } -/** The shared wait budget for relay acceptance's mailbox locks. */ +function undoNewRelayRecord(root: string, delivery: { branchId: string; id: string }): void { + for (const dir of ["tmp", "new", "dlq"]) { + const parent = join(root, dir); + if (!existsSync(parent)) continue; + for (const entry of readdirSync(parent, { withFileTypes: true })) { + if (!entry.isFile() || !entry.name.endsWith(".json")) continue; + const path = join(parent, entry.name); + let record: { relayDelivery?: { branchId?: string; id?: string } }; + try { record = JSON.parse(readFileSync(path, "utf8")); } + catch { throw new Error("relay record recovery incomplete; delivery may be retried"); } + if (record.relayDelivery?.branchId !== delivery.branchId || record.relayDelivery.id !== delivery.id) continue; + unlinkSync(path); + if (dir === "dlq" && existsSync(`${path}.reason`)) unlinkSync(`${path}.reason`); + syncMailDirectory(parent); + } + } +} + +/** Wait budget for relay acceptance locks. */ const RELAY_ACCEPT_LOCK_TIMEOUT_MS = 2000; function relayAcceptLockTimeoutMs(): number { @@ -493,10 +550,10 @@ function relayAcceptLockTimeoutMs(): number { return Number.isFinite(raw) && raw > 0 ? raw : RELAY_ACCEPT_LOCK_TIMEOUT_MS; } -/** The named refusal when an acceptance mailbox lock times out. */ +/** The named refusal when relay acceptance waits too long for a lock. */ export class RelayAcceptLockTimeoutError extends Error { constructor(recipient: string) { - super(`relay acceptance timed out waiting for the mailbox lock for ${recipient}`); + super(`relay acceptance timed out waiting for a lock for ${recipient}`); this.name = "RelayAcceptLockTimeoutError"; } } @@ -519,7 +576,7 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): if (!lock) throw new RelayAcceptLockTimeoutError(body.to); return lock; }; - const acceptanceRoot = relayAcceptLockRoot(body.to); + const acceptanceRoot = join(getMailDir(), ".relay-accept-locks", branchId, body.id); mkdirMailDirectory(acceptanceRoot); const acceptanceLock = acquireLock(acceptanceRoot); let mailboxLock: ReturnType | undefined; @@ -530,21 +587,22 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): relayAcceptTestHook?.(recipientRoot); // The marker path includes the branch: a 64-hex id is deterministic, so two branches can send the same one. const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId); - const marker = join(acceptedDir, body.id); + const marker = relayAcceptanceReceiptPath(branchId, body.id); const legacyMarker = join(getMailDir(), ".relay-accepted", body.id); const receipt = acceptReceipt(body); - const existingMarker = existingAcceptanceMarker(marker, legacyMarker); + const existingMarker = existingAcceptanceMarker(acceptedDir, body.id, legacyMarker); const delivery = { branchId, id: body.id }; const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot, heldAcceptanceRoot: acceptanceRoot, acquireLock }); if (existingRecord) { - // The record is still live: this delivery is already stored. Keep a - // receipt (or upgrade a pre-receipt marker) so a resend after the record is - // ACKed is still recognised. + if (existingMarker && existingMarker !== legacyMarker && !acceptanceReceiptMatches(existingMarker, receipt)) { + throw new Error(`relayed delivery conflict for branch ${branchId} message ${body.id}`); + } if (existingMarker && acceptanceReceiptMatches(existingMarker, receipt)) { syncMailFile(existingMarker); syncMailDirectory(dirname(existingMarker)); } else { - recordAcceptance(acceptedDir, marker, receipt); + try { recordAcceptance(acceptedDir, marker, receipt); } + catch { throw new Error(`relay receipt recovery incomplete for message ${body.id}; retry may duplicate`); } } return false; } @@ -566,30 +624,38 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): sendMessage(body.to, body.content, body.from, delivery, body.timestamp, body.to, recipientRoot); delivered = true; } catch (e: unknown) { - if (e instanceof MailSyncError) throw e; + try { undoNewRelayRecord(recipientRoot, delivery); } + catch { throw new Error(`relay record recovery incomplete for message ${body.id}; retry may duplicate`); } const reason = e instanceof Error ? e.message : String(e); - const cls: PromoteRejectClass = /inbox full/i.test(reason) + const cls: PromoteRejectClass | null = e instanceof MailInboxFullError ? "inbox-full" - : /^(Invalid agent id|Message body)/.test(reason) ? "invalid" : "storage-unavailable"; - console.error(`[relay] local delivery failed for message ${body.id} to ${body.to}: ${reason}`); + : e instanceof MailSendInputError ? "invalid" : null; + if (!cls) throw new Error("relay inbox write failed; retry delivery"); + console.error(`[relay] local delivery failed for message ${body.id} to ${body.to}: ${refusalText(e, body.content)}`); try { deadLetterUndelivered( body.to, { id: body.id, from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, cls, - reason, + refusalText(e, body.content), delivery, recipientRoot, ); - } catch (dlqErr: unknown) { - console.error( - `[relay] dead-letter failed for message ${body.id} to ${body.to}: ${dlqErr instanceof Error ? dlqErr.message : String(dlqErr)}`, - ); - throw dlqErr; + } catch { + try { undoNewRelayRecord(recipientRoot, delivery); } + catch { throw new Error(`relay record recovery incomplete for message ${body.id}; retry may duplicate`); } + console.error(`[relay] dead-letter failed for message ${body.id} to ${body.to}`); + throw new Error("relay dead-letter failed; retry delivery"); } delivered = false; } - recordAcceptance(acceptedDir, marker, receipt); + try { recordAcceptance(acceptedDir, marker, receipt); } + catch (error) { + if (error instanceof ReceiptRecoveryIncompleteError || receiptMayExist(marker)) throw new Error(`relay receipt recovery incomplete for message ${body.id}; retry may duplicate`); + try { undoNewRelayRecord(recipientRoot, delivery); } + catch { throw new Error(`relay record recovery incomplete for message ${body.id}; retry may duplicate`); } + throw new Error("relay acceptance receipt write failed; retry delivery"); + } return delivered; } finally { mailboxLock?.release(); @@ -605,12 +671,6 @@ async function acceptRelayedMail( onAccepted?: () => void, ): Promise { const delivered = deliverRelayedToLocal(branchId, body); - // Fire the caller's accepted-delivery hook only when this call published a - // new inbox record. `delivered` is false for a resend whose record already - // exists, and for an inbox write failure that deliverRelayedToLocal handled - // by dead-lettering the message; both are still ACKed below. A same-id - // conflict throws before this point, as does a sync failure or a failed - // dead-letter, so none of those is announced or ACKed. if (delivered) { const reportError = () => { console.error(`[relay] onAccepted failed for message ${body.id} to ${body.to}`); @@ -636,6 +696,7 @@ function logRefusedDelivery(branchId: string, error: ZodError): void { export function startRelay(agentId: string): () => void { assertAgent(agentId); + const stopPrune = startReceiptPrune(); let remoteCleanup: (() => Promise) | null = null; connectRemoteBranches(transportRegistry, (branchId, msg) => { @@ -660,6 +721,7 @@ export function startRelay(agentId: string): () => void { const stop = () => { clearInterval(timer); + stopPrune(); remoteCleanup?.().catch(() => {}); }; return stop; @@ -841,6 +903,7 @@ export async function syncRemoteBranch(branchId: string): Promise<{ received: nu const hostKp = await loadHostIdentity(); const transport = transportType === "ws" ? new WsNoiseTransport(hostKp) : new NoiseIkTransport(hostKp); const channel = await transport.connect({ host, port, branchId, hostPublicKey: branch.encryptionKey }); + const stopPrune = startReceiptPrune(branchId); let received = 0; try { @@ -862,13 +925,14 @@ export async function syncRemoteBranch(branchId: string): Promise<{ received: nu void acceptRelayedMail(channel, branchId, msg, parsed.data).then((delivered) => { if (delivered) received++; }).catch((error: unknown) => { - console.error(`[relay] acceptance failed for message ${parsed.data.id} to ${parsed.data.to}: ${String(error)}`); + console.error(`[relay] acceptance failed for message ${parsed.data.id} to ${parsed.data.to}: ${refusalText(error, parsed.data.content)}`); }); }; channel.onMessage(handler); }); } finally { + stopPrune(); await channel.close().catch(() => {}); } @@ -910,6 +974,7 @@ export async function connectAndKeepAlive( const branch = lookupBranch(branchId); if (!branch?.encryptionKey) throw new Error(`Branch '${branchId}' missing encryption key`); const hostKp = await loadHostIdentity(); + const stopPrune = startReceiptPrune(branchId); const loop = async () => { let backoff = RECONNECT_BASE_MS; @@ -978,7 +1043,7 @@ export async function connectAndKeepAlive( const parsed = MailDeliverBodySchema.safeParse(msg.body); if (parsed.success) { void acceptRelayedMail(channel, branchId, msg, parsed.data, () => opts.onAccepted?.(msg)).catch((error: unknown) => { - console.error(`[relay] acceptance failed for message ${parsed.data.id} to ${parsed.data.to}: ${String(error)}`); + console.error(`[relay] acceptance failed for message ${parsed.data.id} to ${parsed.data.to}: ${refusalText(error, parsed.data.content)}`); }); } else { logRefusedDelivery(branchId, parsed.error); @@ -1012,6 +1077,7 @@ export async function connectAndKeepAlive( return async () => { stopped = true; + stopPrune(); try { await currentChannel?.close(); } catch {} clearHostState(branchId); }; diff --git a/packages/cli/test/relay-accept-invariants.test.ts b/packages/cli/test/relay-accept-invariants.test.ts new file mode 100644 index 00000000..a4b581e8 --- /dev/null +++ b/packages/cli/test/relay-accept-invariants.test.ts @@ -0,0 +1,163 @@ +import { afterEach, beforeEach, expect, mock, spyOn, test } from "bun:test"; +import * as fs from "node:fs"; +import { randomUUID } from "node:crypto"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { ackMessageAtPath, getInbox } from "../src/utils/mail.js"; +import { deliverRelayedToLocal, pruneRelayAcceptanceReceipts, relayAcceptanceReceiptPath, startRelay } from "../src/utils/relay.js"; + +let root: string; +let saved: Record; + +beforeEach(() => { + root = fs.mkdtempSync(join(tmpdir(), "relay-accept-invariants-")); + saved = Object.fromEntries(["HOME", "TPS_MAIL_DIR", "TPS_RELAY_ACCEPT_PRUNE_INTERVAL_MS"].map((key) => [key, process.env[key]])); + process.env.HOME = root; + process.env.TPS_MAIL_DIR = join(root, "mail"); +}); + +afterEach(() => { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + fs.rmSync(root, { recursive: true, force: true }); +}); + +afterEach(() => { mock.restore(); }); + +function body() { + return { id: randomUUID(), from: "remote", to: "local", content: "private-payload-text", timestamp: new Date().toISOString() }; +} + +function records(): string[] { + const inbox = getInbox("local"); + return fs.readdirSync(inbox.fresh).filter((file) => file.endsWith(".json")); +} + +test("receipt write failure removes the new record before retry, ACK, and resend", () => { + const item = body(); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const errors = spyOn(console, "error").mockImplementation(() => {}); + const write = fs.writeFileSync; + const fault = spyOn(fs, "writeFileSync").mockImplementation((path, data, options) => { + if (String(path) === `${marker}.tmp`) throw new Error(`receipt failure ${item.content}`); + return write(path, data, options); + }); + let refusal = ""; + try { + try { deliverRelayedToLocal("remote", item); } + catch (error) { refusal = String(error); } + } + finally { fault.mockRestore(); } + expect(refusal).toContain("relay acceptance receipt write failed; retry delivery"); + expect(refusal).not.toContain(item.content); + expect(records()).toEqual([]); + expect(fs.existsSync(marker)).toBe(false); + expect(deliverRelayedToLocal("remote", item)).toBe(true); + const [file] = records(); + ackMessageAtPath(join(getInbox("local").fresh, file!)); + expect(deliverRelayedToLocal("remote", item)).toBe(false); + expect(records()).toEqual([]); + expect(fs.statSync(marker).mode & 0o777).toBe(0o600); + expect(errors.mock.calls.flat().join("\n")).not.toContain(item.content); + errors.mockRestore(); +}); + +test("record publication failure leaves no receipt and resend publishes once", () => { + const item = body(); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const rename = fs.renameSync; + const fault = spyOn(fs, "renameSync").mockImplementation((from, to) => { + if (String(to).startsWith(getInbox("local").fresh)) throw new Error("injected record publication failure"); + return rename(from, to); + }); + try { expect(() => deliverRelayedToLocal("remote", item)).toThrow(); } + finally { fault.mockRestore(); } + expect(fs.existsSync(marker)).toBe(false); + expect(records()).toEqual([]); + expect(deliverRelayedToLocal("remote", item)).toBe(true); + expect(deliverRelayedToLocal("remote", item)).toBe(false); + expect(records()).toHaveLength(1); +}); + +test("inbox write failures remain unacked and retryable", () => { + const item = body(); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const tmp = getInbox("local").tmp; + const write = fs.writeFileSync; + const fault = spyOn(fs, "writeFileSync").mockImplementation((path, data, options) => { + if (String(path).startsWith(tmp)) throw new Error("Inbox full: injected write failure"); + return write(path, data, options); + }); + try { expect(() => deliverRelayedToLocal("remote", item)).toThrow("relay inbox write failed; retry delivery"); } + finally { fault.mockRestore(); } + expect(fs.existsSync(marker)).toBe(false); + expect(records()).toEqual([]); + expect(deliverRelayedToLocal("remote", item)).toBe(true); + expect(records()).toHaveLength(1); +}); + +test("a receipt rollback sync failure preserves the record and warns of a possible duplicate", () => { + const item = body(); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const open = fs.openSync; + const sync = fs.fsyncSync; + const paths = new Map(); + const opened = spyOn(fs, "openSync").mockImplementation((path, flags, mode) => { + const fd = open(path, flags, mode); + paths.set(fd, String(path)); + return fd; + }); + const fault = spyOn(fs, "fsyncSync").mockImplementation((fd) => { + if (paths.get(fd) === dirname(marker)) throw new Error("injected receipt directory sync failure"); + return sync(fd); + }); + let refusal = ""; + try { + try { deliverRelayedToLocal("remote", item); } + catch (error) { refusal = String(error); } + } finally { + fault.mockRestore(); + opened.mockRestore(); + } + expect(refusal).toContain("retry may duplicate"); + expect(refusal).not.toContain(item.content); + expect(fs.existsSync(marker)).toBe(false); + expect(records()).toHaveLength(1); +}); + +test("old day buckets with more than one former pass expire together", () => { + const accepted = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"); + for (const day of ["2020-01-01", "2020-01-02"]) { + const bucket = join(accepted, day); + fs.mkdirSync(bucket, { recursive: true }); + for (let i = 0; i < 2050; i++) fs.writeFileSync(join(bucket, `receipt-${i}`), "x"); + } + expect(pruneRelayAcceptanceReceipts(accepted, Date.now(), 1000)).toBe(2); + expect(fs.readdirSync(accepted)).toEqual([]); +}); + +test("relay timer retries a failed prune without a new delivery", async () => { + const accepted = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"); + const old = join(accepted, "2020-01-01"); + fs.mkdirSync(old, { recursive: true }); + fs.writeFileSync(join(old, "receipt"), "x"); + process.env.TPS_RELAY_ACCEPT_PRUNE_INTERVAL_MS = "10"; + const remove = fs.rmSync; + let calls = 0; + const fault = spyOn(fs, "rmSync").mockImplementation((path, options) => { + if (String(path) === old && calls++ === 0) throw new Error("injected prune failure"); + return remove(path, options); + }); + const stop = startRelay("local"); + try { + const deadline = Date.now() + 1000; + while (fs.existsSync(old) && Date.now() < deadline) await Bun.sleep(10); + expect(fs.existsSync(old)).toBe(false); + expect(calls).toBeGreaterThanOrEqual(2); + } finally { + stop(); + fault.mockRestore(); + } +}); diff --git a/packages/cli/test/relay-accept-single-lock.test.ts b/packages/cli/test/relay-accept-single-lock.test.ts index 859ef58c..a8691b12 100644 --- a/packages/cli/test/relay-accept-single-lock.test.ts +++ b/packages/cli/test/relay-accept-single-lock.test.ts @@ -4,11 +4,11 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; -import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { getInbox } from "../src/utils/mail.js"; -import { RelayAcceptLockTimeoutError, deliverRelayedToLocal } from "../src/utils/relay.js"; +import { ackMessageAtPath, getInbox } from "../src/utils/mail.js"; +import { RelayAcceptLockTimeoutError, deliverRelayedToLocal, relayAcceptanceReceiptPath } from "../src/utils/relay.js"; const BRANCH = "remote"; const RECIPIENT = "local"; @@ -163,6 +163,27 @@ async function waitForFile(path: string): Promise { } describe("relay acceptance (cli#561)", () => { + test("a second recipient waits for the same delivery id before any publication", async () => { + const id = randomUUID(); + const pause = join(root, "cross-recipient-pause"); + const started = join(root, "cross-recipient-started"); + const first = deliverChild("same-", [id], { RELAY_CHILD_PAUSE: pause }); + let second: Promise | undefined; + try { + await waitForFile(pause + ".ready"); + second = deliverChild("same-", [id], { RELAY_CHILD_TO: "other", RELAY_CHILD_STARTED: started }); + await waitForFile(started); + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(jsonFiles(getInbox("other").fresh)).toEqual([]); + expect(existsSync(relayAcceptanceReceiptPath(BRANCH, id))).toBe(false); + } finally { writeFileSync(pause + ".release", ""); } + const [a, b] = await Promise.all([first, second]); + expect(a).toEqual({ delivered: 1, duplicate: 0, refused: 0 }); + expect(b).toEqual({ delivered: 0, duplicate: 0, refused: 1 }); + expect(jsonFiles(getInbox(RECIPIENT).fresh)).toHaveLength(1); + expect(jsonFiles(getInbox("other").fresh)).toEqual([]); + }, 60_000); + test.each(["new", "dlq"])("routing change before %s publication", async (destination) => { const id = randomUUID(); const hostRoot = getInbox(RECIPIENT).root; @@ -182,7 +203,7 @@ describe("relay acceptance (cli#561)", () => { await waitForFile(started); await new Promise((resolve) => setTimeout(resolve, 100)); expect(jsonFiles(join(branchRoot, "new"))).toEqual([]); - expect(existsSync(join(mail, ".relay-accepted", "by-branch", BRANCH, id))).toBe(false); + expect(existsSync(relayAcceptanceReceiptPath(BRANCH, id))).toBe(false); } finally { writeFileSync(pause + ".release", ""); results = await Promise.all([first, second]); @@ -221,6 +242,42 @@ describe("relay acceptance (cli#561)", () => { for (const id of ids) expect(byId.get(id)).toHaveLength(1); }, 60_000); + test("two recipients resending the same id around local ACK keep the original receipt", async () => { + const id = randomUUID(); + const body = { id, from: FROM, to: RECIPIENT, content: `same-${id}`, timestamp: TIMESTAMP }; + expect(deliverRelayedToLocal(BRANCH, body)).toBe(true); + const marker = relayAcceptanceReceiptPath(BRANCH, id); + const original = readFileSync(marker, "utf8"); + const inode = statSync(marker).ino; + const holder = await holdLock(join(".relay-accept-locks", BRANCH, id)); + const local = deliverChild("same-", [id]); + const other = deliverChild("same-", [id], { RELAY_CHILD_TO: "other" }); + try { + const [file] = jsonFiles(getInbox(RECIPIENT).fresh); + ackMessageAtPath(join(getInbox(RECIPIENT).fresh, file!)); + } finally { await holder.release(); } + const [a, b] = await Promise.all([local, other]); + expect(a).toEqual({ delivered: 0, duplicate: 1, refused: 0 }); + expect(b).toEqual({ delivered: 0, duplicate: 0, refused: 1 }); + expect(readFileSync(marker, "utf8")).toBe(original); + expect(statSync(marker).ino).toBe(inode); + expect(jsonFiles(getInbox(RECIPIENT).fresh)).toEqual([]); + expect(jsonFiles(getInbox("other").fresh)).toEqual([]); + }, 60_000); + + test("the delivery id lock blocks every recipient for that id", async () => { + const id = randomUUID(); + const holder = await holdLock(join(".relay-accept-locks", BRANCH, id)); + try { + process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS = "75"; + for (const to of [RECIPIENT, "other"]) { + expect(() => deliverRelayedToLocal(BRANCH, { id, from: FROM, to, content: "held", timestamp: TIMESTAMP })).toThrow(RelayAcceptLockTimeoutError); + expect(jsonFiles(getInbox(to).fresh)).toEqual([]); + } + expect(existsSync(relayAcceptanceReceiptPath(BRANCH, id))).toBe(false); + } finally { await holder.release(); } + }, 60_000); + test("a recipient lock timeout refuses by name, publishing no mail record or acceptance marker", async () => { const holder = await holdLock(); try { diff --git a/packages/cli/test/relay-delivery-loss.test.ts b/packages/cli/test/relay-delivery-loss.test.ts index 6a77bbbf..5647379d 100644 --- a/packages/cli/test/relay-delivery-loss.test.ts +++ b/packages/cli/test/relay-delivery-loss.test.ts @@ -7,7 +7,7 @@ import { tmpdir } from "node:os"; import { gcMessages, getInbox, MAX_INBOX_MESSAGES, sendMessage, ackMessageAtPath } from "../src/utils/mail.js"; import { runBranch, writeBranchConf } from "../src/commands/branch.js"; import { runMail } from "../src/commands/mail.js"; -import { syncRemoteBranch, connectAndKeepAlive, deliverRelayedToLocal } from "../src/utils/relay.js"; +import { syncRemoteBranch, connectAndKeepAlive, deliverRelayedToLocal, relayAcceptanceReceiptPath, pruneRelayAcceptanceReceipts } from "../src/utils/relay.js"; import * as ws from "../src/utils/ws-noise-transport.js"; import { generateKeyPair, initHostIdentity, registerBranch, saveKeyPair } from "../src/utils/identity.js"; import { drainOutbox, OUTBOX_RESEND_BASE_MS, queueOutboxMessage } from "../src/utils/outbox.js"; @@ -195,7 +195,7 @@ for (const entry of ["sync", "connect"] as const) { const logs = errors.mock.calls.flat().join("\n"); expect(logs).toContain(body.id); expect(logs).toContain("local"); - expect(logs).toContain(fault === "crash-before-publish" ? "simulated crash" : `injected ${fault}`); + expect(logs).toContain("relay dead-letter failed; retry delivery"); await emit(); expect(acks).toEqual([]); const later = Date.now() + OUTBOX_RESEND_BASE_MS; @@ -207,7 +207,7 @@ for (const entry of ["sync", "connect"] as const) { }); } - test("a one-shot local write failure is dead-lettered retryable and the next mail check delivers it", async () => { + test("a local write failure leaves the source unacked and its resend delivers once", async () => { const env = buildSignedEnvelope("remote", "local", "write fault", SEEDS); queue(JSON.stringify(env)); const inbox = getInbox("local"); @@ -224,11 +224,16 @@ for (const entry of ["sync", "connect"] as const) { }); try { await emit(); } finally { injected.mockRestore(); } expect(failed).toBe(true); - expect(acks.length).toBe(1); - expect(drainOutbox(false)).toEqual([]); - expect(jsonFiles(inbox.dlq).length).toBe(1); - for (const file of jsonFiles(inbox.dlq)) expect(fs.readFileSync(join(inbox.dlq, `${file}.reason`), "utf8")).toContain("class: storage-unavailable"); + expect(acks).toEqual([]); + expect(drainOutbox(false)).toHaveLength(1); + expect(jsonFiles(inbox.dlq)).toEqual([]); expect(jsonFiles(inbox.cur)).toEqual([]); + const later = Date.now() + OUTBOX_RESEND_BASE_MS; + const clock = spyOn(Date, "now").mockReturnValue(later); + try { await emit(); } finally { clock.mockRestore(); } + expect(acks).toHaveLength(1); + expect(drainOutbox(false)).toEqual([]); + expect(jsonFiles(inbox.fresh)).toHaveLength(1); const output = spyOn(console, "log").mockImplementation(() => {}); await runMail({ action: "check", agent: "local", json: true }); const delivered = JSON.parse(String(output.mock.calls.at(-1)![0])); @@ -368,7 +373,7 @@ for (const entry of ["sync", "connect"] as const) { const [file] = jsonFiles(dir).filter((name) => JSON.parse(fs.readFileSync(join(dir, name), "utf8")).relayDelivery?.id === body.id); const source = join(dir, file); const before = fs.readFileSync(source, "utf8"); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); const beforeMarker = fs.readFileSync(marker, "utf8"); const errors = spyOn(console, "error").mockImplementation(() => {}); await start(); @@ -415,7 +420,7 @@ for (const entry of ["sync", "connect"] as const) { if (destination === "cur") { record.read = true; record.ackedAt = body.timestamp; } fs.writeFileSync(join(initialDir, file), JSON.stringify(record)); if (destination === "cur") fs.renameSync(join(initialDir, file), join(inbox.cur, file)); - if (prior === "no-marker") fs.rmSync(join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id)); + if (prior === "no-marker") fs.rmSync(relayAcceptanceReceiptPath("remote", body.id)); } fs.writeFileSync(join(inbox.dlq, "expired-canary.json"), JSON.stringify({ id: "expired", from: "remote", to: "local", body: "expired", timestamp: body.timestamp, read: false })); await start(); @@ -450,38 +455,29 @@ for (const entry of ["sync", "connect"] as const) { } } - for (const consumed of [false, true]) { - test(`marker failure then redelivery keeps one record in ${consumed ? "cur" : "new"}`, async () => { + test("receipt failure removes the new record before redelivery", async () => { const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "accept once", SEEDS))); const inbox = getInbox("local"); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); fs.mkdirSync(`${marker}.tmp`, { recursive: true }); spyOn(console, "error").mockImplementation(() => {}); await start(); const msg: TpsMessage = { type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }; await deliverDirect(msg); expect(acks).toEqual([]); - expect(jsonFiles(inbox.fresh).length).toBe(1); + expect(jsonFiles(inbox.fresh)).toEqual([]); expect(fs.existsSync(marker)).toBe(false); expect(drainOutbox(false).map((m) => m.id)).toEqual([body.id]); - if (consumed) { - const output = spyOn(console, "log").mockImplementation(() => {}); - await runMail({ action: "check", agent: "local", json: true }); - expect(JSON.parse(String(output.mock.calls.at(-1)![0])).length).toBe(1); - expect(jsonFiles(inbox.fresh)).toEqual([]); - expect(jsonFiles(inbox.cur).length).toBe(1); - } fs.rmSync(`${marker}.tmp`, { recursive: true, force: true }); await deliverDirect(msg); expect(acks.map((ack) => (ack.body as { id: string }).id)).toEqual([body.id]); expect(jsonFiles(inbox.fresh).length + jsonFiles(inbox.cur).length).toBe(1); expect(drainOutbox(false)).toEqual([]); }); - } test("ACK failure then redelivery keeps one inbox record", async () => { const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "ACK retry", SEEDS))); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); spyOn(console, "error").mockImplementation(() => {}); await start(); const msg: TpsMessage = { type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }; @@ -539,7 +535,7 @@ for (const entry of ["sync", "connect"] as const) { await start(); await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); await ackOnlyRecord(); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS = "1000"; const old = new Date(Date.now() - 5000); fs.utimesSync(marker, old, old); @@ -552,24 +548,25 @@ for (const entry of ["sync", "connect"] as const) { expect(jsonFiles(getInbox("local").fresh)).toHaveLength(1); }); - test("acceptance receipts past the prune bound are removed when a new one is written", async () => { + test("a whole old receipt bucket is removed when a new receipt is written", async () => { const first = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "first accepted", SEEDS))); await start(); await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body: first }); const accepted = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"); - const firstMarker = join(accepted, first.id); + const firstMarker = relayAcceptanceReceiptPath("remote", first.id); expect(fs.existsSync(firstMarker)).toBe(true); process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS = "1000"; - const old = new Date(Date.now() - 5000); - fs.utimesSync(firstMarker, old, old); + const oldBucket = join(accepted, "2020-01-01"); + fs.mkdirSync(oldBucket); + fs.renameSync(firstMarker, join(oldBucket, first.id)); const second = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "second accepted", SEEDS))); try { await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body: second }); } finally { delete process.env.TPS_RELAY_ACCEPT_RECEIPT_TTL_MS; } - expect(fs.existsSync(firstMarker)).toBe(false); - expect(fs.existsSync(join(accepted, second.id))).toBe(true); + expect(fs.existsSync(oldBucket)).toBe(false); + expect(fs.existsSync(relayAcceptanceReceiptPath("remote", second.id))).toBe(true); expect(jsonFiles(getInbox("local").fresh)).toHaveLength(2); }); @@ -579,7 +576,7 @@ for (const entry of ["sync", "connect"] as const) { await start(); await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }); await ackOnlyRecord(); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); const read = fs.readFileSync; const fault = spyOn(fs, "readFileSync").mockImplementation((path, options) => { if (String(path) === marker) throw Object.assign(new Error("injected marker read denied"), { code: "EACCES" }); @@ -600,7 +597,7 @@ for (const entry of ["sync", "connect"] as const) { const envelope = buildSignedEnvelope("remote", "local", state, SEEDS); const body = queue(JSON.stringify(envelope)); const inbox = getInbox("local"); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); const client = new MailClient(process.env.TPS_MAIL_DIR!, undefined, "local", { async getAgent(id) { const seed = SEEDS[id as keyof typeof SEEDS]; @@ -709,7 +706,7 @@ for (const entry of ["sync", "connect"] as const) { expect(acks).toEqual([]); expect(fs.readFileSync(source, "utf8")).toBe(before); expect(jsonFiles(inbox.fresh).length + jsonFiles(inbox.cur).length + jsonFiles(inbox.dlq).length).toBe(1); - expect(fs.existsSync(join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id))).toBe(false); + expect(fs.existsSync(relayAcceptanceReceiptPath("remote", body.id))).toBe(false); expect(errors.mock.calls.flat().join("\n")).toContain(`relayed record read failed: ${source}`); expect(drainOutbox(false).map((item) => item.id)).toContain(body.id); if (dir === "dlq") fs.writeFileSync(`${source}.reason`, "class: inbox-full\n"); @@ -723,7 +720,7 @@ for (const entry of ["sync", "connect"] as const) { test("inbox and DLQ write failures leave no marker or ACK, then retry writes one record", async () => { const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "write retry", SEEDS))); const inbox = getInbox("local"); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); spyOn(console, "error").mockImplementation(() => {}); await start(); const msg: TpsMessage = { type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }; @@ -755,9 +752,14 @@ for (const entry of ["sync", "connect"] as const) { const trace: number[] = []; const capture = spyOn(fs, "fsyncSync").mockImplementation((fd) => { trace.push(fd); return sync(fd); }); const initial = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "sync trace", SEEDS))); - try { await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body: initial }); } + try { + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body: initial }); + trace.length = 0; + const warmed = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "warmed sync trace", SEEDS))); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body: warmed }); + } finally { capture.mockRestore(); } - expect(acks.length).toBe(1); + expect(acks.length).toBe(2); expect(trace.length).toBeGreaterThanOrEqual(destination === "inbox" ? 4 : 5); for (let step = 1; step <= trace.length; step++) { const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", `sync fault ${step}`, SEEDS))); @@ -770,8 +772,10 @@ for (const entry of ["sync", "connect"] as const) { return sync(fd); }); try { await deliverDirect(msg); } finally { fault.mockRestore(); } - expect(calls).toBe(step); + expect(calls).toBeGreaterThanOrEqual(step); expect(acks.length).toBe(beforeAcks); + expect(fs.existsSync(relayAcceptanceReceiptPath("remote", body.id))).toBe(false); + expect(jsonFiles(inbox.fresh).length + jsonFiles(inbox.dlq).length).toBe(beforeRecords); expect(drainOutbox(false).map((m) => m.id)).toContain(body.id); await deliverDirect(msg); expect(acks.length).toBe(beforeAcks + 1); @@ -781,18 +785,18 @@ for (const entry of ["sync", "connect"] as const) { }); } - test("DLQ record before marker failure is reused on redelivery", async () => { + test("DLQ record is removed after receipt failure and written on redelivery", async () => { fillInbox(); const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "DLQ retry", SEEDS))); const inbox = getInbox("local"); - const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + const marker = relayAcceptanceReceiptPath("remote", body.id); fs.mkdirSync(`${marker}.tmp`, { recursive: true }); spyOn(console, "error").mockImplementation(() => {}); await start(); const msg: TpsMessage = { type: MSG_MAIL_DELIVER, seq: 1, ts: new Date().toISOString(), body }; await deliverDirect(msg); expect(acks).toEqual([]); - expect(jsonFiles(inbox.dlq).length).toBe(1); + expect(jsonFiles(inbox.dlq)).toEqual([]); fs.rmSync(`${marker}.tmp`, { recursive: true }); await deliverDirect(msg); expect(acks.length).toBe(1); diff --git a/packages/cli/test/relay-review.test.ts b/packages/cli/test/relay-review.test.ts index a16ee6ee..ee949c93 100644 --- a/packages/cli/test/relay-review.test.ts +++ b/packages/cli/test/relay-review.test.ts @@ -4,7 +4,7 @@ import { randomUUID } from "node:crypto"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { checkMessages, getInbox, listMessages, promote, sendMessage } from "../src/utils/mail.js"; -import { deliverRelayedToLocal } from "../src/utils/relay.js"; +import { deliverRelayedToLocal, relayAcceptanceReceiptPath } from "../src/utils/relay.js"; import { runMail } from "../src/commands/mail.js"; import { buildSignedEnvelope, pubkeyFromSeed, writeKeyFile } from "./helpers/stub-flair.js"; @@ -76,7 +76,7 @@ describe("relay review regressions", () => { const [file] = fs.readdirSync(inbox.fresh); expect(JSON.parse(fs.readFileSync(join(inbox.fresh, file!), "utf8")).body).toBe(message.content); const directories = synced.filter((path) => !path.endsWith(".json") && !path.endsWith(message.id) && !path.endsWith(".tmp")); - expect(new Set(directories)).toEqual(new Set([inbox.tmp, inbox.fresh, accepted])); + for (const path of [inbox.tmp, inbox.fresh, join(accepted, new Date().toISOString().slice(0, 10))]) expect(directories).toContain(path); }); test("relay publication syncs parents of newly created directories", () => { @@ -86,10 +86,7 @@ describe("relay review regressions", () => { const inbox = getInbox("local"); const acceptedRoot = join(mail, ".relay-accepted"); const directories = synced.filter((path) => !path.endsWith(".json") && !path.endsWith(message.id) && !path.endsWith(".tmp")); - expect(new Set(directories)).toEqual(new Set([ - root, mail, inbox.root, inbox.tmp, inbox.fresh, - acceptedRoot, join(acceptedRoot, "by-branch"), join(acceptedRoot, "by-branch", "remote"), - ])); + for (const path of [root, mail, inbox.root, inbox.tmp, inbox.fresh, join(acceptedRoot, "by-branch", "remote", new Date().toISOString().slice(0, 10))]) expect(directories).toContain(path); }); test("a retry syncs directory entries left pending by a creation sync failure", () => { @@ -97,12 +94,12 @@ describe("relay review regressions", () => { const acceptedRoot = join(mail, ".relay-accepted"); const synced = traceSync(acceptedRoot, true); const message = body(); - expect(() => deliverRelayedToLocal("remote", message)).toThrow("mail fsync failed"); + expect(() => deliverRelayedToLocal("remote", message)).toThrow("relay acceptance receipt write failed; retry delivery"); synced.length = 0; - expect(deliverRelayedToLocal("remote", message)).toBe(false); + expect(deliverRelayedToLocal("remote", message)).toBe(true); expect(synced).toContain(acceptedRoot); expect(synced).toContain(mail); - expect(JSON.parse(fs.readFileSync(join(acceptedRoot, "by-branch", "remote", message.id), "utf8"))).toEqual({ + expect(JSON.parse(fs.readFileSync(relayAcceptanceReceiptPath("remote", message.id), "utf8"))).toEqual({ from: message.from, to: message.to, body: message.content, @@ -119,7 +116,8 @@ describe("relay review regressions", () => { fs.writeFileSync(corrupt, raw); const accepted = join(mail, ".relay-accepted", "by-branch", "remote"); fs.mkdirSync(accepted, { recursive: true }); - fs.writeFileSync(join(accepted, message.id), ""); + fs.mkdirSync(join(accepted, new Date().toISOString().slice(0, 10)), { recursive: true }); + fs.writeFileSync(relayAcceptanceReceiptPath("remote", message.id), ""); const errors = spyOn(console, "error").mockImplementation(() => {}); expect(() => deliverRelayedToLocal("remote", message)).toThrow(`relayed delivery conflict for branch remote message ${message.id}`); const quarantine = join(inbox.root, "quarantine"); @@ -130,7 +128,7 @@ describe("relay review regressions", () => { expect(fs.readFileSync(`${path}.reason`, "utf8")).toContain(corrupt); expect(errors.mock.calls.flat().join("\n")).toContain(corrupt); expect(fs.readdirSync(join(inbox.root, dir))).not.toContain("truncated.json"); - fs.rmSync(join(accepted, message.id)); + fs.rmSync(relayAcceptanceReceiptPath("remote", message.id)); expect(deliverRelayedToLocal("remote", message)).toBe(true); const records = fs.readdirSync(inbox.fresh).filter((file) => file.endsWith(".json")); expect(records).toHaveLength(1); @@ -156,7 +154,7 @@ describe("relay review regressions", () => { expect(errors.mock.calls.flat().join("\n")).toContain(source); const records = fs.readdirSync(inbox.fresh).filter((file) => file !== "unreadable.json" && file.endsWith(".json")); expect(records).toHaveLength(0); - expect(fs.existsSync(join(mail, ".relay-accepted", "by-branch", "remote", message.id))).toBe(false); + expect(fs.existsSync(relayAcceptanceReceiptPath("remote", message.id))).toBe(false); }); test("mail list and CLI list use receipt time with a timestamp fallback", async () => { From 65f15f1effbfd858c5c7f3fbb0cba429f58892e7 Mon Sep 17 00:00:00 2001 From: flint Date: Fri, 9 Oct 2026 01:04:00 -0700 Subject: [PATCH 06/10] fix(relay): delivery locks are a fixed set of stripes, so nothing grows per delivery id Co-Authored-By: Claude Opus 5.5 --- .../fixed-573-relay-resend-after-ack.md | 2 +- packages/cli/src/utils/relay.ts | 11 ++++- .../cli/test/office-connect-announce.test.ts | 6 +-- .../cli/test/relay-accept-single-lock.test.ts | 47 ++++++++++++++++--- packages/cli/test/relay-delivery-loss.test.ts | 5 +- 5 files changed, 57 insertions(+), 14 deletions(-) diff --git a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md index 8731b41a..4b3af046 100644 --- a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md +++ b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md @@ -1 +1 @@ -- **Deduplicate relay resends after local ACK while the receipt remains.** Failed acceptance writes roll back new records; old receipt buckets are pruned on relay start and a timer. +- **Deduplicate relay resends after local ACK while the receipt remains.** A fixed set of lock stripes serializes acceptance; failed acceptance writes roll back new records; old receipt buckets are pruned on relay start and a timer. diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index 361d4754..938497b9 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -1,7 +1,7 @@ import { appendFileSync, existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, statSync, unlinkSync, writeFileSync } from "node:fs"; import { dirname, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; -import { randomUUID } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import { sanitizeIdentifier } from "../schema/sanitizer.js"; import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailInboxFullError, MailSendInputError, relayAcceptRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; import { acquireMailLockSync } from "./mail-lock.js"; @@ -544,6 +544,12 @@ function undoNewRelayRecord(root: string, delivery: { branchId: string; id: stri /** Wait budget for relay acceptance locks. */ const RELAY_ACCEPT_LOCK_TIMEOUT_MS = 2000; +export const RELAY_ACCEPT_LOCK_STRIPES = 64; + +export function relayAcceptanceLockRoot(branchId: string, id: string): string { + const stripe = createHash("sha256").update(branchId).update("\0").update(id).digest()[0]! % RELAY_ACCEPT_LOCK_STRIPES; + return join(getMailDir(), ".relay-accept-locks", String(stripe)); +} function relayAcceptLockTimeoutMs(): number { const raw = Number(process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS); @@ -576,11 +582,12 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): if (!lock) throw new RelayAcceptLockTimeoutError(body.to); return lock; }; - const acceptanceRoot = join(getMailDir(), ".relay-accept-locks", branchId, body.id); + const acceptanceRoot = relayAcceptanceLockRoot(branchId, body.id); mkdirMailDirectory(acceptanceRoot); const acceptanceLock = acquireLock(acceptanceRoot); let mailboxLock: ReturnType | undefined; try { + // Hold the stripe before each mailbox lock; release mailbox locks before the stripe. const recipientRoot = relayAcceptRoot(body.to); mkdirMailDirectory(recipientRoot); if (recipientRoot !== acceptanceRoot) mailboxLock = acquireLock(recipientRoot); diff --git a/packages/cli/test/office-connect-announce.test.ts b/packages/cli/test/office-connect-announce.test.ts index 65cfbf24..2271a8b5 100644 --- a/packages/cli/test/office-connect-announce.test.ts +++ b/packages/cli/test/office-connect-announce.test.ts @@ -445,7 +445,7 @@ describe("office connect announcement follows local acceptance", () => { expect(announced).toEqual([1]); }); - test("a delivery whose inbox write fails is dead-lettered and is not announced", async () => { + test("an inbox write failure is unacked and unannounced", async () => { const inbox = getInbox("local"); const write = fs.writeFileSync; let failed = false; @@ -463,9 +463,9 @@ describe("office connect announcement follows local acceptance", () => { fault.mockRestore(); } expect(failed).toBe(true); - expect(acks.length).toBe(1); + expect(acks.length).toBe(0); expect(jsonFiles(inbox.fresh)).toEqual([]); - expect(jsonFiles(inbox.dlq).length).toBe(1); + expect(jsonFiles(inbox.dlq)).toEqual([]); expect(announced).toEqual([]); }); }); diff --git a/packages/cli/test/relay-accept-single-lock.test.ts b/packages/cli/test/relay-accept-single-lock.test.ts index a8691b12..a21c829b 100644 --- a/packages/cli/test/relay-accept-single-lock.test.ts +++ b/packages/cli/test/relay-accept-single-lock.test.ts @@ -8,7 +8,7 @@ import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, import { tmpdir } from "node:os"; import { join } from "node:path"; import { ackMessageAtPath, getInbox } from "../src/utils/mail.js"; -import { RelayAcceptLockTimeoutError, deliverRelayedToLocal, relayAcceptanceReceiptPath } from "../src/utils/relay.js"; +import { RELAY_ACCEPT_LOCK_STRIPES, RelayAcceptLockTimeoutError, deliverRelayedToLocal, relayAcceptanceLockRoot, relayAcceptanceReceiptPath } from "../src/utils/relay.js"; const BRANCH = "remote"; const RECIPIENT = "local"; @@ -127,9 +127,9 @@ function deliverChild(prefix: string, ids: string[], env: Record } /** Spawn a real process that holds a mailbox lock until released. */ -function holdLock(agent = RECIPIENT): Promise<{ release: () => Promise }> { +function holdLock(lockRoot = join(mail, RECIPIENT)): Promise<{ release: () => Promise }> { const child = spawn("bun", [childScript], { - env: { ...process.env, RELAY_CHILD_MODE: "hold", RELAY_CHILD_ROOT: join(mail, agent) }, + env: { ...process.env, RELAY_CHILD_MODE: "hold", RELAY_CHILD_ROOT: lockRoot }, stdio: ["pipe", "pipe", "inherit"], }); const exited = new Promise((resolve) => child.once("exit", resolve)); @@ -242,6 +242,39 @@ describe("relay acceptance (cli#561)", () => { for (const id of ids) expect(byId.get(id)).toHaveLength(1); }, 60_000); + test("lock directories stay bounded across distinct deliveries", () => { + for (let i = 0; i < RELAY_ACCEPT_LOCK_STRIPES * 2 + 1; i++) { + const id = randomUUID(); + expect(deliverRelayedToLocal(BRANCH, { id, from: FROM, to: RECIPIENT, content: id, timestamp: TIMESTAMP })).toBe(true); + const [file] = jsonFiles(getInbox(RECIPIENT).fresh); + ackMessageAtPath(join(getInbox(RECIPIENT).fresh, file!)); + } + const lockRoot = join(mail, ".relay-accept-locks"); + const countDirectories = (dir: string): number => readdirSync(dir, { withFileTypes: true }).reduce( + (total, entry) => total + (entry.isDirectory() ? 1 + countDirectories(join(dir, entry.name)) : 0), 0, + ); + expect(countDirectories(lockRoot)).toBeLessThanOrEqual(RELAY_ACCEPT_LOCK_STRIPES); + }, 60_000); + + test("different ids on one stripe each publish once across processes", async () => { + const seen = new Map(); + let pair: [string, string] | undefined; + for (let i = 0; i <= RELAY_ACCEPT_LOCK_STRIPES; i++) { + const id = randomUUID(); + const stripe = relayAcceptanceLockRoot(BRANCH, id); + const prior = seen.get(stripe); + if (prior) { pair = [prior, id]; break; } + seen.set(stripe, id); + } + expect(pair).toBeDefined(); + const [a, b] = await Promise.all([deliverChild("same-", pair!), deliverChild("same-", pair!)]); + expect(a.delivered + b.delivered).toBe(2); + expect(a.duplicate + b.duplicate).toBe(2); + expect(a.refused + b.refused).toBe(0); + const byId = recordsByDeliveryId(); + for (const id of pair!) expect(byId.get(id)).toHaveLength(1); + }, 20_000); + test("two recipients resending the same id around local ACK keep the original receipt", async () => { const id = randomUUID(); const body = { id, from: FROM, to: RECIPIENT, content: `same-${id}`, timestamp: TIMESTAMP }; @@ -249,7 +282,7 @@ describe("relay acceptance (cli#561)", () => { const marker = relayAcceptanceReceiptPath(BRANCH, id); const original = readFileSync(marker, "utf8"); const inode = statSync(marker).ino; - const holder = await holdLock(join(".relay-accept-locks", BRANCH, id)); + const holder = await holdLock(relayAcceptanceLockRoot(BRANCH, id)); const local = deliverChild("same-", [id]); const other = deliverChild("same-", [id], { RELAY_CHILD_TO: "other" }); try { @@ -265,9 +298,9 @@ describe("relay acceptance (cli#561)", () => { expect(jsonFiles(getInbox("other").fresh)).toEqual([]); }, 60_000); - test("the delivery id lock blocks every recipient for that id", async () => { + test("the stripe lock blocks every recipient for that id", async () => { const id = randomUUID(); - const holder = await holdLock(join(".relay-accept-locks", BRANCH, id)); + const holder = await holdLock(relayAcceptanceLockRoot(BRANCH, id)); try { process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS = "75"; for (const to of [RECIPIENT, "other"]) { @@ -292,7 +325,7 @@ describe("relay acceptance (cli#561)", () => { }, 60_000); test("a different mailbox's lock uses the acceptance deadline and refuses by name, publishing no mail record or acceptance marker", async () => { - const holder = await holdLock("other"); + const holder = await holdLock(join(mail, "other")); try { process.env.TPS_RELAY_ACCEPT_LOCK_TIMEOUT_MS = "75"; const body = { id: randomUUID(), from: FROM, to: RECIPIENT, content: "timed out", timestamp: TIMESTAMP }; diff --git a/packages/cli/test/relay-delivery-loss.test.ts b/packages/cli/test/relay-delivery-loss.test.ts index 5647379d..267bed49 100644 --- a/packages/cli/test/relay-delivery-loss.test.ts +++ b/packages/cli/test/relay-delivery-loss.test.ts @@ -7,7 +7,7 @@ import { tmpdir } from "node:os"; import { gcMessages, getInbox, MAX_INBOX_MESSAGES, sendMessage, ackMessageAtPath } from "../src/utils/mail.js"; import { runBranch, writeBranchConf } from "../src/commands/branch.js"; import { runMail } from "../src/commands/mail.js"; -import { syncRemoteBranch, connectAndKeepAlive, deliverRelayedToLocal, relayAcceptanceReceiptPath, pruneRelayAcceptanceReceipts } from "../src/utils/relay.js"; +import { RELAY_ACCEPT_LOCK_STRIPES, syncRemoteBranch, connectAndKeepAlive, deliverRelayedToLocal, relayAcceptanceReceiptPath, pruneRelayAcceptanceReceipts } from "../src/utils/relay.js"; import * as ws from "../src/utils/ws-noise-transport.js"; import { generateKeyPair, initHostIdentity, registerBranch, saveKeyPair } from "../src/utils/identity.js"; import { drainOutbox, OUTBOX_RESEND_BASE_MS, queueOutboxMessage } from "../src/utils/outbox.js"; @@ -748,6 +748,9 @@ for (const entry of ["sync", "connect"] as const) { spyOn(console, "error").mockImplementation(() => {}); await start(); fs.mkdirSync(join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"), { recursive: true }); + for (let stripe = 0; stripe < RELAY_ACCEPT_LOCK_STRIPES; stripe++) { + fs.mkdirSync(join(process.env.TPS_MAIL_DIR!, ".relay-accept-locks", String(stripe)), { recursive: true }); + } const sync = fs.fsyncSync; const trace: number[] = []; const capture = spyOn(fs, "fsyncSync").mockImplementation((fd) => { trace.push(fd); return sync(fd); }); From 0835d1dc0ae5203e96c7112f90edecbfc70724ce Mon Sep 17 00:00:00 2001 From: flint Date: Fri, 9 Oct 2026 03:39:41 -0700 Subject: [PATCH 07/10] fix(relay): an id accepted before receipts existed is refused on resend after ACK Co-Authored-By: Claude Opus 5.5 --- .../fixed-573-relay-resend-after-ack.md | 2 +- packages/cli/src/utils/relay.ts | 46 +++++++++++-------- .../cli/test/relay-accept-invariants.test.ts | 29 +++++++++++- packages/cli/test/relay-delivery-loss.test.ts | 18 ++++++++ 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md index 4b3af046..e710cd1d 100644 --- a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md +++ b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md @@ -1 +1 @@ -- **Deduplicate relay resends after local ACK while the receipt remains.** A fixed set of lock stripes serializes acceptance; failed acceptance writes roll back new records; old receipt buckets are pruned on relay start and a timer. +- **Deduplicate relay resends after local ACK while the receipt remains.** A fixed set of lock stripes serializes acceptance; failed acceptance writes roll back what the attempt wrote, and incomplete rollback refuses delivery with a possible-duplicate warning; the receipt TTL also applies to flat per-branch markers. diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index 938497b9..bfc77a7a 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -409,18 +409,23 @@ export function relayAcceptanceReceiptPath(branchId: string, id: string, now = D return join(getMailDir(), ".relay-accepted", "by-branch", branchId, receiptBucket(now), id); } -/** Remove expired day buckets. */ +/** Remove old day buckets and expired flat per-branch markers. */ export function pruneRelayAcceptanceReceipts(acceptedDir: string, now = Date.now(), ttlMs = relayAcceptReceiptTtlMs()): number { if (!existsSync(acceptedDir)) return 0; let removed = 0; for (const entry of readdirSync(acceptedDir, { withFileTypes: true })) { - if (!entry.isDirectory() || !/^\d{4}-\d{2}-\d{2}$/.test(entry.name)) continue; - const end = Date.parse(`${entry.name}T00:00:00.000Z`) + 24 * 60 * 60 * 1000; - if (!Number.isFinite(end) || end > now - ttlMs) continue; - try { - rmSync(join(acceptedDir, entry.name), { recursive: true }); - removed++; - } catch {} + const path = join(acceptedDir, entry.name); + if (entry.isDirectory() && /^\d{4}-\d{2}-\d{2}$/.test(entry.name)) { + const end = Date.parse(`${entry.name}T00:00:00.000Z`) + 24 * 60 * 60 * 1000; + if (!Number.isFinite(end) || end > now - ttlMs) continue; + try { rmSync(path, { recursive: true }); removed++; } catch {} + } else if (entry.isFile() && MailDeliverBodySchema.shape.id.safeParse(entry.name).success) { + try { + if (now - statSync(path).mtimeMs <= ttlMs) continue; + unlinkSync(path); + removed++; + } catch {} + } } if (removed > 0) syncMailDirectory(acceptedDir); return removed; @@ -484,9 +489,7 @@ function recordAcceptance(acceptedDir: string, marker: string, receipt: RelayAcc } /** - * The acceptance marker for a delivery, or undefined when there is none or the - * one present has aged past the receipt TTL (an expired receipt no longer - * blocks). The marker is keyed by branch+id; its receipt records the payload. + * Find a marker unless its age is known to exceed the receipt TTL. */ function existingAcceptanceMarker(acceptedDir: string, id: string, legacyMarker: string): string | undefined { if (receiptMayExist(acceptedDir)) { @@ -499,16 +502,18 @@ function existingAcceptanceMarker(acceptedDir: string, id: string, legacyMarker: return path; } } + const flatMarker = join(acceptedDir, id); + if (receiptMayExist(flatMarker)) { + try { if (Date.now() - statSync(flatMarker).mtimeMs <= relayAcceptReceiptTtlMs()) return flatMarker; } + catch { return flatMarker; } + } if (!receiptMayExist(legacyMarker)) return undefined; try { if (Date.now() - statSync(legacyMarker).mtimeMs > relayAcceptReceiptTtlMs()) return undefined; } catch {} return legacyMarker; } /** - * Whether a surviving marker's receipt is for the same payload. A marker with - * no readable receipt (an empty pre-receipt marker, or a corrupt one) is a - * conflict: the delivery's identity is unproven, so publishing again could - * deliver a second copy. A read failure propagates and refuses the delivery. + * Whether a readable receipt matches the payload; legacy empty markers cannot be compared. */ function acceptanceReceiptMatches(path: string, expected: RelayAcceptReceipt): boolean { const raw = readFileSync(path, "utf-8"); @@ -595,13 +600,14 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): // The marker path includes the branch: a 64-hex id is deterministic, so two branches can send the same one. const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId); const marker = relayAcceptanceReceiptPath(branchId, body.id); + const flatMarker = join(acceptedDir, body.id); const legacyMarker = join(getMailDir(), ".relay-accepted", body.id); const receipt = acceptReceipt(body); const existingMarker = existingAcceptanceMarker(acceptedDir, body.id, legacyMarker); const delivery = { branchId, id: body.id }; const existingRecord = findRelayedRecord(body.to, delivery, { from: body.from, to: body.to, body: body.content, timestamp: body.timestamp }, body.to, { heldRoot: recipientRoot, heldAcceptanceRoot: acceptanceRoot, acquireLock }); if (existingRecord) { - if (existingMarker && existingMarker !== legacyMarker && !acceptanceReceiptMatches(existingMarker, receipt)) { + if (existingMarker && existingMarker !== legacyMarker && existingMarker !== flatMarker && !acceptanceReceiptMatches(existingMarker, receipt)) { throw new Error(`relayed delivery conflict for branch ${branchId} message ${body.id}`); } if (existingMarker && acceptanceReceiptMatches(existingMarker, receipt)) { @@ -614,15 +620,15 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): return false; } if (existingMarker) { - // No record, but this branch+id was accepted before (a normal mail ACK - // removes the record). A matching payload is a duplicate; anything else — - // a different payload, or a marker with no usable receipt — is refused, - // publishing no second record. + // A marker without a record blocks publication unless its receipt matches. if (acceptanceReceiptMatches(existingMarker, receipt)) { syncMailFile(existingMarker); syncMailDirectory(dirname(existingMarker)); return false; } + if (existingMarker === flatMarker || existingMarker === legacyMarker) { + throw new Error(`relayed delivery conflict for branch ${branchId} message ${body.id}: accepted before receipts existed; payload cannot be compared; sender must not retry`); + } throw new Error(`relayed delivery conflict for branch ${branchId} message ${body.id}`); } diff --git a/packages/cli/test/relay-accept-invariants.test.ts b/packages/cli/test/relay-accept-invariants.test.ts index a4b581e8..332eea94 100644 --- a/packages/cli/test/relay-accept-invariants.test.ts +++ b/packages/cli/test/relay-accept-invariants.test.ts @@ -3,7 +3,7 @@ import * as fs from "node:fs"; import { randomUUID } from "node:crypto"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; -import { ackMessageAtPath, getInbox } from "../src/utils/mail.js"; +import { ackMessageAtPath, getInbox, sendMessage } from "../src/utils/mail.js"; import { deliverRelayedToLocal, pruneRelayAcceptanceReceipts, relayAcceptanceReceiptPath, startRelay } from "../src/utils/relay.js"; let root: string; @@ -138,6 +138,33 @@ test("old day buckets with more than one former pass expire together", () => { expect(fs.readdirSync(accepted)).toEqual([]); }); +test("a flat per-branch marker with a live record remains an in-flight duplicate", () => { + const item = body(); + const original = sendMessage(item.to, item.content, item.from, { branchId: "remote", id: item.id }, item.timestamp); + const flatMarker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", item.id); + fs.mkdirSync(dirname(flatMarker), { recursive: true }); + fs.writeFileSync(flatMarker, ""); + expect(deliverRelayedToLocal("remote", item)).toBe(false); + expect(records()).toHaveLength(1); + expect(fs.existsSync(relayAcceptanceReceiptPath("remote", item.id))).toBe(true); + ackMessageAtPath(original.filePath); + expect(deliverRelayedToLocal("remote", item)).toBe(false); + expect(records()).toEqual([]); +}); + +test("flat per-branch markers expire with receipts", () => { + const accepted = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"); + fs.mkdirSync(accepted, { recursive: true }); + const old = join(accepted, randomUUID()); + const current = join(accepted, randomUUID()); + fs.writeFileSync(old, ""); + fs.writeFileSync(current, ""); + fs.utimesSync(old, new Date(0), new Date(0)); + expect(pruneRelayAcceptanceReceipts(accepted, Date.now(), 1000)).toBe(1); + expect(fs.existsSync(old)).toBe(false); + expect(fs.existsSync(current)).toBe(true); +}); + test("relay timer retries a failed prune without a new delivery", async () => { const accepted = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"); const old = join(accepted, "2020-01-01"); diff --git a/packages/cli/test/relay-delivery-loss.test.ts b/packages/cli/test/relay-delivery-loss.test.ts index 267bed49..4b4a8830 100644 --- a/packages/cli/test/relay-delivery-loss.test.ts +++ b/packages/cli/test/relay-delivery-loss.test.ts @@ -530,6 +530,24 @@ for (const entry of ["sync", "connect"] as const) { expect(errors.mock.calls.flat().join("\n")).toContain("conflict"); }); + test("a flat per-branch marker from before receipts refuses resend after local ACK without ACK", async () => { + const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "legacy acceptance", SEEDS))); + const original = sendMessage("local", body.content, body.from, { branchId: "remote", id: body.id }, body.timestamp); + const marker = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote", body.id); + fs.mkdirSync(join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"), { recursive: true }); + fs.writeFileSync(marker, ""); + const errors = spyOn(console, "error").mockImplementation(() => {}); + await start(); + ackMessageAtPath(original.filePath); + await deliverDirect({ type: MSG_MAIL_DELIVER, seq: 2, ts: new Date().toISOString(), body }); + expect(jsonFiles(getInbox("local").fresh)).toEqual([]); + expect(jsonFiles(getInbox("local").dlq)).toEqual([]); + expect(acks).toEqual([]); + expect(drainOutbox(false).map((item) => item.id)).toContain(body.id); + expect(fs.existsSync(relayAcceptanceReceiptPath("remote", body.id))).toBe(false); + expect(errors.mock.calls.flat().join("\n")).toContain(`relayed delivery conflict for branch remote message ${body.id}: accepted before receipts existed; payload cannot be compared; sender must not retry`); + }); + test("a receipt older than the prune bound no longer blocks a resend", async () => { const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "late", SEEDS))); await start(); From 3b48f09b8469d562cc345d06b9c4436d3c0c8016 Mon Sep 17 00:00:00 2001 From: flint Date: Fri, 9 Oct 2026 06:05:20 -0700 Subject: [PATCH 08/10] fix(relay): a failed acceptance removes its receipt temp and dead-letter sidecar, or refuses as incomplete recovery Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/utils/mail.ts | 34 +++++- packages/cli/src/utils/relay.ts | 10 +- .../cli/test/relay-attempt-artifacts.test.ts | 104 ++++++++++++++++++ packages/cli/test/relay-delivery-loss.test.ts | 2 +- 4 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 packages/cli/test/relay-attempt-artifacts.test.ts diff --git a/packages/cli/src/utils/mail.ts b/packages/cli/src/utils/mail.ts index 2fc2a9b9..40e8ebbd 100644 --- a/packages/cli/src/utils/mail.ts +++ b/packages/cli/src/utils/mail.ts @@ -408,6 +408,21 @@ export function syncMailDirectory(path: string): void { syncMailFile(path); } +/** Remove a file, confirm it is gone, and sync its directory; throws if removal cannot be confirmed. */ +export function removeMailFileConfirmed(path: string): void { + try { unlinkSync(path); } + catch (error) { if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; } + try { lstatSync(path); } + catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === "ENOENT" || code === "ENOTDIR") { syncMailDirectory(dirname(path)); return; } + throw error; + } + throw new Error("file still present after removal"); +} + +export class DeadLetterCleanupError extends Error {} + const pendingDirectorySyncs = new Set(); export function mkdirMailDirectory(path: string): void { @@ -738,11 +753,20 @@ export function deadLetterUndelivered( const filename = `${safeTs}-${record.id}-${randomUUID()}.json`; const tmpPath = join(inbox.tmp, filename); writeFileSync(tmpPath, JSON.stringify({ ...record, read: false, ...(relayDelivery ? { relayDelivery, relayPayload: { from: record.from, to: record.to, body: record.body, timestamp: record.timestamp }, receivedAt: new Date().toISOString() } : {}) }, null, 2), "utf-8"); - writeReasonSidecar(inbox.dlq, filename, cls, reason); - if (relayDelivery) { - syncMailFile(join(inbox.dlq, `${filename}.reason`)); - publishRelayedRecord(tmpPath, join(inbox.dlq, filename)); - } else renameSync(tmpPath, join(inbox.dlq, filename)); + const sidecar = join(inbox.dlq, `${filename}.reason`); + try { + writeReasonSidecar(inbox.dlq, filename, cls, reason); + if (relayDelivery) { + syncMailFile(sidecar); + publishRelayedRecord(tmpPath, join(inbox.dlq, filename)); + } else renameSync(tmpPath, join(inbox.dlq, filename)); + } catch (error) { + if (relayDelivery && !existsSync(join(inbox.dlq, filename))) { + try { removeMailFileConfirmed(sidecar); } + catch { throw new DeadLetterCleanupError("dead-letter sidecar removal could not be confirmed"); } + } + throw error; + } return join(inbox.dlq, filename); } diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index bfc77a7a..4c9f5ca4 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -3,7 +3,7 @@ import { dirname, join, resolve, sep } from "node:path"; import { homedir } from "node:os"; import { createHash, randomUUID } from "node:crypto"; import { sanitizeIdentifier } from "../schema/sanitizer.js"; -import { countInboxMessages, deadLetterUndelivered, findRelayedRecord, getMailDir, mkdirMailDirectory, MailInboxFullError, MailSendInputError, relayAcceptRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; +import { countInboxMessages, DeadLetterCleanupError, deadLetterUndelivered, findRelayedRecord, removeMailFileConfirmed, getMailDir, mkdirMailDirectory, MailInboxFullError, MailSendInputError, relayAcceptRoot, syncMailFile, syncMailDirectory, inboxFullMessage, MAX_INBOX_MESSAGES, sendMessage, type PromoteRejectClass } from "./mail.js"; import { acquireMailLockSync } from "./mail-lock.js"; import { LoopDetector } from "./loop-detector.js"; import { FileSystemTransport, resolveTransport, TransportRegistry, type TransportChannel, type TpsMessage } from "./transport.js"; @@ -478,7 +478,10 @@ function recordAcceptance(acceptedDir: string, marker: string, receipt: RelayAcc try { unlinkSync(marker); syncMailDirectory(dirname(marker)); } catch { throw new ReceiptRecoveryIncompleteError("relay receipt recovery incomplete; retry may duplicate"); } } - try { if (existsSync(tmp)) unlinkSync(tmp); } catch {} + try { removeMailFileConfirmed(tmp); } + catch { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw new ReceiptRecoveryIncompleteError("relay receipt recovery incomplete; retry may duplicate"); + } throw error; } try { @@ -654,7 +657,8 @@ export function deliverRelayedToLocal(branchId: string, body: MailDeliverBody): delivery, recipientRoot, ); - } catch { + } catch (deadLetterError) { + if (deadLetterError instanceof DeadLetterCleanupError) throw new Error(`relay record recovery incomplete for message ${body.id}; retry may duplicate`); try { undoNewRelayRecord(recipientRoot, delivery); } catch { throw new Error(`relay record recovery incomplete for message ${body.id}; retry may duplicate`); } console.error(`[relay] dead-letter failed for message ${body.id} to ${body.to}`); diff --git a/packages/cli/test/relay-attempt-artifacts.test.ts b/packages/cli/test/relay-attempt-artifacts.test.ts new file mode 100644 index 00000000..5759e1a5 --- /dev/null +++ b/packages/cli/test/relay-attempt-artifacts.test.ts @@ -0,0 +1,104 @@ +import { afterEach, beforeEach, expect, mock, spyOn, test } from "bun:test"; +import * as fs from "node:fs"; +import { randomUUID } from "node:crypto"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { getInbox, MAX_INBOX_MESSAGES, sendMessage } from "../src/utils/mail.js"; +import { deliverRelayedToLocal, relayAcceptanceReceiptPath } from "../src/utils/relay.js"; + +let root: string; +let saved: Record; + +beforeEach(() => { + root = fs.mkdtempSync(join(tmpdir(), "relay-attempt-artifacts-")); + saved = Object.fromEntries(["HOME", "TPS_MAIL_DIR"].map((key) => [key, process.env[key]])); + process.env.HOME = root; + process.env.TPS_MAIL_DIR = join(root, "mail"); +}); + +afterEach(() => { mock.restore(); }); + +afterEach(() => { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + fs.rmSync(root, { recursive: true, force: true }); +}); + +function body() { + return { id: randomUUID(), from: "remote", to: "local", content: "payload", timestamp: new Date().toISOString() }; +} + +function records(): string[] { + return fs.readdirSync(getInbox("local").fresh).filter((file) => file.endsWith(".json")); +} + +function failOn(name: "renameSync" | "unlinkSync", match: (path: string) => boolean, message: string) { + const real = fs[name] as (...args: any[]) => unknown; + return spyOn(fs, name).mockImplementation(((...args: any[]) => { + if (match(String(args[name === "renameSync" ? 1 : 0]))) throw new Error(message); + return real(...args); + }) as any); +} + +function refusalOf(item: ReturnType): string { + try { deliverRelayedToLocal("remote", item); } catch (error) { return String(error); } + return ""; +} + +test("receipt temp: rename fails and the temp delete fails -> incomplete-recovery refusal", () => { + const item = body(); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const rename = failOn("renameSync", (path) => path === marker, "injected receipt rename failure"); + const unlink = failOn("unlinkSync", (path) => path === `${marker}.tmp`, "injected temp delete failure"); + let refusal = ""; + try { refusal = refusalOf(item); } + finally { unlink.mockRestore(); rename.mockRestore(); } + expect(refusal).toContain("relay receipt recovery incomplete"); + expect(refusal).toContain("retry may duplicate"); + expect(refusal).not.toContain("retry delivery"); +}); + +test("receipt temp: rename fails and the temp delete succeeds -> retryable refusal, no temp left", () => { + const item = body(); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const rename = failOn("renameSync", (path) => path === marker, "injected receipt rename failure"); + let refusal = ""; + try { refusal = refusalOf(item); } + finally { rename.mockRestore(); } + expect(refusal).toContain("relay acceptance receipt write failed; retry delivery"); + expect(fs.existsSync(`${marker}.tmp`)).toBe(false); + expect(records()).toEqual([]); +}); + +function fillInbox(): void { + for (let i = 0; i < MAX_INBOX_MESSAGES; i++) sendMessage("local", `filler-${i}`, "seeder"); +} + +test("dead-letter sidecar: publish fails and the sidecar delete fails -> incomplete-recovery refusal", () => { + const item = body(); + fillInbox(); + const dlq = getInbox("local").dlq; + const rename = failOn("renameSync", (path) => path.startsWith(dlq), "injected dead-letter publish failure"); + const unlink = failOn("unlinkSync", (path) => path.endsWith(".reason"), "injected sidecar delete failure"); + let refusal = ""; + try { refusal = refusalOf(item); } + finally { unlink.mockRestore(); rename.mockRestore(); } + expect(refusal).toContain("relay record recovery incomplete"); + expect(refusal).toContain("retry may duplicate"); + expect(refusal).not.toContain("retry delivery"); +}); + +test("dead-letter sidecar: publish fails and the sidecar delete succeeds -> no sidecar left", () => { + const item = body(); + fillInbox(); + const dlq = getInbox("local").dlq; + const rename = failOn("renameSync", (path) => path.startsWith(dlq), "injected dead-letter publish failure"); + let refusal = ""; + try { refusal = refusalOf(item); } + finally { rename.mockRestore(); } + expect(refusal).toContain("relay dead-letter failed; retry delivery"); + expect(fs.readdirSync(dlq).filter((file) => file.endsWith(".reason"))).toEqual([]); + expect(records()).toHaveLength(MAX_INBOX_MESSAGES); +}); diff --git a/packages/cli/test/relay-delivery-loss.test.ts b/packages/cli/test/relay-delivery-loss.test.ts index 4b4a8830..40371419 100644 --- a/packages/cli/test/relay-delivery-loss.test.ts +++ b/packages/cli/test/relay-delivery-loss.test.ts @@ -191,7 +191,7 @@ for (const entry of ["sync", "connect"] as const) { expect(acks).toEqual([]); expect(drainOutbox(false).map((m) => m.id)).toEqual([body.id]); expect(jsonFiles(inbox.dlq)).toEqual([]); - if (fault === "crash-before-publish") expect(fs.readdirSync(inbox.dlq).some((f) => f.endsWith(".reason"))).toBe(true); + if (fault === "crash-before-publish") expect(fs.readdirSync(inbox.dlq).some((f) => f.endsWith(".reason"))).toBe(false); const logs = errors.mock.calls.flat().join("\n"); expect(logs).toContain(body.id); expect(logs).toContain("local"); From 3d69f795a4444458787aa9c69f2c5ca43d2711dd Mon Sep 17 00:00:00 2001 From: flint Date: Sat, 10 Oct 2026 02:36:16 -0700 Subject: [PATCH 09/10] test(relay): pin that a failed acceptance removes its dead-letter temp record; scope rollback to thrown failures Co-Authored-By: Claude Opus 5.5 --- .../fixed-573-relay-resend-after-ack.md | 2 +- .../cli/test/relay-attempt-artifacts.test.ts | 27 +++++++++++++++++++ packages/cli/test/relay-delivery-loss.test.ts | 8 +++--- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md index e710cd1d..2386f8b1 100644 --- a/.changelog/unreleased/fixed-573-relay-resend-after-ack.md +++ b/.changelog/unreleased/fixed-573-relay-resend-after-ack.md @@ -1 +1 @@ -- **Deduplicate relay resends after local ACK while the receipt remains.** A fixed set of lock stripes serializes acceptance; failed acceptance writes roll back what the attempt wrote, and incomplete rollback refuses delivery with a possible-duplicate warning; the receipt TTL also applies to flat per-branch markers. +- **Deduplicate relay resends after local ACK while the receipt remains.** A fixed set of lock stripes serializes acceptance; a failed acceptance that throws rolls back what the attempt wrote, and incomplete rollback refuses delivery with a possible-duplicate warning; the receipt TTL also applies to flat per-branch markers. diff --git a/packages/cli/test/relay-attempt-artifacts.test.ts b/packages/cli/test/relay-attempt-artifacts.test.ts index 5759e1a5..3edaabfe 100644 --- a/packages/cli/test/relay-attempt-artifacts.test.ts +++ b/packages/cli/test/relay-attempt-artifacts.test.ts @@ -101,4 +101,31 @@ test("dead-letter sidecar: publish fails and the sidecar delete succeeds -> no s expect(refusal).toContain("relay dead-letter failed; retry delivery"); expect(fs.readdirSync(dlq).filter((file) => file.endsWith(".reason"))).toEqual([]); expect(records()).toHaveLength(MAX_INBOX_MESSAGES); + expect(fs.readdirSync(getInbox("local").tmp).filter((file) => file.endsWith(".json"))).toEqual([]); +}); + +test("dead-letter temp record: publish fails and the temp delete succeeds -> no tmp record left, retryable refusal", () => { + const item = body(); + fillInbox(); + const inbox = getInbox("local"); + const rename = failOn("renameSync", (path) => path.startsWith(inbox.dlq), "injected dead-letter publish failure"); + let refusal = ""; + try { refusal = refusalOf(item); } + finally { rename.mockRestore(); } + expect(refusal).toContain("relay dead-letter failed; retry delivery"); + expect(fs.readdirSync(inbox.tmp).filter((file) => file.endsWith(".json"))).toEqual([]); +}); + +test("dead-letter temp record: publish fails and the temp delete fails -> incomplete-recovery refusal", () => { + const item = body(); + fillInbox(); + const inbox = getInbox("local"); + const rename = failOn("renameSync", (path) => path.startsWith(inbox.dlq), "injected dead-letter publish failure"); + const unlink = failOn("unlinkSync", (path) => path.startsWith(inbox.tmp) && path.endsWith(".json"), "injected temp delete failure"); + let refusal = ""; + try { refusal = refusalOf(item); } + finally { unlink.mockRestore(); rename.mockRestore(); } + expect(refusal).toContain("relay record recovery incomplete"); + expect(refusal).toContain("retry may duplicate"); + expect(refusal).not.toContain("retry delivery"); }); diff --git a/packages/cli/test/relay-delivery-loss.test.ts b/packages/cli/test/relay-delivery-loss.test.ts index 40371419..05695a2b 100644 --- a/packages/cli/test/relay-delivery-loss.test.ts +++ b/packages/cli/test/relay-delivery-loss.test.ts @@ -169,7 +169,7 @@ for (const entry of ["sync", "connect"] as const) { expect(rejected.some((r) => r.id === bodies[2]!.id)).toBe(true); }); - for (const fault of ["sidecar", "record", "crash-before-publish"] as const) { + for (const fault of ["sidecar", "record", "publish-throws"] as const) { test(`${fault} failure leaves the branch source unacked and retryable`, async () => { fillInbox(); const body = queue(JSON.stringify(buildSignedEnvelope("remote", "local", "retry me", SEEDS))); @@ -178,9 +178,9 @@ for (const entry of ["sync", "connect"] as const) { await start(); const write = fs.writeFileSync; const rename = fs.renameSync; - const injected = fault === "crash-before-publish" + const injected = fault === "publish-throws" ? spyOn(fs, "renameSync").mockImplementation((src, dst) => { - if (String(dst).startsWith(inbox.dlq)) throw new Error("simulated crash before publish"); + if (String(dst).startsWith(inbox.dlq)) throw new Error("simulated publish failure"); return rename(src, dst); }) : spyOn(fs, "writeFileSync").mockImplementation((path, data, opts) => { @@ -191,7 +191,7 @@ for (const entry of ["sync", "connect"] as const) { expect(acks).toEqual([]); expect(drainOutbox(false).map((m) => m.id)).toEqual([body.id]); expect(jsonFiles(inbox.dlq)).toEqual([]); - if (fault === "crash-before-publish") expect(fs.readdirSync(inbox.dlq).some((f) => f.endsWith(".reason"))).toBe(false); + if (fault === "publish-throws") expect(fs.readdirSync(inbox.dlq).some((f) => f.endsWith(".reason"))).toBe(false); const logs = errors.mock.calls.flat().join("\n"); expect(logs).toContain(body.id); expect(logs).toContain("local"); From e88dd3c8123dc2f0c247f78a8cc2fef74f6682ae Mon Sep 17 00:00:00 2001 From: flint Date: Sat, 10 Oct 2026 03:59:51 -0700 Subject: [PATCH 10/10] test(relay): the lock-coverage and prune-boundary claims each have a test that fails without them Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/utils/relay.ts | 10 ++-- .../cli/test/relay-accept-invariants.test.ts | 31 +++++++++++ .../cli/test/relay-accept-single-lock.test.ts | 52 +++++++++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/utils/relay.ts b/packages/cli/src/utils/relay.ts index 4c9f5ca4..9520bfa3 100644 --- a/packages/cli/src/utils/relay.ts +++ b/packages/cli/src/utils/relay.ts @@ -501,17 +501,20 @@ function existingAcceptanceMarker(acceptedDir: string, id: string, legacyMarker: if (!receiptMayExist(path)) continue; try { if (Date.now() - statSync(path).mtimeMs > relayAcceptReceiptTtlMs()) continue; - } catch {} + } catch { + if (!receiptMayExist(path)) continue; + } return path; } } const flatMarker = join(acceptedDir, id); if (receiptMayExist(flatMarker)) { try { if (Date.now() - statSync(flatMarker).mtimeMs <= relayAcceptReceiptTtlMs()) return flatMarker; } - catch { return flatMarker; } + catch { if (receiptMayExist(flatMarker)) return flatMarker; } } if (!receiptMayExist(legacyMarker)) return undefined; - try { if (Date.now() - statSync(legacyMarker).mtimeMs > relayAcceptReceiptTtlMs()) return undefined; } catch {} + try { if (Date.now() - statSync(legacyMarker).mtimeMs > relayAcceptReceiptTtlMs()) return undefined; } + catch { if (!receiptMayExist(legacyMarker)) return undefined; } return legacyMarker; } @@ -688,6 +691,7 @@ async function acceptRelayedMail( onAccepted?: () => void, ): Promise { const delivered = deliverRelayedToLocal(branchId, body); + // A delivery with delivered=false (dead-lettered or receipt-backed duplicate) is still ACKed below. if (delivered) { const reportError = () => { console.error(`[relay] onAccepted failed for message ${body.id} to ${body.to}`); diff --git a/packages/cli/test/relay-accept-invariants.test.ts b/packages/cli/test/relay-accept-invariants.test.ts index 332eea94..f28deda4 100644 --- a/packages/cli/test/relay-accept-invariants.test.ts +++ b/packages/cli/test/relay-accept-invariants.test.ts @@ -152,6 +152,37 @@ test("a flat per-branch marker with a live record remains an in-flight duplicate expect(records()).toEqual([]); }); +test("a receipt in an ended day bucket younger than the TTL survives prune and still dedups", () => { + const item = body(); + expect(deliverRelayedToLocal("remote", item)).toBe(true); + const [file] = records(); + ackMessageAtPath(join(getInbox("local").fresh, file!)); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const accepted = dirname(dirname(marker)); + const ended = join(accepted, new Date(Date.now() - 3 * 24 * 60 * 60 * 1000).toISOString().slice(0, 10)); + fs.renameSync(dirname(marker), ended); + expect(pruneRelayAcceptanceReceipts(accepted, Date.now(), 7 * 24 * 60 * 60 * 1000)).toBe(0); + expect(fs.existsSync(join(ended, item.id))).toBe(true); + expect(deliverRelayedToLocal("remote", item)).toBe(false); + expect(records()).toEqual([]); +}); + +test("a receipt that vanishes between listing and stat is treated as absent", () => { + const item = body(); + expect(deliverRelayedToLocal("remote", item)).toBe(true); + const [file] = records(); + ackMessageAtPath(join(getInbox("local").fresh, file!)); + const marker = relayAcceptanceReceiptPath("remote", item.id); + const stat = fs.statSync; + const fault = spyOn(fs, "statSync").mockImplementation(((path: fs.PathLike, options?: unknown) => { + if (String(path) === marker) fs.unlinkSync(marker); + return (stat as (p: fs.PathLike, o?: unknown) => unknown)(path, options); + }) as typeof fs.statSync); + try { expect(deliverRelayedToLocal("remote", item)).toBe(true); } + finally { fault.mockRestore(); } + expect(records()).toHaveLength(1); +}); + test("flat per-branch markers expire with receipts", () => { const accepted = join(process.env.TPS_MAIL_DIR!, ".relay-accepted", "by-branch", "remote"); fs.mkdirSync(accepted, { recursive: true }); diff --git a/packages/cli/test/relay-accept-single-lock.test.ts b/packages/cli/test/relay-accept-single-lock.test.ts index a21c829b..175db917 100644 --- a/packages/cli/test/relay-accept-single-lock.test.ts +++ b/packages/cli/test/relay-accept-single-lock.test.ts @@ -154,6 +154,23 @@ function recordsByDeliveryId(): Map { return byId; } +/** The stored receipt carries the delivered body, and a resend of that body dedups after ACK. */ +function expectReceiptKeepsBody(id: string, content: string): void { + const receipt = JSON.parse(readFileSync(relayAcceptanceReceiptPath(BRANCH, id), "utf8")) as { body: string }; + expect(receipt.body).toBe(content); + expect(deliverRelayedToLocal(BRANCH, { id, from: FROM, to: RECIPIENT, content, timestamp: TIMESTAMP })).toBe(false); + expect(recordsByDeliveryId().has(id)).toBe(false); +} + +function expectReceiptMatchesDeliveredRecord(id: string, bodyPattern: RegExp): void { + const [file] = recordsByDeliveryId().get(id)!; + const path = join(getInbox(RECIPIENT).fresh, file!); + const record = JSON.parse(readFileSync(path, "utf8")) as { body: string }; + expect(record.body).toMatch(bodyPattern); + ackMessageAtPath(path); + expectReceiptKeepsBody(id, record.body); +} + async function waitForFile(path: string): Promise { const deadline = Date.now() + 10000; while (!existsSync(path)) { @@ -240,6 +257,41 @@ describe("relay acceptance (cli#561)", () => { const byId = recordsByDeliveryId(); expect([...byId.keys()].sort()).toEqual([...ids].sort()); for (const id of ids) expect(byId.get(id)).toHaveLength(1); + + for (const id of ids) expectReceiptMatchesDeliveredRecord(id, /^[AB]-/); + }, 60_000); + + test("a differing payload waiting on the stripe is refused and the receipt keeps the delivered body", async () => { + const id = randomUUID(); + const pause = join(root, "differing-pause"); + const started = join(root, "differing-started"); + const first = deliverChild("A-", [id], { RELAY_CHILD_PAUSE: pause }); + let second: Promise | undefined; + try { + await waitForFile(pause + ".ready"); + second = deliverChild("B-", [id], { RELAY_CHILD_STARTED: started }); + await waitForFile(started); + await new Promise((resolve) => setTimeout(resolve, 200)); + } finally { writeFileSync(pause + ".release", ""); } + // ACK each record the moment it appears, so a waiter that took the lock late finds no record. + const acked: string[] = []; + let settled = false; + const both = Promise.all([first, second]).finally(() => { settled = true; }); + while (!settled) { + for (const file of jsonFiles(getInbox(RECIPIENT).fresh)) { + const path = join(getInbox(RECIPIENT).fresh, file); + try { + acked.push((JSON.parse(readFileSync(path, "utf8")) as { body: string }).body); + ackMessageAtPath(path); + } catch {} + } + await new Promise((resolve) => setImmediate(resolve)); + } + const [a, b] = await both; + expect(a).toEqual({ delivered: 1, duplicate: 0, refused: 0 }); + expect(b).toEqual({ delivered: 0, duplicate: 0, refused: 1 }); + expect(acked).toEqual([`A-${id}`]); + expectReceiptKeepsBody(id, `A-${id}`); }, 60_000); test("lock directories stay bounded across distinct deliveries", () => {