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:	88a32ea
Status:	✅  Deploy successful!
Preview URL:	https://7dd6a3ef.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.