Implement initial server catalog Websocket flow - #3
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 232f0eea53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if record.connection is not None and record.connection is not state: | ||
| record.duplicate = True | ||
| record.connection = None |
There was a problem hiding this comment.
When a driver reconnect overlaps with the old WebSocket, this branch sets record.duplicate = True and drops the tracked connection, but no disconnect path or later hello ever clears that flag. Because _handle_robot_status ignores duplicate records, the same robotId can never become available again after the conflicting connections go away, so a transient reconnect race leaves the robot unavailable until the server restarts.
Useful? React with 👍 / 👎.
| return | ||
| state.role = role | ||
| state.robot_id = robot_id | ||
| record = self.drivers.setdefault(robot_id, DriverRecord(robot_id=robot_id)) |
There was a problem hiding this comment.
Creating a DriverRecord during connection.hello makes a driver visible in catalog.get before it has ever sent a robot.status; the catalog entry then falls back to the robotId as the name and Droid as the type. When a driver has just connected, is stuck before its first status heartbeat, or the server has just restarted, pilots can see placeholder/incorrect robot entries even though the catalog is supposed to be built from Driver Status Reports and not show a robot until it reports.
Useful? React with 👍 / 👎.
| return | ||
| if role == ROLE_PILOT_CLIENT: | ||
| state.role = role | ||
| state.session_id = payload.get("sessionId") |
There was a problem hiding this comment.
If a pilot hello includes a non-string payload.sessionId (for example from corrupt local storage or an incompatible client), this assignment stores it and the subsequent hello response tries to include it as the envelope sessionId, causing make_envelope validation to raise instead of returning a failed connection.hello.result. That turns a bad resume claim into a dropped WebSocket/handler error rather than the displayable rejection described by the protocol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94d87a93a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if role == ROLE_PILOT_CLIENT: | ||
| state.role = role | ||
| state.session_id = payload.get("sessionId") | ||
| await self._send_result(state, TYPE_CONNECTION_HELLO_RESULT, envelope["messageId"], result_ok({"protocolVersion": PROTOCOL_VERSION, "role": role, "sessionResumed": False} if state.session_id else {"protocolVersion": PROTOCOL_VERSION, "role": role})) |
There was a problem hiding this comment.
When a pilot reconnects with a sessionId, this path accepts the hello and returns ok: true with sessionResumed: False even though this server has no session state and therefore cannot resume any claimed session. The protocol says a claimed session that cannot be accepted should produce a failed connection.hello.result, so after a server restart or stale local storage the client gets an accepted connection without a displayable rejection reason instead of being told the session is gone.
Useful? React with 👍 / 👎.
| detail = payload.get("availabilityDetail") | ||
| record.availability_detail = detail if isinstance(detail, dict) else None |
There was a problem hiding this comment.
When a driver reports an availabilityDetail map that is not a valid Display Reason, such as {"code": 404} or {"detail": "offline"}, this assignment preserves it and catalog.get later returns a catalog entry that violates the protocol's Display Reason shape. That malformed catalog payload can break pilot-side localization/display code even though the bad status report could be ignored or normalized here.
Useful? React with 👍 / 👎.
| except ProtocolError as exc: | ||
| LOGGER.warning("Rejecting invalid Ito envelope: %s", exc) | ||
| await self._send_error(state, TYPE_CONNECTION_HELLO_RESULT, None, "protocol.invalid_message") |
There was a problem hiding this comment.
When a peer sends a parseable connection.hello with the wrong protocolVersion, unpack_envelope raises before the server keeps the request messageId, and this handler replies with a generic protocol.invalid_message whose replyToMessageId is None. The protocol requires version rejections to be failed connection.hello.result responses, so mismatched clients cannot correlate the displayable failure to their hello request and may treat it as a timeout instead of a clear version-mismatch rejection.
Useful? React with 👍 / 👎.
| state.role = role | ||
| state.robot_id = robot_id |
There was a problem hiding this comment.
If a connected driver sends another connection.hello with a different robotId or role, these assignments overwrite the connection state without clearing the previous DriverRecord that still points at this same websocket. A buggy retry on one socket can therefore leave the old robot cataloged against a connection now owned by a different identity, and later disconnect cleanup only touches the new identity.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
server/ito/app.pythat accepts binary frames, requiresconnection.hello, performs role-based routing, and returns standard protocol result envelopes.DriverRecordandConnectionStateto track driver connections,robot.statushandling, disconnect handling, status freshness evaluation, and duplicaterobotIddetection that marks affected robots Unavailable.catalog.gethandling with optionalincludeUnavailablefiltering and catalog entry assembly from driver records.websocketstoserver/requirements.txt, created unit teststests/test_server_app.py, and updateddocs/todo.mdto mark catalog/status tasks complete and note remaining work on outbound request timeouts and session lifecycle.Testing
python -m pip install -r server/requirements.txtand the install succeeded.python -m pytest -qand all tests passed (11 passed).python -m compileall server testswhich completed successfully.This is a natural stopping point because the next TODOs cover acquisition and session-start/request timeouts which introduce significant session-state machinery best implemented and reviewed on top of this WebSocket/catalog foundation.
Codex Task