adapter: scope config-sync-file parameters with segments and rules

[claude]

Requested by Moritz Hoffmann · Slack thread

Builds on the gate removal from #38206, which has landed in main.

Motivation

Before. A self-managed deployment configures system parameters by pointing
systemParameterConfigmapName at a ConfigMap holding a flat
{parameter: value} JSON object. Every key in it applies to the entire
environment, so a setting can only be turned on for everything at once. There is
no way to say "this value, but only for the analytics cluster" or "only for the
replicas of a legacy size family".

After. The same ConfigMap can also scope parameters to a subset of clusters
and replicas, alongside the environment-wide keys. Existing flat ConfigMaps keep
working exactly as before, with the same syntax and the same meaning.

Cluster-coherent and replica-local system parameters already existed and worked:
parameters declare ParameterScope::{Cluster,Replica}, overrides live in the
durable cluster_system_configurations / replica_system_configurations
collections, and resolution happens at plan time for cluster and via the
compute controller's per-replica dyncfg push for replica. They were reachable
only through LaunchDarkly. Both scoped passes in
src/adapter/src/config/frontend.rs early-returned an empty map for the file
client, because a flat JSON object has no key in which to express a scope.
Self-managed deployments never build a LaunchDarkly client, so the whole feature
was unavailable to them.

Why segments, not names. The file must be able to serve as a fallback for
when LaunchDarkly is down. Exact cluster and replica name targeting cannot
express what LaunchDarkly targeting expresses, so a file written that way would
silently change which objects a parameter applies to at the moment it took
over. A segment matches on the same attribute vocabulary that the LaunchDarkly
cluster and replica context kinds already carry, so the two sources can
express the same intent. Scope for this PR is attributes only: no percentage
rollouts and no bucketing. An id is just an attribute you can match on.

Description

The config sync file gains two reserved top-level keys, segments and rules.
Everything else about the file is unchanged.

{
  "max_connections": 1000,

  "segments": {
    "legacy-replicas": {
      "contextKind": "replica",
      "clauses": [
        {"attribute": "replica_size_family", "op": "in", "values": ["legacy"]},
        {"attribute": "replica_name", "op": "matches", "values": ["^scratch-"], "negate": true}
      ]
    },
    "analytics-cluster": {
      "contextKind": "cluster",
      "clauses": [{"attribute": "cluster_name", "op": "in", "values": ["analytics"]}]
    }
  },

  "rules": [
    {"segment": "analytics-cluster", "parameters": {"enable_eager_delta_joins": true}},
    {"segment": "legacy-replicas", "parameters": {"enable_lgalloc": true}}
  ]
}

A segment is a named predicate selecting the clusters or replicas a rule
applies to. Its shape is LaunchDarkly's: a context kind and an array of clauses,
{attribute, op, values, negate}. Clauses are ANDed, values within a clause are
ORed, and negate inverts the clause after that OR, so a negated in over two
values means "neither of them" rather than "not the first one". negate defaults
to false when absent, which is more lenient than upstream, where the field is
required; this file is hand-authored.

Adopting LaunchDarkly's clause rather than inventing a predicate object buys three
things: the whole string operator family in one go instead of just one new
operator, a cloud-side translator that is close to a copy at the clause level, and
a file whose predicates agree with what environmentd already evaluates through
LaunchDarkly by construction rather than by review. A clause pasted out of the
LaunchDarkly API works as written, its REST-only _id being ignored rather than
refused.

The context kind is on the segment, not the clause. LaunchDarkly puts
contextKind on each clause. Hoisting it spares the repetition, and more
importantly it makes the cluster-coherence rule structural: a cluster segment
is the only kind consulted for a cluster and it cannot name a replica attribute
at all, so a cluster-coherent parameter cannot be targeted by something that
varies per replica. A per-clause contextKind is therefore refused as an unknown
clause key. contextKind is required and must be cluster or replica.

Operators. The five string ones are supported: in, startsWith, endsWith,
contains and matches. Every one of the seven attributes is string-valued, so
the numeric (lessThan, lessThanOrEqual, greaterThan, greaterThanOrEqual),
date (before, after) and semantic-version (semVerEqual, semVerGreaterThan,
semVerLessThan) operators could only ever evaluate false. They are recognised
and refused with that reason rather than reported as typos, which is the
difference between an author fixing the clause and an author hunting for a
misspelling that is not there. segmentMatch is refused separately: this file
already names its segments in the rules array, and a clause-level segment
reference would be a second mechanism for the same thing. An operator string that
is not LaunchDarkly's at all is refused as unknown, unlike the SDK, which maps it
to a variant that never matches.

matches. Compiled once when the ConfigFile is parsed, the compiled Regex
living in the parsed structure beside the values, so evaluation never compiles;
the parse cache makes that happen once per change to the file rather than once per
one-second tick. Patterns are unanchored, which is both the regex crate's
default and what LaunchDarkly's matches does, it being literally the same crate,
so a pattern means the same thing in the file and in LaunchDarkly. ^...$ is how
a whole-value match is asked for, and the docs say so because it is the kind of
thing that silently surprises people. A matches value must be a string: a number
or boolean there is a value rather than a pattern.

Mirrored, not imported. The SDK's Op enum and every Clause field are
pub(crate) in launchdarkly-server-sdk-evaluation, so these types can neither
be imported nor constructed here. Operator is a hand-mirrored copy with a
comment saying so, and test_operator_vocabulary_matches_launchdarkly keeps it
honest: all fifteen LaunchDarkly operator strings are accounted for exactly once
as either supported or refused with a reason, and every supported name round-trips
through the parse. An operator added to the SDK will not fail that, which is the
limit of what is possible without the compiler; what it does catch is our own list
drifting.

Removed. The old terse form, "legacy-replicas": {"replica_size_family": ["legacy"]}, is gone rather than kept alongside. Nothing is released yet, so
there is no compatibility to keep, and two forms would double the typo surface.
Every doc example, unit test and the end-to-end test is rewritten.

mz-adapter gains a regex.workspace = true dependency; the Cargo.lock change
is the single line adding regex to the mz-adapter entry, hand-written because
cargo cannot run in this environment, and regex was already in the lock for
other crates.

A rule attaches parameters to a segment. Targeting a single cluster is a
cluster segment with one in clause on cluster_name.

Why an operator family at all. Real targeting rules select clusters and
replicas by name pattern, and a pattern over names cannot be pre-expanded into an
exact list: a cluster or replica that does not exist yet must still be targetable,
and a pattern re-applies to one created later. Operators are accepted on all seven
attributes rather than restricted to the name ones; a startsWith on
replica_size is harmless even where an exact list would also work.

Ordering. rules is an ordered array, and for each (object, parameter) the
first matching rule wins. An array is the only shape that can carry the order:
the parse collects a JSON object into a BTreeMap, so document order in an
object is discarded, and there is no other ordering mechanism in the file. A rule
that does not mention a parameter does not affect it, so a later rule may still
set it. The fallthrough is the environment-wide value. The win is decided before
the value is judged, so a rule that supplies a value equal to the environment-wide
value shadows the later rules for that parameter rather than falling through:
whether an override lands should not depend on a fallthrough that only a malformed
value could trigger. null in a rule means "no opinion", exactly as at the top
level, and leaves the parameter to a later rule.

The coherence guard. A cluster-coherent parameter is consumed once per
cluster, at plan time, so it must resolve identically for every replica of that
cluster. contextKind carries most of this structurally: the cluster pass
consults only cluster segments, and a cluster segment cannot name a replica
attribute. On top of that, a rule supplying a Cluster-scoped parameter through
a replica segment has that parameter dropped, with a warning naming the
segment and the parameter, while any replica-local parameter in the same rule
still applies, and the dropped parameter is left for a later rule rather than
consumed. Being honest about the mechanics: the segment match already fails for
such a segment in the cluster pass, so the guard is not what produces today's
behaviour. It is in the code so the invariant does not rest on the callers pairing
the parameter scope and the context kind correctly, and its operator-facing half,
the warning, is the part that actually earns its keep. Replica-scoped parameters
may use either kind of segment.

Removed. The name-keyed clusters and replicas sections are gone. Segments
replace them, and one mechanism is better than two with an interacting precedence
rule that would then have to be defined, documented and tested. This PR has not
merged, so there is no compatibility obligation.

Fail-safe segments. Anything this binary cannot evaluate makes the whole
segment match nothing, so the rules naming it do not apply: a missing or
unrecognised contextKind, a missing clauses array, an unknown attribute, a
replica attribute in a cluster segment, an unsupported or unknown op, values
that is not an array of scalars, a negate that is not a boolean, an invalid
matches pattern, or an unknown key in a segment or a clause. Dropping the
offending clause instead would leave the surviving ANDed clauses matching a
wider set of objects than the author wrote, and a segment whose every clause was
dropped would match everything, which is how a typo in an attribute name would
turn a one-cluster rule into an environment-wide one. A segment with an unusable
contextKind is kept and matched against nothing rather than dropped from
segments, so the rules naming it report the real reason instead of a misleading
"no segment named".

Diagnosed, not fatal. Each of the operator's realistic mistakes gets a
warning naming what to fix: each of the segment and clause defects above, by
segment name and clause position (the invalid-pattern warning carries the regex
error, collapsed onto one line so a warning stays one log line), a rule naming a
segment that does not exist, a parameter that is not scopable at all (which also
catches a misspelled parameter name), the coherence violation above, and a value
that does not parse for its parameter's type. They are reported once per change to the file, off the back of
the parsed-file cache, not once per one-second tick.

Everything else from the previous rounds stands unchanged: a whole-document read
or parse failure means "no information" and skips the scoped reconcile rather
than pruning every override; per-section leniency, so one bad segment or rule
cannot strand the rest of the file; the parsed-file cache that keeps the
synchronous read off the coordinator loop; and the shared
classify_scoped_value, so the file path records only values that differ from
the environment-wide value in the parameter's canonical encoding (a file's
false matches a var-formatted off and is therefore not recorded) and never
stores an unparseable one, which would poison resolution. The per-object output
contract is untouched: ScopedParameters, the catalog op, the durable
collections, the introspection views and both resolution boundaries are all as
they were. This is purely a change to how the file becomes per-object overrides.

Key collision. The reserved keys segments and rules shadow any system
parameter of the same name. No VarDefinition or dyncfg is named either, and
test_no_synced_parameter_shadows_a_reserved_section now asserts that for both,
so introducing one fails CI with a message telling the author to rename the
parameter rather than silently reinterpreting operators' ConfigMaps.

Docs: the "Scoping Parameters to Clusters or Replicas" section of
doc/user/content/self-managed-deployments/configuration-system-parameters.md
is rewritten for the segment and rule shape, with the attribute table, the
AND/OR rules, a "Matching by pattern" subsection covering the object form, the
OR between in and matches, that patterns are unanchored, and that an invalid
pattern makes the segment match nothing, plus first-match-wins, the guard, the
fail-safe behaviour, and the introspection queries. The
systemParameterConfigmapName CRD doc comment is updated in both the v1alpha1
and v1 copies, and the two generated materialize_crd_descriptions_*.json
files are hand-patched to match byte for byte (the description field is the doc
comment verbatim) because their generator,
cargo run -p mz-cloud-resources --bin crd-writer, needs a working cargo,
which this environment does not have.

Nothing under doc/developer/generated/ is touched.

Two comment-only corrections about LaunchDarkly's own ordering, since this PR is
where the question came up. The ld_ctx comment now says only what the locked
launchdarkly-server-sdk-evaluation source says: the order contexts are added to
the multi-context does not affect evaluation, because a clause or target names its
context kind and the SDK resolves that kind by lookup (Context::as_kind) rather
than by an ordered scan, and precedence within a flag is individual targets, then
rules, then the fallthrough, array order winning within each. test/launchdarkly/mzcompose.py
carried a stronger claim, that evaluation order follows the definition order of the
flag's variations. The SDK evaluates contextTargets in array order, so that would
require LaunchDarkly's server to reorder what it serves; it is an unconfirmed
observation about the server rather than SDK behaviour, and the comment now says so.
Neither change affects any code path.

Verification

Tests updated and added. Rust unit tests in
src/adapter/src/config/frontend.rs, keeping the previous round's coverage in the
new format and adding the new cases:

• Parsing: a flat-only file is wholly environment-wide (and every JSON scalar
  renders to the expected raw string, with null meaning "no opinion"); segments
  and rules parse with the reserved keys excluded from the environment-wide map,
  a boolean attribute written either as true or as "true" normalizing to the
  same value; a malformed segment or rule element is dropped while the entries
  beside it survive and the surviving rules keep their position in the file, so a
  diagnostic's rule number still matches what the operator wrote; a document that
  is not a JSON object yields nothing.
• Resolution: a cluster segment applies to the matching cluster only; a segment
  ANDing a replica and a cluster attribute applies to neither object that matches
  just one of them; a cluster-attribute segment reaches every replica of that
  cluster; an unparseable value is dropped; a value matching the
  environment-wide value records no override; a segment matching nothing live has
  no effect.
• Rule ordering (new): the first matching rule decides a parameter even when
  its value agrees with the environment-wide one, so it shadows a later catch-all,
  while the parameter that first rule does not mention is still decided by the
  catch-all. A second cluster-scoped parameter was needed to observe both halves
  in one file.
• The coherence guard (new): the cluster-scoped parameter of a
  replica-discriminating rule is dropped and decided by a later rule, the
  replica-local parameter in that same rule still applies, and the diagnostic
  names the segment, the parameter and the attribute.
• Segment and clause defects: an unknown attribute, a missing or unrecognised
  contextKind, a missing clauses array, a per-clause contextKind, and a
  replica attribute in a cluster segment each mark their segment
  uninterpretable, resolve to nothing rather than to everything, and are diagnosed
  by segment name and clause position, asserted as exact strings in a
  deterministic order.
• Malformed clauses: nine shapes (not an object, missing or non-string
  attribute, missing op, missing / non-array / non-scalar values, non-boolean
  negate, unknown key) each rejected with their own reason, and LaunchDarkly's
  _id accepted and ignored.
• Operators: each of the five over cluster_name, including that values within
  a clause are ORed, that matches is unanchored, and that an empty values list
  satisfies nothing. Each of the ten refused LaunchDarkly operators asserted to
  produce its own reason string, and starts_with / IN refused as unknown.
• negate: inverts after the OR, so a negated in over two values excludes
  both. Its absence meaning "not negated" is what every other clause in these
  tests relies on, and a non-boolean negate is refused.
• The operator vocabulary: all fifteen LaunchDarkly operator strings accounted
  for exactly once as supported or refused, and every supported name round-trips
  through the parse. This is the test the mirrored-not-imported comment promises.
• Patterns: a pattern obeys first-match-wins across two rules, the narrower
  pattern's rule holding a parameter at the environment-wide value against the
  broader one; an invalid pattern resolves to nothing rather than to what its
  segment's surviving clause allows, and is diagnosed.
• Diagnostics: a parameter that is not scopable, a misspelled parameter name, a
  rule naming a segment that does not exist, and an unparseable value, asserted
  as exact strings and in a deterministic order.
• The reserved-name guard for segments and rules.

Test updated end to end. test/dyncfg/mzcompose.py creates its cluster with
two replicas, so a rule targeting one replica and a rule targeting the whole
cluster contend for the same parameter, and asserts that r1 keeps the
environment-wide value while r2 takes the whole-cluster rule's value: rule
ordering, observed through the durable collections. Because a segment now declares
one context kind, the cluster and its replicas take one segment each, which the
file spells out. The same file exercises a cluster-scoped parameter on a replica
rule (the guard), a segment whose only clause names a misspelled attribute, a
segment whose matches pattern does not compile, a rule naming a segment that does
not exist, a segment matching no live cluster, and an unparseable value, each with
a parameter whose leak would show up as a row that must not exist. The two segments
that are widened so a later-created cluster also matches them use startsWith
rather than a longer exact list, which is the case an exact list cannot express, so
the create-time fold is exercised through a clause authored before that cluster
existed; both clusters' rows are asserted together to pin that the clause still
matches the one the exact list named, and, for the replica half, that widening the
later rule does not take the parameter away from the narrower scoped-r1 rule
ahead of it. It still asserts environment-wide keys keep
working beside the scoped sections, that a cluster created while a segment already
matches it gets its overrides folded into the create transaction, and that removing
the sections prunes every row. Its environment-wide baseline stays pinned as flat
keys in the file, which is what every scoped expectation is derived from, rather
than inherited from the parameters' compiled-in defaults, which the harness's
MZ_SYSTEM_PARAMETER_DEFAULT inverts.

Ran locally

• bin/fmt: ruff, ruff-dbt, black and rustfmt all pass, nothing rewritten.
  buf is not installed here and reports failed; no .proto file is touched.

• MZDEV_NO_SHELLCHECK=1 bin/lint: every check that can run passes, including
  check-copyright.sh, check-whitespace.sh, check-rust-test-attributes.sh,
  check-test-flags.sh, check-python-discouraged.sh, check-mzcompose-files.sh,
  check-python-files.sh and check-formatting.sh apart from buf.

• rustfmt --check on both changed Rust files, python3 -m py_compile on the
  changed Python, and
  bin/pyactivate -m materialize.cli.mzcompose --find dyncfg list-workflows to
  confirm the composition still loads.

• The clause logic, actually run. cargo cannot build this workspace here (see
  below), so ContextKind, ScopeAttribute, Segment, Clause, Operator,
  unsupported_operator, both defect enums with their Display impls,
  Clause::parse, Segment::parse, ConfigFile::parse, Rule::parse and the
  helpers were extracted verbatim, by line range out of the committed file into a
  scratch lib crate depending only on serde_json and regex, together with a
  stubbed copy of the rule-resolution fold and of a SynchronizedParameters that
  pins the same environment-wide baseline the in-repo expectations use. 45 tests
  pass there and cargo clippy --all-targets -- -D warnings is clean on it,
  under a copy of the workspace lint table so that is meaningful. 25 of those
  tests are this PR's own mod tests bodies, byte for byte, every config-file
  test in the diff, lifted in with only #[mz_ore::test] rewritten to #[test],
  so the assertions that run there are the assertions CI runs. The remaining 20
  are extra coverage written against the extraction: the operators and anchoring,
  negate after the OR, ANDed clauses, contextKind selecting the pass and never
  crossing kinds, every fail-closed path, and the exact rendered text of every
  diagnostic. They also parse the test/dyncfg segments and assert the properties
  that file's SQL assertions rest on, since the composition cannot run here.

  This is how the one genuine compile error in the change was caught:
  serde_json::Map has no into_keys, which I had used in two places.
  What the extraction does not cover is the surrounding frontend and the in-repo
  test module, which remain uncompiled.

• A script that extracts all 44 JSON literals from the new unit tests, applies
  format!'s brace rules and the substitutions, and parses each one, plus the same
  over every json fence and embedded system-params.json in the docs page (10)
  and every literal dict in the mzcompose file (16). A brace-escaping typo in a
  raw format string would otherwise present as a mysterious test failure.

Could not run locally, so CI must cover it

• cargo check / cargo clippy, at any scope including a single crate. This
  environment's egress policy refuses two third-party git submodules that Cargo
  resolves before it will build anything, duckdb/duckdb and
  confluentinc/librdkafka, and there is no cached registry or target directory
  to fall back on. The Rust in this PR was never compiled as part of this
  workspace locally, only in CI, though the extracted scratch crate above
  compiles and runs the predicate and resolution code verbatim. I re-read it by hand for imports,
  lifetimes, borrow scopes and trait bounds, and in particular avoided a
  let ... else that would have moved the value it then needs to name in the
  warning, and wrote the scopable_params fold as a plain loop rather than a
  nested-closure flat_map over &self, but the compiler has not confirmed any
  of it.
• The new Rust unit tests, for the same reason. Two of them assert that a
  parameter's environment-wide default is off so that a future default flip
  fails there rather than quietly weakening the test.
• test/dyncfg, which needs a container build.
• Several bin/lint checks fail here for missing tooling only, unrelated to this
  change: check-cargo.sh (no cargo-deny / cargo-deplint),
  check-helm-docs.sh, check-zizmor.sh, check-trufflehog.sh,
  check-protobuf.sh, check-generation.sh (needs cargo), and
  check-python-docs.sh, whose 37 errors are all cargo build -p mz-sql-anonymize failing in mz_workload_anonymize_test.py, present on the
  base commit too.

Deliberately not in this PR

• Only the five string operators. The numeric, date, semantic-version and
  segmentMatch operators are recognised and refused rather than implemented,
  since every scope attribute is a string. An unknown operator makes its segment
  match nothing rather than being misread, so supporting one later is additive and
  safe for an older binary reading a newer file.
• No variations, off value, fallthrough, prerequisites, rollouts or percentage
  bucketing. The clause vocabulary is adopted; the rest of LaunchDarkly's flag
  model is not. rules stays this file's own ordered first-match-wins array.
• The LaunchDarkly evaluation path is untouched. This adds an operator to the
  file format only; LaunchDarkly does its own matching.
• No percentage rollouts or bucketing. Attributes only, per the request.
• No new scope-visibility surface. There is no way to ask, from SQL, which
  parameters accept a scoped value, so an operator putting an environment-only
  parameter in a rule gets a log warning and nothing more. Exposing
  ParameterScope in SHOW ALL or mz_internal would be a genuine improvement
  but is a separate change.
• Unmanaged replicas are still skipped. build_replica_eval_contexts only
  yields managed replicas, since LaunchDarkly targeting needs a size and size
  family. The file path inherits that, so a segment matching an unmanaged replica
  has no effect. Pre-existing, and self-managed replicas are managed.
• The test/orchestratord SystemParamConfigMap modification, a real
  Kubernetes-cluster test, was left alone: extending its combinatorial matrix to
  create a Materialize cluster and assert scoped rows is much heavier than the
  test/dyncfg route and would duplicate that coverage.

[kay-kim]

good from the docs side mod 2 nits.

[kay-kim]

Question for the SM team (and I realize there's been an ongoing discussion since almost the beginning about how we want to curate these parameters): We don't currently provide the scope information. So, would people need to just try it out?

[claude]

Confirmed: today nothing surfaces a parameter's scope to an operator. SHOW ALL returns only name/setting/description, and mz_internal.mz_cluster_system_parameters / mz_replica_system_parameters are (cluster_id|replica_id, name, value), so they show overrides that already exist rather than which parameters accept scoping. ParameterScope lives only in the Rust definitions.

The failure mode if someone guesses wrong is quiet. file_section_overrides iterates the parameters whose declared scope is cluster/replica and looks each one up in the section, so a key in a scoped section for a parameter that is not scoped (or a misspelled name) is parsed and then simply never consulted. No error, no warning. Only an unparseable value for a genuinely scoped parameter gets a warning.

The fix I would suggest as a follow-up is exposing the scope in introspection, e.g. a scope column so an operator can query which parameters accept scoping instead of experimenting. That is not in this PR.

The curation-policy half of your question is the SM team's call, so I will leave that to them.

---

Generated by Claude Code

[kay-kim]

maybe an intro to the example:

For example, the following configuration sets environment-wide parameters and overrides parameters for a specific cluster and replica:

[kay-kim]

need a comma after "In this example"

[claude]

Thanks, all five acted on in 48cec17. Replying here rather than per-thread since points 2 and 3 share one fix.

1. Failure semantics. I went with "no information", not prune-on-failure. ConfigFile::parse now returns Option, read is gone (the cache does the reading), and sync_scoped_params returns early when frontend.has_scoped_desired_state() is false, so the reconcile is skipped wholesale rather than sent an empty desired state. Reason: the desired state is complete, so "empty" is a real instruction and a typo is not. Prune-on-failure would make an unparseable ConfigMap, or a deleted one, revert half the file's effect durably and restore it on the next fix, which is worse than doing nothing until a readable file says otherwise. Environment-wide behaviour is unchanged: a failed read still yields no keys, and every parameter falls back to its current value. Per-section leniency is untouched, as you said, only the whole-document failure short-circuits. New test test_read_failure_keeps_scoped_overrides covers both failure shapes and asserts that a valid {} still prunes.

2 and 3. Cache. The parsed file lives on the frontend now (Mutex<Option<CachedConfigFile>>), refreshed by pull and read by the scoped passes, so the file arms of pull_cluster_overrides / pull_replica_overrides do no I/O and scoped_overrides_create_op no longer reads from the coordinator loop. Three reads per tick became one, and the NOTE about a mixed view is gone rather than merely documented. The cache keys on the raw contents, so unchanged contents skip both the re-parse and every warning: a standing mistake warns once per edit instead of at 1 Hz. I checked your claim about the create path before relying on it and it holds: scoped_overrides_create_op returns None when nothing applies, its prune_scope is bounded to the objects being created, and reconcile_scoped_system_parameters treats a failure as best-effort with the sync loop as the authoritative full-state reconciler, so a value up to one tick old only delays a new object's override to the next tick.

4. Unscopable keys. scoped_section_diagnostics now warns on a section key that is not a parameter scopable at that position, e.g. ignoring enable_lgalloc for cluster "analytics" in the system parameter sync file: not a cluster-scoped system parameter, which also catches a misspelling. It returns the messages so the refresh can log them once per change and so a test can assert them (test_diagnoses_unscopable_section_keys). The unparseable-value warning moved there too, which makes file_section_overrides a silent resolver matching the LaunchDarkly path. The docs bullet on non-scopable parameters now says a warning is logged, which is a partial answer to @kay-kim's question on scope discoverability; the introspection column is still a follow-up.

5. Create-time coverage. test/dyncfg keeps the existing cluster created while the file is flat, and adds dyncfg_scoped_2 created after a section naming it, asserted without a sleep. One caveat I'd rather state than paper over: that composition runs the sync loop at 100ms, so a tick can land between the CREATE and the SELECT. What the assertion pins down is that the create path resolves and commits the overrides, not their ordering against the replica's first configuration; proving the ordering needs controller-level or log inspection, which this harness has no hook for.

6. Nit. as_sections and its closure are gone. Shape warnings now format a FilePosition inside the warn!, so the happy path allocates nothing per live object, and the nested arm names the right thing: an entry inside replicas reports the replicas entry for cluster "prod" or replica "prod"."r1" instead of a cluster-flavoured context.

On the praise: noted, thanks. The three-way enum is what made point 4 cheap to add, since the file path only needed a different consumer of Unparseable.

Same verification caveat as the PR body, made worse by the size of this change: bin/fmt, MZDEV_NO_SHELLCHECK=1 bin/lint (modulo the tooling that isn't installed here), rustfmt --check and py_compile pass, but cargo check / cargo clippy still cannot run in this environment, so none of this Rust has been compiled and none of the new unit tests have been run. I re-read it for borrows, lifetimes and the Option<Arc<..>> plumbing by hand. If you have a build handy, a cargo test -p mz-adapter config::frontend would be worth more than another read from me.

---

Generated by Claude Code

[jubrad]

Awesome! A few things as I was going through the code with claude, nothing major.

Summary

4 considerations
1 nit
1 praise

[jubrad]

consider — The contract stated here is only true for the environment-wide pass. pull maps a missing key back to params.get(param_name), so an empty ConfigFile really does leave every environment-wide value alone. The scoped passes have the opposite meaning: an empty ConfigFile is a complete desired state of "no overrides", and sync_scoped_params prunes accordingly across every live object.

So if an operator applies a ConfigMap with a JSON syntax error (or the ConfigMap is deleted, since the volume is mounted optional: true), the next tick keeps all environment-wide values but durably deletes every cluster- and replica-scoped override, pushing e.g. enable_lgalloc back to on on every replica via the compute controller, then restores them all when the file is fixed. Half the file's effect silently reverts on a typo while the other half stands.

Either make the failure path express "no information" rather than "no overrides" (e.g. read returns Option<ConfigFile> and sync_scoped_params skips update_scoped_system_parameters when the read or parse failed; the per-section leniency below is fine, it's only the whole-document failure that has this reach), or, if prune-on-failure is the intent, say so here instead of claiming every value is left alone.

[jubrad]

consider — This warning is unconditional and the pass runs every tick. generation.rs passes --config-sync-loop-interval=1s for every self-managed deployment, so one unparseable entry emits ~86k identical lines per day, per (object, parameter), until someone edits the ConfigMap. The comment two arms up on the LaunchDarkly path rejects a warning for exactly this reason; the operator-authored argument justifies telling them once, not at 1Hz forever. The as_object warnings in parse have the same property.

The flip side is that the mistake an operator is most likely to make gets no message at all: file_section_overrides only looks up names in param_names, so a misspelled parameter, or a replica-scoped parameter placed in a clusters section, is silently dropped. Since there's no way to see a parameter's scope from SQL, there's nothing to debug against.

Both fall out of one change: keep the previously-parsed ConfigFile and emit scoped-file diagnostics only when it changes from the prior tick. That makes the warning affordable, and lets you also warn on section keys not in param_names ("ignoring enable_lgalloc for cluster "analytics": not a cluster-scoped parameter"), which is the signal an operator can actually act on.

[jubrad]

consider — scoped_overrides_create_op runs on the coordinator task (sequence_create_cluster, cluster.rs:1427) and calls both pull_cluster_overrides and pull_replica_overrides, so every CREATE CLUSTER under a file frontend now performs two blocking fs::read_to_string calls inline on the loop that serializes all DDL and query sequencing. Previously the file client returned instantly there. It's a tmpfs-projected ConfigMap so it's normally microseconds, but synchronous I/O on the coordinator loop isn't a pattern this codebase otherwise has.

Caching the parsed ConfigFile on the frontend (refreshed by pull, read under a lock by the scoped passes) would remove the coordinator-loop I/O, cut the three reads per tick to one, and make the NOTE on read about a tick observing a mixed view unnecessary rather than merely documented. The create path is best-effort and re-reconciled every tick, so a value up to one tick old is fine there.

[jubrad]

consider — The cluster is deliberately created while the file is still flat, which makes the "no scoped rows yet" assertion meaningful. Good. But it means the create-time path is never exercised for the file client, and that's the newly-reachable path with the most machinery behind it: scoped_overrides_create_op folding the override into the create transaction so a new replica's first controller configuration carries it (the reason that fold exists, per the comment at cluster.rs:1417). A CREATE CLUSTER dyncfg_scoped_2 after system_params_3 is written, plus a section naming it, would cover it in a few lines and would also catch a regression in the create-path plumbing that the per-tick reconcile would otherwise paper over.

[jubrad]

nit — as_sections takes a &str context plus an impl Fn(&str) -> String purely to build warning strings, and the call sites format eagerly: file_cluster_overrides/file_replica_overrides allocate a format!("cluster {name:?}") for every live object on every tick even when nothing warns. Passing the name pieces through and formatting inside the warn! would drop the closure parameter and the happy-path allocations.

Minor related wrinkle: the nested arm at line 155 passes format!("cluster {cluster:?}") as the context for entries inside replicas, so a malformed entry there warns about "cluster "prod"" and points an operator at the wrong section.

[claude]

Review

One thing needed fixing before this merges, and it was in the test rather than the shipped code: the new scoped assertions in test/dyncfg/mzcompose.py asserted the opposite of what the resolver does under the mzcompose harness. The Rust looks sound; the semantics I probed match what the docs and doc comments claim, including the fail-closed segment handling, the skip-on-no-information guard, and the caching rewrite.

The new dyncfg scoped assertions are inverted and will red the nightly

The test reasons from the compiled-in defaults of the two parameters it picked, as its own header comment says. But the harness overrides both: test/dyncfg runs a plain Materialized(), which renders MZ_SYSTEM_PARAMETER_DEFAULT from get_default_system_parameters, setting enable_eager_delta_joins=true and enable_lgalloc=false. Neither name appears in get_variable_system_parameters, so the environment-wide values inside the test are the inverse of what the assertions assume.

That matters because resolution records an override only when it differs from the environment-wide value, and that baseline comes from get_system_vars(), i.e. effective values including the CLI defaults. So under the harness the cluster row for enable_eager_delta_joins true never exists, r2 gets no enable_lgalloc row while r1 gets one, which is exactly inverted, and for dyncfg_scoped_2 both values equal their bases so scoped_overrides_create_op returns None and zero rows exist. The create-path coverage added in the previous round therefore asserts nothing reachable, and the first-match-wins check, the reason the second replica was added, tests the opposite of the intent.

dyncfg runs only in the nightly pipeline, never in the PR pipeline, so the green PR run never executed any of it. Merging as-is would turn the nightly dyncfg step red deterministically.

What I changed

Everything above describes 7911049d. Fixed in a3ab5cc, test only, no Rust change, since the resolver's behaviour is the correct one and it was the test's premise that was wrong.

test/dyncfg/mzcompose.py now pins the environment-wide baseline it reasons from, as flat top-level keys in the config file it writes, instead of inheriting whatever the harness passes: enable_eager_delta_joins to false and enable_lgalloc to true, in the same flat section as max_connections, present in every version of the file the test writes. Flat keys are applied to the sync loop's parameter set in the same pull that refreshes the file cache, before sync_scoped_params runs, and the create path resolves from the catalog's system config, so the reconcile and the create-time fold both see the intended baseline. enable_join_prioritize_arranged, the parameter whose leak the rules that must have no effect are checked against, is pinned to false for the same reason, so those negative assertions rest on a stated baseline rather than a compiled-in default a future harness change could invert.

Every expected row was then re-derived from that baseline, and they come out as the rows the test already asserted: the cluster's enable_eager_delta_joins true and r2's enable_lgalloc false are now genuine overrides, r1 keeps the environment-wide value through first-match-wins, and both dyncfg_scoped_2 create-path rows are reachable. The header comment that stated the wrong premise is rewritten, and the rule comments now say which value differs from the baseline and which repeats it.

Two additions while there. First, the baseline itself is asserted once it has synced, read as mz_system since none of these parameters is user-visible, so a future harness or resolver change surfaces as the premise it breaks rather than as an unexplained row mismatch. Second, first-match-wins is now stated per replica rather than by the absence of a row: a LEFT JOIN over both replicas of dyncfg_scoped asserts r1 env-wide and r2 false, so reversing the two rules makes scoped-cluster decide both replicas, turns r1 into a false row, and fails the assertion.

Coverage

The resolution semantics themselves are covered by the unit tests in src/adapter/src/config/frontend.rs, which drive the file frontend against a SynchronizedParameters built from compiled-in defaults and so are unaffected by the harness: segment matching, the AND/OR and exact-match rules, the coherence guard, fail-closed handling of an uninterpretable predicate entry, rule ordering, the differs-from-environment and unparseable classifications, and the diagnostics. test/dyncfg is the only end-to-end coverage of the file path, and it runs in the nightly pipeline only, never on a PR.

I could not execute test/dyncfg here: it needs Docker images and a full build, and this environment has no Docker daemon and cannot resolve two Cargo git submodules. The corrected assertions are derived by reading the resolver, the sync loop and the create-time fold, not observed running. bin/fmt (ruff, ruff-dbt, black, rustfmt), python3 -m py_compile on the changed file, bin/mzcompose --find dyncfg list-workflows to confirm the composition loads, and MZDEV_NO_SHELLCHECK=1 bin/lint's check-python-files.sh all pass; the remaining bin/lint checks fail identically on the tree without this change, for missing tooling and blocked egress. So the first nightly after this merges is what actually confirms the fix, and it is worth watching.

Merge decision: merge. The shipped code needs no change, and the one blocking issue is fixed on this branch.

---

Generated by Claude Code

[def-]

QA LLM review:

1. MEDIUM -- Publish scoped rules only with their environment-wide baseline

src/adapter/src/config/frontend.rs:1164

refresh_config_file publishes the newly parsed rules to the coordinator before the same file's top-level parameter changes are pushed to the catalog. A concurrent cluster or replica creation can therefore classify scoped values against an old or partially updated environment-wide baseline and omit an override that its first configuration requires.

SystemParameterFrontend::pull refreshes this shared cache first, then system_parameter_sync awaits backend.push, which updates modified parameters one at a time, before starting the scoped reconcile. Meanwhile scoped_overrides_create_op reads the new cache but constructs SynchronizedParameters from catalog.system_config(). For example, if the file changes enable_lgalloc from off to on while a matching rule keeps it off, a create in this window sees off as both the base and rule value and records no replica override. The subsequent environment-wide push changes the replica to on; only the later reconcile restores off. The create-time fold exists specifically because that later repair is too late for render-frozen settings. Publish the parsed file only after its baseline is committed, or cache and evaluate an atomic file-plus-baseline snapshot.

2. MEDIUM -- Keep the last valid rules available to create-time evaluation

src/adapter/src/config/frontend.rs:1155

On any read or whole-document parse failure, the cache replaces the last valid ConfigFile with None. Existing objects retain their durable overrides because has_scoped_desired_state skips reconciliation, but objects created during the failure receive no matching override at all, so preserved policy becomes inconsistent across otherwise identical replicas.

Both scoped pull methods return an empty map when cached_config_file() is None, and all four cluster create/recreate paths reach them through scoped_overrides_create_op. If a ConfigMap temporarily disappears or contains partial JSON while a matching replica is created, that replica starts at the environment-wide value and remains there until the file recovers. For a render-frozen parameter, the eventual reconciliation cannot repair work rendered under the wrong initial value. Keep a separate last-known-valid parse for create-time evaluation while retaining a current-read-valid bit to prevent the periodic reconciler from pruning on failure.

3. MEDIUM -- The create-time test can be satisfied by periodic reconciliation

test/dyncfg/mzcompose.py:336

The purported create-time assertion only queries durable override rows after CREATE CLUSTER. The independent config-sync loop runs every 100 ms and can populate those rows after creation but before the following SELECT, so the test can pass even if the create transaction stops folding scoped overrides into the replica's first configuration.

The test's own comment acknowledges the 100 ms reconciler, but omitting an explicit sleep does not exclude it. CREATE CLUSTER includes a durable catalog transaction, and a sync snapshot/update queued while that command runs can be processed before testdrive sends or the coordinator handles the next query. Removing scoped_overrides_create_op would therefore still permit the expected rows to appear, while render-frozen settings would already have been missed. Disable or block the periodic reconcile across this assertion, or assert an observable value captured from the replica's first controller configuration rather than eventual catalog state.

[def-]

QA LLM Review

1. MEDIUM -- Parking the sync loop at 1h leaves testdrive's ALTER SYSTEM RESET ALL permanently in force, so the create-time fold resolves against the wrong baseline

test/dyncfg/mzcompose.py:341

Testdrive() in this composition does not pass no_reset, so every c.testdrive() call runs ALTER SYSTEM RESET ALL before the script executes. Until now the 100ms sync loop undid that within testdrive's retry window, which is why > SHOW max_connections 67 passes at all. With the interval set to 1h nothing undoes it, so the CREATE CLUSTER dyncfg_scoped_2 in that block resolves its overrides against the --system-parameter-default values rather than ENV_WIDE_PARAMS, records nothing, and the dyncfg_scoped_2 rows the block asserts are absent.

Details

Concretely: get_default_system_parameters ships enable_eager_delta_joins=true and enable_lgalloc=false (misc/python/materialize/mzcompose/__init__.py:95,101), while ENV_WIDE_PARAMS pins them to false / true through the file's flat keys. ALTER SYSTEM RESET ALL clears only the environment-wide collection (clear_system_configs deletes system_configurations; cluster_system_configurations and replica_system_configurations are untouched), and reset_durable() restores each var to its --system-parameter-default. So immediately after the reset the baseline is enable_eager_delta_joins = on, enable_lgalloc = off.

scoped_overrides_create_op builds its baseline from catalog.system_config() (src/adapter/src/coord.rs:2292), and classify_scoped_value compares canonically. The scoped-cluster rule value true canonicalizes to on, equal to the reset baseline, so MatchesEnvironment; the scoped-replicas rule value false equals off, likewise. evaluated.is_empty() is then true and the op is None, so dyncfg_scoped_2 enable_eager_delta_joins true and dyncfg_scoped_2 r1 enable_lgalloc false never appear. dyncfg_scoped's rows survive (durable, and re-established by the restarted loop's first tick), so the failure is localised to exactly the rows the commit set out to isolate.

Note also that assert_environment_wide_baseline(c) at line 351 runs before the testdrive block, so it confirms the premise at the one moment it holds and the reset invalidates it a moment later. This composition runs in nightly only, so PR CI will not report it.

The isolation idea is right, it just needs the baseline to survive the reset. Options: give this block its own Testdrive(no_reset=True) service (or run the create through c.sql_query/postgres-execute on a session outside testdrive), or re-assert the flat keys with ALTER SYSTEM SET right after the reset and before the CREATE CLUSTER. If neither is convenient, picking scoped parameters whose rule values differ from their --system-parameter-default (not just from ENV_WIDE_PARAMS) would make the block robust to a reset, at the cost of no longer testing the differs-from-environment classification against a file-supplied baseline.

[claude]

@def- thanks — all four findings held up against the code, and all four are fixed. One consolidated reply rather than one per finding.

1 & 2 (stale baseline on create; no override while the file is unreadable) — 801ab41e. These turned out to be one root cause: the create path was reading a cache whose freshness it could not reason about. Fixed with a single split rather than two patches, per your suggestion of an atomic file-plus-baseline snapshot and a separate last-known-valid parse.

CachedConfigFile now holds current (this tick's read) and published (the newest read whose environment-wide section is committed). refresh_config_file sets current and returns it for the environment-wide pass, leaving published alone; the sync loop calls the new publish_config_file after backend.push and before the scoped reconcile, which advances it. Both scoped pull methods resolve against published, so a file's rules and the baseline they are judged against are always the same file's — your enable_lgalloc example no longer classifies off against a not-yet-pushed off.

The same promotion only advances on a valid parse, which is finding 2: the last valid parse stays available to create-time evaluation across a failed read or a half-written ConfigMap. has_scoped_desired_state now reads current, so a bad read still holds the prune back — the two questions are deliberately answered from the two halves of the cache, and that is spelled out in the docs on published_config_file.

Both windows have regression tests that fail without the split: test_rules_are_published_with_their_baseline (new) and an extended test_read_failure_keeps_scoped_overrides, which previously asserted the empty resolution you identified as the bug and now asserts the last valid overrides instead.

3 (create-time test satisfiable by the reconciler) — df739473. You were right that the comment conceded the problem rather than fixing it. config_sync_loop_interval is a startup argument, so it is now settable on the Materialized service (still 100ms by default) and the block restarts materialized with it at an hour. The loop's first tick fires immediately — that is what installs the shared frontend the fold needs and what reconciles the pre-existing objects — and the next is an hour away, so the dyncfg_scoped_2 rows can only come from its own create transaction. If that first tick has not run the rows are absent and it fails loudly rather than flakily. I did not go for asserting the replica's first controller configuration; the cheaper option looked sufficient.

4 (Testdrive without no_reset) — 86326225. Confirmed, and you were right that the fix introduced the failure it was meant to prevent. Verified the chain: Testdrive() here does not pass no_reset, testdrive's reset_materialize runs ALTER SYSTEM RESET ALL (src/testdrive/src/action.rs:579), get_default_system_parameters ships enable_eager_delta_joins=true and enable_lgalloc=false — exactly the two rule values — and scoped_overrides_create_op takes its baseline from catalog.system_config(), so both would classify MatchesEnvironment and the op would be None. Parking the interval removed the 100ms repair that had been hiding this.

Fixed with Testdrive(no_reset=True) for that block, which is the option that keeps the assertion about a file-supplied baseline. The block also now asserts SHOW max_connections ahead of the CREATE CLUSTER, so it fails at its premise rather than at its conclusion if the harness shifts again, and the comment states the trap explicitly.

Caveats worth knowing. test/dyncfg is nightly-only, so PR CI never exercises findings 3 or 4 — those two are reasoned through and lint-clean, not observed green. The Rust changes are covered by the unit tests above.

On the red CI: merge-skew-cargo-check is failing with Failed (exit status 101), which is base drift, not this change. The branch itself compiles — clippy (--all-targets -D warnings), rust-build-x86-64, rust-build-aarch64 and rust-macos-clippy were all green on the same tree. That check compiles the PR merged against current main, and this branch's base is six days old. origin/main has been merged in once (978704d8) and it is still red; rather than keep merging up on an unreviewed PR, we are leaving it to sit until a human reviews. A separate doctests failure was real and is fixed in db39b1e7 — rustdoc::private_intra_doc_links fires under -D warnings when a public item's docs link a private one, even with --document-private-items.

---

Generated by Claude Code

[antiguru]

@claude please rebase this PR.

[claude]

Claude finished @antiguru's task in 1m 53s —— View job

---

Rebasing PR

• Gather context
• Fetch latest origin/main
• Bring branch up to date with origin/main
• Push updated branch

Done. The branch couldn't be cleanly rebased (it contains prior merge commits from main, causing the rebase to conflict at the repository's root commit with 37,746 commits to replay). Merged origin/main instead, which brings the branch up to date with the same effect. 18 files changed from main were incorporated cleanly with no conflicts.
| Branch

[jubrad]

Summary

2 praise

Other

praise — src/adapter/src/config/frontend.rs — Round two: all six of my findings are addressed, and I'm satisfied on each.

1 (failure semantics) went the way I'd have chosen, and the reasoning for "no information" over prune-on-failure is now in the code rather than in the PR thread. 2 (warning spam) and 3 (coordinator-loop I/O) fell out of the cache, and keying it on the raw contents turns a 1 Hz warning into one per edit. The unscopable-key diagnostic is the part an operator can actually act on, and returning the messages so a test can assert them is better than what I asked for. 4 (create-time coverage) is covered, and the block's premise is now asserted ahead of the CREATE rather than inferred from its result. 5 (as_sections) is gone.

I ran what your environment could not, at 6f20d2b: cargo check -p mz-adapter --all-targets and cargo clippy -p mz-adapter --all-targets are both clean, and all 33 config::frontend unit tests pass, including test_read_failure_keeps_scoped_overrides, test_rules_are_published_with_their_baseline and test_config_file_cached_until_it_changes. So the Rust in this PR does compile and its tests do pass. I did not run test/dyncfg (nightly-only, no mzcompose run here), so that half stays reasoned rather than observed.

written by claude on behalf of @jubrad

[jubrad]

praise — Splitting the cache into current and published is a better answer than either of the two findings that led here asked for: one mechanism decides both "may we prune?" (the current read) and "which rules does a create see?" (the last read whose baseline is committed), and the doc comment says why the two questions get different halves. Worth keeping the publish_config_file call site in sync.rs next to the push it depends on, since the invariant is an ordering that nothing in the type system enforces.

written by claude on behalf of @jubrad