fix(liveness): skip self.close() on SIGTERM during HMR/watch - #1212
fix(liveness): skip self.close() on SIGTERM during HMR/watch#1212guitavano wants to merge 1 commit into
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughThe liveness middleware still runs checks and logs on SIGTERM, but it now calls ChangesSIGTERM handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@runtime/middlewares/liveness.ts`:
- Around line 72-74: The dev-mode check in liveness handling is reading Deno CLI
flags from Deno.args, but those flags are not available there, so the shutdown
path still runs in development. Update the isHMR/isWatch/isDev logic in
runtime/middlewares/liveness.ts to use a launcher-provided environment variable
instead of Deno.args, and keep the SIGTERM/self.close() behavior gated off when
that dev-mode variable is set.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8057ab88-95a0-4bc6-9650-602eb5449d21
📒 Files selected for processing (1)
runtime/middlewares/liveness.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Deno's HMR and --watch modes use a parent/child process model where the parent sends SIGTERM to signal the child to restart. The liveness middleware's SIGTERM handler was calling self.close(), which kills the process before Deno can complete its restart cycle — causing every dev server to die on the first file change instead of hot-reloading. Only call self.close() in k8s (detected via KUBERNETES_SERVICE_HOST) where SIGTERM means the pod must shut down. Deno runtime flags like --unstable-hmr and --watch are not visible in Deno.args, so detecting dev mode directly is not reliable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
52cfcb3 to
e8a32a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@runtime/middlewares/liveness.ts`:
- Around line 75-82: The SIGTERM handler in liveness.ts now gates self.close()
on Kubernetes detection, which makes non-K8s production skips the default
shutdown path. Update the logic in the SIGTERM listener so the exception is
based on the specific dev/watch/HMR mode instead of isK8s, and keep self.close()
as the default behavior for normal shutdown handling in
runChecks()/Deno.addSignalListener.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e339e50-1a89-43a3-aceb-7a1bc0701a13
📒 Files selected for processing (1)
runtime/middlewares/liveness.ts
| const isK8s = Boolean(Deno.env.get("KUBERNETES_SERVICE_HOST")); | ||
| try { | ||
| if (Deno.build.os !== "windows") { | ||
| Deno.addSignalListener("SIGTERM", () => { | ||
| const checks = runChecks(); | ||
| console.log(checks); | ||
| self.close(); | ||
| if (isK8s) { | ||
| self.close(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't use Kubernetes presence as the shutdown default.
Lines 75-82 now change SIGTERM behavior from “close unless we're in watch/HMR” to “close only in Kubernetes”. That widens the behavior change to every non-k8s runtime, so ordinary production deployments outside Kubernetes will now skip self.close() too. The exception should be keyed off the specific dev/watch mode, with self.close() remaining the default for normal SIGTERM handling.
Suggested change
- const isK8s = Boolean(Deno.env.get("KUBERNETES_SERVICE_HOST"));
+ const isWatchMode = Deno.env.get("DECO_WATCH_MODE") === "1";
try {
if (Deno.build.os !== "windows") {
Deno.addSignalListener("SIGTERM", () => {
const checks = runChecks();
console.log(checks);
- if (isK8s) {
+ if (!isWatchMode) {
self.close();
}
});
}
} catch (err) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isK8s = Boolean(Deno.env.get("KUBERNETES_SERVICE_HOST")); | |
| try { | |
| if (Deno.build.os !== "windows") { | |
| Deno.addSignalListener("SIGTERM", () => { | |
| const checks = runChecks(); | |
| console.log(checks); | |
| self.close(); | |
| if (isK8s) { | |
| self.close(); | |
| const isWatchMode = Deno.env.get("DECO_WATCH_MODE") === "1"; | |
| try { | |
| if (Deno.build.os !== "windows") { | |
| Deno.addSignalListener("SIGTERM", () => { | |
| const checks = runChecks(); | |
| console.log(checks); | |
| if (!isWatchMode) { | |
| self.close(); |
🤖 Prompt for 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.
In `@runtime/middlewares/liveness.ts` around lines 75 - 82, The SIGTERM handler in
liveness.ts now gates self.close() on Kubernetes detection, which makes non-K8s
production skips the default shutdown path. Update the logic in the SIGTERM
listener so the exception is based on the specific dev/watch/HMR mode instead of
isK8s, and keep self.close() as the default behavior for normal shutdown
handling in runChecks()/Deno.addSignalListener.
1 participant
Summary
self.close()in the liveness middleware's SIGTERM handler when running in dev mode (--unstable-hmror--watch)self.close()was killing it before the restart cycle could complete, breaking hot-reload entirelyRoot cause
Deno's HMR uses a parent/child model: parent watches files → sends SIGTERM to child → child exits → parent relaunches. The
self.close()call in the SIGTERM handler was terminating the Deno runtime before HMR could complete its restart, causing every dev server to die on the first file change.Test plan
deno task devon any deco site — verify HMR restarts correctly on file changes instead of dyingdeno run -A --watch main.ts— verify watch mode restarts correctlyself.close()when not in dev mode)🤖 Generated with Claude Code
Summary by cubic
Skip calling
self.close()on SIGTERM outside Kubernetes so Deno HMR/--watchcan restart cleanly; production (k8s) shutdown behavior is unchanged.KUBERNETES_SERVICE_HOST; only callself.close()when present.--watchrestart the process without an early runtime close.Written for commit e8a32a1. Summary will update on new commits.
Summary by CodeRabbit