Skip to content

feat: Send server info message on transcriber connection - #101

Merged
bgrozev merged 1 commit into
mainfrom
add-info-event
Jun 23, 2026
Merged

feat: Send server info message on transcriber connection#101
bgrozev merged 1 commit into
mainfrom
add-info-event

Conversation

@bgrozev

@bgrozev bgrozev commented Jun 23, 2026

Copy link
Copy Markdown
Member

Adds an info WebSocket message sent once per connection (and on reattach) carrying the running build, the effective provider, high-level session config, and deployment/runtime details. Motivation: runtime observability of what is actually deployed, independent of the deployment pipeline.

  • gitHash is baked into the bundle at build time via esbuild define (build.mjssrc/buildInfo.ts), so it is present for both docker:build and the Cloudflare/wrangler image build.
  • The CF Worker augments the message in-place with a worker block: deployed worker version (via the new version_metadata / CF_VERSION_METADATA binding), edge colo/location (request.cf), and routing mode.
  • Reports runtime (node vs cloudflare-container), instance id and location when available.
  • The proxy also logs any info message the client (JVB) sends.

Companion changes (JVB side + shared schema): jitsi/jitsi-videobridge#2423, jitsi/jicoco#236.

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Code Review — PR #101: feat: Send server info message on transcriber connection

Good feature overall — clean motivation, readable implementation, and appropriate documentation updates. A few items worth addressing:


Potential Bug: Worker Augmentation Only in Dispatcher Path

The info interception/augmentation in worker/index.ts is placed inside handleWebSocketWithDispatcher. If there is a separate handleWebSocket code-path executed when USE_DISPATCHER=false, connections on that path would receive the container's info message without the worker block. This means clients could see inconsistent message shapes depending on deployment config.

Suggestion: Verify that handleWebSocketWithDispatcher is the single WS-message routing function for all connections (dispatcher or not). If there is a separate non-dispatcher path, add the same info interception there too.


Minor: Fallback Inconsistency Between Build and Dev

In build.mjs the non-git fallback is 'unknown', but in src/buildInfo.ts the tsx/dev fallback is 'dev'. Both are documented comments, but a client receiving this field can't distinguish "built without git" from "running under tsx" unless it knows the convention.

Suggestion: Consider a single canonical fallback (e.g. 'dev') in both places, or keep both but document the distinction in the CLAUDE.md note (the PR description already explains it, but the code comment is brief).


Low-Risk: Log Injection via Client-Sent info

logger.info(`Received info from client for session ${this.sessionId}: ${JSON.stringify(parsedMessage)}`);

parsedMessage here is whatever the client sent. A misbehaving or probing client could send a large or specially crafted info payload that ends up verbatim in your log stream. For JVB-only deployments this is very low risk, but it's a good habit to either:

  • Log only known fields (parsedMessage.application, parsedMessage.version, etc.), or
  • Truncate/cap the serialised length before logging.

Test Coverage Gap

Tests cover buildServerInfo() well. The transcriberproxy.ts changes — receiving a client info event and the sendServerInfo() call — are not tested. These are straightforward code paths, but existing proxy unit tests (if any exercise setupWebSocketListeners) would benefit from:

  1. A case asserting that a client info message is logged (and doesn't crash or change state).
  2. A case asserting that ws.send is called with a message containing event: 'info' when the proxy connects.

Nits

  • src/serverInfo.ts line 27: opts.provider ?? getDefaultProvider() ?? undefined — the trailing ?? undefined is a no-op (if getDefaultProvider() returns null, null ?? undefined already yields undefined). It reads a bit odd; opts.provider ?? getDefaultProvider() ?? null (explicit null) or just dropping it to opts.provider ?? getDefaultProvider() and omitting the key when falsy would be cleaner.

  • wrangler.jsonc: The CF_VERSION_METADATA binding is typed as optional in env.d.ts (CF_VERSION_METADATA?), which is the right call. The if (env.CF_VERSION_METADATA) guard in buildWorkerInfo is consistent. Good.

  • CLAUDE.md update: Clear and accurate. The "An old client that doesn't know the info event type drops it harmlessly" note is a useful backwards-compatibility callout.


Summary

Area Assessment
Code correctness ✅ Good — safe fallbacks, readyState check, try/catch
Conventions ✅ CLAUDE.md updated per project requirement
Security ⚠️ Low-risk log injection concern (client info payload)
Test coverage ⚠️ transcriberproxy.ts changes untested
Potential bug ❓ Worker augmentation scope (dispatcher-only path?)

The feature is well-structured and the build-time git hash injection is a clean approach. Resolving the dispatcher-path question is the most important item before merging.

@bgrozev
bgrozev merged commit 622e72b into main Jun 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants