diff --git a/agent/cmd/breeze-backup/result_bounds.go b/agent/cmd/breeze-backup/result_bounds.go index 350c42e28..867eeb152 100644 --- a/agent/cmd/breeze-backup/result_bounds.go +++ b/agent/cmd/breeze-backup/result_bounds.go @@ -29,8 +29,9 @@ import ( // #3001 RESIDUAL (v0.104.0). The above fixed the loud failure and missed a // quiet one, for a reason worth stating plainly: this file bounded against the // NEXT HOP rather than the DESTINATION. The IPC frame is merely the first of -// four limits the result passes; the tightest is the server's 1 MiB cap on the -// command_result `result` field, 16x below the budget used here. So a +// four limits the result passes; the tightest is the server's cap on the +// command_result `result` field, well below the budget used here (1 MiB against +// 15.9 MiB at the time; 5 MB today). So a // 4,000-file run (~2 MB of snapshot index) cleared every check in this file, // was written to the socket successfully, and was refused on arrival — with no // error logged on either side, because the send had genuinely succeeded and the @@ -59,10 +60,15 @@ const ( // 16 MiB IPC frame, but Stdout does not stop at the agent: the forwarder // (internal/heartbeat, case TypeBackupResult) parses it and assigns it to // the `result` field of the WS command_result, where the server caps it at - // wire.MaxCommandResultBytes — 16x tighter. So every tier below was dead - // code for the failure that mattered: a 4,000-file run built a ~2 MB - // snapshot index, sailed through a 15.9 MiB budget, and was refused by the - // server. Nothing logged on either side and the job was reaped as stalled. + // wire.MaxCommandResultBytes — 16x tighter at the time (1 MiB), and still + // over 3x tighter now that the cap has been raised to 5 MB to match the + // sibling `stdout`/`stderr` caps. So every tier below was dead code for the + // failure that mattered: a 4,000-file run built a ~2 MB snapshot index, + // sailed through a 15.9 MiB budget, and was refused by the server. Nothing + // logged on either side and the job was reaped as stalled. + // + // The raise moved the degradation threshold from ~2,000 files to ~9,500; it + // did not remove it. This budget remains the binding one. // // Checked against Stdout alone rather than the whole marshalled result // because that is the field the cap applies to server-side; Stderr rides @@ -304,8 +310,8 @@ func sendBackupResult(conn *ipc.Conn, envelopeID string, result backupipc.Backup "limitName", limit.name, // Both numbers: budgetBytes is the threshold actually crossed, // limitBytes is what the enforcing party allows. Reporting only the - // latter would describe a 1,000,000-byte payload as having exceeded - // the 1,048,576-byte server cap, which is false. + // latter would describe a 4,960,000-byte payload as having exceeded + // the server cap, which is false. "budgetBytes", limit.budget, "limitBytes", limit.cap, "originalStdoutBytes", len(result.Stdout), @@ -373,9 +379,9 @@ func exceededLimit(result backupipc.BackupCommandResult) deliveryLimit { // // BUDGET AND CAP ARE TRACKED SEPARATELY because they are not the same number, // and conflating them produces the very class of false statement this file's -// #3001 fix set out to kill. The server's cap is 1,048,576 but degradation -// trips at serverResultBudget (983,040), so reporting the cap as the thing that -// was "exceeded" tells an operator a 1,000,000-byte payload overflowed a limit +// #3001 fix set out to kill. The server's cap is 5,000,000 but degradation +// trips at serverResultBudget (4,934,464), so reporting the cap as the thing that +// was "exceeded" tells an operator a 4,960,000-byte payload overflowed a limit // it was comfortably under. The warning text names the budget — the threshold // actually crossed — and the structured log carries both. type deliveryLimit struct { diff --git a/agent/cmd/breeze-backup/result_bounds_server_cap_test.go b/agent/cmd/breeze-backup/result_bounds_server_cap_test.go index c12daa70c..8b8714969 100644 --- a/agent/cmd/breeze-backup/result_bounds_server_cap_test.go +++ b/agent/cmd/breeze-backup/result_bounds_server_cap_test.go @@ -11,29 +11,76 @@ import ( "github.com/breeze-rmm/agent/internal/wire" ) -// qaResidualFileCount is the file count from the #3001 residual reproduction on -// v0.104.0: two 4,000-file / 200 MB runs whose terminal result never arrived, -// against a 1,200-file run that landed normally. The loss threshold sat between -// them, which is 1_048_576 / ~522 B-per-entry ≈ 2,008 files. +// The file counts from the #3001 residual reproduction on v0.104.0: two +// 4,000-file / 200 MB runs whose terminal result never arrived, against a +// 1,200-file run that landed normally. The loss threshold sat between them, +// which under the original 1 MiB cap was 1_048_576 / ~522 B-per-entry ≈ 2,008 +// files. +// +// The cap is now 5,000,000, so BOTH of those runs deliver their file index +// intact and the degradation threshold has moved out to ~9,500 files — +// oversizeIndexFileCount is what exercises it. The 4,000-file fixture stays in +// the suite precisely because it used to fail: it is the regression that proves +// the raise reached the endpoints the QA reproduction was about. const ( qaResidualFileCount = 4000 qaPassingFileCount = 1200 + + // oversizeIndexFileCount marshals to roughly 6.0 MB — buildLargeRunJob's + // entries encode to ~375 B each, shorter than the ~522 B of the field + // report — putting it over the ~4.93 MB server budget and comfortably under + // the ~15.9 MiB IPC budget. That band is the one the server cap owns, and + // every test using this count asserts it, so a cap change fails loudly + // instead of quietly retargeting these tests at the IPC limit. + oversizeIndexFileCount = 16000 ) -// TestFourThousandFileRunIsDegradedForTheServerCap is the residual #3001 -// regression, and the one that would have caught it. +// TestFourThousandFileRunSendsItsIndexIntact is the raise, stated as the +// regression it is meant to be. // -// The previous bounding stopped at the 15.9 MiB IPC budget, so a ~2 MB result -// "fitted" and was sent verbatim — then refused by the server's 1 MiB `result` -// cap with no log on either side, and the job was reaped as stalled 15 minutes -// after a backup that had SUCCEEDED. The fixture is deliberately the size that -// passed the old check and failed the real one. -func TestFourThousandFileRunIsDegradedForTheServerCap(t *testing.T) { +// This exact fixture is #3001's residual reproduction. Under the 1 MiB cap it +// was refused by the server with no log on either side, and the job was reaped +// as stalled 15 minutes after a backup that had SUCCEEDED; after the first fix +// it was degraded to a terminal status with no file index. It must now arrive +// whole — that is what raising the cap to match `stdout` bought, and a +// regression to either earlier behaviour is invisible without this test. +func TestFourThousandFileRunSendsItsIndexIntact(t *testing.T) { result := mustRunResult(t, buildLargeRunJob(qaResidualFileCount, 3)) + if len(result.Stdout) <= 1048576 { + t.Fatalf("fixture stdout is %d bytes, under the ORIGINAL 1 MiB cap — it no longer represents "+ + "the payload that reproduced #3001 and proves nothing about the raise", len(result.Stdout)) + } + + fitted, notes, limit := fitBackupResult(result) + + if notes != "" { + t.Fatalf("the 4,000-file QA reproduction was degraded (%q); it fits the raised cap and must "+ + "now deliver its file index intact", notes) + } + if limit.fired() { + t.Fatalf("no limit should have fired for a %d-byte body under the %d byte budget, got %q", + len(result.Stdout), serverResultBudget, limit.name) + } + if fitted.Stdout != result.Stdout { + t.Fatal("stdout was modified for an in-budget result") + } + assertFileIndexEntries(t, fitted.Stdout, qaResidualFileCount) + assertTerminalStatusSurvives(t, fitted, result.CommandID) +} + +// TestOversizeIndexIsDegradedForTheServerCap keeps the degradation path pinned +// now that the QA fixture no longer reaches it. +// +// Without this the raise would have silently deleted coverage of the entire +// reason the tiers exist: every remaining fixture would either fit outright or +// be so large that the IPC frame could be blamed instead. +func TestOversizeIndexIsDegradedForTheServerCap(t *testing.T) { + result := mustRunResult(t, buildLargeRunJob(oversizeIndexFileCount, 3)) + if len(result.Stdout) > resultPayloadBudget { t.Fatalf("fixture stdout is %d bytes, over the IPC budget %d — this test must exercise a "+ - "payload the OLD (IPC-only) bounding considered acceptable", len(result.Stdout), resultPayloadBudget) + "payload only the SERVER cap rejects", len(result.Stdout), resultPayloadBudget) } if len(result.Stdout) <= wire.MaxCommandResultBytes { t.Fatalf("fixture stdout is only %d bytes, under the server cap %d — the test would prove nothing", @@ -65,7 +112,7 @@ func TestFourThousandFileRunIsDegradedForTheServerCap(t *testing.T) { // The warning PERSISTED to backup_jobs.errorLog must name the same limit // the log line does. Hardcoding the IPC limit here told the customer their - // 2 MB result had overflowed a 16 MiB frame. + // oversize result had overflowed a 16 MiB frame. assertWarningNamesLimit(t, fitted.Stdout, limit) assertTerminalStatusSurvives(t, fitted, result.CommandID) @@ -90,16 +137,7 @@ func TestTwelveHundredFileRunIsSentIntact(t *testing.T) { t.Fatal("stdout was modified for an in-budget result") } - var job map[string]any - if err := json.Unmarshal([]byte(fitted.Stdout), &job); err != nil { - t.Fatalf("unmarshal fitted stdout: %v", err) - } - snap, _ := job["snapshot"].(map[string]any) - files, _ := snap["files"].([]any) - if len(files) != qaPassingFileCount { - t.Fatalf("file index has %d entries, want the full %d — restore browsing must survive here", - len(files), qaPassingFileCount) - } + assertFileIndexEntries(t, fitted.Stdout, qaPassingFileCount) } // TestHundredThousandFileRunStillReportsCompletion is fix requirement 1 stated @@ -176,7 +214,7 @@ func TestStderrOnlyDegradationNamesTheTextCap(t *testing.T) { // wrapper honest, so the existing suite that calls it keeps testing the code // the sender actually runs. func TestDeliveryWrapperMatchesAttributedForm(t *testing.T) { - result := mustRunResult(t, buildLargeRunJob(qaResidualFileCount, 3)) + result := mustRunResult(t, buildLargeRunJob(oversizeIndexFileCount, 3)) wrappedResult, wrappedNotes := fitBackupResultForDelivery(result) fullResult, fullNotes, _ := fitBackupResult(result) @@ -195,13 +233,13 @@ func TestDeliveryWrapperMatchesAttributedForm(t *testing.T) { // The structured log line stays on the endpoint; the `warning` these tiers // write is persisted to backup_jobs.errorLog and rendered in the UI. All five // tier warnings used to hardcode ipc.MaxMessageSize, so after this PR shifted -// the dominant trigger to the 1 MiB server cap, the headline repro would have +// the dominant trigger to the server result cap, the headline repro would have // told a customer that a 2 MB result exceeded a 16 MiB limit — false on its // face, and self-contradictory in oversizeFailureResult, which prints the // actual size right next to the limit it supposedly exceeded. func TestPersistedWarningNamesTheLimitThatFired(t *testing.T) { t.Run("snapshot index dropped for the server cap", func(t *testing.T) { - result := mustRunResult(t, buildLargeRunJob(qaResidualFileCount, 3)) + result := mustRunResult(t, buildLargeRunJob(oversizeIndexFileCount, 3)) fitted, _, limit := fitBackupResult(result) warning := warningFromStdout(t, fitted.Stdout) @@ -223,8 +261,10 @@ func TestPersistedWarningNamesTheLimitThatFired(t *testing.T) { // backup_list's array body: it cannot be summarised, so tier 2 replaces // it with an explicit failure. This is the site where the wrong limit // was most visibly self-contradictory. - big := make([]string, 0, 40000) - for i := 0; i < 40000; i++ { + // Sized past the server budget: at ~43 B per element this is ~6.5 MB. + const arrayElements = 150000 + big := make([]string, 0, arrayElements) + for i := 0; i < arrayElements; i++ { big = append(big, strings.Repeat("s", 40)) } encoded, err := json.Marshal(big) @@ -282,7 +322,7 @@ func TestIPCFrameAttributionForOversizeStderr(t *testing.T) { // TestDeliveryLimitReportsTheThresholdActuallyCrossed guards the budget/cap // split. Reporting the cap as the thing "exceeded" would describe a payload of -// 1,000,000 bytes — over the 983,040 budget, under the 1,048,576 cap — as +// 4,960,000 bytes — over the 4,934,464 budget, under the 5,000,000 cap — as // having overflowed a limit it never reached. func TestDeliveryLimitReportsTheThresholdActuallyCrossed(t *testing.T) { between := serverResultBudget + (wire.MaxCommandResultBytes-serverResultBudget)/2 @@ -307,6 +347,24 @@ func TestDeliveryLimitReportsTheThresholdActuallyCrossed(t *testing.T) { } } +// assertFileIndexEntries checks how many snapshot file entries survived — the +// difference between "this snapshot is browsable" and "the index was dropped +// to fit". Zero is a legitimate degraded outcome; the WRONG non-zero count +// would be a silently truncated index, which the server cannot distinguish +// from a complete one. +func assertFileIndexEntries(t *testing.T, stdout string, want int) { + t.Helper() + var job map[string]any + if err := json.Unmarshal([]byte(stdout), &job); err != nil { + t.Fatalf("unmarshal fitted stdout: %v", err) + } + snap, _ := job["snapshot"].(map[string]any) + files, _ := snap["files"].([]any) + if len(files) != want { + t.Fatalf("file index has %d entries, want %d — restore browsing depends on this", len(files), want) + } +} + // warningFromStdout extracts the `warning` field the tiers write into the run // body — the string the server persists to backup_jobs.errorLog. func warningFromStdout(t *testing.T, stdout string) string { diff --git a/agent/internal/websocket/result_server_cap_test.go b/agent/internal/websocket/result_server_cap_test.go index 87ba13ef0..c6787f4f4 100644 --- a/agent/internal/websocket/result_server_cap_test.go +++ b/agent/internal/websocket/result_server_cap_test.go @@ -9,9 +9,17 @@ import ( "github.com/breeze-rmm/agent/internal/wire" ) -// oversizeResultBody builds a `result` body that marshals past the server's -// cap, shaped like the per-file arrays that actually cause this (a snapshot -// index, a software inventory, a filesystem walk). +// oversizeEntryCount is the entry count used wherever a test needs a body that +// marshals past the server's cap. Each entry encodes to roughly 340 bytes, so +// this lands near 6.8 MB against a ~4.93 MB budget — comfortably over without +// being so large it slows the suite under -race. Every test that uses it +// asserts the resulting size, so a change to either the cap or the entry shape +// fails loudly here rather than quietly making a test vacuous. +const oversizeEntryCount = 20000 + +// oversizeResultBody builds a `result` body of `entries` per-file records, +// shaped like the arrays that actually cause this (a snapshot index, a software +// inventory, a filesystem walk). func oversizeResultBody(entries int) map[string]any { files := make([]map[string]any, 0, entries) for i := 0; i < entries; i++ { @@ -35,7 +43,7 @@ func oversizeResultBody(entries int) map[string]any { // status. Before this the whole message was refused server-side and the job was // reaped as stalled. func TestBoundResultFieldDropsOversizeBodyAndKeepsTerminalStatus(t *testing.T) { - body := oversizeResultBody(4000) + body := oversizeResultBody(oversizeEntryCount) encoded, err := json.Marshal(body) if err != nil { t.Fatalf("marshal fixture: %v", err) @@ -99,7 +107,10 @@ func TestBoundResultFieldLeavesInBudgetResultsAlone(t *testing.T) { }{ {"nil body", nil}, {"small object", map[string]any{"filesBackedUp": 1200, "status": "completed"}}, - {"just under the budget", oversizeResultBody(1200)}, + // Sized off the budget rather than an entry count so it stays genuinely + // "just under" whatever the cap becomes. + {"just under the budget", map[string]any{"p": strings.Repeat("x", wire.CommandResultBudget-1000)}}, + {"a realistic in-budget file index", oversizeResultBody(1200)}, } { t.Run(tc.name, func(t *testing.T) { if tc.body != nil { @@ -259,7 +270,7 @@ func TestSendResultBoundsOversizeBodyBeforeEnqueue(t *testing.T) { Type: "command_result", CommandID: "cmd-3001", Status: "completed", - Result: oversizeResultBody(4000), + Result: oversizeResultBody(oversizeEntryCount), }); err != nil { t.Fatalf("SendResult: %v", err) } diff --git a/agent/internal/websocket/server_error_frame_test.go b/agent/internal/websocket/server_error_frame_test.go index 2b5d2c147..ce8a8d33d 100644 --- a/agent/internal/websocket/server_error_frame_test.go +++ b/agent/internal/websocket/server_error_frame_test.go @@ -20,7 +20,7 @@ const canonicalRejectionFrame = `{ "message": "Invalid message format", "messageType": "command_result", "commandId": "cmd-7", - "details": [{"code":"custom","path":["result"],"message":"Command result payload exceeds the 1048576-byte ` + "`result`" + ` limit"}] + "details": [{"code":"custom","path":["result"],"message":"Command result payload exceeds the 5000000-byte ` + "`result`" + ` limit"}] }` // TestServerErrorFrameParsesEveryAttributionField is the agent half of the diff --git a/agent/internal/wire/limits.go b/agent/internal/wire/limits.go index 52393bf71..dc2412b52 100644 --- a/agent/internal/wire/limits.go +++ b/agent/internal/wire/limits.go @@ -22,22 +22,36 @@ package wire // pinned by schemas.commandResult.test.ts. Both assert the literal, so raising // one alone reddens CI rather than quietly reintroducing #3001. // -// WHY THIS IS THE LIMIT THAT MATTERS. It is the tightest bound anywhere on the -// result path, and by a wide margin: +// WHY THIS IS THE LIMIT THAT MATTERS. It is still the tightest bound anywhere +// on the result path, even after the raise below: // -// 1 MiB this — server-side Zod refine on `result` +// 5 MB this — server-side Zod refine on `result` // 16 MiB ipc.MaxMessageSize (helper→agent frame) // 16 MiB websocket.maxMessageSize (agent's INBOUND read limit) // 100 MiB the `ws` server's default maxPayload // // #3001: the backup helper's tiered degradation bounded against the 16 MiB IPC // frame — the next hop, not the binding one — so it stayed inert while every -// backup over ~2,000 files was rejected by the server 16x below that budget. +// backup over ~2,000 files was rejected by the server far below that budget. // The rejection logged as a generic invalid-message server-side and as nothing // at all agent-side, so a backup that had SUCCEEDED was reported to the user as // stalled by the stale-backup reaper. Bound against the tightest limit in the // whole chain, never merely the next one. -const MaxCommandResultBytes = 1024 * 1024 +// +// THE VALUE IS 5_000_000, NOT 5 * 1024 * 1024, and the difference is the point. +// It is set to equal the `stdout`/`stderr` caps in the same schema exactly. +// Those three fields travel in one message from one authenticated agent, and +// the 1 MiB/5 MB split between them was itself a cause of #3001: the backup +// forwarder put its run body in `result` rather than `stdout` and inherited a +// limit five times tighter than the one the payload was sized against. Choosing +// 5 * 1024 * 1024 here would leave `result` 242,880 bytes looser than `stdout` +// and re-create a smaller version of exactly that mismatch. +// +// At ~522 B per snapshot file entry this carries a browsable restore index to +// roughly 9,500 files, up from ~2,000. Past that the helper's tiers still drop +// the index and land the terminal status — the raise widens the good path, it +// does not replace the degradation machinery. +const MaxCommandResultBytes = 5_000_000 // CommandResultHeadroom is subtracted from MaxCommandResultBytes to get the // budget agent-side code should actually target. @@ -61,6 +75,13 @@ const MaxCommandResultBytes = 1024 * 1024 // the budget. Overshooting the margin costs a degraded body in a narrow band // below the cap; undershooting it costs the whole message, which is the failure // this package exists to prevent. +// +// Left at 64 KiB when the cap rose from 1 MiB to 5 MB. It is an absolute +// allowance for encoding differences, not a percentage of the payload: the +// re-encoding deltas it covers (HTML escaping, number formatting, string +// quoting) scale with the number of affected characters, and 64 KiB already +// covers a pathological body several times over. Scaling it with the cap would +// have quietly widened it to 320 KiB for no reason. const CommandResultHeadroom = 64 * 1024 // CommandResultBudget is the size agent-side code should keep its encoded diff --git a/agent/internal/wire/limits_test.go b/agent/internal/wire/limits_test.go index 4b6a14afb..74fe6d80a 100644 --- a/agent/internal/wire/limits_test.go +++ b/agent/internal/wire/limits_test.go @@ -21,10 +21,12 @@ const serverSchemaPath = "../../../apps/api/src/routes/agents/schemas.ts" // the agent, which leaves the agent bounding to a stale, tighter budget (merely // wasteful) or, if lowered, to a stale looser one, which is exactly the #3001 // failure: the agent believed it had 16 MiB of room while the server allowed -// 1 MiB, and every backup over ~2,000 files was silently rejected. +// far less, and every backup over ~2,000 files was silently rejected. func TestMaxCommandResultBytesMatchesServerSchema(t *testing.T) { - if MaxCommandResultBytes != 1048576 { - t.Fatalf("MaxCommandResultBytes = %d, want 1048576; if the server cap really moved, update "+ + // The literal, spelled out: this must equal the `stdout`/`stderr` caps in + // the server schema (5_000_000), not a rounded 5 * 1024 * 1024. + if MaxCommandResultBytes != 5000000 { + t.Fatalf("MaxCommandResultBytes = %d, want 5000000; if the server cap really moved, update "+ "apps/api/src/routes/agents/schemas.ts MAX_COMMAND_RESULT_BYTES in the SAME commit", MaxCommandResultBytes) } diff --git a/apps/api/src/routes/agents/schemas.commandResult.test.ts b/apps/api/src/routes/agents/schemas.commandResult.test.ts index 5220e79f4..fe263e31e 100644 --- a/apps/api/src/routes/agents/schemas.commandResult.test.ts +++ b/apps/api/src/routes/agents/schemas.commandResult.test.ts @@ -29,14 +29,40 @@ function resultOfExactBytes(bytes: number) { return { p: 'x'.repeat(bytes - structural) }; } +/** A backup run body carrying `count` snapshot file-index entries (~522 B each). */ +function snapshotBody(count: number) { + const files = Array.from({ length: count }, (_, i) => ({ + sourcePath: `C:\\Users\\jdoe\\AppData\\Local\\Cache\\Cache_Data\\f_${i}${'x'.repeat(60)}`, + backupPath: `snapshot-1/C_/Users/jdoe/AppData/Local/Cache/Cache_Data/f_${i}${'y'.repeat(60)}`, + checksum: 'a'.repeat(64), + size: 4096 + i, + modTime: '2026-07-14T09:12:33Z', + })); + return { id: 'job-1', status: 'completed', snapshot: { id: 'snapshot-1', files } }; +} + describe('command_result `result` size cap (#3001)', () => { it('pins MAX_COMMAND_RESULT_BYTES to the value the agent mirrors', () => { // The agent bounds its payloads against its own copy of this number. If // this changes without agent/internal/wire/limits.go changing in the same // commit, agents build results to a budget the server no longer honours — // which is exactly how #3001 stayed invisible: the agent bounded against a - // 16 MiB IPC frame while the server enforced 1 MiB here. - expect(MAX_COMMAND_RESULT_BYTES).toBe(1_048_576); + // 16 MiB IPC frame while the server enforced far less here. + expect(MAX_COMMAND_RESULT_BYTES).toBe(5_000_000); + }); + + it('equals the stdout/stderr caps in the same schema', () => { + // The invariant the raise from 1 MiB established, asserted so it cannot + // erode back. `result`, `stdout` and `stderr` travel in one message from + // one authenticated agent; the old split between them is what let the + // backup forwarder inherit a five-times-tighter limit than anyone had + // reasoned about (#3001). Derived from the schema rather than hardcoded so + // moving any one of the three without the others fails here. + const stdoutMax = 'x'.repeat(MAX_COMMAND_RESULT_BYTES); + expect(commandResultSchema.safeParse(validResult({ stdout: stdoutMax })).success).toBe(true); + expect(commandResultSchema.safeParse(validResult({ stdout: stdoutMax + 'x' })).success).toBe(false); + expect(commandResultSchema.safeParse(validResult({ stderr: stdoutMax })).success).toBe(true); + expect(commandResultSchema.safeParse(validResult({ stderr: stdoutMax + 'x' })).success).toBe(false); }); it('matches the Go mirror in agent/internal/wire/limits.go', () => { @@ -44,17 +70,21 @@ describe('command_result `result` size cap (#3001)', () => { // Anchored on `const` so a doc comment containing // "MaxCommandResultBytes = " cannot retarget the pin onto prose, // letting the real constant drift while this test passes against a sentence. - const declaration = source.match(/const\s+MaxCommandResultBytes\s*=\s*([0-9*\s]+)/)?.[1]; + const declaration = source.match(/const\s+MaxCommandResultBytes\s*=\s*([0-9_*\s]+)/)?.[1]; if (declaration === undefined) { throw new Error(`no \`const MaxCommandResultBytes = \` declaration found in ${GO_LIMITS_PATH}`); } - // The Go side writes it as an arithmetic literal (1024 * 1024). + // Go may write it as a plain literal with digit separators (5_000_000) or + // as an arithmetic one (1024 * 1024); accept both so the pin survives a + // reformat of the constant it guards. const goValue = declaration .split('*') - .map((part) => Number(part.trim())) + .map((part) => Number(part.trim().replace(/_/g, ''))) .reduce((a, b) => a * b, 1); + expect(Number.isFinite(goValue)).toBe(true); + expect(goValue).toBe(MAX_COMMAND_RESULT_BYTES); }); @@ -77,26 +107,27 @@ describe('command_result `result` size cap (#3001)', () => { ); }); - it('rejects a ~2 MB snapshot file index — the shape that reproduced #3001', () => { - // ~4,000 snapshot entries at ~522 B each is what a 4,000-file backup - // produced; the terminal result was refused here and the job was reaped as - // stalled. A 1,200-file run (~0.6 MB) passed, which is why the loss - // threshold sat between the two. - const files = Array.from({ length: 4000 }, (_, i) => ({ - sourcePath: `C:\\Users\\jdoe\\AppData\\Local\\Cache\\Cache_Data\\f_${i}${'x'.repeat(60)}`, - backupPath: `snapshot-1/C_/Users/jdoe/AppData/Local/Cache/Cache_Data/f_${i}${'y'.repeat(60)}`, - checksum: 'a'.repeat(64), - size: 4096 + i, - modTime: '2026-07-14T09:12:33Z', - })); - const body = { id: 'job-1', status: 'completed', snapshot: { id: 'snapshot-1', files } }; - - expect(commandResultResultByteLength(body)!).toBeGreaterThan(MAX_COMMAND_RESULT_BYTES); - expect(commandResultSchema.safeParse(validResult({ result: body })).success).toBe(false); - - // ...and the degraded form the agent now sends instead is accepted, with - // the terminal status intact. - const degraded = { ...body, snapshot: { id: 'snapshot-1', files: [] } }; + it('now ACCEPTS the ~2 MB index that reproduced #3001, and still rejects a genuinely oversize one', () => { + // The whole point of the raise. ~4,000 entries at ~522 B each is what the + // v0.104.0 QA reproduction produced; under the old 1 MiB cap this was + // refused and the job was reaped as stalled. It now lands WITH its file + // index, so those endpoints keep restore browsing instead of degrading. + const under = snapshotBody(4000); + expect(commandResultResultByteLength(under)!).toBeGreaterThan(1_048_576); + expect(commandResultResultByteLength(under)!).toBeLessThan(MAX_COMMAND_RESULT_BYTES); + const accepted = commandResultSchema.safeParse(validResult({ result: under })); + expect(accepted.success).toBe(true); + expect(accepted.success && accepted.data.status).toBe('completed'); + + // The cap still exists: a large enough index is refused, which is what the + // agent's tiered degradation is there to prevent from ever being sent. + const over = snapshotBody(20000); + expect(commandResultResultByteLength(over)!).toBeGreaterThan(MAX_COMMAND_RESULT_BYTES); + expect(commandResultSchema.safeParse(validResult({ result: over })).success).toBe(false); + + // ...and the degraded form the agent sends instead is accepted, with the + // terminal status intact. + const degraded = { ...over, snapshot: { id: 'snapshot-1', files: [] } }; const parsed = commandResultSchema.safeParse(validResult({ result: degraded })); expect(parsed.success).toBe(true); expect(parsed.success && parsed.data.status).toBe('completed'); @@ -107,14 +138,18 @@ describe('command_result `result` size cap (#3001)', () => { expect(commandResultSchema.safeParse(validResult({ result: null })).success).toBe(true); }); - it('does not apply the result cap to stdout, which has its own 5 MB budget', () => { - // Guards against a fix that "unifies" the caps by tightening stdout: script - // output legitimately runs to megabytes and must not start being rejected. + it('applies the result cap to `result` only, not to the sibling text fields', () => { + // The caps are equal by design, but they must stay INDEPENDENT checks: a + // message may legitimately carry a full-size stdout AND a full-size result. + // A refactor that folded them into one shared budget would start rejecting + // that, so the combination is asserted directly. + const full = 'x'.repeat(MAX_COMMAND_RESULT_BYTES); const parsed = commandResultSchema.safeParse( - validResult({ stdout: 'x'.repeat(MAX_COMMAND_RESULT_BYTES + 1) }) + validResult({ stdout: full, result: resultOfExactBytes(MAX_COMMAND_RESULT_BYTES) }) ); expect(parsed.success).toBe(true); }); + }); describe('commandResultResultByteLength', () => { diff --git a/apps/api/src/routes/agents/schemas.ts b/apps/api/src/routes/agents/schemas.ts index d6e8df276..f34332fa1 100644 --- a/apps/api/src/routes/agents/schemas.ts +++ b/apps/api/src/routes/agents/schemas.ts @@ -290,22 +290,34 @@ export const processSampleSchema = z.object({ * `snapshot.files` entry per backed-up file (~522 B each), the agent put that * body in `result`, and every backup over ~2,000 files was rejected here. The * agent's own tiered degradation was bounding against the 16 MiB IPC frame — - * 16x too loose — so it never fired, nothing logged on either side, and the job + * far too loose — so it never fired, nothing logged on either side, and the job * sat `running` until the stale-backup reaper falsely failed a backup that had * in fact succeeded. * + * IT EQUALS THE `stdout`/`stderr` CAPS IN THIS SAME SCHEMA, DELIBERATELY. All + * three fields ride one message from one authenticated agent, and the old + * 1 MiB-vs-5 MB split between them was a cause of #3001 rather than an + * incidental detail: the backup forwarder assigns its run body to `result` + * instead of `stdout` (agent/internal/heartbeat/heartbeat.go, case + * TypeBackupResult), so the payload silently inherited a limit five times + * tighter than the one anyone reasoned about. Keeping the three equal removes + * the trap. Do not "round" this to 5 * 1024 * 1024 — that would put `result` + * 242,880 bytes above `stdout` and re-create a smaller version of the same + * mismatch. + * + * Raising it does NOT replace the agent's degradation machinery, which is still + * what guarantees a terminal status: a 100k-file index is ~52 MB and fits no + * sane cap. It widens the band in which a snapshot keeps a browsable file index + * from ~2,000 files to ~9,500. + * * MIRRORED IN GO as `wire.MaxCommandResultBytes` * (agent/internal/wire/limits.go). The two are pinned equal by * `schemas.commandResult.test.ts` here and `TestMaxCommandResultBytesMatchesServerSchema` - * there — both assert the literal 1048576, so raising one alone reddens CI on - * both sides rather than silently re-opening #3001. - * - * Deliberately NOT raised to match the 5 MB `stdout`/`stderr` caps: a larger cap - * only moves the cliff (a 100k-file snapshot index is ~52 MB and fits no sane - * cap), and the agent-side bound is the actual fix. Changing this value is a - * one-line, reversible decision — but it must be changed on BOTH sides. + * there — both assert the literal 5000000 AND cross-parse the other language's + * declaration, so raising one alone reddens CI on both sides rather than + * silently re-opening #3001. */ -export const MAX_COMMAND_RESULT_BYTES = 1_048_576; +export const MAX_COMMAND_RESULT_BYTES = 5_000_000; /** * commandResultResultByteLength returns the encoded size the `result` field