[HIGH] perf(backup): stop scan_case_dir re-walking symlink loops

[elhoim]

BLUF

• Measured: a 2001-file case tree with one evidence/host1/loop -> .. symlink takes 73.4s -> 0.263s (279x) in scan_case_dir, and yields 82001 -> 2001 entries; with --include-evidence those 80000 surplus entries are surplus file copies and SHA-256 hashes written to distinct manifest paths.
• Three such links in one directory did not terminate at all before the fix (killed at 120s); after, 0.265s. Reached by a plain vhir backup <dest> with no flags — the walk at backup.py:486 is unconditional.
• Growth driver: the link structure inside evidence/, which is copied verbatim off collected hosts, plus the project's own documented ln -s <resolved> case_dir/evidence/<name> workflow. os.walk swallows the ELOOP, so the operator gets no warning at all.
• Fix: carry the (st_dev, st_ino) of every directory on the path from case_dir to the current root and prune a subdirectory only when it resolves to one of them. 28 lines, local to scan_case_dir, stdlib only.
• Trade-off (deliberate, please read): I did not use the global inode seen-set from the finding — it can silently drop files under --include-evidence when evidence/ and extractions/ overlap, and makes backup contents depend on scandir order. So sibling-link 2x duplication is preserved and complexity is O(F_acyclic_reachable), not O(F_distinct).
• Behaviour preserved: 428 existing tests untouched and passing on 3.10/3.11/3.12 (433 total with the 5 added, 3 of which fail on the original code); two of the new tests pin that symlinked evidence trees are still followed, so a followlinks=False regression cannot slip in.
• Re-checked on current main @ b2324ac (2026-10-05): the bug is still present — this PR's 3 new loop tests still fail on main — e.g. two mutually reachable symlinks make the walk visit 146 entries instead of 7. The branch now has main merged in.

Priority: HIGH

Needs a deliberate decision. This change trades away a property you may care about. The trade-off is stated below; it should be accepted deliberately, not merged by accident.

The hot construct

src/vhir_cli/commands/backup.py:486 — scan_case_dir() walks the case tree following symlinks, and the only thing it ever does to dirs is the skip-name filter:

for root, dirs, files in os.walk(case_dir, followlinks=True):
    # Filter out skip names
    dirs[:] = [d for d in dirs if d not in _SKIP_NAMES]

There is no record of which directories have already been entered. A directory symlink that points back up the tree — ln -s .. evidence/host1/loop, or a link back at the case directory itself — therefore gets descended again, and again, until the kernel's MAXSYMLINKS limit (40) refuses the next resolution: the subtree is listed 41 times. os.walk reports nothing: the entry that finally fails ELOOP has is_dir() return False, so onerror never fires and the operator sees no warning.

This is not a hypothetical shape for this tool. evidence/ holds trees copied verbatim off compromised hosts during triage collection, evidence.py instructs examiners to ln -s <resolved> <case_dir>/evidence/<name>, and prune_manifests.py confirms case/evidence/ is normally itself a symlink to /mnt/evidence/. The walk is unconditional — plain vhir backup <destination> with no flags hits it. With --include-evidence or --all, every duplicate entry becomes a real shutil.copy2 and a real sha256_file pass writing the same bytes to a distinct manifest path.

Two or more mutually reachable links compound: measured, three ancestor links in one directory never finished.

The fix

Carry the (st_dev, st_ino) of every directory on the path from case_dir down to the current root, keyed by the path os.walk yields, and prune a subdirectory only when it resolves to one of those:

    # (st_dev, st_ino) of every directory from case_dir down to the current
    # root, keyed by the path os.walk yields. A directory symlink pointing back
    # at one of them (evidence/ is normally itself a symlink to a collection
    # mount) is re-entered until the kernel's symlink limit bites, so the same
    # subtree gets listed — and copied and hashed — up to 41 times.
    try:
        case_st = case_dir.stat()
        chains = {str(case_dir): ((case_st.st_dev, case_st.st_ino),)}
    except OSError:
        chains = {}

    for root, dirs, files in os.walk(case_dir, followlinks=True):
        dirs[:] = [d for d in dirs if d not in _SKIP_NAMES]

        # Drop subdirectories that loop back onto the path we came in by
        chain = chains.pop(str(root), ())
        kept = []
        for d in dirs:
            sub = os.path.join(str(root), d)
            try:
                sub_st = os.stat(sub)
            except OSError:
                kept.append(d)  # unreadable — os.walk skips it as before
                continue
            key = (sub_st.st_dev, sub_st.st_ino)
            if key in chain:
                continue
            chains[sub] = chain + (key,)
            kept.append(d)
        dirs[:] = kept

chains is popped as each root is consumed, so it only ever holds the pending frontier, not one entry per directory visited. Cost is one extra stat() per directory (not per file). Stdlib only, no new module, no new dependency, entirely local to scan_case_dir.

Pruning only the ancestor chain — rather than a global inode set — catches every looping shape: .., ., a link straight back at the case dir, mutually reachable sideways pairs (evidence/ext -> /mnt/x, /mnt/x/back -> case/evidence), and ln -s / (which now walks the root filesystem once rather than 41 times).

Measurements

