fix(preview-create-restart): fix 3 root causes behind new-preview 502

[samo-agent]

Root cause analysis

New PR previews returned 502 for samo-we-field-record. Three compounding root causes were identified in b308f87:

Bug 1 — unit restart (src/env/script.ts:~878)

systemctl enable --now "$UNIT" is a systemd no-op when the unit is already active — systemd returns 0 without touching the running process. The process started on OLD_PORT keeps listening on OLD_PORT even after .env is rewritten with NEW_PORT. Caddy routes to NEW_PORT, nothing listens → external probe sees HTTP 502.

Fix: replace enable --now with enable "$UNIT" && restart "$UNIT". restart always brings a fresh process that re-reads the current EnvironmentFile (.env). Also adds the required systemctl restart sudoers grant to buildHostPrepScript (and replaces the old combined enable --now grant with separate enable + restart grants).

Bug 2 — upsert-on-failure clobbers prNumber (src/commands/env.ts:~717–731)

envStore.upsert(record) ran unconditionally on both success and failure. The record construction does not include prNumber (it is not in EnvScriptTarget). On a re-create failure, upserting spread a record without prNumber → silently cleared the field from an existing PR-managed env record → broke the closed-PR reaper guard (env.prNumber !== undefined) → the broken env was never reaped, stranded permanently.

Fix: skip the upsert when outcome !== "ok" AND existing !== undefined. Fresh creates (no existing record) still upsert on failure for idempotent retry. The success path is unchanged.

Bug 3 — noop err sink + unconditional lastDeployedSha stamp (src/commands/trigger.ts:~795)

ensurePreviewImpl passed noop as the err sink to runEnvCreate. When create failed, the diagnostic message (e.g. "external probe failed … HTTP 502") was silently swallowed — journalctl -u samohost-trigger only showed action:"failed" with no reason. Additionally, lastDeployedSha was stamped onto the env record unconditionally even on failure. The next trigger cycle saw existing.lastDeployedSha === pr.headSha → needDeploy = false → never retried the broken preview. The env was permanently stuck.

Fix: wire a real err sink that writes to process.stderr AND forwards to the caller's err sink (mirrors reapPreviewImpl ~line 904). Stamp lastDeployedSha only when outcome === "ok". Update TriggerDeps.prPreview signature to accept an optional err parameter so runTriggerRun can forward its own err sink — all existing tests are backward-compatible.

TDD evidence

• RED commit: d7c019a — 3 new failing tests, 231 passing
• GREEN commit: 65d9d52 — 234 passing, 0 failing
• Full bun test output: 1021 pass, 0 fail

Test plan

• bun test passes (1021 tests)
• E2E: env create samo-we-field-record field-record --branch preview/bright-green-background --db dblab --json returns outcome:"ok"
• https://field-record-preview-bright-green-background.samo.cat/api/version returns HTTP 200
• ~/bin/dev-story-previews.sh shows the PR passing
• Merge only after pipeline green + samorev PASS (do not merge this PR yourself)

Intentionally deferred

• The live trigger runs unfixed origin/main every 3 min and may re-break the env until this MR is merged. The E2E run proves the fix works; durable resolution needs merge.
• lastDeployedSha is not currently stamped at the right level in the runPrPreviewPass algorithm (it delegates to ensurePreview). A follow-up could move stamping into runPrPreviewPass itself for cleaner separation of concerns.

🤖 Generated with Claude Code

---

⚠️ Operator prerequisite — REQUIRED merge/deploy ordering

This fix calls systemctl restart on active preview units, which needs a sudoers grant that only lands when host-prep is re-run as root on the VM. Safe sequence (do NOT merge first):

1. Run host-prep as root on samo-we-field-record → applies the restart sudoers grant.
2. Verify sudo -u agent sudo /usr/bin/systemctl restart 'field-record@*.service' has no permission-denied.
3. Then merge this MR.
4. Confirm next trigger cycle reports outcome: "ok" and ~/bin/dev-story-previews.sh goes green.

If merged before step 1: fresh creates are fine; re-creates of active units hard-fail (retriable each cycle, not a silent 502).

[samo-agent]

VERDICT: REQUEST CHANGES

Gate status

• M1 (MR-head pipeline): GREEN — https://github.com/NikolayS/samohost/actions/runs/27975082387 — head SHA bcf2f3a confirmed match
• M2 (samorev verdict): REQUEST CHANGES (this verdict)
• M3 (walk evidence): N/A — backend/CLI-only, no UI surface touched

Findings

BLOCKING

