fix(backup): bound the command result against the server's 1 MiB cap, not the IPC frame (#3001)

[ToddHebebrand]

Fixes the residual reported in #3001 (re-confirmed on v0.104.0 / a3dc568ec during release QA). The original 64 MB-vs-16 MiB IPC oversize path was closed by #3004/#3037; this is the second, silent loss path underneath it.

Confirmed root cause

The terminal result is refused by the server's 1 MiB cap on the result field — commandResultSchema in apps/api/src/routes/agents/schemas.ts — not by anything on the agent.

The backup helper's stdout is the full BackupJob JSON including one snapshot.files entry per backed-up file (~522 B: source path + backup path + sha256 + modTime). The forwarder (agent/internal/heartbeat/heartbeat.go, case TypeBackupResult) parses that body and assigns it to CommandResult.Result, i.e. the wire result field — not to stdout, which has a 5 MB budget. So:

1048576 / 522  =  ~2008 files

which sits exactly inside the observed bracket: 1,200 files (~0.6 MB) lands, 4,000 files (~2.1 MB) is refused. It is the tightest limit anywhere on the path, and by a wide margin — 16 MiB agent IPC frame, 16 MiB agent WS read limit, 100 MiB ws server maxPayload.

The prime hypothesis in the issue (a WS max-payload) is ruled out. @hono/node-ws creates its WebSocketServer with no maxPayload, so ws applies its 100 MiB default; the ~2 MB frame arrives intact. No proxy layer caps it either. The frame is dropped at the application layer, one safeParse before the backup handler.

Why it was silent at every layer

Three independent causes, all fixed here:

• Agent, producer. The tiered degradation from fix(agent): bound the backup result payload to the IPC frame (#3001) #3004 bounded against ipc.MaxMessageSize - 64 KiB ≈ 15.9 MiB — the next hop, not the destination. A 2 MB result cleared it untouched, so no tier ran and no degradation line was logged.
• Server. agentWs.ts logged the rejection as a generic Invalid message from agent <id>: carrying only the raw Zod issues — no commandId, no size, no message type. Both backup-specific lines (Processing backup result / Dropping backup result) live inside processCommandResult, downstream of the failed parse, so neither could ever print. Grepping for them reads as "the frame vanished".
• Agent, consumer. The server's {type:'error', code:'INVALID_MESSAGE'} reply carries no id, so readPump discarded it under the "not a command" skip. The write had genuinely succeeded, so every send path reported success.

The fix

1. Make the terminal result survive. New leaf package agent/internal/wire holds the server's cap. result_bounds.go now bounds Stdout against wire.CommandResultBudget as well as the IPC frame, so tier 2 empties the per-file index at the limit that actually binds. A 100k-file backup degrades to scalars-plus-snapshot-identity and reports completion; a 1,200-file backup is still sent byte-for-byte intact with its full file index.

The rule the change encodes: bound against the tightest limit anywhere on the path, never the one nearest to hand.

2. Kill the silence.

• Server: a rejected command_result now logs at error with commandId, frame bytes, measured result bytes and the limit, and says plainly that the job will be reaped. Everything else stays a warning. The error reply echoes commandId and messageType.
• Agent: readPump handles inbound error frames and logs them at error with the server's code and details.
• Agent: a generic backstop in SendResult — if the result body still exceeds the cap, it is replaced with a _breezeResultOmitted marker and logged at error, so the terminal status lands regardless. This is deliberately not backup-specific: software inventory, patch scans and filesystem analysis are all result bodies that scale with the endpoint and share the same exposure.

3. The misleading log line. sendBackupResult reported limitBytes=16777216 unconditionally, so a 10 KB payload truncated by the 8 KiB maxResultTextBytes stderr cap was described as overflowing a 16 MiB frame. Attribution is now tracked as the tiers run and reported as limitName + limitBytes.

4. Regression tests at the layer the cause lives in — see below.

fitBackupResultToIPC is renamed fitBackupResultForDelivery: the old name asserted the exact wrong thing about which limit matters, and that assumption is what shipped this bug.

Not raising the cap — needs your sign-off

The result cap stays at 1 MiB. A larger cap only moves the cliff (a 100k-file index is ~52 MB and fits no sane limit), and the agent-side bound is the actual fix. There is a defensible argument for raising it to 5 MB to match stdout/stderr — it would preserve restore browsing for ~5x more endpoints, and the inconsistency is arguably what caused this — but that is a security-surface change, so it is left as a one-line, mirrored decision rather than made here.

Related: the new loud logging will likely reveal other command types already hitting this cap silently. Worth watching the first week of REJECTED command_result lines.

Verification

Command	Result
go test -race ./cmd/breeze-backup/... ./internal/websocket/... ./internal/wire/... ./internal/heartbeat/... ./internal/ipc/...	pass (5 packages)
go vet + gofmt -l on changed packages	clean
GOOS=windows go build ./..., GOOS=linux go build ./...	clean
vitest run schemas.commandResult.test.ts schemas.test.ts schemas.heartbeatTolerance.test.ts commands.test.ts	98 passed
vitest run agentWs.test.ts agentWs.enqueueContract.test.ts agentWs.terminalResultSchema.test.ts	105 passed
tsc --noEmit (apps/api)	clean

New tests:

• TestFourThousandFileRunIsDegradedForTheServerCap — the QA reproduction. Asserts the fixture is a size the old IPC-only bounding accepted, then that it is now degraded, attributed to the server cap, with the file index as the thing dropped.
• TestTwelveHundredFileRunIsSentIntact — the other half: the run that worked must keep its full index. Guards against a fix that degrades everything.
• TestHundredThousandFileRunStillReportsCompletion — requirement 1 as a test, including snapshot identity survival.
• TestStderrOnlyDegradationNamesTheTextCap — requirement 3.
• TestMaxCommandResultBytesMatchesServerSchema (Go) parses the TypeScript declaration; schemas.commandResult.test.ts parses the Go one. The cap is pinned from both directions, so raising one alone reddens CI rather than quietly re-opening this issue.
• boundResultFieldForServer coverage: oversize dropped with status preserved, in-budget untouched, unmarshallable handled, and SendResult proven to bound before enqueue.

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying breeze with    Cloudflare Pages

Latest commit:	16c9132
Status:	✅  Deploy successful!
Preview URL:	https://270e16c2.breeze-9te.pages.dev
Branch Preview URL:	https://toddhebebrand-fix-3001-silen.breeze-9te.pages.dev

View logs

[ToddHebebrand]

Review run: /pr-review-toolkit:review-pr — code-reviewer + silent-failure-hunter (both ran the touched Go and Vitest suites themselves, not just read the diff).

Findings: 1 raised → addressed in 88a32ea49; 0 outstanding.

• code-reviewer (1 finding, Important). The generic backstop boundResultFieldForServer and the SendResult short-circuit compared against the bare wire.MaxCommandResultBytes rather than the headroom-adjusted wire.CommandResultBudget. The backup helper correctly targeted the budget, but the backstop — which is the only guard for every non-backup command type (software inventory, patch scans, filesystem analysis) — ran with no margin for the server's JSON.stringify(JSON.parse(...)) re-measurement. A body landing in the 64 KiB band below the cap could pass agent-side and still be refused on arrival: this issue's exact failure, for the commands with no producer-side bounding. Both comparisons now use the budget; the bare cap is kept only for reporting the server's contract in logs and the omission marker. TestBoundResultFieldUsesTheBudgetNotTheBareCap pins the choice so it cannot drift back.

  Also verified clean by that reviewer: tier-4 convergence under the tighter budget, the limit-attribution capture point, the readPump type == "error" branch placement (no command type is literally "error", and these frames were already being discarded by the msg.ID == "" skip, so it is pure added logging), the len(data) short-circuit's soundness, and both cross-language pin tests actually resolving and reading the other language's file rather than skipping.

• silent-failure-hunter (0 findings). Confirmed end-to-end rather than from comments: every degradation path emits both a note and a log line; agent-side rejections log at slog.LevelError, which is above the warn shipping threshold in config.go:329, so they actually reach the server rather than only a local file on the endpoint; agentMessageSchema is a discriminated union that genuinely routes an oversize result through the new command_result-specific error branch; and backupResultPersistence.ts commits the backup_jobs status update unconditionally ahead of the providerSnapshotId gate, so a degraded body still flips the job out of running — the property that actually matters for the reaper. Empty snapshot.files is truthy, so the stale-row cleanup still runs.

  One low-severity edge it flagged, not fixed here: if tier 4 ever fires for a snapshot that a prior delivery already indexed, lastResortStdout drops the nested snapshot object, so the file-rows cleanup (gated on result.snapshot?.files) is skipped and stale backup_snapshot_files rows survive while hasIndexedFiles reports false. Data-consistency nit, not a stuck job or a lost log — and tier 4 now requires the backup-specific bounding and the generic backstop to both miss. Recording it as known rather than expanding this PR.

Tests: go test -race ./cmd/breeze-backup/... ./internal/websocket/... ./internal/wire/... ./internal/heartbeat/... ./internal/ipc/... pass; go vet + gofmt -l clean; GOOS=windows/GOOS=linux go build ./... clean; vitest run 98 passed (schemas + commands) and 105 passed (agentWs); tsc --noEmit clean. Full CI green on 88a32ea49, including Test Agent, Test Agent (race), Test API, Type Check and all four Integration Tests shards.

Open decision for the maintainer: the result cap stays at 1 MiB. Raising it to 5 MB to match stdout/stderr would preserve restore browsing for ~5x more endpoints, but it is a security-surface change and the agent-side bound is the real fix, so it is left as a deliberate, mirrored one-liner rather than made here. Separately, the new loud rejection logging will likely surface other command types that have been hitting this cap silently — worth watching REJECTED command_result for the first week after deploy.

Status: review-clean, CI green, awaiting maintainer merge. Issue #3001 left open and assigned.

[ToddHebebrand]

Review round 2: code-reviewer + silent-failure-hunter + pr-test-analyzer. Nothing blocking; delivery correctness verified sound end-to-end. Four confirmed findings, all fixed in 16c9132b5.

1 (HIGH, found independently by two reviewers) — the operator-facing warnings still named the wrong limit. Five persisted strings in result_bounds.go hardcoded ipc.MaxMessageSize. These are not logs: they land in result.warning → backup_jobs.errorLog → the UI. Since this PR moves the dominant trigger to the 1 MiB server cap, the headline 4,000-file repro would have told a customer that a 2 MB result exceeded a 16 MiB limit — and oversizeFailureResult prints the actual size right next to the limit it supposedly exceeded, so the contradiction was visible in a single line. This is the same defect the PR fixes in the log line, one layer further out.

The limit is now a deliveryLimit value threaded into every tier, so the warning, the failure text and the structured log cannot tell different stories. It carries the enforced budget separately from the enforcing party's cap — the second half of that finding: degradation trips at 983,040 while the server allows 1,048,576, so reporting the cap as "exceeded" described a 1,000,000-byte payload as overflowing a limit it was under. Warnings name the budget; the log carries both.

Correction to the finding: it is not a free change. Five existing tests asserted the literal "IPC limit" as a proxy for "the tier explained itself" — which had quietly welded the suite to one specific limit being the trigger forever, so it passed on wording that had become false and would have failed on wording that became true. They now assert a shared limitExceededPhrase, with a comment at the constant saying why a test must never pin a bare limit name here.

2 (MEDIUM) — the unencodable-body recovery was dead code. SendResult marshalled the whole result first and returned on error, so boundResultFieldForServer's marshal-error branch was unreachable from the wired path and its unit test asserted a repair that never ran. A NaN in any nested map — metrics-shaped results carry floats routinely — still cost the terminal status. SendResult now substitutes the marker and retries; TestSendResultRecoversFromAnUnencodableBody drives it through the wired path, and a companion test pins that it still errors when there is no body to drop, so the recovery cannot broaden into swallowing unrelated marshal failures.

3 — event-loop guard. commandResultResultByteLength re-serialised a just-parsed object with no size guard, on the one branch guaranteed to see unbounded input. Above 8 MB the frame size is reported instead.

4 — the two nits. frameBytes used String.length (UTF-16 code units), the exact confusion this PR's own test pins; now Buffer.byteLength(data, 'utf8'). The echoed messageType/commandId came off an unvalidated message unbounded; now clamped, along with the issues array.

The rejection branch is extracted into an exported pure buildAgentMessageRejection so the frame shape is testable without a socket.

Tests (the pr-test-analyzer fast-follows). The error-frame contract is now pinned from both sides — the rated-most-valuable gap, since a rename of messageType/commandId on either side silently regresses to this issue's no-trace state. Vitest asserts the emitted shape and checks each key against a json:"…" tag in the Go parser; a Go test asserts the field names against agentWs.ts and that the readPump error branch still precedes the id-less skip — the ordering the whole fix depends on, and the one a refactor would quietly undo. Also added: IPC-frame attribution (the branch where naming the IPC limit is correct, so the assertions can't pass with the server cap hardcoded in the other direction), budget-vs-cap reporting, persisted-warning wording, and the wired NaN path. Both cross-language pin regexes are anchored on the declaration keyword so a doc comment containing NAME = <number> cannot retarget them onto prose.

Verification: go test -race ./... (entire agent module) clean; go vet ./... clean; gofmt clean on all touched packages; GOOS=windows/linux/darwin + native builds clean; 172 Vitest tests across the 7 affected API suites; tsc --noEmit clean. Full CI green on 16c9132b5 — Test Agent, Test Agent (race), Test API, Test Web, Type Check, Lint Agent (Go), and all four Integration Tests shards.

Status: review-clean, CI green, awaiting maintainer merge. The 1 MiB-vs-5 MB cap decision noted in the previous comment is still yours; issue #3001 stays open and assigned.