Repository navigation
fix(watch): drop events on paths the OS cannot stat - #959
Chang-Jin-Lee wants to merge 1 commit into
Conversation
WatchBatchProcessor._relative_path walked up to the nearest existing
ancestor with a bare Path.exists(), which propagates OSError rather than
answering False when the OS cannot stat the path at all. A component past
NAME_MAX is the easy case:
>>> Path('/tmp/' + 'a'*300 + '/x.py').exists()
OSError: [Errno 36] File name too long
That escaped _relative_path, was caught by process()'s except BaseException
into self.failure, and the next raise_if_failed ended the watch loop with
RuntimeError: watch update failed. Under the daemon the watcher restarted,
paid for a full initial update, and died again on the next such event.
collect_all_files already skips paths past the OS limits, so the concern is
acknowledged elsewhere in the file; the watch path just lacked the guard.
The exists() walk, the resolve() call and the symlink scan now share one
guarded block. OSError drops the event the way an ignored path is dropped
and logs at debug, matching the sibling guard in collect_all_files. The
pre-existing ValueError branch keeps its meaning: a path outside the root
is not ours.
Closes tirth8205#897
code-review-graph reviewOverall risk: 0.55 (MEDIUM) — 7 changed function(s)/class(es), 2 affected flow(s), 3 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~29,354 fewer tokens (~73%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
|
Integrated on |
|
Thanks for taking it. On the FSEvents gap: the guard sits in |
|
Merged into |
…annot stat Port the reviewed token-efficiency implementation with targeted regression coverage. Source-PR: tirth8205#959
Pull Request
Linked issue
Closes #897
What & why
WatchBatchProcessor._relative_pathwalks up to the nearest existing ancestor with a barePath.exists(). That call answersFalsefor a path that is merely absent, but it raisesOSErrorfor a path the OS cannot stat at all. A component pastNAME_MAXis the easy case:The exception escaped
_relative_path, was caught byprocess()'sexcept BaseExceptionintoself.failure, and the nextraise_if_failed()ended the watch loop withRuntimeError: watch update failed. Under the daemon the watcher then restarted with backoff, paid for a full initial update, and died again the next time the same event arrived — so any tool writing a too-long name inside a watched repo takes the watcher down repeatedly.collect_all_filesalready skips paths past the OS limits, so the concern is acknowledged elsewhere in this file; the watch path just did not carry the guard.The
exists()walk, theresolve()call and the symlink scan now sit in one guarded block.OSErrordrops the event the way an ignored path is dropped and logs at debug, matching the sibling guard incollect_all_files. The pre-existingValueErrorbranch keeps its meaning, that a path resolving outside the root is not ours; widening it over the walk costs nothing, since a path Windows rejects as malformed should be dropped for the same reason.I took the
OSErrorroute rather than reusing the length check because it also covers the cases a length check cannot see — a broken mount, a parent that has become unreadable — and every one of them wants the same answer.The two tests are the reproduction from the issue. The first asserts the loop survives the event; the second puts a real file in the same batch and asserts it is still indexed, so the guard drops the bad event rather than the whole batch.
How it was tested
Both new tests fail on
mainwithRuntimeError: watch update failed, which is the symptom in the issue.ruff formatreportsincremental.pyandtest_watch_robustness.pyas unformatted, but it reports the same on unmodifiedmain, so I left formatting alone rather than bury this change in an unrelated reflow.Verified on Linux (Python 3.12). The issue reports macOS/3.13; the failing call is
Path.exists()raisingENAMETOOLONG, which is POSIX behaviour rather than platform-specific, and the repro reproduces here unchanged.One note in case it saves someone else the detour: I first ran the suite with
TMPDIRon a tmpfs path that happened to contain the substringtest, andtest_parser.py::TestCodeParser::test_non_test_file_describe_not_specialfailed because the temp file it writes then sits under a test-looking path. Nothing to do with this change, but the test is sensitive to whereTMPDIRpoints.Checklist
uv run pytest tests/ --tb=short -quv run ruff check code_review_graph/uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optionaldocs/, docstrings)docs/TROUBLESHOOTING.mddescribes watcher stalls in terms of a dead observer thread; this failure ended the loop throughraise_if_failedinstead, and now does not happen at all, so I did not find a line there that became wrong. Happy to add one if you would rather it be named.