Prefix-complete per-partition WAL applied watermark — closes a pre-existing acked-write-loss defect [SPEC-350] - #119
Merged
Conversation
Reads the docs site's real search performance (impressions, position, which page ranks for which query) without clicking through the GSC web UI. The service account key is NOT in this repo and must never be: this repo is public, and the key is a real RSA private key. It lives at ~/.config/gsc/service-account.json outside every git working tree, so there is nothing here to leak; the script reads it from there and errors with instructions if absent. Access is read-only (GSC role Restricted, scope webmasters.readonly). The .gitignore rule is a backstop, not the mechanism — *.key stays commented out since test fixtures rely on it. Site defaults to sc-domain:topgun.build; --site queries any other property the service account can see, and the property is printed in the result header so reading the wrong site's numbers is self-evident. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t seams - WalOp::subsumes_on_coalesce(): exhaustive, no catch-all, survivor-evaluated - wal_fail_stop(tier, ctx) -> !: tracing::error! + process::abort(), panics under cfg(test) so the tier stays observable - append(): sealed-target pre-check routes to tier P; residual write/fsync failures route to tier B through the same helper - three cfg(test) accessors: test_seal_active_segment, test_read_applied_sequence, test_fail_stop_observations
… test seams - PendingOrigin and WalWatermarkAlarm enums, with the cause carried on the abandoned-write class at the type level - cfg(test) WatermarkMode (ScalarMax / PrefixComplete) plus its selector seam, so the differential test keeps a live negative control - classifier seam: per-partition sample counter, mid-sample hook positioned between the registry probe and the queue scan, test_run_classifier_sample - pub(crate) seams so sibling test modules can reach this store's internals
- Bundle the WAL handle with its tracking state (`wal: Option<WalTracking>`) so a tracker without a WAL is unrepresentable - Add the per-partition pending map (`wal_seq` -> `PendingOrigin`), `max_assigned`, and the in-flight registry under one per-partition mutex - Compute the applied watermark prefix-complete: `min(unresolved) - 1`, empty => `max_assigned`, never the counter's load - Assign tagging `Appending` under the same lock as the counter bump; promote to `Live` after the queue guard drops, conditionally so a terminal absorbs it - Roll back only the failed sequence on a pre-frame append error - Retype `DelayedEntry.wal_sequence` scalar -> set and drop its false zero-means-no-WAL doc claim - Add `#[cfg(test)]` seams for the pending map, max-assigned, registry and watermark, plus tracker unit tests incl. the scalar-max negative control
…rability class - seed a partition's pending map from wal.unapplied() on its first assign, tagging prior-incarnation frames BootUnreplayed so this process's watermark cannot advance past frames an earlier one left un-applied - make the prefix-complete watermark a typed Result: an empty pending map that has not been seeded is unknowable, never a silent advance - seed max_assigned at 0; the value is dominated by sequence_start - abandoned terminals (max-retries discard, shutdown-drain timeout/error, flush_key error) re-tag Live -> Abandoned and do NOT resolve, keeping the frame replayable instead of licensing the watermark past a non-durable write - flush_key success now resolves AND advances, so a flush_key-only partition cannot move its pending set while the sidecar stands still - register the in-flight registry at the three drain/queue.remove points only, deregister on every exit from in-flight including the retry reinsert - every existing entry-space resolve_pending call is untouched
- add wal_watermark_stall_bound_ms (default 60_000) to WriteBehindConfig - parse TOPGUN_WAL_WATERMARK_STALL_BOUND_MS: warn-and-default on unparseable, FATAL below MIN_WAL_WATERMARK_STALL_BOUND_MS (6_000) - derive reconfirm_delay_ms() and watchdog_tick_ms() with their floors - add the 7th from_env doc-table row with the raise-alongside dependency - replace the false blanket never-panics prose and document the second fatal config-parse case
- dispatch coalesce-retire on the survivor's subsumes_on_coalesce(): early-resolve when the survivor's frame subsumes, carry the retired sequences forward onto the survivor when it does not - close the under-advance stall where a repeatedly coalesced key left its predecessors' sequences pending with no owner - amend the max-retries discard comment: drop the spec reference and replace the false GC-retention rationale with the true cause
…gauge - publish topgun_wal_applied_watermark_lag per partition and register HELP/TYPE for it and topgun_wal_gc_skipped_total on the metrics facade - classify a stalled sequence by dispatching on its origin tag first and consulting the registry and queue only inside the Live arm - require two independent samples before a TrackerLeak verdict, so a sequence drained mid-classification cannot accuse correct code - spawn a dedicated watchdog task at the derived tick, reporting only a stall that outlives the bound while writes keep arriving - resolve coalesce-subsumed sequences without advancing, keeping the mark_applied cadence and its disk I/O off the write path
…faults - add TOPGUN_WAL_WATERMARK_STALL_BOUND_MS with its default, its raise-alongside dependency and both misconfiguration dispositions - add TOPGUN_WRITEBEHIND_SHUTDOWN_TIMEOUT_MS, which the new row references and the section did not define
- WalRecovery::run advances the watermark only across a contiguous prefix of successful replays, so a failed entry no longer marks itself applied - raise the boot abandoned-write alarm when a partition's frontier stops below its highest enumerated frame, covering idle partitions the watchdog never samples - document the violation posture: environmental input is handled as a value, so a legacy or pre-fix on-disk WAL still boots and replays - rewrite the Wal trait's mark_applied/unapplied contracts around the prefix-complete rule and drop their stale spec references
…nt unlink - add WalGcError, a typed error the GC path counts and logs instead of asserting - re-read the .applied sidecar immediately before each unlink and stop the reclaim prefix at the first segment it does not cover - increment topgun_wal_gc_skipped_total via the metrics facade on every retention - move the two WAL watermark metric names and their describe fn into wal/mod.rs so both producers share one definition rather than a parallel path - add a per-writer test seam forcing the re-read low, since a monotonic sidecar makes the path unreachable otherwise - rewrite the mark_applied and segment-GC doc contracts: the monotonic guard governs only the sidecar write, while seal/rotate and GC run at max(sequence, current) under their own preconditions
…append stop - add FAIL_STOP_TEST_LOCK so index-based reads of the process-global observation log cannot return a concurrent scenario's tier - assert the sealed-active append records exactly ONE stop, so the tier-P pre-check is proven to run BEFORE the residual class (B) disposition
- differential proptest over the stranded-entry interleaving: acked writes survive under PrefixComplete and are LOST under ScalarMax, so the negative control stays live rather than being a one-off revert - pin the exact boundary: min(unresolved) - 1 when pending, max_assigned when empty-and-seeded (never the counter's load, proven by replaying a frame appended after the partition drained), typed Err when unseeded - audit every terminal by durability class: coalesce-retire across add/remove/ remove_all advances the full prefix; an abandoned terminal does not resolve and its frame is replayed; a pre-frame append failure rolls back ONLY the frameless sequence; a post-frame failure fail-stops at tier B without rolling back; a mid-loop remove_all failure keeps the earlier tombstones and replays them across an incarnation boundary - drive boot seeding through THREE incarnations with an injected replay failure: the seeded frontier holds, the stall reads AbandonedWrite, and the naive scalar advance erases the retained frames (the live control) - site the file as a child module of write_behind, since the store's internals sit in a private module no sibling can name
…ce and the skipped-GC scrape - add a cfg(test) partial-state WalOp framing with explicit arms in both matches (predicate => false, replay => typed Err), no catch-all - assert survivor-only evaluation on the mixed retired/survivor pair - assert a flush_key-only Ok terminal moves the durable .applied sidecar - assert the retained-segment counter is described and moves on /metrics - drop the Once guard on the metric describe: a pre-init emission burned it against the no-op recorder and left the metrics permanently HELP-less
…ot line - move the boot summary info! out of main into log_boot_summary and call it in its place, so the emission itself can be asserted - add wal_watermark_stall_bound_ms as a bare read of the parsed config - capture it with a scoped subscriber in the bin's own tests (value substring) - assert the config parse: default, override, unparseable warn-and-default, 6000 accepted vs 5999 fatal, and the derived floors at the boundary - add a cfg(test) non-subsuming-survivor seam for the carry-forward route - CLAUDE.md: list the new field on the boot line
- six scenarios, one per classifier row: hung store, boot-unreplayed, a queued-then-dequeued ownerless sequence, a max-retries discard, a sequence queued behind a blocked worker, and a hung wal_append - each asserts the right class AND explicitly not the wrong one, incl. the Live/Appending origin discrimination between the two hangs - add the transient-ownerless control: a drain inside the classifier's own mid-sample seam must not fire any alarm, and the second sample must be taken - assert the lag gauge on a /metrics scrape with per-test-unique partitions - give both fault doubles a releasable hang mode
… durability, due-time stability and both coalesce routes - widened-window re-replay settles on the newest frame and leaves the durable tombstone ground truth a boot reconciliation would recompute unchanged - recovery stops at the contiguous-success frontier and self-heals next boot - a legacy bare-Value WAL still boots and replays - a committed redb write survives drop-and-reopen (the assumption the resolve-on-flush-success advance rests on) - a coalesce never shifts the key's store_time - a subsuming survivor early-resolves the retired sequence; a non-subsuming one carries it forward, stalls the watermark at the OLDEST carried sequence, and the retired frame replays when the survivor is abandoned
… pending sequences An unseeded partition's own sequences all start ABOVE the frames a prior incarnation left un-applied, so min(pending)-1 computed before seeding sits above them: mark_applied would mark never-replayed frames applied and hand their segments to GC. The seeded guard covered only the empty-pending arm, so two or more pending sequences walked straight past it. - gate both arms of the watermark computation on seeding - seed inside resolve_and_advance, so a flush_key-only partition (which no mutating entry point seeds) still advances instead of growing forever - regression test: an unseeded partition with pending sequences advances nothing and every frame stays replayable Found by the cross-vendor review of the branch diff.
…provide replay_entry and recover both asserted that idempotency comes from the inner store's merge, so re-replaying an already-applied frame was safe. That is false for RedbDataStore: write_one is a blind table.insert with no read-compare, and a stale replayed frame can clobber a newer durable value written outside the window (reproduced). State the true, narrower property instead — re-replay safety rests only on in-sequence-order replay plus last-frame-wins WITHIN the replayed window — and point at the tracker, since the doc contract is where the next contributor looks, not the spec.
The two metric consts and their describe fn were moved to wal/mod.rs as pub only to cross a private-module wall inside the crate. Every consumer is crate- internal, and no crate depends on topgun-server, so pub(crate) expresses the real reach without exporting them from the library.
…heir own failure modes
The advance site's seed call was justified as covering a partition whose only
terminal is flush_key. That reason is wrong: every enqueue site is add/remove/
remove_all (the fourth is a retry-reinsert of one of those), all three seed, and
flush_key advances only when it removed an entry those calls created — so no
partition reaches an advance without a prior seeding entry point.
The real mechanism is seed RETRY. ensure_wal_seeded swallows a WAL-read error
and leaves the partition unseeded, and the mutating entry points retry only when
another write for that partition arrives; a partition that goes quiet after a
failed seed reaches its terminals through flush_key and the flush loop alone.
Correct the comment to say so.
Add the experiment that was missing: the existing flush_key test seeds via its
own store.add, so it stayed green with the seed call deleted. This one injects a
transient unapplied() failure and asserts each guard against its own failure
mode — verified by reverting each in turn:
- refuse-guard reverted to empty-arm-only -> Ok(2), an over-advance above
never-replayed frames 1 and 2
- seed retry deleted from resolve_and_advance -> partition never seeded, the
sidecar never moves and the WAL grows unbounded
Deploying topgun with
|
| Latest commit: |
2b00e42
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8cc2139d.topgun-f45.pages.dev |
| Branch Preview URL: | https://feat-sf-350-prefix-complete.topgun-f45.pages.dev |
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
1 participant
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.
Summary
Closes a pre-existing acked-write-loss defect: the WAL applied watermark was a per-partition scalar advanced to
max(flushed entry's seq), while the write-behind flush drains by due-time (not seq order) and coalescing assigns fresh high seqs — so the watermark could advance past OTHER keys' un-flushed lower seqs. Akill -9in that window silently dropped acked writes from replay and handed their segments to GC. Reachable today under ordinary churn; full-snapshot frames only masked it when the key happened to be rewritten.The fix: each partition now carries a prefix-complete watermark
W(p) = min(unresolved_wal_seq) − 1, boot-seeded fromwal.unapplied(p)(frames a prior incarnation left un-applied block advance), resolved only on genuinely durable terminals, with the tracker bundled into one struct with theArc<dyn Wal>so the two cannot diverge. Segment GC can no longer reclaim never-applied frames. An unseeded partition refuses to advance rather than over-advancing (loss-class guard, discrimination verified by revert during review).Also hardened along the way:
wal_fail_stop(tier, ctx) -> !— abort-based fail-stop (survivespanic = "abort"being forbidden; tokio cannot contain it), with a#[cfg(test)]seam as the assertion hook. Err taxonomy: (P) sealed-target = programming-bug panic-tier; (A) pre-frame = rollback; (B) write/fsync = FAIL-STOP per the fsyncgate precedent (PostgreSQL 2018, TiKV) — never retry a failed fsync.⚠ Carried forward (recorded, honest — read before citing this branch)
RedbDataStore::write_oneis a blind insert — re-replaying an older frame can clobber a newer durable value (reproduced). AC4's literal merge-level idempotency is NOT met and the doc-contracts now state the truth and point at the tracker. This branch must not be cited as establishing re-replay idempotency.mark_appliedfailure is invisible to both alarm tiers (growth, no loss).OR_TOMBSTONE_BYTESglobal-counter test flake (main is equally fragile); may need a CI re-run if it trips.Test evidence
12 audit cycles (incl. cross-vendor checkpoints) + review v1 CHANGES_REQUESTED → fixes → v2 APPROVED.
cargo fmt/clippy --all-targets --all-features -D warningsclean; 1676/0/1 lib tests; loss-guard discrimination verified by revert (Ok(3)/Ok(2)over never-replayed frames on pre-fix code); load harness 37,196 ops/s @ p50 1.5 ms — no regression; /xreview 4 findings dispositioned.Extraction refs: TiKV applied-index discipline + Hazelcast write-behind (
.specflow-tracked memos) — the strict prefix-complete design is externally validated (TiKV never hits this bug only because its apply order is gap-free; Hazelcast HAS the bug but their seq never gates truncation).72h soak chain: next = TODO-597 (cross-incarnation model harness) → SPEC-349a/b (OR delta-framing) → SPEC-348 (hard gates) → TODO-586 → 72h.