fix(client): address PR #5 review — race condition, buffer corruption, input error

Critical fixes from Hoshe review: 1. Race condition: defer TCP connect to _process() with retry logic (MAX_CONNECT_RETRIES=20, 100ms interval) so server has time to bind. 2. Buffer corruption: disconnect on oversized message instead of clearing valid buffered data (_corrupt flag, fail-safe). 3. Silent input drop: send_input() returns Error so callers can detect encode/validation failures. Warnings addressed: - ServerProcess validates server_path exists before spawning - SIGKILL and health check TODOs documented for future work - Diagonal keybindings documented as intentional deferral - send_message uses single put_data() call (no partial write risk) - Static frame helpers documented as D-030 Layer 2 test-only New tests (36 total, up from 33): - Partial read scenario (chunked TCP delivery) - Multi-message sequential decode (exercises buffer corruption fix) - send_input error return on invalid action Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-02-11 21:14:52 +01:00
parent c4e1b07346
commit 3befd2309c
5 changed files with 175 additions and 33 deletions
@@ -1,6 +1,8 @@
 extends Node

 # Semantic actions — NO raw key codes cross the bridge
+# Diagonal directions registered in project.godot with empty event arrays (intentional).
+# Keybindings deferred until input design is finalized — likely numpad or composite WASD.
 enum Action {
 	MOVE_NORTH, MOVE_NORTHEAST, MOVE_EAST, MOVE_SOUTHEAST,
 	MOVE_SOUTH, MOVE_SOUTHWEST, MOVE_WEST, MOVE_NORTHWEST,
@@ -15,6 +15,12 @@ var _server: ServerProcess = null
 var server_port: int = 9800
 var server_path: String = ""  # Path to server binary — set before connect_to_sim()

+# Connection retry state — handles server startup delay (Critical fix #1)
+const MAX_CONNECT_RETRIES: int = 20  # ~2 seconds at 60fps with 100ms delay
+const CONNECT_RETRY_INTERVAL: float = 0.1  # Seconds between retry attempts
+var _connect_retries: int = 0
+var _retry_timer: float = 0.0
+
 # Signals
 signal connection_state_changed(old_state: ConnectionState, new_state: ConnectionState)
 signal snapshot_received(snapshot: Dictionary)
@@ -32,7 +38,8 @@ func _set_state(new_state: ConnectionState) -> void:

 # Connect to simulation server.
 # In test mode, immediately transitions to CONNECTED.
-# In live mode, spawns server subprocess and connects via TCP.
+# In live mode, spawns server subprocess and defers TCP connection to _process()
+# to allow the server time to bind its port.
 func connect_to_sim() -> void:
 	_set_state(ConnectionState.CONNECTING)

@@ -49,14 +56,10 @@ func connect_to_sim() -> void:
 			_set_state(ConnectionState.ERROR)
 			return

-	# Connect TCP bridge
-	_bridge = LocalBridge.new()
-	var err := _bridge.connect_to_server("127.0.0.1", server_port)
-	if err != OK:
-		push_error("SimBridge: failed to initiate TCP connection: %s" % error_string(err))
-		_set_state(ConnectionState.ERROR)
-		return
-	# State transitions to CONNECTED in _process() when TCP handshake completes
+	# Defer TCP connection to _process() — server needs time to bind its port
+	_connect_retries = 0
+	_retry_timer = 0.0
+	_bridge = null

 # Disconnect from simulation server
 func disconnect_from_sim() -> void:
@@ -66,19 +69,61 @@ func disconnect_from_sim() -> void:
 	if _server != null:
 		_server.stop()
 		_server = null
+	_connect_retries = 0
 	_set_state(ConnectionState.DISCONNECTED)

+# Attempt TCP connection. Called from _process() during CONNECTING state.
+func _try_connect() -> void:
+	_bridge = LocalBridge.new()
+	var err := _bridge.connect_to_server("127.0.0.1", server_port)
+	if err != OK:
+		push_warning("SimBridge: TCP connect attempt %d/%d failed: %s" % [
+			_connect_retries + 1, MAX_CONNECT_RETRIES, error_string(err)])
+		_bridge = null
+
 # Poll transport layer every frame (non-test mode only)
-func _process(_delta: float) -> void:
-	if test_mode or _bridge == null:
+func _process(delta: float) -> void:
+	if test_mode:
+		return
+
+	# CONNECTING state: retry TCP connection until server is ready
+	if state == ConnectionState.CONNECTING:
+		if _bridge == null:
+			_retry_timer += delta
+			if _retry_timer >= CONNECT_RETRY_INTERVAL or _connect_retries == 0:
+				_retry_timer = 0.0
+				_connect_retries += 1
+				if _connect_retries > MAX_CONNECT_RETRIES:
+					push_error("SimBridge: TCP connection failed after %d retries" % MAX_CONNECT_RETRIES)
+					_set_state(ConnectionState.ERROR)
+					return
+				_try_connect()
+			return
+
+		# Bridge exists — poll for connection completion
+		_bridge.poll()
+		match _bridge.get_status():
+			StreamPeerTCP.STATUS_CONNECTED:
+				_set_state(ConnectionState.CONNECTED)
+			StreamPeerTCP.STATUS_CONNECTING:
+				pass  # Still connecting, wait
+			StreamPeerTCP.STATUS_ERROR:
+				# Connection attempt failed — retry
+				_bridge = null
+				if _connect_retries >= MAX_CONNECT_RETRIES:
+					push_error("SimBridge: TCP connection failed after %d retries" % MAX_CONNECT_RETRIES)
+					_set_state(ConnectionState.ERROR)
+			StreamPeerTCP.STATUS_NONE:
+				_bridge = null  # Reset and retry
+		return
+
+	if _bridge == null:
 		return

 	_bridge.poll()

 	match _bridge.get_status():
 		StreamPeerTCP.STATUS_CONNECTED:
-			if state != ConnectionState.CONNECTED:
-				_set_state(ConnectionState.CONNECTED)
 			# Receive: drain all complete messages from the bridge
 			var msg := _bridge.poll_message()
 			while msg.size() > 0:
@@ -91,7 +136,7 @@ func _process(_delta: float) -> void:
 				if err != OK:
 					push_error("SimBridge: failed to send message: %s" % error_string(err))
 		StreamPeerTCP.STATUS_CONNECTING:
-			pass  # Still connecting, wait
+			pass  # Should not happen in CONNECTED state
 		StreamPeerTCP.STATUS_ERROR:
 			if state != ConnectionState.ERROR:
 				push_error("SimBridge: TCP connection error")
@@ -104,20 +149,23 @@ func _process(_delta: float) -> void:
 # Send input to simulation server.
 # player_input: Dictionary with "action" (int from InputMapper.Action enum) and "timestamp_msec".
 # In test mode, inputs are silently dropped. In live mode, encoded and buffered for transport.
-func send_input(player_input: Dictionary) -> void:
+# Returns OK on success, or an error code on failure.
+func send_input(player_input: Dictionary) -> Error:
 	if state != ConnectionState.CONNECTED:
-		return
+		return ERR_CONNECTION_ERROR
 	if test_mode:
-		return
+		return OK
 	var action_name := _action_enum_to_wire(player_input.get("action", -1))
 	if action_name.is_empty():
-		return
+		# _action_enum_to_wire already emits push_warning for invalid actions
+		return ERR_INVALID_PARAMETER
 	var tick: int = player_input.get("timestamp_msec", 0)
 	var encoded := Protocol.encode_player_input(tick, action_name)
 	if encoded.size() == 0:
 		push_error("SimBridge: failed to encode player input (action=%s)" % action_name)
-		return
+		return ERR_CANT_CREATE
 	_outbound_buffer.append(encoded)
+	return OK

 # Poll for snapshot from simulation.
 # In test mode returns hardcoded data. In live mode, returns the last decoded snapshot (if any).
@@ -9,6 +9,7 @@ const MAX_MESSAGE_SIZE: int = 16 * 1024 * 1024  # 16 MB, matching server
 var _stream: StreamPeerTCP
 var _read_buffer: PackedByteArray = PackedByteArray()
 var _pending_length: int = -1  # -1 = awaiting header, >= 0 = awaiting payload
+var _corrupt: bool = false  # Set on unrecoverable framing error


 func _init() -> void:
@@ -27,15 +28,18 @@ func poll() -> void:

 ## Current TCP connection status.
 func get_status() -> StreamPeerTCP.Status:
+	if _corrupt:
+		return StreamPeerTCP.STATUS_ERROR
 	return _stream.get_status()


 ## Whether the TCP stream is in the connected state.
 func is_connected_to_server() -> bool:
-	return _stream.get_status() == StreamPeerTCP.STATUS_CONNECTED
+	return not _corrupt and _stream.get_status() == StreamPeerTCP.STATUS_CONNECTED


 ## Send a framed message: [4-byte BE length][payload].
+## Concatenates header and payload into a single put_data() call to avoid partial writes.
 func send_message(payload: PackedByteArray) -> Error:
 	if not is_connected_to_server():
 		return ERR_CONNECTION_ERROR
@@ -45,18 +49,17 @@ func send_message(payload: PackedByteArray) -> Error:
 		push_error("LocalBridge: message too large: %d bytes (max %d)" % [len, MAX_MESSAGE_SIZE])
 		return ERR_PARAMETER_RANGE_ERROR

-	# 4-byte big-endian length header
-	var header := PackedByteArray()
-	header.resize(4)
-	header[0] = (len >> 24) & 0xFF
-	header[1] = (len >> 16) & 0xFF
-	header[2] = (len >> 8) & 0xFF
-	header[3] = len & 0xFF
+	# Build complete frame: [4-byte BE length][payload]
+	var frame := PackedByteArray()
+	frame.resize(4 + len)
+	frame[0] = (len >> 24) & 0xFF
+	frame[1] = (len >> 16) & 0xFF
+	frame[2] = (len >> 8) & 0xFF
+	frame[3] = len & 0xFF
+	for i in range(len):
+		frame[4 + i] = payload[i]

-	var err := _stream.put_data(header)
-	if err != OK:
-		return err
-	return _stream.put_data(payload)
+	return _stream.put_data(frame)


 ## Non-blocking poll for a complete framed message.
@@ -88,9 +91,13 @@ func _try_extract_message() -> PackedByteArray:
 		_read_buffer = _read_buffer.slice(4)

 		if _pending_length > MAX_MESSAGE_SIZE:
-			push_error("LocalBridge: incoming message too large: %d bytes (max %d)" % [_pending_length, MAX_MESSAGE_SIZE])
+			# Stream is corrupt — we can't find the next valid frame boundary.
+			# Disconnect rather than silently discarding valid buffered data.
+			push_error("LocalBridge: incoming message too large: %d bytes (max %d) — disconnecting" % [_pending_length, MAX_MESSAGE_SIZE])
+			_corrupt = true
 			_pending_length = -1
 			_read_buffer.clear()
+			disconnect_from_server()
 			return PackedByteArray()

 	# Phase 2: read payload
@@ -110,7 +117,16 @@ func disconnect_from_server() -> void:
 	_pending_length = -1


-# -- Static helpers for framing (used in tests without a live TCP connection) --
+## Reset corrupt state (for reconnection after disconnect).
+func reset() -> void:
+	_corrupt = false
+	_read_buffer.clear()
+	_pending_length = -1
+
+
+# -- Static helpers for D-030 Layer 2 testing ----------------------------------
+# These exist for unit tests that verify framing logic without a live TCP
+# connection. Not used in production code paths.

 ## Encode a payload into a framed byte array: [4-byte BE length][payload].
 static func frame_encode(payload: PackedByteArray) -> PackedByteArray:
@@ -5,10 +5,14 @@ var _pid: int = -1


 ## Start the server process. Returns the PID, or -1 on failure.
+## Validates that server_path exists and is accessible before spawning.
 func start(server_path: String, args: Array = []) -> int:
 	if _pid > 0 and is_alive():
 		push_warning("ServerProcess: server already running (pid %d)" % _pid)
 		return _pid
+	if not FileAccess.file_exists(server_path):
+		push_error("ServerProcess: server binary not found at %s" % server_path)
+		return -1
 	_pid = OS.create_process(server_path, args)
 	if _pid <= 0:
 		push_error("ServerProcess: failed to start server at %s" % server_path)
@@ -16,6 +20,8 @@ func start(server_path: String, args: Array = []) -> int:


 ## Check if the server process is still running.
+## NOTE: OS.is_process_running() doesn't distinguish starting/healthy/zombie.
+## TODO: Add health check (e.g. TCP readiness probe) when server supports it.
 func is_alive() -> bool:
 	if _pid <= 0:
 		return false
@@ -23,6 +29,7 @@ func is_alive() -> bool:


 ## Stop the server process.
+## TODO: Send graceful shutdown signal before SIGKILL when server supports it.
 func stop() -> void:
 	if _pid > 0 and is_alive():
 		OS.kill(_pid)