fix(activation): gate verification on an observed publish (v1.1.49, step 1 of 3) - #323
Open
JustMaier wants to merge 4 commits into
Open
fix(activation): gate verification on an observed publish (v1.1.49, step 1 of 3)#323JustMaier wants to merge 4 commits into
JustMaier wants to merge 4 commits into
Conversation
…rier
Step 1 of v1.1.49. Adds the scheduler the barrier deletion needs; the
barrier itself, Inconclusive, and the enum follow in steps 2-3.
The verifier judges a slot absent by reading its membership bitmap. That
read is only meaningful once the slot's activation batch has been PUBLISHED
— before that, a healthy post is indistinguishable from a dropped one. The
current answer is an inline force-publish barrier that waits a bounded time
for a publish it cannot make happen faster, times out on a large share of
them, and then guesses. Waiting longer doesn't fix it: barrier ms are paid
as WAL-reader stall, so the cap is an asymptote, not a knob.
Observe the publish instead of forcing it. The flush thread applies the
batch, records the publish count while queueing the slot, and publishes at
the end of that same cycle. Once the live count exceeds the recorded value,
the batch is provably in the published snapshot — proof rather than
inference, no stall, and nothing to tune.
VecDeque<u32> -> VecDeque<PendingVerify { slot, gate }>
drain_activation_verify -> releases only slots whose gate is open
publish_count_acquire() -> the verifier's Acquire read
VerifyGate is an enum, not a count, because a slot restored from disk has
NOTHING to wait for: its state is already in the boot snapshot. No count can
say that — the gate is "count moved PAST my tag" and the counter starts at
0 — so encoding it as a number would either strand restored slots on a quiet
index (no writes, no publish, no verification, silently) or loosen the
comparison for everyone and let live slots be judged pre-publish. Found by
the rehydrate test, which is why it exists.
Ordering: fetch_add moves Relaxed -> Release, read via a dedicated
publish_count_acquire (Acquire). flush_stats stays Relaxed and untouched —
it's the Prometheus path, and quietly retuning it to serve a correctness
consumer is how the next reader inherits a mystery. x86-TSO would hide a
Relaxed bug here; other ISAs would not.
Rehydrate seam (found by ava, not by desk-checking): flush_publish_count is
process-local and restarts at 0 while the v1.1.44 ring persists. Persisting
a gate value would therefore name a count the new process may never reach —
the slot waits forever, unverified, with no log line, no counter, no alarm.
A slot that quietly stops being watched looks exactly like a slot with
nothing wrong. So tags are re-derived on load, never restored; the on-disk
format stays a plain slot list and needs no migration. Both call sites say
so, because "we're losing information, serialize it" is a reasonable thing
to think while breaking it.
Mutation H (persist the gate verbatim) kills
test_activation_verify_ring_survives_restart: checked 0 vs 1. Run and
reverted.
Tests: ring_survives_restart now enqueues at a HIGH count and asserts
verification with the fresh engine still at count 0 — both halves of the
seam. Plus drain-stops-at-first-unpublished (the short-circuit must not skip
or drop slots) and waits-for-the-publish-of-its-own-batch (the gate must
delay judgement, never prevent it).
KNOWN GAP, reported not hidden: test_publish_count_advances_only_after_publish
does NOT pin its invariant. Mutation G (fetch_add above inner.store) leaves
it green — the violation window is nanoseconds and any polling test looks
after the store lands. The ordering is currently held by comment alone. The
structural fix is publish_seq INSIDE InnerEngine, which makes the mutation
inexpressible; awaiting a call on that before rebuilding on it.
Suite: 1293 passed (1290 + 3), 1 failed (query_stream_full_channel_drops_
oldest, always-allowed), 8 ignored.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebuild of the verify gate on publish_seq, replacing the external counter. The previous shape worked; it just couldn't be trusted to keep working. WHAT I TRIED TO BREAK, AND WHAT HAPPENED Mutation G (increment above the store), against the external counter: SURVIVED. The test written to catch it stayed green, because the violation window is nanoseconds and any observational test polls straight past it. That invariant was not pinnable, so the design's whole premise — "acceptable because we'll pin the coupling" — was false. A test that cannot fail is worse than none: it is the reason the next person stops looking. Mutation G against this shape: NOT EXPRESSIBLE. There is no separate increment to hoist. `staging.publish_seq = flush_cycle` writes a field of the object the next line publishes, so the seq and the state it certifies cross the ArcSwap in ONE atomic store. The counter cannot run ahead of the snapshot because it IS the snapshot. Nothing to order, nothing to signpost, nothing to rot. Mutation H (persist the gate verbatim instead of re-stamping): still KILLS test_activation_verify_ring_survives_restart (checked 0 vs 1). Run and reverted. CONSEQUENCES - Release/Acquire and publish_count_acquire: deleted. ArcSwap's own load/store carries the ordering. flush_publish_count reverts to Relaxed and stays reporting-only — its doc now says why nothing may depend on it: it sits BESIDE the snapshot and can drift from what is published, which is precisely the defect being removed. - The ordering signpost: deleted with the thing it guarded. - The decoration test: deleted, with a note in its place recording that it was mutated, survived, and why no replacement is needed. Keeping it "for coverage" would certify nothing and discourage looking. - ForcePublish handlers become correct by construction rather than harmless by luck of direction: they publish staging carrying its own seq, so the seq always describes the snapshot it arrived on. ORDERING, CONFIRMED NOT INFERRED The seq is stamped after the batch is applied and immediately before the store. Verified there is no `inner.store` between the enqueue (which reads the live seq) and `apply_prepared_traced` — all 15 store sites are below the apply. Stamping at store-time makes the question moot rather than answered. A THIRD INSTANCE OF THE SAME SEAM The two InnerEngine rebuild sites (field unload) would have reset the seq to 0 and rewound it — stranding every slot tagged above the new value on a value that never returns, silently, exactly as persisting the tag would. Both now carry the seq forward: an unload changes what is resident, not which publish we are on. Neither is reachable from reading either side alone; they only meet at a rebuild. Suite: 1292 passed (1290 + 2; the third was the decoration test, removed), 1 failed (query_stream_full_channel_drops_oldest, always-allowed), 8 ignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…th gauges reviewer5's blockers on #323. What I tried to break, and what happened: BLOCKER — reproduced before fixing. `published > c` -> `published >= c` at the gate: 79 passed, 0 failed. Confirmed with my own run, not taken on report. It isn't a weakening, it's a total no-op: enqueue tags a slot with the CURRENTLY published seq, so `>=` opens the instant the slot is queued and every live slot is drainable before its own batch publishes. The false-orphan bug this gate exists to remove, restored in one character. Why the tests missed it: they used AfterPublish(u64::MAX) (closed under both operators) and AfterPublish(0) with published > 0 (open under both). Neither touches `published == c` — the one value where the operators disagree, and the exact state every live slot occupies between enqueue and publish. The discriminating value was the untested one, and test_verify_waits_for_the_publish_of_its_own_batch was NAMED for the invariant it did not pin. That is the decoration-test species this PR argues against, in the PR that argues it — green for the right-looking reason. Replaced by test_gate_is_closed_at_the_slots_own_publish_seq, which gates at the live seq and asserts the drain returns nothing, then lets the batch publish and asserts it returns the slot. Demonstrated, not asserted: fails under `>=` with left:[42] right:[], matching reviewer5's reproduction. Bug 3 pinned. `publish_seq: 0` at both rebuild sites -> full suite green, comment-guarded only — the exact thing this PR refuses to accept for Mutation G, and the G argument does NOT transfer: `publish_seq: 0` is expressible, it compiles. test_unload_does_not_rewind_publish_seq now fails on it (0 < 1). Corrected three comments that taught false models: - Bug 3 does NOT strand "permanently". flush_cycle never rewinds, so it self-heals in a cycle. The real cost is the window: nothing drains, the ring fills, and past ring_cap the front is evicted — silently dropping unverified slots. Lower severity, different mechanism. An overstated comment is its own defect. - Drain ordering is NOT structural. requeue read published_seq outside the ring lock, so the ring could go decreasing. Fixed (read inside the lock), and the claim is downgraded to what it is: an optimisation that is safe either way, because an out-of-order tag can only stop the drain EARLY. - The "HIGH count fixture is what makes this able to fail" rationale is false — Mutation H hardcodes the tag at the load site, so it fails at AfterPublish(0) too. Right about the seam, wrong about why. Fixture kept (it looks like production), reason fixed. Thread confinement is now stated at the enqueue site. §3's "the design does not rely on that" was overstated: it relies on apply, tag and store all happening on the single flush thread in one iteration with no publish between. Add a store in that span and the gate opens on a correctly-stamped snapshot that lacks the batch — the seq honest, the verdict wrong. Depth gauges wired: bitdex_activation_verify_pending / _ready, sampled every pass, unconditionally. This PR built the instrument that would falsify a strand and left it unwired, so the failure we have called the worst all night — no log line, no counter, no alarm — was still exactly that. Every other verifier signal is a counter that reports a strand by going QUIET, which is what a clean result looks like; depth is the only one where a strand is a rising line. Suite: 1293 passed, 1 failed (query_stream_full_channel_drops_oldest, always-allowed), 8 ignored. NOT swept: one earlier run also failed concurrent_engine::tests::test_patch_document_updates_existing_slot, which did not recur and passes in isolation. It is not on the allowed list and I cannot attribute it — flagging rather than calling it green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
reviewer5's level 4: is_open was pinned, its only production caller wasn't. Every gate test built its gate BY HAND, so the gate was verified as arithmetic while the one line deciding the argument went untested. Confirmed by running: VerifyGate::Ready at the enqueue site -> 1234 pass, 0 fail AfterPublish(seq.saturating_sub(1)) at the enqueue site -> 1234 pass, 0 fail Under either, every live slot is drainable before its own batch publishes and the whole PR is a no-op, silently. Same miss as the `>=` hole, one seam over: the operator got fixed, the argument stayed open. test_enqueue_site_tags_with_the_live_publish_seq drives a REAL deferred activation through the flush thread and asserts the tag it actually received. Deterministic by quiescence: scheduled two seconds out, engine left idle so nothing publishes, so the live seq is stable at P when the activation cycle reads it. The tag must be exactly P. Mutation L (Ready) -> KILLS: left: Ready, right: AfterPublish(1) Mutation M (seq-1) -> KILLS: left: AfterPublish(0), right: AfterPublish(1) AND A FINDING OF MY OWN, from following reviewer5's run-every-kill-twice rule: the bug-3 pin I added in 0e01a49 was HALF A PIN. Re-running its mutation site by site (rather than both at once, as I first did) shows the test drove only `save_and_unload` -> SyncUnloaded. Mutating the OTHER rebuild site alone — the flush loop's ExitLoadingSaveUnload handler — SURVIVED, twice. Two rebuild sites, two different commands, and I pinned one and reported it as pinned. The test now drives both; each site mutated alone now kills, twice. That is the same pattern a third time: pin the site you are looking at, miss the one next to it. It only surfaced because the double-run rule forced me to mutate the sites separately instead of together — mutating both at once proves that AT LEAST ONE is covered, which is not the claim being made. Re-verified under the double-run rule (flakes in this suite make a single-run "it kills" unreliable in the direction that comforts you; a SURVIVES cannot be manufactured by a flake, only a KILL can): >= gate hole -> kills twice (left: [42], right: []) publish_seq: 0 -> kills twice, at EACH site independently Suite: 1294 passed, 1 failed (query_stream_full_channel_drops_oldest, always-allowed), 8 ignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 1 of the verifier rework, shipping alone as v1.1.49. It adds the publish gate; deleting the barrier,
Inconclusive, andinconclusive_totalis step 2 and is deliberately held until the prediction in §4 is read in prod.Read the evidence before the diff. The diff is small. What justifies it is a design that was built, broken, and rebuilt — and three bugs that lived where two correct things met.
1. Mutation G: survived the design we almost shipped, inexpressible in this one
The first implementation kept a publish counter in an atomic beside the ArcSwap, ordered against the store by hand. It was accepted on the explicit premise that the coupling was pinnable by a test. So the test was written, and then the killing mutation was run:
The invariant is not pinnable. The violation window is nanoseconds; any observational test polls straight past it. The premise was false — and we would have shipped a comment-guarded invariant with a decoration test in front of it, which looks more rigorous than the honest version. A test that cannot fail is worse than no test, because it is trusted.
This PR removes the ability to express the bug rather than testing for it.
publish_seqlives insideInnerEngine, so the seq and the state it certifies cross the ArcSwap in one atomic store:There is no separate increment to hoist. The counter cannot run ahead of the snapshot because it is the snapshot. So no Release/Acquire pair to get right, no dedicated accessor, no ordering signpost, nothing to rot — those requirements do not get satisfied, they cease to exist. The decoration test is deleted, with a note in its place recording that it was mutated, survived, and why no replacement is needed.
flush_publish_countreverts toRelaxedand stays reporting-only; its doc now says why nothing may depend on it — it sits beside the snapshot and can drift from what is published, which is the exact defect removed.2. Three bugs, all at seams. Zero inside the logic.
The counter's own logic was desk-traced four times and was correct every time. Every real bug was at a seam between two individually-correct features:
flush_publish_countis process-local and restarts at 0; a persisted tag names a count the new process never reaches, so the slot waits forever, unverifiedInnerEnginerebuild sites have no seq;publish_seq: 0compiles, reads fine, and rewinds it — stranding every slot tagged above it>->>=makes the gate a total no-op and 79 tests pass. The tests usedu64::MAXand0— two values where>and>=agree — and neverpublished == c, the state every live slot occupiesis_openx its only callerVerifyGate::Readyat the enqueue site passed 1234 tests. Operator pinned, argument opensave_and_unloadonly, so mutating the other rebuild site alone SURVIVED. Mutating both at once proves only that AT LEAST ONE is covered, which is not the claimAll of them fail silently: no log line, no counter, no alarm. A slot that quietly stops being watched looks exactly like a slot with nothing wrong. None of it was reachable by reading either side.
Six seams, zero interior bugs. The logic was desk-traced four times and was correct every time — because nothing was there. Rigour inside a component cannot see between components. Findings 4-6 are the sharper version: they are seams in the tests, not the code. The design got adversarial attention; the tests protecting it got none, and were individually correct and collectively blind.
Methodological, and load-bearing for anyone re-running this: this suite has pre-existing flakes (
mainfails ~1-in-6 full-suite runs in the same module — barethread::sleep(50ms)where a flush-wait belongs). That makes mutation testing report FALSE KILLS. A single-run "it kills" is unreliable in the direction that comforts you; a SURVIVES cannot be manufactured by a flake, only a KILL can. Every kill here was run twice. Finding 6 only surfaced because that rule forced me to mutate sites separately rather than together.Bug 2 is why
VerifyGateis an enum, not a number:Readyis not a sentinel — it is a different kind of thing. Nou64can say "nothing to wait for" when the gate is "count moved past my tag" and the counter starts at 0. Encoding it as a number forces a false choice: strand restored slots, or loosen the comparison for everyone and judge live slots pre-publish (the quieter, worse option).Mutation H (
Ready->AfterPublish(5_000)on load — "we're losing information, keep the seq") killstest_activation_verify_ring_survives_restart:checked: 0 vs 1. Run and reverted. Both call sites carry a comment naming that seductive change.The pre-existing v1.1.44 restart test would not have caught bug 1: it enqueued at tag
0, where persisting the tag is harmless. Green because the fixture dodged the seam — the same species as a test that cannot fail. The new fixture uses a realistic high count.3. Correction: fourteen publish sites, not three
An earlier report of mine said "3 construction sites". That was
InnerEngineconstruction; the publish surface is fourteeninner.storesites (I first said fifteen; four of the grep hits were comments — reviewer5's count is right, and the point is unchanged: 14 ≫ 3). This cuts against my own earlier recommendation: the external-counter design needed one ordering to hold, at fourteen sites, forever, guarded by a comment. It was nearly adopted on a count of three.The stamp sits at store-time, which makes "did anything publish between the stamp and the apply?" unaskable rather than answered — for the stamp. But the enqueue does rely on a real property, and I originally overstated this as "the design does not rely on that". It relies on thread confinement: the apply, the tag and the store all happen on the single flush thread within one iteration, with no publish between, so the next publish is necessarily the one carrying the batch. Add a store in that span and the gate opens on a correctly-stamped snapshot that lacks the batch — the seq honest, the verdict wrong. That invariant is now stated at the enqueue site rather than credited to the wrong property.
Bonus: the ForcePublish handlers become correct by construction rather than harmless by luck of direction — they publish staging carrying its own seq, so the seq always describes the snapshot it arrived on.
4. The prod prediction — pre-registered, version-gated, and falsifiable
This is why step 1 ships alone. We have deterministic proof the gate works in a test. We do not have proof that our model of prod is right — that pre-publish reads are what produce publish-lag. This release makes that model falsifiable while the barrier is still in place to catch us.
This is not inert. It changes when the verifier reads: later, and only later. That is the safe direction — a slot verified late is fine, a slot verified early is the bug — but it is a behaviour change, and the prediction only exists because it is one.
Pre-registered before deploy:
The LOUD term (checked first — without it the rest is unfalsifiable):
📈
bitdex_activation_verify_checked_totalMUST keep climbing, at roughly its current rate.This gate is precisely the kind of change that can silently switch the verifier off: if the seq never advances, or the gate never opens, no slots are drained — and then
publish_lag,inconclusiveandredrivenall go to zero, which is indistinguishable from a perfect result. A prediction made only of zeros cannot be falsified. Ifchecked_totalflatlines, this release FAILED, however clean everything else looks.Then the quiet terms:
publish_lag_totalrate drops andinconclusive_total-> ~0, whilechecked_totalholds its rate. Slots stop being read pre-publish, so apparent orphans stop being manufactured ⇒ the barrier is provably redundant, and step 2 deletes something we watched become unnecessary.checked_totalholds,_pending > _readydid occur (so the gate is genuinely working), andpublish_lag_totalstill does not drop. Then pre-publish reads were not the cause. We learn it with the barrier still there. Step 2 does not proceed.redriven_totalshould stay 0 throughout. If it moves, that is a real drop signal, independent of this change.Why this can't wait for step 2: under Mutations L/M today, the still-present barrier catches the bypass and
publish_lagclimbs — the release fails loudly, which is exactly why step 1 ships alone and why it works. Step 2 deletes the barrier, which is the only thing covering the enqueue site. After step 2, a bypassed gate reads: ratio 1.0 ✅, ring depth 0 ✅,checkedholding ✅, lag gone (deleted) ⇒ every surviving term confirms while the gate does nothing. So pinning the enqueue site is a hard prerequisite for step 2, and it lands now, while the barrier is still here to prove it works.Reviewers: please attack the prediction as well as the code. If the falsification condition is wrong it needs to be wrong now — pre-registering it is what stops us reading whatever happens as success.
Diff
InnerEnginegainspublish_seq; stamped at store-time; carried across both rebuild sites.VecDeque<u32>->VecDeque<PendingVerify { slot, gate }>.drain_activation_verifyreleases only slots whose gate is open, stopping at the first closed one — sound because ordering is structural: a single flush thread assigns strictly increasing seqs, so no config knob can perturb it (unlike a time-based delay).Ready; the on-disk format stays a plain slot list — no tag to serialize, so no version bump and no migration.published_seq()replacespublish_count_acquire().Tests: 1292 passed, 1 failed (
query_stream_full_channel_drops_oldest— always-allowed), 8 ignored.cargo check --features server,pg-sync --libclean.bd21987(the external-counter shape) is kept beneath255ecf4unsquashed on purpose: the Mutation G evidence is the argument against the design we almost shipped, and squashing it would leave the conclusion without the reasoning.🤖 Generated with Claude Code