Add auto-repair options for broken AOF tail on startup#7
Conversation
📝 WalkthroughWalkthroughThis change introduces automatic AOF corruption recovery functionality to Redis. Two new configuration options ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aof.c (1)
1893-1895:⚠️ Potential issue | 🟠 MajorBug:
stopLoadingdoes not treatAOF_BROKEN_RECOVEREDas success.When AOF recovery succeeds with
AOF_BROKEN_RECOVERED, thestopLoadingcall marks it as a failure because onlyAOF_OKandAOF_TRUNCATEDare considered successful outcomes. This means successful broken-tail recovery will incorrectly report loading failure.Proposed fix
cleanup: - stopLoading(ret == AOF_OK || ret == AOF_TRUNCATED); + stopLoading(ret == AOF_OK || ret == AOF_TRUNCATED || ret == AOF_BROKEN_RECOVERED); return ret; }
🤖 Fix all issues with AI agents
In `@tests/integration/aof.tcl`:
- Around line 723-726: The test currently uses "exec rm -f
$aof_dirpath/appendonly.aof.*" which does not glob in Tcl; replace that with a
glob-based removal and manifest rewrite: use "glob -nocomplain
$aof_dirpath/appendonly.aof.*" to collect incremental AOF files then "file
delete" each returned path so only the base AOF remains, and update the manifest
(appendonly.aof.manifest) in the same directory to list only the base file
(ensure start_server_aof/test "Corrupted base AOF (last file): should recover"
sees a base-only manifest); keep references to $aof_dirpath, appendonly.aof.*,
and the manifest file when making the change.
🧹 Nitpick comments (3)
src/server.h (1)
15-16: Header guard still uses reserved identifier namespace.The change from
__REDIS_Hto_REDIS_His an improvement, but identifiers starting with an underscore followed by an uppercase letter are still reserved by the C standard (C99 §7.1.3) in the global namespace. Consider usingREDIS_SERVER_HorREDIS_Hwithout a leading underscore for full compliance.This is a minor concern since the pattern is common in practice and unlikely to cause issues.
♻️ Suggested fix
-#ifndef _REDIS_H -#define _REDIS_H +#ifndef REDIS_SERVER_H +#define REDIS_SERVER_HNote: The corresponding
#endifat the end of the file would not need to change.src/config.c (1)
3257-3259: Consider allowing memory-unit suffixes foraof-load-broken-max-size.
UsingINTEGER_CONFIGrejects values like4kbeven though this is a byte-size option. Switching toMEMORY_CONFIGpreserves defaults and aligns with other size configs.Proposed change
- createOffTConfig("aof-load-broken-max-size", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.aof_load_broken_max_size, 4*1024, INTEGER_CONFIG, NULL, NULL), + createOffTConfig("aof-load-broken-max-size", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.aof_load_broken_max_size, 4*1024, MEMORY_CONFIG, NULL, NULL),src/aof.c (1)
1830-1843: Inconsistency:AOF_BROKEN_RECOVEREDhandling differs between BASE and INCR files.Line 1841 includes
AOF_BROKEN_RECOVEREDin thegoto cleanupcondition, but line 1875 (INCR handling) does not. This causes asymmetric behavior:
- For INCR files:
AOF_BROKEN_RECOVEREDon last file proceeds normally through cleanup- For BASE files:
AOF_BROKEN_RECOVEREDalways jumps to cleanup, even when it's the last file and recovery succeededIf the BASE file is the only file (no INCR files), this still works, but the explicit inclusion of
AOF_BROKEN_RECOVEREDhere appears inconsistent with the INCR handling pattern.Suggested fix for consistency
- if (ret == AOF_OPEN_ERR || ret == AOF_FAILED || ret == AOF_BROKEN_RECOVERED) { + if (ret == AOF_OPEN_ERR || ret == AOF_FAILED) { goto cleanup; }
| # Remove all incr AOF files to make the base file being the last file | ||
| exec rm -f $aof_dirpath/appendonly.aof.* | ||
| start_server_aof [list dir $server_path aof-load-broken yes] { | ||
| test "Corrupted base AOF (last file): should recover" { |
There was a problem hiding this comment.
Fix incremental AOF cleanup—exec doesn’t glob in Tcl.
exec rm -f $aof_dirpath/appendonly.aof.* passes the * literally, so no files are removed (and if globbed, it would also delete the base + manifest). This makes the “base is last file” recovery test unreliable. Use glob/file delete and rewrite a base-only manifest.
Proposed fix
- exec rm -f $aof_dirpath/appendonly.aof.*
+ foreach f [glob -nocomplain "$aof_dirpath/appendonly.aof.*.incr.aof"] {
+ file delete -force $f
+ }
+ create_aof_manifest $aof_dirpath $aof_manifest_file {
+ append_to_manifest "file appendonly.aof.1.base.aof seq 1 type b\n"
+ }🤖 Prompt for AI Agents
In `@tests/integration/aof.tcl` around lines 723 - 726, The test currently uses
"exec rm -f $aof_dirpath/appendonly.aof.*" which does not glob in Tcl; replace
that with a glob-based removal and manifest rewrite: use "glob -nocomplain
$aof_dirpath/appendonly.aof.*" to collect incremental AOF files then "file
delete" each returned path so only the base AOF remains, and update the manifest
(appendonly.aof.manifest) in the same directory to list only the base file
(ensure start_server_aof/test "Corrupted base AOF (last file): should recover"
sees a base-only manifest); keep references to $aof_dirpath, appendonly.aof.*,
and the manifest file when making the change.
✅ Actions performedReview triggered.
|
2 participants
Benchmark PR from agentic-review-benchmarks#7
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.