fix(collab): ship the relay image's runtime dependencies - #1867
Conversation
The v2.5.0 collab-node image exited at startup with ERR_MODULE_NOT_FOUND for `ws`. The runtime stage copied only /app/node_modules, but that is not where npm puts this worker's `ws`: the root lockfile carries two versions (8.21.0, hoisted for geolibre-desktop's transitive deps, and ^8.21.3 for the relay), so `npm ci --workspace geolibre-collab-node` installs the relay's copy at the nested workers/collab-node/node_modules, which nothing copied. The same stage left node_modules/@geolibre/collab-core dangling, since packages/collab-core was never copied either. Assemble the runtime tree under /runtime in the build stage instead, and copy that: the nested node_modules when it exists (it disappears the moment a dependency bump lets npm hoist `ws` to the root, so the copy is guarded), packages/collab-core so the workspace symlink resolves, and a prod-only reinstall so esbuild and the rest of the dev tree stay behind. The staged bundle is then imported once at build time -- side-effect-free, since server.js only listens when it is process.argv[1] -- so a future layout regression fails the build rather than the container. publish-container.yml only ever built the web Dockerfile, which is how this shipped. Add a CI job that builds the relay image, runs it, and drives scripts/smoke.mjs against it: health, session creation, and a WebSocket join round-trip. A build alone would not have caught this, only starting the container does. Fixes #1866
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe collaboration relay Docker image now stages production dependencies and workspace packages under ChangesCollaboration relay runtime
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant CollabImage as collab-node container
participant SmokeScript as smoke.mjs
GitHubActions->>CollabImage: Build and start image
GitHubActions->>SmokeScript: Run smoke test
SmokeScript->>CollabImage: Poll GET /health
CollabImage-->>SmokeScript: Return body.ok
SmokeScript->>CollabImage: POST /sessions
CollabImage-->>SmokeScript: Return sessionId and hostToken
SmokeScript->>CollabImage: Join WebSocket with host credentials
CollabImage-->>SmokeScript: Return welcome frame with host role
GitHubActions->>CollabImage: Print container logs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # so the workspace symlink under node_modules resolves rather than dangling. The | ||
| # reinstall drops esbuild and the other dev dependencies now that the bundle is | ||
| # built, which is also what keeps the copied tree small. | ||
| RUN npm ci --omit=dev --workspace geolibre-collab-node --workspace @geolibre/collab-core \ |
There was a problem hiding this comment.
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-inclusive npm ci at line 6). Since the lockfile and workspace selection are identical, npm prune --omit=dev after 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 clean npm ci.)
There was a problem hiding this comment.
Measured this rather than guessing, and it does not hold up — leaving the second npm ci --omit=dev in place.
The literal suggestion, unscoped npm prune --omit=dev, is a no-op here: node_modules stays at 41M with esbuild and @esbuild still 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:
| time | resulting root node_modules | |
|---|---|---|
npm ci --omit=dev --workspace … (current) |
1s | 4.0K, clean |
npm prune --omit=dev --workspace … |
1s | 4.0K, plus empty @esbuild/, @types/, @typescript/, .bin/ shells |
There is no second network round-trip to save: the first npm ci has already populated the npm cache in that layer, so the reinstall is cache-served. The whole Collab relay image CI job, docker build and container smoke test included, runs in 21s.
So it is the same speed, and npm ci leaves 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.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workers/collab-node/README.md`:
- Around line 36-38: Update the build-validation description in the README to
state that the Dockerfile’s staged server.js import catches missing ws
dependencies and broken `@geolibre/collab-core` targets during docker build, while
the smoke test validates actual container startup and HTTP/WebSocket behavior.
In `@workers/collab-node/scripts/smoke.mjs`:
- Around line 29-44: Update waitForHealth so each fetch/response.json probe uses
an AbortSignal timeout calculated from the remaining time until deadline, capped
to that remaining startup duration rather than a fixed timeout. Preserve the
existing retry, error-capture, and final fail behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9baa176b-cf55-4e58-a93d-db08cfbc8be7
📒 Files selected for processing (4)
.github/workflows/ci.ymlworkers/collab-node/Dockerfileworkers/collab-node/README.mdworkers/collab-node/scripts/smoke.mjs
🔍 GitHub Pages PR preview
|
- Correct the Dockerfile's framing of packages/collab-core: esbuild inlines @geolibre/collab-core, so the running server never imports it and it was not what broke the image. It is copied only so the symlink npm leaves in node_modules points at something, and the build-time `test` on it asserts the tree was copied whole rather than guarding startup. - Fix the README's stale claim that the failure is invisible to `docker build`: the Dockerfile now imports the staged bundle there, so the smoke test covers what only a running container shows (the process stays up and serves HTTP and a WebSocket upgrade). - Bound the smoke test's health probes with AbortSignal.timeout, capped by the remaining startup budget. A container that accepts the connection and never answers otherwise falls back on undici's minutes-long timeouts and stalls the job past the budget instead of failing at it. Verified against a black-hole listener: fails at the budget rather than hanging. POST /sessions is bounded the same way.
Code reviewI reviewed the Dockerfile restructuring, the new Verification performed:
Bugs: None found. Security: None found — the smoke script's Performance: Minor, not worth blocking — the runtime-assembly stage does a second full Quality: The Dockerfile/CI/script comments are unusually thorough and accurately describe the actual behavior verified above. No naming or readability concerns. CLAUDE.md: No violations — this doesn't touch any of the mirrored-constant or catalog-generation conventions called out there, and it's a bug fix + regression-test addition, consistent with repo conventions (branch + PR, no direct main commits implied by the PR flow). No inline comments posted — I didn't find defects meeting the bar for a specific line-level finding. |
Fixes #1866.
What was wrong
The v2.5.0 collab-node image builds clean and then exits at startup with
ERR_MODULE_NOT_FOUND: Cannot find package 'ws'.The runtime stage copied only
/app/node_modules, but that is not where npm putsthis worker's
ws. The root lockfile carries two versions of it:so
npm ci --workspace geolibre-collab-node --workspace @geolibre/collab-coreinstalls the relay's copy at the nested
workers/collab-node/node_modules/ws,which nothing copied into the runtime stage. And because the selected workspaces
exclude whatever pulls 8.21.0, the root
node_modules/wsis not there either, sonothing resolves. The same stage left
node_modules/@geolibre/collab-coredangling, since
packages/collab-core(its symlink target) was never copied.Nothing in CI caught it:
publish-container.ymlonly builds the webDockerfile, so the relay image was never built outside a user'sdocker compose up.The fix
Assemble the runtime tree under
/runtimein the build stage and copy that:workers/collab-node/node_moduleswhen it exists, guarded withif [ -d ... ]because it disappears the moment a dependency bump lets npmhoist
wsto the root. The image is then correct under either layout ratherthan depending on a hoisting coincidence.
packages/collab-core, so the workspace symlink resolves instead of dangling.behind. The copied tree drops from the full dev
node_modulesto 276 KB.Then import the staged bundle once at build time. That is side-effect-free
(
server.jsonly listens when it isprocess.argv[1]), uses the real ESMresolver from the path the file will actually sit at, and makes a future layout
regression fail the build instead of the container.
Guard against a repeat
Added a
collab-imageCI job that builds the relay image, runs it, and drives anew
workers/collab-node/scripts/smoke.mjsagainst it:GET /health,POST /sessions, and a WebSocket join round-trip. Adocker buildalone wouldnot have caught this; only starting the container does. The script needs nothing
beyond Node (it uses the global
WebSocket), so it also works against anydeployed relay, and the README documents it.
Verification
No container runtime was available here, so I replayed the Dockerfile's stages on
the filesystem against the real lockfile, which reproduces the report exactly:
node_modules/wsafter the build stage'snpm ciwsresolvable fromdist/server.jsin the runtime treenode_modules/@geolibre/collab-corenode workers/collab-node/dist/server.jsERR_MODULE_NOT_FOUNDAgainst the fixed tree:
GET /healthreturns{"ok":true,"service":"geolibre-collab"}, the image'sHEALTHCHECKcommandexits 0,
POST /sessionsallocates a session, and a real WebSocket client joinsand gets
{"type":"welcome",...,"role":"host"}back. I also simulated a futurelockfile where
wshoists to the root and the nested directory is gone: theguard skips cleanly and the relay still serves.
npm run typecheck -w geolibre-collab-nodeandnpm run test -w geolibre-collab-node(7/7) pass; pre-commit is clean on the changed files.Summary by CodeRabbit
Bug Fixes
Documentation
Tests