Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 407
fix(runtime-host): let operators raise the election deadline and clarify timeout copy#3480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import test from 'node:test'; | ||
| import { | ||
| ELECTION_DEADLINE_MS_ENV_VAR, | ||
| connectOrSpawnRuntimeHostWithDependencies, | ||
| electionDeadlineMsFromEnvironment, | ||
| } from '../client/connect-or-spawn.js'; | ||
| import { | ||
| INTERACTIVE_RUNTIME_HOST_COMPOSITION_ID, | ||
| RUNTIME_HOST_PROTOCOL_VERSION, | ||
| } from '../protocol/index.js'; | ||
| test('treats an unset or blank override as unconfigured', () => { | ||
| assert.equal(electionDeadlineMsFromEnvironment(undefined), undefined); | ||
| assert.equal(electionDeadlineMsFromEnvironment(''), undefined); | ||
| assert.equal(electionDeadlineMsFromEnvironment(' '), undefined); | ||
| }); | ||
| test('parses a valid millisecond override', () => { | ||
| assert.equal(electionDeadlineMsFromEnvironment('90000'), 90_000); | ||
| assert.equal(electionDeadlineMsFromEnvironment(' 5000 '), 5_000); | ||
| }); | ||
| test('fails closed on an invalid override instead of silently ignoring it', () => { | ||
| for (const invalid of ['abc', '0', '-100', '120001', '1.5']) { | ||
| assert.throws(() => electionDeadlineMsFromEnvironment(invalid), RangeError); | ||
| } | ||
| assert.throws( | ||
| () => electionDeadlineMsFromEnvironment('abc'), | ||
| new RegExp(`${ELECTION_DEADLINE_MS_ENV_VAR} must be an integer`, 'u'), | ||
| ); | ||
| }); | ||
| test('an invalid environment override fails the election before touching storage', async () => { | ||
| await assert.rejects( | ||
| connectOrSpawnRuntimeHostWithDependencies( | ||
| { | ||
| rootPath: '/nonexistent-maka-3474-root', | ||
| protocol: { min: RUNTIME_HOST_PROTOCOL_VERSION, max: RUNTIME_HOST_PROTOCOL_VERSION }, | ||
| compositionId: INTERACTIVE_RUNTIME_HOST_COMPOSITION_ID, | ||
| candidateEntrypoint: 'candidate-entry.js', | ||
| }, | ||
| { | ||
| launchCandidate: () => ({ spawned: Promise.reject(new Error('must not spawn')) }), | ||
| random: Math.random, | ||
| env: { [ELECTION_DEADLINE_MS_ENV_VAR]: 'not-a-number' }, | ||
| }, | ||
| ), | ||
| (error: unknown) => | ||
| error instanceof RangeError && /MAKA_RUNTIME_HOST_ELECTION_DEADLINE_MS/u.test(error.message), | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -38,6 +38,7 @@ const DEFAULT_ELECTION_DEADLINE_MS = 45_000; | ||
| const DEFAULT_BACKOFF_MIN_MS = 20; | ||
| const DEFAULT_BACKOFF_MAX_MS = 250; | ||
| const MIN_CANDIDATE_INTERVAL_MS = 250; | ||
| export const ELECTION_DEADLINE_MS_ENV_VAR = 'MAKA_RUNTIME_HOST_ELECTION_DEADLINE_MS'; | ||
| export interface ConnectOrSpawnRuntimeHostInput { | ||
| rootPath: string; | ||
| @@ -56,13 +57,35 @@ export interface ConnectOrSpawnRuntimeHostInput { | ||
| interface ConnectOrSpawnRuntimeHostDependencies { | ||
| launchCandidate: CandidateLauncher; | ||
| random(): number; | ||
| /** Defaults to `process.env`; injected so tests never mutate the real environment. */ | ||
| env?: NodeJS.ProcessEnv; | ||
| } | ||
| const defaultDependencies: ConnectOrSpawnRuntimeHostDependencies = { | ||
| launchCandidate: launchDetachedRuntimeHostCandidate, | ||
| random: Math.random, | ||
| }; | ||
| /** | ||
| * Resolves the operator override for the client election deadline. Large | ||
| * workspaces can legitimately take longer than the default window on their | ||
| * first start after an upgrade, so the deadline must be raisable without a | ||
| * code change. Invalid values fail closed: a silently ignored typo would leave | ||
| * the operator believing they widened the window when they did not. | ||
| */ | ||
| export function electionDeadlineMsFromEnvironment( | ||
| rawValue: string | undefined, | ||
| ): number | undefined { | ||
| if (rawValue === undefined || rawValue.trim() === '') return undefined; | ||
| const parsed = Number(rawValue); | ||
| if (!Number.isSafeInteger(parsed) || parsed <= 0 || parsed > 120_000) { | ||
| throw new RangeError( | ||
| `${ELECTION_DEADLINE_MS_ENV_VAR} must be an integer between 1 and 120000 milliseconds`, | ||
| ); | ||
| } | ||
| return parsed; | ||
| } | ||
| export type ConnectOrSpawnRuntimeHostResult = | ||
| | { | ||
| kind: 'connected'; | ||
| @@ -173,7 +196,12 @@ export async function connectOrSpawnRuntimeHostWithDependencies( | ||
| input: ConnectOrSpawnRuntimeHostInput, | ||
| dependencies: ConnectOrSpawnRuntimeHostDependencies, | ||
| ): Promise<ConnectOrSpawnRuntimeHostResult> { | ||
| const deadlineMs = input.electionDeadlineMs ?? DEFAULT_ELECTION_DEADLINE_MS; | ||
| const deadlineMs = | ||
| input.electionDeadlineMs ?? | ||
| electionDeadlineMsFromEnvironment( | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P3] On the owned-launch path an invalid value loses the variable name and surfaces as
This is the same shape as the finding above: the mechanism is fine, the diagnosis points the wrong way. Config errors specifically are worth letting through that catch (or resolving before it, as a typed configuration failure), since they're the one class the user can fix immediately once they know the variable name. Worth a test on both the owned and hosted paths. | ||
| (dependencies.env ?? process.env)[ELECTION_DEADLINE_MS_ENV_VAR], | ||
| ) ?? | ||
| DEFAULT_ELECTION_DEADLINE_MS; | ||
| if (!Number.isSafeInteger(deadlineMs) || deadlineMs <= 0 || deadlineMs > 120_000) { | ||
| throw new RangeError('electionDeadlineMs must be an integer between 1 and 120000'); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] The error text points every startup failure at this variable, including the class it can't fix.
There's a second failure shape this knob doesn't address. From a real CI run on #3454, which now emits election diagnostics:
{"deadlineMs":45000,"elapsedMs":45174,"candidateLaunches":69, "observations":{"notRegistered":73,"connectFailed":0,"handshakeFailed":0, "connected":1,"readyWaitFailed":1}}69 candidate launches in 45 seconds. Zero connect failures, zero handshake failures — it connected once, then ready-wait failed, and candidates kept being launched roughly every 0.65s until the deadline. It missed by 174ms, so raising the deadline would have made that particular run pass, but the run isn't slow — it isn't converging. At 90s it launches ~138 candidates instead of ~69 and passing becomes a matter of luck.
That's different from #3474's genuinely-slow start, where waiting longer is exactly right. The problem is that both produce the same message, so an operator hitting the retry-storm shape is told to raise the timeout, raises it, fails again, and raises it further.
Not blocking, and possibly not yours to fix here — but worth asking: with
candidateLauncheshigh andconnectFailed/handshakeFailedat zero, should the message distinguish "this start is slow, wait longer" from "candidates aren't converging, the timeout isn't your problem"? You have the #3474 case in hand and probably know which shape operators hit more often.