fix #98: replace deleteOnExit() with explicit cleanup to prevent temp file leak

[phaneendra-injarapu]

Fixes #98

Problem

DefaultDownloadManager.download() registered every temporary download file with
File.deleteOnExit(). Each call adds an entry to the JVM-wide static
java.io.DeleteOnExitHook set, and entries are never removed during the JVM's
lifetime — they are only iterated at shutdown. Over many invocations this caused:

• Unbounded memory growth — one retained entry, with its path string, per downloaded file
• Shutdown-time bookkeeping proportional to the number of downloads

Fix

deleteOnExit() is removed. Cleanup is now handled by three mechanisms:

1. One temp directory, one shutdown hook (O(1) instead of O(downloads))

All downloads land in a single lazily-created temp directory
(maven-shared-io-downloads-*), and exactly one shutdown hook removes that
directory recursively. Nothing is registered per file, so the amount of retained
state stays constant no matter how many files are downloaded. Each manager
instance gets its own subdirectory, which is what allows per-instance cleanup
(below) without touching another instance's files. No static field ever
references an individual download, so a discarded manager and its cache remain
collectible.

Two properties of this hook are less obvious than they look, and each has a
dedicated test:

• The hook is registered once per class loader, not once per root. The root is
  recreated if it disappears behind our back (a temp-dir sweeper such as
  systemd-tmpfiles, or macOS periodic cleanup). Registering a hook per root
  would reintroduce exactly the accumulation this issue is about, and a Thread
  per entry is heavier than the string DeleteOnExitHook retained. A
  shutdownHookAttempted guard registers once, and the hook reads the current
  root when it runs rather than capturing one.
• The hook's delete path uses only JDK types. It runs at JVM exit, when the
  class loader that defined this class may already be closed — which is what
  Maven does to a plugin class realm at the end of a build. A class the hook
  only needs at that point could no longer be resolved, and the hook would die
  with NoClassDefFoundError and delete nothing. FileUtils.deleteQuietly() is
  therefore replaced by a small deleteRecursively() built on DirectoryStream
  and Files.deleteIfExists(), deleting depth-first and treating symbolic links
  as leaves (NOFOLLOW_LINKS) so nothing outside the tree is touched.

2. Immediate deletion when a file will not be returned

A retainTempFile flag is set only once the file is the one reachable through
the cache. The merged finally block deletes the temp file straight away
otherwise — covering ConnectionException / AuthenticationException (connect
failure), TransferFailedException / ResourceDoesNotExistException /
AuthorizationException (transfer failure), and losing a race to cache the same
URL. Failed downloads therefore contribute nothing at all at shutdown.

3. New DefaultDownloadManager.cleanup() for long-lived JVMs

Deletes this manager's downloads and empties its cache, so a Maven daemon or
embedded build can release the files without waiting for JVM exit. It is
optional, and it is added on the implementation only — the DownloadManager
interface is unchanged, so no existing implementor breaks.

Cache correctness

cache.put() is replaced with a guarded putIfAbsent: a concurrent download of
the same URL now returns the file already published in the cache (which callers
may already be reading) and discards its own redundant copy, while a stale
cache entry — one whose file has since been deleted from disk — is replaced by
the fresh download instead of being handed back. The previous code could return
a File that no longer existed; shouldDownloadAgainWhenTheCachedFileWasDeleted
fails without this change.

Refactor

The separate try/catch blocks around wagon.connect() and wagon.get() are
merged into one. A connected flag keeps wagon.disconnect() limited to the
case where connect() actually succeeded, and transfer listeners are now removed
even when connecting failed — they are added before the connect attempt, so
leaving them attached leaked listeners onto a Wagon that may be reused.

addShutdownHook() catches SecurityException alongside IllegalStateException,
so a restrictive policy makes cleanup fall back to the OS temp sweeper instead of
throwing an unchecked exception out of download().

Tests

DefaultDownloadManagerTest grows from 20 to 31 tests. The new ones:

• shouldDownloadIntoTheSharedTempDirectoryInsteadOfRegisteringDeleteOnExit — the
  download lands under the single shared directory, the structural property that
  keeps shutdown bookkeeping constant
• shouldDeleteTempFileOnConnectionFailure — no file is left behind under the
  download directory after a ConnectionException
• shouldDeleteTempFileOnTransferFailure — uses an EasyMock Capture to get the
  exact File passed to wagon.get() and asserts it no longer exists after a
  TransferFailedException
• shouldDownloadAgainWhenTheCachedFileWasDeleted — a stale cache entry is
  replaced rather than returned
• shouldDeleteDownloadedFilesOnCleanup — cleanup() removes the files and the
  directory
• shouldStillBeUsableAfterCleanup — the manager recreates its directory and
  keeps working after cleanup()
• shouldNotDeleteTheFilesOfAnotherManagerOnCleanup — cleanup() is isolated
  per instance
• shouldRegisterAtMostOneShutdownHookHoweverManyDownloadsAndManagers — 25
  managers, repeated downloads and cleanup() calls add no hooks
• shouldNotRegisterAnotherShutdownHookWhenTheRootIsRemovedBehindOurBack — 25
  simulated sweeps of the root recreate it and keep downloading, still on one hook
• shouldDeleteNestedDirectoriesOnCleanupWithoutFollowingSymbolicLinks — whatever
  a wagon leaves in the directory is removed, and a link's target outside the tree
  survives
• shouldDeleteTheRootWithoutClassesThatAShutdownHookCouldNoLongerLoad — loads the
  manager in a class loader where commons-io does not exist and runs the hook's
  body, proving the delete path needs nothing the hook could fail to resolve

