Conversation
|
commit: |
![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: aaa69fbe05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ws.addEventListener('open', () => { | ||
| this.reconnectAttempt = 0; | ||
| this.subscribeRequestId = crypto.randomUUID(); | ||
| this.send({ |
There was a problem hiding this comment.
When the TCP path is silently black-holed, this client never discovers the failure: the commit removes the server's control-frame heartbeat, but ChatWs only sends this subscription and never emits the new JSON ping or times out a missing response; GlobalEventsWs likewise sends nothing. Consequently neither close handler runs, so transcript updates and activity badges remain permanently stale instead of reconnecting and catching up. Add periodic ping/response tracking and force a reconnect on timeout in both shipped clients.
AGENTS.md reference: apps/kimi-inspect/AGENTS.md:L40-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 8dfc3dc: both v3 clients (transcript ChatWs and activity GlobalEventsWs) now run a shared WsHeartbeat — a uuid-request_id JSON ping every 10s, at most one outstanding, reconnect after 2 missed intervals; timer gaps from throttled tabs/system sleep are forgiven instead of counted. Response dispatch consults heartbeat.consume() before the subscribe matcher.
💡 Codex Reviewkimi-code/packages/kap-server/src/transport/ws/v3/wsConnectionV3.ts Lines 166 to 167 in 8dfc3dc  Retain server-side liveness detection
When a ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
8dfc3dc to
fe4ca35
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4ca3568d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (frame.type === 'ping') { | ||
| this.enqueue({ type: 'response', request_id: frame.request_id, code: ErrorCode.SUCCESS }); |
There was a problem hiding this comment.
When a client disappears without completing the TCP close handshake—for example, a laptop loses its network while subscribed—the new JSON heartbeat only lets the client detect that the server is unreachable; the server never receives the ping or the client's eventual close. Since the control-frame heartbeat was removed from both wsConnectionV3.ts and wsConnectionDebug.ts, these sockets can remain in the connection registry, hub, session lanes, and debug collector indefinitely while continuing to receive fanout. Keep a server-originated ping/pong timeout (independently of the client JSON heartbeat) so abandoned connections are released.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 57c9772, without reintroducing a server-originated heartbeat: the protocol already requires clients to run their own JSON ping (kimi-inspect does so every 10s), so any live connection has constant inbound traffic. Both wsConnectionV3 and wsConnectionDebug now stamp lastInboundAt on every inbound frame and terminate the socket after 10 minutes of silence (idleTimeoutMs, checked every 60s), which releases abandoned connections from the registry, hub, session lanes, and debug collector.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57c9772212
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (Date.now() - this.lastInboundAt <= this.idleTimeoutMs) return; | ||
| this.close(); |
There was a problem hiding this comment.
For /api/v1/debug/ws, a healthy subscribed client normally sends only the initial subscribe frame while the server streams inspection envelopes, so outbound activity never refreshes lastInboundAt; this new check therefore closes every valid long-running debug stream roughly 11 minutes after subscription. Fresh evidence beyond the earlier heartbeat comment is this revision's inbound-idle replacement, which now deterministically disconnects healthy receive-only clients; retain ping/pong probing for this endpoint or add an explicit client heartbeat protocol before enforcing inbound idleness.
Useful? React with 👍 / 👎.
Replace the server-pushed hello handshake and ack replies with a unified response message, move heartbeat from WS control frames to a client-initiated JSON ping, switch client request ids from numbers to string request_id, and replace numeric epoch-ms timestamps with ISO 8601 event_created_at across all v3 messages. Remove the WS-level heartbeat from the debug channel as well, and migrate kimi-inspect to the new handshake-free flow.
…clients The server no longer probes liveness after the protocol convergence, so both kimi-inspect clients (transcript and activity) now ping on an interval with a uuid request_id, track the matching response, and drop the socket into the reconnect path after two missed intervals. Timer gaps from throttled tabs or system sleep are forgiven instead of being counted as failures.
Since the protocol requires clients to run their own JSON ping, a healthy connection always has inbound traffic. Track the last inbound frame time on both the v3 and debug WS connections and terminate the socket once it has been silent for idleTimeoutMs (default 10 minutes), so abandoned connections are released from the registry, hub, and session lanes without reintroducing a server-originated heartbeat.
57c9772 to
7969b2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7969b2e4ff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (frame.type === 'ping') { | ||
| this.enqueue({ type: 'response', request_id: frame.request_id, code: ErrorCode.SUCCESS }); |
There was a problem hiding this comment.
When a live client has crossed the 1 MiB high-water mark, enqueue places this heartbeat response behind the existing application-message backlog. If that backlog takes more than two 10-second heartbeat intervals to drain, both shipped clients interpret the delayed response as a dead connection and reconnect even while they are still receiving server traffic; large recovery payloads or tool output on a slower link can therefore cause a reconnect loop. Send ping responses immediately or prioritize them over queued data so liveness is not coupled to application backpressure.
Useful? React with 👍 / 👎.
#3810) * feat(kap-server): converge v3 WS protocol on client JSON ping/response Replace the server-pushed hello handshake and ack replies with a unified response message, move heartbeat from WS control frames to a client-initiated JSON ping, switch client request ids from numbers to string request_id, and replace numeric epoch-ms timestamps with ISO 8601 event_created_at across all v3 messages. Remove the WS-level heartbeat from the debug channel as well, and migrate kimi-inspect to the new handshake-free flow. * feat(kimi-inspect): add client-side ping/response heartbeat to v3 WS clients The server no longer probes liveness after the protocol convergence, so both kimi-inspect clients (transcript and activity) now ping on an interval with a uuid request_id, track the matching response, and drop the socket into the reconnect path after two missed intervals. Timer gaps from throttled tabs or system sleep are forgiven instead of being counted as failures. * feat(kap-server): terminate inbound-idle WS connections after 10 minutes Since the protocol requires clients to run their own JSON ping, a healthy connection always has inbound traffic. Track the last inbound frame time on both the v3 and debug WS connections and terminate the socket once it has been silent for idleTimeoutMs (default 10 minutes), so abandoned connections are released from the registry, hub, and session lanes without reintroducing a server-originated heartbeat.
1 participant
Related Issue
Internal protocol-convergence task (no tracking issue). The v3 WS protocol is still pre-stable (
/api/v3/ws), so no compatibility is promised to older clients.Problem
The v3 WebSocket protocol carried three redundant mechanism families that the protocol design review decided to remove:
helloframe on connect and relied on RFC 6455 control-frame ping/pong for liveness. Control-frame heartbeats only let the server detect dead clients, and browser WebSocket APIs can neither send ping frames nor observe pongs — so the client (the side that actually reconnects) had no working liveness signal.id+ackreplies, a second id concept disjoint from the REST stringrequest_id.timestampalongside ISO 8601 fields, two time formats for the same purpose.What changed
Converge the v3 protocol on the reviewed design (
ping/response+ stringrequest_id+ ISO 8601event_created_at):hello/ackmessages; addping({type:'ping', request_id}) andresponse({type:'response', request_id, code, msg?});subscribe/unsubscribeswitch from numericidto stringrequest_id(z.string().min(1)); client message union now acceptsping.wsConnectionV3sends no handshake frame (protocol version is determined by the/api/v3/wspath) and answerspingimmediately withresponse{code:0}; the WS control-frame heartbeat timer is removed from both the v3 and debug channels; subscribe/unsubscribe replies becomeresponsewith the original timing guarantee (response precedes recovery messages);has_client_hello,V3_PROTOCOL_VERSION,V3_CAPABILITIES, and the hello-onlyserverIdoption are removed.epochMsSchemaand the numerictimestampfield are gone from all message bases; every event message carriesevent_created_at(ISO 8601). Value sources are unchanged (projection push time, wireevent.time, cold-foldrecordAtMs) — only the field name and format change. The duplicateisoDateTimeSchemainmessages/base.tsnow re-exports the agent-core-v2 one.crypto.randomUUID()request ids, matchesresponseinstead ofack, and compares event times as ISO strings.has_client_helloremoved from the connections route docs.Full suite on Linux: 873 files / 15,680 tests green.
Compatibility note: older clients that wait for
hello(including the pre-built web bundle underapps/kimi-code/dist-web) will not connect until the app repo adapts to the new protocol — accepted, since v3 is pre-stable.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.