2001-file case tree (20 dirs x 100 files + notes.md), scan_case_dir only, best of 5 after a warm pass, Python 3.12:

tree	before	after	speedup	entries before / after
one evidence/host1/loop -> ..	73.409s	0.263s	279x	82001 / 2001
three ancestor links	did not finish (killed at 120s)	0.265s	terminates	— / 2001
no links (control)	0.212s	0.222s	1.0x (noise)	2001 / 2001

The entry counts matter as much as the seconds: with --include-evidence, 82001 entries are 82001 shutil.copy2 calls and 82001 SHA-256 passes over the same 2001 files, landing in 41 sets of manifest paths. Scaled to a real 1M-file evidence tree, one such link is ~40M stats and ~40M accumulated tuples.

Behaviour preservation

• The 428 existing tests are unmodified and pass: 433 passed on Python 3.10.20, 3.11.15 and 3.12.13 (428 + the 5 added). ruff check . and ruff format --check . clean.
• New tests/test_backup_scan.py (there were no backup tests):
  • test_parent_symlink_scans_subtree_once — red on original: assert 246 == 6
  • test_symlink_to_case_root_scans_case_once — red on original: assert 286 == 6
  • test_mutually_reachable_symlinks_terminate — red on original: assert 146 == 7
  • test_evidence_symlink_target_is_scanned — passes on both; pins that the documented ln -s <resolved> evidence/<name> layout is still followed and scanned. This is the guard against "fixing" this with followlinks=False, which would silently drop evidence from backups.
  • test_sibling_symlink_is_still_scanned — passes on both; pins that a non-looping duplicate path (evidence/latest -> host1) is still backed up under both names.

Trade-off the maintainer should decide on

I deviated from the obvious fix (a global seen: set[(st_dev, st_ino)]) on purpose, and the PR is weaker in one dimension because of it:

• A global set dedupes by inode anywhere in the tree. With evidence -> /mnt/evidence and extractions -> /mnt/evidence/extractions, the overlapping subtree is pruned from whichever list is reached second — so --include-evidence without --include-extractions would silently omit those files from the backup. Evidence loss beats a constant factor.
• It also makes which of two duplicate paths survives depend on os.scandir order, i.e. the same case backed up twice produces different archive contents.

So: sibling-link duplication (2x/4x) is preserved, and the complexity here is O(F_acyclic_reachable) with guaranteed termination, not O(F_distinct). If you would rather have single-path semantics, the global set is a two-line change on the same dirs[:] line — happy to take it, with those two caveats understood.

Assumption, stated plainly: this trusts (st_dev, st_ino) to be unique among directories on one root-to-leaf path, which every POSIX filesystem guarantees. A spurious collision would have to be on the ancestor chain, not anywhere in the tree — which is why this is safer than a global set on filesystems with weak inode semantics.

Independent review notes

This change was reviewed adversarially by a second pass that re-ran the benchmarks. Where that review contradicted or qualified the account above, it is recorded here rather than quietly edited out:

• The benchmark's tree shape hides a real cost. The control is 20 dirs x 100 files, so one extra
  os.stat() per directory is invisible. The reviewer rebuilt the same 2001 files as 2000 dirs x 1 file:
  +10% to +40% (0.172-0.190 s before, 0.183-0.285 s after). Directory-heavy evidence trees — registry
  hive dumps, per-PID captures — pay that.
• The author deliberately did not take the audit's suggested global seen inode set, because with
  evidence -> /mnt/evidence and extractions -> /mnt/evidence/extractions it would silently drop the
  overlapping subtree from whichever list is reached second. The ancestor-only guard is the conservative
  choice; the global set reaches the audit's O(F_distinct) but changes which files get backed up.

Deliberately not done here

If the maintainer decides single-path semantics are acceptable for backups (i.e. each physical directory backed up under exactly one path), a global seen: set[tuple[int, int]] on the same dirs[:] line reaches the finding's O(F_distinct) and also kills the sibling 2x/4x duplication — but only with the two caveats above understood: overlapping evidence/extractions trees can lose files when only one category is included, and which path survives becomes scandir-order dependent. That would want an explicit decision and probably a note in the backup output listing the deduped paths.

Separately, the same dirs[:] line is the subject of finding #2 (depth-0 prune of excluded top-level categories); the two fixes are independent and compose — neither substitutes for the other — but whichever lands second will need a trivial rebase of that one hunk. Deliberately did not build a shared walk helper: it would couple these PRs.

---

CI status

The checks on this PR will show action_required: GitHub gates workflow runs on pull requests from
first-time fork contributors until a maintainer approves them. They need an Approve and run click.

The CI job was reproduced locally after merging main @ b2324ac (761 tests on main), using the same steps as .github/workflows/ci.yml:

Step	Result
pip install -e ".[dev]" && pytest tests/ — Python 3.10	766 passed
pip install -e ".[dev]" && pytest tests/ — Python 3.11	766 passed
pip install -e ".[dev]" && pytest tests/ — Python 3.12	766 passed
ruff check . + ruff format --check . (latest ruff, unpinned as in CI)	check + format clean

🤖 Generated with Claude Code

[elhoim]

This PR is a draft because it needs a decision on whether to accept single-path backup semantics. That decision is written up on its own, with the options laid out, in #49 — probably the better place to discuss it than this diff.