Files
multica/server/internal/handler/search_test.go
Multica Eve 359ef61dc3 fix(search): pg_trgm index fallback + statement_timeout guard (MUL-4059) (#4925)
* fix(search): add pg_trgm index fallback + statement_timeout guard (MUL-4059)

Root cause of the "search freezes with no response" symptom reported in
MUL-4059: the search handler runs LOWER(col) LIKE '%pattern%' queries
that expect a pg_bigm GIN index (migrations 032, 033, 036), but every
migration wraps the CREATE EXTENSION + CREATE INDEX in a DO/EXCEPTION
handler that silently skips when pg_bigm is unavailable. The bundled
self-host / dev / CI Postgres image (pgvector/pgvector:pg17) does not
ship pg_bigm, so on every self-hosted deployment the migrations no-op
and no GIN indexes get built. Every /api/issues/search + /api/projects/search
request then falls back to a Seq Scan on `issue` + correlated Seq Scans
on `comment` — verified with EXPLAIN on the local dev DB, which has zero
title/description/comment search indexes before this change.

Two independent guardrails are added, either of which alone would have
prevented the reported hang:

1) Migration 134 installs pg_trgm (ships in all standard Postgres +
   pgvector images) and builds GIN indexes with gin_trgm_ops on
   `LOWER(title)`, `LOWER(COALESCE(description, ''))`, and
   `LOWER(content)`. The expression signatures match the search
   handler's WHERE clauses exactly, so the planner picks the index
   without further changes. The pg_bigm indexes from 036 are left
   intact — deployments on AWS RDS with pg_bigm 1.2 keep the CJK-friendly
   bigram path; deployments without it get the trigram fallback. Verified
   against a local 25k-row fixture: the description LIKE hits
   `Bitmap Index Scan on idx_issue_description_trgm` in 0.5 ms.

2) runSearchQuery wraps both search handlers in a short-lived read-only
   transaction with SET LOCAL statement_timeout = 3 s. In the pathological
   case where indexes are still missing or the query plan is bad, callers
   see a fast 503 with a descriptive error instead of a stalled request.
   Verified against a live Postgres: a deliberate pg_sleep(2) with the
   test override at 200 ms is cut off in 230 ms with SQLSTATE 57014, as
   asserted by TestRunSearchQuery_StatementTimeoutFires.

Non-goals: this change does not remove the pg_bigm code path, does not
change the SQL the handler builds, and does not change the API response
shape. It is the minimum diff to unblock production while preserving
the CJK-search advantage that pg_bigm provides where it is available.

Co-authored-by: multica-agent <github@multica.ai>

* fix(search): scope comment subqueries to workspace to unblock prd hang (MUL-4059)

Follow-up correction after PRD investigation: pg_bigm IS installed on
prd `multica-prod` and all five bigm indexes exist in the correct
`LOWER(...) gin_bigm_ops` form. The initial "missing index" hypothesis
was wrong; migration 134 (pg_trgm fallback) still helps self-host but
does not touch the production hang path.

Actual prd EXPLAIN (workspace with 60k issues, keyword "search"):

    Index Scan using idx_issue_workspace on issue i
    Rows Removed by Filter: 59123
    SubPlan 2
      Bitmap Heap Scan on comment c
        Rows Removed by Index Recheck: 1928275
        Heap Blocks: exact=48297 lossy=164696
      Bitmap Index Scan on idx_comment_content_bigm
        rows=536761
    Execution Time: 32345.002 ms

Root cause: the correlated `EXISTS` over `comment` gets rewritten by
the planner into a *hashed* subplan. Without a workspace_id filter in
the subquery, that hashed set covers every comment in every workspace
matching the LIKE — 536k rows for "search" — which spills work_mem
into a lossy bitmap and rechecks 1.9M rows.

Two-part fix:

1. Query rewrite. buildSearchQuery now emits
   `c.workspace_id = $wsParam` inside every comment subquery (WHERE
   phrase match, WHERE multi-term match, tier 7 rank, tier 8 rank, and
   the matched_comment_content COALESCE). The same $4 parameter is
   reused so Postgres treats it as a compile-time constant and pushes
   it into the hashed subplan's key, collapsing the set to this
   workspace's comments.

2. Supporting index (migration 135). New
   `idx_comment_workspace ON comment (workspace_id)`. Without it, the
   pushed-down filter still triggers a Seq Scan on `comment` because
   comment has no btree on workspace_id (only the FK constraint and
   composite (issue_id, ...) indexes).

Locally verified against a repro that mirrors prd (5k issues in the
target workspace, 100k comments in a sibling workspace all containing
"search"): the plan drops from 60 ms (hashed global scan, no support
index) to 3 ms (subplan uses idx_comment_workspace). Prd extrapolation
from the same shape: 32.3 s → tens of milliseconds.

Regression test TestBuildSearchQuery_CommentSubqueryWorkspaceScope
asserts every `FROM comment c` in the generated SQL is followed by a
`c.workspace_id = $4` filter, so a future refactor can't silently
regress the plan back to the global-hash pathology.

The statement_timeout guard from the earlier commit in this branch is
kept — it still bounds the worst case if any future query shape
regresses.

Co-authored-by: multica-agent <github@multica.ai>

* fix(search): address PR review — unwrap 135 + add project trigram indexes (MUL-4059)

Both must-fix items from GPT-Boy's review:

1. Migration 135 unwrapped. The previous version buried
   `CREATE INDEX idx_comment_workspace` inside `DO $$ ... EXCEPTION
   WHEN OTHERS $$` — exactly the anti-pattern that caused MUL-4059 in
   the first place. `idx_comment_workspace` is not a CJK-bonus fallback;
   it is the critical support that makes the query rewrite land on an
   Index Scan instead of a Seq Scan. A silent failure (lock timeout,
   disk full, permission denied, schema drift) MUST abort the migration
   and fail deployment, not slip through as green. The unwrapped
   `CREATE INDEX IF NOT EXISTS` now propagates real errors to the
   migration runner, which aborts and does NOT record the version as
   applied. IF NOT EXISTS keeps idempotency for the operator-precreated
   case (`CREATE INDEX CONCURRENTLY ...` before running migrations on
   large prd tables).

2. Migration 134 now covers project search too. SearchProjects reads
   `LOWER(project.title)` and `LOWER(COALESCE(project.description, ''))`,
   and the pg_bigm equivalents in migration 039 silently no-op on
   pg_bigm-less images just like 032/033/036. Without the trigram
   fallback, project searches on self-host would still Seq Scan and hit
   the 3 s statement_timeout guard as a 503 — technically bounded but
   not actually fixed. Added `idx_project_title_trgm` and
   `idx_project_description_trgm`; the down migration drops them too.

Also: fixed the search.go comment that said callers get a "standard
500" — they get a 503 with SQLSTATE-57014 mapping; the comment now
matches reality.

Verified: build clean, vet clean, existing search / timeout tests
still green. Migration 135 dry-run (dropping the index, re-applying
the unwrapped SQL under `ON_ERROR_STOP=1`) creates the index cleanly;
a deliberate `CREATE INDEX` on a non-existent column now aborts psql
with exit 3, confirming the migration runner would fail loudly on any
real error.

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: Eve <eve@multica-ai.local>
Co-authored-by: multica-agent <github@multica.ai>
2026-07-06 12:34:51 +08:00

297 lines
11 KiB
Go

