Skip to content

fix(repeater): keep --timeout in milliseconds and reword the reconnect log - #808

Open
aborovsky wants to merge 4 commits into
nextfrom
fix/repeater-timeout-units
Open

aborovsky wants to merge 4 commits into
nextfrom
fix/repeater-timeout-units

Conversation

@aborovsky

@aborovsky aborovsky commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

No description provided.

The repeater redeclares --timeout with a 30000 ms default, but yargs kept
applying the global seconds-based coercion to it, so the value became
30000000 ms (about 8h20m). It is used both as the Bright socket connect
timeout and as the target request timeout, so a reconnection attempt that
never got an upgrade response could hang for hours without a retry.

Give the repeater option its own millisecond-based coercion, sharing the
duration parsing with the global option through Helpers.parseDuration.
This also makes duration strings such as "1m" work for the repeater, which
previously parsed to NaN.
The warning is emitted when a reconnection attempt starts, not after it
fails, so "Failed to connect to Bright cloud" was misleading.
@aborovsky aborovsky self-assigned this Oct 23, 2026
@aborovsky aborovsky added the Type: bug Something isn't working. label Oct 23, 2026
@aborovsky

Copy link
Copy Markdown
Contributor Author

What

  • bright-cli repeater declares its own --timeout with a default of 30000 (ms). yargs also applied the global seconds-based --timeout coercion from CliBuilder to that default, so it became 30000000 ms (about 8h20m). Explicit values were not multiplied, only the default.
  • The value is used as the Bright socket connect timeout (DefaultRepeaterServer) and as the target request timeout (RequestExecutorOptions).
  • The reconnect warning Failed to connect to Bright cloud (attempt N) is logged when a reconnection attempt starts, not when it fails.

Why

A repeater lost its connection when the bridges-api pods restarted. socket.io started one reconnection attempt, and the WebSocket upgrade never got a response. With an 8h20m connect timeout, that attempt neither failed nor got retried for hours. The process stayed up, disconnected, with Failed to connect to Bright cloud (attempt 1) as its last log line.

How

  • New Helpers.parseDuration(value, numericUnit): duration strings (10s, 1m) go through ms, and a plain number is read in the given unit.
  • The global --timeout keeps seconds semantics: parseDuration(arg, 's').
  • The repeater --timeout gets its own coercion: parseDuration(arg, 'ms'). Redeclaring an option's coerce replaces the global one in yargs, so the default stays at 30000 ms. Explicit values behave as before. Duration strings such as --timeout 1m now work; they used to become NaN because of number: true.
  • The reconnect log now reads Reconnecting to Bright cloud (attempt N).

Tests

  • Helpers.parseDuration: plain numbers, numeric strings and duration strings, for both units.
  • RunRepeater.builder: runs through the real CliBuilder and checks that args.timeout, the socket connect timeout and the request timeout resolve to 30000 by default, 45000 for --timeout 45000, and 60000 for --timeout 1m. The test fails on the current code (30000000).
  • ServerRepeaterLauncher.run: checks the new reconnect message.

With the built CLI against a local unreachable host, socket.io debug logging shows connect attempt will timeout after 30000 by default and 60000 with --timeout 1m.

Not in this PR

Each reconnect attempt is logged twice. socket.io's Manager emits reconnect_attempt, and DefaultRepeaterServer.scheduleReconnection also emits RECONNECT_ATTEMPT on connect_error with its own counter, so two reconnect mechanisms run side by side. This is left for a separate change.

Comment thread src/Utils/Helpers.ts Outdated
@aborovsky
aborovsky marked this pull request as ready for review October 23, 2026 17:00

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: bug Something isn't working.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant