Skip to content

fix: log websocket handshake phase#82029

Closed
prantikmedhi wants to merge 9 commits into
openclaw:mainfrom
prantikmedhi:pr-81054
Closed

fix: log websocket handshake phase#82029
prantikmedhi wants to merge 9 commits into
openclaw:mainfrom
prantikmedhi:pr-81054

Conversation

@prantikmedhi

@prantikmedhi prantikmedhi commented May 15, 2026

Copy link
Copy Markdown

Adds a tracked websocket handshake phase so failures now include where the connection stalled.\n\nSummary:\n- track phase across upgrade, auth, session attach, and ready\n- include phase in close/timeout logs\n- add tests for phase logging and the handshake phase flow\n\nTests:\n- pnpm vitest src/gateway/server/ws-connection.test.ts src/gateway/server/ws-connection/message-handler.post-connect-health.test.ts\n\nCloses #79603

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime commands Command implementations agents Agent runtime and tooling size: L triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 15, 2026
@clawsweeper

clawsweeper Bot commented May 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the contribution. I reviewed the branch, and this PR is not a good landing base for OpenClaw.

Close: the gateway handshake-phase idea is useful, but this branch is not a good landing base because it mixes the stated gateway logging fix with unrelated model-exhaustion session/config/provider-routing work and is currently conflicting.

So I’m closing this PR rather than keeping an unmergeable branch open. A new narrow PR that carries only the useful part is welcome.

Review details

Best possible solution:

Close this branch, keep the linked gateway issue open, and accept a narrow gateway-only replacement while model-exhaustion cooldowns remain in their dedicated PRs or product review path.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows current main still logs only coarse WebSocket handshake state/cause, and the PR diff clearly shows the mixed-scope config/session/provider changes.

Is this the best way to solve the issue?

No. A gateway phase tracker is a plausible fix direction, but this branch is not the best solution because it mixes unrelated model-cooldown state/config work and is currently conflicting.

Security review:

Security review cleared: No concrete security or supply-chain issue was found; the blockers are compatibility, session-state, provider-routing scope, and missing real behavior proof.

AGENTS.md: found and applied where relevant.

What I checked:

  • Root policy read: Read the full root AGENTS.md and applied its whole-surface PR review, config/session compatibility, real behavior proof, and scoped-policy requirements. (AGENTS.md:1, f1b8827d20c8)
  • Scoped policy read: Read the gateway, agents, and plugins scoped AGENTS.md files for the touched runtime and plugin/session-slot surfaces; the only maintainer note found was Telegram-specific and not applicable. (src/gateway/AGENTS.md:1, f1b8827d20c8)
  • Current main still lacks the narrow gateway behavior: Current main's preauth timeout and pre-connect close logs carry cause/coarse handshake state, but no last-completed handshake phase field or log text. (src/gateway/server/ws-connection.ts:328, f1b8827d20c8)
  • PR includes the useful gateway idea: The PR head adds a GatewayWsHandshakePhase type and threads phase into timeout and closed-before-connect logs, so the stated idea is separable from the rest of the branch. (src/gateway/server/ws-connection/message-handler.ts:144, 82a0f42bdb3e)
  • Unrelated config/default surface: The PR head adds agents.defaults.modelExhaustionRetryMinutes, a new user config/default surface unrelated to WebSocket phase logging. (src/config/zod-schema.agent-defaults.ts:255, 82a0f42bdb3e)
  • Unrelated session/provider-routing behavior: The PR head adds model health persistence, session lookup plumbing, and skip/clear behavior in model fallback execution, which changes provider/model routing and session state rather than gateway diagnostics. (src/agents/model-fallback.ts:321, 82a0f42bdb3e)

Likely related people:

  • ragesaq: Current-main git blame and git log for the gateway WebSocket and model-fallback files point to recent broad work in the same runtime paths. (role: recent area contributor; confidence: medium; commits: a15427d605fe; files: src/gateway/server/ws-connection.ts, src/gateway/server/ws-connection/message-handler.ts, src/agents/model-fallback.ts)
  • prantikmedhi: Authored this branch and the two open dedicated model-exhaustion cooldown PRs that overlap the unrelated session/config part of this branch, so they are the likely split-follow-up contributor rather than current-main owner. (role: adjacent proposal owner; confidence: medium; commits: 82a0f42bdb3e, 717423995981, 937932563858; files: src/agents/model-fallback.ts, src/config/zod-schema.agent-defaults.ts, src/gateway/server/ws-connection.ts)

Codex review notes: model internal, reasoning high; reviewed against f1b8827d20c8.

@prantikmedhi

prantikmedhi commented May 15, 2026

Copy link
Copy Markdown
Author

Addressed the review notes: removed the unused model-health cooldown constant and moved ready to after hello-ok is sent successfully. Verified with src/gateway/server/ws-connection/message-handler.post-connect-health.test.ts.

@IWhatsskill IWhatsskill mentioned this pull request May 16, 2026
Merged
25 tasks
This was referenced May 16, 2026
Closed
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jun 2, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jun 2, 2026
@clawsweeper clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 2, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 14, 2026
@clawsweeper

clawsweeper Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper applied the proposed close for this PR.

@clawsweeper clawsweeper Bot closed this Jun 14, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling commands Command implementations gateway Gateway runtime merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: L stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@prantikmedhi