From 31e6f1ef41d66d4c74d3e98de36c3e40469ba751 Mon Sep 17 00:00:00 2001 From: Quick104 <31828688+Quick104@users.noreply.github.com> Date: Tue, 21 Jul 2026 14:53:47 -0400 Subject: [PATCH] fix(diagnostics): address PR #450 review findings - service.go: reject trailing data after the decoded manifest object. Decoder.More() only reports array/object iteration, so a stray closing delimiter (e.g. {...}}) slipped through where json.Unmarshal used to reject it. Require the stream to reach io.EOF after decoding on both the received and embedded sides; add a regression test. - repo.go: split the list projection from cleanup. reportListSelectSQL keeps the app_build JSONB extraction for the admin list; new reportCleanupSelectSQL omits it so retention/stale batches don't touch each candidate's manifest JSONB just to delete a row. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012e3QjbPo96ed9Mn2qRiUkh --- internal/diagnostics/repo.go | 89 ++++++++++++++++++++-------- internal/diagnostics/service.go | 10 +++- internal/diagnostics/service_test.go | 22 +++++++ 3 files changed, 92 insertions(+), 29 deletions(-) diff --git a/internal/diagnostics/repo.go b/internal/diagnostics/repo.go index fc756d42..91cb077a 100644 --- a/internal/diagnostics/repo.go +++ b/internal/diagnostics/repo.go @@ -315,13 +315,13 @@ func (r *PostgresRepository) RetentionCandidates(ctx context.Context, olderThan return nil, nil } - query := reportListSelectSQL() + ` + query := reportCleanupSelectSQL() + ` WHERE received_at < $1 ORDER BY received_at ASC, id ASC ` args := []any{olderThan} if olderThan.IsZero() { - query = reportListSelectSQL() + ` + query = reportCleanupSelectSQL() + ` WHERE false ` args = nil @@ -342,12 +342,12 @@ func (r *PostgresRepository) RetentionCandidates(ctx context.Context, olderThan ) ranked WHERE retained_bytes > $1 ) - ` + reportListSelectSQL() + ` + ` + reportCleanupSelectSQL() + ` WHERE id IN (SELECT id FROM quota_candidates) ORDER BY received_at ASC, id ASC ` if olderThan.IsZero() { - return r.queryReportSummaries(ctx, byteCapQuery, perUserByteCap) + return r.queryReportCleanup(ctx, byteCapQuery, perUserByteCap) } query = ` @@ -364,14 +364,14 @@ func (r *PostgresRepository) RetentionCandidates(ctx context.Context, olderThan ) ranked WHERE retained_bytes > $2 ) - ` + reportListSelectSQL() + ` + ` + reportCleanupSelectSQL() + ` WHERE received_at < $1 OR id IN (SELECT id FROM quota_candidates) ORDER BY received_at ASC, id ASC ` args = []any{olderThan, perUserByteCap} } - return r.queryReportSummaries(ctx, query, args...) + return r.queryReportCleanup(ctx, query, args...) } func (r *PostgresRepository) StaleReceiving(ctx context.Context, grace time.Duration) ([]Report, error) { @@ -379,7 +379,7 @@ func (r *PostgresRepository) StaleReceiving(ctx context.Context, grace time.Dura grace = time.Hour } cutoff := time.Now().UTC().Add(-grace) - return r.queryReportSummaries(ctx, reportListSelectSQL()+` + return r.queryReportCleanup(ctx, reportCleanupSelectSQL()+` WHERE state IN ('receiving', 'failed') AND received_at < $1 ORDER BY received_at ASC, id ASC @@ -418,9 +418,10 @@ func (r *PostgresRepository) LiveBlobKeys(ctx context.Context, keys []string) (m return live, nil } -// queryReportSummaries runs a reportListSelectSQL-shaped query; the returned -// reports carry no manifest. Only list/cleanup paths use it. -func (r *PostgresRepository) queryReportSummaries(ctx context.Context, query string, args ...any) ([]Report, error) { +// queryReportCleanup runs a reportCleanupSelectSQL-shaped query; the returned +// reports carry no manifest and no app_build. Only the retention and +// stale-cleanup batches use it. +func (r *PostgresRepository) queryReportCleanup(ctx context.Context, query string, args ...any) ([]Report, error) { rows, err := r.pool.Query(ctx, query, args...) if err != nil { return nil, fmt.Errorf("query diagnostic reports: %w", err) @@ -429,7 +430,7 @@ func (r *PostgresRepository) queryReportSummaries(ctx context.Context, query str reports := []Report{} for rows.Next() { - report, err := scanReportSummary(rows) + report, err := scanReportCleanup(rows) if err != nil { return nil, err } @@ -468,8 +469,9 @@ func validateInsertReceivingInput(input InsertReceivingInput) error { // reportSelectSQL is the full projection including the manifest JSONB (up to // MaxManifestBytes per row). Reserve it for single-report detail/download paths; -// list and cleanup queries use reportListSelectSQL so browsing a page or running -// a retention/stale batch doesn't drag every report's manifest through the DB. +// the admin list uses reportListSelectSQL and retention/stale batches use +// reportCleanupSelectSQL so browsing a page or running a cleanup pass doesn't +// drag every report's manifest through the DB. func reportSelectSQL() string { return `SELECT id::text, short_id, user_id, profile_id, state, captured_at, received_at, report_type, platform, app_version, crash_summary, manifest, playback_session_ids, @@ -478,11 +480,12 @@ func reportSelectSQL() string { FROM client_diagnostic_reports` } -// reportListSelectSQL mirrors reportSelectSQL but omits the manifest column. -// Reports scanned with scanReportSummary therefore have a nil Manifest; the -// list and cleanup callers only need summary and blob fields. It still projects -// app_build out of the manifest JSONB (a cheap text extraction) so list rows can -// show the build number without shipping the whole manifest. +// reportListSelectSQL is the admin-list projection: it omits the full manifest +// column but still extracts app_build from the manifest JSONB (a cheap text +// extraction) so list rows can show the build number without shipping the whole +// manifest. Reports scanned with scanReportSummary therefore have a nil +// Manifest. Cleanup batches must not pay for the JSONB extraction, so they use +// reportCleanupSelectSQL instead. func reportListSelectSQL() string { return `SELECT id::text, short_id, user_id, profile_id, state, captured_at, received_at, report_type, platform, app_version, crash_summary, playback_session_ids, @@ -491,6 +494,19 @@ func reportListSelectSQL() string { FROM client_diagnostic_reports` } +// reportCleanupSelectSQL is the retention/stale-cleanup projection. It omits +// both the manifest column and the app_build extraction because cleanup only +// needs id/state/blob fields to delete a row and its object; touching the JSONB +// per candidate would turn a scheduled pass over many large manifests into +// needless manifest I/O. Reports scanned with scanReportCleanup therefore have +// a nil Manifest and an empty AppBuild. +func reportCleanupSelectSQL() string { + return `SELECT id::text, short_id, user_id, profile_id, state, captured_at, received_at, + report_type, platform, app_version, crash_summary, playback_session_ids, + blob_bucket, blob_key, blob_bytes, uncompressed_bytes, blob_sha256 + FROM client_diagnostic_reports` +} + type reportScanner interface { Scan(dest ...any) error } @@ -569,12 +585,12 @@ func scanReport(row reportScanner) (*Report, error) { return &report, nil } -// scanReportSummary scans the reportListSelectSQL projection, which omits the -// manifest; the returned report's Manifest is left nil. -func scanReportSummary(row reportScanner) (*Report, error) { - var report Report - var nulls reportNulls - if err := row.Scan( +// summaryScanTargets returns the scan destinations shared by the admin-list and +// cleanup projections, in column order. The two scanners differ only in whether +// app_build is appended, so keeping the common prefix in one place stops the +// long Scan lists from drifting apart. +func summaryScanTargets(report *Report, nulls *reportNulls) []any { + return []any{ &report.ID, &report.ShortID, &report.UserID, @@ -592,8 +608,29 @@ func scanReportSummary(row reportScanner) (*Report, error) { &nulls.blobBytes, &nulls.uncompressedBytes, &nulls.blobSHA256, - &report.AppBuild, - ); err != nil { + } +} + +// scanReportSummary scans the reportListSelectSQL projection, which omits the +// manifest but includes app_build; the returned report's Manifest is left nil. +func scanReportSummary(row reportScanner) (*Report, error) { + var report Report + var nulls reportNulls + targets := append(summaryScanTargets(&report, &nulls), &report.AppBuild) + if err := row.Scan(targets...); err != nil { + return nil, fmt.Errorf("scan diagnostic report: %w", err) + } + nulls.apply(&report) + return &report, nil +} + +// scanReportCleanup scans the reportCleanupSelectSQL projection, which omits +// both the manifest and app_build; the returned report's Manifest is nil and +// AppBuild is empty. Cleanup only reads id/state/blob fields. +func scanReportCleanup(row reportScanner) (*Report, error) { + var report Report + var nulls reportNulls + if err := row.Scan(summaryScanTargets(&report, &nulls)...); err != nil { return nil, fmt.Errorf("scan diagnostic report: %w", err) } nulls.apply(&report) diff --git a/internal/diagnostics/service.go b/internal/diagnostics/service.go index 976d28e6..60040d91 100644 --- a/internal/diagnostics/service.go +++ b/internal/diagnostics/service.go @@ -559,9 +559,13 @@ func decodeJSONObject(data []byte) (map[string]any, error) { return nil, err } // json.Unmarshal rejects trailing bytes after the value; Decoder.Decode does - // not, so re-assert that strictness — a manifest with trailing junk is not a - // clean single object and must not compare equal. - if dec.More() { + // not, and Decoder.More() misses a stray closing delimiter (e.g. `{...}}`) + // because it only reports iteration inside an array/object. Re-assert that + // strictness by requiring the stream to be exhausted: a second Decode must + // hit io.EOF, so a manifest with any trailing token — junk or an extra + // `}`/`]` — is rejected and cannot compare equal. Trailing whitespace is + // fine; the decoder skips it and still returns io.EOF. + if err := dec.Decode(new(json.RawMessage)); err != io.EOF { return nil, errors.New("unexpected trailing data after JSON object") } return obj, nil diff --git a/internal/diagnostics/service_test.go b/internal/diagnostics/service_test.go index d62c232c..c4b13ccf 100644 --- a/internal/diagnostics/service_test.go +++ b/internal/diagnostics/service_test.go @@ -180,6 +180,28 @@ func TestEmbeddedManifestMatchesPreservesLargeIntegers(t *testing.T) { } } +func TestEmbeddedManifestMatchesRejectsTrailingData(t *testing.T) { + // A stray closing delimiter after an otherwise-matching object must be + // rejected. Decoder.More() returns false here (it only tracks array/object + // iteration), so the decode-to-EOF check is what catches it; without it a + // bundle whose embedded manifest is not a single clean JSON object would + // still compare equal to the accepted part-1 manifest. + received := []byte(`{"log_summary":{"lines":1},"archive":{"bytes":1}}`) + for _, trailer := range []string{"}", "]", "garbage", `{"extra":1}`} { + embedded := []byte(`{"log_summary":{"lines":1}}` + trailer) + if embeddedManifestMatches(received, embedded) { + t.Fatalf("embeddedManifestMatches = true for embedded manifest with trailing %q, want false", trailer) + } + } + + // Trailing whitespace after the object is not trailing data; it must still + // compare equal. + embeddedWithWhitespace := []byte("{\"log_summary\":{\"lines\":1}}\n ") + if !embeddedManifestMatches(received, embeddedWithWhitespace) { + t.Fatal("embeddedManifestMatches = false for embedded manifest with trailing whitespace, want true") + } +} + func TestServiceIngestReturnsStorageErrorForMidStreamPutFailure(t *testing.T) { bundle, info := testDiagnosticsBundle(t) putErr := errors.New("s3: connection reset by peer")