Skip to content

fix(count_paths): walk an explicit stack so deep graphs do not overflow - #815

Open
tachsin wants to merge 1 commit into
evenfurther:mainfrom
tachsin:fix/count-paths-stack-overflow
Open

fix(count_paths): walk an explicit stack so deep graphs do not overflow#815
tachsin wants to merge 1 commit into
evenfurther:mainfrom
tachsin:fix/count-paths-stack-overflow

Conversation

@tachsin

@tachsin tachsin commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Closes #814.

cached_count_paths recursed once per node, so the depth of the recursion was the length of the longest path. A chain of 200 000 nodes overflows the stack, although that graph has exactly one path and no loop in it anywhere.

Walks an explicit stack instead. The nodes are held in an FxIndexMap that the stack refers to by index, which keeps T: Clone off the bounds and folds the node's state into the same lookup that reads its count.

Worth reviewing

This is the decision raised in #814. Once the recursion is gone there is no stack left to overflow, so an undetected loop would grow the working set until memory ran out — a worse failure than the one documented today. A node reached again while its own count is still unknown lies on a loop by definition, so it is recognised at no extra cost and reported as a panic naming the problem.

The documentation changes accordingly: the note that loops overflow the stack becomes a # Panics section. If you would rather it stayed a crash, or returned a Result, say the word and I will change it.

Checking

Counting is unchanged — each node is still expanded once and its count reused. Three tests cover it:

  • a 200 000 node chain run on a deliberately small 1 MiB stack, so a return to a recursive walk fails loudly
  • a 20x20 lattice, checked against C(38, 19) and against an independent dynamic-programming count, asserting that successors is called at most once per node rather than once per path
  • a loop, asserting the panic

The existing grid test and both Advent of Code tests that use count_paths are unaffected.

`cached_count_paths` recursed once per node, so the depth of the recursion was
the length of the longest path. A chain of 200 000 nodes overflows the stack,
and that graph has exactly one path and no loop anywhere in it, although the
documentation implies a loop-free graph is safe.

Walk an explicit stack instead. The nodes are held in an `FxIndexMap` that the
stack refers to by index, which keeps `T: Clone` off the bounds and makes the
node's state part of the same lookup that reads its count.

That state also settles what a loop does now. There is no stack left to
overflow, so an undetected loop would instead grow the working set until memory
ran out, which is a worse failure than the documented one. A node reached again
while its own count is still unknown lies on a loop by definition, so it is
recognised at no extra cost and reported as a panic naming the problem. The
documentation changes accordingly, from a note that loops overflow the stack to
a `# Panics` section.

Counting itself is unchanged: each node is still expanded once and its count
reused, which the added test pins down by counting the calls.
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.

count_paths overflows the stack on deep graphs, loop or no loop

1 participant