Skip to content

Close the window between the emptiness check and the namespace drop - #285

Open
bjmeetsfo wants to merge 2 commits into
mainfrom
oss/close-the-namespace-drop-window
Open

Close the window between the emptiness check and the namespace drop#285
bjmeetsfo wants to merge 2 commits into
mainfrom
oss/close-the-namespace-drop-window

Conversation

@bjmeetsfo

Copy link
Copy Markdown
Collaborator

Independent of the other open work — main base, two files.

A namespace could be dropped out from under a live table

set_namespace_state checked "namespace still holds a live table" under a
read lock, released it, then recorded and applied the change. A table created
in that window was stranded.

Demonstrated on main, deterministically:

guarded drop     -> ok=false  namespace_not_empty     the public path refuses
post-check apply -> ok=true                           what runs after the release
namespace now=Dropped   live tables in it=1

This is not theoretical. serve_with_stream_handler does thread::spawn per
connection, so a drop_namespace and an add_table arriving together is
ordinary traffic. It is also the same end state as the raft-path defect in #231,
reached a different way — there the guard was absent, here it was released too
early.

The change

One write lock across the check, the record and the apply.

The apply body moves into apply_namespace_state_locked, which takes an
already-held &mut MetaState. The public path checks and applies under a single
lock; replay keeps entering through apply_set_namespace_state and reapplying
unconditionally, so the rule that guards must not live in the apply path is
unchanged.

record_mutation stays before the state moves, so a crash between the two
replays the change rather than losing it. It does not touch self.inner, so
holding the lock across it cannot deadlock.

The cost, and why it is affordable here

Holding the lock across record_mutation means one fsync with readers blocked.

That is affordable in this method and would not be in most: the only callers are
freeze_namespace, unfreeze_namespace and drop_namespace — operator actions
reached from admin routes, never a background loop, never per-request. Topology
reads are the hot path and they are not on it.

I checked the rest of the metaserver for the same shape — meta.rs,
registration.rs, table_ops.rs, state_setters.rs, proxy_groups.rs — and
this was the only method that released its lock between checking and applying.

Tests

  • a namespace is never dropped out from under a live table — twenty-four
    rounds, each racing a drop against a table creation on separate threads, with a
    durable log so the fsync sits in the window. On the unfixed code this catches
    the bug 6 runs out of 6: a reliable reproduction, not a flaky one.
  • the emptiness check still refuses and still lets go — the guard keeps
    refusing a non-empty namespace, stops refusing once the table is deleted, and
    freeze/unfreeze still work. It passes on the old code too, which is what a
    guard against over-correction should do: holding the lock longer must not buy
    atomicity by making the namespace undroppable.

Verification

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

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

The namespace state change has to decide and act without letting go: the
emptiness check used to run under a read lock that was released before the
change was applied, and a table created in that window was stranded with
its namespace already Dropped.

The judgement it needs now lives in admission_refusal, which takes its own
read lock. Calling that under the write lock this path must hold is a read
acquired inside a write, on the same lock and the same thread, and it does
not return -- measured, the namespace drop test never finishes.

So each judgement gains a form that takes the state to read rather than
fetching it: admission_refusal_in, reserved_name_refusal_in and
namespace_not_empty_in. The propose path takes a read lock and calls it;
the namespace path passes the write guard it already holds. add_namespace
and add_table now go through admission_refusal too, so one judgement
serves every caller.
@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

Copy link
Copy Markdown
Collaborator Author

This PR also collides with #285, and that resolution is written and tested — branch oss/one-lock-and-the-recorded-drop-clock.

No branch of yours has been touched. That one is #231, then #285 composed onto
it, then this PR composed onto that, so the whole registration.rs cluster is
there in one place.

The collision is over the same two lines. This PR makes the drop clock come from
the time record_mutation recorded, so replay stamps the same instant instead of
whatever the clock says when it replays. #285 splits the apply so the guarded
path can check and apply under one write lock without letting go.

Git's automatic merge produces something that does not compile, which is the
good outcome: it carries this PR's at_ms into the body of the split-out helper
while the helper's signature comes from #285 and does not take it. The compiler
stops there.

The trap is what comes next. The quickest way to make it compile is to delete
at_ms and put now_ms() back, and that is exactly this PR undone -- the clock
would restart on every replay, and no test in either PR fails, because each one
passes on its own branch.

The resolution that keeps both: the lock-holding helper takes at_ms, the
guarded path passes what record_mutation answered, and the replay entry passes
its own through. meta:: is 295 green, and the namespace drop still completes
rather than deadlocking:

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

# Conflicts:
#	crates/temporalstore-rust/src/meta/registration.rs
@bjmeetsfo
bjmeetsfo force-pushed the oss/close-the-namespace-drop-window branch from dd10297 to 0d06faa Compare August 31, 2026 09:22
@bjmeetsfo

Copy link
Copy Markdown
Collaborator Author

Rebuilt on main now that the shared admission check has landed.

This branch and that change both rewrote the same region, and the resolution is
not either side. The judgement now lives in admission_refusal, which takes its
own read lock, and this path has to hold the write lock across the check and the
apply. Calling the &self form under that lock is a read acquired inside a
write, on the same lock and the same thread, which does not return.

So each judgement has a form that takes the state to read: the propose path
takes its own read lock, and this path passes the guard it is already holding.

It also carries the recorded time through to the drop stamp. Main threads the
time record_mutation returns into the apply, so a replay stamps what the log
says rather than when the replay happened. Keeping this branch's apply as it
was would have quietly put now_ms() back — the conflict marker was on the
call, not on the stamp, so nothing would have pointed at it.

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