Robust cleanup after failed download

[marktsuchida]

Motivated by #28 (but turned out to be separate; see below).

• Delete partial/failed downloaded file using a method that (should) not fail if Microsoft Defender Antivirus is scanning it
• Also use a retry loop for cleaning up temporary files and directories
• Improve messages when waiting for another cjdk to download the same file, and when that other cjdk fails.

[gselzer]

I totally dropped the ball on this issue, but thank you for investigating and finding a fix @marktsuchida! Happy to test things if it is helpful!

[marktsuchida]

No prob. If the original problem is reproducible, I'd be curious to see if this fixes it (i.e., consistently exits on CTRL-C without leaving the temp dir dangling).

[gselzer]

Okay, yes, the general bug is reproducible, and this branch does not yet fix things. I am happy to debug further if necessary however it seems like you have a pretty good understanding of the situation, so let me know whether you want me to go deeper.

Steps for reproduction:

1. Clear your cjdk cache if it exists.
2. Check out this branch of napari-imagej
3. uv run napari. This will install this feature branch of cjdk during the implicit sync. Unlikely that the pyimagej branch is also necessary but it was what I was testing with.
4. Plugins -> ImageJ2. Exit napari-imagej during the jdk download. I see a traceback similar to:

 napari-imagej  uv run --no-sync napari
cjdk: Installing JDK zulu-jre:11.0.29 to C:\Users\gjselzer\AppData\Local\cjdk\cache
Download  56% of  38.8 MiB |#########################################################################################################                                                                                | Elapsed Time: 0:00:00 ETA:   0:00:00Traceback (most recent call last):
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\shutil.py", line 618, in _rmtree_unsafe
    os.unlink(fullname)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\gjselzer\\AppData\\Local\\Temp\\cjdk-l4azqz5y\\archive.zip'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\tempfile.py", line 852, in onerror
    _os.unlink(path)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\gjselzer\\AppData\\Local\\Temp\\cjdk-l4azqz5y\\archive.zip'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\weakref.py", line 667, in _exitfunc
    f()
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\weakref.py", line 591, in __call__
    return info.func(*info.args, **(info.kwargs or {}))
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\tempfile.py", line 868, in _cleanup
    cls._rmtree(name, ignore_errors=ignore_errors)
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\tempfile.py", line 864, in _rmtree
    _shutil.rmtree(name, onerror=onerror)
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\shutil.py", line 750, in rmtree
    return _rmtree_unsafe(path, onerror)
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\shutil.py", line 620, in _rmtree_unsafe
    onerror(os.unlink, fullname, sys.exc_info())
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\tempfile.py", line 855, in onerror
    cls._rmtree(path, ignore_errors=ignore_errors)
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\tempfile.py", line 864, in _rmtree
    _shutil.rmtree(name, onerror=onerror)
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\shutil.py", line 750, in rmtree
    return _rmtree_unsafe(path, onerror)
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\shutil.py", line 601, in _rmtree_unsafe
    onerror(os.scandir, path, sys.exc_info())
  File "C:\Users\gjselzer\AppData\Roaming\uv\python\cpython-3.10.18-windows-x86_64-none\lib\shutil.py", line 598, in _rmtree_unsafe
    with os.scandir(path) as scandir_it:
NotADirectoryError: [WinError 267] The directory name is invalid: 'C:\\Users\\gjselzer\\AppData\\Local\\Temp\\cjdk-l4azqz5y\\archive.zip'
 napari-imagej  uv run --no-sync napari
cjdk: Another process is currently downloading the same file
cjdk: If you are sure this is not the case (e.g., previous download crashed), try again after deleting the directory C:\Users\gjselzer\AppData\Local\cjdk\cache\v0\fetching\jdks\98914a86d0b373cf73d629e05d9db71228492be9
Already downloading; waiting | |                                                                                                       #                                                                                          | 0 Elapsed Time: 0:00:28

5. uv run napari, Plugins -> ImageJ2 again will cause the hanging.

[marktsuchida]

@gselzer Thanks, that helps. Will investigate.

[marktsuchida]

Some notes:

• The exceptions printed in @gselzer's repro are from tempfile.TemporaryDirectory() used in _download.download_and_extract(). They occur in a finalizer that TemporaryDirectory registers using weakref.finalize(), i.e., when the TemporaryDirectory object is garbage-collected. In particular, finalizers are called when the interpreter exits, normally or due to an uncaught exception. The stack traces aren't printed if I pass ignore_cleanup_errors=True to TemporaryDirectory.

