-
-
Notifications
You must be signed in to change notification settings - Fork 679
fix(collab): ship the relay image's runtime dependencies #1867
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
Merged
+213
−3
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| #!/usr/bin/env node | ||
| // Smoke-test a *running* GeoLibre collaboration relay: health, session | ||
| // creation, and a WebSocket join round-trip. | ||
| // | ||
| // The relay's worst failure mode is an image that builds clean and then exits at | ||
| // startup because the runtime stage is missing a dependency the bundle imports | ||
| // (GeoLibre#1866). No unit test or type-check sees that, and neither does | ||
| // `docker build` -- only starting the container does. So CI builds the image, | ||
| // runs it, and points this script at it. It talks plain HTTP plus the global | ||
| // WebSocket, so it needs nothing installed beyond Node itself and can be aimed | ||
| // at any deployed relay: | ||
| // | ||
| // node workers/collab-node/scripts/smoke.mjs http://127.0.0.1:8787 | ||
|
|
||
| const baseUrl = (process.argv[2] ?? "http://127.0.0.1:8787").replace(/\/+$/, ""); | ||
| // Generous: on a cold CI runner the container has to start Node and open the | ||
| // SQLite database before it listens. | ||
| const STARTUP_TIMEOUT_MS = 60_000; | ||
| const WS_TIMEOUT_MS = 15_000; | ||
| const PROBE_TIMEOUT_MS = 5_000; | ||
|
|
||
| function fail(message, detail) { | ||
| console.error(`FAIL: ${message}`); | ||
| if (detail !== undefined) console.error(detail); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
|
|
||
| async function waitForHealth() { | ||
| const deadline = Date.now() + STARTUP_TIMEOUT_MS; | ||
| let lastError = "no response"; | ||
| while (Date.now() < deadline) { | ||
| try { | ||
| // Bounded, because a container that accepts the connection and then never | ||
| // answers falls back on undici's own header/body timeouts, which are | ||
| // minutes long: the job would stall well past the startup budget instead | ||
| // of failing at it. Capped at whatever is left of that budget so a probe | ||
| // started near the deadline cannot overrun it either. | ||
| const signal = AbortSignal.timeout(Math.min(PROBE_TIMEOUT_MS, deadline - Date.now())); | ||
| const response = await fetch(`${baseUrl}/health`, { signal }); | ||
| // The same signal still covers this: aborting after the headers arrive | ||
| // errors the body stream rather than leaving the read hanging. | ||
| const body = await response.json(); | ||
| if (response.ok && body?.ok) return body; | ||
| lastError = `HTTP ${response.status} ${JSON.stringify(body)}`; | ||
| } catch (error) { | ||
| lastError = error instanceof Error ? error.message : String(error); | ||
| } | ||
| await sleep(500); | ||
| } | ||
| fail(`the relay never answered GET /health at ${baseUrl}`, lastError); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| async function createSession() { | ||
| const response = await fetch(`${baseUrl}/sessions`, { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: "{}", | ||
| // Bounded for the same reason as the health probe: the relay has answered | ||
| // by now, so anything slower than this is a hang, not a slow start. | ||
| signal: AbortSignal.timeout(PROBE_TIMEOUT_MS), | ||
| }); | ||
| const body = await response.json().catch(() => null); | ||
| if (!response.ok || !body?.sessionId || !body?.hostToken) | ||
| fail( | ||
| "POST /sessions did not return a session", | ||
| `HTTP ${response.status} ${JSON.stringify(body)}`, | ||
| ); | ||
| return body; | ||
| } | ||
|
|
||
| // The join round-trip is the part that actually exercises `ws`: the relay only | ||
| // reaches WebSocketServer.handleUpgrade here, so a missing or broken copy of it | ||
| // shows up as a failed upgrade rather than a passing health check. | ||
| function join(session) { | ||
| const wsUrl = `${baseUrl.replace(/^http/, "ws")}/sessions/${session.sessionId}/ws`; | ||
| return new Promise((resolve) => { | ||
| const socket = new WebSocket(wsUrl); | ||
| const timer = setTimeout(() => { | ||
| socket.close(); | ||
| fail(`no welcome frame within ${WS_TIMEOUT_MS}ms of joining ${wsUrl}`); | ||
| }, WS_TIMEOUT_MS); | ||
|
|
||
| socket.addEventListener("open", () => { | ||
| socket.send( | ||
| JSON.stringify({ | ||
| type: "join", | ||
| clientId: "smoke-test", | ||
| displayName: "Smoke test", | ||
| color: "#2563eb", | ||
| hostToken: session.hostToken, | ||
| }), | ||
| ); | ||
| }); | ||
| socket.addEventListener("message", (event) => { | ||
| clearTimeout(timer); | ||
| let message; | ||
| try { | ||
| message = JSON.parse(String(event.data)); | ||
| } catch { | ||
| fail("the relay sent a frame that is not JSON", String(event.data).slice(0, 200)); | ||
| } | ||
| if (message.type !== "welcome") | ||
| fail( | ||
| `expected a welcome frame, got "${message.type}"`, | ||
| JSON.stringify(message).slice(0, 200), | ||
| ); | ||
| if (message.role !== "host") | ||
| fail(`the host token did not claim the host role (got "${message.role}")`); | ||
| socket.close(); | ||
| resolve(message); | ||
| }); | ||
| socket.addEventListener("error", () => { | ||
| clearTimeout(timer); | ||
| fail(`the WebSocket upgrade to ${wsUrl} failed`); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| const health = await waitForHealth(); | ||
| console.log(`ok GET /health -> ${JSON.stringify(health)}`); | ||
| const session = await createSession(); | ||
| console.log(`ok POST /sessions -> ${session.sessionId} (${session.mode})`); | ||
| const welcome = await join(session); | ||
| console.log( | ||
| `ok WebSocket join -> welcome as ${welcome.role}, ${welcome.participants.length} participant(s)`, | ||
| ); | ||
| console.log(`PASS: the relay at ${baseUrl} is serving`); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Minor performance nit: this reinstalls with a fresh
npm ci --omit=dev, which re-resolves and re-fetches the whole tree a second time (on top of the dev-inclusivenpm ciat line 6). Since the lockfile and workspace selection are identical,npm prune --omit=devafter the build would remove the devDependencies-only packages from the already-installed tree without a second network round-trip, and should produce the same pruned layout. Not a correctness issue — the current approach is just slower to build than necessary. (Low confidence this is worth the churn vs. the simplicity of a second cleannpm ci.)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.
Measured this rather than guessing, and it does not hold up — leaving the second
npm ci --omit=devin place.The literal suggestion, unscoped
npm prune --omit=dev, is a no-op here: node_modules stays at 41M withesbuildand@esbuildstill installed, so it would ship the build toolchain into the runtime image. It needs the same workspace filters to do anything:npm prune --omit=dev --workspace geolibre-collab-node --workspace @geolibre/collab-core.With those filters it works, but it is not faster. Both take 1s on an already-populated tree:
npm ci --omit=dev --workspace …(current)npm prune --omit=dev --workspace …@esbuild/,@types/,@typescript/,.bin/shellsThere is no second network round-trip to save: the first
npm cihas already populated the npm cache in that layer, so the reinstall is cache-served. The wholeCollab relay imageCI job, docker build and container smoke test included, runs in 21s.So it is the same speed, and
npm cileaves a cleaner tree to copy and cannot silently degrade to a no-op if the workspace filters drift. Leaving the thread open in case you disagree.