package handler
import (
"strings"
"testing"
)
func TestBuildSearchQuery_SingleTerm(t *testing.T) {
query, args := buildSearchQuery("Hello", []string{"Hello"}, 0, false, false)
// Pattern should be lowercased in Go.
if args[0] != "hello" {
t.Errorf("expected phrase arg to be lowercased, got %q", args[0])
}
// Must use LOWER(column) LIKE, not ILIKE.
if strings.Contains(query, "ILIKE") {
t.Error("query should not contain ILIKE")
}
if !strings.Contains(query, "LOWER(i.title) LIKE") {
t.Error("query should contain LOWER(i.title) LIKE")
}
if !strings.Contains(query, "LOWER(COALESCE(i.description, '')) LIKE") {
t.Error("query should contain LOWER(COALESCE(i.description, '')) LIKE")
}
if !strings.Contains(query, "LOWER(c.content) LIKE") {
t.Error("query should contain LOWER(c.content) LIKE")
}
// Exact title rank should not double-LOWER the pattern.
if strings.Contains(query, "LOWER(i.title) = LOWER(") {
t.Error("exact title rank should not wrap pattern in LOWER (already lowercased in Go)")
}
if !strings.Contains(query, "LOWER(i.title) = $1") {
t.Error("exact title rank should compare LOWER(i.title) = $1 directly")
}
// Should exclude closed issues by default.
if !strings.Contains(query, "NOT IN ('done', 'cancelled')") {
t.Error("query should exclude done/cancelled when includeClosed=false")
}
}
func TestBuildSearchQuery_MultiTerm(t *testing.T) {
query, args := buildSearchQuery("Foo Bar", []string{"Foo", "Bar"}, 0, false, false)
// Both phrase and terms should be lowercased.
if args[0] != "foo bar" {
t.Errorf("expected phrase arg lowercased, got %q", args[0])
}
// args[0]=exact, args[1]=%phrase%, args[2]=phrase%, args[3]=workspace_id placeholder; term args start at args[4].
if args[4] != "%foo%" {
t.Errorf("expected first term arg as contains pattern, got %q", args[4])
}
if args[5] != "%bar%" {
t.Errorf("expected second term arg as contains pattern, got %q", args[5])
}
// Multi-word query should have AND conditions.
if !strings.Contains(query, " AND ") {
t.Error("multi-word query should contain AND conditions for per-term matching")
}
}
func TestBuildSearchQuery_WithNumber(t *testing.T) {
query, args := buildSearchQuery("MUL-42", []string{"MUL-42"}, 42, true, false)
_ = args
// Number match should be in WHERE.
if !strings.Contains(query, "i.number = ") {
t.Error("query should contain number match in WHERE clause")
}
// Tier 0 rank for identifier match.
if !strings.Contains(query, "THEN 0") {
t.Error("query should contain tier 0 rank for identifier match")
}
}
func TestBuildSearchQuery_IncludeClosed(t *testing.T) {
query, _ := buildSearchQuery("test", []string{"test"}, 0, false, true)
if strings.Contains(query, "NOT IN ('done', 'cancelled')") {
t.Error("query should not exclude done/cancelled when includeClosed=true")
}
}
func TestBuildSearchQuery_SpecialChars(t *testing.T) {
query, args := buildSearchQuery("100%", []string{"100%"}, 0, false, false)
_ = query
// % should be escaped in the phrase arg.
if escaped, ok := args[0].(string); !ok || !strings.Contains(escaped, `\%`) {
t.Errorf("expected %% to be escaped in phrase arg, got %q", args[0])
}
}
// --- Project search tests ---
func TestBuildProjectSearchQuery_SingleTerm(t *testing.T) {
query, args := buildProjectSearchQuery("Hello", []string{"Hello"}, false)
if args[0] != "hello" {
t.Errorf("expected phrase arg to be lowercased, got %q", args[0])
}
if strings.Contains(query, "ILIKE") {
t.Error("query should not contain ILIKE")
}
if !strings.Contains(query, "LOWER(p.title) LIKE") {
t.Error("query should contain LOWER(p.title) LIKE")
}
if !strings.Contains(query, "LOWER(COALESCE(p.description, '')) LIKE") {
t.Error("query should contain LOWER(COALESCE(p.description, '')) LIKE")
}
// Should exclude completed/cancelled by default.
if !strings.Contains(query, "NOT IN ('completed', 'cancelled')") {
t.Error("query should exclude completed/cancelled when includeClosed=false")
}
}
func TestBuildProjectSearchQuery_MultiTerm(t *testing.T) {
query, args := buildProjectSearchQuery("Foo Bar", []string{"Foo", "Bar"}, false)
if args[0] != "foo bar" {
t.Errorf("expected phrase arg lowercased, got %q", args[0])
}
if args[2] != "foo" {
t.Errorf("expected first term arg lowercased, got %q", args[2])
}
if args[3] != "bar" {
t.Errorf("expected second term arg lowercased, got %q", args[3])
}
if !strings.Contains(query, " AND ") {
t.Error("multi-word query should contain AND conditions for per-term matching")
}
}
func TestBuildProjectSearchQuery_IncludeClosed(t *testing.T) {
query, _ := buildProjectSearchQuery("test", []string{"test"}, true)
if strings.Contains(query, "NOT IN ('completed', 'cancelled')") {
t.Error("query should not exclude completed/cancelled when includeClosed=true")
}
}
// --- extractSnippet regression tests ---
func TestExtractSnippet_PhraseMatch(t *testing.T) {
content := "The quick brown fox jumps over the lazy dog near the river bank"
snippet := extractSnippet(content, "brown fox")
if !strings.Contains(snippet, "brown fox") {
t.Errorf("snippet should contain the phrase 'brown fox', got %q", snippet)
}
}
func TestExtractSnippet_MultiWordNonContiguous(t *testing.T) {
// "deploy" and "kubernetes" both appear but not as a contiguous phrase.
content := "We need to deploy the new service. The kubernetes cluster is ready for production workloads."
snippet := extractSnippet(content, "deploy kubernetes")
// Should NOT fall back to first 120 chars blindly — should center on earliest term.
if !strings.Contains(strings.ToLower(snippet), "deploy") && !strings.Contains(strings.ToLower(snippet), "kubernetes") {
t.Errorf("snippet should contain at least one search term, got %q", snippet)
}
// Specifically, "deploy" appears first so snippet should be centered around it.
if !strings.Contains(strings.ToLower(snippet), "deploy") {
t.Errorf("snippet should center on earliest term 'deploy', got %q", snippet)
}
}
func TestExtractSnippet_FallbackWhenNoMatch(t *testing.T) {
content := strings.Repeat("a", 200)
snippet := extractSnippet(content, "zzz")
if len([]rune(snippet)) > 124 { // 120 + "..."
t.Errorf("snippet should be truncated to ~120 runes when no match, got len=%d", len([]rune(snippet)))
}
}
func TestExtractSnippet_ShortContent(t *testing.T) {
content := "short text"
snippet := extractSnippet(content, "missing")
if snippet != content {
t.Errorf("short content with no match should return as-is, got %q", snippet)
}
}
func TestExtractSnippet_CaseInsensitive(t *testing.T) {
content := "Error in HTML rendering pipeline"
snippet := extractSnippet(content, "html")
if !strings.Contains(snippet, "HTML") {
t.Errorf("snippet should find case-insensitive match, got %q", snippet)
}
}
func TestExtractSnippet_CJKContent(t *testing.T) {
content := "这是一段很长的中文内容,包含了搜索关键词测试用例,用来验证多字节字符不会被截断的情况"
snippet := extractSnippet(content, "搜索关键词")
if !strings.Contains(snippet, "搜索关键词") {
t.Errorf("snippet should contain CJK phrase, got %q", snippet)
}
}
// --- Ranking regression tests ---
func TestBuildSearchQuery_CommentRankTiers(t *testing.T) {
query, _ := buildSearchQuery("test phrase", []string{"test", "phrase"}, 0, false, false)
// Comment phrase match should be tier 7
if !strings.Contains(query, "THEN 7") {
t.Error("query should contain tier 7 for comment phrase match")
}
// Comment all-term match should be tier 8
if !strings.Contains(query, "THEN 8") {
t.Error("query should contain tier 8 for comment all-term match")
}
// Fallback should be 9, not 7
if !strings.Contains(query, "ELSE 9") {
t.Error("query fallback should be ELSE 9")
}
}
func TestBuildSearchQuery_DescriptionRankTiers(t *testing.T) {
query, _ := buildSearchQuery("foo bar", []string{"foo", "bar"}, 0, false, false)
// Description phrase match should be tier 5
if !strings.Contains(query, "THEN 5") {
t.Error("query should contain tier 5 for description phrase match")
}
// Description all-term match should be tier 6
if !strings.Contains(query, "THEN 6") {
t.Error("query should contain tier 6 for description all-term match")
}
}
func TestBuildSearchQuery_SingleTermNoAllTermTiers(t *testing.T) {
query, _ := buildSearchQuery("html", []string{"html"}, 0, false, false)
// Extract the rank CASE expression (ends with "ELSE 9 END") to avoid
// false matches against statusRank which also contains THEN 4/6.
rankEnd := strings.Index(query, "ELSE 9 END")
if rankEnd == -1 {
t.Fatal("query should contain rank expression with ELSE 9 END")
}
rankExpr := query[:rankEnd]
// Single-term queries should NOT have tier 4 (title all-terms), 6 (desc all-terms), or 8 (comment all-terms)
if strings.Contains(rankExpr, "THEN 4") {
t.Error("single-term query should not have tier 4 (title all-terms)")
}
if strings.Contains(rankExpr, "THEN 6") {
t.Error("single-term query should not have tier 6 (description all-terms)")
}
if strings.Contains(rankExpr, "THEN 8") {
t.Error("single-term query should not have tier 8 (comment all-terms)")
}
}
// TestBuildSearchQuery_CommentSubqueryWorkspaceScope regressions the
// MUL-4059 fix: every EXISTS / correlated subquery over `comment` MUST
// filter by c.workspace_id = $wsParam. Without this, Postgres rewrites
// the correlated subquery into a hashed subplan that materializes every
// comment in the entire table matching the LIKE — on prd this was
// 536k rows / 32.3 s for '%search%'. With the filter the hashed set
// collapses to this workspace's comments and the plan uses the
// idx_comment_workspace supporting btree.
//
// $4 is buildSearchQuery's canonical workspace_id placeholder (the
// caller writes wsUUID into args[3] before executing).
func TestBuildSearchQuery_CommentSubqueryWorkspaceScope(t *testing.T) {
singleQuery, _ := buildSearchQuery("html", []string{"html"}, 0, false, false)
// Every occurrence of `FROM comment c` must be followed by the
// c.workspace_id = $4 constraint. Counting is safer than a single
// substring check because the WHERE, rank CASE, matched_comment_content
// subqueries all touch `comment` and must each carry the filter.
fromCount := strings.Count(singleQuery, "FROM comment c")
scopedCount := strings.Count(singleQuery, "c.workspace_id = $4")
if fromCount == 0 {
t.Fatalf("single-term query has no comment subquery — did buildSearchQuery drop it?")
}
if scopedCount < fromCount {
t.Errorf("single-term query has %d comment subqueries but only %d workspace_id filters — %d unscoped subquery(ies) will trigger the MUL-4059 global-hash plan",
fromCount, scopedCount, fromCount-scopedCount)
}
// Multi-term uses one extra comment subquery in the WHERE and one in
// the rank CASE for the all-terms match — same invariant applies.
multiQuery, _ := buildSearchQuery("foo bar", []string{"foo", "bar"}, 0, false, false)
fromCountMulti := strings.Count(multiQuery, "FROM comment c")
scopedCountMulti := strings.Count(multiQuery, "c.workspace_id = $4")
if scopedCountMulti < fromCountMulti {
t.Errorf("multi-term query has %d comment subqueries but only %d workspace_id filters",
fromCountMulti, scopedCountMulti)
}
}