fix: stop CPU/memory blowup on non-git projects (#713, #937, #841)#1001
Open
bethzyy wants to merge 3 commits into
Open
fix: stop CPU/memory blowup on non-git projects (#713, #937, #841)#1001bethzyy wants to merge 3 commits into
bethzyy wants to merge 3 commits into
Conversation
…ta#937, DeusData#841) Two independent resource-exhaustion bugs hit non-git folders: 1. Memory (DeusData#713): the auto-index file-count guard ran `git ls-files | wc -l`, which returns 0 in a non-git folder, so the auto_index_limit cap could never fire and auto-index would eat tens of GB on large code folders. The pipe was also broken on Windows (cmd.exe passes single quotes literally and has no `wc`). Fix: count `git ls-files` lines in C (portable), and fall back to a new bounded native walk - cbm_discover_count_files() - when the git count is 0 (non-git roots). The walk honors the same skip dirs, suffix filters, and language gate as discovery and stops at the limit, so huge trees cost O(limit) work. 2. CPU (DeusData#937/DeusData#841): the watcher treated "working tree is dirty" as "changed", so a tree that merely STAYED dirty re-indexed on every poll cycle (5-60s), forever. Worse, `git rev-parse` walks up the tree, so a plain non-git folder nested under an unrelated ancestor repo was misclassified as git - and, being entirely untracked, it looked permanently dirty: a guaranteed infinite re-index loop. Fix: (a) treat a folder with no .git of its own and zero tracked files as non-git (skip polling); an empty repo root keeps its .git, a monorepo sub-package keeps its tracked files, so both stay watched. (b) Replace the boolean dirty check with a status signature - an FNV-1a hash of `git status --porcelain --untracked-files=all -- .` output plus the mtime/size of every listed file. A poll triggers a re-index only when the signature MOVES, so persistent dirt no longer loops while repeated edits to the same file are still caught. Status is scoped to the watched subtree so a monorepo package does not churn on sibling changes. The signature is committed only after a successful re-index, so failed runs retry on the next poll. Tests: watcher/discover/gitignore/git_context (173) and mcp (145) suites pass under ASan+UBSan. New regression tests: watcher_nested_non_git_skips, discover_count_files_bounded; the old watcher_continued_dirty test asserted the buggy every-poll re-index and now asserts the fixed behavior. E2E-verified with the production binary: a 15-file non-git folder with auto_index_limit=10 now logs autoindex.skip reason=too_many_files files=11 (early exit at limit+1); with limit=100 auto-index proceeds normally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: bethz <bethz@263.net>
The new pending_status_sig member is longer than the previous longest declaration, so clang-format's trailing-comment alignment moves every comment in the struct block two columns right. Formatted with clang-format 20 using the repo .clang-format; fixes the lint / lint CI failure on PR DeusData#1001. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: bethz <bethz@263.net>
The status signature mixes each listed file's mtime/size via stat(worktree_root + "/" + rel). worktree_root came from `git rev-parse --show-toplevel`, but MSYS/Cygwin git prints POSIX-mapped absolute paths (/tmp/...) that a native binary's stat() cannot resolve, so on the CLANG64 CI runner every stat failed silently and the mtime/size mix-in was lost. A second edit to an already-dirty file then left the signature unchanged and no re-index fired - watcher_continued_dirty failed at test_watcher.c:1008 (expected index_call_count 2, got 1) on windows-latest/CLANG64. Use `git rev-parse --show-cdup` instead: it prints the RELATIVE walk-up from root_path to the toplevel (empty when root_path is the toplevel), which we join onto root_path so the result always keeps the caller's path form regardless of which git flavor answered. Verified the output contract with git on Windows: toplevel prints an empty line, a subdir prints "../../" (trailing slash trimmed before the join). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: bethz <bethz@263.net>
Owner
|
Thanks for tackling this resource-safety class. I have triaged it into 0.9.1-rc as high priority. Since it touches discovery, watcher behavior, MCP paths, and the security allowlist, review will focus on non-git guard behavior, watcher churn, the allowlist delta, and confirming there are no new file, process, or network side effects. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #713. Also addresses the non-git half of #937 / #841 (watcher re-index churn).
Non-git folders currently hit two independent resource-exhaustion bugs. They share one root cause — every guard in these paths assumes
gitanswers for the project — so this PR fixes both; happy to split it into two PRs if you prefer (the mcp/discover and watcher changes are fully independent).1. Memory: auto-index OOM guard never fires on non-git roots (#713)
maybe_auto_indexcounted files withgit ls-files | wc -l:auto_index_limitcan never trigger and auto-index walks/indexes the entire tree — tens of GB of RAM on large code folders (auto_index uses tens of GB of memory in a non-git folder containing code #713).wc, so the guard silently returned nothing even for git repos.Fix (
src/mcp/mcp.c,src/discover/discover.{c,h}):git ls-fileslines in C — portable, nowc, double quotes.cbm_discover_count_files(root, limit): a bounded native walk that honors the same skip dirs, suffix filters, and language gate as discovery and stops atlimit + 1, so arbitrarily large trees cost O(limit) work. It deliberately does not load gitignore — over-counting only makes the guard more conservative, and the path must stay cheap.2. CPU: watcher re-indexes forever when a tree stays dirty (#937/#841)
Two stacked defects in
src/watcher/watcher.c:is_git_repo()usedgit rev-parse --git-dir, which walks up the directory tree. A plain non-git folder nested under any unrelated ancestor repo is misclassified as git — and since every file in it is untracked in that ancestor,git statusnever comes back clean.check_changes()treated "working tree is dirty" as "changed", so a persistently dirty tree triggered a full re-index on every poll cycle (5–60s), forever. For the nested non-git case this is a guaranteed infinite loop with zero user action; Watcher can repeatedly re-index dirty repos and cause large disk write amplification #937 measured >1 TB/day of disk writes from this class of churn.Fix:
.gitentry of its own and zero tracked files is treated as non-git (watcher skips polling, per the module's design). An empty freshly-git init-ed root keeps its.git; a monorepo sub-package keeps its tracked files — both stay watched.git status --porcelain --untracked-files=all -- .output, mixing in the mtime+size of each listed path. A poll re-indexes only when the signature moves:-- .scopes status to the watched subtree, so a monorepo package no longer churns on sibling changes;submodule foreachcheck).Tests
watcher/discover/gitignore/git_context(173) andmcp(145) suites pass under ASan+UBSan on Linux.watcher_nested_non_git_skips(baseline logsstrategy=nonefor a non-git folder under a git ancestor),discover_count_files_bounded(count, early exit, edge cases).watcher_continued_dirtypreviously asserted the buggy every-poll re-index; it now asserts stay-dirty → no re-trigger, further edit → re-trigger.auto_index_limit=10logsautoindex.skip reason=too_many_files files=11 limit=10(files=11= early exit at limit+1); withlimit=100auto-index completes normally.scripts/security-allowlist.txtupdated for the renamed/addedcbm_popencall sites in watcher.c (no new call classes).🤖 Generated with Claude Code