• The leaked temporary directory (in AppData/Local/Temp) is not a concern; it will not block future downloads. What blocks the second download are the directories like AppData\Local\cjdk\cache\v0\fetching\jdks\98914a86d0b373cf73d629e05d9db71228492be9 (for zulu-jre:11.0.29) which are keyed by a hash of the URL being downloaded.

• All of cjdk's cleanup is either completely absent, or failing silently. I believe it's the former, and it's because we're running in a QThread in this repro. Printing a message in _download.download()'s except clause does not work, confirming this. Regular Python daemon threads would behave similarly: threads are abruptly terminated on sys.exit() or other process exit. Non-daemon threads (which are the only "correct" way to run tasks that require cleanup) would cause the app to hang on exit unless we provide a way to async-cancel cjdk tasks (and users correctly cancel) -- totally worth considering but a separate topic.

• Maybe we can also use weakref.finalize() for more reliable cleanup. It should work in most cases other than a hard crash.

• The PermissionError in the repro is almost certainly caused because cjdk is not given the opportunity to even close the file, not because Antivirus is scanning it. If we want to successfully clean up everything, we need to use a single finalizer and manage our own stack of cleanup tasks, so that they are invoked in reverse order. We also need to ensure that none of the resources requiring cleanup have their own finalizer that may fire before our cleanup (this is easy because TemporaryDirectory can have delete=False; or we can use mkdtemp()).

• As a side note, I should remove the Win32 API calls added in this PR to "safely" delete files and just keep the retry loops. This is because those calls only mark the file for deletion and don't actually delete before returning (the files are deleted when they are closed by the other process). That causes further cleanup (such as deleting the parent directory) to fail, and only makes things complicated. But none of this is tested in the repro. (And the whole suspicion about Antivirus scans might have been a red herring -- at least it's now clear that we have not observed it ourselves.)

So

• Remove special Win32 delete code
• Disable built-in cleanup for TemporaryDirectory (and any others, though I think that's the only one)
• When starting any operation involving temporary files and dirs, register a finalizer that references a stack of cleanup tasks. Pass this down the call chain and push tasks. Pop tasks when normal cleanup succeeds and/or finalize when we return to the starting point.

[marktsuchida]

Finalizer-based cleanup is likely to mitigate the issue for the exact case we have here, for the manageable cost of using an explicit cleanup-stack (as proposed above). I started implementing this and it looked relatively clean, but then I started thinking about the fact that Python is moving to free-threaded.

Running anything real on a daemon thread (including QThread) without a way to cleanly stop the thread on exit is fundamentally unsafe, because the thread will be terminated abruptly by the operating system, and because the thread causing the process exit will start cleanup while the daemon thread is still running. This is true in any programming language, and Python with GIL is probably extra nice with daemon threads -- until you encounter the rough edges. And with free-threaded Python, the consequences are probably going to be ... different. I don't know for sure, but at least in theory, deadlocks could occur if not crashes.

For QThread, the correct way to ensure cleanup is to connect the QApplication's aboutToQuit signal to a handler that cancels the background operation and wait()s for the thread to exit. For regular Python threads, you'd probably use a non-daemon thread and register an atexit handler to cancel the operation and join the thread.

So I think it's better for the cjdk API to offer a way to safely run cancellable background installations rather than add hard-to-test complexity to work around what is inherently unsafe usage. I don't want to find out in a few years, when PyQt supports free-threaded, that yet more workarounds are needed, for example. I also don't want to make creating technical debt be the "easy thing" for user code to do.

Possibilities:

1. Internally run all cache-touching cjdk operations in a subprocess. The subprocess can clean up on SIGTERM (the CLI already does).
2. When running cjdk in a multi-threaded program, the caller must ensure that they cancel the operation and join or exit the thread before the program exits. For this to be possible, cjdk needs to offer an async cancel API.
3. We can add an async API to cjdk itself (like cjdk.cache_jdk_async()), returning cancelable futures.

1 has the various downsides of any multi-process arrangement. 2 puts a burden on the caller to get things right, and ends up being less clean than 3. 3 has the added advantage that we can stream progress back to the caller as an alternative to the CLI progress bar, which should be nicer for GUI apps. It might be just as easy to support both 2 and 3, though. Given that we already have good cleanup upon cancellation by KeyboardInterrupt, it shouldn't be hard to make it also clean up on CancelledError. We just need to start the operation on a thread and register to cancel atexit.

I'm going to prune this PR back to just the cleanup that makes sense assuming no abrupt thread termination, and start another one to add an async API (maybe post 0.5.0).