numinnex commented on code in PR #3946:
URL: https://github.com/apache/iggy/pull/3946#discussion_r3830604750
##########
core/server/src/segment_recovery.rs:
##########
@@ -92,53 +134,99 @@ pub async fn load_persisted_segments(
.system
.get_index_path(stream_id, topic_id, partition_id,
start_offset);
- let messages_size = file_len(&messages_path);
- let index_size = file_len(&index_path);
+ let raw_messages_size = file_len(&messages_path)?;
let bounds = recover_segment_bounds(
+ identity,
&index_path,
&messages_path,
start_offset,
- messages_size,
- stream_id,
- topic_id,
- partition_id,
+ raw_messages_size,
+ &mut scratch,
)
.await?;
- // `bounds == None` now means the log holds no whole BATCH either (the
- // index-less path above already tried walking the log), so there is
- // nothing to recover: zeroed sizes make the next append overwrite the
- // torn bytes, where counting them with `end_offset == start_offset`
- // would fabricate one phantom message for the bootstrap non-empty
- // filters and strand undecodable garbage inside the readable range.
- // Note this is NOT tail-only -- a torn index is reachable mid-chain on
- // the shipped `enforce_fsync = false`, which is why the walk above
- // exists rather than refusing the partition.
- let (start_timestamp, end_timestamp, end_offset,
effective_messages_size) =
- if let Some((start_timestamp, end_timestamp, end_offset,
walked_size)) = bounds {
- (start_timestamp, end_timestamp, end_offset, walked_size)
- } else {
- if messages_size > 0 {
- warn!(
- stream_id,
- topic_id,
- partition_id,
- start_offset,
- messages_size,
- "segment log holds bytes but its index holds no whole \
- entry (torn write); recovering the segment as empty"
- );
- }
- (0, 0, start_offset, 0)
- };
- let effective_index_size = if bounds.is_some() { index_size } else { 0
};
+ // `bounds == None` means the log holds no whole batch ANYWHERE: the
+ // index-less walk tried from byte 0 and the damage probe found no
+ // surviving batch deeper in the file. There is nothing to recover:
+ // zeroed sizes make the next append overwrite the torn bytes, where
+ // counting them with `end_offset == start_offset` would fabricate one
+ // phantom message for the bootstrap non-empty filters and strand
+ // undecodable garbage inside the readable range. Note this is NOT
+ // tail-only -- a torn index is reachable mid-chain on the shipped
+ // `enforce_fsync = false`, which is why the walk exists rather than
+ // refusing the partition.
+ let bounds = bounds.unwrap_or_else(|| {
Review Comment:
**Blocker: this arm now physically destroys the only durable copy of the
data.**
`set_len` and `truncate` appear nowhere in master's `segment_recovery.rs` —
master set the effective sizes to 0 and the bytes stayed on disk, so an
operator could still extract batches from the file. With pass C truncating to
these bounds, recover-as-empty becomes `set_len(0)` on both files with no
quarantine copy. That makes it the only "cannot interpret these bytes" verdict
in the module that destroys rather than preserves; every other one fences and
keeps the files byte-identical.
That asymmetry matters more than it might look:
- The partition journal is memory-only, so the `.log` is a node's only
durable copy.
- `server_error.rs` tells operators to "repair the quarantined files
offline" — files this arm has already deleted.
- The `warn!` in `truncate_to` says "discarding torn tail bytes" while this
route deletes the entire log, so the log line understates the event an operator
would need to page on.
- Any future batch-layout or checksum change makes existing segments fail to
decode, and a single-segment partition then loses its whole log on first boot.
Renaming aside into the existing `.fenced.N` scheme instead of truncating is
a few lines, and it is also *less* boot I/O than `set_len` plus `sync_all`.
##########
core/common/src/error/iggy_error.rs:
##########
@@ -425,6 +430,11 @@ pub enum IggyError {
InvalidOptionValue(String) = 4042,
#[error("Options block exceeds its limits: {0}")]
OptionsBlockTooLarge(String) = 4043,
+ /// The on-disk segment file length disagrees with the recovered bounds the
+ /// writer was seeded with; appending would corrupt the segment, so the
+ /// open fails instead. Field order: `(on_disk, expected)`.
+ #[error("Segment file size on disk: {0} does not match expected size:
{1}")]
+ SegmentSizeMismatchAtOpen(u64, u64) = 4044,
Review Comment:
**Blocker, and the only item in my review that cannot be corrected after
release.**
I could not find any in-tree path that trips `SegmentSizeMismatchAtOpen`:
- Pass C truncates the file to exactly the size it then hands
`SegmentStorage::new`, so the comparison there cannot fail.
- `hydrate_partition_log` reopens the same paths against the same
`Rc<AtomicU64>` obtained via `size_counter()`, with nothing in between that
mutates a segment, and preallocation uses `FALLOC_FL_KEEP_SIZE` so it cannot
change the logical length. That makes `hydrate_reopen_error`'s mismatch arm
unreachable.
- `state_transfer.rs:2267` matches by construction (`meta.size == entry.len
== bytes.len()`, and `File::create` truncates the staging file, so no oversized
abandoned attempt survives into the rename).
The guard itself is worth keeping — compare-and-refuse is the actual fix for
the clobber, and an assertion on a destructive path earns its keep even when
nothing in tree trips it. What I would push back on is the **discriminant
position**, because that is the part that can never be reclaimed: 4044 is
mirrored into two *generated* SDK tables and Go's is a typed error matched by
`errors.Is`, and by the analysis above this condition cannot cross the wire.
Suggestion: move the variant to 4102 (4101 is `InvalidReservedField`, so
that satisfies the previous mechanical "above the highest code in its range"
rule), regenerate `errors_gen.go`, update `error.code.ts`, and restore the
previous allocation comment. If the guard does not need to be wire-visible at
all, a `debug_assert!` plus a node-fatal generic I/O error would give the same
protection without spending a discriminant.
For the record, the SDK mechanics here are correct: Go and Node are the only
complete mirrors, `errors_gen.go` regenerates byte-identical from
`foreign/go/errors/generator`, skipping Java/C#/Python/C++ matches how prior
codes were added, and 4044 was never previously shipped or retired. My
objection is only to spending the slot.
##########
core/common/src/error/iggy_error.rs:
##########
@@ -22,11 +22,16 @@ use std::sync::Arc;
use strum::{EnumDiscriminants, FromRepr, IntoStaticStr};
use thiserror::Error;
-// A gap in the discriminants is a RETIRED code, not free space. Shipped SDKs
-// keep their own code tables (foreign/go/errors/errors.yaml,
-// foreign/node/src/wire/error.code.ts) that still map the old meaning, and
-// Go's is a typed error matched by errors.Is, so refilling a gap reroutes
-// caller control flow. Allocate above the highest code in its range.
+// Codes are allocated per semantic family: a new code goes one above its
Review Comment:
This rewritten rule does not hold up against the file it governs, and it is
what licenses placing the new code at 4044.
The worked example is wrong in both halves: 4041-4043 are topic-**option**
validation (`UnsupportedOptionKey`, `InvalidOptionValue`,
`OptionsBlockTooLarge`), not "extended message validation", and 4044 is segment
storage, so it is not "one above its family's highest code" for any family
present here. More structurally, the enum carries no family markers at all — no
section comments, no documented ranges — so a rule that requires knowing family
boundaries needs boundaries the file does not record.
The practical risk is the last clause: "the headroom below that base belongs
to the family under it" licenses a future contributor to refill 4045-4049,
which is exactly what the previous wording existed to prevent, and Go's table
matches by `errors.Is` so a refilled gap reroutes caller control flow.
The old rule ("Allocate above the highest code in its range") was mechanical
and checkable. I would restore it and take 4102.
##########
core/server/src/bootstrap.rs:
##########
@@ -1911,13 +1914,16 @@ async fn build_shard_for_thread(
{
Ok(partition) => partition,
// ONE damaged local chain must not take the node down. The shapes
- // this refuses are exactly what a failed state-transfer quarantine
- // leaves behind, so fence that group the same way the runtime path
- // does -- move its segment files aside, keeping the superblock so
it
- // cannot re-enter view 0 -- and materialise it fresh. The ordinary
- // rejoin path (repair, then state transfer on a refused floor)
- // recovers its data from a peer.
- Err(ServerError::PartitionChainRefused { dir, reason, .. }) => {
+ // this refuses are structural -- what a failed state-transfer
+ // quarantine leaves behind, or damage the recovery walk proved
+ // inside a segment -- so fence that group the same way the runtime
+ // path does -- move its segment files aside, keeping the
superblock
+ // so it cannot re-enter view 0 -- and materialise it fresh. The
+ // ordinary rejoin path (repair, then state transfer on a refused
+ // floor) recovers its data from a peer; a single-replica group has
+ // no peer, so it comes back EMPTY while every refused byte stays
+ // in the quarantine directory for the operator.
+ Err(ServerError::PartitionRecoveryRefused { dir, reason, .. }) => {
Review Comment:
**Blocker, scoped to one arm:** `IndexLogDivergence` reaching here is a
behaviour change from master, and at `replica_count = 1` it loses acked data
silently.
Master routed `RecoveredSegmentSizeDivergence` past both arms to `?`, i.e.
node-fatal and loud. This PR fences the partition and calls
`build_partition_fresh`, so with no peer to rebuild from the partition comes
back empty while the node reports healthy. The shape is reachable on the
shipped `enforce_fsync = false`, where nothing orders the message write against
the index write.
Documentation cannot close this one: the existing `TODO(hubcio)` at
`core/server/src/responses.rs:1378-1390` notes that a partition fenced for
rebuild and a freshly materialized one are observationally identical, because
the stats registry carries no materialization signal. So there is provably no
way for an operator or a client to tell the difference.
Tombstoning at `replica_count == 1` — as the superblock arm already does at
`:2012`, and `topology.replica_count` is in scope here — turns it into a signal
instead. An unroutable namespace is diagnosable; an empty served one is not.
I would scope this to the arm whose behaviour this PR changed. The
pre-existing `Hole` / `EmptyNonTailSegment` fence policy is separate debt and
does not need to be settled here.
Two smaller notes on this arm:
`PartitionRecoveryRefusal::StorageSizeMismatch` cannot actually reach it (see
my note on the error variant), and when `quarantine_segment_files` fails the
code tombstones without rebuilding, which the error Display does not admit.
##########
core/server/src/bootstrap.rs:
##########
@@ -2763,6 +2788,40 @@ async fn hydrate_partition_log(
Ok(())
}
+/// Routes a hydrate-reopen writer failure. The seed-vs-stat divergence guard
+/// (`SegmentSizeMismatchAtOpen`) is the same structural contradiction the
+/// recovery walk refuses on -- and the heal path for data directories an
Review Comment:
This rationale is attached to a branch that cannot execute, and it is what
the commit message's headline claim rests on.
By the time `hydrate_partition_log` runs, pass C has already truncated each
file to S and `SegmentStorage::new` has already compared S. Hydrate then
reopens the same paths against the same `Rc<AtomicU64>` returned by
`size_counter()`, and nothing between the two mutates a segment
(`hydrate_applied_purge_generation` only reads `purge.gen`); preallocation uses
`FALLOC_FL_KEEP_SIZE`, so it cannot change the logical length either. So
`SegmentSizeMismatchAtOpen` cannot arrive here.
That makes "the heal path for data directories an earlier size-counter bug
left with resurrected tails" inaccurate — the heal is `truncate_to`, which is
what actually cuts the resurrected tail off an existing data directory. The
commit body's "Nodes already stuck on this bug can boot again" is true, but
because of `truncate_to`, not because of this guard.
Worth correcting so the next person to touch recovery does not reason from a
rationale on dead code. Either delete the arm, or keep it as defence-in-depth
and say so plainly. If it stays, the only honest test for it is injecting an
external truncation between pass C and hydrate.
##########
core/server/src/server_error.rs:
##########
@@ -164,19 +164,23 @@ pub enum ServerError {
},
// Per-partition, not fatal: the boot path fences this one group
(quarantines
// its segment files and materialises it fresh) instead of taking the node
- // down for one damaged local chain. The shapes it reports are exactly
what a
- // failed state-transfer quarantine leaves behind, and the rebuild recovers
- // the data from a peer.
+ // down for one damaged local chain. Only STRUCTURAL refusals route here --
+ // shapes where the local files contradict themselves, so a retried boot
+ // cannot help. Transient recovery I/O failures (stat, open, read,
truncate,
+ // fsync) stay node-fatal on purpose: a retried boot can still serve that
+ // partition, while fencing it would quarantine healthy data.
#[error(
- "partition {stream_id}/{topic_id}/{partition_id} at {dir} recovered an
\
- unusable segment chain: {reason}"
+ "partition {stream_id}/{topic_id}/{partition_id} at {dir} refused
segment \
+ recovery: {reason}. The boot path quarantines this partition's
segment \
+ files beside its directory and rebuilds it empty for the rejoin path;
\
+ restore from a healthy replica, or repair the quarantined files
offline."
Review Comment:
I would delete the "repair the quarantined files offline" clause.
It prescribes a concrete operator action against artifacts this same code
path may have already mutated: pass C truncates every plan before
`SegmentStorage::new` can raise `StorageSizeMismatch`, and pass C is not atomic
across plans, so a refusal on plan N leaves plans 0..N-1 truncated and fsynced.
The recover-as-empty arm deletes its files outright. And there is no tooling to
do the repair with — no fsck or repair binary in `core/cli` or `core/tools`
(only `data-seeder-tool`), and the 24-byte index and batch record layouts are
documented nowhere outside the source.
Incomplete guidance costs an operator time. Guidance that prescribes a wrong
action against silently-altered evidence costs them the forensics the
quarantine exists for.
Two more things in this same message: it promises "rebuilds it empty for the
rejoin path" unconditionally, but `bootstrap.rs:1938-1966` tombstones without
rebuilding when `quarantine_segment_files` fails, and at `replica_count = 1`
there is no rejoin. And it never names `.fenced.N`, which is the one string an
operator needs in order to find the files — `config.toml` names it, this
message does not.
##########
core/server/src/server_error.rs:
##########
@@ -281,14 +273,16 @@ pub enum ServerError {
ShardJoinFailures { failures: Vec<ShardJoinFailure> },
}
-/// Why a recovered segment chain cannot be served.
+/// Why a partition's recovered segments cannot be served.
///
-/// Both shapes mean the same thing operationally -- the local files do not
form
-/// a chain this replica can serve -- but they are distinguished because they
-/// point at different causes: an empty non-tail segment is a failed rebuild's
-/// orphan pairing, a hole is a stray or half-unlinked file.
+/// Every shape here is structural -- the local files contradict themselves or
Review Comment:
This claim does not hold for `OffsetDiscontinuity`, and the mismatch
surfaces to operators as a corruption report for a non-corruption event.
That shape is reachable without any damage: `restore_offset_frontier`
(`iggy_partition.rs:761-780`) raises the counter to the superblock frontier,
which after a crash normally sits above the recovered end offset, and
`ensure_initial_segment` returns early because segments exist
(`partition_helpers.rs:311`). The next append then stamps `base_offset =
frontier` into the *existing* tail segment, producing an offset hole with no
byte hole. Those files are exactly what the writer wrote — they do not
contradict themselves or each other.
To be clear, I think the refusal itself is right and should not be relaxed:
a `break` there would let pass C delete the post-hole batches, which are acked
data. Refusing preserves them. The defect is upstream — boot should seal and
rotate when `restore_offset_frontier` raises the counter above the recovered
end, so no segment ever contains a hole — and that is follow-up work, not this
PR.
So this is just about the wording, plus a note that the enum's own doc
calling every variant structural is what made the upstream bug hard to spot.
##########
core/server/src/segment_recovery.rs:
##########
@@ -52,14 +74,23 @@ pub struct RecoveredSegment {
/// Loads every persisted segment for a partition, sorted by start offset.
///
/// Segment offsets and timestamps are recovered from the 24-byte sparse index
-/// (see module docs); segment byte size comes from the `.log` file. The last
-/// segment is left unsealed so it can accept further writes.
+/// (see module docs); segment byte size comes from walking the `.log` batch
+/// chain. Recovery runs in three passes: every segment is bounded READ-ONLY
+/// first, then the chain guard runs over those bounds, and only an accepted
+/// chain is made physical -- torn tails truncated, index-less indexes rebuilt
+/// -- before storage opens over it. A refusal at any point therefore leaves
+/// every file byte-identical to what boot found. The last segment is left
Review Comment:
This claim is not true for pass C, and the module relies on it elsewhere.
Pass C truncates each plan (log then index) *before* `SegmentStorage::new`
runs, and that open can raise `SegmentSizeMismatchAtOpen`, which is mapped to a
`StorageSizeMismatch` refusal. `hydrate_reopen_error` in `bootstrap.rs` fires
later still, after the whole chain has been truncated. Pass C is also not
atomic across plans: a refusal or an `EIO` on plan N leaves plans 0..N-1
truncated and fsynced.
The property does hold for passes A and B, which is the valuable part and
worth stating precisely — the holed-chain test does verify that a pass-B
refusal leaves even truncation candidates byte-identical. I would scope the
sentence to A and B.
This matters beyond wording because `server_error.rs` tells operators to
repair the quarantined files offline on the strength of it.
##########
core/server/config.toml:
##########
@@ -463,6 +463,14 @@ archive_expired = false
# Unsupported: setting this to `true` aborts boot.
recreate_missing_state = false
+# At boot, segment recovery walks each partition's segments: bytes after the
Review Comment:
Since this is the only operator-facing description of the new destructive
boot path and the repo has no `docs/` directory, I would either correct this
block or drop it from this PR rather than ship it as written.
Three things it overstates:
1. "bytes after the last verifiable batch" — the indexed arm only
header-decodes (`peek_header` → `BatchHeader::decode`, no checksum), per the
TODO in `recover_segment_bounds`. Only the index-less walk verifies checksums.
If the indexed arm gains a checksum this becomes accurate; until then "the last
decodable batch header" is what happens.
2. It does not mention that bytes *before* the last index entry are never
re-examined at boot at all, so at-rest damage inside `[0, last.position)` is
`validate_checksum`'s job on the poll path (already configured a few lines up),
not recovery's. That is a reasonable design, but it is the honest scope.
3. "the partition is rebuilt from replicas" omits `replica_count = 1`, where
there is no peer and the partition comes back EMPTY. `bootstrap.rs` states this
in a code comment; the operator-facing doc does not.
Also, "The metadata WAL is stricter and refuses boot instead" reads as
though the WAL never truncates. `prepare_journal.rs:240-247` does truncate a
torn WAL tail; only interior damage (or trailing bytes above `MAX_ENTRY_SIZE`)
refuses. "Interior WAL damage refuses boot" would be accurate — and that
`MAX_ENTRY_SIZE` refusal is the width-cap precedent worth reusing in
`probe_for_survivor`.
##########
core/partitions/src/messages_writer.rs:
##########
@@ -256,4 +271,40 @@ mod tests {
assert_eq!(writer.file.metadata().await.unwrap().len(), 0);
}
+
+ #[compio::test]
+ async fn
given_seeded_size_matching_disk_when_opening_existing_file_should_keep_counter()
{
Review Comment:
This test passes on master too, so it cannot fail in either direction.
Master's `messages_size_bytes.store(actual_messages_size, ...)` also leaves
the counter at 128 when the file is 128 bytes and the seed was 128, so the
assertion holds whether or not the fix is present. The divergence test right
below it is the one that pins the new behaviour.
In a PR whose purpose is regression-proofing this path, a test that cannot
fail is worth removing rather than keeping. (The other 15 new tests all look
sound to me — two of them, the clean-segment and second-recovery cases, also
pass on master but legitimately guard the new code against over-truncating,
which is different.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
