bug: shared directory output (e.g. clang crashreports) fails with racing

[ola-rozenfeld]

https://github.com/bazelbuild/reclient/commit/35813b877b5fbf0fe69bb558c15ed239a816c226 introduced a directory type output to clang actions, which is shared among all actions of this type (with a -fcrash-diagnostics-dir option on). However, in racing strategy, this results in the following error:

reclient[b1872204-b036-4d9d-85ca-575109a989fd]: LocalErrorResultStatus: b1872204-b036-4d9d-85ca-575109a989fd: cannot move directory ../../tools/clang/crashreports from /projects/brave-browser-3/src/out/android_Release_x86/.reproxy_tmp/racing/b1872204-b036-4d9d-85ca-575109a989fd/src/out/android_Release_x86 to /projects/brave-browser-3/src/out/android_Release_x86: rename /projects/brave-browser-3/src/out/android_Release_x86/.reproxy_tmp/racing/b1872204-b036-4d9d-85ca-575109a989fd/src/tools/clang/crashreports /projects/brave-browser-3/src/tools/clang/crashreports: file exists

This is because here we're trying to use os.Rename to move the output directory from the temporary racing directory, and the directory already exists, because it was populated by a different action.

I'm not sure what is the intended behavior of the shared output directory -- as I understand the current code, any output gets removed before it is downloaded, so it looks to me like the behavior is badly defined when multiple actions share an output directory.

If the intended behavior was to aggregate outputs from multiple actions in the same ../../tools/clang/crashreports directory, then this will need to be fixed both in the SDK and Reclient, IIUC.

[andusy]

I've rolled back the change but there are some technical difficulties on our end which is preventing github from picking up the change.

However, this did spark some discussions on shared output directories. Our initial thinking is we may not want to support this. If you have any opinions on this topic feel free to leave a comment as we haven't come to a conclusion on how we should handle these cases yet.

[ola-rozenfeld]

Wow, that was quick! Thank you!

My opinion is that for the crashreports in particular to be useful, we would need to add an argument to the OutputSpec in Command (here) for setting the output directory behavior (I'm thinking an enum with UNKNOWN, REPLACE, and AGGREGATE), with REPLACE being the default (current behavior), and propagate that all the way from the rewrapper config. Then, we can support it in the SDK and the reproxy as well. I would envision the AGGREGATE option to apply to sub directories as well, meaning -- the action does not delete any directories, it only replaces same named files under the same path, but otherwise only adds nodes. The SDK implementation should be rather simple, I think? The reproxy one will be a bit more annoying because we can no longer os.Rename anything, we'd need to be both careful and efficient (still use os.Rename when it's possible, but copy when not).

The question is -- is it worth it? I don't know how useful the crashreports directory is.

[MikeS-rec]

Time to dreg up old bugs...

I suppose it isn't out of the question for some build to arbitrarily say that multiple actions have the same output directory (and not just for clang crash reports).

There are a few other options that might be worth considering as well:

• Check for destination existence -> if it doesn't exist use rename, if it does, rename individual contents of directory (probably fine in most cases, most actions aren't producing huge numbers of outputs).
• Create the destination directory ahead of time (before the action even starts the remote execution), and always rename the individual contents.

I wonder if doing the file moving in this kind of manner might help performance on windows, as it has some... different... filesystem performance compared to linux. Creating and moving directories is less performant.