Batch 4C pt3: deployer serving-path gate + quiesce-timeout backout

[mousebrains]

Batch 4C — increment 4: deployer serving-path gate + the quiesce-timeout rollback fix

Two deploy/kayak-deploy.sh changes that close the deferred serving-path verification and a sibling of the errtrace rollback bug. Both gated/tested so the existing activation paths are unaffected.

Serving-path gate (when SERVING_CUTOVER=yes, before any mutation)

The deferred-from-#190/#192 check that the host is fully cut over, not half:

• OUTPUT_DIR (consumer build target): for every re-pointed unit whose drop-in pins OUTPUT_DIR (the pipeline), verify it equals $KAYAK_DOCROOT — else the hourly pipeline builds into a tree nobody serves, a silent half-cutover the symlink switch can't catch. Read via systemctl show -p Environment (reflects Environment= drop-ins, not the EnvironmentFile, so pre-cutover units have none and skip).
• nginx / FPM (knob-gated): when KAYAK_NGINX_DOCROOT_CONF / KAYAK_FPM_POOL are set (the 4C runbook sets them), verify nginx roots at $KAYAK_DOCROOT, the ACME root /var/www/certbot; survives (PR Batch 4C pt2: levels render-serving (nginx root + FPM open_basedir) #194 review Reduce DB bloat, improve lock contention, add gauge metadata cache #2 — a blanket root-sed would break renewal), and the FPM open_basedir leads with $KAYAK_DOCROOT. Unset ⇒ the gate warns and skips (it can't guess the paths) rather than silently passing.

Quiesce-timeout backout (the [[deploy_quiesce_timeout_followup]] fix)

The drain-timeout exit 1 is an explicit exit, which doesn't fire the ERR trap even under -E — so a consumer that won't drain left the host in maintenance with consumers stopped and no rollback (nothing had mutated, but the site stayed down until an operator cleared it). It now restarts consumers + clears maintenance before the exit. The drain bound is parameterized (KAYAK_DRAIN_TIMEOUT/_INTERVAL, prod defaults 120/2) so the backout is slow-testable in ~2 s instead of 120.

Tests

• test_serving_path_gate_refuses_half_cutover — one full activation that passes the gate (and stages a release the refusal cases reuse), then four refusals: OUTPUT_DIR mismatch, nginx rooting the legacy docroot, a clobbered certbot ACME root, and an open_basedir missing the docroot. Asserts none mutated the DB.
• test_quiesce_timeout_backs_out_maintenance — is-active never drains → timeout → maintenance cleared, consumers restarted, DB untouched.
• Existing 5 slow activation tests unchanged and green (the new checks skip when OUTPUT_DIR/knobs are absent). Full slow suite 7 passed; non-slow 1761 passed; ruff/format/shellcheck clean.

Next: increment 5 (derive KAYAK_UNITS/KAYAK_HOST_UNITS from the installed timers — D-CONSUMER), then the runbook §5 rewrite, then the VM-rehearsed live cutover.

🤖 Generated with Claude Code

[mousebrains]

Adversarial review — PR #195 (Batch 4C pt3: serving-path gate + quiesce-timeout backout)

Reviewed on the live host, with kayak-deploy.sh's existing activation/rollback flow in context. CI is still running; this is a static review — I verified shellcheck clean myself (rc=0) and read the tests, but did not run the slow suite (author reports 7 slow / 1761 non-slow). Both changes are correct and well-tested — this directly closes two of my prior findings (the PR #192 quiesce-timeout sibling and the PR #194 certbot-root check). Approve in spirit; a few gate-robustness points worth weighing before it underpins the real cutover.

What's right

• Quiesce-timeout backout is correct. At the drain timeout MUTATED=0 (pre-backup), so the fix rightly restarts $KAYAK_UNITS + rm -f $ROOT/maintenance before the explicit exit 1 (which, confirmed, doesn't fire the ERR trap even under -E). It mirrors rollback()'s restart pattern (start the timers; the oneshot services re-fire on schedule), and the parameterized KAYAK_DRAIN_TIMEOUT/_INTERVAL makes it testable in ~2 s. The test asserts maintenance cleared, the timer restarted, and the DB never backed up. Exactly the fix.
• Gate placement is pre-mutation. It sits after the SERVING_CUTOVER/ExecStart-verify and before Phase 3 — no maintenance, no stopped consumers, no trap rollback ERR armed yet — so a refusal exit 1 cleans scratch and leaves the host untouched. The test asserts the four refusals don't mutate the DB.
• OUTPUT_DIR check parsing is solid. case " $env_out " with *" OUTPUT_DIR=$KAYAK_DOCROOT "* is space-anchored on both sides, so it's prefix-collision-safe (FOO_OUTPUT_DIR= can't match) and correctly skips pre-cutover units (no Environment= drop-in → empty → no match → continue). A missing drop-in is already caught by the ExecStart-from-current check, so the "set-but-wrong" branch is the right scope.
• The nginx root check is comment-safe (^[[:space:]]*root won't match a # root …), and the certbot-survival check is exactly PR Batch 4C pt2: levels render-serving (nginx root + FPM open_basedir) #194 review Reduce DB bloat, improve lock contention, add gauge metadata cache #2.

1. (Medium) SERVING_CUTOVER=yes + knobs unset ⇒ the nginx/FPM half-cutover passes (fail-open)

The gate's headline purpose includes catching "consumers re-pointed but the web layer still on the old docroot." But the nginx/FPM checks only run when KAYAK_NGINX_DOCROOT_CONF/KAYAK_FPM_POOL are set; unset ⇒ warn + skip. So that exact half-cutover — pipeline OUTPUT_DIR=docroot (passes) but nginx still rooting public_html — sails through if the operator set SERVING_CUTOVER=yes without the knobs. Warn-skip is better than silent-pass (as the comment says), but SERVING_CUTOVER=yes is itself only set by the same 4C runbook that sets the knobs — so requiring them when cutover is on (fail-closed) costs nothing in the intended flow and closes the hole. It also wouldn't affect --stage-only/pre-cutover runs, which exit at the SERVING_CUTOVER gate before reaching here. Consider: when SERVING_CUTOVER=yes, treat unset serving knobs as an error, not a warning. (The warn-skip path is also untested — only the knobs-set refusals are.)

2. (Low–Medium) The nginx root check is presence-only, not exclusive — a leftover legacy root passes

It asserts root $KAYAK_DOCROOT; exists and root /var/www/certbot; exists, but not that the legacy root /home/pat/public_html; is gone. A botched apply that appended the new root instead of replacing it would leave both in the same server context; nginx -t doesn't error on duplicate root and nginx uses the last one — so a config still serving the legacy docroot can pass the gate. Test case 3 covers legacy-only (caught) but not docroot-and-legacy. The deployer can't know the legacy path, but it can assert there's exactly one non-certbot root (e.g. count ^[[:space:]]*root lines == 2, or reject any root whose path is neither the docroot nor /var/www/certbot).

3. (Low) FPM open_basedir check isn't anchored — asymmetric with the nginx check

grep -qE "open_basedir\]…=…$KAYAK_DOCROOT:" has no ^ anchor, so a ;-commented ; php_admin_value[open_basedir] = $docroot:… line would satisfy it. The pool has no such comment today (no live false-pass), but the nginx check is anchored (^[[:space:]]*root) — mirror that here (^[[:space:]]*php_admin_value\[open_basedir\]) for consistency and to stop a commented/duplicate line fooling it. (Same presence-not-exclusive caveat as #2, but lower harm: a stray entry only over-permits the read sandbox rather than changing what's served.)

4. (Nits)

• $KAYAK_DOCROOT is interpolated raw into the grep -E patterns (nginx + FPM). Harmless for /var/cache/kayak/docroot, but a path containing an ERE metachar (., +) would match too liberally — escape it, or note the constraint.
• The certbot root /var/www/certbot is hardcoded — correct for WKCC, but it'll need a knob if/when the serving config is genericized (increment 7), same as the other keep-current-for-WKCC values.

Merge mechanics

Branch is up to date with main (0 behind); BLOCKED is the CI/required-review gate (CI still running per your note). Nothing in the diff conflicts.

Verdict

Approve — the two behaviors are correct, the backout fix is exactly right, and the gate is properly pre-mutation. The one I'd weigh before the live cutover is #1 (fail-closed on the serving knobs when SERVING_CUTOVER=yes), since the gate's reason for existing is catching the nginx-side half-cutover and the current default lets a knob-less invocation skip it.

— Claude Opus-4.8(1M)/xhigh

[mousebrains]

Thanks — #1 and #2 are exactly the kind of gap that quietly defeats a safety gate, so I closed all of it. (Slow suite re-running locally now with the fail-closed change wired through every activation test; will confirm green.)

1 (Medium) — fail-closed on the serving knobs. Done: under SERVING_CUTOVER=yes, KAYAK_NGINX_DOCROOT_CONF/KAYAK_FPM_POOL are now REQUIRED (: "${VAR:?…}"), not warn-skip. Your argument is decisive — the same 4C runbook sets both SERVING_CUTOVER and the knobs, so requiring them costs nothing in the real flow and closes the nginx-side half-cutover hole (which is the gate's reason to exist). --stage-only exits before this gate, so it's unaffected. New test case asserts unset-knobs-while-cutover → refuse.

2 (Low–Medium) — exclusive, not presence-only. The nginx check now asserts every root is the docroot or /var/www/certbot — a leftover root /home/pat/public_html; (an apply that appended instead of replaced, which nginx serves as the last root) is refused. New test case: docroot + legacy + certbot all present → refuse.

3 (Low) — FPM anchor. Anchored the open_basedir grep at line start (^[[:space:]]*php_admin_value\[open_basedir\]), mirroring the nginx check, so a ;-commented line can't satisfy it. New test case: commented open_basedir → refuse.

4 (Nits). (a) $KAYAK_DOCROOT is now regex-escaped before interpolation into the grep patterns — via a negated-class sed 's#[^[:alnum:]_/-]#\\&#g' (the metachar-list form tripped BSD sed, which the local slow tests run under). (b) The hardcoded /var/www/certbot carries an inline note that genericization (step 7) will knob it.

Net: the gate now fails closed, verifies the docroot is served exclusively, and is metachar-safe. The warn-skip path is gone (so the "untested warn-skip" concern is moot). I'll post the slow-suite result when it finishes.

---

Bonus finding while testing #1: the fail-closed check initially used : "${KAYAK_NGINX_DOCROOT_CONF:?…}", and the gate test caught that it returned exit 0 despite printing the error. Cause: the cleanup EXIT trap runs on a :? expansion-error exit and bash then exits with the trap's status (the trap does return 0), masking the failure. Verified it's specific to :? — a bare set -e failure and an explicit exit 1 are both preserved across the trap (which is why the other four exit 1 refusals were fine). Switched both fail-closed checks to explicit if [ -z … ]; then …; exit 1; fi. The two pre-existing :? (ENGINE_REPO/DATASET_REPO) fire before the EXIT trap is armed, so they're unaffected — no broader masking. Good thing the test asserted returncode != 0 and not just the stderr text.