[Audit v2 #353] 30s watchdog on filesystem_changed during update reload (#381)

* [Audit v2 #353] 30s watchdog on filesystem_changed during update reload

Pre-fix, `_start_filesystem_scan` connected `filesystem_changed`
(CONNECT_ONE_SHOT) and called `fs.scan()`, then sat in
`_waiting_for_scan = true` forever if the signal never fired (slow
disk, NFS, AV holding the just-extracted addon files open). The
update completed mechanically — files on disk, plugin disabled —
but the dock stayed disabled because `_finish_scan_wait` was the
only path back to `_enable_new_plugin`. User had to kill and
restart Godot.

Add a 30-second `Timer` armed alongside the signal connect:

- `_arm_scan_watchdog`: lazy-creates a one-shot `Timer` child node
  on first use, reuses it on subsequent arms (the runner does two
  filesystem scans per update — new files then existing files).
- `_on_scan_watchdog_timeout`: if the wait is still open, push a
  warning, disconnect the `filesystem_changed` listener so a
  delayed signal can't double-call, then dispatch
  `_finish_scan_wait`. `_finish_scan_wait`'s existing
  `if not _waiting_for_scan: return` guard makes it idempotent
  against the signal-arrives-just-before-watchdog race.
- `_finish_scan_wait` now also stops the still-running timer on the
  happy path, so a queued late `timeout` can't fire after a
  successful scan.

Worst case after the watchdog fires: the new files aren't visible
on the first frame the new plugin enables, but they get picked up
on the next scan. That's strictly better than the prior dock-
permanently-disabled state.

Tests in `test_project/tests/test_update_reload_runner.gd`:
- `test_watchdog_timeout_proceeds_when_signal_never_fires` — the
  deadlock case the issue describes.
- `test_watchdog_no_op_when_signal_already_settled` — late-Timer
  race after the signal won.
- `test_finish_scan_wait_stops_armed_watchdog` — happy path
  cancels the Timer.
- `test_watchdog_timer_reused_across_arms` — second scan in the
  same update doesn't leak a second Timer child.

Tests fire `_on_scan_watchdog_timeout` and `_finish_scan_wait`
directly rather than waiting for the wall-clock Timer, keeping
runs deterministic and sub-second.

Closes #353

* Fix CI: remove add_child(runner) from watchdog tests

McpTestSuite extends RefCounted (not Node), so add_child(runner) errors
at test runtime — Linux/macOS/Windows all failed with the same root
cause. The tests fire _on_scan_watchdog_timeout() directly instead of
relying on the Timer counting down, so the runner doesn't actually need
to be in a SceneTree; the Timer just needs to be a child of the runner
(works regardless of where the runner sits).

* Redesign per Copilot review: sticky bypass after watchdog + tests in tree

Two issues from Copilot's review on PR #381 + the second CI failure:

(A) Cross-scan signal race [Copilot]: scan #1 watchdog'd, scan #2 then
    arms a fresh `filesystem_changed` listener. A delayed emission
    from scan #1 fires on whichever listener is currently connected
    to the shared signal — Godot can't tag emissions with their
    source scan — so scan #2's listener falsely settles before its
    actual filesystem scan completed, re-enabling the plugin against
    a potentially incomplete on-disk install. Generation-counter +
    Callable.bind doesn't help because a single emission still fires
    every connected listener (CONNECT_ONE_SHOT only auto-disconnects
    after firing).

(B) Second CI failure: `Timer.start()` requires the Timer to be
    inside a SceneTree. The previous fix removed `add_child(runner)`
    from tests but didn't parent the runner anywhere, so when
    `_arm_scan_watchdog` called `add_child(timer)` and `timer.start()`
    inside the runner, the timer wasn't in any tree.

Fix:

- Add a sticky `_scan_timed_out` flag set by `_on_scan_watchdog_timeout`.
- `_start_filesystem_scan` checks the flag at the top and bypasses the
  connect + `fs.scan()` path entirely, falling straight through to
  `call_deferred(deferred_step)`. With no listener armed, a delayed
  emission from the timed-out scan has nothing to satisfy. Godot's
  normal background scan catches up after the plugin re-enables.
- Tests now parent the runner via `(Engine.get_main_loop() as
  SceneTree).root.add_child(runner)` so `Timer.start()` works.
  Cleanup via a `_free_runner` helper (remove_child + free).
- New regression test `test_subsequent_scan_after_watchdog_bypasses_
  listener_arm` explicitly drives scan #1 → watchdog → scan #2 and
  asserts scan #2 does NOT set `_waiting_for_scan = true` (i.e. no
  listener armed for a delayed scan-#1 emission to satisfy).
- Existing watchdog test `test_watchdog_no_op_when_signal_already_settled`
  also asserts `_scan_timed_out` stays false on the late-Timer-after-
  successful-signal path — guards against the watchdog poisoning
  subsequent scans when no actual deadlock happened.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: dsarno

plugin/addons/godot_ai/update_reload_runner.gd

@@ -35,6 +35,23 @@ var _next_step := ""
 var _frames_remaining := 0
 var _waiting_for_scan := false
 var _scan_next_step := ""
+## Watchdog for `_start_filesystem_scan`: if Godot's `filesystem_changed`
+## signal never fires (slow disk, NFS, AV holding the just-extracted addon
+## files open), the runner used to hang in `_waiting_for_scan = true`
+## forever and the dock stayed disabled. After this timeout we disconnect
+## the signal and proceed anyway — worst case the new files aren't visible
+## on the first frame, but they get picked up on the next scan. See
+## audit-v2 finding #9 (issue #353). Untyped to match the codebase's
+## defensive pattern for state that survives `fs.scan()` during update.
+const SCAN_WATCHDOG_SECS := 30.0
+var _scan_watchdog_timer = null
+## Sticky flag set by `_on_scan_watchdog_timeout`. Subsequent
+## `_start_filesystem_scan` calls in the same update bypass connect+scan
+## so a delayed `filesystem_changed` emission from the timed-out scan
+## can't fire on a freshly-armed listener for the next scan and falsely
+## settle it before that scan actually completed. See PR #381 review for
+## the cross-scan race this guards against.
+var _scan_timed_out := false
 ## Keep Array fields untyped: this runner survives fs.scan() during update,
 ## and typed Variant storage is part of the hot-reload crash class.
 var _new_file_paths = []
@@ -121,13 +138,68 @@ func _start_filesystem_scan(next_step: String = "_enable_new_plugin") -> void:
 		call_deferred(deferred_step)
 		return
 
+	## Bypass: a previous scan in this update already watchdog'd, so the
+	## editor's filesystem is unresponsive. Re-arming a `filesystem_changed`
+	## listener now would race with a delayed emission from the timed-out
+	## scan: that single emission would fire whichever listener is currently
+	## connected to the shared signal, falsely settling this scan before it
+	## actually completed. Skip the wait; Godot's normal background scan
+	## catches up after the plugin re-enables. See PR #381 review.
+	if _scan_timed_out:
+		push_warning(
+			"MCP | skipping filesystem_changed wait after previous timeout (next_step=%s)"
+			% deferred_step
+		)
+		call_deferred(deferred_step)
+		return
+
 	_waiting_for_scan = true
 	_scan_next_step = deferred_step
 	if not fs.filesystem_changed.is_connected(_on_filesystem_changed):
 		fs.filesystem_changed.connect(_on_filesystem_changed, CONNECT_ONE_SHOT)
+	_arm_scan_watchdog()
 	fs.scan()
 
 
+func _arm_scan_watchdog() -> void:
+	if _scan_watchdog_timer == null:
+		_scan_watchdog_timer = Timer.new()
+		_scan_watchdog_timer.one_shot = true
+		_scan_watchdog_timer.timeout.connect(_on_scan_watchdog_timeout)
+		add_child(_scan_watchdog_timer)
+	_scan_watchdog_timer.start(SCAN_WATCHDOG_SECS)
+
+
+func _stop_scan_watchdog() -> void:
+	if _scan_watchdog_timer != null:
+		_scan_watchdog_timer.stop()
+
+
+func _on_scan_watchdog_timeout() -> void:
+	## Signal didn't fire within SCAN_WATCHDOG_SECS — most likely the
+	## filesystem scan is blocked behind a slow disk / NFS / AV scanner
+	## still reading the just-extracted addon files.
+	## Set the sticky `_scan_timed_out` flag so any subsequent
+	## `_start_filesystem_scan` in this update skips its connect+scan
+	## (otherwise a delayed emission from this scan would falsely settle
+	## the next scan's listener — see PR #381 review).
+	## Disconnect the current listener too, so this scan's listener can't
+	## double-call `_finish_scan_wait` if the signal arrives quickly after
+	## the timeout fires. `_finish_scan_wait` is idempotent on
+	## `_waiting_for_scan == false`.
+	if not _waiting_for_scan:
+		return
+	push_warning(
+		"MCP | filesystem_changed didn't fire within %ds; proceeding without scan confirmation"
+		% int(SCAN_WATCHDOG_SECS)
+	)
+	_scan_timed_out = true
+	var fs := EditorInterface.get_resource_filesystem()
+	if fs != null and fs.filesystem_changed.is_connected(_on_filesystem_changed):
+		fs.filesystem_changed.disconnect(_on_filesystem_changed)
+	_finish_scan_wait()
+
+
 func _read_update_manifest() -> bool:
 	var zip_path := ProjectSettings.globalize_path(_zip_path)
 	var install_base := ProjectSettings.globalize_path(INSTALL_BASE_PATH)
@@ -392,6 +464,7 @@ func _finish_scan_wait() -> void:
 	if not _waiting_for_scan:
 		return
 	_waiting_for_scan = false
+	_stop_scan_watchdog()
 	var next_step := _scan_next_step
 	_scan_next_step = ""
 	set_process(false)

test_project/tests/test_update_reload_runner.gd

@@ -478,3 +478,156 @@ func test_install_zip_paths_rolls_back_when_mid_loop_write_fails() -> void:
 	# (rare but possible) doesn't re-attempt rollback against stale records.
 	assert_eq(runner._paths_written.size(), 0, "_paths_written cleared after rollback")
 	runner.free()
+
+
+# ----- _arm_scan_watchdog / _on_scan_watchdog_timeout (audit-v2 #9) -----
+
+
+func _scene_root() -> Node:
+	# `Timer.start()` requires the timer (and thus its parent) to be
+	# inside a SceneTree. `McpTestSuite` extends `RefCounted` so it can't
+	# act as a parent itself — parent the runner under the editor's
+	# SceneTree root for the duration of the test, then remove + free.
+	var tree := Engine.get_main_loop() as SceneTree
+	return tree.root if tree != null else null
+
+
+func _new_runner_in_tree():
+	var runner = _new_runner()
+	var root := _scene_root()
+	assert_true(root != null, "test setup: SceneTree root must exist")
+	root.add_child(runner)
+	return runner
+
+
+func _free_runner(runner) -> void:
+	if runner.get_parent() != null:
+		runner.get_parent().remove_child(runner)
+	runner.free()
+
+
+func _arm_scan_state(runner) -> void:
+	# Mirror what `_start_filesystem_scan` would do, minus the actual
+	# `EditorInterface.get_resource_filesystem().scan()` call. We can't
+	# trigger Godot's filesystem_changed signal from a test, so we drive
+	# the state machine directly.
+	runner._waiting_for_scan = true
+	runner._scan_next_step = "_enable_new_plugin"
+	runner._arm_scan_watchdog()
+
+
+func test_watchdog_timeout_proceeds_when_signal_never_fires() -> void:
+	## Pre-fix, if `filesystem_changed` deadlocked the runner sat in
+	## `_waiting_for_scan = true` forever. The watchdog must clear that
+	## flag and dispatch `_scan_next_step` so the rest of the update
+	## sequence can finish.
+	var runner = _new_runner_in_tree()
+	_arm_scan_state(runner)
+	assert_true(runner._waiting_for_scan, "scan wait armed by precondition")
+	assert_true(runner._scan_watchdog_timer != null, "watchdog timer node exists once armed")
+
+	runner._on_scan_watchdog_timeout()
+
+	assert_false(runner._waiting_for_scan, "watchdog cleared the wait flag")
+	assert_eq(runner._scan_next_step, "", "next-step token consumed by _finish_scan_wait")
+	assert_true(runner._scan_timed_out, "watchdog must set the sticky bypass flag")
+	_free_runner(runner)
+
+
+func test_watchdog_no_op_when_signal_already_settled() -> void:
+	## Race: signal fires, `_finish_scan_wait` cleans up, then the watchdog
+	## Timer fires anyway (Godot's Timer can have queued timeout). Calling
+	## `_on_scan_watchdog_timeout` after a settled scan must be a no-op —
+	## otherwise it would double-dispatch `_scan_next_step` AND it must
+	## NOT poison subsequent scans by setting `_scan_timed_out = true`.
+	var runner = _new_runner_in_tree()
+	_arm_scan_state(runner)
+
+	# Simulate the happy path: signal arrived, _finish_scan_wait ran.
+	runner._finish_scan_wait()
+	assert_false(runner._waiting_for_scan, "wait already cleared by signal handler")
+
+	# Now the late watchdog timeout fires. Must not flip _waiting_for_scan
+	# back to true, must not set _scan_timed_out (the scan succeeded).
+	runner._on_scan_watchdog_timeout()
+	assert_false(runner._waiting_for_scan, "watchdog stays no-op after settled wait")
+	assert_false(
+		runner._scan_timed_out,
+		"watchdog no-op must not poison _scan_timed_out — the scan actually succeeded",
+	)
+	_free_runner(runner)
+
+
+func test_finish_scan_wait_stops_armed_watchdog() -> void:
+	## Happy path: filesystem_changed signal arrives; `_finish_scan_wait`
+	## must stop the still-running Timer so it doesn't fire later and
+	## attempt a second cleanup. Verify by inspecting the Timer state.
+	var runner = _new_runner_in_tree()
+	_arm_scan_state(runner)
+	assert_false(runner._scan_watchdog_timer.is_stopped(), "timer running after arm")
+
+	runner._finish_scan_wait()
+
+	assert_true(
+		runner._scan_watchdog_timer.is_stopped(),
+		"finish_scan_wait must stop the watchdog so it can't fire later",
+	)
+	_free_runner(runner)
+
+
+func test_watchdog_timer_reused_across_arms() -> void:
+	## `_arm_scan_watchdog` lazy-creates the Timer on first use and reuses
+	## it on subsequent arms. The runner makes two filesystem scans during
+	## a single update (new files, then existing files), so the second arm
+	## must not leak a second Timer child.
+	var runner = _new_runner_in_tree()
+	_arm_scan_state(runner)
+	var first_timer = runner._scan_watchdog_timer
+	runner._finish_scan_wait()
+
+	_arm_scan_state(runner)
+	var second_timer = runner._scan_watchdog_timer
+
+	assert_true(first_timer == second_timer, "timer reused, not recreated")
+	runner._finish_scan_wait()
+	_free_runner(runner)
+
+
+func test_subsequent_scan_after_watchdog_bypasses_listener_arm() -> void:
+	## Cross-scan race regression (PR #381 review): if scan #1 watchdog'd,
+	## scan #2 must NOT arm a fresh `filesystem_changed` listener.
+	##
+	## Why: a delayed `filesystem_changed` emission from scan #1 fires on
+	## any listener currently connected to the shared signal — Godot can't
+	## tag emissions with their source scan. So if scan #2 has armed a
+	## fresh listener by the time scan #1's emission finally arrives, that
+	## listener fires and falsely settles scan #2 before its actual
+	## filesystem scan completed — re-enabling the plugin against a
+	## potentially-incomplete on-disk install.
+	##
+	## Fix: the watchdog sets the sticky `_scan_timed_out` flag, and
+	## `_start_filesystem_scan` checks it at the top and bypasses the
+	## connect+scan path entirely. The pending plugin re-enable still
+	## happens via `call_deferred(next_step)` — Godot's normal background
+	## scan catches up after the plugin re-enables.
+	var runner = _new_runner_in_tree()
+
+	# Scan #1: arm + watchdog timeout.
+	_arm_scan_state(runner)
+	runner._on_scan_watchdog_timeout()
+	assert_true(runner._scan_timed_out, "watchdog set the sticky flag")
+	assert_false(runner._waiting_for_scan, "watchdog cleared the wait")
+
+	# Scan #2 attempts to start. Pre-fix, this re-armed the listener. Now,
+	# the bypass path must short-circuit before _waiting_for_scan flips to
+	# true. We pass a benign deferred step (`set_process`) so the
+	# `call_deferred` in the bypass branch doesn't re-enable the plugin
+	# in the test environment.
+	runner._start_filesystem_scan("set_process")
+	assert_false(
+		runner._waiting_for_scan,
+		"post-watchdog _start_filesystem_scan must NOT arm a new listener — "
+		+ "no listener means no false-settle from a delayed scan-#1 emission",
+	)
+
+	_free_runner(runner)