JCRVLT-851: Remove package tmp files through closewatcher by kwin · Pull Request #434 · apache/jackrabbit-filevault

kwin · 2026-09-27T09:27:53Z

No description provided.

This reverts commit 67e4e79.

kwin · 2026-09-27T09:34:08Z

This requires more work as registering the same object as closeable is not possible, as this prevents the phantomreference to be GCed in the first place....

Previously just the JarFile has been closed leading to orphan temp files. Introduce a new Closer class which is called from CloseWatcher taking care of both cleanup operations.

kwin · 2026-09-27T10:01:11Z

This requires more work as registering the same object as closeable is not possible, as this prevents the phantomreference to be GCed in the first place....

Fixed by extracting the close operations to a dedicated class.
Now just need to add some more test assertions to ArchiveTest.

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved moderate cleanup issues affect partially opened JARs and temporary files during failures or JVM exit.

Review effort: Lite
Findings: 2

Open (2)

Close partially opened JarFile and reset watcher on scan failure · New
Delete temporary file when copy or package construction fails · New

What changed in this PR

Moves temporary ZIP cleanup to CloseWatcher-managed archive lifecycles.

Changes:

Adds watcher-based cleanup for ZIP and NIO archives.
Removes direct temporary-file cleanup from JcrPackageImpl.
Updates archive documentation and removes the regression test.

File	Summary	Findings
`vault-core/src/test/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImplTempFileTest.java`	Removes temporary-file cleanup regression coverage.	—
`vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java`	Creates temporary package archives.	Two moderate issues: cleanup gaps during copy/construction and possible retention until JVM exit.
`vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipNioArchive.java`	Adds watcher-based NIO ZIP cleanup.	—
`vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipArchive.java`	Adds watcher-based JAR cleanup.	Moderate issue: partially opened JARs may remain unclosed after metadata-scan failure.
`vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/Archive.java`	Documents temporary archive close requirements.	—

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

                File tmpFile = File.createTempFile("vaultpack", ".zip");
-                // make sure the temp file is removed at the latest when the JVM terminates,
-                // in case it is never explicitly closed via ZipArchive.close()/ZipVaultPackage.close()
-                tmpFile.deleteOnExit();
-                try {
-                    Binary bin = getData().getBinary();
-                    try (FileOutputStream out = FileUtils.openOutputStream(tmpFile);
-                            InputStream in = bin.getStream()) {
-                        IOUtils.copy(in, out);
-                    } finally {
-                        bin.dispose();
-                    }
-                    pack = new ZipVaultPackage(tmpFile, true);
-                } catch (RepositoryException | IOException | RuntimeException e) {
-                    FileUtils.deleteQuietly(tmpFile);
-                    throw e;
+                Binary bin = getData().getBinary();
+                try (FileOutputStream out = FileUtils.openOutputStream(tmpFile);
+                        InputStream in = bin.getStream()) {
+                    IOUtils.copy(in, out);
+                } finally {
+                    bin.dispose();
                }


Copilot

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Review effort: Lite
Findings: 2

Open (2)

Clean up binary and temp file on all failure paths · New
Delete temporary file when copy or package construction fails

Resolved since last review (1)

Close partially opened JarFile and reset watcher on scan failure

Previously missed (1)

In code that hasn't changed since last review

Clear jar after closing to allow reopening

vault-core/src/main/java/org/apache/jackrabbit/vault/fs/io/ZipArchive.java:236

The new close path closes the JarFile through Closer but never clears the jar field. Consequently, a later open() still returns at the jar != null guard and subsequent reads use the already-closed JarFile; the old implementation reset jar to null. Clear jar when closing so the archive cannot appear open after close and can be reopened consistently.

+                Binary bin = getData().getBinary();
                File tmpFile = File.createTempFile("vaultpack", ".zip");
-                // make sure the temp file is removed at the latest when the JVM terminates,
-                // in case it is never explicitly closed via ZipArchive.close()/ZipVaultPackage.close()
-                tmpFile.deleteOnExit();
-                try {
-                    Binary bin = getData().getBinary();
-                    try (FileOutputStream out = FileUtils.openOutputStream(tmpFile);
-                            InputStream in = bin.getStream()) {
-                        IOUtils.copy(in, out);
-                    } finally {
-                        bin.dispose();
-                    }
-                    pack = new ZipVaultPackage(tmpFile, true);
-                } catch (RepositoryException | IOException | RuntimeException e) {
-                    FileUtils.deleteQuietly(tmpFile);
+                try (FileOutputStream out = FileUtils.openOutputStream(tmpFile);
+                        InputStream in = bin.getStream()) {
+                    IOUtils.copy(in, out);
+                } catch (IOException e) {
+                    tmpFile.delete();
                    throw e;
+                } finally {
+                    bin.dispose();
                }
+                pack = new ZipVaultPackage(tmpFile, true);


Copilot

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Review effort: Lite
Findings: 3

Open (3)

Retain deleteOnExit fallback for package temp files · New
Clean up binary and temp file on all failure paths
Delete temporary file when copy or package construction fails

                    throw new IOException("Error while reading stream", e);
                }
            } else {
                File tmpFile = File.createTempFile("vaultpack", ".zip");


Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 7 · 2

Open (10)

The new error path only deletes tmpFile for IOException, but failures here can also occur as… · New
Same pattern as ZipNioArchive: watcher is not set to null after unregistering, which makes… · New
close() unregisters the watcher but leaves watcher non-null, so repeated close() calls will… · New
The prior regression test covering temp-file cleanup on JcrPackageImpl#getPackage(true) failure… · New
These tests rely on System.gc() + a fixed Thread.sleep(100) to make the archive eligible for… · New
Retain deleteOnExit fallback for package temp files
Clean up binary and temp file on all failure paths
Delete temporary file when copy or package construction fails
This Javadoc is inaccurate for ZipArchive: it refers to closing a “zip file system”, but the… · New
ZipNioArchiveTest loading resources via ZipArchiveCloseTest.class creates unnecessary coupling… · New

@@ -334,8 +333,8 @@ protected VaultPackage getPackage(boolean forceFileArchive) throws RepositoryExc
                        bin.dispose();
                    }
                    pack = new ZipVaultPackage(tmpFile, true);
