Repository navigation
feat(config): make startup bounds configurable - #412
Conversation
EnRaiha
changed the title
7ed16c1 to
b0f1c8f
Compare
b0f1c8f to
c620167
Compare
There was a problem hiding this comment.
Request changes. The head commit does not compile its own test target. The WAL fix is at the right layer, but it covers one of four places that read record headers.
Rebase first
The branch is 25 commits behind main. It merges cleanly today, but rebase onto current main before the next push. Re-run the checks on the rebased head and post the output from that head.
Claims checked
| Claim | Result |
|---|---|
cargo fmt --all -- --check passes |
Holds. |
| Full library suite passes on this head | Does not hold. cargo nextest run -p nodedb-wal fails to compile at c620167 with E0382 at nodedb-wal/src/reader.rs:595. |
nodedb-types startup tests |
Hold: 3/3 pass. |
Every pass claim in the description needs re-running on the rebased head.
Layer
The startup bounds are at the right layer and match what #354 asks for.
The WAL change is at the right layer: the reader that both recovery and the startup gate share. It needs four changes before it fixes the bug class instead of one instance:
- Fix every reader.
nodedb-wal/src/lazy_reader.rs:153andnodedb-wal/src/mmap_reader/reader.rs:299still turnUnsupportedVersioninto aCorruptionstop.nodedb-wal/src/torn_tail.rs:156(intact_record_lsn) still treats a non-current version as "not a record". After this PR, the sentence the description writes about the old reader is still true of three of them. - Decide the version per segment, not per record.
header.validatechecks the version before the CRC, so the reader declares a format gap from two unverified bytes. The version-0 carve-out patches one case of that. A writer never mixes format versions inside one segment. A mismatch on the segment's first record (after any preamble) is a format gap. A mismatch after an intact current-version record is damage, andtorn_tailmust classify it. - Attach the segment path where the open fails. The pre-open
validate_segments_for_startupcall runs a second full scan of every segment. It exists only because the writer's open returns the version error without a path.recovery::recover(path)andSegmentedWal::openalready know the path. Attach it there, and keep one validation pass. - Name an action that exists. No WAL migration path exists, so "needs a migration" sends the operator looking for a tool that is not there.
Scope
- Split the clippy changes out.
vector_primary.rsandcalvin_reply/stage.rskeep the logic the same but are unrelated to #354. They belong in their own PR. - Split the newest-segment relaxation out. Accepting a record-less newest segment changes startup behavior, separate from version diagnosis. It needs its own commit and an accurate justification (see the inline note).
What landed correctly
The [tuning.startup] section follows the existing tuning pattern, and the bounds are wired from main.rs to both gates. The [server] stale-key error does what #354 asks for.
| // An empty stream is also what a silent skip or a clean end would | ||
| // return, so the reason has to be asserted for this to prove a stop. | ||
| assert!( | ||
| matches!(reader.stop_reason(), Some(StopReason::Corruption { .. })), |
There was a problem hiding this comment.
Blocker: this does not compile. records(self) moves reader at line 585, and this line borrows it after the move (E0382). Keep the reader and iterate with next_record() in a loop, then read stop_reason(). Re-run the full suite on the rebased head after the fix.
| offset: header_offset, | ||
| }); | ||
| } | ||
| Err(error @ WalError::UnsupportedVersion { .. }) => return Err(error), |
There was a problem hiding this comment.
Blocker: this reader now returns the version error, but lazy_reader.rs:153, mmap_reader/reader.rs:299, and torn_tail.rs:156 still turn the same UnsupportedVersion into corruption or "not a record". Fix all four in this PR.
This arm also fires before the CRC check and at any offset. A mismatch after an intact current-version record in the same segment cannot be a format gap, because the writer never mixes versions in one segment. Return the version error only for the segment's first record (after any preamble). Route a later mismatch through the corruption stop, so torn_tail classifies it. The version-0 arm above then needs no special case.
| }; | ||
| let status_fn = Arc::clone(status_fn); | ||
| let deadline = Instant::now() + DATA_GROUP_RECOVERY_TIMEOUT; | ||
| let deadline = Instant::now() + timeout; |
There was a problem hiding this comment.
Blocker: a configured value overflows Instant and panics the boot. For example, data_group_recovery_timeout_ms = 18446744073709551615 panics here, and admitted_within in auth_lease/status.rs does the same. 0 fails every boot at once. Check both bounds at config load against a stated range, and return a config error that names the key and its range.
| /// resets the clock, so a large replay finishes; only a stuck group fails. | ||
| /// Default: 300_000 (5 minutes). | ||
| #[serde(default = "default_raft_ready_timeout_ms")] | ||
| pub raft_ready_timeout_ms: u64, |
There was a problem hiding this comment.
Nothing checks either bound. 0 and u64::MAX both deserialize, and both break the boot (see the note on data_group_recovery.rs:185). Add a range check at config load that names the key, the value, and the valid range.
| pub async fn await_planning_admitted(state: &SharedState, timeout: Duration) -> crate::Result<()> { | ||
| pub async fn await_planning_admitted( | ||
| state: &SharedState, | ||
| timeout: RaftReadyTimeout, |
There was a problem hiding this comment.
Planning admission is a different wait from the metadata apply stall. It is a hard deadline, and the stall bound resets on progress. Typing it RaftReadyTimeout couples them, and it raises this wait's default from 30 s to 300 s without saying so. startup.rs says different waits get different types. Give this wait its own type and bound, or keep its own constant, and state the default in the description.
| "the message must name the version it found, and it named none: {text}" | ||
| ); | ||
| assert!( | ||
| text.contains("requires 3"), |
There was a problem hiding this comment.
"requires 3" breaks on the next format bump. Build the expected text from nodedb_wal::record::WAL_FORMAT_VERSION.
| // torn tail must not stop a store from opening. | ||
| let info = match recovered { | ||
| Ok(info) => info, | ||
| // A store written by another build used to be reported as |
There was a problem hiding this comment.
Comments describe what the code does now, not what an earlier version did. "used to be reported as corruption" inverts once this merges. The same applies to line 510 ("The production store failed every boot"), reader.rs:548 ("The reader used to file..."), and the comment at reader.rs:233. Rewrite each to state the current rule.
| /// write rather than a format this build cannot read. Opening over it must | ||
| /// succeed, because the writer's open is the first thing a boot does. | ||
| /// | ||
| /// A review of an earlier version of the reader change found this missing: |
There was a problem hiding this comment.
Remove the review history from the comment. State what the test pins: a newest segment with the record magic and a zeroed version opens.
| /// rolled would refuse to open. This uses the real writer to produce the | ||
| /// state, rather than a file crafted by hand. | ||
| #[test] | ||
| fn open_accepts_a_segment_the_writer_rolled_and_never_wrote() { |
There was a problem hiding this comment.
This passes on main too. The writer creates the rolled segment with 0 bytes, and every path already skips a 0-byte segment. It pins no behavior this PR adds. Remove it, or make it cover the state the relaxation exists for.
| /// names the path that is read instead. `deny_unknown_fields` catches the key | ||
| /// anyway, but its message lists every valid field instead of the replacement. | ||
| /// | ||
| /// Neither key was ever a `[server]` field on `main`. They appear at that path |
There was a problem hiding this comment.
Remove the branch history ("Neither key was ever a [server] field on main... never landed"). The guard's purpose is enough: it names the path that is read for a misplaced key.
ddbc5be to
b112e60
Compare
Startup waits for the metadata group, the data groups, and Calvin catch-up used fixed wall-clock deadlines, so a node that was making steady progress could still be refused. Each wait now ends only when it makes no progress for a configured stall bound, and every applied entry resets the clock. The bounds live in the new StartupTuning config and are range checked on load. Setting them under [server] is rejected with a message that names the [tuning.startup] path. The Calvin scheduler's caught-up registry exposes a watermark so its wait can see progress. Test harness call sites and the getting-started guide are updated.
A segment written in a WAL format this build cannot read was reported as corruption, which sends an operator toward repair instead of toward the build that wrote the data. Readers now judge the format at the segment's first record and return SegmentFormatVersion with the segment path and both versions. Startup turns it into a version-gap message that lists the actions that exist. A zeroed or damaged version field is still corruption, not a format gap, and only a record whose checksum matches its declared version counts as another format. The restore walkers report the same error.
A crash right after a segment rollover leaves the newest segment with zeros or a torn first record. The next boot resumes that file, but startup validation refused it as corrupt, so every later boot failed too. holds_only_crash_residue recognises that state, and validate_for_startup exempts the newest segment when it matches. Any other content, and any older segment without a record, is still corruption.
b112e60 to
1dc0333
Compare
farhan-syah
added
the
run-ci
A cold ASan build uses most of the job time limit. Pin the nightly so the cache key stays stable across days, and cache the fuzz target directory per matrix target, keyed on the toolchain and fuzz/Cargo.lock.
Closes #354
Rebased onto
mainat410ecae55.What changed
feat(config)raft_ready_timeout_msanddata_group_recovery_timeout_msfrom[tuning.startup]. Each boot wait fails only when it makes no progress for its bound. Range-checks both values, refuses an unknown key, and refuses either key under[server]with the path that is read.fix(wal)fix(wal)Startup waits
bootstrap/stall_clock.rstracks progress for every boot wait. A measure that rises, or a group that finishes, resets the clock.CaughtUpRegistrycarries each scheduler's watermark for this.WAL format version
nodedb-wal/src/record/foreign_version.rsjudges a header whose version this build does not read. It recomputes the checksum with this build's version. A match means a flipped bit in this build's record, so the reader stops there as corruption. No match means another build's format.torn_tail::holds_only_crash_residuedecides the newest-segment exemption invalidate_for_startup.Behaviour changes
tuning.startup.raft_ready_timeout_ms, default 30 s, reset on progresstuning.startup.data_group_recovery_timeout_ms, default 60 s, reset on progressBoth keys accept
1to86400000ms.How to check
cargo nextest run --workspace --exclude nodedb-cluster-tests --all-features --retries 0: 21833 of 21834 passed. The failure wasstartup_failure::nodedb_exits_nonzero_on_corrupted_wal: the newest-segment exemption accepted junk. The crash-residue rule fixes it, and the WAL and startup tests pass, 432 of 432.cargo nextest run -p nodedb-cluster-tests --all-features --retries 0: 504 of 504 passed.cargo clippy --all-targets --all-features -- -D warningsonnodedb,nodedb-wal,nodedb-types,nodedb-test-support: clean.scripts/ci/check_*gate passes, including--self-test.