[Jefiya-MJ]

Problem

When felix.fileinstall.subdir.mode = recurse is active, FileInstall scans all subdirectories under the watched root and registers every matching file as an OSGi Configuration object.

Both setConfig() and deleteConfig() derive the ConfigurationAdmin PID from the filename alone via parsePid(f.getName()), discarding the directory path entirely. This means any two files with the same name —
regardless of where they sit in the directory tree — resolve to the same PID.

Consequences:

• If a file with the same name as a live configuration is copied into a subdirectory, setConfig() overwrites the live configuration's felix.fileinstall.filename property with the duplicate file's URI, silently stealing ownership.

• When the duplicate file is later deleted, deleteConfig() resolves the same PID and calls config.delete() on the live Configuration object — removing it from ConfigurationAdmin even though the original physical file was never touched. Dependent OSGi services stop immediately and cannot restart.

Fix

Before acting on a resolved Configuration object, both setConfig()and deleteConfig() now compare the absolute URI of the file under operation against the felix.fileinstall.filename property already stored in that Configuration.

String registeredFileName = (String) props.get(DirectoryWatcher.FILENAME);
if (registeredFileName != null && !registeredFileName.equals(toConfigKey(f))) {
    return false;
}

Reproduction Scenario

1. Configure felix.fileinstall.subdir.mode = recurse (default).
2. Let FileInstall register /watched/etc/app.cfg as PID app.
3. Copy /watched/etc/app.cfg to /watched/backup/etc/app.cfg.
4. Delete /watched/backup/etc/app.cfg.
5. Before this patch: deleteConfig() resolves PID app and removes the live configuration from ConfigurationAdmin. The service stops.
6. After this patch: deleteConfig() detects the URI mismatch and returns false without touching ConfigurationAdmin.

[Jefiya-MJ]

Can someone take a look into this PR? TIA

[paulrutter]

Review of the duplicate-PID guard. The direction looks right, but the placement of the setConfig() check leaks the READ_ONLY attribute, and the skip is permanent (never retried), which can lose a configuration outright on a legitimate file move. Details inline.

[paulrutter]

Proposal: wire fileinstall into GitHub CI so these unit tests actually run

While reviewing this PR I noticed fileinstall is not covered by .github/workflows/maven-ci.yml, so the new ConfigInstallerTest cases added here never run in CI. Posting the change as a proposal rather than a separate PR — feel free to fold it into this branch or take it separately.

Two things were needed.

1. mvn verify cannot run on fileinstall under JDK 17+

fileinstall still inherits felix-parent 6 (every module currently in CI is on 8 or 9). Parent 6 binds ianal-maven-plugin:1.0-alpha-1, which dies under strong encapsulation:

Failed to execute goal org.codehaus.mojo:ianal-maven-plugin:1.0-alpha-1:verify-legal-files
  Unable to make private java.io.File(java.lang.String,java.io.File) accessible:
  module java.base does not "opens java.io" to unnamed module

So the CI step uses clean test rather than the clean verify the other modules use. Moving the module to a newer parent would be the better long-term fix, but that is a bigger change than a CI tweak and would affect the released bundle, so I left it alone and documented the reason in the workflow.

2. DirectoryWatcherTest.testInvalidTempDir is stale and fails on Linux

This one is a genuine pre-existing failure on master, unrelated to this PR. FELIX-6794 (3263270) replaced the hand-rolled temp-dir logic — which read System.getProperty("java.io.tmpdir") fresh on every call — with Files.createTempDirectory(). That uses TempFileHelper.tmpdir, a cached static initialised on first use, so the test's System.setProperty("java.io.tmpdir", ...) no longer influences the code under test at all. Demonstrated standalone on JDK 21:

prop now = /nonexistent/nope
created  = /tmp/fileinstall-10512125911307238276

The rewrite drives the still-reachable failure path instead: configure felix.fileinstall.tmpdir to a directory that cannot be created, and assert the constructor rejects it. The parent is a regular file rather than a write-protected directory, so it fails deterministically for any user — including root, which matters if anyone runs the build in a container.

I mutation-checked the rewritten test: it passes with prepareDir's validation intact and fails when that validation is stubbed out, so it is not passing vacuously.

The extra getServiceReference(LogService.class) stub in setUp is needed because Util.getLogService() calls the Class-typed overload, while the fixture only stubbed the String one — previously unnoticed since no test reached an ERROR-level log.

Verification

Run on Linux (CI runs ubuntu-latest), mvn clean test:

JDK	Result
21	Tests run: 32, Failures: 0, Errors: 0 — BUILD SUCCESS
25	Tests run: 32, Failures: 0, Errors: 0 — BUILD SUCCESS

JDK 25 stands in as an upper bound for the matrix's 23. I could not verify JDK 17 locally (not packaged for my distro) — worth a glance at the first CI run, though source/target 8 on 17 is the least risky of the three.

Note for anyone reproducing on Windows: 5 further tests in DirectoryWatcherTest/ConfigInstallerTest fail there because they build expected URIs by string concatenation ("file:" + absolutePath, which yields file:C:\... instead of file:/C:/...). Those are Windows-only and I left them untouched, since CI is Linux.

Diff

diff --git a/.github/workflows/maven-ci.yml b/.github/workflows/maven-ci.yml
index 3658e01105..cf36e0bc76 100644
--- a/.github/workflows/maven-ci.yml
+++ b/.github/workflows/maven-ci.yml
@@ -11,6 +11,7 @@ on:
       - 'log/**'
       - 'webconsole/**'
       - 'framework/**'
+      - 'fileinstall/**'
   pull_request:
     branches: [ "master" ]
     paths:
