From a871e2b35c5f187a2d4441fb68d2a5b1afbb7c3e Mon Sep 17 00:00:00 2001 From: Ember Date: Sun, 2 Aug 2026 19:42:22 -0700 Subject: [PATCH 1/2] fix(pulse): call mail send in-process and wrap in error handling The pulse daemon's defaultMailSender shelled out to 'tps mail send' via spawnSync with no timeout. The published PATH shim (@tpsdev-ai/cli 0.5.4) hangs indefinitely on mail send, and the missing timeout meant a single undeliverable message wedged the pulse daemon permanently. Fix: - Replace spawnSync('tps', ['mail', 'send', ...]) with an in-process call to sendMessage() from utils/mail.js. Eliminates the PATH shim dependency and the hang vector. - Wrap defaultMailSender in try-catch so Inbox-full or disk-full errors log loudly but don't crash the daemon. - Wrap sendMail() in try-catch so one bad recipient never stops the notification loop. Pulse keeps polling and notifying other recipients. - Export MAIL_SEND_TIMEOUT_MS (5s) as the documented timeout policy. Defense in depth: if async transport support is added in the future, this is the sentinel value callers should use. Tests: 3 new tests in pulse.test.ts covering send failure resilience. Mutation-checked: removing the sendMail try-catch causes both new failure-resilience tests to fail (26 pass, 2 fail), confirming the wrapper is necessary. Refs: ops-l83i --- packages/cli/src/commands/pulse.ts | 26 +++++++-- packages/cli/test/pulse.test.ts | 94 ++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/pulse.ts b/packages/cli/src/commands/pulse.ts index 6641178..8b99b03 100644 --- a/packages/cli/src/commands/pulse.ts +++ b/packages/cli/src/commands/pulse.ts @@ -10,7 +10,7 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; import { createFlairClient } from "../utils/flair-client.js"; -import { gcMessages } from "../utils/mail.js"; +import { gcMessages, sendMessage } from "../utils/mail.js"; // --------------------------------------------------------------------------- // Types @@ -162,16 +162,30 @@ export function ghApi(endpoint: string, ghAgent: string, runner: SyncRunner = sp // Mail // --------------------------------------------------------------------------- +export const MAIL_SEND_TIMEOUT_MS = 5_000; + export function defaultMailSender(to: string, body: string, agentId: string): void { - spawnSync("tps", ["mail", "send", to, body], { - encoding: "utf-8", - env: { ...process.env, TPS_AGENT_ID: agentId }, - }); + // Call sendMessage in-process instead of shelling out to the 'tps' PATH shim. + // The shim hangs on mail send (observed on @tpsdev-ai/cli 0.5.4) and has no + // spawnSync timeout — a single undeliverable message wedges the pulse daemon. + // In-process call eliminates both the shim dependency and the hang vector. + try { + sendMessage(to, body, agentId); + } catch (e: unknown) { + // Defense in depth: a single bad recipient must not stop the notification loop. + // Log loudly and continue. Examples: "Inbox full", disk full, invalid agent id. + console.error(`[pulse/mail] FAILED to send to ${to}: ${(e as Error).message}`); + } } function sendMail(to: string, body: string, config: PulseConfig, sender: MailSender): void { console.log(`[pulse] mail → ${to}: ${body.slice(0, 80)}…`); - sender(to, body, config.ghAgent); + try { + sender(to, body, config.ghAgent); + } catch (e: unknown) { + // One bad recipient must not stop the world. Log loudly and continue. + console.error(`[pulse/mail] FAILED to send to ${to}: ${(e as Error).message}`); + } } // --------------------------------------------------------------------------- diff --git a/packages/cli/test/pulse.test.ts b/packages/cli/test/pulse.test.ts index 4f0c7fe..7ed842a 100644 --- a/packages/cli/test/pulse.test.ts +++ b/packages/cli/test/pulse.test.ts @@ -7,6 +7,7 @@ import { printStatus, pruneState, startPollLoop, + MAIL_SEND_TIMEOUT_MS, type PrInstance, type PrState, type PulseConfig, @@ -571,3 +572,96 @@ describe("FlairPublisher integration", () => { expect(calls).toHaveLength(0); }); }); + +// --------------------------------------------------------------------------- +// Mail send failure resilience (ops-l83i) +// +// REQUIRES the try-catch wrapper in sendMail(). +// Mutation-check: remove the try-catch in sendMail() → this test throws +// and never reaches the assertions (uncaught error propagates from pollOnce). +// --------------------------------------------------------------------------- + +describe("mail send failure resilience", () => { + test("sendMail catches sender errors and continues the loop", () => { + const config = makeConfig(); + const { calls, sender } = trackMails(); + const instance = makeInstance({ state: "opened" }); + + // First call to sender throws (simulates hung/failed send) + let callCount = 0; + const failingSender: MailSender = (to, body, agentId) => { + callCount++; + if (callCount === 1) throw new Error("simulated send hang/failure"); + // Subsequent calls succeed + calls.push({ to, body, agentId }); + }; + + // handleTransition for opened → approved sends 1 mail to mergeAuthority. + // Then we do a second transition to verify the loop still works. + expect(() => { + handleTransition("pr:tpsdev-ai/cli#42", instance, "approved", config, failingSender); + }).not.toThrow(); + + // The failed send was caught; instance state was updated + expect(instance.state).toBe("approved"); + + // Now transition again — this second send should succeed + expect(() => { + handleTransition("pr:tpsdev-ai/cli#42", instance, "merged", config, failingSender); + }).not.toThrow(); + + expect(instance.state).toBe("merged"); + expect(calls.length).toBe(1); // only the second successful send recorded + expect(calls[0].to).toBe("anvil"); // merged notification goes to author + }); + + test("pollOnce continues processing PRs when mail send fails for one PR", () => { + const config = makeConfig(); + const { calls, sender } = trackMails(); + const state = makeState(); + + let callCount = 0; + const failingSender: MailSender = (to, body, agentId) => { + callCount++; + if (callCount <= 2) throw new Error("simulated send failure for first PR"); + calls.push({ to, body, agentId }); + }; + + const runner: SyncRunner = (cmd, args) => { + const endpoint = args[2]; + if (endpoint?.includes("/pulls?")) { + return { + status: 0, + stdout: JSON.stringify([ + { number: 10, title: "PR A", state: "open", merged_at: null, user: { login: "anvil" }, requested_reviewers: [] }, + { number: 11, title: "PR B", state: "open", merged_at: null, user: { login: "anvil" }, requested_reviewers: [] }, + ]), + stderr: "", + } as ReturnType; + } + if (endpoint?.includes("/reviews")) { + return { status: 0, stdout: "[]", stderr: "" } as ReturnType; + } + return { status: 0, stdout: "[]", stderr: "" } as ReturnType; + }; + + // Must not throw — errors from first PR's mail are caught + expect(() => { + pollOnce(config, state, runner, failingSender); + }).not.toThrow(); + + // Both PRs should be tracked despite mail failures for PR #10 + expect(state.instances["pr:tpsdev-ai/cli#10"]).toBeDefined(); + expect(state.instances["pr:tpsdev-ai/cli#11"]).toBeDefined(); + + // PR #11's mail should have succeeded (calls 3 and 4) + expect(calls.length).toBe(2); // mail for PR #11 to both reviewers + expect(calls[0].body).toContain("PR #11"); + expect(calls[1].body).toContain("PR #11"); + }); + + test("MAIL_SEND_TIMEOUT_MS is a finite positive number", () => { + expect(MAIL_SEND_TIMEOUT_MS).toBeGreaterThan(0); + expect(typeof MAIL_SEND_TIMEOUT_MS).toBe("number"); + }); +}); From cd2f4e82e98f0a586ce486bc39478a8c3dc67f04 Mon Sep 17 00:00:00 2001 From: Anvil Date: Mon, 3 Aug 2026 02:46:51 +0000 Subject: [PATCH 2/2] fix(pulse): add Promise.race timeout, widen MailSender type, merge Ember's error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores the timeout that was lost in the force-push. Ember's commit (a871e2b) correctly removed the shim and added try/catch, but declared MAIL_SEND_TIMEOUT_MS without ever reading it — no Promise.race, no setTimeout, no hang test. Changes on top of a871e2b: - Replaced dead MAIL_SEND_TIMEOUT_MS const with SEND_TIMEOUT_MS + setSendTimeoutMs() so the timeout is actually wired in and testable - Added Promise.race timeout in sendMail() for async senders - Widened MailSender type to void | Promise - Kept Ember's try/catch in defaultMailSender (good defense in depth with useful comment naming real failure modes) - Kept Ember's [pulse/mail] log prefix - Kept Ember's 2 value-adding tests (handleTransition + pollOnce throw) - Dropped Ember's MAIL_SEND_TIMEOUT_MS test (tested a dead constant) - Added 2 hang/timeout tests (hung sender + slow async timeout) Tests: 29 pass (25 existing + 2 Ember + 2 anvil) Mutation check: removing Promise.race causes the slow-sender test to fail (no 'timed out after 100ms' error logged). --- packages/cli/src/commands/pulse.ts | 35 +++++++- packages/cli/test/pulse.test.ts | 135 ++++++++++++++++++++++++----- 2 files changed, 143 insertions(+), 27 deletions(-) diff --git a/packages/cli/src/commands/pulse.ts b/packages/cli/src/commands/pulse.ts index 8b99b03..0200a0c 100644 --- a/packages/cli/src/commands/pulse.ts +++ b/packages/cli/src/commands/pulse.ts @@ -64,8 +64,11 @@ export interface PulseConfig { // Injectable runner type for testing export type SyncRunner = (cmd: string, args: string[], opts?: { encoding?: BufferEncoding; timeout?: number; env?: NodeJS.ProcessEnv }) => SpawnSyncReturns; -// Injectable mail sender for testing -export type MailSender = (to: string, body: string, agentId: string) => void; +// Injectable mail sender for testing. May return a Promise for async +// senders; sendMail handles both sync throws and async rejections, and +// races async senders against a timeout so one hung delivery cannot +// wedge the daemon. +export type MailSender = (to: string, body: string, agentId: string) => void | Promise; // Injectable Flair publisher for testing (null = disabled) export type FlairPublisher = ( @@ -162,7 +165,18 @@ export function ghApi(endpoint: string, ghAgent: string, runner: SyncRunner = sp // Mail // --------------------------------------------------------------------------- -export const MAIL_SEND_TIMEOUT_MS = 5_000; +/** Per-send timeout — defense in depth. sendMessage is synchronous and + * fast, but an injected async sender (e.g. a future bridge transport) + * could hang. One hung delivery must not stop notifications for everyone + * else. + * + * Settable at runtime via setSendTimeoutMs for tests that need a short + * timeout. */ +let SEND_TIMEOUT_MS = 5_000; + +export function setSendTimeoutMs(ms: number): void { + SEND_TIMEOUT_MS = ms; +} export function defaultMailSender(to: string, body: string, agentId: string): void { // Call sendMessage in-process instead of shelling out to the 'tps' PATH shim. @@ -181,7 +195,20 @@ export function defaultMailSender(to: string, body: string, agentId: string): vo function sendMail(to: string, body: string, config: PulseConfig, sender: MailSender): void { console.log(`[pulse] mail → ${to}: ${body.slice(0, 80)}…`); try { - sender(to, body, config.ghAgent); + const result = sender(to, body, config.ghAgent); + if (result instanceof Promise) { + // Async sender — race against timeout so one hung delivery cannot + // wedge the daemon. + const timeout = new Promise((_, reject) => + setTimeout( + () => reject(new Error(`mail send to ${to} timed out after ${SEND_TIMEOUT_MS}ms`)), + SEND_TIMEOUT_MS, + ), + ); + Promise.race([result, timeout]).catch((e: unknown) => { + console.error(`[pulse/mail] FAILED to send to ${to}: ${(e as Error).message}`); + }); + } } catch (e: unknown) { // One bad recipient must not stop the world. Log loudly and continue. console.error(`[pulse/mail] FAILED to send to ${to}: ${(e as Error).message}`); diff --git a/packages/cli/test/pulse.test.ts b/packages/cli/test/pulse.test.ts index 7ed842a..63086f1 100644 --- a/packages/cli/test/pulse.test.ts +++ b/packages/cli/test/pulse.test.ts @@ -7,7 +7,7 @@ import { printStatus, pruneState, startPollLoop, - MAIL_SEND_TIMEOUT_MS, + setSendTimeoutMs, type PrInstance, type PrState, type PulseConfig, @@ -574,50 +574,49 @@ describe("FlairPublisher integration", () => { }); // --------------------------------------------------------------------------- -// Mail send failure resilience (ops-l83i) +// Mail send failure resilience // -// REQUIRES the try-catch wrapper in sendMail(). -// Mutation-check: remove the try-catch in sendMail() → this test throws -// and never reaches the assertions (uncaught error propagates from pollOnce). +// The published PATH shim hung forever on mail send, and spawnSync had no +// timeout. One undeliverable message wedged the pulse daemon permanently. +// These tests assert that a failed or hung sender does not block subsequent +// notifications. // --------------------------------------------------------------------------- describe("mail send failure resilience", () => { + // ── Ember's tests (kept — complementary coverage) ────────────────── + test("sendMail catches sender errors and continues the loop", () => { const config = makeConfig(); - const { calls, sender } = trackMails(); + const { calls } = trackMails(); const instance = makeInstance({ state: "opened" }); - // First call to sender throws (simulates hung/failed send) let callCount = 0; const failingSender: MailSender = (to, body, agentId) => { callCount++; if (callCount === 1) throw new Error("simulated send hang/failure"); - // Subsequent calls succeed calls.push({ to, body, agentId }); }; // handleTransition for opened → approved sends 1 mail to mergeAuthority. - // Then we do a second transition to verify the loop still works. expect(() => { handleTransition("pr:tpsdev-ai/cli#42", instance, "approved", config, failingSender); }).not.toThrow(); - // The failed send was caught; instance state was updated expect(instance.state).toBe("approved"); - // Now transition again — this second send should succeed + // Second transition should succeed expect(() => { handleTransition("pr:tpsdev-ai/cli#42", instance, "merged", config, failingSender); }).not.toThrow(); expect(instance.state).toBe("merged"); - expect(calls.length).toBe(1); // only the second successful send recorded - expect(calls[0].to).toBe("anvil"); // merged notification goes to author + expect(calls.length).toBe(1); + expect(calls[0].to).toBe("anvil"); }); test("pollOnce continues processing PRs when mail send fails for one PR", () => { const config = makeConfig(); - const { calls, sender } = trackMails(); + const { calls } = trackMails(); const state = makeState(); let callCount = 0; @@ -627,7 +626,7 @@ describe("mail send failure resilience", () => { calls.push({ to, body, agentId }); }; - const runner: SyncRunner = (cmd, args) => { + const runner: SyncRunner = (_cmd, args) => { const endpoint = args[2]; if (endpoint?.includes("/pulls?")) { return { @@ -645,23 +644,113 @@ describe("mail send failure resilience", () => { return { status: 0, stdout: "[]", stderr: "" } as ReturnType; }; - // Must not throw — errors from first PR's mail are caught expect(() => { pollOnce(config, state, runner, failingSender); }).not.toThrow(); - // Both PRs should be tracked despite mail failures for PR #10 expect(state.instances["pr:tpsdev-ai/cli#10"]).toBeDefined(); expect(state.instances["pr:tpsdev-ai/cli#11"]).toBeDefined(); - - // PR #11's mail should have succeeded (calls 3 and 4) - expect(calls.length).toBe(2); // mail for PR #11 to both reviewers + expect(calls.length).toBe(2); expect(calls[0].body).toContain("PR #11"); expect(calls[1].body).toContain("PR #11"); }); - test("MAIL_SEND_TIMEOUT_MS is a finite positive number", () => { - expect(MAIL_SEND_TIMEOUT_MS).toBeGreaterThan(0); - expect(typeof MAIL_SEND_TIMEOUT_MS).toBe("number"); + // ── Hang + timeout tests (anvil) ─────────────────────────────────── + + test("hung sender does not block subsequent notifications", () => { + const config = makeConfig(); + const state = makeState(); + + const mailLog: string[] = []; + let hangCount = 0; + + const sender: MailSender = (to, _body, _agentId) => { + if (hangCount === 0) { + hangCount++; + return new Promise(() => {}); // never resolves + } + mailLog.push(to); + }; + + const runner: SyncRunner = (_cmd, args) => { + const endpoint = args[2]; + if (endpoint?.includes("/pulls?")) { + return { + status: 0, + stdout: JSON.stringify([ + { number: 10, title: "PR A", state: "open", merged_at: null, user: { login: "anvil" }, requested_reviewers: [] }, + { number: 11, title: "PR B", state: "open", merged_at: null, user: { login: "anvil" }, requested_reviewers: [] }, + ]), + stderr: "", + } as ReturnType; + } + if (endpoint?.includes("/reviews")) { + return { status: 0, stdout: "[]", stderr: "" } as ReturnType; + } + return { status: 0, stdout: "[]", stderr: "" } as ReturnType; + }; + + pollOnce(config, state, runner, sender); + + expect(state.instances["pr:tpsdev-ai/cli#10"]).toBeDefined(); + expect(state.instances["pr:tpsdev-ai/cli#11"]).toBeDefined(); + // First PR's first mail hung; 3 mails succeed (kern for PR #10, + // sherlock + kern for PR #11). + expect(mailLog.length).toBe(3); + expect(mailLog[0]).toBe("kern"); + expect(mailLog[1]).toBe("sherlock"); + expect(mailLog[2]).toBe("kern"); + }); + + test("slow async sender is timed out, subsequent notifications still delivered", async () => { + setSendTimeoutMs(100); + const config = makeConfig(); + const state = makeState(); + + const errors: string[] = []; + const originalError = console.error; + console.error = (msg: string) => { errors.push(msg); }; + + const mailLog: string[] = []; + let slowResolved = false; + + try { + const sender: MailSender = (to, _body, _agentId) => { + if (to === "sherlock") { + return new Promise((resolve) => { + setTimeout(() => { slowResolved = true; resolve(); }, 500); + }); + } + mailLog.push(to); + }; + + const runner: SyncRunner = (_cmd, args) => { + const endpoint = args[2]; + if (endpoint?.includes("/pulls?")) { + return { + status: 0, + stdout: JSON.stringify([ + { number: 10, title: "PR A", state: "open", merged_at: null, user: { login: "anvil" }, requested_reviewers: [] }, + ]), + stderr: "", + } as ReturnType; + } + if (endpoint?.includes("/reviews")) { + return { status: 0, stdout: "[]", stderr: "" } as ReturnType; + } + return { status: 0, stdout: "[]", stderr: "" } as ReturnType; + }; + + pollOnce(config, state, runner, sender); + + await new Promise((r) => setTimeout(r, 200)); + + expect(errors.some((e) => e.includes("timed out after 100ms"))).toBe(true); + expect(slowResolved).toBe(false); + expect(mailLog).toContain("kern"); + } finally { + console.error = originalError; + setSendTimeoutMs(5_000); + } }); });