fix(outbound): record queue-write loss and fail every payload on prep abort (#124756)

* fix(outbound): record queue-write loss and fail every payload on prep abort

Two silent-observer gaps in the outbound queue admission path:

1. Best-effort staging/queue-write failures were swallowed with a bare
   'return null'. The send proceeded live-only, so a crash mid-send lost
   the message with zero forensic trace of why the write-ahead row was
   missing. Every sibling degradation on this boundary logs
   (persistQueuedPreSendState, the dispatch-refresh fallback); this one
   now warns with channel/target and the error.

2. Batch preparation failure emitted message_sent for only the first
   payload while audit emitted per-payload terminals, so plugins with
   message_sent hooks saw 1 failure for an N-payload batch. The failure
   path now emits one hook failure per logical payload, matching the
   audit terminals and the recovery sibling's
   queuedTerminalFailureEvents. The unused error.payload pick and its
   OutboundPayloadPreparationError import went with it.

Regressions: queue-write warn asserted in the existing best-effort
fallback test; new multi-payload prep-failure test asserts 2 hook
failures. Both fail pre-fix.

* chore(outbound): keep OutboundPayloadPreparationError module-local

Last external consumer went away with the per-payload emit; the
deadcode:exports gate rejects unused exports.
This commit is contained in:
Peter Steinberger
2026-08-16 12:21:26 -07:00
committed by GitHub
parent 12138d2cee
commit 820bcb78cd
3 changed files with 59 additions and 12 deletions
+1 -1
View File
@@ -21,7 +21,7 @@ import {
} from "./prepared-batch.js";
import { createReplyToDeliveryPolicy } from "./reply-policy.js";
export class OutboundPayloadPreparationError extends Error {
class OutboundPayloadPreparationError extends Error {
readonly sourceIndex: number;
readonly payload: ReplyPayload;
+22 -11
View File
@@ -1,6 +1,7 @@
// Owns durable queue admission and hands stable custody to the execution loop.
import { deriveDurableFinalDeliveryRequirementsForBatch } from "../../channels/message/capabilities.js";
import { createRenderedMessageBatchPlan } from "../../channels/message/rendered-batch.js";
import { createSubsystemLogger } from "../../logging/subsystem.js";
import { getGlobalHookRunner } from "../../plugins/hook-runner-global.js";
import { formatErrorMessage } from "../errors.js";
import { resolveDeferredDeliveryAdmission } from "./deferred-delivery-admission.js";
@@ -8,7 +9,7 @@ import { resolveOutboundDurableFinalDeliverySupport } from "./deliver-channel.js
import type { DeliverOutboundPayloadsParams } from "./deliver-contracts.js";
import { OUTBOUND_DELIVERY_LOG_SCOPE } from "./deliver-log.js";
import { buildPayloadSummary } from "./deliver-payload.js";
import { OutboundPayloadPreparationError, prepareOutboundPayloadBatch } from "./deliver-prepare.js";
import { prepareOutboundPayloadBatch } from "./deliver-prepare.js";
import {
restoreQueuedDeliveryCustody,
stageAndEnqueueOutboundDelivery,
@@ -33,6 +34,8 @@ import { createMessageSentEmitter } from "./message-sent-hook.js";
import { emitOutboundAuditTerminals, uniformOutboundAuditTerminals } from "./outbound-audit.js";
import { acceptedPreparedOutboundEntries } from "./prepared-batch.js";
const log = createSubsystemLogger("outbound/deliver");
export async function runOutboundDelivery(
params: DeliverOutboundPayloadsParams,
): Promise<OutboundDeliveryResult[]> {
@@ -186,10 +189,10 @@ async function runOutboundDeliveryWithQueue(
stablePreparationOwner?.markPrepared();
} catch (error) {
emitPreQueueFailure();
const failedPayload =
error instanceof OutboundPayloadPreparationError ? error.payload : params.payloads[0];
if (failedPayload) {
const summary = buildPayloadSummary(failedPayload);
// Preparation aborts the whole batch, so hooks get one failure per
// logical payload — matching the per-payload audit terminals above and
// the recovery sibling's queuedTerminalFailureEvents.
if (params.payloads.length > 0) {
const { emitMessageSent } = createMessageSentEmitter({
hookRunner: getGlobalHookRunner(),
channel,
@@ -201,11 +204,14 @@ async function runOutboundDeliveryWithQueue(
runId: params.replyPayloadSendingHook?.runId,
logPrefix: OUTBOUND_DELIVERY_LOG_SCOPE,
});
emitMessageSent({
success: false,
content: summary.hookContent ?? summary.text,
error: formatErrorMessage(error),
});
for (const payload of params.payloads) {
const summary = buildPayloadSummary(payload);
emitMessageSent({
success: false,
content: summary.hookContent ?? summary.text,
error: formatErrorMessage(error),
});
}
}
throw error;
}
@@ -271,8 +277,13 @@ async function runOutboundDeliveryWithQueue(
emitPreQueueFailure();
throw err;
}
// Best-effort delivery continues live-only, but a crash mid-send now
// loses the message — record why the write-ahead row is missing.
log.warn(
`outbound queue write failed; continuing without durability (channel=${params.channel} to=${params.to}): ${formatErrorMessage(err)}`,
);
return null;
}); // Best-effort delivery falls back to direct send if staging or the queue write fails.
});
const queueId = queued?.id ?? null;
if (queued?.created && stablePreparationOwner) {
+36
View File
@@ -1758,6 +1758,42 @@ describe("deliverOutboundPayloads", () => {
expect(results[0]?.messageId).toBe("m1");
expect(sendMatrix).toHaveBeenCalled();
// The lost write-ahead durability must leave a recorded reason: a crash
// mid-send now loses the message with no queue row to recover.
const warnLines = logMocks.warn.mock.calls.map((call) => String(call[0]));
const queueWarn = warnLines.find((line) => line.includes("outbound queue write failed"));
expect(queueWarn).toBeDefined();
expect(queueWarn).toContain("channel=matrix");
expect(queueWarn).toContain("queue offline");
});
it("emits one message_sent failure per payload when batch preparation fails", async () => {
hookMocks.runner.hasHooks.mockImplementation(
(hookName?: string) => hookName === "message_sent" || hookName === "reply_payload_sending",
);
hookMocks.runner.runReplyPayloadSending.mockRejectedValueOnce(new Error("modifier exploded"));
const sendMatrix = vi.fn();
await expect(
deliverMatrix({
payloads: [{ text: "first payload" }, { text: "second payload" }],
deps: { matrix: sendMatrix },
replyPayloadSendingHook: {
kind: "final",
channel: "matrix",
context: { channelId: "matrix", conversationId: "!room:example" },
},
}),
).rejects.toThrow("modifier exploded");
expect(sendMatrix).not.toHaveBeenCalled();
// Preparation aborts the whole batch: hooks must see every logical
// payload fail, matching the per-payload audit terminals.
expect(hookMocks.runner.runMessageSent).toHaveBeenCalledTimes(2);
const contents = hookMocks.runner.runMessageSent.mock.calls.map(
(call) => (call[0] as { content?: string }).content,
);
expect(contents).toEqual(["first payload", "second payload"]);
});
it("runs afterCommit hooks after best-effort queue fallback direct sends", async () => {