Conversation
- Add tsWalRecoveryPolicy global variable (default 0) - Only affects single replica: 0=refuse, 1=delete corrupted - Three replicas always auto-recover regardless of this config
- Add replica parameter to walCheckAndRepairMeta function - Temporarily pass replica=1 in walOpen (will be updated in next task)
- Three replicas: always auto-recover (ignore walRecoveryPolicy) - Single replica: controlled by walRecoveryPolicy - 0 (default): refuse to start, preserve corrupted WAL - 1: delete corrupted part and try to start - Add detailed error messages for single replica refusal
- Add replica parameter to walOpen function - Pass actual replica count from vnode configuration - Pass replica count from mnode syncMgmt - Enable replica-aware recovery policy decision
- Add syncNotifyWalTruncated interface in sync module - Call notification after three-replica recovery - Enable Raft to detect and sync missing logs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a WAL recovery policy and enhances corruption handling by incorporating replica counts into the WAL opening process. Key changes include the addition of the tsWalRecoveryPolicy configuration and a notification mechanism for the sync module when WAL truncation occurs. Review feedback identifies critical issues where the notification logic is unreachable due to early returns and the recovery logic is overly specific to three-replica setups. Improvements are also suggested regarding the use of header files instead of extern declarations and the need for a more functional implementation of the sync notification to properly handle log synchronization after truncation.
| if (shouldRecover && tsWalDeleteOnCorruption) { | ||
| TAOS_RETURN(walRenameCorruptedDir(pWal)); | ||
| } | ||
| goto _exit; |
There was a problem hiding this comment.
There are two issues here:
- The notification to the sync module (intended at line 680) is unreachable because this block either returns via
TAOS_RETURNor jumps to_exitbefore reaching the end of the function. - For multi-replica setups, the log at line 620 claims auto-recovery is enabled, but the actual recovery is still gated by
tsWalDeleteOnCorruption(which defaults to false). If recovery is intended to be automatic for clusters, this guard should be adjusted.
if (shouldRecover && (tsWalDeleteOnCorruption || replica > 1)) {
if (replica > 1) {
syncNotifyWalTruncated(pWal->cfg.vgId, truncatedVer);
}
TAOS_RETURN(walRenameCorruptedDir(pWal));
}
goto _exit;| if (walTruncated && replica == 3) { | ||
| int32_t syncCode = syncNotifyWalTruncated(pWal->cfg.vgId, truncatedVer); | ||
| if (syncCode != TSDB_CODE_SUCCESS) { | ||
| wWarn("vgId:%d, failed to notify sync module, code:0x%x", pWal->cfg.vgId, syncCode); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block is unreachable when walTruncated is true. As noted in the previous comment, any path that detects corruption and sets walTruncated = true results in an early exit from the function. The notification logic should be moved into the recovery block where the actual truncation/renaming occurs.
| #include "tutil.h" | ||
| #include "walInt.h" | ||
|
|
||
| extern int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer); |
There was a problem hiding this comment.
Instead of using an extern declaration inside a source file, it is better practice to include the header file where the function is declared. This ensures type safety and a single source of truth for the function signature.
#include "sync.h"| if (tsWalDeleteOnCorruption) { | ||
|
|
||
| bool shouldRecover = false; | ||
| if (replica == 3) { |
There was a problem hiding this comment.
The check for replica == 3 is too specific. TDengine supports other replica counts (e.g., 2 or 5). It is better to check if the node is part of a multi-replica group.
if (replica > 1) {| int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer) { | ||
| sInfo("vgId:%d, notified sync module: WAL truncated to ver:%" PRId64, vgId, truncatedVer); | ||
| return TSDB_CODE_SUCCESS; | ||
| } |
There was a problem hiding this comment.
The implementation of syncNotifyWalTruncated currently only logs a message. To fulfill the PR's objective of 'Enabling Raft to detect and sync missing logs', this function should update the state of the corresponding sync node (e.g., resetting its applied index or marking it as needing a snapshot/sync).
There was a problem hiding this comment.
Pull request overview
This PR extends WAL open/repair behavior with replica-aware recovery handling and introduces a sync-module notification hook intended to let Raft detect/sync missing logs after WAL truncation during recovery.
Changes:
- Extend
walOpen()/walCheckAndRepairMeta()to take areplicacount and use it to decide corruption recovery behavior. - Add
walRecoveryPolicyserver config to control single-replica corruption recovery behavior. - Add
syncNotifyWalTruncated()API and invoke it from WAL repair logic (replica=3 path).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| source/libs/wal/src/walMgmt.c | Extends walOpen() to accept replica count and passes it into meta repair. |
| source/libs/wal/src/walMeta.c | Adds replica-aware corruption handling and calls syncNotifyWalTruncated() after truncation. |
| source/libs/wal/inc/walInt.h | Updates internal prototype for walCheckAndRepairMeta(). |
| include/libs/wal/wal.h | Updates public walOpen() signature to include replica count. |
| source/dnode/vnode/src/vnd/vnodeOpen.c | Passes vnode replica count into walOpen(). |
| source/dnode/mnode/impl/src/mndMain.c | Passes mnode replica count into walOpen(). |
| source/common/src/tglobal.c | Adds walRecoveryPolicy global + config plumbing. |
| include/common/tglobal.h | Exposes tsWalRecoveryPolicy with documentation. |
| source/libs/sync/src/syncMain.c | Adds syncNotifyWalTruncated() implementation (currently logs only). |
| source/libs/sync/inc/syncInt.h | Declares internal syncNotifyWalTruncated() symbol. |
| include/libs/sync/sync.h | Exposes syncNotifyWalTruncated() in public sync API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| if (shouldRecover && tsWalDeleteOnCorruption) { |
There was a problem hiding this comment.
The new "auto-recovery enabled for replica=3" path is inconsistent: when replica==3 and WAL scan fails, the code only recovers if tsWalDeleteOnCorruption is enabled; otherwise it falls through to _exit with an error code, causing walOpen() to fail despite logging that auto-recovery is enabled. Either perform the recovery action unconditionally for replica==3 (still preserving the corrupted dir), or make the log/error handling reflect the actual required config and set a deterministic return code.
| if (shouldRecover && tsWalDeleteOnCorruption) { | |
| if (replica == 3 || (shouldRecover && tsWalDeleteOnCorruption)) { |
| } | ||
|
|
||
| if (shouldRecover && tsWalDeleteOnCorruption) { | ||
| TAOS_RETURN(walRenameCorruptedDir(pWal)); |
There was a problem hiding this comment.
When recovery triggers TAOS_RETURN(walRenameCorruptedDir(pWal)), the function returns before reaching the later syncNotifyWalTruncated() block, so the sync module is never notified in the main recovery path. If notification is required for replica=3 recovery, move the notification before the early return (or avoid returning early).
| TAOS_RETURN(walRenameCorruptedDir(pWal)); | |
| code = walRenameCorruptedDir(pWal); |
| extern int64_t tsWalFsyncDataSizeLimit; | ||
| extern bool tsWalForceRepair; | ||
| extern bool tsWalDeleteOnCorruption; | ||
| // WAL recovery policy (only affects single replica) |
There was a problem hiding this comment.
Header comment says walRecoveryPolicy "only affects single replica", but the new logic checks it for all replica != 3 cases (so it would also affect replica=2 if that configuration exists). Please align the comment (and possibly the config semantics) with actual behavior to avoid misconfiguration.
| // WAL recovery policy (only affects single replica) | |
| // WAL recovery policy (applies when replica count is not 3, including single-replica deployments) |
|
|
||
| // handle open and ctl | ||
| SWal *walOpen(const char *path, SWalCfg *pCfg); | ||
| SWal *walOpen(const char *path, SWalCfg *pCfg, int32_t replica); |
There was a problem hiding this comment.
walOpen signature change is not fully propagated: multiple test call sites still use the old 2-arg form (e.g. source/libs/wal/test/walMetaTest.cpp and several source/libs/sync/test/*.cpp), which will break compilation. Consider either updating all callers in this PR or providing a backward-compatible wrapper API (e.g. keep walOpen(path,cfg) and introduce a new function / default replica handling).
| SWal *walOpen(const char *path, SWalCfg *pCfg, int32_t replica); | |
| SWal *walOpen(const char *path, SWalCfg *pCfg, int32_t replica); | |
| static inline SWal *walOpenDefaultReplica(const char *path, SWalCfg *pCfg) { return walOpen(path, pCfg, 0); } | |
| #define WAL_OPEN_SELECT(_1, _2, _3, NAME, ...) NAME | |
| #define walOpen(...) WAL_OPEN_SELECT(__VA_ARGS__, walOpen, walOpenDefaultReplica)(__VA_ARGS__) |
| if (walTruncated && replica == 3) { | ||
| int32_t syncCode = syncNotifyWalTruncated(pWal->cfg.vgId, truncatedVer); | ||
| if (syncCode != TSDB_CODE_SUCCESS) { | ||
| wWarn("vgId:%d, failed to notify sync module, code:0x%x", pWal->cfg.vgId, syncCode); | ||
| } | ||
| } |
There was a problem hiding this comment.
walCheckAndRepairMeta() now calls syncNotifyWalTruncated(), but the wal static library does not link against sync (and sync already links against wal), so this introduces a circular/undefined-symbol risk (e.g. walTest links wal without sync). Replace this direct call with a decoupled mechanism such as a callback function pointer registered by sync at runtime, or a weak/no-op default implementation compiled into wal.
- Remove syncNotifyWalTruncated function and calls - Three-replica recovery works without explicit sync notification - Raft will automatically detect and sync missing logs
…L truncation - Remove tsWalDeleteOnCorruption parameter (no longer needed) - Implement proper WAL truncation instead of renaming entire directory - Delete corrupted WAL files from corruption point onwards - Update WAL metadata to reflect truncation - Ensure Raft can write with correct version after recovery
…engine into feat/336/wal-recover
- Add walTruncateCorruptedFiles() function to handle all truncation logic - Consolidate replica-based recovery policy decision in one place - Call truncation function from both walCheckAndRepairMeta and walLogEntriesComplete - Ensure consistent truncation behavior across all corruption scenarios
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool walTruncated = false; | ||
| int64_t truncatedVer = -1; |
There was a problem hiding this comment.
walTruncated and truncatedVer are declared but never used, which is dead code and can trigger -Wunused-variable warnings in some build configurations. Either remove them or use them for the intended truncation notification/metadata tracking (e.g., record truncatedVer and propagate it to the sync module).
| bool walTruncated = false; | |
| int64_t truncatedVer = -1; |
| code = walTruncateCorruptedFiles(pWal, fileIdx, replica); | ||
| if (code != TSDB_CODE_SUCCESS) { | ||
| goto _exit; | ||
| } | ||
| goto _exit; | ||
| } | ||
| // empty log file | ||
| lastVer = pFileInfo->firstVer - 1; | ||
|
|
||
| code = TSDB_CODE_SUCCESS; | ||
| // After truncation, set lastVer based on remaining files | ||
| lastVer = (fileIdx > 0) ? ((SWalFileInfo*)taosArrayGet(pWal->fileInfoSet, fileIdx - 1))->lastVer : -1; | ||
| wInfo("vgId:%d, WAL truncated, new lastVer:%" PRId64, pWal->cfg.vgId, lastVer); | ||
| updateMeta = true; | ||
| code = TSDB_CODE_SUCCESS; | ||
| } else { | ||
| // empty log file | ||
| lastVer = pFileInfo->firstVer - 1; | ||
| code = TSDB_CODE_SUCCESS; | ||
| } | ||
| } | ||
| wInfo("vgId:%d, repaired file %s, last index:%" PRId64 ", fileSize:%" PRId64 ", fileSize in meta:%" PRId64, | ||
| pWal->cfg.vgId, fnameStr, lastVer, fileSize, pFileInfo->fileSize); | ||
|
|
||
| // update lastVer | ||
| pFileInfo->lastVer = lastVer; | ||
| totSize += pFileInfo->fileSize; | ||
| if (code == TSDB_CODE_SUCCESS && lastVer >= 0) { | ||
| wInfo("vgId:%d, repaired file %s, last index:%" PRId64 ", fileSize:%" PRId64 ", fileSize in meta:%" PRId64, | ||
| pWal->cfg.vgId, fnameStr, lastVer, fileSize, pFileInfo->fileSize); | ||
|
|
||
| // update lastVer | ||
| pFileInfo->lastVer = lastVer; | ||
| totSize += pFileInfo->fileSize; | ||
| } |
There was a problem hiding this comment.
In the lastVer<0 && code!=TSDB_CODE_WAL_LOG_NOT_EXIST branch, walTruncateCorruptedFiles() mutates pWal->fileInfoSet (removes entries starting at fileIdx). After that, pFileInfo still points to the removed element, but the code can still enter the update block and write pFileInfo->lastVer / use pFileInfo->fileSize, which becomes invalid memory access. Also, totSize may already include sizes of files that just got deleted. After truncation, avoid using the old pFileInfo pointer and recompute totSize/versions from the remaining fileInfoSet (e.g., restart the scan or break and rebuild totals).
|
|
||
| int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer) { | ||
| sInfo("vgId:%d, notified sync module: WAL truncated to ver:%" PRId64, vgId, truncatedVer); | ||
| return TSDB_CODE_SUCCESS; | ||
| } |
There was a problem hiding this comment.
syncNotifyWalTruncated() is added to public headers, but there are no call sites in the repo. If the intent is to notify Raft after WAL truncation/recovery, wire this into the WAL recovery path (e.g., after walTruncateCorruptedFiles succeeds) and pass the actual truncated version; otherwise keep the symbol internal or remove it to avoid exposing an unused API surface.
| int32_t syncNotifyWalTruncated(int32_t vgId, int64_t truncatedVer) { | |
| sInfo("vgId:%d, notified sync module: WAL truncated to ver:%" PRId64, vgId, truncatedVer); | |
| return TSDB_CODE_SUCCESS; | |
| } |
|
|
||
| TAOS_CHECK_EXIT(walLogEntriesComplete(pWal)); | ||
| TAOS_CHECK_EXIT(walLogEntriesComplete(pWal, replica)); | ||
|
|
There was a problem hiding this comment.
walLogEntriesComplete() can now mutate pWal->fileInfoSet by truncating WAL files, but walCheckAndRepairMeta() doesn't recompute pWal->vers/totSize or persist meta after this call. If truncation happens here, pWal->vers.lastVer (and the on-disk meta) can remain stale/inconsistent with the new file set. Consider having walLogEntriesComplete() report whether truncation occurred (or return a distinct code), then re-run version alignment + walSaveMeta() (and adjust totSize) when truncation happens.
| actualFileNum = taosArrayGetSize(pWal->fileInfoSet); | |
| if (actualFileNum > 0) { | |
| pWal->vers.firstVer = ((SWalFileInfo*)taosArrayGet(pWal->fileInfoSet, 0))->firstVer; | |
| pWal->vers.lastVer = ((SWalFileInfo*)taosArrayGetLast(pWal->fileInfoSet))->lastVer; | |
| } else { | |
| pWal->vers.firstVer = -1; | |
| pWal->vers.lastVer = -1; | |
| } | |
| walAlignVersions(pWal); | |
| TAOS_CHECK_EXIT(walSaveMeta(pWal)); |
| } else { | ||
| TAOS_RETURN(TSDB_CODE_WAL_LOG_INCOMPLETE); | ||
| } | ||
| ", snaphot index:%" PRId64 ", fileIdx:%d", |
There was a problem hiding this comment.
Typo in log message: "snaphot" should be "snapshot".
| ", snaphot index:%" PRId64 ", fileIdx:%d", | |
| ", snapshot index:%" PRId64 ", fileIdx:%d", |
| if (replica == 3) { | ||
| shouldRecover = true; | ||
| SWalFileInfo* pFileInfo = taosArrayGet(pWal->fileInfoSet, fileIdx); | ||
| wInfo("vgId:%d, WAL corrupted at ver:%" PRId64 ", auto-recovery enabled for replica=3", | ||
| pWal->cfg.vgId, pFileInfo->firstVer); | ||
| } else { | ||
| shouldRecover = (tsWalRecoveryPolicy == 1); | ||
| if (shouldRecover) { | ||
| SWalFileInfo* pFileInfo = taosArrayGet(pWal->fileInfoSet, fileIdx); | ||
| wWarn("vgId:%d, WAL corrupted at ver:%" PRId64 ", force recovery enabled by walRecoveryPolicy=1", | ||
| pWal->cfg.vgId, pFileInfo->firstVer); | ||
| } else { | ||
| SWalFileInfo* pFileInfo = taosArrayGet(pWal->fileInfoSet, fileIdx); | ||
| wError("vgId:%d, WAL corrupted at ver:%" PRId64 ", refusing to start to prevent data loss", | ||
| pWal->cfg.vgId, pFileInfo->firstVer); | ||
| wError("vgId:%d, corrupted WAL files are preserved for manual inspection", pWal->cfg.vgId); | ||
| wError("vgId:%d, to force recovery with data loss, set 'walRecoveryPolicy 1' in taos.cfg and restart", | ||
| pWal->cfg.vgId); |
There was a problem hiding this comment.
walTruncateCorruptedFiles() doesn't validate fileIdx before calling taosArrayGet(pWal->fileInfoSet, fileIdx) and before computing the remove batch length (size - fileIdx). If fileIdx is negative or >= array size, this can lead to invalid memory access or a huge batch removal due to underflow. Add defensive checks at the start of this function and return an appropriate error when fileIdx is out of range.
| } | ||
| ", snaphot index:%" PRId64 ", fileIdx:%d", | ||
| pWal->cfg.vgId, pWal->vers.firstVer, pWal->vers.lastVer, index, pWal->vers.snapshotVer, fileIdx); | ||
| TAOS_RETURN(walTruncateCorruptedFiles(pWal, fileIdx, replica)); |
There was a problem hiding this comment.
walTruncateCorruptedFiles() assumes fileIdx is a valid index into pWal->fileInfoSet (taosArrayGet/taosArrayRemoveBatch). In walLogEntriesComplete(), fileIdx can become == sz when the loop completes without breaking, which would make taosArrayGet out-of-bounds and can crash. Add explicit range checks (fileIdx >= 0 && fileIdx < taosArrayGetSize(...)) before using fileIdx, and decide a safe truncation point (or return an error) when the inconsistency is only in meta/vers rather than a specific corrupted file.
| TAOS_RETURN(walTruncateCorruptedFiles(pWal, fileIdx, replica)); | |
| if (fileIdx >= 0 && fileIdx < sz) { | |
| TAOS_RETURN(walTruncateCorruptedFiles(pWal, fileIdx, replica)); | |
| } | |
| wError("vgId:%d, WAL metadata/version inconsistency detected without a valid corrupted file index, " | |
| "skip truncation, fileCnt:%d, fileIdx:%d", | |
| pWal->cfg.vgId, sz, fileIdx); | |
| TAOS_RETURN(TSDB_CODE_WAL_FILE_CORRUPTED); |
…eting - Read idx file to locate last valid log entry - Read log entry header to calculate exact truncation offset - Truncate both log and idx files to preserve valid data - Update fileInfo metadata after truncation - Ensure Raft can continue writing from correct position
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.