Skip to content

search: bound Python regex CPU per request (#38) - #100

Merged
IceRhymers merged 1 commit into
integration/search-hardening-and-ci-integrityfrom
feat/38-match-budget
Jul 24, 2026
Merged

search: bound Python regex CPU per request (#38)#100
IceRhymers merged 1 commit into
integration/search-hardening-and-ci-integrityfrom
feat/38-match-budget

Conversation

@IceRhymers

Copy link
Copy Markdown
Owner

Summary

Grep's Python-side highlight rescan (extract_line_matches in app/search/grep.py) previously
ran user-supplied Substring/Regex patterns through stdlib re with no CPU/wall-clock bound.
Because stdlib re never releases the GIL and cannot be interrupted mid-call, a
catastrophic-backtracking pattern on a single under-cap file could hang not just its own
request but the whole process's event loop (/health, /ready, every concurrent request on
both the MCP app and webui) — reachable from the web-exposed webui /api/search endpoint and
the MCP search_code tool since #35.

This PR closes that gap per the Critic-approved plan for #38:

  • Switches grep's compile/match path from stdlib re to the third-party regex module
    (regex>=2021.11.10, re-compatible superset, still not RE2). regex supports a timeout=
    kwarg that is checked periodically inside the match loop (genuinely interruptible, unlike
    re) and releases the GIL while matching str, so a budgeted pathological match no longer
    starves the event loop.
  • Adds a new per-request wall-clock match budget: Settings.match_budget_ms (default
    2000, env CODE_SEARCH_MATCH_BUDGET_MS), threaded through grep_search as a single
    deadline covering all pattern × line × file matching in the request.
  • A trip stops scanning, keeps every fully-scanned file's matches, and surfaces a recoverable
    partial result: truncated=True, truncation_reason="match_budget" — no envelope keys
    added/removed, existing pinned-key test unchanged.
  • Cursor/pagination semantics: a mid-file trip treats that file as consumed (its partial
    matches are discarded) so pagination steps past a pathological file instead of stalling
    forever on it; a pre-file trip leaves the file unconsumed so a resumed page re-fetches it
    fresh with a new budget.
  • Review-driven fix beyond the base plan: independent code review (below) found that a
    pre-file trip on the very first row of a resumed page — reachable via content-fetch
    latency, not just an undersized budget, since per-row yield_per=1 fetch time also falls
    inside the deadline window — left next_cursor=None while truncated=True, indistinguishable
    from "exhausted" and silently dropping the rest of the corpus. Fixed by falling back to the
    unchanged incoming resume cursor in that case, with a dedicated deterministic regression
    test (test_match_budget_pre_file_trip_on_first_row_of_resumed_page_falls_back_to_resume_cursor,
    using a negative match_budget_ms so the trip is guaranteed rather than timing-dependent).
  • app/search/errors.py's regex_invalid/RegexInvalidError handling (Postgres-invalid regex,
    issue search: negated (and un-negated) Postgres-invalid regex surfaces as an unhandled 500-class fault #75, merged as part of this branch's rebase) is untouched — out of scope for search: bound Python regex CPU per request (web-exposed ReDoS) #38.

Review

  • Independent code review: one actionable MEDIUM finding (the pre-file-trip-on-first-row
    cursor dead-end above) — fixed, with a regression test. A few LOW/informational notes (some
    incidental uv export transitive dependency churn in app/requirements.txt; no floor on
    match_budget_ms, consistent with existing unvalidated statement_timeout_ms/
    max_content_bytes) were reviewed and left as-is, matching repo convention.
  • Independent security review: verdict LOW risk, "the DoS mitigation genuinely closes the
    gap, no exploitable regression found" — empirically verified against the installed regex
    version that the exemplar pattern actually catastrophically backtracks unbudgeted, that
    timeout= interrupts it, and that the GIL is released during matching (event loop stays
    responsive under a full-budget pathological match). One LOW suggestion (pin a regex version
    floor) was applied (regex>=2021.11.10, the point at which timeout= support is available).
    Residual CPU-cost risk (match_budget_ms × CapacityLimiter(5) slots per volley, no rate
    limiting) is accepted and documented — availability (not throughput) is what this fix
    guarantees, matching the plan's stated scope.

Gates (all run fresh, locally)

  • make lint (ruff check + ruff format --check + mypy app indexer webui) — clean.
  • make test (unit + observability) — 1082 passed.
  • make test-integration (integration + e2e, against local codesearch-pg Postgres) —
    190 passed, 8 failed, 41 errors, 3 xfailed, 2 xpassed. The 8 failures / 41 errors are
    pre-existing and environmental, confirmed identical against the unmodified base branch:
    they require Lakebase-only extensions (lakebase_ann/lakebase_bm25/lakebase_tokenizer)
    this local vanilla-pgvector Postgres doesn't have (test_semantic_rrf, test_migrations
    0004+, test_reconcile, test_store_chunk_writer, test_webui_semantic), or need
    provisioned PG roles not present locally (test_commit_search, test_mcp_server). All
    plain-Postgres grep/service suites this PR touches run and pass in full:
    tests/integration/test_grep.py42 passed (all pre-existing tests + this PR's 3 new
    match-budget tests, including the review-driven regression test above), plus #75's
    regex_invalid-flavored grep tests, unaffected.
  • CI's Lakebase workflow (ci-lakebase.yml) currently no-ops repo-wide
    (CI_LAKEBASE_ENABLED unprovisioned — known state since PR indexer: extend typed reference edges to JS/TS/TSX/Go/Java/Rust (#85) #92); ci.yml
    (lint/unit/webui) is the CI check expected to be green on this PR.

Refs

Refs #38

Switch grep's highlight rescan from stdlib re to the regex module and
thread a per-request wall-clock match budget (default 2000ms,
CODE_SEARCH_MATCH_BUDGET_MS) through grep_search. A trip stops
scanning, keeps fully-scanned files, and surfaces truncated=True +
truncation_reason="match_budget" instead of stalling the event loop on
a catastrophic-backtracking pattern.
@IceRhymers
IceRhymers merged commit f27565b into integration/search-hardening-and-ci-integrity Jul 24, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant