Skip to content

Apply the guards the raft propose path was skipping - #231

Merged
bjmeetsfo merged 2 commits into
mainfrom
oss/guards-the-raft-path-skipped
Aug 25, 2026
Merged

Apply the guards the raft propose path was skipping#231
bjmeetsfo merged 2 commits into
mainfrom
oss/guards-the-raft-path-skipped

Conversation

@bjmeetsfo

@bjmeetsfo bjmeetsfo commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #230. It fixes the same funnel (mutation_status) and reuses the
replica-peek added there. Basing it on main would only guarantee a conflict.
If #230 is rejected, this goes with it.

The problem

apply_mutation dispatches straight to the apply_* functions:

MetaMutation::AddNamespace(request) => self.apply_add_namespace(request).status,

Every guard that lives in a public method is therefore skipped when the raft
backend proposes. #230 found this for the mute. It is not the only one.

I probed the raft path against each guard the single-node path applies:

Guard Raft path
namespace does not exist refused correctly (the check is in apply_)
name is reserved allowed — the reservation held nothing back
namespace still holds a live table allowed — the table was stranded
state is unchanged (not_modified) allowed (see below)

Two of these cause real harm:

  • A reserved name could be taken. Reserved names exist to hold a name back
    from creation; on a raft-backed metaserver the reservation did nothing.
  • A namespace could be dropped out from under a live table. The single-node
    path refuses this precisely so a drop cannot strand tables in a namespace
    that no longer exists. The raft path dropped it and left the table behind.

Both tests fail on the unmodified path — the first with "a reserved namespace was
created anyway", the second with "a namespace was dropped out from under a live
table".

The change

The guards move into admission_refusal(&MetaMutation) — expressed once,
against state either backend can consult — and the single-node path now defers
to it too
, 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 from #230 generalises into with_readable_meta, so the
admission check also answers without cloning the cluster's metadata on every
write.

What I did not change

not_modified — setting a namespace to the state it is already in succeeds on
the raft path and is refused on the single-node path. It causes no harm beyond a
no-op change being accepted, and turning a success into an error is a
wire-visible behaviour change for anything already calling it. That is your call,
not mine; the guard is one line away in admission_refusal if you want it.

Verification

  • cargo check --all-targets — 0 errors
  • cargo test --bin metaserver -- --test-threads=1
  • cargo test --lib -- --test-threads=1

The data_node::…jitter_backoff… and engine::…recovery_validates_… failures
reproduce on an unmodified tree at this base and are untouched here.


Correction to the verification note above

I described two failures as reproducing on an unmodified tree. Re-checked on
a quiet machine with disk headroom, run in isolation against unmodified main:

test unmodified main
data_node::…jitter_backoff… fails 3/3 — genuinely pre-existing, deterministic
engine::…recovery_validates_all_timestamped_kv_page_families passes 3/3

Only the first is pre-existing. My original evidence for the second came from a
run taken immediately after the disk hit 100%, so it was environmental — that
test is sensitive to disk pressure and load, not broken on main.

The conclusion this change is not responsible for either failure is unchanged.
The evidence offered for half of it was wrong, and the record should say so.

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.
@bjmeetsfo

Copy link
Copy Markdown
Collaborator Author

Merge-order note. This PR and the other one listed below both rewrite the
same region of set_namespace_state in crates/temporalstore-rust/src/meta/registration.rs:

Both report MERGEABLE, because GitHub compares each pull request against
main and never against the other one. Merging one and then the other gives:

CONFLICT (content): Merge conflict in crates/temporalstore-rust/src/meta/registration.rs

Resolving it by simply taking #231's side compiles, and then hangs.
admission_refusal does self.inner.read() while #285 holds
self.inner.write() — same lock, same thread. Measured, not predicted:

test meta::tests::the_emptiness_check_still_refuses_and_still_lets_go ...
EXIT_CODE=124        <- killed by a 45s timeout; it never returns

Worth knowing that only the Dropped arm of admission_refusal takes that
lock, so freeze and unfreeze still pass and only dropping a namespace
wedges
— a quick smoke test would not catch it.

The resolution that works is to factor the guard so it takes &MetaState: the
public path passes the write guard it already holds, and the propose path takes
its own read lock. #299 is stacked on #285 and adds a second check to the same
block, so it wants the same treatment.

@bjmeetsfo
bjmeetsfo changed the base branch from oss/make-the-incident-lever-work to main August 25, 2026 07:42
@bjmeetsfo bjmeetsfo closed this Aug 25, 2026
@bjmeetsfo bjmeetsfo reopened this Aug 25, 2026
@bjmeetsfo

Copy link
Copy Markdown
Collaborator Author

The resolution for the conflict with #231 is written and measured — branch oss/one-lock-over-the-shared-admission-check.

I have not touched this PR's branch. That branch is #231 with this change
composed on top of it, so it is there to be taken rather than something you have
to work out at merge time.

What it does: each judgement gains a form that takes the state to read rather
than fetching it — admission_refusal_in, reserved_name_refusal_in,
namespace_not_empty_in. The propose path takes its own read lock and calls it;
the namespace path passes the write guard it already holds. add_namespace and
add_table go through admission_refusal too, so one judgement serves every
caller and the two cannot drift apart again.

Why it is not simply "take one side": resolving in favour of #231 leaves
admission_refusal acquiring a read lock inside the write lock this path holds,
same thread. Measured on that resolution:

test meta::tests::the_emptiness_check_still_refuses_and_still_lets_go ...
EXIT_CODE=124        <- killed by a 45s timeout, never returns

and on the composed branch:

test meta::tests::the_emptiness_check_still_refuses_and_still_lets_go ... ok
EXIT_CODE=0

Only the Dropped arm takes that lock, so freeze and unfreeze pass either way
and only dropping a namespace wedges — a smoke test would not catch it.

Both tests from this PR are carried over verbatim; meta:: is 292 green.

Two things this does not cover. #235 also touches the same function — it threads
an at_ms from record_mutation into the apply so a dropped namespace's
retention clock survives replay — so that resolution still has to thread at_ms
into apply_namespace_state_locked. Keeping the locked helper as-is reinstates
now_ms() and undoes #235 with no conflict marker to say so. And #299 is
stacked on this PR's current branch, so it wants rebasing onto whichever version
of this you take.

@bjmeetsfo
bjmeetsfo merged commit 9c2a86d into main Aug 25, 2026
10 checks passed
@bjmeetsfo
bjmeetsfo deleted the oss/guards-the-raft-path-skipped 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