@@ -22,6 +23,7 @@ on:
       - 'log/**'
       - 'framework/**'
       - 'gogo/**'
+      - 'fileinstall/**'
 
 permissions: {}
 
@@ -65,6 +67,8 @@ jobs:
             - 'framework/**'
           gogo:
             - 'gogo/**'
+          fileinstall:
+            - 'fileinstall/**'
 
     - name: Felix SCR
       if: steps.changes.outputs.scr == 'true'
@@ -93,6 +97,13 @@ jobs:
     - name: Felix Gogo Shell
       if: steps.changes.outputs.gogo == 'true'
       run: mvn -B -V -Dstyle.color=always --file gogo/pom.xml clean verify
+    # fileinstall still inherits felix-parent 6, which binds ianal-maven-plugin
+    # 1.0-alpha-1. That plugin fails on JDK 17+ ("module java.base does not opens
+    # java.io"), so the 'verify' phase cannot run here. Stick to 'test' until the
+    # module is moved to a newer parent.
+    - name: Felix File Install
+      if: steps.changes.outputs.fileinstall == 'true'
+      run: mvn -B -V -Dstyle.color=always --file fileinstall/pom.xml clean test
     - name: Upload Test Results
       if: always()
       uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
diff --git a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
index 6769c4d3d5..49d02467fd 100644
--- a/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
+++ b/fileinstall/src/test/java/org/apache/felix/fileinstall/internal/DirectoryWatcherTest.java
@@ -74,6 +74,8 @@ public class DirectoryWatcherTest extends TestCase
                         .andStubReturn(null);
         EasyMock.expect(mockBundleContext.getServiceReference(LogService.class.getName()))
                         .andStubReturn(null);
+        EasyMock.expect(mockBundleContext.getServiceReference(LogService.class))
+                        .andStubReturn(null);
         EasyMock.expect(mockBundleContext.getBundle()).andReturn(mockBundle).anyTimes();
         EasyMock.expect(mockBundleContext.getBundle(Constants.SYSTEM_BUNDLE_LOCATION)).andReturn(mockSysBundle).anyTimes();
         EasyMock.expect(mockSysBundle.getState()).andReturn(Bundle.ACTIVE).anyTimes();
@@ -237,39 +239,27 @@ public class DirectoryWatcherTest extends TestCase
     
     public void testInvalidTempDir() throws Exception
     {
-        String oldTmpDir = System.getProperty("java.io.tmpdir");
-        
-        try 
+        // Point felix.fileinstall.tmpdir at a path whose parent is a regular file, so that
+        // creating the directory is bound to fail for any user running the build (root
+        // included). Overriding java.io.tmpdir would not work here: Files.createTempDirectory
+        // caches that property on first use, so a later System.setProperty has no effect.
+        File blocker = new File( "target/not-a-directory" );
+        blocker.getParentFile().mkdirs();
+        blocker.delete();
+        assertTrue( "Unable to create test fixture file " + blocker, blocker.createNewFile() );
+
+        props.put( DirectoryWatcher.TMPDIR, new File( blocker, "tmp" ).getAbsolutePath() );
+
+        EasyMock.replay(mockBundleContext, mockBundle, mockSysBundle, mockStartLevel);
+
+        try
         {
-            File parent = new File("target/tmp");
-            parent.mkdirs();
-            parent.setWritable(false, false);
-            File tmp = new File(parent, "tmp");
-            System.setProperty("java.io.tmpdir", tmp.toString());
-
-            mockBundleContext.addBundleListener((BundleListener) org.easymock.EasyMock.anyObject());
-            EasyMock.expect(mockBundleContext.createFilter((String) EasyMock.anyObject()))
-                    .andReturn(null);
-
-            BundleRevision mockBundleRevision = EasyMock.createNiceMock(BundleRevision.class);
-            EasyMock.expect(mockBundle.adapt(BundleRevision.class)).andReturn(mockBundleRevision);
-            EasyMock.expect(mockBundleRevision.getTypes())
-                    .andReturn(BundleRevision.TYPE_FRAGMENT);
-            EasyMock.replay(mockBundleContext, mockBundle, mockBundleRevision, mockSysBundle, mockStartLevel);
-    
-            try
-            {
-                dw = new DirectoryWatcher( new FileInstall(), props, mockBundleContext );
-                fail("Expected an IllegalStateException");
-            } 
-            catch (IllegalStateException e)
-            {
-                // expected
-            }
+            dw = new DirectoryWatcher( new FileInstall(), props, mockBundleContext );
+            fail("Expected a RuntimeException for a temp directory that cannot be created");
         }
-        finally
+        catch (RuntimeException e)
         {
-            System.setProperty("java.io.tmpdir", oldTmpDir);
+            // expected
         }
     }
 

[Jefiya-MJ]

Hi @paulrutter

Thanks for the review! I have addressed all the feedback and rebased onto latest master:

READ_ONLY leak fixed: Moved the ownership check before clearReadOnlyIfWritable() in setConfig() and added a test to ensure READ_ONLY attributes are preserved.
Normalized file comparison: Switched from URI string equality to normalized File paths in setConfig() and deleteConfig() to handle ./.. segments and equivalent path representations.
Logging & Javadoc: Added WARNING logs with PID and owner details when skipping updates/deletions, and updated deleteConfig() Javadoc.
CI & DirectoryWatcherTest: Resolved merge conflicts, wired fileinstall into GitHub Actions CI (clean test), and fixed DirectoryWatcherTest.testInvalidTempDir with a deterministic fixture.

[paulrutter]

Running ci and will merge on a pass