fix(cli): make post-deploy-failure hook best-effort and honor proxy.app_port in deploy/rollback - #3143
Conversation
…pp_port in deploy/rollback Two same-file defects in DeployMainCli.cfc: - #3087: $fireHook(post-deploy-failure) fired unguarded inside the deploy catch block, so a flaky notification hook threw DeployMainCli.HookFailed and replaced the original deploy error. The hook call is now wrapped in its own try/catch (mirroring the allowFail lock release in finally): the hook's non-zero exit is logged with the [hook:post-deploy-failure] prefix and the original exception rethrows. - #3089: deploy() and rollback() hardcoded the kamal-proxy target to <container>:3000, ignoring proxy.app_port entirely (code default 80, init template 8080). Both verbs now build the target from config.proxy().appPort(). Default story aligned on Kamal's 80; docs and the init template's explicit 8080 (matching its Dockerfile) unchanged. Docs: hooks.mdx / migrating-from-kamal.mdx / cli docs hooks.md updated to the shipped best-effort behavior; config-reference.mdx caution block about the hardcoded :3000 removed and app_port description now states the proxy target wiring; cli docs proxy.md example no longer suggests 3000. Specs: 5 new in DeployMainCliSpec (43 specs green), plus a new with-proxy.yml fixture. CLI suite 931 pass with only the 3 pre-existing environment-dependent failures (verified identical on baseline); core suite 4445 pass with only the 12 tolerated testClientSpec artifacts. Fixes #3087 Fixes #3089 Signed-off-by: Peter Amiri <peter@alurium.com>
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This PR fixes two verified defects in cli/lucli/services/deploy/cli/DeployMainCli.cfc — a failing post-deploy-failure hook masking the original deploy error (#3087), and the kamal-proxy target hardcoded to :3000 instead of honoring proxy.app_port (#3089). Both fixes are minimal, correct, red-first tested, and the docs are aligned end-to-end. Verdict: approve.
Correctness (verified, no findings)
I attempted to refute every load-bearing claim against the code at 45b5ff18c; all held up:
- #3087: the hook call is wrapped in its own
try/catchwithrethrowoutside it (DeployMainCli.cfc:123-131), so the original exception survives.catch (any hookError)also covers non-HookFailedfailures (e.g. a ProcessBuilder I/O error), which is strictly better than catching onlyDeployMainCli.HookFailed. The dry-run path never throws from$fireHook(it only appends to the buffer,DeployMainCli.cfc:494-500), so behavior there is unchanged. UsingwriteOutputfor the hook-failure log is consistent —$fireHookalready streams hook output viawriteOutputatDeployMainCli.cfc:517. - #3089: both
deploy()(DeployMainCli.cfc:80,100) androllback()(:158,165) now build the target fromcfg.proxy().appPort().Config.proxy()returns a defaultProxy({})when the block is absent (Config.cfc:59-64) andProxy.appPort()defaults to80(Proxy.cfc:27-33), so the no-proxy fixture path is safe.redeploy()andsetup()delegate todeploy()and inherit the fix.appPort()'s declarednumericreturn type also means a non-numericapp_portfails at cast rather than flowing into the shell command.
Tests
The 5 new specs assert real contracts, not incidental strings — I cross-checked each assertion against its source:
Wheels.Deploy.RemoteExecutionFailedmessage"Remote command failed on … (exit 1): docker pull …"(FakeSshPool.cfc:73-77) satisfies bothtoInclude("docker pull")and the marker-filetoInclude("exit 1")viaKAMAL_ERROR.- The logged line is
"[hook:post-deploy-failure] Hook post-deploy-failure exited with code 1 (ignored — …)"($fireHookthrow atDeployMainCli.cfc:522-526plus the new catch), matching bothtoIncludeassertions. --target demo-web-v1:8080matchesProxyCommands.deploy()'s--targetflag (ProxyCommands.cfc:37-43) andcontainer_name()'sservice-role-versionformat (AppCommands.cfc:63-64).- FakeSshPool in non-strict mode default-passes the unexpected lock acquire/release commands (
FakeSshPool.cfc:93-94), so the real-mode #3087 specs are deterministic. - Inline
directoryDeletecleanup andfileSetAccessMode(path, "755")follow this spec file's existing prior art (DeployMainCliSpec.cfc:115-133and the #2671 spec).
Happy path (hook runs, receives KAMAL_ERROR, marker proves execution) and error path (hook exit 1 logged, original error rethrown) are both covered, plus the default-80 fallback.
Docs
All four doc surfaces (guides hooks.mdx / config-reference.mdx / migrating-from-kamal.mdx and the embedded wheels deploy docs hooks.md / proxy.md) now describe the shipped behavior, the stale :3000 caution block is removed, and the changelog fragment changelog.d/deploy-hook-masking-and-app-port.fixed.md uses a valid type. The PR body's test-evidence claims are specific and honest about the 3 pre-existing environment-dependent CLI-suite failures.
Commits
Single commit, header is a valid conventional-commit (fix(cli): …, 95 chars ≤ 100), body explains the why, and the DCO sign-off Signed-off-by: Peter Amiri <peter@alurium.com> matches the git author identity exactly.
Summary
Two same-file
wheels deploydefects incli/lucli/services/deploy/cli/DeployMainCli.cfc, both verified to still reproduce on currentdevelopbefore coding:#3087 — failing
post-deploy-failurehook masked the original deploy error$fireHook(hooks, "post-deploy-failure", ...)fired unguarded inside the deploycatchblock beforerethrow, and$fireHookthrowsDeployMainCli.HookFailedon any non-zero hook exit. A flaky notification hook therefore replaced the real deploy failure.Fix: the hook call is wrapped in its own try/catch — mirroring the
allowFaillock release in the adjacentfinallyblock, which exists for exactly this shadowing reason. The hook still runs (best-effort, still receivesKAMAL_ERROR), its non-zero exit is logged as[hook:post-deploy-failure] Hook post-deploy-failure exited with code N (ignored — surfacing the original deploy error), and the original exception rethrows. Exit stays non-zero.#3089 —
proxy.app_portthree-way contradiction, target hardcoded to:3000deploy()androllback()hardcoded the kamal-proxy target to<container>:3000, ignoringproxy.app_portentirely (code default 80, docs said 3000, init template scaffolds 8080 — and none of it reached the proxy).Fix (issue-recommended resolution): both verbs now build the target from
config.proxy().appPort(). Default story aligned on Kamal's80(the existingProxy.cfccode default, unchanged); the init template keeps its explicitapp_port: 8080(correct for its generated Dockerfile, whichEXPOSEs 8080); docs now agree everywhere — theapp_port: 3000examples inconfig-reference.mdxand the embeddedproxy.mdare gone, and theconfig-reference.mdxcaution block documenting this bug is removed.Red-first evidence
4 new specs failed before the fix with exactly the defect signatures:
The #3087 specs exec a real failing hook via ProcessBuilder (marker file proves it ran and received
KAMAL_ERROR) and assert the[hook:post-deploy-failure] ... exited with code 1log line viasavecontent.Test evidence (Docker lucee7 harness)
DeployMainCliSpec: 43 pass / 0 fail / 0 error (38 existing + 5 new)internal.testClientSpecartifactsDocs updated
web/.../deployment/hooks.mdx— event table row, failure-path paragraph, PagerDuty example commentweb/.../deployment/config-reference.mdx—app_portbullet now describes the--targetwiring; stale:3000caution removed; example uses 8080web/.../deployment/migrating-from-kamal.mdx— stale "make hooks exit 0" caveat removedhooks.md+proxy.md(wheels deploy docs ...) — same alignmentchangelog.d/deploy-hook-masking-and-app-port.fixed.mdfragmentFixes #3087 and Fixes #3089
🤖 Generated with Claude Code