fix(container): handle SIGTERM - node is PID 1 and was ignoring it (#120) - #154
Merged
Conversation
) #120 measured the container staying awake ~8.5 minutes against a configured `sleepAfter = "20s"`, and reasoned the likely cause was scheduling granularity in @cloudflare/containers' alarm loop, flagged UNVERIFIED. Tracing that loop instead of inferring it says otherwise, and the root cause is in this repo. The alarm clamp is not it. `minTime` starts at `Date.now() + 3 * 60 * 1000` and `sleepAfterMs` is also an absolute timestamp, so `Math.min(minTimeFromSchedules, minTime, this.sleepAfterMs)` picks the EARLIER of them - a 20s sleepAfter wins over the 3m ceiling. On expiry the loop calls onActivityExpired() then renewActivityTimeout(), i.e. it re-checks every 20s. There is no minutes-long floor there. What the trace does show, at the SDK's own `sleepAfter` declaration: // The signal sent to the container by default is a SIGTERM. // The container won't get a SIGKILL if this threshold is triggered. `onActivityExpired()` calls `stop()`, which sends SIGTERM and never escalates. And our Dockerfile is `ENTRYPOINT ["node", "/app/server.mjs"]` - exec form, no shell, no init - so node runs as **PID 1**, which Linux gives no default signal dispositions. `server.mjs` registered no handler, so the SIGTERM was silently DISCARDED. The SDK therefore never stopped the container at all and it lingered until the platform reclaimed it, which is the ~8.5 minute tail. Fix: handle SIGTERM (and SIGINT) and `server.close()` then exit(0). Closing rather than exiting outright lets an in-flight render finish streaming, which matters because the Worker must drain the body for its own inflight counter to decrement. Adds `test/shutdown.test.mjs`, which spawns the real server and sends a real SIGTERM. Its limits are stated in the file rather than overclaimed: the spawned child is NOT pid 1, so it keeps the default disposition and dies on SIGTERM even with no handler - deleting the handler makes the test fail on the EXIT-CODE assertion (`code: null, signal: SIGTERM` instead of a clean exit(0)) in ~34ms, not by timing out. Verified by planting that removal. The timeout arm covers the real pid-1 case, which no harness outside a container can reproduce. `PORT` becomes overridable (default still 8080) so the test can bind a free port. NOT yet confirmed in production. That needs a deploy and a re-read of the `containersUsageAdaptiveGroups` buckets; the issue's step 2 (setting sleepAfter to "10m" to test whether it is inert) is now unnecessary if this is right, and remains the fallback if the tail survives. Verified: pnpm run verify green - 1588 app + 13 worker + 4 container (up 1), 0 warnings, exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HKjzpWJw1HbnigPP9udqN2
wormeyman
added a commit
that referenced
this pull request
Aug 5, 2026
…ifact (#120) (#155) #120 published an "~8.5 minute" idle tail from `containersUsageAdaptiveGroups`, read ~14 minutes after the test render that produced it. Re-read now that the dataset has settled, the same window tells a different story: the placement that render woke (`a20877a2`, 2026-08-03T22:35Z) was still allocated 29.3 hours later - 352 of 352 five-minute buckets present, zero gaps, 98.1% of a 1 GiB instance's full allocation - against a total of 5 worker requests in that entire window. There is no tail. The container simply never sleeps, which is exactly what the SIGTERM diagnosis in #154 predicts: `@cloudflare/containers` sends SIGTERM and never escalates, node is PID 1 and ignored it, so the instance only ever goes away when a deploy replaces the placement. Two claims in CLAUDE.md were wrong as a result, and both were cost claims: - "That cost ~$28/month for weeks" implied the drain fix ended it. Billing says the instance ran at 100% of a 4 GiB day on EVERY full day from 2026-07-20 through 2026-08-03 (95.3 - 99.0 GiB-hours/day against the 96.0 a 4 GiB instance bills for 24h), including the five days after that fix deployed on 2026-07-29. The bill was still being paid; the 2026-08-03 downsize to `basic` cut it ~4x rather than ending it. The drain guard stays - the hazard is real - but it was never the load-bearing cause. - The disk-to-memory ratios "2.0" and "4.0" are dashboard units. Queried in bytes, which is what the GraphQL dataset returns, they read 1.86 and 3.73. Adds the SIGTERM cause and the backfill trap as their own bullets, so the next person reading absence-of-bucket as sleep has the rule in front of them. Measured via the `containersUsageAdaptiveGroups` and `workersInvocationsAdaptive` GraphQL datasets on account 3e467e5d..., 2026-08-05. NOT a behaviour change: the fix itself is 343df53, already on main and NOT yet deployed. Production still runs worker version 2e67470f and container app version 3 (image sha256:16205eda..., updated 2026-08-03T21:24:01Z), both of which predate the handler. Verified: pnpm run verify green, exit 0 - 1588 app tests passed / 3 skipped, 13 worker, 4 container (including "exits promptly on SIGTERM"), 0 warnings. Claude-Session: https://claude.ai/code/session_01QZvNS2H4cbaZk46ybA7hbj Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
wormeyman
added a commit
that referenced
this pull request
Aug 5, 2026
…) (#159) * fix(container): don't read PORT - the base image sets it to 34197 (#120) The preview service has been returning 502 on every render since the 2026-08-05 deploy. Root cause is in this repo, and it is one line. #154 made the listen port overridable so `test/shutdown.test.mjs` could bind a free port: const PORT = Number(process.env.PORT ?? 8080); But our base image is `factoriotools/factorio`, and its own image config sets `PORT=34197` - Factorio's UDP game port. Read straight out of the registry config blob for the pinned digest: ENV: PATH=... PORT=34197 RCON_PORT=27015 SAVES=/factorio/saves ... So in production that expression resolves to 34197, not 8080. The server binds the wrong port, Cloudflare's runtime waits `TIMEOUT_TO_GET_PORTS_MS` (20s) for something to answer on 8080, and gives up: Failed to start container: There has been an internal error connecting to the port Every render 502s after ~21s. Confirmed against production at 05:55, 05:56, 06:00, 06:02 and 06:05 UTC - five fresh unused seeds, five 502s. Fix: name the override `FMW_CONTAINER_PORT`. A project-prefixed variable cannot collide with whatever a base image happens to export. 8080 stays the default, matching the Dockerfile's EXPOSE and the Worker's `defaultPort` in `preview-service/worker/src/container.ts`. WHY THE ORIGINAL CHECK MISSED IT, because the lesson generalises: the local verification was run as `env -u PORT node server.mjs` - with PORT explicitly UNSET, which is the single condition under which an inherited-variable bug cannot appear. A test that clears the variable it is defending against proves nothing. `test/port.test.mjs` therefore SETS `PORT=34197` in both of its cases. The new guard is not vacuous, and that was established by planting the regression rather than by reading it: restoring `process.env.PORT` makes BOTH tests fail (pass 0, fail 2). The second case asserts the 8080 default without depending on 8080 being free - it accepts either "listening on 8080" or an EADDRINUSE naming 8080, since both prove the server targeted it. Container tests: 3/3 pass (render, shutdown, port). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QZvNS2H4cbaZk46ybA7hbj * style(container): oxfmt wrap in port.test.mjs The static CI job formats .mjs too. The hotfix worktree had no node_modules, so the formatter could not run before the first push. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#120 measured the container staying awake ~8.5 minutes against a configured
sleepAfter = "20s", and reasoned the likely cause was scheduling granularity in@cloudflare/containers' alarm loop - explicitly flagged UNVERIFIED, with a note that someone should trace it rather than infer it from a comment.Tracing it says otherwise. The root cause is in this repo.
The alarm clamp is not it
minTimestarts atDate.now() + 3 * 60 * 1000andsleepAfterMsis also an absolute timestamp, sopicks the earlier of them - a 20s
sleepAfterwins over the 3m ceiling. And on expiry the loop callsonActivityExpired()thenrenewActivityTimeout(), i.e. it re-checks every 20s. There is no minutes-long floor there.What the trace does show
At the SDK's own
sleepAfterdeclaration:onActivityExpired()callsstop(), which sends SIGTERM and never escalates.And our Dockerfile is
ENTRYPOINT ["node", "/app/server.mjs"]- exec form, no shell, no init - so node runs as PID 1. Linux gives PID 1 no default signal dispositions, andserver.mjsregistered no handler, so the SIGTERM was silently discarded. The SDK therefore never stopped the container at all; it lingered until the platform reclaimed it. That is the ~8.5 minute tail.The fix
Handle
SIGTERM(andSIGINT),server.close()thenexit(0). Closing rather than exiting outright lets an in-flight render finish streaming - which matters because the Worker must drain the response body for its own inflight counter to decrement (thesleepAfternote in CLAUDE.md).The test, and what it cannot prove
test/shutdown.test.mjsspawns the real server and sends it a real SIGTERM. Its limits are stated in the file rather than overclaimed:code: null, signal: SIGTERMinstead of a cleanexit(0)) in ~34ms - not by timing out. Verified by planting that removal.PORTbecomes overridable (default still 8080) so the test can bind a free port.Not yet confirmed in production
This is a code-level root cause, not a measured fix. Confirming it means deploying and re-reading the
containersUsageAdaptiveGroupsbuckets for a single render. The issue's step 2 - settingsleepAfterto"10m"to test whether the value is inert - becomes unnecessary if this is right, and stays the fallback if the tail survives.I have not deployed; that is your call, and the issue notes the current cost is ~$0.12/month so there is no urgency.
Verification
pnpm run verifygreen - 1588 app + 13 worker + 4 container (up 1), 0 warnings, exit 0.🤖 Generated with Claude Code
https://claude.ai/code/session_01HKjzpWJw1HbnigPP9udqN2