Skip to content

Fix production chat handshake timeout and Apollo warnings - #2451

Merged
JSv4 merged 1 commit into
mainfrom
fix/production-chat-handshake
Oct 18, 2026
Merged

JSv4 merged 1 commit into
mainfrom
fix/production-chat-handshake

Conversation

@JSv4

@JSv4 JSv4 commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

New corpus questions intermittently failed with HTTP 502 because database-backed corpus, access, and agent lookups exceeded Daphne's default five-second WebSocket handshake deadline. Allow 30 seconds in production startup commands. The matching GKE manifest change is tracked in https://github.com/Open-Source-Legal/opencontracts-infra/pull/30; its timeout mitigation is already running on both production replicas.

Production bundles also left Apollo's development checks enabled, producing encoded deprecation warnings. Define its development flag at build time. Show chat transport failures and reconnect without reloading the page or losing a drafted question.

Validation:

  • TypeScript compilation and production Vite build passed; no Apollo globalThis.__DEV__ checks remain in the generated JavaScript.
  • 34 WebSocket unit tests and four focused CorpusChat browser tests passed, including draft preservation and no duplicate initial question on reconnect.
  • Prettier, strict any baseline, pre-commit checks, shell syntax, and changelog validation passed.
  • After the production timeout rollout, five anonymous handshakes succeeded and a test response streamed to completion for corpus 297.

@claude

claude Bot commented Oct 18, 2026

Copy link
Copy Markdown

Review

Overall this is a small, focused fix and it looks sound. A few notes:

Maintainability

  • --websocket_connect_timeout 30 is now hardcoded in three places (start, start-no-collectstatic, production.yml). CLAUDE.md asks for no magic numbers and for DRY. Consider reading it from an env var with a default, e.g. --websocket_connect_timeout ${DJANGO_WS_CONNECT_TIMEOUT:-30}, the same way DJANGO_WORKER_TIMEOUT is handled. The next tuning then doesn't need a three-file edit. Note that production.yml needs $$ escaping for it.
  • The GKE manifest is tracked separately, so the value can drift between deployments. A cross-reference to that PR in the comment or changelog would help.

Frontend

  • The error text says "Retrying automatically." and the button offers a manual Reconnect. This is only accurate if useWebSocketAuth really keeps retrying after the failure. The test only exercises the manual button. If auto-retry stops after N attempts, the message will be misleading, so please confirm the hook's behaviour or reword it.
  • chatError is derived from wsTransportError only while !wsReady. Please confirm lastError is cleared on a successful open. The test covers this indirectly via the manual reconnect.
  • The Reconnect click calls setWsError(null) and then wsReconnect(). That is fine. The "no duplicate initial question on reconnect" claim in the PR description is not covered by the new test. It may be covered by existing tests, so please point to one if so.
  • The test pokes socket.readyState, onerror and onclose on the mock directly. That is acceptable for the mock, but it couples the test to the mock's internals. A helper on the mock such as simulateClose() would be sturdier.

Build

  • "globalThis.__DEV__" is driven by process.env.NODE_ENV at config-evaluation time. This works as you verified, but using the mode/command arguments to the Vite config would be more robust than depending on when Vite sets the env var. Also check that vitest and Playwright CT builds (which share this config) still behave as intended with __DEV__ = true.

Security / performance

  • A longer handshake window means unauthenticated or stalled sockets hold resources for up to 30s. That is an acceptable trade-off, but it is worth making sure the proxy and ingress timeouts are at least as long. That is the infra PR, so just confirm the ordering.
  • Treating this as a mitigation, the underlying slow permission lookups before accept() are probably worth a follow-up issue, for example accepting first and then authorizing. I don't think this PR needs to change for that.

The changelog fragment is present and follows the convention. LGTM with the above as suggestions.

@JSv4 JSv4 closed this Oct 18, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 18, 2026
@JSv4 JSv4 reopened this Oct 18, 2026
@Open-Source-Legal Open-Source-Legal unlocked this conversation Oct 18, 2026
@JSv4
JSv4 merged commit 8bc8254 into main Oct 18, 2026
32 of 34 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 18, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant