Review findings from #110 that were deliberately deferred rather than patched in at merge time. None blocked that fix; all are small and independent.
1. PositiveDuration still wraps on large day counts, and now disagrees with ParseDuration
internal/app/backup_schema.go:211-224 multiplies its day branch unchecked, so max_data_loss: 213504d wraps to 25m26s, passes the days <= 0 guard (which checks the day count, not the product), and then passes validateBackupPolicy's one-minute floor. Same for retention.window.
#110 capped app.ParseDuration at maxDurationDays, so the two parsers now diverge on the same field: backup_walg.go:331/:520 compute archive timeout and retain count from the wrapped 25m26s, while backup_state.go:339 and backup_postgres_ops.go:442 get false and silently fall back. Before #110 both wrapped identically — worse, but at least consistent.
Fix: apply the same maxDurationDays cap inside PositiveDuration's day branch. ParsePostgresDuration's d arm (runtime.go:456) has the same unchecked multiply, though its input is a server-reported setting rather than authored.
2. Bounded fields have a self-contradicting description
Every field #110 capped now reads "…at most 7d. … Expects a duration such as 30s, 5m, 1h30m or 14d." The suffix comes from gDur's blurb (internal/app/constraints.go:38-39) and names an example the same sentence forbids. Eight entries across both schema copies and the two generated .mdx tables.
Fix: drop 14d from the appended example for ceiling-bearing fields only — it remains legitimate for retention windows, so gDur should not change globally.
3. The rolling plan hides its drain step
#110 aligned the recreate branch of Describe with what recreateRoleForRelease does. The rolling branch (internal/engine/plan.go:504-511) still never mentions the authored drain.wait sleep or the non-TERM docker kill --signal that retireContainer performs (roll.go:200-208). With strategy: rolling and drain: {signal: USR1, wait: 12s}, the plan hides a kill and a pause the deploy takes.
Note the execution asymmetry is deliberate and should stay: rolling suppresses the signal when it is TERM, because an early TERM would kill a container still serving traffic.
4. interval has a ceiling but no floor, and zero means something different than intended
checkLifecycleDuration rejects durations above 7d but accepts interval: 500ms, which docker create then rejects with "cannot be less than 1s" — a failure that could be caught at validation.
Separately, composeDuration's "an explicit zero is a real instruction" rationale holds for start_period (Docker's default is 0, so 0 genuinely means no grace) but not for interval, where Docker reads 0 as unset and probes at 30s while HealthInterval() models 5s. Not dangerous — the drain budget reads the baked value, and bakedHealthcheck maps 0 to 30s correctly — but the model and the runtime disagree, which is the class of thing #110 set out to remove.
5. The bakedHealthcheck clamp is silent and untested
internal/engine/roll.go:252-256 clamps out-of-range values read back from a container without logging. The operator sees only "container never reported unhealthy… proceeding after buffer", which points at the container rather than at the out-of-range baked timing that caused it. It is also the one behavioural change in #110's engine half with no test.
Fix: a warnf at clamp time, and a case in drain_budget_test.go with a baked Interval above the ceiling.
6. applyStopGrace would fail dangerously if its guard were ever bypassed
internal/app/generate.go:977 calls composeDuration(w.Drain.Grace, 0). Validation guarantees a non-empty grace parses, so the fallback is currently unreachable — but if that ever changed, a malformed value would render as stop_grace_period: 0s, which is an immediate SIGKILL: the most dangerous possible reading of a typo. Dropping the key, or failing loudly, would fail safer than defaulting to zero grace.
Review findings from #110 that were deliberately deferred rather than patched in at merge time. None blocked that fix; all are small and independent.
1.
PositiveDurationstill wraps on large day counts, and now disagrees withParseDurationinternal/app/backup_schema.go:211-224multiplies its day branch unchecked, somax_data_loss: 213504dwraps to 25m26s, passes thedays <= 0guard (which checks the day count, not the product), and then passesvalidateBackupPolicy's one-minute floor. Same forretention.window.#110 capped
app.ParseDurationatmaxDurationDays, so the two parsers now diverge on the same field:backup_walg.go:331/:520compute archive timeout and retain count from the wrapped 25m26s, whilebackup_state.go:339andbackup_postgres_ops.go:442getfalseand silently fall back. Before #110 both wrapped identically — worse, but at least consistent.Fix: apply the same
maxDurationDayscap insidePositiveDuration's day branch.ParsePostgresDuration'sdarm (runtime.go:456) has the same unchecked multiply, though its input is a server-reported setting rather than authored.2. Bounded fields have a self-contradicting description
Every field #110 capped now reads "…at most 7d. … Expects a duration such as 30s, 5m, 1h30m or 14d." The suffix comes from
gDur's blurb (internal/app/constraints.go:38-39) and names an example the same sentence forbids. Eight entries across both schema copies and the two generated.mdxtables.Fix: drop
14dfrom the appended example for ceiling-bearing fields only — it remains legitimate for retention windows, sogDurshould not change globally.3. The rolling plan hides its drain step
#110 aligned the recreate branch of
Describewith whatrecreateRoleForReleasedoes. The rolling branch (internal/engine/plan.go:504-511) still never mentions the authoreddrain.waitsleep or the non-TERMdocker kill --signalthatretireContainerperforms (roll.go:200-208). Withstrategy: rollinganddrain: {signal: USR1, wait: 12s}, the plan hides a kill and a pause the deploy takes.Note the execution asymmetry is deliberate and should stay: rolling suppresses the signal when it is TERM, because an early TERM would kill a container still serving traffic.
4.
intervalhas a ceiling but no floor, and zero means something different than intendedcheckLifecycleDurationrejects durations above 7d but acceptsinterval: 500ms, whichdocker createthen rejects with "cannot be less than 1s" — a failure that could be caught at validation.Separately,
composeDuration's "an explicit zero is a real instruction" rationale holds forstart_period(Docker's default is 0, so 0 genuinely means no grace) but not forinterval, where Docker reads 0 as unset and probes at 30s whileHealthInterval()models 5s. Not dangerous — the drain budget reads the baked value, andbakedHealthcheckmaps 0 to 30s correctly — but the model and the runtime disagree, which is the class of thing #110 set out to remove.5. The
bakedHealthcheckclamp is silent and untestedinternal/engine/roll.go:252-256clamps out-of-range values read back from a container without logging. The operator sees only "container never reported unhealthy… proceeding after buffer", which points at the container rather than at the out-of-range baked timing that caused it. It is also the one behavioural change in #110's engine half with no test.Fix: a
warnfat clamp time, and a case indrain_budget_test.gowith a bakedIntervalabove the ceiling.6.
applyStopGracewould fail dangerously if its guard were ever bypassedinternal/app/generate.go:977callscomposeDuration(w.Drain.Grace, 0). Validation guarantees a non-empty grace parses, so the fallback is currently unreachable — but if that ever changed, a malformed value would render asstop_grace_period: 0s, which is an immediate SIGKILL: the most dangerous possible reading of a typo. Dropping the key, or failing loudly, would fail safer than defaulting to zero grace.