Skip to content

fix(watch): report failed directory adoption - #910

Open
yzxcj797 wants to merge 1 commit into
tirth8205:mainfrom
yzxcj797:fix/907-watch-adoption-failure
Open

fix(watch): report failed directory adoption#910
yzxcj797 wants to merge 1 commit into
tirth8205:mainfrom
yzxcj797:fix/907-watch-adoption-failure

Conversation

@yzxcj797

Copy link
Copy Markdown
Contributor

Problem

When observer.schedule() raises OSError while adopting a new directory, _schedule() logs the failure and returns, but _adopt_directory() still returns True. The directory is therefore reported as adopted, degraded stays false, and watcher health gives no path-level evidence of the lost coverage.

Change

  • _schedule() now reports whether registration succeeded.
  • Failed paths are retained and published in the health payload as failed_paths.
  • _adopt_directory() only reports adoption when at least one planned watch was registered; parent promotion follows the same rule.
  • Any failed registration marks the watcher degraded, so crg-daemon status reports partial rather than ok.
  • Updated the daemon status guidance and troubleshooting documentation to distinguish failed registration from watch-budget promotion.

Validation

  • New regression: failed directory adoption is not reported as adopted and publishes degraded health plus the failed path.
  • pytest tests/test_watch_robustness.py::TestNewDirectoryAdoption tests/test_daemon.py: 77 passed, 5 skipped.
  • Watch robustness suite with the Windows-only SIGTERM interpreter crash deselected: 56 passed, 1 deselected. The same SIGTERM test also crashes the pristine main run in this environment.
  • ruff check on all changed Python files.
  • git diff --check.

Fixes #907

@github-actions

Copy link
Copy Markdown

code-review-graph review

Overall risk: 0.70 (HIGH) — 11 changed function(s)/class(es), 12 affected flow(s), 5 test gap(s)

Risk-scored changes

Risk Level Symbol Location Tested
0.70 high code_review_graph/incremental.py::_WatchSupervisor._schedule code_review_graph/incremental.py:1850 no
0.65 medium code_review_graph/incremental.py::_WatchSupervisor.report_health code_review_graph/incremental.py:2104 yes
0.60 medium code_review_graph/incremental.py::_WatchSupervisor._promote_to_recursive code_review_graph/incremental.py:1968 no
0.55 medium code_review_graph/incremental.py::_WatchSupervisor code_review_graph/incremental.py:1775 yes
0.35 low code_review_graph/daemon.py::watcher_status code_review_graph/daemon.py:598 yes
0.35 low code_review_graph/daemon_cli.py::_handle_status code_review_graph/daemon_cli.py:127 yes
0.35 low code_review_graph/incremental.py::_WatchSupervisor._adopt_directory code_review_graph/incremental.py:1921 no
0.30 low code_review_graph/incremental.py::_WatchSupervisor.__init__ code_review_graph/incremental.py:1789 yes
0.30 low code_review_graph/incremental.py::_WatchSupervisor.degraded code_review_graph/incremental.py:1826 no
0.05 low tests/test_watch_robustness.py::TestNewDirectoryAdoption tests/test_watch_robustness.py:285 no

Affected execution flows

  • start — criticality 0.37, 7 node(s) across 1 file(s)
  • _run — criticality 0.36, 68 node(s) across 1 file(s)
  • start — criticality 0.36, 3 node(s) across 1 file(s)
  • on_modified — criticality 0.36, 2 node(s) across 1 file(s)
  • _poll — criticality 0.36, 2 node(s) across 1 file(s)
  • ...and 7 more affected flow(s)

Test gaps

  • code_review_graph/incremental.py::_WatchSupervisor.degraded (code_review_graph/incremental.py:1826)
  • code_review_graph/incremental.py::_WatchSupervisor._schedule (code_review_graph/incremental.py:1850)
  • code_review_graph/incremental.py::_WatchSupervisor._adopt_directory (code_review_graph/incremental.py:1921)
  • code_review_graph/incremental.py::_WatchSupervisor._promote_to_recursive (code_review_graph/incremental.py:1968)
  • tests/test_watch_robustness.py::TestNewDirectoryAdoption (tests/test_watch_robustness.py:285)

Token savings: this graph-backed report used ~40,721 fewer tokens (~70%) 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.

@tirth8205

Copy link
Copy Markdown
Owner

The core of this is right and it lands where I hoped: _schedule returns a bool, _adopt_directory no longer claims a directory it did not watch, and the health payload names the paths. I checked the two things that would have made it worse than the bug and both are fine — a transient failure recovers (tick 1 adopted=[], tick 2 adopted=['services'] with the path back in watched_paths), and no new exit path fires on an ordinary rm -rf, because check_liveness still gates on root not in self._watches. The new test also fails on the parent commit, as it should.

Three things in the new code need fixing before it goes in.

1. _degraded is a one-way latch, and an OS failure is not a one-way condition. _schedule sets self._degraded = True on failure but nothing ever clears it, while _failed_paths.discard(key) does clear on success. After a hiccup recovers you get degraded=True, failed_paths=[] — which is byte-identical to genuine budget exhaustion. watcher_status then reports partial forever, and daemon_cli prints the wrong remedy:

  still complete, but ignored trees are watched again. Raise CRG_MAX_WATCH_SCHEDULES.

Raising the schedule limit does nothing for an ENOSPC that already cleared. The latch was correct for promotion — a recursive watch is never split back — so keep that irreversible and derive the flag instead: degraded = self._promoted or bool(self._failed_paths).

2. _failed_paths is never pruned for directories that no longer exist. The vanished-detection loop in sync_watches iterates _children_of(parent), which reads _watches — a failed path is by definition not in there — and _release_directory never touches _failed_paths. So an entry survives rm -rf permanently. On a CI-shaped repo that creates and deletes build directories, the set grows for the lifetime of the watcher: 200 short-lived directories left 200 entries and a 15 KB health file, none of which existed any more, with degraded stuck on. Prune in sync_watches when os.path.isdir(path) is false.

3. Health writes are amplified exactly when the machine is starved. Adding tuple(sorted(self._failed_paths)) to _last_health_state bypasses the 10s _WATCH_HEALTH_INTERVAL whenever the set churns — 100 ticks produced 1 write before this PR and 100 after, i.e. a write-plus-rename per second on a box that has just run out of watch descriptors. Fixing (2) removes most of the churn, but the state tuple should still not be able to defeat the rate limiter.

Smaller, worth doing in the same pass: failed_paths never reaches crg-daemon status. _health_fields (daemon.py:614) is what WatchDaemon.status() returns and it does not carry the field, and status has no --json, so the new CLI text points the user at something they cannot see. _handle_status already has health in hand in that loop — printing the paths there is a couple of lines.

One thing I am not holding against you, filed separately: if the promotion schedule fails, _promote_to_recursive has already released the parent and its descendants, so the supervisor ends with zero watches and sync_watches never revisits the parent. That behaves identically on the parent commit — your change only makes it honest about the count.

Everything else is green: 2986 passed, the three real-Observer tests pass, ruff and mypy clean, and all 13 checks pass on the PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

watch: a directory whose watch registration fails is still reported as adopted, and health still says ok

2 participants