From 8a1cb94631ce8f117ffc33155e4d635af6fd7d5e Mon Sep 17 00:00:00 2001 From: Dhruv Khara <66153057+BeLazy167@users.noreply.github.com> Date: Thu, 3 Sep 2026 19:53:05 -0500 Subject: [PATCH 1/7] fix: add /readyz Fly check so a dead DB fails health /healthz returns a constant 200 and never touches the store. It was the only Fly check, so argus-db filling its volume and crash-looping for three days still reported the app healthy while every data request failed. Add a second check on /readyz, which pings the store pool. Longer grace period covers pool open lagging the listener on boot. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GJehDM4aXz1AkJrTaX2mSL --- backend/fly.toml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/backend/fly.toml b/backend/fly.toml index c83ca8cd..15ff0877 100644 --- a/backend/fly.toml +++ b/backend/fly.toml @@ -46,6 +46,18 @@ swap_size_mb = 512 size = 'shared-cpu-1x' memory = '1024mb' +# Two checks, two questions. `health` asks "is the process alive?" and hits +# /healthz, which returns a constant 200 and touches nothing. `ready` asks "can +# this machine actually serve?" and hits /readyz, which pings the store pool. +# +# Only /healthz was wired up until 2026-09-03. That day argus-db filled its +# volume and Postgres crash-looped for three days; /healthz kept returning +# {"status":"ok"} the whole time, so Fly reported the app healthy while every +# data request failed. A dead database must turn a check red. +# +# `ready` carries the longer grace_period because a booting machine opens the +# pool after the listener, and the longer interval because each probe costs a +# round trip to Postgres. [checks] [checks.health] port = 8080 @@ -53,3 +65,11 @@ swap_size_mb = 512 interval = '15s' timeout = '2s' path = '/healthz' + + [checks.ready] + port = 8080 + type = 'http' + interval = '30s' + timeout = '5s' + grace_period = '20s' + path = '/readyz' From d8263f41ff490a62293e52c493ce63504912eccb Mon Sep 17 00:00:00 2001 From: Dhruv Khara <66153057+BeLazy167@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:32:45 -0500 Subject: [PATCH 2/7] perf: skip the graph publish swap when the generation is unchanged Publishing a full generation deletes every code_node for the repo and re-inserts the staged snapshot. That swap is the visibility boundary, so it cannot be incremental. But when the staged generation reproduces the live graph exactly, it rewrites every row to arrive where it already was. Between Aug 12 and Sep 3 that churn wrote 10.2M rows into the pggraph CDC log (graph._sync_log, 5.9GB) and filled the argus-db volume, crash-looping Postgres for three days. Fingerprint the staged payloads, compare against the published generation's hash, and skip the swap on a match. Bookkeeping still advances via the extracted finishGraphGenerationPublish, so the repo records the new head and clears its refresh request. The skip also requires that nothing wrote to code_nodes or code_edges since that publish: the PR indexer mutates both between full publishes, and a swap removes the rows it added. Every uncertain case falls through to the swap. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GJehDM4aXz1AkJrTaX2mSL --- backend/internal/graph/fullindex.go | 79 +++++++++++++++++ backend/internal/graph/fullindex_pg_test.go | 85 +++++++++++++++++++ ...087_graph_generation_content_hash.down.sql | 1 + .../087_graph_generation_content_hash.up.sql | 12 +++ 4 files changed, 177 insertions(+) create mode 100644 backend/internal/store/migrations/087_graph_generation_content_hash.down.sql create mode 100644 backend/internal/store/migrations/087_graph_generation_content_hash.up.sql diff --git a/backend/internal/graph/fullindex.go b/backend/internal/graph/fullindex.go index 9365a9d8..cfb32c36 100644 --- a/backend/internal/graph/fullindex.go +++ b/backend/internal/graph/fullindex.go @@ -498,6 +498,65 @@ func failGraphPublishLimit(ctx context.Context, st *store.Store, tx pgx.Tx, repo return limitErr } +// stageGenerationIsNoOp reports whether publishing this generation would leave +// code_nodes and code_edges exactly as they already are, making the +// delete-and-reinsert swap pure churn. It also records this generation's +// content hash, so the next publish has something to compare against. +// +// Two conditions must hold together: +// +// - The staged payloads hash identical to the generation currently published +// for this repo. Same staged input, same derived graph. +// - Nothing has written to code_nodes or code_edges since that publish. The +// PR indexer mutates both between full publishes, and a swap removes the +// rows it added. Skipping while they exist would leave the live graph +// diverged from the generation that claims to describe it. +// +// An unset hash on either side reads as unknown and publishes. Every uncertain +// case falls through to the swap, so this can only skip work it has proven is +// redundant. +func stageGenerationIsNoOp(ctx context.Context, tx pgx.Tx, repoID, generationID int64) (bool, error) { + // Hashing each row first keeps the aggregate to 32 bytes per file instead of + // concatenating every staged payload into one value. + var hash string + if err := tx.QueryRow(ctx, ` + SELECT COALESCE(md5(string_agg(h, '' ORDER BY file_path)), '') + FROM ( + SELECT file_path, + md5(file_path || symbols::text || edges::text || endpoints::text) AS h + FROM graph_index_generation_files + WHERE generation_id = $1 AND status = 'ready' + ) staged`, generationID).Scan(&hash); err != nil { + return false, fmt.Errorf("publish graph generation: hash staged files: %w", err) + } + if _, err := tx.Exec(ctx, `UPDATE graph_index_generations SET content_hash = $2 + WHERE id = $1`, generationID, hash); err != nil { + return false, fmt.Errorf("publish graph generation: record content hash: %w", err) + } + if hash == "" { + return false, nil + } + + // A NULL updated_at counts as modified: it cannot be ordered against + // published_at, and guessing "untouched" would skip a swap that was needed. + var noOp bool + if err := tx.QueryRow(ctx, ` + SELECT EXISTS ( + SELECT 1 FROM repos r + JOIN graph_index_generations g ON g.id = r.graph_published_generation_id + WHERE r.id = $1 AND g.content_hash = $2 AND g.published_at IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM code_nodes n WHERE n.repo_id = $1 + AND (n.updated_at IS NULL OR n.updated_at > g.published_at)) + AND NOT EXISTS ( + SELECT 1 FROM code_edges e WHERE e.repo_id = $1 + AND (e.updated_at IS NULL OR e.updated_at > g.published_at)) + )`, repoID, hash).Scan(&noOp); err != nil { + return false, fmt.Errorf("publish graph generation: compare published hash: %w", err) + } + return noOp, nil +} + func publishGraphGeneration(ctx context.Context, st *store.Store, repoID, generationID int64) error { tx, err := st.Pool.Begin(ctx) if err != nil { @@ -550,6 +609,14 @@ func publishGraphGeneration(ctx context.Context, st *store.Store, repoID, genera return commitGraphPublishPreflightFailure(ctx, tx, repoID, generationID, limitErr) } + noOp, err := stageGenerationIsNoOp(ctx, tx, repoID, generationID) + if err != nil { + return err + } + if noOp { + return finishGraphGenerationPublish(ctx, st, tx, repoID, generationID) + } + // This transaction is the visibility boundary. Any later limit or decoding // error rolls the deletes and inserts back before the generation is failed. if _, err := tx.Exec(ctx, `DELETE FROM code_nodes WHERE repo_id = $1`, repoID); err != nil { @@ -791,6 +858,18 @@ func publishGraphGeneration(ctx context.Context, st *store.Store, repoID, genera return err } + return finishGraphGenerationPublish(ctx, st, tx, repoID, generationID) +} + +// finishGraphGenerationPublish commits the bookkeeping half of a publish: mark +// the generation published, advance the repo's graph pointers, drop the staging +// payloads, commit, then relink cross-repo API endpoints. +// +// Both publish paths end here. The swap path runs it after rewriting +// code_nodes; the no-op path runs it instead of rewriting anything. The repo +// pointers must advance either way — they record which commit the graph now +// reflects and clear the refresh request that triggered this generation. +func finishGraphGenerationPublish(ctx context.Context, st *store.Store, tx pgx.Tx, repoID, generationID int64) error { if _, err := tx.Exec(ctx, `UPDATE graph_index_generations SET status = 'published', published_at = NOW(), updated_at = NOW() WHERE id = $1 AND repo_id = $2`, generationID, repoID); err != nil { diff --git a/backend/internal/graph/fullindex_pg_test.go b/backend/internal/graph/fullindex_pg_test.go index 83fae728..57428c33 100644 --- a/backend/internal/graph/fullindex_pg_test.go +++ b/backend/internal/graph/fullindex_pg_test.go @@ -1635,3 +1635,88 @@ func TestListReposDueForGraphIndexSelectsLegacyRecentTimestampWithoutPublishedAu t.Fatalf("legacy repo with recent timestamp and null generation authority not immediately due: %+v", targets) } } + +// generationNodeIDs returns the repo's published node IDs in a stable order. +// Identity matters here: the publish swap deletes and re-inserts, so surviving +// IDs are proof the swap did not run. +func generationNodeIDs(t *testing.T, ctx context.Context, pool *pgxpool.Pool, repoID int64) []int64 { + t.Helper() + rows, err := pool.Query(ctx, `SELECT id FROM code_nodes WHERE repo_id = $1 ORDER BY id`, repoID) + if err != nil { + t.Fatalf("read node ids: %v", err) + } + defer rows.Close() + var ids []int64 + for rows.Next() { + var id int64 + if err := rows.Scan(&id); err != nil { + t.Fatalf("scan node id: %v", err) + } + ids = append(ids, id) + } + if err := rows.Err(); err != nil { + t.Fatalf("iterate node ids: %v", err) + } + return ids +} + +// A commit that touches no indexed symbol still resolves to a new head and +// still builds a generation. Publishing it must advance the repo's bookkeeping +// without rewriting code_nodes, because the swap would delete and re-insert +// every row to arrive at the graph already there. That churn is what filled +// the pggraph CDC log and the database volume on 2026-09-03. +func TestPublishSkipsSwapWhenGenerationMatchesLiveGraph(t *testing.T) { + pool, ctx := generationTestPool(t) + st := store.NewWithDB(pool) + installationID := generationSeedInstallation(t, ctx, pool, "{}") + repoID := generationSeedRepo(t, ctx, pool, installationID, "generation/noop-publish") + + gh := &fakeFullIndexGitHub{ + sha: "1111111111111111111111111111111111111111", + tree: ghpkg.RepoTree{Files: []ghpkg.RepoTreeFile{{Path: "a.go", SHA: "sha-a.go"}, {Path: "b.go", SHA: "sha-b.go"}}}, + contents: map[string]string{"a.go": "package p\nfunc Alpha() { Beta() }\n", "b.go": "package p\nfunc Beta() {}\n"}, + fetchErr: map[string]error{}, + } + first, err := IndexRepoBounded(ctx, st, gh, 1, "o", "r", "main", repoID, 10, 0) + if err != nil { + t.Fatalf("first index: %v", err) + } + if !first.Published { + t.Fatalf("first index did not publish: %+v", first) + } + before := generationNodeIDs(t, ctx, pool, repoID) + if len(before) == 0 { + t.Fatal("first publish wrote no nodes") + } + + // New head, byte-identical sources: the generation hashes to what is live. + const secondSHA = "2222222222222222222222222222222222222222" + gh.sha = secondSHA + second, err := IndexRepoBounded(ctx, st, gh, 1, "o", "r", "main", repoID, 10, 0) + if err != nil { + t.Fatalf("second index: %v", err) + } + if !second.Published { + t.Fatalf("second index did not publish: %+v", second) + } + + if after := generationNodeIDs(t, ctx, pool, repoID); !slices.Equal(before, after) { + t.Fatalf("identical generation rewrote code_nodes: %v -> %v", before, after) + } + + // Bookkeeping still has to advance, or the repo re-indexes this head forever. + var publishedSHA string + var status string + if err := pool.QueryRow(ctx, ` + SELECT r.graph_index_commit_sha, g.status + FROM repos r JOIN graph_index_generations g ON g.id = r.graph_published_generation_id + WHERE r.id = $1`, repoID).Scan(&publishedSHA, &status); err != nil { + t.Fatalf("read repo graph pointers: %v", err) + } + if publishedSHA != secondSHA { + t.Fatalf("graph_index_commit_sha = %q, want %q", publishedSHA, secondSHA) + } + if status != "published" { + t.Fatalf("published generation status = %q, want published", status) + } +} diff --git a/backend/internal/store/migrations/087_graph_generation_content_hash.down.sql b/backend/internal/store/migrations/087_graph_generation_content_hash.down.sql new file mode 100644 index 00000000..f3f97a87 --- /dev/null +++ b/backend/internal/store/migrations/087_graph_generation_content_hash.down.sql @@ -0,0 +1 @@ +ALTER TABLE graph_index_generations DROP COLUMN IF EXISTS content_hash; diff --git a/backend/internal/store/migrations/087_graph_generation_content_hash.up.sql b/backend/internal/store/migrations/087_graph_generation_content_hash.up.sql new file mode 100644 index 00000000..06e19859 --- /dev/null +++ b/backend/internal/store/migrations/087_graph_generation_content_hash.up.sql @@ -0,0 +1,12 @@ +-- A full graph publish deletes every code_node for the repo and re-inserts the +-- staged generation. That swap is the visibility boundary, so it cannot be made +-- incremental -- but when the staged generation reproduces the live graph +-- exactly, the swap rewrites every row for no change at all. +-- +-- Between 2026-08-12 and 2026-09-03 that churn wrote 10.2M rows into the +-- pggraph CDC log (graph._sync_log, 5.9GB) and filled the database volume. +-- +-- content_hash fingerprints a generation's staged file payloads so publish can +-- recognise a no-op and skip the swap. Empty means "not yet computed", which +-- always publishes. +ALTER TABLE graph_index_generations ADD COLUMN content_hash TEXT NOT NULL DEFAULT ''; From 4d37f0432cee60346e8424d9d11942e128d3b264 Mon Sep 17 00:00:00 2001 From: Dhruv Khara <66153057+BeLazy167@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:45:56 -0500 Subject: [PATCH 3/7] fix: stop NUL in suppression keys stranding pipeline runs forever suppressionKey joined path/line/body with NUL. Those strings are map keys on PipelineRun.SuppressedKeys, and persistState marshals the run into the jsonb pipeline_states.payload column. jsonb cannot represent a NUL, so Postgres rejected the whole upsert: failed to recover run ... error="persisting state: upserting pipeline_states: ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05)" The payload is how a run resumes, so a run that hits this can never persist and never recover. Recovery claims it, fails identically, and damps it for 30 minutes -- forever. Six production runs sat stranded in synthesizing between 2026-08-21 and 2026-09-02, and the sweep looked healthy the whole time: 8ms scans, error_present false, and an empty pipeline_states.error column. Switch the separator to U+001F (Unit Separator). It keeps the property NUL was chosen for -- a control character no path or review body carries -- and jsonb represents it fine. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GJehDM4aXz1AkJrTaX2mSL --- backend/internal/pipeline/suppression.go | 21 ++++++-- .../pipeline/suppression_jsonb_test.go | 52 +++++++++++++++++++ backend/internal/pipeline/types.go | 2 +- 3 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 backend/internal/pipeline/suppression_jsonb_test.go diff --git a/backend/internal/pipeline/suppression.go b/backend/internal/pipeline/suppression.go index af347308..d281584d 100644 --- a/backend/internal/pipeline/suppression.go +++ b/backend/internal/pipeline/suppression.go @@ -473,13 +473,28 @@ func countSuppressedFindings(reviews []FileReview) int { return n } +// suppressionSeparator joins the fields of a suppression key. It is U+001F +// (Unit Separator), not NUL. +// +// NUL cannot be used here. These keys are map keys on PipelineRun.SuppressedKeys, +// which persistState marshals into the jsonb pipeline_states.payload column, and +// jsonb cannot represent a NUL — Postgres rejects the whole upsert with +// SQLSTATE 22P05. Because the payload is how a run resumes, such a run can never +// persist and never recover: recovery re-reads it, fails identically, and backs +// off for 30 minutes forever. Six production runs stranded that way between +// 2026-08-21 and 2026-09-02. +// +// U+001F keeps the property NUL was chosen for — a control character no file +// path or review body carries — while remaining representable in jsonb. +const suppressionSeparator = "\x1f" + // suppressionKey identifies a finding across the FileReviews and AllFileReviews // snapshots. Path/Line/Body are value-copied identically into both (scoring // snapshots FileReviews by copy BEFORE enrichment sets the Suppressed flag), so -// the same key resolves the finding in either snapshot. The \x00 separators keep -// the key unambiguous even when a body contains delimiters. +// the same key resolves the finding in either snapshot. The separators keep the +// key unambiguous even when a body contains delimiters. func suppressionKey(path string, line int, body string) string { - return path + "\x00" + strconv.Itoa(line) + "\x00" + body + return path + suppressionSeparator + strconv.Itoa(line) + suppressionSeparator + body } // isSuppressed reports whether a finding (path/line/body) was dropped by the diff --git a/backend/internal/pipeline/suppression_jsonb_test.go b/backend/internal/pipeline/suppression_jsonb_test.go new file mode 100644 index 00000000..a1e73e69 --- /dev/null +++ b/backend/internal/pipeline/suppression_jsonb_test.go @@ -0,0 +1,52 @@ +package pipeline + +import ( + "bytes" + "encoding/json" + "strings" + "testing" +) + +// SuppressedKeys are map keys on PipelineRun, and persistState marshals the run +// into the jsonb pipeline_states.payload column. jsonb cannot represent a NUL: +// Postgres rejects the whole upsert with SQLSTATE 22P05. Because that payload is +// how a run resumes, such a run can never persist and never recover -- recovery +// re-reads it, fails identically, and backs off for 30 minutes, forever. +// +// Six production runs stranded exactly that way between 2026-08-21 and +// 2026-09-02, reporting: +// +// failed to recover run ... error="persisting state: upserting pipeline_states: +// ERROR: unsupported Unicode escape sequence (SQLSTATE 22P05)" +// +// Both assertions fail if suppressionSeparator goes back to NUL. +func TestSuppressionKeyIsRepresentableInJSONB(t *testing.T) { + key := suppressionKey("src/lib/acquisition/submit.ts", 45, "cookies retry indefinitely") + if strings.ContainsRune(key, 0) { + t.Fatal("suppression key contains NUL: jsonb rejects it with SQLSTATE 22P05, leaving the run unrecoverable") + } + + // The marshalled shape is what reaches Postgres, so assert on that too. Go + // encodes a NUL inside a map key as a six-character escape, and it is that + // escape, not the raw byte, which jsonb refuses. + payload, err := json.Marshal(map[string]struct{}{key: {}}) + if err != nil { + t.Fatalf("marshal suppressed keys: %v", err) + } + if bytes.Contains(payload, []byte("\\u0000")) { + t.Fatalf("marshalled SuppressedKeys carries a NUL escape that jsonb rejects: %s", payload) + } +} + +// The separator only works as a delimiter if it cannot appear in the fields it +// joins, which is why NUL was chosen originally. Keep that property explicit so +// a future move to a printable separator has to confront it. +func TestSuppressionKeySeparatorStaysUnambiguous(t *testing.T) { + key := suppressionKey("a.go", 12, "body") + if got := strings.Count(key, suppressionSeparator); got != 2 { + t.Fatalf("separator count = %d, want 2 (path/line/body must stay distinguishable)", got) + } + if strings.ContainsAny(suppressionSeparator, "abcdefghijklmnopqrstuvwxyz0123456789/.-_ ") { + t.Fatalf("separator %q is a character review bodies and file paths can contain", suppressionSeparator) + } +} diff --git a/backend/internal/pipeline/types.go b/backend/internal/pipeline/types.go index 37a3ed04..2dd90d30 100644 --- a/backend/internal/pipeline/types.go +++ b/backend/internal/pipeline/types.go @@ -125,7 +125,7 @@ type PipelineRun struct { // below their severity threshold) plus nits demoted from files that carry a // blocking finding. Rendered collapsed in the summary, never inline. MinorNotes []MinorNote `json:"minor_notes,omitempty"` - SuppressedKeys map[string]struct{} // path\x00line\x00body of dismissal-dropped findings; gates pattern-learning that reads the pre-enrich AllFileReviews snapshot + SuppressedKeys map[string]struct{} // path\x1fline\x1fbody of dismissal-dropped findings; gates pattern-learning that reads the pre-enrich AllFileReviews snapshot. Separator must stay jsonb-safe: see suppressionSeparator Synthesis *SynthesisResult Tokens RunTokenUsage Persona Persona From bbe9da7a4c4fd276352e3ecbff53a0ff7272db6d Mon Sep 17 00:00:00 2001 From: Dhruv Khara <66153057+BeLazy167@users.noreply.github.com> Date: Fri, 4 Sep 2026 00:16:07 -0500 Subject: [PATCH 4/7] fix(test): assert the separator property instead of a blacklist The adversarial gate on #287 found TestSuppressionKeySeparatorStaysUnambiguous did not enforce what its comment promised. It scanned strings.ContainsAny(sep, "abc...0123/.-_ "), which lets through every uppercase letter and every punctuation mark outside that short list. Reproduced: swapping the separator to "|" or "A" kept the test green. Both appear constantly in review bodies -- "|" in every markdown table -- and either makes suppressionKey ambiguous: suppressionKey("a", 1, "b|2|c") == suppressionKey("a|1|b", 2, "c") == "a|1|b|2|c" so one dismissal suppresses an unrelated live finding and pattern-learning silently skips it. Assert the actual property: a single C0 control byte other than NUL. NUL breaks jsonb (22P05); anything >= 0x20 is printable and can appear in a path or a body. Verified the new assertion rejects "|" (0x7c) and accepts U+001F. --- .../pipeline/suppression_jsonb_test.go | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/backend/internal/pipeline/suppression_jsonb_test.go b/backend/internal/pipeline/suppression_jsonb_test.go index a1e73e69..2a04e2b3 100644 --- a/backend/internal/pipeline/suppression_jsonb_test.go +++ b/backend/internal/pipeline/suppression_jsonb_test.go @@ -39,14 +39,31 @@ func TestSuppressionKeyIsRepresentableInJSONB(t *testing.T) { } // The separator only works as a delimiter if it cannot appear in the fields it -// joins, which is why NUL was chosen originally. Keep that property explicit so -// a future move to a printable separator has to confront it. +// joins, which is why NUL was chosen originally. +// +// Asserted as a character-class property, not a blacklist. An earlier version +// of this test scanned `strings.ContainsAny(sep, "abc...0123/.-_ ")`, which let +// through every uppercase letter and every punctuation mark outside that short +// list. Swapping the separator to "|" or "A" kept it green even though both +// appear constantly in review bodies -- "|" in every markdown table -- and +// either makes suppressionKey ambiguous: with "|", +// +// suppressionKey("a", 1, "b|2|c") == suppressionKey("a|1|b", 2, "c") == "a|1|b|2|c" +// +// so one dismissal suppresses an unrelated live finding and pattern-learning +// silently skips it. +// +// A single C0 control byte other than NUL is the property that actually holds: +// no file path or LLM-authored review body carries one, and jsonb represents it. func TestSuppressionKeySeparatorStaysUnambiguous(t *testing.T) { key := suppressionKey("a.go", 12, "body") if got := strings.Count(key, suppressionSeparator); got != 2 { t.Fatalf("separator count = %d, want 2 (path/line/body must stay distinguishable)", got) } - if strings.ContainsAny(suppressionSeparator, "abcdefghijklmnopqrstuvwxyz0123456789/.-_ ") { - t.Fatalf("separator %q is a character review bodies and file paths can contain", suppressionSeparator) + if len(suppressionSeparator) != 1 { + t.Fatalf("separator %q is %d bytes, want a single control byte", suppressionSeparator, len(suppressionSeparator)) + } + if b := suppressionSeparator[0]; b == 0x00 || b >= 0x20 { + t.Fatalf("separator byte %#x must be a C0 control other than NUL: NUL breaks jsonb (SQLSTATE 22P05), and anything >= 0x20 is printable and can appear in a path or review body", b) } } From 725017211f8da5e0a470a3c5ea155207bc16f536 Mon Sep 17 00:00:00 2001 From: Dhruv Khara <66153057+BeLazy167@users.noreply.github.com> Date: Fri, 4 Sep 2026 00:52:24 -0500 Subject: [PATCH 5/7] fix(test): reject whitespace separators the byte-class check admitted The gate's re-run flagged that the C0-control assertion still accepts tab, LF and CR. LF is not an edge case here -- review bodies are multi-line markdown, so it is the ordinary case -- and it collides exactly like a printable separator: suppressionKey("a.go", 1, "b\n2\nc") == suppressionKey("a.go\n1\nb", 2, "c") Reject 0x09/0x0a/0x0d explicitly, and assert the property directly against a body shaped like a real finding (markdown table, blank lines, an indented line, CRLF) rather than trusting the byte class to imply it. Verified the assertion rejects "\n" (0x0a) and still accepts U+001F. --- .../pipeline/suppression_jsonb_test.go | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/backend/internal/pipeline/suppression_jsonb_test.go b/backend/internal/pipeline/suppression_jsonb_test.go index 2a04e2b3..7818f267 100644 --- a/backend/internal/pipeline/suppression_jsonb_test.go +++ b/backend/internal/pipeline/suppression_jsonb_test.go @@ -63,7 +63,23 @@ func TestSuppressionKeySeparatorStaysUnambiguous(t *testing.T) { if len(suppressionSeparator) != 1 { t.Fatalf("separator %q is %d bytes, want a single control byte", suppressionSeparator, len(suppressionSeparator)) } - if b := suppressionSeparator[0]; b == 0x00 || b >= 0x20 { - t.Fatalf("separator byte %#x must be a C0 control other than NUL: NUL breaks jsonb (SQLSTATE 22P05), and anything >= 0x20 is printable and can appear in a path or review body", b) + switch b := suppressionSeparator[0]; { + case b == 0x00: + t.Fatal("separator is NUL: jsonb rejects it with SQLSTATE 22P05 and the run becomes unrecoverable") + case b >= 0x20: + t.Fatalf("separator byte %#x is printable and can appear in a file path or review body", b) + case b == 0x09 || b == 0x0a || b == 0x0d: + // Caught by the gate on #287: the byte-class check alone admits tab, LF + // and CR. Review bodies are multi-line markdown, so LF is the ordinary + // case, and it collides exactly like a printable separator would: + // suppressionKey("a.go", 1, "b\n2\nc") == suppressionKey("a.go\n1\nb", 2, "c") + t.Fatalf("separator byte %#x is whitespace that review bodies contain routinely", b) + } + + // Assert the property directly against a body shaped like a real finding, + // rather than trusting the byte class to imply it. + body := "Guard the write.\n\n| file | line |\n| --- | --- |\n| a.go | 12 |\n\n\tindented\r\n" + if strings.Contains(body, suppressionSeparator) { + t.Fatalf("separator %q occurs in a representative review body, so keys are ambiguous", suppressionSeparator) } } From 64b2f50d86e98356cdbbfdccf72b409430546941 Mon Sep 17 00:00:00 2001 From: Dhruv Khara <66153057+BeLazy167@users.noreply.github.com> Date: Fri, 4 Sep 2026 01:06:04 -0500 Subject: [PATCH 6/7] fix(test): rewrite mirror ownership test that migration 086 invalidated TestMirrorWorkerDeleteKeepsMemoryOwnedByDuplicatePattern built its setup from two co-existing patterns sharing one memory_custom_id. Migration 086 added patterns_installation_memory_custom_uniq over (installation_id, memory_custom_id), so that cannot produce a second row and the test deleted the only owner, then asserted a memory nothing owned would survive. Verified against PG 19beta2 on this tree: the second create fails with "duplicate key value violates unique constraint". (Where CreatePattern upserts instead of raw-inserting, it silently returns the first row -- same dead end, different symptom. The comment covers both.) The file now covers three branches of patternMirrorDeleteAuthorized: DuplicatePattern two rows, memory_doc_id set, custom_id NULL -> explicitlyOwned COALESCE LivePattern live row + a superseded row's late delete -> explicitlyOwned, sequential LegacyDuplicatePattern two rows, both columns NULL (pre-existing) -> legacyOwner derivation 086's index is PARTIAL (WHERE memory_custom_id IS NOT NULL), so legacy memory_doc_id rows are unconstrained and the duplicate-owner case is still reachable -- which is why the first branch is restored rather than dropped. All three verified by mutation: stubbing patternMirrorDeleteAuthorized to always authorise fails all three. Full suite 2875 passed. --- .../internal/memory/mirror_worker_pg_test.go | 119 ++++++++++++++++-- 1 file changed, 107 insertions(+), 12 deletions(-) diff --git a/backend/internal/memory/mirror_worker_pg_test.go b/backend/internal/memory/mirror_worker_pg_test.go index fb34ba9a..70cb308e 100644 --- a/backend/internal/memory/mirror_worker_pg_test.go +++ b/backend/internal/memory/mirror_worker_pg_test.go @@ -122,9 +122,27 @@ func TestMirrorWorkerPreservesPipelinePatternProvenanceAndMetadata(t *testing.T) } } -// Two relational pattern rows may intentionally converge on the same -// deterministic memory custom ID. Removing either projection must not remove -// the shared memory while the other projection remains authoritative. +// Two co-existing pattern rows can still share one deterministic memory +// identity, and deleting either must not tombstone the memory the other owns. +// +// This case survives migration 086 because that index is PARTIAL: +// +// CREATE UNIQUE INDEX patterns_installation_memory_custom_uniq +// ON patterns (installation_id, memory_custom_id) +// WHERE memory_custom_id IS NOT NULL; +// +// Rows carrying only a legacy memory_doc_id leave memory_custom_id NULL, so the +// index never applies. Both DeletePattern (firstNonEmpty(MemoryCustomID, +// MemoryDocID)) and patternMirrorDeleteAuthorized (COALESCE over the same two +// columns) resolve identity through memory_doc_id, so the duplicate-owner guard +// is live for exactly these rows. +// +// The memory_custom_id spelling of this test is what 086 made unconstructible: +// with that column set, a second create cannot produce a second row. How it +// fails depends on CreatePattern's conflict handling -- here the raw insert +// raises "duplicate key value violates unique constraint"; where CreatePattern +// upserts, it silently returns the first row instead. Either way the old test +// deleted the only owner and asserted a memory nothing owned would survive. func TestMirrorWorkerDeleteKeepsMemoryOwnedByDuplicatePattern(t *testing.T) { pool, install := pgTestPool(t) ctx := context.Background() @@ -138,19 +156,20 @@ func TestMirrorWorkerDeleteKeepsMemoryOwnedByDuplicatePattern(t *testing.T) { var repoID int64 if err := pool.QueryRow(ctx, ` INSERT INTO repos (installation_id, github_id, full_name) - VALUES ($1, (random() * 1000000000)::bigint, 'acme/mirror-owner-duplicate') + VALUES ($1, (random() * 1000000000)::bigint, 'acme/mirror-owner-legacy-dup') RETURNING id`, install).Scan(&repoID); err != nil { t.Fatalf("seed repo: %v", err) } source := "manual" - customID := PatternCustomID("", "mirror-owner-duplicate", source, "guard shared writes") - first, err := st.CreatePattern(ctx, install, &repoID, "guard shared writes", nil, nil, &source, nil, nil, stringPointer(customID), nil) + // Legacy identity: memory_doc_id set, memory_custom_id left NULL. + docID := fmt.Sprintf("sm_legacy_dup_%d", time.Now().UnixNano()) + first, err := st.CreatePattern(ctx, install, &repoID, "guard shared writes", stringPointer(docID), nil, &source, nil, nil, nil, nil) if err != nil { - t.Fatalf("create first pattern: %v", err) + t.Fatalf("create first legacy pattern: %v", err) } - second, err := st.CreatePattern(ctx, install, &repoID, "guard shared writes", nil, nil, &source, nil, nil, stringPointer(customID), nil) + second, err := st.CreatePattern(ctx, install, &repoID, "guard shared writes", stringPointer(docID), nil, &source, nil, nil, nil, nil) if err != nil { - t.Fatalf("create duplicate pattern: %v", err) + t.Fatalf("create duplicate legacy pattern: %v", err) } t.Cleanup(func() { bg := context.Background() @@ -158,9 +177,12 @@ func TestMirrorWorkerDeleteKeepsMemoryOwnedByDuplicatePattern(t *testing.T) { _, _ = pool.Exec(bg, `DELETE FROM patterns WHERE id=ANY($1::bigint[])`, []int64{first.ID, second.ID}) _, _ = pool.Exec(bg, `DELETE FROM repos WHERE id=$1`, repoID) }) + if first.ID == second.ID { + t.Fatalf("legacy duplicates collapsed to one row (id %d): the partial index now covers memory_doc_id, so this scenario is gone", first.ID) + } - if _, err := idx.IndexPattern(ctx, "mirror-owner-duplicate", PatternMemory{ - Content: "guard shared writes", CustomID: customID, Source: source, + if _, err := idx.IndexPattern(ctx, "mirror-owner-legacy-dup", PatternMemory{ + Content: "guard shared writes", CustomID: docID, Source: source, }); err != nil { t.Fatalf("seed memory: %v", err) } @@ -178,11 +200,84 @@ func TestMirrorWorkerDeleteKeepsMemoryOwnedByDuplicatePattern(t *testing.T) { break } } - if row := readRow(t, pool, install, customID); row.deletedAt != nil { + if row := readRow(t, pool, install, docID); row.deletedAt != nil { t.Fatal("deleting one duplicate pattern tombstoned memory still owned by the other pattern") } } +// The sequential counterpart: a live row owns the custom ID and a superseded +// row's delete arrives afterwards. patternMirrorDeleteAuthorized must see the +// live owner and skip the tombstone. +func TestMirrorWorkerDeleteKeepsMemoryOwnedByLivePattern(t *testing.T) { + pool, install := pgTestPool(t) + ctx := context.Background() + lockMirrorOutboxPGTests(t, pool, ctx) + st := store.NewWithDB(pool) + if _, err := pool.Exec(ctx, `DELETE FROM memory_mirror_outbox`); err != nil { + t.Fatalf("clear mirror outbox: %v", err) + } + idx := NewPGIndexer(pool, nil, install, pgTestDims, slog.New(slog.DiscardHandler)) + + var repoID int64 + if err := pool.QueryRow(ctx, ` + INSERT INTO repos (installation_id, github_id, full_name) + VALUES ($1, (random() * 1000000000)::bigint, 'acme/mirror-owner-live') + RETURNING id`, install).Scan(&repoID); err != nil { + t.Fatalf("seed repo: %v", err) + } + source := "manual" + customID := PatternCustomID("", "mirror-owner-live", source, "guard shared writes") + live, err := st.CreatePattern(ctx, install, &repoID, "guard shared writes", nil, nil, &source, nil, nil, stringPointer(customID), nil) + if err != nil { + t.Fatalf("create live pattern: %v", err) + } + t.Cleanup(func() { + bg := context.Background() + _, _ = pool.Exec(bg, `DELETE FROM memory_mirror_outbox WHERE installation_id=$1 AND aggregate_type='pattern'`, install) + _, _ = pool.Exec(bg, `DELETE FROM patterns WHERE id=$1`, live.ID) + _, _ = pool.Exec(bg, `DELETE FROM repos WHERE id=$1`, repoID) + }) + + if _, err := idx.IndexPattern(ctx, "mirror-owner-live", PatternMemory{ + Content: "guard shared writes", CustomID: customID, Source: source, + }); err != nil { + t.Fatalf("seed memory: %v", err) + } + + // A superseded row's delete, arriving after the live row already owns the + // custom ID. It has to be enqueued directly: the delete must be the LAST + // event drained, and DeletePattern-then-CreatePattern would order a reviving + // upsert behind it, which masks a tombstone rather than preventing one. + stalePayload, err := NewDeleteMirrorPayload(customID) + if err != nil { + t.Fatalf("build delete payload: %v", err) + } + staleAggregateID := live.ID + 1_000_000 + if err := st.WithMemoryMirrorTx(ctx, func(pgx.Tx) (store.MemoryMirrorEvent, error) { + return store.MemoryMirrorEvent{ + InstallationID: install, AggregateType: store.MemoryMirrorPattern, + AggregateID: staleAggregateID, Operation: store.MemoryMirrorDelete, + Payload: stalePayload, + }, nil + }); err != nil { + t.Fatalf("enqueue stale delete: %v", err) + } + + worker := NewMirrorWorker(st, func(context.Context, int64) MirrorIndexer { return idx }, slog.New(slog.DiscardHandler)) + for { + processed, err := worker.RunOnce(ctx, 50) + if err != nil { + t.Fatalf("drain mirror events: %v", err) + } + if processed == 0 { + break + } + } + if row := readRow(t, pool, install, customID); row.deletedAt != nil { + t.Fatal("a superseded pattern's delete tombstoned memory the live pattern still owns") + } +} + func stringPointer(value string) *string { return &value } // An old aggregate's delete and a recreated aggregate's upsert share memory From 088b1c5b9628e7a3bfa7829259b2246c69f4d90c Mon Sep 17 00:00:00 2001 From: Dhruv Khara <66153057+BeLazy167@users.noreply.github.com> Date: Fri, 4 Sep 2026 02:04:27 -0500 Subject: [PATCH 7/7] build: regenerate sqlc models for migration 087 Migration 087 adds graph_index_generations.content_hash, and sqlc derives internal/store/db/models.go from the migration schema, so the committed output drifted and `make sqlc-check` failed the build job: diff --git a/backend/internal/store/db/models.go make: *** [Makefile:39: sqlc-check] Error 1 Regenerated with sqlc v1.30.0 (the version CI pins). The change is one field, ContentHash, on GraphIndexGeneration. Any PR adding a store migration needs this same regeneration. --- backend/internal/store/db/models.go | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/internal/store/db/models.go b/backend/internal/store/db/models.go index 1c9dcda7..69ce9b23 100644 --- a/backend/internal/store/db/models.go +++ b/backend/internal/store/db/models.go @@ -170,6 +170,7 @@ type GraphIndexGeneration struct { PublishedAt *time.Time `json:"published_at"` RefreshVersion int64 `json:"refresh_version"` UnavailableFiles int `json:"unavailable_files"` + ContentHash string `json:"content_hash"` } type GraphIndexGenerationFile struct {