hubcio commented on code in PR #3269:
URL: https://github.com/apache/iggy/pull/3269#discussion_r3263411709
##########
core/journal/src/prepare_journal.rs:
##########
@@ -387,6 +437,18 @@ impl Journal<FileStorage> for PrepareJournal {
// Reopen the file descriptor at the same path.
self.storage.reopen().await?;
+ // Advance the snapshot watermark only AFTER the WAL rewrite is
+ // durable (tmp create -> write -> fsync -> rename -> fsync parent
+ // -> reopen). Advancing earlier would leave `snapshot_op` past
+ // entries still present on disk on any `?` failure above, letting
+ // a future `append()` pass the slot collision check at
+ // `existing.op <= snapshot_op` and silently evict a live entry
+ // from the index. The entry would survive on disk but become
+ // unreachable, stalling `RetransmitPrepares` until view change.
+ if end_op > self.snapshot_op.get() {
+ self.snapshot_op.set(end_op);
Review Comment:
yep, exact framing. the failure mode is the slot-collision check at
`existing.op <= snapshotpares. caught it while staring at the drain ordering;
no existing test reproduced it because all tests crash-injected before rename,
not after watermark.
you know, there is so much to do under clustering feature to we kind of have
to do such big changes and possibly unrelated changes.
##########
core/journal/src/prepare_journal.rs:
##########
@@ -387,6 +437,18 @@ impl Journal<FileStorage> for PrepareJournal {
// Reopen the file descriptor at the same path.
self.storage.reopen().await?;
+ // Advance the snapshot watermark only AFTER the WAL rewrite is
+ // durable (tmp create -> write -> fsync -> rename -> fsync parent
+ // -> reopen). Advancing earlier would leave `snapshot_op` past
+ // entries still present on disk on any `?` failure above, letting
+ // a future `append()` pass the slot collision check at
+ // `existing.op <= snapshot_op` and silently evict a live entry
+ // from the index. The entry would survive on disk but become
+ // unreachable, stalling `RetransmitPrepares` until view change.
+ if end_op > self.snapshot_op.get() {
+ self.snapshot_op.set(end_op);
Review Comment:
yep, exact framing. the failure mode is the slot-collision check at
`existing.op <= snapshotpares. caught it while staring at the drain ordering;
no existing test reproduced it because all tests crash-injected before rename,
not after watermark.
you know, there is so much to do under clustering feature to we kind of have
to do such big and possibly unrelated changes.
--
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]
