feat(agents): raise the command-result result cap to 5 MB, matching stdout/stderr

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 {

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 {

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)
 	}

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

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