The two hook-count tests read a package-private registeredShutdownHooks()
counter. Asserting on java.io.DeleteOnExitHook or
java.lang.ApplicationShutdownHooks directly would need --add-opens java.base/java.lang, which this module does not configure for Surefire, so the
class exposes the count instead.

All 31 DefaultDownloadManagerTest tests pass, and no
maven-shared-io-downloads-* directory remains after the test JVM exits, which
exercises the shutdown hook end to end.

Notes for reviewers

• The hook holds a reference to the class loader that defined this class. Any
  shutdown hook running library code does; deleteOnExit() did not, since it
  retains only strings. In Maven that means one plugin class realm stays reachable
  for the life of the JVM once something downloads. The hook's inherited context
  class loader is cleared, which removes the other reference along that path, but
  the remaining one cannot be avoided while keeping a hook at all. It is one
  reference per class loader instead of one entry per downloaded file, so it is
  still a large net reduction — flagging it because it is a real change in kind,
  not because it is believed to be a problem. Dropping the hook entirely and
  relying on cleanup() plus the OS temp sweeper is the alternative, at the cost
  of no longer cleaning up after a plain mvn run.
• Downloaded files still occupy disk for the lifetime of the manager — the caller
  reads the returned File, so it has to exist. Bounding disk usage (LRU or size
  cap) would be a separate feature.
• cleanup() is documented as unsafe to call while a download is in flight on
  another thread, since that download writes into the directory being removed.
• URLLocation still calls deleteOnExit() per fetched URL. Same pattern,
  different class, and it sits behind the public tempFileDeleteOnExit
  constructor flag, so changing it is an API-semantics decision; left out to keep
  this PR to one issue. Happy to open a follow-up.

Contribution Checklist

• Your pull request should address just one issue, without pulling in other changes.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
  (11 new tests. The two hook-count tests and the class-loading test were each
  verified by mutation: restoring the per-root hook registration fails the
  first two, and putting FileUtils.deleteQuietly() back in the hook path
  fails the third with the NoClassDefFoundError described above.)
• [ X] Run mvn verify to make sure basic checks pass.
  (Partially verified locally. Spotless, Checkstyle and RAT all pass —
  spotless:check reports 41 files clean with the cache cleared, Checkstyle
  reports 0 violations, RAT 0 unapproved licences — and main and test sources
  compile at --release 8. A full mvn verify could not run in this
  environment: the enforcer requires Maven 3.9+ and only 3.8.5 is available,
  and Surefire 3.5.6 needs plexus-utils:1.1, which is not in the local
  repository and cannot be fetched. Tests were therefore run through a JUnit
  Platform launcher against the same compiled classes. Relying on CI for the
  full lifecycle.)
• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

[Copilot]

Pull request overview

This PR addresses #98 by removing per-download File.deleteOnExit() usage in DefaultDownloadManager (which accumulates JVM-wide DeleteOnExitHook entries) and replacing it with explicit cleanup behavior to prevent temp file leaks.

Changes:

• Remove deleteOnExit() from the download temp-file creation path and add immediate deletion on failure.
• Add a per-instance shutdown hook to delete cached download temp files at JVM shutdown.
• Add new unit tests asserting temp files are deleted on connection and transfer failures.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java	Reworks temp file lifecycle management (no deleteOnExit()), adds shutdown cleanup, and refactors connect/get flow with success/connected flags.
src/test/java/org/apache/maven/shared/io/download/DefaultDownloadManagerTest.java	Adds regression tests to ensure temp files are cleaned up on connection/transfer failures.

Suppressed comments (1)

src/main/java/org/apache/maven/shared/io/download/DefaultDownloadManager.java:179

• On failure the temp file is deleted via File.delete(), but the return value is ignored. If deletion fails (Windows file locks, AV scanners, etc.), the method will still leak the temp file silently. Consider using Files.deleteIfExists(...) and recording any IOException in the MessageHolder so failures are observable.

            if (!success && downloaded != null) {
                downloaded.delete();
            }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[elharo]

need to run mvn spotless:apply

done

[elharo]

I'm worried about this synchronization. This synchronizes the variable but is it still possible that two threads are going to try to delete the same directory at the same time?

[phaneendra-injarapu]

The synchronized block isn't guarding the deletion - it only publishes the downloadRoot field, which is written under the same class lock in downloadRoot(), so the hook doesn't read a stale value. I've reworded the comment to say that.

One overlap is possible, and it's harmless: cleanup() running on another thread while the hook deletes the root. The hook can't race itself, since it's registered once and the JVM runs it on a single thread. Both paths now use FileUtils.deleteQuietly, which ignores failures, so the loser of that race either finds the entry already gone or leaves onedirectory to the OS temp sweep - the fallback this class already relies on. Nothing throws.

[elharo]

Let's not reinvent the wheel. There are existing methods to do this in various places. Maybe maven-shared-utils and definitely Apache commons. Probably other places too. This functionality is really hard to get right.

[phaneendra-injarapu]

Agreed, removed. Both call sites now use FileUtils.deleteQuietly from commons-io, which is already a compile-scope dependency here (URLLocation uses it). I checked 2.22.0 does what this code needs: it removes the tree, doesn't follow symbolic links (tested with links to a file and to a directory outside the tree both targets survived), and returns false rather than throwing when the file is missing.

I did add one call to go with it. The hook runs at JVM exit, when the class loader that defined this class may be closed - a Maven plugin realm is, at the end of a build. FileUtils can't be loaded at that point, so the hook has to touch it earlier; it now does, when the hook is registered. Without that call the regression test fails with ClassNotFoundException: org.apache.commons.io.FileUtils, so it's covered. Full suite passes.