PAT @yoav-steinberg

@chenyang8094 i didn't review the code yet, but i'd like to respond to the top comment.

1. i think redis-check-aof should still support checking old aof files (preamble or non-preamble files without manifest). i think someone may be using a new redis-check-aof with on an AOF file from an old server. (if this complicates the code too much, maybe we can drop that, but i'd like to try supporting it).
2. i think your reasoning above about not truncating or fixing INCR files other than the last one, are acceptable. but i think we need to print a message to a user trying to use --truncate-to-timestamp, telling him what to do (edit the manifest file manually). i.e. if we find the timestamp in a non-last INCR file, we should tell him in which file we found it, and instruct him to edit the manifest and re-run.

Before I review here are some questions:
1.

If we want to support truncate any file, we need to add very complicated code to support the atomic modification of multiple file deletion and update manifest, I think this is unnecessary

I don't think the tool needs to be atomic. If we have BASE+INCR1+INCR2 and the specified timestamp is in the middle of INCR1 then we need to delete to truncate INCR1, delete INCR2 and update the manifest. I think it's OK not to be atomic here. The user should always backup the directory before running this tool.

2. Don't we still support a non RDB BASE? If so then the tool can also truncate the base file in such a case.

3. Is the tool still backwards compatible with the old (non multi-part) AOF file?

Don't we still support a non RDB BASE? If so then the tool can also truncate the base file in such a case.

Ahe AOF file produced by rewrite won't have timestamps.

And note that corruptions are only expected at the last INCR file (due to fsync or bad mount options)

1. i think redis-check-aof should still support checking old aof files (preamble or non-preamble files without manifest). i think someone may be using a new redis-check-aof with on an AOF file from an old server. (if this complicates the code too much, maybe we can drop that, but i'd like to try supporting it).

Unless we add special parameters, I can't think of any effective way to identify whether this file is an old AOF or a manifest. It is not reliable to rely on the file name alone.

2. i think your reasoning above about not truncating or fixing INCR files other than the last one, are acceptable. but i think we need to print a message to a user trying to use --truncate-to-timestamp, telling him what to do (edit the manifest file manually). i.e. if we find the timestamp in a non-last INCR file, we should tell him in which file we found it, and instruct him to edit the manifest and re-run.

I thought, in this case, the user will see the following message:

Failed to truncate AOF appendonly.aof.1.incr.aof to timestamp 1628217471 because it is not the last file.
If you insist, please delete all files after this file according to the manifest file and delete the corresponding records in manifest file manually. Then re-run redis-check-aof.

regarding the detection of an old AOF file, or a manifest i'm not sure it's that bad to rely on the file name (we do control the name of the manifest file, and it's unlikely that users will name their old AOF files with a "manifest" suffix).
however, i think it's not that complicated to detect automatically by the content.
either we try to match the first word we know must be on the first line of the manifest, or even add some new unique header to our manifest files (it's not too late).

regarding the truncation message, that's great.
two suggestions for improvements:

1. we can take this opportunity to mention the offset we found in that file (so user can use other tools to manually truncate it, rather than go though the slow AOF file processing again)
2. do we really have to instruct them to delete the files? isn't it enough to just modify the manifest?

I don't think the tool needs to be atomic. If we have BASE+INCR1+INCR2 and the specified timestamp is in the middle of INCR1 then we need to delete to truncate INCR1, delete INCR2 and update the manifest. I think it's OK not to be atomic here. The user should always backup the directory before running this tool.

We only recommend that users back up but it is not mandatory. Without the guarantee of atomicity, I don't want to destroy the original data. In addition, if it crashes due to some reason in the middle, and we have no chance to print out enough log, may cause the manifest and AOF to be incompatible, the user will not know whether the problem is caused by the redis-check-aof tool itself or the original data error. This will be confusing.

2. Don't we still support a non RDB BASE? If so then the tool can also truncate the base file in such a case.

Of course we can (if BASE is the last file).

3. Is the tool still backwards compatible with the old (non multi-part) AOF file?

No, we are discussing this.

The manifest must start with file ， like：
file appendonly.aof.1.incr.aof seq 1 type i

So, we can do like this:

if (file start  with REDIS) {
   check rdb file
} else if (file start with file) {
  check multi part aof
} else {
  check old aof
}

2. do we really have to instruct them to delete the files? isn't it enough to just modify the manifest?

Yes, only modifying the manifest is enough, but a better suggestion is to delete the files together, otherwise these files will never be seen by redis and will not be recycled by the GC mechanism.

ok, so i think we can agree on:

1. no need to automatically handle truncation of non-last files (for both atomicity and complexity concerns)
2. detect old aof files vs manifest files like you just described.
3. keep the message about delete (or move) files when you manually remove them from the manifest.
4. let's add the offset inside the file where the truncation is needed in the log message.

For the record I think a recovery tool shouldn't worry about atomicity. For two main reasons:

1. When dealing with production issues that require manual fixes to get back up online we always perform non-atomic operations. We don't have the luxury to stop and consider what happens if we have another failure (causing atomicity issues) while performing the recovery process. This is simply in order to fix things and get back online as soon as possible because developing an atomic custom recovery plan during a crisis is never practical.
2. To mitigate the risk in 1. we backup our data or any configuration before performing a risky operation. Sometimes the cost of this is large (copying a 500GB AOF file is expensive) but there's no practical way around it.
   I think this tool should in general disregard atomicity concerns but should definitely warn the user to backup the files before applying any fixes to them.

I'm also fine with not editing the manifest file at this point to keep the tool simple (in 99% of the cases we won't need to truncate a non last INCR file), but I don't think atomicity has anything to do with it.

@yoav-steinberg Aside from atomicity, but it does bring a lot of complexity. Thinking from another perspective, if the user really wants to truncate the non-last INCR file, we print a friendly message and ask him to manually delete these files (this time he may realize that these files should be backed up first). I think this is a safe practice, and deleting files is a very easy thing for user, and the probability of this happening is very small.

@oranagra 1，2，3，4 all done.

@chenyang8094 please take a look at this failure
https://github.com/redis/redis/runs/5266360953?check_suite_focus=true#step:5:1732

child process exited abnormally
    while executing
"exec src/redis-check-aof --truncate-to-timestamp 1628217470 $aof_manifest_file"
    ("uplevel" body line 23)
    invoked from within
"uplevel 1 $code"
    (procedure "test" line 51)
    invoked from within
"test {Truncate AOF to specific timestamp} {

another similar error:
https://github.com/redis/redis/runs/5273856527?check_suite_focus=true#step:5:1456

[err]: Short read: Utility should confirm the AOF is not valid in tests/integration/aof.tcl
Expected 'Start checking Multi Part AOF
Start to check INCR files.
Cannot open file: appendonly.aof.1.incr.aof, aborting...
child process exited abnormally' to match '*not valid*' (context: type eval line 5 cmd {assert_match "*not valid*" $result} proc ::test) 

i don't understand how we could get "aborting...", if we didn't pass the --fix argument.

@oranagra GOT, I have added more logs (printf the errno and AOF abs path) in my branch and re-run the workflow, but so far it has not reappeared.

finally reappeared.

It looks like we are getting a wrong path, is there a bug in the dirname function?

[exception]: Executing test client: Start checking Multi Part AOF
Start to check INCR files.
Cannot open file ./tests/tmp/server.aof.2565.11/appendonlydir/appendonly.aof.manifest/appendonly.aof.1.incr.aof: Not a directory, aborting...
child process exited abnormally.
Start checking Multi Part AOF

I add this log:

and finally reproduced it (https://github.com/chenyang8094/redis/runs/5283813036?check_suite_focus=true):

So it's dirname that is returning us the wrong path, I'm not sure that dirname will have some kind of exception under SANITIZER=address, at least I haven't found a similar answer yet.

Copy a glibc implementation:

char *dirname(char *path) {
    static const char dot[] = ".";
    char *last_slash;
    /* Find last '/'.  */
    last_slash = path != NULL ? strrchr(path, '/') : NULL;
    if (last_slash != NULL && last_slash != path && last_slash[1] == '\0') {
        /* Determine whether all remaining characters are slashes.  */
        char *runp;
        for (runp = last_slash; runp != path; --runp)
            if (runp[-1] != '/') break;
        /* The '/' is the last character, we have to look further.  */
        if (runp != path) last_slash = __memrchr(path, '/', runp - path);
    }
    if (last_slash != NULL) {
        /* Determine whether all remaining characters are slashes.  */
        char *runp;
        for (runp = last_slash; runp != path; --runp)
            if (runp[-1] != '/') break;
        /* Terminate the path.  */
        if (runp == path) {
            /* The last slash is the first character in the string.  We have to
               return "/".  As a special case we have to return "//" if there
               are exactly two slashes at the beginning of the string.  See
               XBD 4.10 Path Name Resolution for more information.  */
            if (last_slash == path + 1)
                ++last_slash;
            else
                last_slash = path + 1;
        } else
            last_slash = runp;
        last_slash[0] = '\0';
    } else
        /* This assignment is ill-designed but the XPG specs require to
           return a string containing "." in any case no directory part is
           found and so a static and constant string is required.  */
        path = (char *)dot;
    return path;
}

@yossigo @oranagra Do you have any relevant insights on this?

finally found the bug:

    char temp_filepath[PATH_MAX + 1];
    memcpy(temp_filepath, filepath, strlen(filepath));
    dirpath = dirname(temp_filepath);

Since temp_filepath is allocated on the stack and not initialized, and we did not copy '\0' when executing memcpy, so when a lot of '\' are left on the stack, dirname may get a wrong result.