mirror of
https://github.com/jcreek/CosmicClash.git
synced 2026-09-14 01:22:17 +00:00
fix(multiplayer): actually connect once matchmaking assigns a match
A player who completed the entire queue -> proposal -> allocate -> assign pipeline would reach ASSIGNED, see "Your match server is ready", and then simply sit there forever. connect_to_assignment() existed in control_plane_client.gd, fully validated (checks the assignment is available and fresh, splits and validates the endpoint, carries the join authorisation via MatchNet's hello payload rather than the URL) with its own assignment_connection_started/ assignment_connection_failed signals, and multiplayer-next.md's own §8.41 row already described it as wired -- but grepping the whole client found zero callers. Nothing anywhere in matchmaking.gd or control_plane_client.gd itself ever invoked it. ControlPlaneClient._connect_when_assigned() now calls it automatically the moment state.phase reaches ASSIGNED. Wired into the single call site every queue-shaped HTTP response already shares (heartbeat, recover, and resync-triggered recover alike, since the WebSocket match-lifecycle path always funnels into a REST resync first), so both the ordinary poll path and the WebSocket-push path are covered without a second call site to keep in sync. Two orderings are handled: if the assignment fetch triggered earlier by ASSIGNMENT_READY has already completed, it connects immediately; if not, it defers via _pending_connect_match_id and resolves once the assignment becomes available. _connect_attempted_match_id guards against a duplicate or replayed ASSIGNED event reattempting the connection. Verified against the real Godot 4.7.1 binary now that headless testing has resumed: two new test_control_plane_client.gd tests cover both orderings and the duplicate-attempt guard directly (216/216 total, 0 failed, no crash, no engine-level error, stable across repeated runs); full make verify-multiplayer-local and the complete make verify-enet-integration suite (all five cases, including the 3-process match) both pass clean; zero new crash reports throughout. multiplayer-next.md's §8.41 row is corrected to describe what was actually true (connect_to_assignment existed but was never called) rather than repeating the prior, inaccurate 'already wired' claim.
This commit is contained in:
@@ -38,6 +38,16 @@ var _authoritative_recovery_seconds := AUTHORITATIVE_RECOVERY_INTERVAL_SECONDS
|
|||||||
var _pending_proposal_id := ""
|
var _pending_proposal_id := ""
|
||||||
var _pending_assignment_match_id := ""
|
var _pending_assignment_match_id := ""
|
||||||
var _pending_resync_resource_id := ""
|
var _pending_resync_resource_id := ""
|
||||||
|
# The final wiring step of the matchmaking pipeline: once state.phase reaches
|
||||||
|
# ASSIGNED, the client must actually start the game transport. connect_to_assignment()
|
||||||
|
# already existed with correct validation/signal behavior, but nothing ever
|
||||||
|
# called it -- a player would sit on "Your match server is ready" forever.
|
||||||
|
# These two fields defer the connect attempt until the assignment fetch
|
||||||
|
# (triggered independently, earlier, by ASSIGNMENT_READY) has actually
|
||||||
|
# completed, and prevent a duplicate/replayed ASSIGNED update from firing a
|
||||||
|
# second connection attempt for the same match.
|
||||||
|
var _pending_connect_match_id := ""
|
||||||
|
var _connect_attempted_match_id := ""
|
||||||
|
|
||||||
|
|
||||||
func _ready() -> void:
|
func _ready() -> void:
|
||||||
@@ -85,9 +95,39 @@ func _process(_delta: float) -> void:
|
|||||||
var match_id := _pending_assignment_match_id
|
var match_id := _pending_assignment_match_id
|
||||||
_pending_assignment_match_id = ""
|
_pending_assignment_match_id = ""
|
||||||
fetch_assignment(match_id)
|
fetch_assignment(match_id)
|
||||||
|
if not _pending_connect_match_id.is_empty() and _assignment_ready_for(_pending_connect_match_id):
|
||||||
|
var match_id := _pending_connect_match_id
|
||||||
|
_pending_connect_match_id = ""
|
||||||
|
_connect_attempted_match_id = match_id
|
||||||
|
connect_to_assignment()
|
||||||
_poll_authoritative_recovery(_delta)
|
_poll_authoritative_recovery(_delta)
|
||||||
|
|
||||||
|
|
||||||
|
# The assignment fetch (triggered independently by ASSIGNMENT_READY, which
|
||||||
|
# always precedes ASSIGNED) and the ASSIGNED transition that should start the
|
||||||
|
# transport can arrive in either order. This is the shared readiness check
|
||||||
|
# both _connect_when_assigned and the deferred _process retry above use.
|
||||||
|
func _assignment_ready_for(match_id: String) -> bool:
|
||||||
|
return assignment != null and assignment.available and assignment.match_id == match_id and _assignment_is_fresh(assignment)
|
||||||
|
|
||||||
|
|
||||||
|
# Starts (or defers, if the assignment fetch triggered by the earlier
|
||||||
|
# ASSIGNMENT_READY event hasn't completed yet) the game transport once the
|
||||||
|
# ticket-state machine reaches ASSIGNED. connect_to_assignment() itself
|
||||||
|
# already existed with full validation and failure signalling; nothing ever
|
||||||
|
# called it, so a player reaching "Your match server is ready" never actually
|
||||||
|
# connected. _connect_attempted_match_id guards against a duplicate/replayed
|
||||||
|
# ASSIGNED update firing a second connection attempt for the same match.
|
||||||
|
func _connect_when_assigned(match_id: String) -> void:
|
||||||
|
if state.phase != MatchmakingState.ASSIGNED or not is_valid_resource_id(match_id) or match_id == _connect_attempted_match_id:
|
||||||
|
return
|
||||||
|
if _assignment_ready_for(match_id):
|
||||||
|
_connect_attempted_match_id = match_id
|
||||||
|
connect_to_assignment()
|
||||||
|
else:
|
||||||
|
_pending_connect_match_id = match_id
|
||||||
|
|
||||||
|
|
||||||
func configure(url: String, token: String) -> bool:
|
func configure(url: String, token: String) -> bool:
|
||||||
var normalized := url.strip_edges().trim_suffix("/")
|
var normalized := url.strip_edges().trim_suffix("/")
|
||||||
var normalized_token := token.strip_edges()
|
var normalized_token := token.strip_edges()
|
||||||
@@ -508,6 +548,7 @@ func _on_request_completed(result: HTTPRequest.Result, response_code: int, _head
|
|||||||
if state.apply_ticket_update(normalize_ticket(payload), operation == "queue_recover"):
|
if state.apply_ticket_update(normalize_ticket(payload), operation == "queue_recover"):
|
||||||
_queue_proposal_if_ready(payload)
|
_queue_proposal_if_ready(payload)
|
||||||
_queue_assignment_if_ready(payload)
|
_queue_assignment_if_ready(payload)
|
||||||
|
_connect_when_assigned(String(payload.get("match_id", "")))
|
||||||
elif operation.begins_with("proposal_"):
|
elif operation.begins_with("proposal_"):
|
||||||
state.apply_proposal_update(normalize_proposal(payload))
|
state.apply_proposal_update(normalize_proposal(payload))
|
||||||
elif operation == "ranked_profile":
|
elif operation == "ranked_profile":
|
||||||
|
|||||||
@@ -231,6 +231,85 @@ func test_generic_mutation_retry_is_not_offered_for_unsafe_failures() -> void:
|
|||||||
client.free()
|
client.free()
|
||||||
|
|
||||||
|
|
||||||
|
# connect_to_assignment() already existed, fully validated, with its own
|
||||||
|
# assignment_connection_started/assignment_connection_failed signals -- but
|
||||||
|
# nothing anywhere in the client ever called it. A player reaching the
|
||||||
|
# ASSIGNED phase (server confirms the complete roster) with a fetched, fresh
|
||||||
|
# assignment would simply sit on "Your match server is ready" forever,
|
||||||
|
# because the transport was never actually started. This is the wiring fix,
|
||||||
|
# not just new test coverage for existing behavior.
|
||||||
|
func test_client_starts_the_transport_once_the_ticket_reaches_assigned() -> void:
|
||||||
|
var client := ControlPlaneClient.new()
|
||||||
|
client._ready()
|
||||||
|
assert_true(client.configure("https://match.example", "session-id:opaque-token"), "client configures")
|
||||||
|
client.player_id = "player_1234567890"
|
||||||
|
assert_true(client.state.begin_queue("ticket-connect-ready", "casual"), "queue setup succeeds")
|
||||||
|
|
||||||
|
# The assignment fetch (triggered independently, earlier, by
|
||||||
|
# ASSIGNMENT_READY) has already completed by the time ASSIGNED arrives --
|
||||||
|
# the common case.
|
||||||
|
client._operation = "assignment"
|
||||||
|
var assignment_payload := {"match_id": "match_connect_1234567890", "server_id": "server_connect_1234567890", "player_id": "player_1234567890", "slot": 0, "expires_at": "2099-08-31T12:00:00Z", "protocol_version": 1, "transport": "enet", "endpoint": "127.0.0.1:65500", "join_authorisation": "opaque-join-token"}
|
||||||
|
client._on_request_completed(HTTPRequest.RESULT_SUCCESS, 200, PackedStringArray(), JSON.stringify(assignment_payload).to_utf8_buffer())
|
||||||
|
assert_true(client.assignment.available, "assignment fetch applies")
|
||||||
|
|
||||||
|
var connect_started := [false]
|
||||||
|
var connect_failed := [false]
|
||||||
|
client.assignment_connection_started.connect(func(_a): connect_started[0] = true)
|
||||||
|
client.assignment_connection_failed.connect(func(_d): connect_failed[0] = true)
|
||||||
|
|
||||||
|
client._operation = "queue_recover"
|
||||||
|
var ticket_payload := {"ticket_id": "ticket-connect-ready", "player_id": "player_1234567890", "playlist": "casual", "revision": 5, "state": "ASSIGNED", "match_id": "match_connect_1234567890", "enqueued_at": "2026-08-31T12:00:00Z", "expires_at": "2099-08-31T12:00:00Z"}
|
||||||
|
client._on_request_completed(HTTPRequest.RESULT_SUCCESS, 200, PackedStringArray(), JSON.stringify(ticket_payload).to_utf8_buffer())
|
||||||
|
|
||||||
|
# connect_to_assignment() itself calls state.mark_connecting() as part of a
|
||||||
|
# successful attempt, so by the time control returns here phase has
|
||||||
|
# already advanced past ASSIGNED to CONNECTING -- that advancement is
|
||||||
|
# itself the proof the connect was actually attempted.
|
||||||
|
assert_eq(client.state.phase, MatchmakingState.CONNECTING, "reaching ASSIGNED with a ready assignment actually started the transport, rather than sitting idle")
|
||||||
|
assert_true(connect_started[0] or connect_failed[0], "connect_to_assignment's own signal fired")
|
||||||
|
assert_true(client._pending_connect_match_id.is_empty(), "an attempted connect is not left pending")
|
||||||
|
|
||||||
|
# A duplicate/replayed ASSIGNED event for the same match (e.g. an
|
||||||
|
# at-least-once outbox redelivery) must not fire a second connection
|
||||||
|
# attempt. Called directly against the guarded function rather than
|
||||||
|
# through another full _on_request_completed round-trip: phase has
|
||||||
|
# already moved on to CONNECTING, so both of _connect_when_assigned's own
|
||||||
|
# guards (phase != ASSIGNED, and the _connect_attempted_match_id match)
|
||||||
|
# now independently refuse a second attempt for this match.
|
||||||
|
connect_started[0] = false
|
||||||
|
connect_failed[0] = false
|
||||||
|
client._connect_when_assigned("match_connect_1234567890")
|
||||||
|
assert_true(not connect_started[0] and not connect_failed[0], "a duplicate connect attempt for an already-attempted match is not reattempted")
|
||||||
|
|
||||||
|
NetworkManager.shutdown()
|
||||||
|
client.free()
|
||||||
|
|
||||||
|
|
||||||
|
func test_client_defers_the_connect_until_the_assignment_fetch_completes() -> void:
|
||||||
|
var client := ControlPlaneClient.new()
|
||||||
|
client._ready()
|
||||||
|
assert_true(client.configure("https://match.example", "session-id:opaque-token"), "client configures")
|
||||||
|
assert_true(client.state.begin_queue("ticket-connect-deferred", "casual"), "queue setup succeeds")
|
||||||
|
|
||||||
|
var connect_started := [false]
|
||||||
|
var connect_failed := [false]
|
||||||
|
client.assignment_connection_started.connect(func(_a): connect_started[0] = true)
|
||||||
|
client.assignment_connection_failed.connect(func(_d): connect_failed[0] = true)
|
||||||
|
|
||||||
|
# ASSIGNED arrives before the assignment fetch (triggered earlier by
|
||||||
|
# ASSIGNMENT_READY) has actually completed -- the ordering the deferred
|
||||||
|
# path exists for. client.assignment is still the default, unavailable one.
|
||||||
|
client._operation = "queue_recover"
|
||||||
|
var ticket_payload := {"ticket_id": "ticket-connect-deferred", "player_id": "player_1234567890", "playlist": "casual", "revision": 5, "state": "ASSIGNED", "match_id": "match_deferred_1234567890", "enqueued_at": "2026-08-31T12:00:00Z", "expires_at": "2099-08-31T12:00:00Z"}
|
||||||
|
client._on_request_completed(HTTPRequest.RESULT_SUCCESS, 200, PackedStringArray(), JSON.stringify(ticket_payload).to_utf8_buffer())
|
||||||
|
|
||||||
|
assert_eq(client.state.phase, MatchmakingState.ASSIGNED, "ticket state machine still reaches ASSIGNED")
|
||||||
|
assert_eq(client._pending_connect_match_id, "match_deferred_1234567890", "the connect attempt is deferred until the assignment is actually available")
|
||||||
|
assert_true(not connect_started[0] and not connect_failed[0], "no connection attempt is made before the assignment is ready -- nothing to connect to yet")
|
||||||
|
client.free()
|
||||||
|
|
||||||
|
|
||||||
func test_rest_resource_identifiers_use_the_opaque_contract_shape() -> void:
|
func test_rest_resource_identifiers_use_the_opaque_contract_shape() -> void:
|
||||||
assert_true(ControlPlaneClient.is_valid_resource_id("ticket_1234567890"), "contract-sized resource id is accepted")
|
assert_true(ControlPlaneClient.is_valid_resource_id("ticket_1234567890"), "contract-sized resource id is accepted")
|
||||||
assert_true(not ControlPlaneClient.is_valid_resource_id("ticket-1"), "short resource id is rejected")
|
assert_true(not ControlPlaneClient.is_valid_resource_id("ticket-1"), "short resource id is rejected")
|
||||||
|
|||||||
+1
-1
File diff suppressed because one or more lines are too long
Reference in New Issue
Block a user