amadan
provision: adoption dry-run reports environment, not just channel
Review fix: the adoption branch's dry-run block printed a fixed "would adopt ... converging channel X -> Y" line and skipped convergeOwned entirely, so an operator backfilling a fleet whose records carry Environment never learned that column was about to change too -- a material omission in the report this mitigation exists to let someone read before trusting it. It also claimed a channel convergence would happen even when the route was already on the record's channel. Fixed by falling through to convergeOwned(reg, existing, rec, true) instead of hand-rolling the report: it already knows which of the two columns actually differ, and its own dryRun guard (added in the prior commit specifically so a shared function can't be called unsafely from a caller that forgot to guard) makes the call write-free. Also added the positive control the review flagged: re-running the same TestReconcileDryRunWritesNothing fixture with DryRun false now asserts the registry DID change, so the dry-run half's "unchanged" assertion can't pass vacuously if a future change ever caused a branch to silently stop being exercised.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+38
−4
internal/provision/reconcile.go+11 −2internal/provision/reconcile_test.go+27 −2
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index 649636d7d..2f68aba87 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -469,8 +469,17 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| continue |
| } |
| if cfg.DryRun { |
| - log.Printf("provision: dry-run: would adopt existing route %s (app %q acct %q), converging channel %s -> %s", |
| - rec.Host, rec.App, rec.Account, existing.Channel, rec.Channel) |
| + log.Printf("provision: dry-run: would adopt existing route %s (app %q acct %q) — this record would own its channel and environment", |
| + rec.Host, rec.App, rec.Account) |
| + // convergeOwned reports channel and environment together, |
| + // and only the columns that actually differ (design §4.1) — |
| + // a fixed "converging channel X -> Y" line here would both |
| + // omit environment (the defect this fixes) and claim a |
| + // convergence that would not happen when the route is |
| + // already on the record's channel. Sites 1 and 2 (SetChannel, |
| + // SetEnvironment) are already guarded inside convergeOwned, |
| + // so this call writes nothing. |
| + convergeOwned(reg, existing, rec, true) |
| continue |
| } |
| if err := reg.SetProvisioned(rec.Host, true); err != nil { |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index 00e0965d2..03bba08b3 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -1251,9 +1251,12 @@ func TestReconcileDryRunWritesNothing(t *testing.T) { |
| } |
| // Adopt: a hand-added route matching the record's shape, on a |
| - // different channel — adoption would converge it in the same pass. |
| + // different channel AND environment — adoption would converge both in |
| + // the same pass. Two differing columns, deliberately: a dry-run report |
| + // that only ever mentioned channel here would leave environment's |
| + // change invisible to the operator reading it (design §4.3 review). |
| adoptRec := Record{Schema: 1, Host: "adopt.go.tito.io", Slug: "adopt", App: "tito", |
| - Account: sqid, Pool: "tito", Region: "ie", Channel: "edge", CreatedAt: time.Now()} |
| + Account: sqid, Pool: "tito", Region: "ie", Channel: "edge", Environment: "staging", CreatedAt: time.Now()} |
| if err := CreateRecord(ctx, s, adoptRec); err != nil { |
| t.Fatal(err) |
| } |
| @@ -1303,6 +1306,28 @@ func TestReconcileDryRunWritesNothing(t *testing.T) { |
| if rt, err := reg.Get("acme.go.tito.io"); err != nil || rt != nil { |
| t.Errorf("dry-run minted a route: %+v %v", rt, err) |
| } |
| + |
| + // Positive control (reviewer-flagged): the assertion above — total |
| + // state unchanged — would also pass if this fixture simply never |
| + // exercised the mint/adopt/converge/remove branches at all, e.g. if a |
| + // future validation change caused ownedRec to be skipped and |
| + // protect-ed, silently dropping the converge branch from coverage while |
| + // the test still reported green. Re-running the identical fixture for |
| + // real proves the fixture DOES drive every branch: if state came out |
| + // unchanged here too, the dry-run assertion above would have been |
| + // vacuous, not a proof of anything. |
| + cfg.DryRun = false |
| + if err := Reconcile(ctx, s, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + live, err := reg.List() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if reflect.DeepEqual(before, live) { |
| + t.Fatal("a real pass over the identical fixture left the registry unchanged — " + |
| + "the dry-run assertion above proves nothing if nothing was ever going to write") |
| + } |
| } |
| // The other side of that coin, and the one that matters more: a route no |