fix(consensus): keep a client's dedup fence across capacity eviction - #3934
fix(consensus): keep a client's dedup fence across capacity eviction#3934numinnex wants to merge 2 commits into
Conversation
|
Merge after #3931 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3934 +/- ##
============================================
- Coverage 83.87% 83.56% -0.31%
Complexity 1358 1358
============================================
Files 1212 1212
Lines 166843 166741 -102
Branches 134306 134329 +23
============================================
- Hits 139937 139345 -592
- Misses 23266 23614 +348
- Partials 3640 3782 +142
🚀 New features to boost your workflow:
|
| /// state machine ran the operation a second time. | ||
| #[iggy_harness(cluster_nodes = 1, server(metadata.clients_table_max = "2"))] | ||
| async fn given_a_low_client_table_cap_when_connects_churn_should_erase_a_live_dedup_watermark( | ||
| async fn given_a_low_client_table_cap_when_connects_churn_should_keep_a_live_dedup_watermark( |
There was a problem hiding this comment.
This test currently fails with the original duplicate-name rejection, proving request 1 was re-executed. The PR should not unignore this spec until it passes end to end.
| @@ -830,6 +907,13 @@ impl ClientTable { | |||
| /// | |||
| /// [`Operation::Register`]: iggy_binary_protocol::Operation | |||
| pub fn remove_client(&mut self, client_id: u128) -> bool { | |||
There was a problem hiding this comment.
This removes every fence sharing client_id, including fences belonging to another user. It also erases the evicted client’s fence when disconnect cleanup commits Logout before resume, which is why the integration test fails. Fence removal needs the owning user_id and must distinguish explicit session termination from reconnect cleanup.
| /// most, the watermark request's own reply, so it costs a fraction of the | ||
| /// entry it replaces. Trimmed oldest-first. | ||
| /// | ||
| /// Replica-local best-effort, NOT replicated state: the bound is |
There was a problem hiding this comment.
The watermark is safety state, not a volatile reply cache. Dropping these fences on checkpoint or state transfer restores the original re-execution bug after restart or failover and lets replicas derive different watermarks from the same register commit. Please include fences in snapshot and state-transfer encoding.
| watermark_checksum: entry.watermark_checksum, | ||
| latest: entry.find_cached(entry.watermark).cloned(), | ||
| }); | ||
| while self.evicted_fences.len() > self.slots.len() { |
There was a problem hiding this comment.
Silently trimming the oldest fence after slots.len() active-client evictions only postpones the at-most-once failure. Please define and test the retention guarantee, or use durable bounded tombstones with explicit expiry rather than silently restoring watermark zero.
| // Reclaim the replies, keep the fence: the evicted client's own resume | ||
| // must not read as a first-time register, or the retry of a committed | ||
| // request re-executes. | ||
| self.remember_fence(&entry); |
There was a problem hiding this comment.
This snapshots the watermark at eviction time. If an already-prepared request commits afterward, commit_reply finds no entry and cannot advance this fence. A later resume restores the older watermark and may re-execute that committed request. Please add this ordering test and update the retained fence when the late commit lands.
github-actions
Bot
added
S-waiting-on-author
2 participants
No description provided.