Tempdir conflict with `--appimage-extract-and-run` on parallel runs

[lalten]

When an appimage is run with --appimage-extract-and-run, the path is calculated as $TMPDIR/appimage_extracted_$MD5DIGEST. This usually works, but it seems there are problems when the same appimage is run in parallel. Likely there is a race condition where one appimage is still running while the other one is removing its files already.

It seems this happened to me in this CI run: https://github.com/lalten/rules_appimage/actions/runs/3085571024/jobs/4989010345 where I added a testing matrix. I think that caused the same appimage to be run in parallel and led to really weird errors.

I wonder if it makes sense to include the date or maybe even just the pid of the process into the tempdir name as well.

Relevant places in the runtime code:

AppImageKit/src/runtime.c

Line 587 in 90704a0

strcpy(temp_base, getenv("TMPDIR"));

AppImageKit/src/runtime.c

Lines 690 to 696 in 90704a0

	hexlified_digest = appimage_hexlify(digest.bytes, sizeof(digest.bytes));
	}
	char* prefix = malloc(strlen(temp_base) + 20 + strlen(hexlified_digest) + 2);
	strcpy(prefix, temp_base);
	strcat(prefix, "/appimage_extracted_");
	strcat(prefix, hexlified_digest);

[TheAssassin]

For CI purposes, where the build host is reset anyway after every build, you should export NO_CLEANUP=1 as a workaround.

[lalten]

That does indeed seem to do the trick, thanks a lot for the quick and insightful hint @TheAssassin!

[lalten]

The NO_CLEANUP is definitely a good workaround for CI environments. Should running the same extracted appimage in parallel be supported regardless?

[TheAssassin]

I guess it should at least be discussed. This kind of use case requires quite some complexity in the runtime (we'd have to try to make all running runtimes communicate with each other and coordinate the cleanup in some way). APPIMAGE_EXTRACT_AND_RUN as well as the CLI parameter are typically used in edge cases where NO_CLEANUP is a viable workaround...

[lalten]

What do you think of my suggestion to include the PID in the dirname? Downside is more disk space used and more extraction time (since no existing files will be skipped). Upside is no problems with parallelism. A compromise would be to enable this behavior via another env var, but then that's not much better than NO_CLEANUP 🤷

[TheAssassin]

You may have guessed it from the name, NO_CLEANUP's original purpose was for subsequent runs to not re-extract every AppImage every time. So it's just a workaround for your situation. It's not a solution.

[probonopd]

Why don't we use something random (like we do for the mountpoint)?

[schspa]

Why don't we use something random (like we do for the mountpoint)?

I got this problem too.

For the case of two users, if user 1 is killed while running and some not fully uncompressed files remain, user 2 will no longer be able to run this command because it has no permission to recover.

I think there should be a UID/username added to the directory name, even with NO_CLEANUP it shouldn't share the same cache with different users.

[TheAssassin]

Again: no. See #1215 (comment)

[schspa]

It still failed with multi-user usage with NO_CLEANUP=1

File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
File exists and file size matches, skipping
fopen error: Permission denied
Failed to extract AppImage