Skip to content

shortest_walks cannot produce a witness for a flow flows_to_* confirms (all four languages) #446

Open

Description

@sinha108

Describe the bug

cldk/analysis/commons/graphs.py::shortest_walks cannot return a witness for a flow that
flows_to_argument confirms exists. Same application, same source, same sink:

accessor depth=None goes through
flows_to_argument True in 0.9 s reach
taint / paths_between abandoned after 180 s shortest_walks

For a triage surface this is the worst failure shape: the caller is told a flow exists and then
cannot obtain the evidence for it. taint()'s whole purpose is to produce the witness that
flows_to_* only asserts.

Affects all four languages — shortest_walks backs paths_between and taint on Python,
Java, TypeScript and Go. Pre-existing (lifted to commons in leg 2.5b); not introduced by #444.

To Reproduce

Needs a real application; the committed fixtures have path distances around 3, which is why this
has never shown up in the suite.

# the analyzer's own eval corpus, pinned
git -C codeanalyzer-go checkout f8f6db8  # nats-server pin, see scripts/eval_l3.sh
EVAL_SRC=/tmp/eval bash scripts/eval_l3.sh --prepare nats-server
cango -i /tmp/eval/nats-server -a 4 --app-name nats-server --namespace nats-server -o /tmp/l4

Then, over that payload:

b = GoCodeanalyzer(project_dir=None, cache_dir=..., analysis_level="system_dependency_graph",
                   application_name="nats-server", namespace="nats-server")

b.flows_to_argument(src, sink_sig, port, within=src_sig)            # True, 0.9 s
b.taint(sources=[(src, src_sig)], sinks=[(port, sink_sig)])         # does not return
b.taint(sources=[(src, src_sig)], sinks=[(port, sink_sig)], depth=12)  # 0.0 s, paths=0

Expected behavior

taint / paths_between return the witness, in time comparable to the predicate that confirms
the same flow. A bound should be an optimization, not the only usable mode.

Root cause

The depth-first replay's only pruning is dist.get(d) == len(walked) + 1
(commons/graphs.py, in shortest_walks' inner walk). That keeps every step on a BFS layer
boundary, so each explored path has exactly the target length — but it never restricts the search
toward dst. The replay therefore enumerates shortest paths to every node at the target
distance, and only the ones that happen to end at dst are counted.

Measured on the nats-server pair above (1,693,330 indexed nodes, 6,761,391 adjacency pairs):

distance from src to dst 42 hops
nodes at distance exactly 42 110,833 (dst is one of them)
shortest paths src → distance 42 14,942,058,556,479,936
shortest paths src → dst 736,848
over-exploration factor ~2 × 10¹⁰

The forward BFS that builds dist is fine — it finds dst at distance 42 in 1.1 s. The cost is
entirely in the replay.

Suggested fix

Standard bidirectional pruning: run a backward BFS from dst over the reversed adjacency to get
dist_bwd, then in the replay step only to d where dist_fwd[d] + dist_bwd[d] == target. That
restricts the DFS to nodes lying on some shortest src→dst path, which is exactly the set the
caller asked for.

Why this is not a small patch

Changing which walks are explored touches three published contracts, each of which needs
re-verification rather than assumption:

  • "shortest, in order" — max_paths is documented as a prefix of a total order
    (hop_sort_key), not an arbitrary subset. The pruned replay must emit the same prefix.
  • sanitizer cuts — allow_edge / allow_node are applied inside steps() so the breadth-first
    pass measures the shortest satisfying distance. A backward BFS has to apply the same predicates,
    or the two directions disagree about which graph they are searching and a sanitized route could
    be pruned as unreachable, producing a false refutation.
  • both backends — the Neo4j side answers the same question with allShortestPaths in Cypher
    and is unaffected, so the parity tests must still pass afterwards. Divergence here is a
    correctness regression, not a performance one.

Additional context

Found while evaluating #444 (Go L3/L4, PR for #436) against the analyzer's real-app corpus. That
evaluation also turned up two defects now fixed in #444, both invisible on the committed fixtures
for the same reason as this one:

  • _reach expanded each node once per predecessor rather than once, so the frontier tracked path
    count: 151,417,783 expansions at depth 8 against 670,450, and unbounded it did not terminate.
    Fixed and lifted into commons.graphs.reach, so all four languages share one walk.
  • split_body_node_id re-sorted its whole candidate list on every call, making a bulk get_source
    over a page's endpoints sort 5,955 ids per endpoint on nats-server.

A related scale note, relevant to how urgent this is: on nats-server an unbounded reach from a
typical port touches 1,631,237 of 1,693,330 nodes — 96% of the graph. The slices default to
DEFAULT_DEPTH = 5 and are unaffected, but paths_between, flows_to_* and taint default to
depth=None — correct for soundness, since a bound makes a False ambiguous, but it means the
default query on a large application is inherently whole-graph. The fix above does not change that;
it changes the replay from exploring 10¹⁶ paths to exploring the ones that can reach the sink.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    fixBug fixesperformancePerformance updates

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions