Repository navigation
fix(watch): bound each watch rebuild with GRAPHIFY_REBUILD_TIMEOUT, with a Timer fallback where SIGALRM is missing (#2173) - #4724
Conversation
…ith a Timer fallback where SIGALRM is missing (Graphify-Labs#2173) graphify/watch.py:2845 calls _rebuild_code(watch_path) with no timeout. A hung rebuild blocks the watch loop with no upper bound. The hooks bound their rebuild by GRAPHIFY_REBUILD_TIMEOUT (graphify/hooks.py:200-228), and their SIGALRM path does not exist on Windows. I added _rebuild_timeout() to graphify/watch.py and wrapped the watch loop's _rebuild_code call in it, with the same two paths as the hooks. With SIGALRM, the handler kills multiprocessing.active_children() and raises TimeoutError; the loop logs it and continues. Without SIGALRM, a daemon threading.Timer kills the children and calls os._exit(1). A timeout of 0 disables the guard. tests/test_hooks.py gains one AST test of the fallback shape. It fails when graphify/watch.py is reverted to v8. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the pull request, @danieldutu. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 4 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 1 changed function(s) (1 not verified).
Formal verification. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 vacuous).
Not verified on this run: watch (vacuous: never exercised).
Graphify review — findings
Adds a GRAPHIFY_REBUILD_TIMEOUT bound (default 600s, 0 disables) around each graphify watch rebuild, so a hung rebuild no longer stalls the loop indefinitely. On POSIX a SIGALRM handler kills the pool workers and raises TimeoutError, which the loop catches and logs before carrying on. Where SIGALRM doesn't exist (Windows), a daemon timer kills the workers and calls os._exit(1), so a timeout there ends the whole watcher rather than skipping one rebuild. The timeout value is parsed with a bare int() on the env var, and the alarm can interrupt _rebuild_code at any point, including mid-write. A new AST-based test checks that the timer fallback sits in the no-SIGALRM branch, that workers are killed before os._exit, and that watch() wraps its rebuild in _rebuild_timeout.
Worth a look
- Malformed GRAPHIFY_REBUILD_TIMEOUT crashes the watch loop —
graphify/watch.py:2762· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- Windows fallback terminates the long-running watcher via os._exit —
graphify/watch.py:2783· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- Async TimeoutError can interrupt _rebuild_code mid-write —
graphify/watch.py:2770· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- Rebuild timeout raises outside the main thread on POSIX —
graphify/watch.py:2767· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 818 functions depend on the 343 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
watch()— 6 callers, 11 callees
Verification — 818 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 589 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
27 of 453 test file(s) selected (6%) via static blast radius.
tests/test_cache_stale_import_target.py— impacttests/test_compact_graph_json.py— impacttests/test_elixir_import_resolution.py— impacttests/test_elixir_qualified_calls.py— impacttests/test_elixir_unqualified_call_scope.py— impacttests/test_export_direction.py— impacttests/test_external_stub_endpoints.py— impacttests/test_hooks.py— impact, changed-testtests/test_incremental_unscanned_placeholders.py— impacttests/test_java_member_calls.py— impacttests/test_kotlin_grammar.py— impacttests/test_labeling.py— impacttests/test_language_override_lifecycle.py— impacttests/test_lost_saved_semantic_nodes.py— impacttests/test_markdown_code_spans.py— impacttests/test_no_cluster_undeclared_endpoints.py— impacttests/test_pr10_watch_overrides.py— impacttests/test_python_inherited_method_calls.py— impacttests/test_python_property_reads.py— impacttests/test_python_pytest_fixture_receivers.py— impacttests/test_rust_use_trees.py— impacttests/test_skill_persisted_scan_options.py— impacttests/test_solidity_inherited_calls.py— impacttests/test_terraform_modules.py— impacttests/test_watch.py— impacttests/test_watch_identity_cache.py— impacttests/test_watch_manifest_location.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Risky patterns (advisory)
- medium
no-stderr-to-stdoutingraphify/watch.py:2898: error printed to stdout instead of stderr: print(f"[graphify watch] {exc}")
Formal verification
Could not verify: Could not verify watch.
The verifier did not have enough to check watch, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 142 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ImportError — names the real obstacle, not a sampling gap)
· 1 grounded finding(s) anchored inline below.
| watchdog.cancel() | ||
|
|
||
|
|
||
| def watch(watch_path: Path, debounce: float = 3.0) -> None: |
There was a problem hiding this comment.
watch()
fans out to 11 callees (efferent coupling); 6 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
/firstpr explain |
… thread #4724 wrapped each watch-loop rebuild in _rebuild_timeout(), which arms SIGALRM. signal.signal() only works on the main thread and raises ValueError('signal only works in main thread') anywhere else. watch()'s dispatch loop is run in a worker thread both under test (test_watch_handler_honors_graphifyignore, gh-928) and when embedded, so that ValueError escaped the loop's 'except TimeoutError' guard and tore the watcher down before _rebuild_code ran: a non-ignored .py write never triggered a rebuild. Gate the SIGALRM path on threading.current_thread() is main_thread(); off the main thread, use the existing signal-free Timer fallback, which bounds the rebuild just the same. Adds a behavioral regression test that runs _rebuild_timeout() off the main thread and asserts it yields without raising. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Landed in v0.9.96, now on PyPI (pip install graphifyy==0.9.96). Each watch rebuild is bounded by GRAPHIFY_REBUILD_TIMEOUT with a Timer fallback. Thanks for the fix, @danieldutu. |
Defect
On
v8(92741d1),graphify/watch.pycontains 0 references toSIGALRM,threading.TimerorGRAPHIFY_REBUILD_TIMEOUT.graphify/hooks.pycontains 2 of each, one per embedded rebuild body. A rebuild that hangs insidegraphify watchtherefore blocks the watch loop with no upper bound. This is the remaining half of #2173.Root cause
graphify/watch.py:2850calls_rebuild_code(watch_path)outside any timeout guard. The hooks guard the same call atgraphify/hooks.py:200-228:signal.alarmwhereSIGALRMexists, athreading.Timerwhere it does not (#2148).watch.pyhas neither path.Fix
I added
_rebuild_timeout()tographify/watch.pyand wrapped the loop's_rebuild_codecall in it. It has the two paths the hooks have:SIGALRMavailable: the handler killsmultiprocessing.active_children(), then raisesTimeoutError. The loop prints[graphify watch] <message>and keeps watching. The previous handler is restored on exit.SIGALRMunavailable (Windows): a daemonthreading.Timerkills the children and callsos._exit(1). The watcher process exits.GRAPHIFY_REBUILD_TIMEOUT=0disables the guard. The default is600, as in the hooks.Evidence
test_watch_rebuild_timeout_arms_a_timer_without_sigalrm, parseswatch.pywithast. It asserts that theTimerfallback sits in the else-branch of theSIGALRMcheck, that_bailkills children beforeos._exit, and thatwatch()references_rebuild_timeout.tests/test_ext_tagged_id_clash.py. The same file gives7 failed, 7 passedonv8with this change stashed.pyrightreports errors in my environment.watchdogis not installed, sographify/watch.pyreports 3 unresolvedwatchdog.*imports. Other files report errors unrelated to this change. No error falls on a changed line. I did not compare the total againstv8.Behavior change
POSIX
graphify watchnow has a 600 s default rebuild timeout where it had none. This matches the600default in the hooks. On timeout the POSIX watcher logs and continues, and the Windows watcher exits.GRAPHIFY_REBUILD_TIMEOUT=0restores the previous behavior.Boundaries and trade-offs
-cargument, and a shared import would change that carrier. A second copy of the 25 lines inwatch.pywas the alternative; a context manager was shorter and restores the handler.os._exit(1)because no other call interrupts a main thread blocked in a rebuild whenSIGALRMis missing.Question
I kept the 600 s default to match the hooks. Do you want
watchto default to0instead, so POSIX behavior stays unchanged? I can change the default or split the Windows fallback into its own PR.