-                } catch (RepositoryException | IOException | RuntimeException e) {
-                    FileUtils.deleteQuietly(tmpFile);
+                } catch (IOException e) {
+                    tmpFile.delete();
                    throw e;
                }


    @Override
    public void close() {
-        if (zipFileSystem != null) {
-            if (watcher != null) {
-                CloseWatcher.unregister(watcher);
-            }
+        if (watcher != null) {
            try {
-                zipFileSystem.close();
-            } catch (IOException e) {
-                log.warn("Error during close.", e);
+                watcher.getCloseable().close();
+            } catch (Exception e) {
+                // should not happen
            }
+            CloseWatcher.unregister(watcher);
+            zipFileSystem = null;
        }
-        if (deleteAtClose) {
-            try {
-                Files.delete(path);
-            } catch (IOException e) {
-                log.warn("Could not delete " + path, e);
+    }


+        new ZipNioArchive(tmpFile, true);
+        System.gc();
+        Thread.sleep(100);
+        // Now dump unclosed archives
+        assertTrue("Couldn't find unclosed archives", AbstractArchive.dumpUnclosedArchives());
+        assertFalse("Temp file should have been deleted but still exists", Files.exists(tmpFile));


Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 2 · 10

Open (12)

tmpFile.delete() ignores the return value and failures leave the temp file behind. Also, the… · New
The new error path only deletes tmpFile for IOException, but failures here can also occur as…
Similar to ZipNioArchive.close(): swallowing all exceptions without logging makes close failures… · New
The catch (Exception e) silently swallows unexpected failures during close, which makes… · New
The PR removes JcrPackageImplTempFileTest, but the updated JcrPackageImpl still has important… · New
Same flakiness concern as in ZipNioArchiveTest: relying on System.gc() and a fixed sleep can… · New
These tests are likely to be flaky: System.gc() is not guaranteed, and a fixed… · New
These tests rely on System.gc() + a fixed Thread.sleep(100) to make the archive eligible for…
close() unregisters the watcher but leaves watcher non-null, so repeated close() calls will…
Retain deleteOnExit fallback for package temp files
Clean up binary and temp file on all failure paths
Delete temporary file when copy or package construction fails

Resolved since last review (4)

The prior regression test covering temp-file cleanup on JcrPackageImpl#getPackage(true) failure…
Same pattern as ZipNioArchive: watcher is not set to null after unregistering, which makes…
ZipNioArchiveTest loading resources via ZipArchiveCloseTest.class creates unnecessary coupling…
This Javadoc is inaccurate for ZipArchive: it refers to closing a “zip file system”, but the…

@@ -334,8 +334,11 @@ protected VaultPackage getPackage(boolean forceFileArchive) throws RepositoryExc
                        bin.dispose();
                    }
                    pack = new ZipVaultPackage(tmpFile, true);
-                } catch (RepositoryException | IOException | RuntimeException e) {
-                    FileUtils.deleteQuietly(tmpFile);
+                } catch (IOException e) {
+                    tmpFile.delete();
+                    throw e;
+                } catch (RepositoryException e) {
+                    tmpFile.delete();
                    throw e;
                }


+        if (watcher != null) {
+            try {
+                watcher.getCloseable().close();
+            } catch (Exception e) {
+                // should not happen
            }
-            if (file != null && isTempFile) {
-                FileUtils.deleteQuietly(file);
+            CloseWatcher.unregister(watcher);
+            watcher = null;
+            jar = null;
+        }


+        if (watcher != null) {
            try {
-                zipFileSystem.close();
-            } catch (IOException e) {
-                log.warn("Error during close.", e);
+                watcher.getCloseable().close();
+            } catch (Exception e) {
+                // should not happen
            }
+            CloseWatcher.unregister(watcher);
+            watcher = null;
+            zipFileSystem = null;
        }


                File tmpFile = File.createTempFile("vaultpack", ".zip");
-                // make sure the temp file is removed at the latest when the JVM terminates,
-                // in case it is never explicitly closed via ZipArchive.close()/ZipVaultPackage.close()
-                tmpFile.deleteOnExit();
                try {
+                    // used as last resort, usually tmpFile is deleted via ZipVaultPackage.close() or the enclosed
+                    // CloseWatcher
+                    tmpFile.deleteOnExit();


+    public void testDumpUnclosedArchivesClosesTmpFile() throws IOException, InterruptedException, URISyntaxException {
+        Path tmpFile = createTempPackage();
+        new ZipArchive(tmpFile.toFile(), true);
+        System.gc();
+        Thread.sleep(100);
+        // Now dump unclosed archives
+        assertTrue("Couldn't find unclosed archives", AbstractArchive.dumpUnclosedArchives());
+        assertFalse("Temp file should have been deleted but still exists", Files.exists(tmpFile));
+    }


+    public void testDumpUnclosedArchivesClosesTmpFile() throws IOException, InterruptedException, URISyntaxException {
+        Path tmpFile = createTempPackage();
+        new ZipNioArchive(tmpFile, true);
+        System.gc();
+        Thread.sleep(100);
+        // Now dump unclosed archives
+        assertTrue("Couldn't find unclosed archives", AbstractArchive.dumpUnclosedArchives());
+        assertFalse("Temp file should have been deleted but still exists", Files.exists(tmpFile));
+    }


sonarqubecloud · 2026-09-28T16:53:51Z

Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

joerghoh · 2026-10-03T15:58:20Z

@kwin nice find. Indeed it would render the need for JCRVLT-852 redundant, as the actual leaks could be mitigated to some large extent.

Revert "JCRVLT-851 cleanup temp files properly (#432)"

420b8da

This reverts commit 67e4e79.

kwin requested a review from joerghoh September 27, 2026 09:27

kwin marked this pull request as draft September 27, 2026 09:33

JCRVLT-851 Let CloseWatcher also delete underlying temp file

bb574f1

Previously just the JarFile has been closed leading to orphan temp files. Introduce a new Closer class which is called from CloseWatcher taking care of both cleanup operations.

kwin force-pushed the bugfix/remove-package-tmp-files-through-closewatcher branch from a16332a to bb574f1 Compare September 27, 2026 09:59

kwin requested a lite review from Copilot September 27, 2026 10:05

Copilot started reviewing on behalf of kwin September 27, 2026 10:06 View session

Copilot AI reviewed Sep 27, 2026

View reviewed changes

Add tests

e9557ca

kwin changed the title ~~Bugfix/remove package tmp files through closewatcher~~ JCRVLT-851: Remove package tmp files through closewatcher Sep 28, 2026

kwin force-pushed the bugfix/remove-package-tmp-files-through-closewatcher branch from 4a20d3c to da9a608 Compare September 28, 2026 08:21

kwin requested a lite review from Copilot September 28, 2026 08:23

Copilot started reviewing on behalf of kwin September 28, 2026 08:23 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Apply fixes from PR review

87f4eb2

kwin force-pushed the bugfix/remove-package-tmp-files-through-closewatcher branch from da9a608 to 87f4eb2 Compare September 28, 2026 08:35

kwin requested a lite review from Copilot September 28, 2026 11:30

Copilot started reviewing on behalf of kwin September 28, 2026 11:30 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Comment thread vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/JcrPackageImpl.java

throw new IOException("Error while reading stream", e);

}

} else {

File tmpFile = File.createTempFile("vaultpack", ".zip");

further improvements

ae5cce2

kwin requested a lite review from Copilot September 28, 2026 14:53

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Copilot started reviewing on behalf of kwin September 28, 2026 15:17 View session

kwin and others added 4 commits September 28, 2026 17:26

Update Javadoc for Closer class in ZipArchive

0833309

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Fix path reference in createTempPackage method

126eaec

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Handle RepositoryException in JcrPackageImpl

4a46951

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

null the CloseWatcher instance

e9c4be2

kwin force-pushed the bugfix/remove-package-tmp-files-through-closewatcher branch from f7842a7 to e9c4be2 Compare September 28, 2026 15:30

kwin requested a lite review from Copilot September 28, 2026 15:40

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Copilot started reviewing on behalf of kwin September 28, 2026 16:16 View session

Navigation Menu

JCRVLT-851: Remove package tmp files through closewatcher - #434

JCRVLT-851: Remove package tmp files through closewatcher#434
kwin wants to merge 9 commits into
masterfrom
bugfix/remove-package-tmp-files-through-closewatcher

kwin commented Sep 27, 2026

Uh oh!

kwin commented Sep 27, 2026

Uh oh!

kwin commented Sep 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

sonarqubecloud Bot commented Sep 28, 2026

Uh oh!

joerghoh commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kwin commented Sep 27, 2026

Uh oh!

kwin commented Sep 27, 2026

Uh oh!

kwin commented Sep 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

sonarqubecloud Bot commented Sep 28, 2026

Quality Gate failed

Uh oh!

joerghoh commented Oct 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants