game_eval: bound eval with a deadline and always free the temp node (#488)

* game_eval: bound eval with a deadline and always free the temp node

Evaluated GDScript that awaits something which never completes (a signal
that never fires, a timer on a paused tree) hung _handle_eval's
`await temp_node.execute()` with no escape hatch: the reply and
queue_free() below were never reached, leaking the eval Node and pinning
the request until the dispatcher's 15s deferred budget / the server's 15s
command timeout fired it back as an opaque INTERNAL_ERROR.

Telemetry shows this is ~19-20% of all game_eval calls, steady across
2.5.6-2.5.9 and all three platforms (500+ installs), with failed-call
durations clustered at the 10-15s timeout thresholds vs ~40ms for
successes.

Drive execute() as a fire-and-forget coroutine recording into a holder,
poll process_frame until done or an 8s deadline (comfortably under the
15s ceiling, with round-trip margin), and on timeout detach + free the
node and reply with an actionable mcp:eval_error instead of letting the
request ride to INTERNAL_ERROR. Does not cover CPU-bound loops with no
await (they block the main thread, including this poll).

Refs #487

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* game_eval: document timeout ordering + extract eval reply helpers

Follow-up cleanups from a code review of the deadline guard, all
behavior-preserving:

- Document the load-bearing cross-file timeout ordering (game 8s <
  editor 10s < dispatcher 15s). Only the game-side EVAL_TIMEOUT_SEC
  guard emits the actionable "Eval exceeded 8s" message; the editor's
  fallback timer emits a generic one. The relationship lives in three
  separate files with nothing enforcing it, so a tweak to any one
  could silently swap the actionable diagnostic for the generic
  timeout. Note added at both EVAL_TIMEOUT_SEC and request_game_eval.
- Note the accepted residual leak in _drive_eval: an await that never
  fires leaves the coroutine suspended and its (detached) node unfreed
  for the game-process lifetime. GDScript can't cancel a suspended
  coroutine; still strictly better than the pre-#487 in-tree leak.
- Extract _reply_eval_error / _reply_eval_response to match the file's
  existing _reply_error / _reply_game_command_error convention,
  replacing four inline EngineDebugger.send_message call sites.

Verified in a live editor (4.6.3): normal eval, sub-deadline await,
and the 8s timeout path all return correctly through the new helpers;
no node leak; headless --import is parse-clean.

Refs #487

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

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

Author: dsarno

plugin/addons/godot_ai/debugger/mcp_debugger_plugin.gd

@@ -331,6 +331,13 @@ func _clear_pending(request_id: String) -> void:
 
 ## --- game_eval: execute arbitrary GDScript in the running game ---
 
+## Editor-side fallback timer for game_eval. MUST stay above the game-side
+## EVAL_TIMEOUT_SEC (8.0) in runtime/game_helper.gd and below the dispatcher's
+## game_eval budget (15000 ms) in dispatcher.gd — i.e. game 8s < editor 10s <
+## dispatcher 15s. This timer only fires when the game never replies at all,
+## and its message (the timeout_callable below) is intentionally generic. Drop
+## timeout_sec at/below 8s and it pre-empts the game's actionable "Eval
+## exceeded 8s" message — see the TIMEOUT ORDERING note on EVAL_TIMEOUT_SEC.
 func request_game_eval(
 	code: String,
 	request_id: String,

plugin/addons/godot_ai/runtime/game_helper.gd

@@ -519,12 +519,38 @@ func _mouse_button_index(name: String) -> int:
 
 ## --- game_eval: execute arbitrary GDScript in the running game ---
 
+## Wall-clock ceiling for a single game_eval. Evaluated code that awaits
+## something which never completes (a signal that never fires, a timer on a
+## paused tree) would otherwise pin the request open until the dispatcher's
+## 15s deferred budget / the server's 15s command timeout fires it as an
+## opaque INTERNAL_ERROR — with the temp eval Node leaked into the tree.
+## Bounding it here lets us free the node and reply with an actionable
+## message instead. See hi-godot/godot-ai#487.
+##
+## TIMEOUT ORDERING — load-bearing across three files: this value MUST stay
+## below the editor-side fallback timer in
+## `debugger/mcp_debugger_plugin.gd::request_game_eval` (`timeout_sec`,
+## default 10.0), which in turn stays below the dispatcher's `game_eval`
+## budget in `dispatcher.gd` (15000 ms). So: game 8s < editor 10s <
+## dispatcher 15s. Only this game-side guard emits the actionable
+## "Eval exceeded 8s" message; the editor timer emits a *generic* "Game eval
+## timed out" message. Raise this at/above the editor timer (or drop that
+## timer below this) and the generic message wins the race, silently losing
+## the diagnostic this fix exists to provide. Nothing enforces the order —
+## change one, re-check the other two.
+##
+## NOTE: this catches a hung `await`, not a CPU-bound loop with no `await` —
+## a tight `while true:` with no yield blocks the main thread, so nothing
+## (including this poll) runs until it yields. That case is out of scope.
+const EVAL_TIMEOUT_SEC := 8.0
+
+
 func _handle_eval(data: Array) -> void:
 	var request_id: String = data[0] if data.size() > 0 else ""
 	var code: String = data[1] if data.size() > 1 else ""
 
 	if code.is_empty():
-		EngineDebugger.send_message("mcp:eval_error", [request_id, "No code provided"])
+		_reply_eval_error(request_id, "No code provided")
 		return
 
 	## Wrap user code so we can capture a return value.
@@ -543,22 +569,85 @@ func _handle_eval(data: Array) -> void:
 	script.source_code = script_source
 	var err: int = script.reload()
 	if err != OK:
-		EngineDebugger.send_message("mcp:eval_error",
-			[request_id, "Failed to compile GDScript (error %d). Check syntax." % err])
+		_reply_eval_error(request_id,
+			"Failed to compile GDScript (error %d). Check syntax." % err)
 		return
 
 	var temp_node := Node.new()
 	temp_node.set_script(script)
 	temp_node.process_mode = Node.PROCESS_MODE_ALWAYS
 	add_child(temp_node)
 
-	var result = null
-	if temp_node.has_method("execute"):
-		result = await temp_node.execute()
+	if not temp_node.has_method("execute"):
+		temp_node.queue_free()
+		_reply_eval_error(request_id, "Internal error: eval wrapper is missing execute().")
+		return
+
+	## Drive execute() as a fire-and-forget coroutine that records its
+	## outcome into `holder`, then poll frames until it finishes or the
+	## deadline passes. A plain `await temp_node.execute()` has no escape
+	## hatch: if user code never returns, we never reach the reply/cleanup
+	## below and the request hangs with the node leaked.
+	var holder := {"done": false, "value": null, "abandoned": false}
+	_drive_eval(temp_node, holder)
+
+	var tree := get_tree()
+	var deadline_ms := int(EVAL_TIMEOUT_SEC * 1000.0)
+	var start_ms := Time.get_ticks_msec()
+	## process_frame fires every idle frame regardless of tree pause, so this
+	## deadline still elapses while the game is paused.
+	while not holder["done"] and (Time.get_ticks_msec() - start_ms) < deadline_ms:
+		await tree.process_frame
+
+	if not holder["done"]:
+		## Still running past the deadline. Mark abandoned so a late
+		## completion in _drive_eval drops its result and frees the node,
+		## detach the node now so it stops touching the scene, and reply with
+		## a clear timeout instead of letting the request ride to INTERNAL_ERROR.
+		holder["abandoned"] = true
+		if is_instance_valid(temp_node):
+			remove_child(temp_node)
+		_reply_eval_error(request_id,
+			("Eval exceeded %ds and was aborted — the code likely awaits "
+				+ "something that never completes (a signal that never fires, a timer on "
+				+ "a paused tree) or loops forever. Check logs_read(source='game').")
+				% int(EVAL_TIMEOUT_SEC))
+		return
 
 	temp_node.queue_free()
+	_reply_eval_response(request_id, holder["value"])
+
+
+## Run the compiled eval node's execute() and stash the result. Kept
+## separate from _handle_eval so the latter can race it against a deadline
+## via frame polling. If the eval was abandoned (timed out) before this
+## resumes, drop the result and free the now-detached node — _handle_eval
+## has already replied.
+##
+## RESIDUAL LEAK (accepted): if the awaited thing *never* fires, this
+## coroutine never resumes, so the `node` it holds is detached (via
+## _handle_eval's remove_child) but never freed — one orphaned Node per such
+## timeout, for the game-process lifetime. GDScript has no way to cancel a
+## suspended coroutine, so this is the best achievable in-process. It is still
+## strictly better than the pre-#487 behavior, where the node leaked *into*
+## the live tree and the request hung to the 15s ceiling.
+func _drive_eval(node: Node, holder: Dictionary) -> void:
+	var value = await node.execute()
+	if holder.get("abandoned", false):
+		if is_instance_valid(node):
+			node.queue_free()
+		return
+	holder["value"] = value
+	holder["done"] = true
+
+
+func _reply_eval_error(request_id: String, message: String) -> void:
+	EngineDebugger.send_message("mcp:eval_error", [request_id, message])
+
+
+func _reply_eval_response(request_id: String, value: Variant) -> void:
 	EngineDebugger.send_message("mcp:eval_response",
-		[request_id, JSON.stringify(_variant_to_json(result))])
+		[request_id, JSON.stringify(_variant_to_json(value))])
 
 
 func _indent_eval_code(code: String) -> String: