From 130af9ac7bb85945e205535fe1a06e388e530177 Mon Sep 17 00:00:00 2001 From: Albert Lionelle Date: Tue, 9 Jun 2026 17:42:47 -0600 Subject: [PATCH] fix(degree): bound DNF expansion so render_plan_graph can't hang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/core/prerequisite_parser.rs | 72 +++++++++++++++++++++++++++++++-- src/mcp/tools/plan_graph.rs | 60 +++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 4 deletions(-) diff --git a/src/core/prerequisite_parser.rs b/src/core/prerequisite_parser.rs index c94d37d..d7322a5 100644 --- a/src/core/prerequisite_parser.rs +++ b/src/core/prerequisite_parser.rs @@ -394,6 +394,16 @@ pub fn extract_all_courses(raw: &str) -> Vec { // ============================================================================ /// Recursively parse prerequisite expression into DNF +/// Hard cap on the number of DNF paths produced for a single prerequisite +/// expression. DNF is an OR-of-ANDs, so an `(A|B) & (C|D) & …` expression with +/// `n` two-way OR groups expands to `2^n` paths — a pathological scrape (~30 +/// groups) is a billion `Vec`s and effectively hangs the caller (this is the +/// `render_plan_graph` "Tool execution failed" timeout). No real prerequisite +/// has anywhere near this many genuinely distinct satisfying paths, so we cap +/// the expansion: every kept path is still complete and valid (it includes a +/// course from each AND part), we just stop enumerating further alternatives. +const MAX_DNF_PATHS: usize = 1024; + fn parse_dnf_recursive(s: &str) -> Vec> { let trimmed = s.trim(); if trimmed.is_empty() { @@ -407,8 +417,11 @@ fn parse_dnf_recursive(s: &str) -> Vec> { let or_parts = split_at_level(unwrapped, '|'); let mut result = Vec::new(); for part in or_parts { - let part_dnf = parse_dnf_recursive(part.trim()); - result.extend(part_dnf); + result.extend(parse_dnf_recursive(part.trim())); + if result.len() >= MAX_DNF_PATHS { + result.truncate(MAX_DNF_PATHS); + break; + } } return result; } @@ -424,13 +437,18 @@ fn parse_dnf_recursive(s: &str) -> Vec> { continue; } - // Cartesian product + // Cartesian product, bounded at MAX_DNF_PATHS so a pathological + // AND-of-ORs can't explode. Stop pushing once the cap is hit; the + // paths kept so far each still span every AND part processed. let mut new_paths = Vec::new(); - for existing in ¤t_paths { + 'product: for existing in ¤t_paths { for new_part in &part_dnf { let mut combined = existing.clone(); combined.extend(new_part.clone()); new_paths.push(combined); + if new_paths.len() >= MAX_DNF_PATHS { + break 'product; + } } } current_paths = new_paths; @@ -898,4 +916,50 @@ mod tests { // Should extract courses even with special chars assert!(!result.is_empty()); } + + #[test] + fn test_dnf_bounds_pathological_and_of_ors() { + // `(A0|B0) & (A1|B1) & … & (A29|B29)` is 2^30 ≈ 1.07e9 DNF paths + // unbounded — the exact explosion that hung render_plan_graph. The cap + // must keep this fast and bounded. (This test running at all, rather + // than hanging/OOMing, is the regression guard.) + const GROUPS: usize = 30; + let expr = (0..GROUPS) + .map(|i| format!("(A{i}|B{i})")) + .collect::>() + .join(" & "); + + let paths = parse_to_dnf(&expr); + + assert!( + paths.len() <= MAX_DNF_PATHS, + "DNF expansion must be capped at {MAX_DNF_PATHS}, got {}", + paths.len() + ); + assert!( + !paths.is_empty(), + "a satisfiable expression must yield paths" + ); + // Every kept path must still be complete — one course from each of the + // 30 AND groups — so a capped path remains a valid satisfying set. + for path in &paths { + assert_eq!( + path.len(), + GROUPS, + "each kept path must span every AND group: {path:?}" + ); + } + } + + #[test] + fn test_dnf_bounds_wide_or() { + // A degenerate wide OR (`X0 | X1 | … | X4999`) must also stay bounded. + let expr = (0..5000) + .map(|i| format!("X{i}")) + .collect::>() + .join(" | "); + let paths = parse_to_dnf(&expr); + assert!(paths.len() <= MAX_DNF_PATHS, "wide OR must be capped"); + assert!(!paths.is_empty()); + } } diff --git a/src/mcp/tools/plan_graph.rs b/src/mcp/tools/plan_graph.rs index 6c5961e..2b27dd2 100644 --- a/src/mcp/tools/plan_graph.rs +++ b/src/mcp/tools/plan_graph.rs @@ -649,4 +649,64 @@ courses: // The shared GRAPH_VANILLA_JS prelude marker must be absent. assert!(!html.contains("window.nuGraphs =")); } + + #[test] + fn test_render_does_not_hang_on_pathological_prerequisites() { + // Regression for the field-report "render_plan_graph → Tool execution + // failed" hang: an in-plan course whose prerequisites_raw is a wide + // AND-of-ORs makes build_edges_from_courses → parse_to_dnf explode + // (2^30 paths) and the tool times out. The expression resolves to just + // CS101 (so the plan is valid and CS201 is schedulable) but the raw + // string still drives the unbounded expansion. With the DNF cap this + // returns promptly; without it, this test hangs. + let groups = vec!["(CS101|CS101)"; 30].join(" & "); + let yaml = format!( + r#" +degree: + id: pathological + institution: Test University + program: Test Program + total_credits: 8 + gpa_minimum: 2.0 + major_subjects: ["CS"] + +requirements: + intro: + name: Intro + type: all + category: major + courses: [CS101, CS201] + +courses: + CS101: + title: Intro CS + prefix: CS + number: "101" + credits: 4 + CS201: + title: Data Structures + prefix: CS + number: "201" + credits: 4 + prerequisites_raw: "{groups}" +"# + ); + + let response = execute( + &yaml, + Some("shortest"), + None, + None, + VisualizationFormat::Standalone, + Some(10), + None, + false, + ); + assert!( + response.success, + "render must stay bounded on a pathological prereq: {:?}", + response.error + ); + assert!(response.node_count.is_some_and(|n| n > 0)); + } }