adapter: correct the comments around the read-only write-enable

Review of MaterializeInc#38399 found several claims that no longer match the code:

- The `allow_writes_in_read_only` tripwire falls back to a no-op, which
  now blocks promotion rather than leaving a cold collection at cut-over,
  and the storage-side check it points at is a hard assert, not a soft
  panic with a fallback. Say both, and say why compute is deliberately
  softer.
- Exclusive ownership of a replacement shard holds per (build version,
  deploy generation), not per process. Two read-only processes of one
  generation both write the shard, the same shape as a multi-replica MV,
  which the self-correcting persist sink is built to tolerate.
- The replica-side handler enables persist compaction process-wide, which
  this path is the first thing to trigger inside a read-only deployment.
- "Replacement" meant two unrelated things three lines apart in
  `bootstrap`: user `REPLACEMENT FOR` DDL and the builtin-migration
  mechanism. The sets are disjoint, so this is wording only.
- Record why a *new* builtin MV cannot be write-enabled the same way: its
  shard allocation lives only in the read-only savepoint, so the promoted
  leader allocates a different shard.
- Cite PR MaterializeInc#35402 for the v26.17 leader floor, and note that forcing the
  replacement mechanism across all builtins, as the 0dt tests do, leaves
  the caught-up gate reading its own frontiers.

The design doc stated the force-write unconditionally; give the two
conditions and the exclude-from-gate fallback.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: mtabebe

doc/developer/design/20251015_builtin_schema_migration.md

@@ -81,6 +81,11 @@ Because this environment exclusively owns the replacement shard, the read-only p
 This lets a migrated builtin materialized view and its dependents hydrate before cut-over instead of all at once at cut-over.
 It is safe only for the self-owned replacement shard, never a shard the leader is still serving, which is why it applies to shard replacement and not schema evolution.
 
+The force-write is conditional on two things.
+First, the leader must be at v26.17 or later, because every builtin materialized view reads the catalog shard and only leaders from that version on keep its frontier advancing with the current time; against an older leader the dataflow would sit at a stale frontier.
+Second, the `enable_0dt_hydrate_migrated_builtin_mvs` feature flag must be on; it exists as a break-glass revert.
+When either condition does not hold, the migrated materialized views and their dependents are instead excluded from the 0dt caught-up check, which is the older behaviour: promotion proceeds without them and they hydrate at cut-over.
+
 A leader process performing shard replacement performs the same steps as in read-only mode.
 Additionally, it cleans up durable state written by earlier versions and/or deploy generations by:
   - arranging for the previous shards used by the migrated storage collections to be finalized

src/adapter/src/coord.rs

@@ -250,10 +250,10 @@ mod validity;
 ///
 /// Every builtin materialized view reads `mz_internal.mz_catalog_raw`, so its dataflow only makes
 /// progress up to the catalog shard's frontier. Holding that frontier at the current time is the
-/// leader's job, and leaders only started doing it in v26.17. Write-enable such an MV against an
-/// older leader and it sits at a stale frontier and never reports caught up, which blocks
-/// promotion outright instead of merely leaving the collection cold at cut-over. We still support
-/// upgrading from before v26.17, so that leader is a real case, not a hypothetical.
+/// leader's job, and leaders only started doing it in v26.17 (PR #35402). Write-enable such an MV
+/// against an older leader and it sits at a stale frontier and never reports caught up, which
+/// blocks promotion outright instead of merely leaving the collection cold at cut-over. We still
+/// support upgrading from before v26.17, so that leader is a real case, not a hypothetical.
 const MIN_LEADER_VERSION_FOR_MIGRATED_MV_WRITES: Version = Version::new(26, 17, 0);
 
 /// A pool of pre-allocated user IDs to avoid per-DDL persist writes.
@@ -2784,8 +2784,10 @@ impl Coordinator {
                     self.ship_dataflow(df_desc, mview.cluster_id, mview.target_replica)
                         .await;
 
-                    // If this is a replacement MV, it must remain read-only until the replacement
-                    // gets applied.
+                    // If this MV is a pending `REPLACEMENT FOR` another MV, it must stay
+                    // read-only until `ALTER ... APPLY REPLACEMENT` swaps it in. Unrelated to the
+                    // builtin-migration `Replacement` mechanism below, and disjoint from it:
+                    // builtin MVs never come from user DDL.
                     if mview.replacement_target.is_none() {
                         let gid = mview.global_id_writes();
                         if hydrate_migrated_mvs
@@ -2796,6 +2798,13 @@ impl Coordinator {
                             // writing it while read-only hydrates the MV and its dependents before
                             // cut-over. An `Evolution`-migrated MV reuses the leader's live shard
                             // and must never reach here.
+                            //
+                            // A *new* builtin MV gets no such treatment: its shard allocation
+                            // lives only in this read-only savepoint, so the promoted leader
+                            // allocates a different shard and anything written here would be
+                            // discarded. A replacement shard instead survives promotion via its
+                            // durable migration-shard entry, which is what makes the write worth
+                            // doing.
                             self.controller
                                 .compute
                                 .allow_writes_in_read_only(mview.cluster_id, gid)

src/adapter/src/coord/caught_up.rs

@@ -271,6 +271,12 @@ impl Coordinator {
         // `snapshot_latest` requires that the collection consolidates to a
         // set. `mz_cluster_replica_frontiers` is a controller-managed builtin
         // written with ±1 diffs, so it satisfies that invariant.
+        //
+        // NOTE: these are the *leader's* frontiers only because this deployment reads the leader's
+        // shard. A release that `Replacement`-migrates `mz_cluster_replica_frontiers` itself would
+        // hand us a fresh shard that we write our own replica frontiers into, so the lag check
+        // below would compare this deployment against itself and pass trivially. Forcing the
+        // replacement mechanism across all builtins, as the 0dt tests do, has the same effect.
         let live_frontiers = self
             .controller
             .storage_collections

src/compute-client/src/controller.rs

@@ -1153,26 +1153,46 @@ impl ComputeController {
 
     /// Like [`Self::allow_writes`], but takes effect even in read-only mode.
     ///
-    /// The caller must guarantee that no other environment writes the collection's output shard.
+    /// The caller must guarantee that no leader environment writes the collection's output shard.
     /// In a 0dt deployment that means a shard this environment created for itself, the replacement
     /// shard of a `Replacement`-migrated builtin collection, never one the leader is still serving
     /// from. `Evolution` migrates in place and reuses the leader's shard, so it must not reach this
     /// path. Violating the guarantee races two writers on one shard.
     ///
+    /// NOTE: exclusive ownership of a replacement shard holds per (build version, deploy
+    /// generation), not per process: the migration shard entry that names the shard is keyed by
+    /// that pair, and a read-only catalog open is a savepoint, so it fences nothing. Two read-only
+    /// processes of the same generation therefore both write it. That is the same shape as a
+    /// multi-replica materialized view, whose replicas also all write one shard, and the persist
+    /// sink is built to tolerate exactly that: it is self-correcting, so a conflicting write is
+    /// reconciled on the next append. See the `mz_compute::sink::materialized_view` module docs.
+    ///
     /// This is the compute-side counterpart to the storage controller's `force_writable` handling
     /// of migrated storage collections. Migrated builtin tables are storage collections that
     /// storage force-writes read-only; migrated builtin MVs are compute collections that only this
     /// path can force-write. Both rest on the same guarantee (this environment exclusively owns the
     /// replacement shard) but run on separate write paths, so each needs its own bypass.
+    ///
+    /// NOTE: the replica-side handler enables persist compaction process-wide on the clusterd
+    /// (`ComputeState::handle_allow_writes`). This path is the first thing that triggers that
+    /// inside a read-only deployment.
     pub fn allow_writes_in_read_only(
         &mut self,
         instance_id: ComputeInstanceId,
         collection_id: GlobalId,
     ) -> Result<(), CollectionUpdateError> {
-        // Every builtin eligible for the read-only bypass has a system id. A non-system id means
-        // the caller's `Replacement`-only invariant broke, so degrade to the read-only no-op (cold
-        // collection at cut-over) rather than risk writing a shard the leader still serves. Mirrors
-        // the storage-side tripwire in `StorageController::register_introspection_collection`.
+        // Every builtin eligible for the read-only bypass has a system id, so a non-system id
+        // means the caller's `Replacement`-only invariant broke. No-op rather than risk writing a
+        // shard the leader still serves.
+        //
+        // NOTE: that no-op is not the harmless degradation it reads as. A collection left in the
+        // caught-up gate on a shard nobody writes never advances past the minimum frontier, so it
+        // blocks promotion rather than merely arriving cold at cut-over.
+        //
+        // The storage side asserts the same invariant hard, in
+        // `StorageController::register_introspection_collection`. The asymmetry is deliberate: the
+        // write is refused either way, and a soft panic keeps a caller bug loud in CI and Sentry
+        // without taking a production deployment down over it.
         if self.read_only && !collection_id.is_system() {
             soft_panic_or_log!(
                 "allow_writes_in_read_only called for non-system collection {collection_id}; \

test/0dt/mzcompose.py

@@ -1819,6 +1819,11 @@ def get_persist_shard_id(item_id: str, service: str) -> str:
         # caught-up gate they would hydrate at cut-over instead, spiking catalog-server CPU. Reading
         # them here from the read-only generation asserts the write-enable-while-read-only path
         # actually filled those shards.
+        #
+        # NOTE: this covers hydration, not the caught-up gate's lag comparison.
+        # `force_migrations="replacement"` replaces `mz_cluster_replica_frontiers` too, and the gate
+        # reads the leader's "live" frontiers out of that collection, so here it compares this
+        # generation against itself.
         for relation in ["mz_databases", "mz_clusters"]:
             count = c.sql_query(
                 f"SELECT count(*) FROM mz_catalog.{relation}",