fix(docker): restore meriyah's ESM entry and smoke-test the standalone server - #3167
Conversation
|
@MrRussLuther is attempting to deploy a commit to the Inbox Zero OSS Program Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker/scripts/smoke-standalone.sh">
<violation number="1" location="docker/scripts/smoke-standalone.sh:42">
P3: The build-time server log is left in the final image because the temporary file created here is never removed. Since this script runs in a Docker `RUN` layer, every built image retains the smoke-test log under `/tmp`, including potentially sensitive startup diagnostics and unnecessary build artifacts. A cleanup trap would remove it on both successful and failed checks.</violation>
<violation number="2" location="docker/scripts/smoke-standalone.sh:55">
P2: The smoke test can report success even when the server dies before it serves `/login`. This request's failure is explicitly discarded, and the later checks only require that `Ready in` was previously logged and that two specific error phrases are absent; the child process is also terminated without waiting for or checking its exit status. A request-time crash or an unrecognized startup/runtime failure can therefore leave a `Ready in` line behind and still produce a passing image build. The test would be more reliable if it required a successful connection/response and validated the spawned server's exit status after shutdown, while still allowing the expected HTTP 500 response.</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| done | ||
|
|
||
| # The hook is evaluated on the request path, so issue one request before judging. | ||
| wget -q -O /dev/null -T 5 "http://127.0.0.1:${PORT}/login" 2>/dev/null || true |
There was a problem hiding this comment.
P2: The smoke test can report success even when the server dies before it serves /login. This request's failure is explicitly discarded, and the later checks only require that Ready in was previously logged and that two specific error phrases are absent; the child process is also terminated without waiting for or checking its exit status. A request-time crash or an unrecognized startup/runtime failure can therefore leave a Ready in line behind and still produce a passing image build. The test would be more reliable if it required a successful connection/response and validated the spawned server's exit status after shutdown, while still allowing the expected HTTP 500 response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docker/scripts/smoke-standalone.sh, line 55:
<comment>The smoke test can report success even when the server dies before it serves `/login`. This request's failure is explicitly discarded, and the later checks only require that `Ready in` was previously logged and that two specific error phrases are absent; the child process is also terminated without waiting for or checking its exit status. A request-time crash or an unrecognized startup/runtime failure can therefore leave a `Ready in` line behind and still produce a passing image build. The test would be more reliable if it required a successful connection/response and validated the spawned server's exit status after shutdown, while still allowing the expected HTTP 500 response.</comment>
<file context>
@@ -0,0 +1,72 @@
+done
+
+# The hook is evaluated on the request path, so issue one request before judging.
+wget -q -O /dev/null -T 5 "http://127.0.0.1:${PORT}/login" 2>/dev/null || true
+sleep 2
+kill "$PID" 2>/dev/null || true
</file context>
| export GOOGLE_PUBSUB_TOPIC_NAME=d | ||
| export NEXT_PUBLIC_BASE_URL="http://127.0.0.1:${PORT}" | ||
|
|
||
| LOG="$(mktemp)" |
There was a problem hiding this comment.
P3: The build-time server log is left in the final image because the temporary file created here is never removed. Since this script runs in a Docker RUN layer, every built image retains the smoke-test log under /tmp, including potentially sensitive startup diagnostics and unnecessary build artifacts. A cleanup trap would remove it on both successful and failed checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docker/scripts/smoke-standalone.sh, line 42:
<comment>The build-time server log is left in the final image because the temporary file created here is never removed. Since this script runs in a Docker `RUN` layer, every built image retains the smoke-test log under `/tmp`, including potentially sensitive startup diagnostics and unnecessary build artifacts. A cleanup trap would remove it on both successful and failed checks.</comment>
<file context>
@@ -0,0 +1,72 @@
+export GOOGLE_PUBSUB_TOPIC_NAME=d
+export NEXT_PUBLIC_BASE_URL="http://127.0.0.1:${PORT}"
+
+LOG="$(mktemp)"
+node /app/apps/web/server.js >"$LOG" 2>&1 &
+PID=$!
</file context>
…e server Turbopack's tracing resolves meriyah through the `require` export condition, so the standalone output ships dist/meriyah.cjs alone. At runtime @sentry/nextjs is external and therefore loads from that traced output, reaches meriyah as ESM through @apm-js-collab/code-transformer, and cannot find dist/meriyah.mjs. The server starts, applies migrations, logs Ready, and then 500s every request, so container state and healthchecks all look fine while the app is dead. outputFileTracingIncludes cannot fix this. Turbopack builds skip collect-build-traces, which is what implements that option, so setting it changes nothing and warns about nothing. Copy the ESM entries into the standalone tree after the build, and add a build-time smoke test that boots the server and fails if the instrumentation hook does not load. The copy is narrow on purpose: tracing omits files by design and a healthy image already has around 550 export targets that do not exist, so a blanket repair would defeat the point. The smoke test is what catches the next occurrence. The existing check only proved that next itself resolves. Resolving @sentry/nextjs by name would not have caught this either, because pnpm deploy leaves a complete copy in apps/web/node_modules that resolves fine while the traced copy the server actually uses is incomplete.
f73d35d to
26d01b9
Compare
Add changelog entries for: - 2026-08-07: LLM-guided onboarding flow (#3170) - 2026-08-02: Inbox Health reports (#3129, pending from prior run) Skipped: #3176, #3173 (docs), #3172 (evals), #3167 (docker), #3174 (onboarding input polish), #3166 (notetaker owner label) Co-authored-by: Elie Steinbock <elie222@users.noreply.github.com>
Fixes #3150. Since #3149, self-hosted images start, apply migrations, log
✓ Ready, and then 500 every request.Cause
meriyah publishes its ESM entry only through the
defaultandmodule-syncexport conditions. Tracing resolves it throughrequire, so the standalone output getsdist/meriyah.cjsalone. At runtime@sentry/nextjsis inserverExternalPackages, so it loads from that traced output, reaches meriyah as ESM via@apm-js-collab/code-transformer, and cannot finddist/meriyah.mjs.The truncated copy is not new.
@sentry/nextjs10.69.0 is what began importing it as ESM, which is why the gap only became reachable in #3149.Why not
outputFileTracingIncludesIt is inert here. Since #2981 these are Turbopack builds, and Turbopack skips trace collection entirely:
collect-build-tracesis what implementsoutputFileTracingIncludes, so setting it changes nothing and warns about nothing. I tried it first and confirmed it has no effect on the built image.Changes
docker/Dockerfile.prod: after the build, copy meriyah's ESM entries into the standalone tree. Deliberately narrow, because tracing omits files by design and a healthy image already has ~550 export targets that legitimately do not exist. A blanket repair would undo the point of tracing.docker/Dockerfile.prodanddocker/scripts/smoke-standalone.sh: boot the built server during the image build and fail if the instrumentation hook does not load. This is the durable half. The targeted copy is whack-a-mole on its own; the smoke test means the next occurrence fails the build instead of shipping.Why the smoke test boots the server
The existing check proves
nextresolves and stops there. Two cheaper checks look like they would cover this and do not:exportstarget exists. ~550 false positives on a healthy image.import("@sentry/nextjs")by name. That resolves to the completepnpm deploy --prodcopy inapps/web/node_modules, which is intact even when the traced copy the server uses is not. I verified this passes on the broken image, so it would have shipped this bug.Only running the built server exercises the path the runtime actually resolves. I checked whether a static
scan could work instead: the chunks contain just five literal absolute paths, all unrelated route entries,
so the store path is constructed at load time and cannot be asserted without booting.
Verification
Against the published broken image
sha256:b59cfafc…:Cannot find module .../meriyah/dist/meriyah.mjsmeriyah.mjsmakes it pass, and/loginreturns 200 instead of 500On this branch, built end to end with
docker/Dockerfile.prod:.next/standalone/.pnpm-store/v11/links/@/meriyah/6.1.4/2e61a82c…/node_modules/meriyah/dist/, which is the path the chunks reference/loginreturns 200 with zeroCannot find modulelines in the logI also confirmed the smoke test fails the build on this tree without the copy, so it is testing what it claims to.
Note
The underlying mismatch, tracing resolving one export condition while the runtime uses another, looks like it belongs in Turbopack. This PR is the workaround plus a guard so it cannot ship silently again.