perf(boot): drop two dead Node processes from container start - #236
Merged
Conversation
Two of the three items in #205. The third, moving migrations out of the entrypoint, needs a one-off task in seamless-iac and is not safe to do here alone, so it is left in place. initKeys was doing nothing. The Dockerfile sets NODE_ENV=production and ensureKeys() returns immediately on that branch, so the image was paying a full Node cold start for an empty function. Even in development it writes ./keys/private.pem and ./keys/public.pem, which nothing reads: signingKeyStore keeps its own keys under ./keys/dev and generates them lazily, and that is what the JWKS route serves. exec node instead of `npm run start` drops a second Node program whose only job was to run one command, and puts the server at PID 1 so SIGTERM on task draining reaches it rather than npm.
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
1 participant
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.
Two of the three items in #205. The third is deliberately left alone, see below.
initKeys was doing nothing, in both modes
The issue treated this as a small 0.29s win worth folding into server startup. It is better than that: the step does no useful work at all.
ENV NODE_ENV=production, andensureKeys()isif (!isProduction) { ... }followed by a comment placeholder. The image was paying a full Node cold start to call an empty function. Production keys come fromSEAMLESS_JWKS_KEY_*_PRIVATE, whichvalidateEnvs.shalready requires../keys/private.pemand./keys/public.pem, and nothing reads either.signingKeyStore.tskeeps its own keys at./keys/dev/and generates them lazily with an exclusive write, andjwks.tsserves./keys/dev/public.pem. Two different directories.So this is removed rather than folded into startup, which would only have moved dead code into the hot path.
exec node rather than npm run start
startisnode dist/server.js, so npm was a second Node program loaded to run one command. It also sits between the init process and the server for the life of the container, soSIGTERMduring ECS draining reaches npm rather than the server at PID 1.Not done: moving migrations out of the entrypoint
This is the large item, 3.07s of the measured 6.78s, and it is the one I have left in place. Removing
run_migrationshere without a one-off migration task inseamless-iacrunning first would let a task boot against an un-migrated schema, which is worse than a slow boot and is the property the current design is protecting. It also does not fix thedesiredCount: 2race on its own. That needs a coordinated change in the IaC repo and should land there first.#205 should stay open for that item.
Measurements
Local numbers, on a much faster machine than the 0.5 vCPU task in the issue, so these confirm the mechanism rather than the production magnitudes:
node dist/scripts/initKeys.jsatNODE_ENV=production: 0.03s for a no-op (0.29s on the task)npm runvs directly: 0.07s vs 0.00s (0.37s on the task)Against the issue's production log those two are about 0.66s of 6.78s. Re-measure on the task before touching
seamless-iac#99, as the issue asks.Verification
sh -nand shellcheck clean onvalidateEnvs.sh. Full gate green via the pre-commit hook: lint, format, typecheck, 1194 tests, coverage, build. No source change, so no changeset.