fix: linear-time depends_on cycle check (three-colour DFS, both runtimes) - #88
Conversation
…ntimes) The recursion-stack-only DFS re-walked shared subgraphs once per path, so a lattice of stacked diamonds cost 2^N walks (~97s at 28 levels) before the first model call. Track fully-explored nodes and skip them: the check is now linear, diamond DAGs stay legal, and true cycles are still caught. New conformance case depends-on/deep-lattice (2^40 paths) holds both runtimes to it under the 60s adapter timeout.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
aswhitehouse
left a comment
aswhitehouse
left a comment
There was a problem hiding this comment.
Reviewed — the algorithm is correct and this is a real bug worth fixing before 1.6.0. Three-colour DFS is the textbook answer and it's applied identically in both runtimes; the stack-before-done check ordering is right, so true cycles (back edges to the current path) are still caught and completed subgraphs are provably safe to skip. The 40-level lattice unit test and the conformance tripwire are exactly the right way to hold both runtimes to it.
Two inline comments (one test nit, one pre-existing hardening note — neither blocking) plus one sequencing thought:
Merge order with #87: rather than landing 1.6.0 and immediately having an orphaned [Unreleased] fix, I'd land this PR first and roll the changelog entry into the 1.6.0 block — an exponential pre-flight hang is a good fix to ship in the release rather than the one after. You flagged the same thing in the PR body; happy either way as long as the entry ends up in the right section.
Minor observation, no action needed: the deep-lattice conformance case regresses as a 60s adapter timeout (reported 💥 adapter error) rather than a ❌ FAIL. That's an acceptable tripwire — just worth knowing when reading a future matrix.
Move the lattice back-edge from a4 to b4 in test_cycle_through_lattice_still_detected. With the edge on a4 the cycle was found on the first DFS descent before any node entered the done-set, so the test did not actually exercise the property it names. Looping the b-side base instead lets the a-branch mark a4 done first, so the cycle is detected through b4 after a node has been marked done. Addresses review feedback from @aswhitehouse on #88. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XdPprn1xTRiR1rGZf4jUfP
test: exercise done-set through b4 back-edge in lattice cycle test
3 participants
Problem
depends_oncycle detection in both runtimes uses a recursion-stack-only DFS. That's correct, but it re-walks a shared subgraph once per path that reaches it — on a lattice of stacked diamonds that's 2^N walks before the first model call. Measured on the Python runner's algorithm: 0.4s at 20 levels, 97s at 28, ~4.5 days at 40. All spent validating a graph with fewer than 100 tasks.Fix
Textbook third colour: alongside the recursion stack ("gray"), track fully-explored nodes ("black") and skip them on re-encounter. The check becomes linear in graph size. Semantics are unchanged:
depends-on/diamond-graphcase),Applied identically to
oas_cli/runner.py::_check_cyclesandnpm/src/runner.ts::checkChainCycles.Coverage
test_deep_lattice_cycle_check_is_linear(40-level lattice — would hang pre-fix) andtest_cycle_through_lattice_still_detected(done-set must not mask real cycles).depends-on/deep-lattice(2^40 paths): any runtime that regresses blows the harness's 60s adapter timeout. Matrix regenerated — 33/33 Python, 31+2 unsupported npm, both PASS the new case.Gates
pytest 505 passed / ruff / mypy clean;
tscclean; full conformance suite green on both adapters.Note: changelog entry sits under
[Unreleased]— if this lands after #87, it belongs in a fresh Unreleased section for the next release.