Skip to content

fix(server): flush trailing duplicate batch when last mutation is ignored - #2408

Open
cauchy1988 wants to merge 1 commit into
apache:masterfrom
cauchy1988:fix/duplication-ignored-last-mutation
Open

cauchy1988 wants to merge 1 commit into
apache:masterfrom
cauchy1988:fix/duplication-ignored-last-mutation

Conversation

@cauchy1988

Copy link
Copy Markdown
Contributor

Problem

pegasus_mutation_duplicator::duplicate() iterates the mutation set (sorted by timestamp), accumulating entries into a batch and flushing it when the batch-byte threshold is reached or the last mutation is seen. Mutations whose rpc code is RPC_RRDB_RRDB_DUPLICATE or RPC_RRDB_RRDB_BULK_LOAD are intentionally not duplicated.

The ignored-code path used continue, which also skipped the batch-flush check. So when the last mutation in the set was an ignored code, the in-progress batch of valid mutations was never moved to _inflights and was silently dropped. Because the caller has already advanced last_decree past the whole set, those mutations were never re-read and never duplicated — silent data loss (reported in #2284, with duplicate_log_batch_bytes > 0).

for (auto mut : muts) {
    batch_count++;
    ...
    if (gutil::ContainsKey(ingnored_rpc_code, rpc_code)) {
        ...
        continue;          // <-- skips the flush check below
    }
    ... add entry to batch ...
    if (batch_count == muts.size() || batch_bytes >= FLAGS_duplicate_log_batch_bytes || ...) {
        ... flush batch to _inflights ...   // <-- never reached if the last mutation was ignored
    }
}

Fix

  1. Replace continue with if/else so the flush check always runs — the trailing batch is now flushed even when the last mutation is ignored.
  2. Flush only when the batch is non-empty (an ignored mutation with an empty trailing batch is a no-op, preserving the all-ignored → cb(0) path covered by the existing duplicate_duplicate test).
  3. Compute the batch's routing hash from the last entry in the batch (always a duplicatable code: PUT/REMOVE/MULTI_*), not the current (possibly ignored) mutation — get_hash_from_request would LOG_FATAL-abort on DUPLICATE/BULK_LOAD.

Testing

Added regression test duplicate_ignored_last_mutation: 3 valid PUTs followed by an ignored BULK_LOAD with the highest timestamp (iterated last). It fails before the fix (mail_box empty — the trailing PUT batch is dropped) and passes after.

Full duplicator suite passes (16/16):

[ RUN      ] pegasus_mutation_duplicator_test.duplicate_ignored_last_mutation/0
[       OK ] pegasus_mutation_duplicator_test.duplicate_ignored_last_mutation/0 (1 ms)
[ RUN      ] pegasus_mutation_duplicator_test.duplicate_ignored_last_mutation/1
[       OK ] pegasus_mutation_duplicator_test.duplicate_ignored_last_mutation/1 (2 ms)
...
[==========] 16 tests from 1 test suite ran. (6130 ms total)
[  PASSED  ] 16 tests.

Existing tests (duplicate, duplicate_failed, duplicate_isolated_hashkeys, duplicate_duplicate, get_hash_from_request, read_after_get_hash_key, create_duplicator) all still pass — no regression.

Closes #2284

@github-actions github-actions Bot added the cpp label Jun 14, 2026
…ored

pegasus_mutation_duplicator::duplicate() iterates the mutation set
(sorted by timestamp), accumulating entries into a batch and flushing it
when the batch-byte threshold is reached or the last mutation is seen.
Mutations whose rpc code is RPC_RRDB_RRDB_DUPLICATE or
RPC_RRDB_RRDB_BULK_LOAD are intentionally not duplicated.

The ignored-code path used `continue`, which skipped the batch-flush
check too. So when the LAST mutation in the set was an ignored code, the
in-progress batch of valid mutations was never moved to _inflights and
was silently dropped. Because the caller has already advanced last_decree
past the whole set, those mutations were never re-read and never
duplicated — silent data loss (reported in apache#2284, with
duplicate_log_batch_bytes > 0).

Fix:
- Replace `continue` with if/else so the flush check always runs.
- Flush only when the batch is non-empty (an ignored mutation with an
  empty trailing batch is a no-op).
- Compute the batch's routing hash from the last ENTRY in the batch
  (always a duplicatable code), not the current (possibly ignored)
  mutation: get_hash_from_request LOG_FATAL-aborts on DUPLICATE/BULK_LOAD.

Added a regression test (duplicate_ignored_last_mutation) that fails
before the fix (the trailing batch is dropped, nothing is shipped) and
passes after.

Closes apache#2284

Co-Authored-By: Claude <noreply@anthropic.com>
@cauchy1988
cauchy1988 force-pushed the fix/duplication-ignored-last-mutation branch from fc68cb0 to bc7e62d Compare June 14, 2026 06:54

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG(duplication): When "duplicate_log_batch_bytes" is greater than 0 , there is a probability that some data will be lost.

1 participant