fix(kg-extract): drop ReDoS-prone regex in _parse_json_blob - #206
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical performance issue in the KG extraction worker where a regex-based JSON parser was causing catastrophic backtracking on malformed inputs. By replacing the regex with a deterministic, linear-time bracket-counting scanner, the fix eliminates event loop freezes and significantly improves throughput in production environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request replaces a regex-based JSON array extraction with a linear, bracket-counting scanner (_scan_balanced_array) to prevent catastrophic backtracking and event loop freezes on pathological inputs. The feedback suggests enhancing this scanner and the parser to support a start_pos parameter and a retry loop. This ensures that if the LLM output contains leading bracketed prose (e.g., [draft]) before the actual JSON array, the parser can skip the invalid candidate and successfully extract the valid JSON array.
| candidate = _scan_balanced_array(raw_stripped) | ||
| if candidate is None: | ||
| logger.debug("no JSON array found in LLM response: %r", raw_stripped[:200]) | ||
| return [] | ||
| try: | ||
| parsed = json.loads(match.group(0)) | ||
| parsed = json.loads(candidate) | ||
| except (json.JSONDecodeError, ValueError) as e: | ||
| logger.debug("regex-extracted JSON failed to parse: %s", e) | ||
| logger.debug("scanned JSON failed to parse: %s", e) | ||
| return [] |
There was a problem hiding this comment.
If the LLM output contains a false start or bracketed prose (e.g., Here is the [draft] JSON: [...]), the current implementation will extract the first balanced bracketed substring ([draft]), fail to parse it as JSON, and immediately return [], ignoring the actual JSON array that follows. Implementing a loop to try subsequent [ candidates ensures the parser successfully extracts the valid JSON array even in the presence of leading bracketed prose.
| candidate = _scan_balanced_array(raw_stripped) | |
| if candidate is None: | |
| logger.debug("no JSON array found in LLM response: %r", raw_stripped[:200]) | |
| return [] | |
| try: | |
| parsed = json.loads(match.group(0)) | |
| parsed = json.loads(candidate) | |
| except (json.JSONDecodeError, ValueError) as e: | |
| logger.debug("regex-extracted JSON failed to parse: %s", e) | |
| logger.debug("scanned JSON failed to parse: %s", e) | |
| return [] | |
| start_pos = 0 | |
| while True: | |
| candidate = _scan_balanced_array(raw_stripped, start_pos) | |
| if candidate is None: | |
| logger.debug("no JSON array found in LLM response: %r", raw_stripped[:200]) | |
| return [] | |
| try: | |
| parsed = json.loads(candidate) | |
| break | |
| except (json.JSONDecodeError, ValueError) as e: | |
| logger.debug("scanned JSON failed to parse: %s", e) | |
| current_start = raw_stripped.find("[", start_pos) | |
| start_pos = current_start + 1 |
| def _scan_balanced_array(s: str) -> Optional[str]: | ||
| """Return the first balanced ``[...]`` substring, respecting JSON strings. | ||
|
|
||
| Linear scan, no regex backtracking. Used when the model leaks prose | ||
| around its JSON output and a strict ``json.loads`` fails. The previous | ||
| regex (``\\[\\s*\\{.*?\\}...\\]`` with re.DOTALL) had catastrophic | ||
| backtracking on long, malformed inputs and could freeze the async | ||
| event loop for seconds at a time. | ||
| """ | ||
| start = s.find("[") | ||
| if start < 0: | ||
| return None |
There was a problem hiding this comment.
To support robust parsing when there are multiple bracketed expressions (e.g., false starts or bracketed prose like [draft] before the actual JSON array), update _scan_balanced_array to accept a start_pos parameter. This allows the parser to scan for subsequent candidates if the first one fails to parse as valid JSON.
| def _scan_balanced_array(s: str) -> Optional[str]: | |
| """Return the first balanced ``[...]`` substring, respecting JSON strings. | |
| Linear scan, no regex backtracking. Used when the model leaks prose | |
| around its JSON output and a strict ``json.loads`` fails. The previous | |
| regex (``\\[\\s*\\{.*?\\}...\\]`` with re.DOTALL) had catastrophic | |
| backtracking on long, malformed inputs and could freeze the async | |
| event loop for seconds at a time. | |
| """ | |
| start = s.find("[") | |
| if start < 0: | |
| return None | |
| def _scan_balanced_array(s: str, start_pos: int = 0) -> Optional[str]: | |
| """Return the first balanced ``[...]`` substring, respecting JSON strings. | |
| Linear scan, no regex backtracking. Used when the model leaks prose | |
| around its JSON output and a strict ``json.loads`` fails. The previous | |
| regex (``\\[\\s*\\{.*?\\}\\s*(?:,\\s*\\{.*?\\}\\s*)*\\]`` with re.DOTALL) had catastrophic | |
| backtracking on long, malformed inputs and could freeze the async | |
| event loop for seconds at a time. | |
| """ | |
| start = s.find("[", start_pos) | |
| if start < 0: | |
| return None |
There was a problem hiding this comment.
Pull request overview
This PR hardens KG LLM response parsing by removing a ReDoS-prone regex from _parse_json_blob and replacing it with a linear-time bracket-counting scanner, with new regression tests to prevent performance regressions.
Changes:
- Replaced
_JSON_ARRAY_PATTERNregex extraction with_scan_balanced_arrayto avoid catastrophic backtracking on malformed inputs. - Updated
_parse_json_blobto use the new scanner and adjusted debug messaging accordingly. - Added two regression tests covering pathological input performance and bracket handling inside JSON strings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mempalace/kg_llm_extractor.py |
Removes ReDoS-prone regex parsing and introduces a linear balanced-array scanner used by _parse_json_blob. |
tests/test_kg_extractor.py |
Adds regression tests for the new parsing behavior and prior performance failure mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| start = s.find("[") | ||
| if start < 0: | ||
| return None | ||
| depth = 0 | ||
| in_string = False |
| raw = "[" + "{" * 5000 + " no closing" | ||
| t0 = time.monotonic() | ||
| out = _parse_json_blob(raw) | ||
| elapsed = time.monotonic() - t0 | ||
| assert out == [] | ||
| assert elapsed < 0.1, f"parser took {elapsed:.3f}s on pathological input" | ||
|
|
||
|
|
…nPool Replaces psycopg2 + asyncio.Lock with psycopg_pool.AsyncConnectionPool on the KG triple-extraction hot path. Each coroutine now claims its own connection so writes can overlap with the N LLM calls feeding them; the previous single-shared-connection design serialised every postgres write after PR #206 raised LLM throughput ~10x. Worker changes (mempalace/kg_triple_worker.py): - _SyncConnPool retains its name but wraps psycopg_pool.AsyncConnectionPool internally; `configure=` callback runs LOAD 'age' + SET search_path once per fresh connection (was per-cursor before). - _KGHandle.add_triple is async; no asyncio.Lock anywhere. - Pool sized min=max(4, max_concurrency // 2), max=max_concurrency + 2 so steady-state idle capacity is modest but bursts can keep all LLM writers concurrent with the queue claim loop. - Async variants of _claim_batch, _fetch_drawer_text, _mark_completed, _mark_error, _seed_backfill live alongside the sync versions which the CLI --status flag still uses. Driver swap elsewhere is mechanical (`import psycopg as psycopg2`) so the existing test monkeypatch surface keeps working: - backends/postgres.py, knowledge_graph_age.py: _load_psycopg2 still returns the driver+sql modules (now psycopg3) under the legacy name. - searcher.py, migrate_to_postgres.py, cli.py, kg_writethrough.py: aliased imports + error-message updates. Tests: - tests/test_kg_triple_worker.py fakes upgraded to dual-mode sync/async via _NoopAwaitable + _SyncResult helpers — same fakes back both the AsyncConnectionPool hot path and the kept-sync CLI status path. - _FakeKG.add_triple is async with the worker's predicate-positional shape; kg_factory test seam takes the pool, not the dsn. - All 14 kg_triple_worker tests pass; full suite: 3157 passed (up from 3151), 38 pre-existing failures unrelated to psycopg (CLI search output, daemon routing, readme parity, source-adapter entry-point ordering). pyproject.toml: psycopg lines only — psycopg[binary]>=3.2,<4 and psycopg-pool>=3.2,<4 in both [postgres] and [kg-extract] extras. References: #206 (LLM-blob ReDoS fix that made the lock binding).
…nPool (#208) * perf(kg-extract): migrate KG triple worker to psycopg3 AsyncConnectionPool Replaces psycopg2 + asyncio.Lock with psycopg_pool.AsyncConnectionPool on the KG triple-extraction hot path. Each coroutine now claims its own connection so writes can overlap with the N LLM calls feeding them; the previous single-shared-connection design serialised every postgres write after PR #206 raised LLM throughput ~10x. Worker changes (mempalace/kg_triple_worker.py): - _SyncConnPool retains its name but wraps psycopg_pool.AsyncConnectionPool internally; `configure=` callback runs LOAD 'age' + SET search_path once per fresh connection (was per-cursor before). - _KGHandle.add_triple is async; no asyncio.Lock anywhere. - Pool sized min=max(4, max_concurrency // 2), max=max_concurrency + 2 so steady-state idle capacity is modest but bursts can keep all LLM writers concurrent with the queue claim loop. - Async variants of _claim_batch, _fetch_drawer_text, _mark_completed, _mark_error, _seed_backfill live alongside the sync versions which the CLI --status flag still uses. Driver swap elsewhere is mechanical (`import psycopg as psycopg2`) so the existing test monkeypatch surface keeps working: - backends/postgres.py, knowledge_graph_age.py: _load_psycopg2 still returns the driver+sql modules (now psycopg3) under the legacy name. - searcher.py, migrate_to_postgres.py, cli.py, kg_writethrough.py: aliased imports + error-message updates. Tests: - tests/test_kg_triple_worker.py fakes upgraded to dual-mode sync/async via _NoopAwaitable + _SyncResult helpers — same fakes back both the AsyncConnectionPool hot path and the kept-sync CLI status path. - _FakeKG.add_triple is async with the worker's predicate-positional shape; kg_factory test seam takes the pool, not the dsn. - All 14 kg_triple_worker tests pass; full suite: 3157 passed (up from 3151), 38 pre-existing failures unrelated to psycopg (CLI search output, daemon routing, readme parity, source-adapter entry-point ordering). pyproject.toml: psycopg lines only — psycopg[binary]>=3.2,<4 and psycopg-pool>=3.2,<4 in both [postgres] and [kg-extract] extras. References: #206 (LLM-blob ReDoS fix that made the lock binding). * fix(ci): update test-postgres workflow to psycopg3 + apply ruff format CI on PR #208 failed two checks: 1. `ruff format --check` flagged three migration-touched files (mempalace/cli.py, mempalace/kg_triple_worker.py, mempalace/migrate_to_postgres.py). All three are cosmetic line-wrap diffs from string literals that exceeded the line limit after the driver-rename swap. Ran `ruff format` to fix. 2. .github/workflows/ci.yml's test-postgres job had an inline `python - <<'PY' ... import os, psycopg2 ...` step that creates the pgvector extension before pytest runs. Since psycopg2-binary is no longer in [postgres], this failed with ModuleNotFoundError. Swapped to `import os, psycopg` + the matching `psycopg.connect(...)`. Sync API is identical (autocommit attribute + cursor().execute()) so no other changes needed. Real connection is still exercised — the step continues to create the vector extension against the live pgvector/pgvector:pg16 service container. * fix(ci): swap psycopg2 alias in postgres backend test + regen API docs Two CI failures on PR #208: 1. test-postgres: tests/test_backends_postgres.py:129 still did a literal `import psycopg2`. With psycopg2 removed from [postgres], the import raises ModuleNotFoundError before any assertion runs. Swap to `import psycopg as psycopg2` — the rest of the test body (connect, autocommit, cursor, execute, fetchone) is psycopg3-compatible. 2. check-docs: website/reference/python-api/{kg_triple_worker,kg_writethrough}.md were stale after the worker rewrite (714a930) and the writethrough docstring update. Regenerated via scripts/render-api-docs.py. Worker unit tests still 14/14 green. The other test files that import psycopg2 (test_migrate_to_postgres.py, test_palace_graph.py) are not exercised by PR #208's test-postgres job — keeping this commit scoped to the actually-failing surface. * docs(ci): regen API docs + llms-full.txt after rebase After rebasing onto d2ef152 (PR #209), check-docs flagged two drifts — both downstream of #209's content, picked up here so #208's CI goes green on its own: - website/reference/python-api/sources/registry.md: documents the new reset_discovery() function added in #209's entry-point-pollution fix. - website/public/llms-full.txt: one-line shield bump from #209's README version-badge update (3.3.5 -> 3.3.6 to match version.py). Re-ran `scripts/render-api-docs.py` (81 files, 1 changed) and `scripts/render-llms-full.py` (73417 bytes, 1 line changed). No mempalace/ source touched — pure rendered-artifact sync.
…nner
The `_JSON_ARRAY_PATTERN` regex `\[\s*\{.*?\}\s*(?:,\s*\{.*?\}\s*)*\]`
with `re.DOTALL` exhibits catastrophic backtracking on long pathological
inputs. The non-greedy `.*?` inside the repeating non-capturing group
forces the engine to retry every combination of where each `}` could
match — exponential on malformed responses where the model leaked
unbalanced braces.
Production symptom (observed via `py-spy dump`): MainThread blocked in
`_parse_json_blob` for seconds at a time, freezing the async event loop
on the KG extraction worker. GPU utilization stayed near 0% on both
extractor hosts because the workers couldn't drain LLM responses.
Replace with `_scan_balanced_array`: a linear bracket-counting scanner
that walks the input once, tracks string-literal context (so a `[`
inside a JSON string doesn't unbalance the count), and returns the
first balanced `[...]` substring. O(n) time, no backtracking, no
event-loop stalls.
Tests added in `tests/test_kg_extractor.py`:
- `test_parse_json_blob_no_redos_on_pathological_input` — pumps
`"[" + "{" * 5000` through the parser and asserts completion in
under 100ms (the old regex hung indefinitely on this input).
- `test_parse_json_blob_brackets_inside_strings` — confirms the
string-aware scanner returns the right boundary when a `[` appears
inside a JSON string value.
All 30 tests in `tests/test_kg_extractor.py` pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The ReDoS fix added two regression tests (no-redos on pathological input, brackets inside strings), bumping the suite from 3232 to 3234.
319768f to
3c8c5ac
Compare
Summary
_JSON_ARRAY_PATTERNregex (catastrophic backtracking on.*?inside(?:...)*withre.DOTALL) with_scan_balanced_array, a linear bracket-counting scanner that respects JSON string literals.tests/test_kg_extractor.pylock in the fix.Root cause
The non-greedy
.*?inside the repeating non-capturing group(?:,\s*\{.*?\}\s*)*made the engine retry every combination of where each}could match. On long, unbalanced responses (e.g. a model dump truncated mid-object) the worst-case is exponential in input length.Production symptom
py-spy dumpagainst the KG extraction worker showed the MainThread blocked in_parse_json_blobfor seconds at a time, freezing the async event loop. Both extractor hosts (katana 2080 Ti and familiar P102) sat at ~0% GPU because the workers couldn't drain LLM responses fast enough to feed continuous-batching.After the fix:
The fix
_scan_balanced_arraywalks the input once:[inside a JSON string value doesn't unbalance the depth counter[...]substring, orNoneif no balanced bracket existsO(n) time, no backtracking, no event-loop stalls.
Tests
test_parse_json_blob_no_redos_on_pathological_input— pumps"[" + "{" * 5000through the parser and asserts completion in under 100ms. The old regex hung indefinitely on this input.test_parse_json_blob_brackets_inside_strings— confirms the string-aware scanner returns the right boundary when a[appears inside a JSON string value (e.g. an object literal containing"[draft]").All 30 tests in
tests/test_kg_extractor.pypass in 0.10s.Test plan
tests/test_kg_extractor.pylocally — 30/30 passfamiliar.jphe.in— GPU util jumped from ~0% to ~43%_parse_json_blobhappy-path shapes (clean array, object-wrapping-triples, prose leakage)🤖 Generated with Claude Code