Skip to content

Commit 5ef9581

Browse files
authored
fix(debug-files): accurate upload count and preserve --require-all hint (#1167)
Follow-up to #1146, addressing two bot findings on that PR. ## 1. `filesUploaded` over-counted (Bugbot/Seer, Low) #1146 surfaces size-dropped files as `error` results so a partial drop exits non-zero. But `doUpload` reported `filesUploaded: results.length`, which then included those `error`/`not_found` entries — over-reporting the number of files actually uploaded in the JSON output. Fixed: compute `failures` before the summary and report `results.length - failures.length`. ## 2. `--require-all` hint dropped after a failure (Bugbot, Medium) When an upload failure or a size-drop returned first, `doUpload` returned before the `--require-all` branch, so the "missing requested debug id(s)" note was omitted from the hint. The exit code was still correctly non-zero, but the actionable feedback was lost. Fixed: build a `requireAllNote` once and append it to whichever hint returns (failure / size-drop), so the missing-id feedback is never swallowed. ## Tests - `filesUploaded excludes failed/dropped results` — mixed ok/error results ⇒ `filesUploaded` counts only the ok one. - `--require-all note is preserved alongside an upload failure` — failure + missing required id ⇒ hint contains both "had failures" and the missing id. `typecheck`, `lint`, and the debug-files upload suite (36 tests) all pass.
1 parent 47cffe8 commit 5ef9581

2 files changed

Lines changed: 75 additions & 6 deletions

File tree

src/commands/debug-files/upload.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,12 @@ async function* doUpload(
450450
) {
451451
const results = await uploadDebugFiles(params);
452452

453+
// Files the server (or the local size gate) rejected come back as
454+
// error/not_found results; they must not be counted as uploaded.
455+
const failures = results.filter(
456+
(r) => r.state === "error" || r.state === "not_found"
457+
);
458+
453459
yield new CommandOutput<DebugFilesUploadResult>({
454460
org: params.org,
455461
project: params.project,
@@ -460,7 +466,7 @@ async function* doUpload(
460466
state: r.state,
461467
detail: r.detail,
462468
})),
463-
filesUploaded: results.length,
469+
filesUploaded: results.length - failures.length,
464470
});
465471

466472
// Scan-time oversized files were dropped before the queue was built, so they
@@ -471,9 +477,13 @@ async function* doUpload(
471477
? ` ${params.oversizedCount} file(s) were skipped for exceeding the maximum file size (${params.maxFileSize} bytes).`
472478
: "";
473479

474-
const failures = results.filter(
475-
(r) => r.state === "error" || r.state === "not_found"
476-
);
480+
// Appended to whichever hint returns first so `--require-all` feedback is
481+
// never lost behind an upload failure or a size-drop (all already exit 1).
482+
const requireAllNote =
483+
params.requireAll && params.missingRequestedIds.length > 0
484+
? ` Missing requested debug id(s): ${params.missingRequestedIds.join(", ")}.`
485+
: "";
486+
477487
if (failures.length > 0) {
478488
setExitCode(1);
479489
const details = failures
@@ -483,14 +493,14 @@ async function* doUpload(
483493
)
484494
.join("; ");
485495
return {
486-
hint: `${failures.length === 1 ? "1 file" : `${failures.length} files`} had failures: ${details}.${scanOversize}`,
496+
hint: `${failures.length === 1 ? "1 file" : `${failures.length} files`} had failures: ${details}.${scanOversize}${requireAllNote}`,
487497
};
488498
}
489499

490500
if (params.oversizedCount > 0) {
491501
setExitCode(1);
492502
return {
493-
hint: `Uploaded ${results.length} debug file(s) to ${params.org}/${params.project}, but ${params.oversizedCount} file(s) were skipped for exceeding the maximum file size (${params.maxFileSize} bytes).`,
503+
hint: `Uploaded ${results.length} debug file(s) to ${params.org}/${params.project}, but ${params.oversizedCount} file(s) were skipped for exceeding the maximum file size (${params.maxFileSize} bytes).${requireAllNote}`,
494504
};
495505
}
496506

test/commands/debug-files/upload.test.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -461,6 +461,65 @@ describe("sentry debug-files upload", () => {
461461
expect(exitCode).toBe(1);
462462
});
463463

464+
test("filesUploaded excludes failed/dropped results", async () => {
465+
process.env.SENTRY_ORG = "test-org";
466+
process.env.SENTRY_PROJECT = "test-project";
467+
await writeBreakpad();
468+
469+
vi.spyOn(debugFilesApi, "uploadDebugFiles").mockResolvedValue([
470+
{
471+
name: "ok.sym",
472+
debugId: KNOWN_DEBUG_ID,
473+
checksum: "a".repeat(40),
474+
state: "ok",
475+
detail: null,
476+
},
477+
{
478+
name: "toobig.sym",
479+
debugId: "22222222-2222-2222-2222-222222222222",
480+
checksum: "",
481+
state: "error",
482+
detail: "Exceeds server maximum file size",
483+
},
484+
]);
485+
486+
const { output } = await runUpload([tempDir, "--json"]);
487+
const parsed = JSON.parse(output);
488+
// Both results are listed, but only the non-failed one counts as uploaded.
489+
expect(parsed.files).toHaveLength(2);
490+
expect(parsed.filesUploaded).toBe(1);
491+
});
492+
493+
test("--require-all note is preserved alongside an upload failure", async () => {
494+
process.env.SENTRY_ORG = "test-org";
495+
process.env.SENTRY_PROJECT = "test-project";
496+
await writeBreakpad();
497+
498+
vi.spyOn(debugFilesApi, "uploadDebugFiles").mockResolvedValue([
499+
{
500+
name: "example.sym",
501+
debugId: KNOWN_DEBUG_ID,
502+
checksum: "a".repeat(40),
503+
state: "error",
504+
detail: "could not process",
505+
},
506+
]);
507+
508+
const { exitCode, output } = await runUpload([
509+
tempDir,
510+
"--id",
511+
KNOWN_DEBUG_ID,
512+
"--id",
513+
"11111111-1111-1111-1111-111111111111",
514+
"--require-all",
515+
]);
516+
expect(exitCode).toBe(1);
517+
// The failure hint must still surface the missing required id — the
518+
// --require-all feedback must not be swallowed by the earlier failure exit.
519+
expect(output).toContain("had failures");
520+
expect(output).toContain("11111111-1111-1111-1111-111111111111");
521+
});
522+
464523
test("--require-all on a real upload fails when an --id is missing", async () => {
465524
process.env.SENTRY_ORG = "test-org";
466525
process.env.SENTRY_PROJECT = "test-project";

0 commit comments

Comments
 (0)