Skip to content

Fix npm crawler oracle flake from symlink-cycle trees - #582

Merged
Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
ci-janitor/npm-oracle-symlink-cycle
Oct 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
ci-janitor/npm-oracle-symlink-cycle

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

test (ubuntu-latest) in CI failed on run 37020762793, attempt 1 (job 110882935639, PR #442 head dce7d1af). The re-run on the same SHA passed:

crawlers::npm_crawler::oracle::tests::walk_without_a_pool_matches_the_sequential_oracle ... FAILED
assertion `left == right` failed: no pool, seed 12: find_by_purls differs under /tmp/.tmpqxLwMU/proj/node_modules

This was the only real test failure among the last 100 CI runs (the other 57 non-green runs were concurrency cancels). It's in a required job, and the only fix available today is a manual re-run of a ~12 min job.

Root cause

The test bug is in the generator, not in either crawler. In Gen::pnpm_store, case 76..=79 ("an entry whose node_modules is a symlink") links .pnpm/<entry>/node_modules to self.nm_dirs.first(). That is usually the importer node_modules holding the store, so the link points back at one of its own ancestors. Both crawlers follow an entry's node_modules link (is_dir follows symlinks), so find_by_purls walked

proj/node_modules/.pnpm/<e>/node_modules/.pnpm/<e>/node_modules/.pnpm/<e>/…

until the OS refused the path (about 40 levels: the Linux symlink limit). Both walkers treat every I/O error as an empty dir, so the assertion ended up comparing where each walker gave up, not what it found. In the failing run the new walker returned 87 copies (40 levels) and the sequential oracle 75 (34 levels). I could not reproduce the early stop locally, so I don't know which errno ended the oracle walk early. The test only depends on it because the tree has a cycle, and the tree should not have one.

The generator already tries to avoid this. The symlinked-scope case says "to a scope living outside the tree, so the followed walk cannot cycle", and the vlt store's symlinked-node_modules case points into scratch. Only the pnpm case missed it.

Seeds that drew the cycle, from dumping every generated tree and checking .pnpm/*/node_modules links that point at an ancestor:

  • randomized_trees_match_the_sequential_oracle (seeds 0..64): seeds 5, 7, 12, 41
  • walk_without_a_pool_matches_the_sequential_oracle (seeds 0..16): seeds 5, 7, 12

Fix

Test-only change in crates/socket-patch-core/src/crawlers/npm_crawler/oracle.rs. The entry's node_modules link now points at a fresh out-of-tree scratch/pnpm-nm<N>/ holding a package, the same way the vlt case does. The followed-symlink shape is still covered, and the walk can no longer depend on OS limits. No assertion changed and no test was removed.

Proof

  • No cycles left: across seeds 0..64, 0 store-entry links point at an ancestor after the change (4 before). 16 of the 64 seeds still generate a symlinked .pnpm/<e>/node_modules, so that shape is still exercised.
  • Stress: the two randomized oracle tests ran 90 times, 6 processes in parallel: 0 failures. All 24 oracle tests pass. The nonempty > 32 check still holds.
  • Speed: the two tests take 8.4–9.6s, down from 12.5–13.6s (3 runs each, same machine), because they no longer walk 40 levels deep on 4 seeds.
  • rustfmt --check passes on the touched file. cargo clippy -p socket-patch-core --all-targets -D warnings reports 6 errors locally, all in files this PR doesn't touch, and unmodified main gives the same result locally. The CI clippy job passes.
  • CI on a1d819b: all 337 checks completed: 331 passed, 6 skipped, 0 failed. That includes test (ubuntu/macos/windows-latest), test-release and clippy. Bugbot found no issues.

Where tests still run

Nothing moved or removed. Both tests still run in test (*) in ci.yml.

Follow-up (not in this PR)

The product crawlers themselves follow a store entry's node_modules symlink without cycle detection. A real tree with such a back-link would make find_by_purls report up to about 40 "copies" of the same package. Real pnpm never writes this shape, but adding cycle detection is worth a separate issue. It changes behavior, so it doesn't belong in a test-flake PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_0142M5G6kR7NJke3PJ1Ra74p


Generated by Claude Code

The randomized npm-crawler oracle generator gave some pnpm store
entries a node_modules symlink to nm_dirs.first(), which is usually
the importer node_modules that holds the store. Both crawlers follow
an entry's node_modules link, so they walked
.pnpm/<e>/node_modules/.pnpm/<e>/... until the OS refused the path.
Both treat every I/O error as an empty dir, so the oracle comparison
came down to where each walker gave up, not what it found. Seeds 5,
7, 12 and 41 drew this shape; seed 12 failed CI once with the
sequential walker stopping 6 levels short of the new one.

Point the link at a fresh node_modules outside the tree instead, the
same way the vlt store case and the symlinked-scope case already do.
The followed-symlink shape is still generated (16 of 64 seeds), and
the two oracle tests run about 30% faster.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mikolalysenko Mikola Lysenko (mikolalysenko) added the ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) label Oct 2, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit a1d819b. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 2, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Labeled Ready for review at a1d819bb0df2993d0b1e9235437103aafbd8f951.

  • CI: 331/337 check runs green on this head (6 skipped by path/matrix filters), 0 failing. Mergeable, merges cleanly into current main. 7 commits behind main, no file overlap.
  • Bugbot: reviewed a1d819bb0d, no issues found; no unresolved review threads.
  • Reviewer focus: test-only change to the oracle tree generator (oracle.rs); confirm the symlinked-node_modules case now points outside the tree.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Reviewed a1d819bb0df2993d0b1e9235437103aafbd8f951. Ready to merge from this review’s perspective.

The generated pnpm link still exercises a symlinked node_modules, but its fresh scratch target removes the ancestor cycle that made the comparison depend on OS path-resolution limits. The exact oracle comparisons, seed loops, and nonempty-tree guard remain intact. No actionable regression found.

Validation: the exact head has 331 successful checks, 6 skipped, and 0 failures. I verified Ubuntu’s test log contains passing results for all four npm oracle tests, including the previously failing no-pool case; Windows/macOS, release tests, clippy, and Bugbot also pass. The diff passes whitespace checks and merges cleanly with main at 203e092b. Local Cargo/stress tests were not rerun.

Runtime detection of actual directory cycles remains the separate follow-up described in the PR; this change fixes the randomized fixture.

@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit 42e7734 into main Oct 2, 2026
337 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the ci-janitor/npm-oracle-symlink-cycle branch October 2, 2026 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants