Repository navigation
🤖 fix: ship the analytics worker in the Docker image and smoke-test it on image PRs - #5627
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cf008df47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Readiness record:
Generated with |
Summary
The Docker server image now ships
dist/runtime/analyticsWorker.jsand the DuckDB native bindings it needs, so the analytics worker starts in Docker.Smoke / Dockernow also runs on PRs that touch server image inputs, and it starts the shipped analytics worker instead of checking only/health.Fixes #5603
Fixes #5605
Background
AnalyticsServicestartsanalyticsWorker.jsfrom the directory of the running server bundle.make build-docker-runtimenever built that file, so every analytics request in the Docker image failed withCannot find module '/app/dist/runtime/analyticsWorker.js'./healthdoes not start the worker (it starts lazily), andSmoke / Dockerran only in the merge queue, so nothing caught this.The two issues share one PR on purpose: the new smoke step is the regression test for #5603, and this PR touches the
Dockerfile, so its own CI run shows the new trigger.Implementation
Makefile: a newdist/runtime/analyticsWorker.jstarget bundles the worker with esbuild.@duckdb/*stays external because esbuild cannot bundle the native.nodebinding.verify-docker-runtime-artifactsnow checks the file.Dockerfile: the runtime stage copiesnode_modules/@duckdb. The builder stage first deletes the musl bindings, because the runtime image is glibc (node:22-slim).detect-libc, which the bindings use, is already in the image for sharp.scripts/check-analytics-worker.cjs: starts a built worker, runs itsinittask (opens a DuckDB database through the native binding), then shuts it down. It exits nonzero on any worker error..github/workflows/pr.yml:dockerpath filter inchangeslists everything the Dockerfile copies into the build stage (src/**,docs/**, package, lock and patch files, the Makefile, the copied scripts, the Vite entry files,public/**,static/**), plusDockerfile,.dockerignoreandpr.ymlitself. A Dockerfile comment asks to keep the two lists in sync.Smoke / Dockerruns onpull_requestwhen that filter matches. Merge queue and push behavior is unchanged.docker exec -i mux-test node - /app/dist/runtime/analyticsWorker.js < scripts/check-analytics-worker.cjs.Decision (conservative option): ship the worker instead of disabling analytics in Docker. The image grows by about 75 MB (436 MB to 511 MB locally) for
libduckdb.so.Validation
I built the image from
origin/main(before) and from this branch (after) withdocker build, ran each container, started the worker with the check script, and calledPOST /api/analytics/getSummarywith the container's auth token.Before (origin/main)
Server log after
POST /api/analytics/getSummary(response:INTERNAL_SERVER_ERROR):After (this branch)
Smoke / Docker trigger:
Dockerfile, so the job ran on its first head (pass, and the log showsanalytics worker OK): https://github.com/coder/xum/actions/runs/37226228085/job/111506356866src/node/**, before this PR widened the filter tosrc/**), so the job was skipped there (skipping): https://github.com/coder/xum/actions/runs/37226538034/job/111507277313Risks
Low. The change adds files to the image and a CI job to most code PRs.
Smoke / Dockertook about 4 minutes on this PR, in parallel with the other jobs. A new Dockerfile COPY input that is missing from the filter reaches the merge queue unchecked, as before.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high