Code Review Bench PR #14058 - Add auto-repair options for broken AOF tail on startup #7

ketkarameya · 2026-02-10T22:05:28Z

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#7

Add auto-repair options for broken AOF tail on startup

…oken

gitar-bot-staging · 2026-02-10T22:10:44Z

🔍 CI failure analysis for 5c59ae6: CI failure in build-debian-old jobs caused by Debian Buster EOL - package repositories return 404 errors preventing build-essential installation.

Issue

The build-debian-old CI jobs are failing with exit code 127 (make: not found).

Root Cause

Debian Buster (10) reached End-of-Life and its package repositories have been removed from the main Debian mirrors. The CI job attempts to run:

apt-get update && apt-get install -y build-essential

But all repository URLs return 404 Not Found:

Err:4 http://deb.debian.org/debian buster Release
  404  Not Found [IP: 146.75.30.132 80]
Err:5 http://deb.debian.org/debian-security buster/updates Release
  404  Not Found [IP: 146.75.30.132 80]
Err:6 http://deb.debian.org/debian buster-updates Release
  404  Not Found [IP: 146.75.30.132 80]

Without build-essential, the make command cannot be found, causing the build to fail.

Details

Debian Buster was released in 2019 and reached EOL in 2024. EOL'd Debian releases are moved to archive.debian.org rather than the standard mirrors. The CI configuration still points to deb.debian.org which no longer serves Buster packages.

This is an infrastructure issue affecting the CI environment configuration, not the code changes in this PR.

Code Review ⚠️ Changes requested 0 resolved / 2 findings

AOF auto-repair feature is well-designed and conservative, but stopLoading() doesn't account for the new AOF_BROKEN_RECOVERED return code, causing successful recoveries to be reported as failures. Additionally, the incremental AOF block is missing the AOF_BROKEN_RECOVERED check that the base AOF block has.

⚠️

Bug: stopLoading reports failure after successful AOF broken recovery

📄 src/aof.c:1894

At line 1894, stopLoading is called with:

stopLoading(ret == AOF_OK || ret == AOF_TRUNCATED);

When AOF broken recovery succeeds, ret is AOF_BROKEN_RECOVERED (6), which doesn't match either AOF_OK or AOF_TRUNCATED, so the success parameter evaluates to 0 (false). This means a successful recovery is reported as a failed load.

The stopLoading(int success) function likely updates server state and emits events based on whether loading succeeded. Reporting failure here can cause incorrect server behavior — e.g., replication or client notifications may be wrong.

This affects both base AOF recovery (where line 1841 goes to cleanup directly) and incremental AOF recovery (where the loop finishes normally).

Suggested fix

    stopLoading(ret == AOF_OK || ret == AOF_TRUNCATED || ret == AOF_BROKEN_RECOVERED);

⚠️

Bug: Incr AOF block missing AOF_BROKEN_RECOVERED in cleanup check

📄 src/aof.c:1875

The base AOF loading block (line 1841) includes AOF_BROKEN_RECOVERED in the early exit check:

if (ret == AOF_OPEN_ERR || ret == AOF_FAILED || ret == AOF_BROKEN_RECOVERED) {
    goto cleanup;
}

But the corresponding incremental AOF loading block (line 1875) does NOT:

if (ret == AOF_OPEN_ERR || ret == AOF_FAILED) {
    goto cleanup;
}

This inconsistency means:

When an intermediate incr file is recovered: ret gets set to AOF_FAILED at line 1870-1871 (correct), and line 1875 catches it (OK).
When the last incr file is recovered: ret stays AOF_BROKEN_RECOVERED, and line 1875 doesn't exit the loop. The code falls through to set aof_current_size and aof_rewrite_base_size and then reaches stopLoading(0).

The asymmetry is suspicious. For the base file, a successful recovery immediately jumps to cleanup (skipping size assignments). For incr files, a successful recovery falls through and sets sizes. The behavior should be made consistent. If the intent was to mirror the base block behavior, AOF_BROKEN_RECOVERED should be added to line 1875 as well. However, note that either way the stopLoading bug above needs to be fixed.

Suggested fix

            if (ret == AOF_OPEN_ERR || ret == AOF_FAILED || ret == AOF_BROKEN_RECOVERED) {
                goto cleanup;
            }

Rules ✅ All requirements met 🎸 3 actions taken

Gitar Rules

✅ Unaddressed Items Follow-up: PR merged with 0 unresolved review comments and 0 new TODO/FIXME comments in code

🎸 Linear Auto-Linking: Searched author's recent Linear issues to link AOF Auto-Repair feature PR (no reference found in title/branch/commits)

🎸 Flaky Test Retry: Attempted to retry build-debian-old CI jobs that failed due to infrastructure issue (Debian Buster EOL, 404 repository errors). Jobs cannot be rerun via gh CLI.

🎸 Summary Enhancement: Appended technical summary to minimal PR description covering configuration parameters (aof-load-broken, aof-load-broken-max-size), recovery logic with thresholds, data integrity safeguards, and comprehensive test coverage

_{2 rules not applicable. Show all rules by commenting gitar display:verbose.}

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

StavRLevi and others added 22 commits January 25, 2026 12:12

Add auto-repair options for broken AOF tail on startup

76826d9

change aof-load-broken-max-size configuration

f9f91f4

fix of-load-broken-max-size configuration

f7854e1

fix unnecessary changes

9487237

remove unnecessary change

11754c1

Fix format specifier in AOF error message

023f121

Fix format specifier in AOF error message

09e8235

Fix tcl error msg

459faa7

Add incr file support + remove spaces

bcd7eea

add tests for base aof

9751360

fix spaces

37e5bb5

add support to incr aof + add test for an intermediate AOF file is br…

825c783

…oken

fix indentation and add more tests

a202286

add tls

a2703d9

add space + file for test

4bc5794

try to remove tls for tests

293ff89

fix tls

c67106f

add aof-load-broken to redis.conf

9da7ec3

add expansion about aof-load-broken-max-size

008dff1

add expansion about aof-load-broken-max-size

8408c32

add aof-load-broken-max-size to conf file

34df5ba

update pr

5c59ae6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code Review Bench PR #14058 - Add auto-repair options for broken AOF tail on startup #7

Code Review Bench PR #14058 - Add auto-repair options for broken AOF tail on startup #7

ketkarameya commented Feb 10, 2026

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026 •

edited

Loading

Issue

Root Cause

Details

Gitar Rules

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Code Review Bench PR #14058 - Add auto-repair options for broken AOF tail on startup #7

Are you sure you want to change the base?

Code Review Bench PR #14058 - Add auto-repair options for broken AOF tail on startup #7

Conversation

ketkarameya commented Feb 10, 2026

Uh oh!

gitar-bot-staging bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Root Cause

Details

Gitar Rules

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gitar-bot-staging bot commented Feb 10, 2026 •

edited

Loading