Skip to content

A proxy group needs a name on the raft path too - #305

Merged
bjmeetsfo merged 3 commits into
mainfrom
oss/the-raft-path-checks-a-proxy-group-name
Aug 25, 2026
Merged

A proxy group needs a name on the raft path too#305
bjmeetsfo merged 3 commits into
mainfrom
oss/the-raft-path-checks-a-proxy-group-name

Conversation

@bjmeetsfo

Copy link
Copy Markdown
Collaborator

put_proxy_group refuses a group that names neither itself nor a namespace.
That check lived only in the public method, and the propose path dispatches
straight to apply_put_proxy_group, which does not check.

Measured

The same request, both backends:

single-node -> ok=false code=bad_request groups=0
raft        -> ok=true  code=ok          groups=1
               group name="" namespace="" location="rack-1"

A nameless group is committed into replicated metadata by the raft path and
refused outright by the single-node one.

Why it does not simply sit there harmlessly

The group name is the key it is stored under, and an empty name is also the
value an unattached proxy carries. So the group is indexed by "no group at
all", and calibration counts every idle proxy as already belonging to it. I
expected that to churn — plan an attach every round, forever — and measured it
instead: three rounds, attach=[] detach=[] shortfalls=0. It does not churn.
It does something quieter: the phantom group reads as permanently satisfied by
proxies that are actually idle, so nothing ever reports it and nothing ever
cleans it up. It then survives snapshot and replay, because by that point it is
legitimately committed history.

The change

Judge it in admission_refusal, alongside the other pre-propose checks, and
let the public method go through that same judgement so the two cannot drift
apart again. Before proposing and never while applying — replay has to reapply
what was already accepted, which is why this does not go in apply_*.

The check takes no lock: it judges the request alone, not the state around it.

Test

The raft path now answers bad_request and stores nothing, matching the
single-node path, and a properly named group still goes through so the guard
is not simply refusing everything. It fails before the change with the raft path accepted a proxy group with no name.

Full meta:: (290) and raft:: (219) suites pass.

Base

Stacked on #231, which introduced admission_refusal. Merge #231 first.

It targets main rather than #231's branch on purpose: rust-ci and
oss-readiness declare pull_request: branches: [main, rust-main], and for
pull_request that filters the base. A pull request aimed anywhere else is
never compiled and its tests never run, while still reporting CLEAN.
Targeting main means this diff also carries #231's commit and CI builds the
two together, which is the state that will land; once #231 merges this reduces
to its own commit.

The mute is the incident lever: while it is set the metaserver is meant to
refuse every recorded metadata mutation, so an operator can stop it making
changes mid-incident without a restart.

On a raft-backed metaserver it did nothing at all. The check lived only in
SingleNodeMeta's public methods, and MetaRaftCluster proposes straight past
them -- neither the propose path nor the apply path consulted the mute, and
raft/cluster_meta.rs did not mention it anywhere. Set it, and every mutation
still went through, on the deployment mode a real cluster runs.

Gated in mutation_status, the single funnel for raft mutations. Checked
before proposing rather than while applying: replay has to reapply what was
already accepted, including changes recorded before the mute was set.

The check does not clone the cluster's metadata. read_meta() deep-copies the
whole state to answer one question, which on the mutation path would make
every write pay for a full copy; peek_meta_change_muted reuses the same
replica selection and reads one bool. A cluster no replica can answer for is
left to propose and fail on its own terms, because refusing there would turn
"cannot tell" into "muted".

What stays permitted while muted lives on MetaMutation, so the rule cannot
drift between the backend that checks it and the one that does not: the
lever itself, or muting would be a one-way door, and the retention purge,
which the single-node path has always allowed.
apply_mutation dispatches straight to the apply_ functions, so every guard
that lives in a public method is skipped when the raft backend proposes.
The mute was one instance; it was not the only one.

Probing the raft path against each guard the single-node path applies found
two that cause real harm:

A reserved name could be taken. Reserved names exist to hold a name back
from creation, and on a raft-backed metaserver the reservation held nothing
back at all.

A namespace could be dropped out from under a live table. The single-node
path refuses that precisely so a drop cannot strand tables in a namespace
that no longer exists; the raft path dropped it and left the table behind.

The guards move into admission_refusal, expressed once against state either
backend can consult, and the single-node path now defers to it as well so
the two cannot drift apart again -- drift is what caused this.

Judged before proposing and never while applying: replay has to reapply what
was already accepted, and a name reserved today must not invalidate a
namespace legitimately created before it.

peek_meta_change_muted generalises into with_readable_meta so the admission
check also answers without copying the cluster's metadata on every write.

One difference is left alone: setting a namespace to the state it already
has succeeds here and is refused on the single-node path. It causes no harm
beyond a no-op being accepted, and turning a success into an error is
wire-visible for anything already calling it.
put_proxy_group refuses a group that names neither itself nor a namespace.
That check lived only in the public method, and the propose path dispatches
straight to apply_put_proxy_group, which does not check. So a raft-backed
metaserver committed a nameless group into replicated metadata where a
single-node one answered bad_request:

    single-node -> ok=false code=bad_request groups=0
    raft        -> ok=true  code=ok          groups=1
                   group name="" namespace="" location="rack-1"

The group name is the key it is stored under, and an empty one is also the
value an unattached proxy carries. So the group is indexed by "no group at
all", and calibration reads every idle proxy as already belonging to it --
it looks permanently satisfied and is never cleaned up. It survives
snapshot and replay, because by then it is legitimately committed history.

Judge it in admission_refusal with the other pre-propose checks, and let
the public method go through the same judgement so the two cannot drift
apart again. Before proposing and never while applying: replay has to
reapply what was already accepted.

Test: the raft path now answers bad_request and stores nothing, matching
the single-node path, while a properly named group still goes through.
@bjmeetsfo
bjmeetsfo requested a review from superhaiou as a code owner August 25, 2026 08:21
@bjmeetsfo
bjmeetsfo merged commit 7bad85d into main Aug 25, 2026
7 of 8 checks passed
@bjmeetsfo
bjmeetsfo deleted the oss/the-raft-path-checks-a-proxy-group-name branch August 25, 2026 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants