Skip to content

fix(degree): bound DNF expansion so render_plan_graph can't hang#13

Merged
lionelle merged 1 commit into
mainfrom
fix/render-plan-graph-hang
Jun 10, 2026
Merged

fix(degree): bound DNF expansion so render_plan_graph can't hang#13
lionelle merged 1 commit into
mainfrom
fix/render-plan-graph-hang

Conversation

@lionelle

@lionelle lionelle commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Second phase of the MCP tester feedback fixes (independent of #12 — touches different files).

Issue 4 — render_plan_graph "Tool execution failed" on sample / plan_index≥2

shortest/longest rendered fine; every other selector failed with a generic error. Investigation showed:

  • The picker (pick_by_category / plan_index) is correct and unit-tested, and render_plan_graph is panic-guarded (catch_unwind) — so a panic would surface as JSON, not "Tool execution failed".
  • All the tester's calls shared one cache key (max_plans=None) → the analysis ran once and succeeded (shortest rendered) → the failure is in per-plan rendering, and the generic message is a client-side timeout on a hang.

Root cause: build_edges_from_courses calls parse_to_dnf on each in-plan course's prerequisite string, and parse_dnf_recursive expands an AND-of-ORs by cartesian product. A pathological (A|B) & (C|D) & … (~30 groups) is 2³⁰ paths and effectively hangs. That course only appears in the electives a sample/calc-ready plan draws — which is exactly why the extremes worked.

Fix

Cap the DNF expansion at MAX_DNF_PATHS = 1024 inside parse_dnf_recursive, bounding both the AND cartesian product and the OR accumulation. Every kept path is still complete (one course per AND group) and valid; we just stop enumerating further alternatives. This protects all three parse_to_dnf callers (render, plan_export, course_graph), not just the render path. No realistic prerequisite has near 1024 genuinely distinct satisfying paths, so existing behavior is unchanged (all prior parser tests still pass).

The plan's optional defense-in-depth render wall-clock guard was not needed: the DNF cap removes the only superlinear step, and the rest of the render is O(nodes+edges), bounded by plan size.

Verification

  • New tests: a 2³⁰-path AND-of-ORs and a 5000-way OR return bounded + fast in 0.04s (would hang unbounded); a render_plan_graph of a plan whose course carries a pathological prerequisites_raw completes in 0.09s.
  • cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, cargo doc -D warnings, cargo test --all-features all green.

🤖 Generated with Claude Code

Issue 4 from the MCP tester report: render_plan_graph returned a generic
"Tool execution failed" for sample / calc-ready / plan_index>=2 selectors while
shortest/longest worked. The picker is correct and panic-guarded, and all the
tester's calls shared one (successful) cached analysis — so the failure was a
client-side timeout on a *hang* during per-plan rendering, not a panic.

Root cause: build_edges_from_courses calls parse_to_dnf on each in-plan course's
prerequisite string, and parse_dnf_recursive expands an AND-of-ORs by cartesian
product. A pathological scrape like (A|B) & (C|D) & ... (~30 groups) is 2^30
paths and effectively hangs. The offending course appears only in the electives
a sample/calc-ready plan draws, which is why the extremes rendered fine.

Fix: cap the DNF expansion at MAX_DNF_PATHS (1024) inside parse_dnf_recursive —
bounding both the AND cartesian product and the OR accumulation. Every kept path
is still complete (one course per AND group) and valid; we just stop enumerating
further alternatives. This protects all three parse_to_dnf callers (render,
plan_export, course_graph), not just the render path. No realistic prerequisite
has anywhere near 1024 genuinely distinct satisfying paths, so existing behavior
is unchanged.

Tests: a 2^30-path AND-of-ORs and a 5000-way OR now return bounded and fast
(would hang unbounded); a render of a plan whose course carries a pathological
prerequisites_raw now completes promptly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lionelle lionelle merged commit 340ec74 into main Jun 10, 2026
4 checks passed
@lionelle lionelle deleted the fix/render-plan-graph-hang branch June 10, 2026 00:11
@lionelle lionelle mentioned this pull request Jun 10, 2026
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