fix: HLC is ahead of wall clock. iscp failed to update - #24765
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
1 task
XuPeng-SH
requested changes
Jun 1, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
Request Changes — one blocking issue
The fix for HandleGetChangedTableList is correct and necessary, but the same bug class exists in HandleBackup and should be fixed here too.
🔴 Must fix: HandleBackup at handle_debug.go:683 uses wall-clock TS
// handle_debug.go:683 (current code, outside the diff)
backupTime := time.Now().UTC()
currTs := types.BuildTS(backupTime.UnixNano(), 0) // ❌ wall clockThis has the exact same bug that this PR fixes in HandleGetChangedTableList. When HLC advances past wall clock (due to logtail replay or cross-node sync), currTs will be less than the true HLC, meaning:
ForceCheckpoint(ctx, currTs)only flushes up to the wall clock timestamp — committed data between wall clock and HLC may be omitted- If the backup tool reads checkpoint data using a higher TS, data committed between wall clock and HLC is missing from the backup
The file already has the correct pattern in HandleForceCheckpoint (line 655):
now, err := h.db.TxnMgr.Now() // ✅ HLCFix: change line 683 to use h.db.TxnMgr.Now() instead of BuildTS(time.Now().UTC().UnixNano(), 0).
Comment (non-blocking): HandleDiskCleaner at handle_debug.go:890
ts := types.BuildTS(time.Now().UTC().UnixNano()-int64(ttl), 0)This uses wall clock for a TTL threshold compared against HLC checkpoint endTS. When HLC > wall clock, the threshold is lower than intended, resulting in less data being cleaned — so it errs on the safe side (over-retention). This is a resource/efficiency issue rather than a correctness one, but worth fixing for consistency.
Summary
| Item | Verdict |
|---|---|
HandleGetChangedTableList fix |
✅ Correct |
HandleBackup wall-clock bug |
🔴 Must fix in this PR |
HandleDiskCleaner TTL |
🟡 Non-blocking comment |
| Test coverage |
matrix-meow
added
size/M
Contributor
Merge Queue Status
This pull request spent 1 hour 24 minutes 41 seconds in the queue, including 1 hour 22 minutes 40 seconds running CI. Required conditions to merge
|
Contributor
Author
|
@Mergifyio refresh |
Contributor
✅ Pull request refreshed |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24764
What this PR does / why we need it:
Avoid using wall clock and use HLC as a common time.
ISCP failed to update when startTs > 0 and HLC is ahead of wall clock.