1. Operator prerequisite not documented — operator-prereq-or-degraded-gate rule
src/env/script.ts:~878 — BLOCKING — [[operator-prereq-or-degraded-gate]]

The fix emits sudo /usr/bin/systemctl restart "$UNIT" for re-creates of active units. That command requires a new sudoers grant (NOPASSWD: /usr/bin/systemctl restart field-record@*.service) that buildHostPrepScript now generates — but only takes effect after host-prep is re-run as root on the live VM.

The MR body says "adds the required sudoers grant to buildHostPrepScript" but does NOT document the required deployment ordering.

Failure mode if merged before sudoers is applied:

• Fresh creates (unit inactive): unaffected — the enable --now branch uses the existing sudoers grant, succeeds.
• Re-creates of active units (the bug scenario): is-active returns 0, then restart hits permission-denied (exit 1) → first group fails → !is-active is now false → second group skips → overall exits 1 → env-create hard-fails upfront.

This is not silently worse than today (today: no-op → 502 → sha stamped → never retried; after merge without sudoers: hard fail → no sha stamp → trigger retries every 3 min). But it is still a broken state until host-prep is run, and the MR gives no operator guidance on ordering.

Per [[operator-prereq-or-degraded-gate]]: the MR must either (a) document that host-prep must be run on the VM BEFORE the merged code takes effect, with explicit ordering in the MR description, OR (b) add a degraded/warn-only path when the restart grant is absent (e.g. detect permission-denied and fall back with a warning).

Required fix: add an "Operator prerequisite" section to the MR description (or a DEPLOYMENT.md note) stating: "Run host-prep as root on samo-we-field-record BEFORE the first trigger cycle after merge to apply the restart sudoers grant. Until host-prep runs, re-creates of active units will hard-fail (retriable) instead of 502-ing silently."

NON-BLOCKING (nits, no hold)

2. Bash conditional has an edge case on FAILED-state units
src/env/script.ts:~186-196 — LOW — informational

If the unit is in systemd failed state (not active), systemctl is-active returns non-zero. The second branch runs enable --now on a failed unit, which may fail or behave unexpectedly without a prior reset-failed. This edge case exists in the original code too (enable --now on failed units is equally broken) — not introduced by this MR, but worth a follow-up issue.

3. TDD sequence has a post-GREEN fixup commit
test/trigger.test.ts (commit bcf2f3a756) — LOW — [[tdd-always-includes-playwright]] waiver applied (CLI-only, no browser surface)

Commit ec0b086a67 (backward-compat refactor of script.ts) comes after the GREEN commit 65d9d52dfd. This is structurally fine — the refactor keeps all tests passing and the pipeline is green on head. Not a blocker, but note that the canonical RED→GREEN sequence is d7c019ae40 → 65d9d52dfd → ec0b086a67 (refactor preserving green) → bcf2f3a756 (TS import fix). All four commits pass CI. TDD contract is satisfied.

Diff correctness summary (for reference)

Fix 1 (script.ts): Logic is correct. { is-active && restart } || { !is-active && enable --now } correctly distinguishes re-create (active unit, re-reads .env via restart) from fresh create (inactive unit, enable --now). systemctl restart is atomic — no stop/start downtime gap for healthy envs. No regression to success path. The new sudoers entry in buildHostPrepScript is correct for the new command path.

Fix 2 (env.ts): shouldUpsert = outcome === "ok" || existing === undefined is correct. Success path: shouldUpsert=true → upsert runs → identical to old behavior. Fresh-fail path: existing===undefined → shouldUpsert=true → record pinned for idempotent retry, same as before. Re-create-fail path: shouldUpsert=false → skip upsert → preserves existing record including prNumber. No regression.

Fix 3 (trigger.ts): ensureErr sink writes to process.stderr and forwards to callerErr?.(s). TriggerDeps.prPreview signature change is backward-compatible (optional err param). lastDeployedSha stamped only when outcome === "ok" — correct, ensures trigger retries on failure. No regression to success path.

No secrets, no fake-UX strings, no wall-clock LLM timeouts, no unrelated churn. The two sleep: async (_ms) => {} additions are test-only mock deps, not production timeouts.

Required merge/deploy ordering

1. Run host-prep as root on samo-we-field-record to apply the restart sudoers grant.
2. Verify grant: sudo -u agent sudo /usr/bin/systemctl restart field-record@*.service --dry-run (or equivalent).
3. THEN merge this MR.
4. Wait for next trigger cycle (3 min) to confirm re-create succeeds with outcome:"ok".

Merging without step 1 leaves re-creates hard-failing (retriable) rather than 502-ing silently — not catastrophic, but the blocking finding above requires the MR to document this ordering before merge.