Fix race condition corrupting cfg files when updating an existing configuration - #2897
Merged
jbonofre merged 3 commits intoSep 14, 2026
Merged
Conversation
…figuration FeatureConfigInstaller.updateExistingConfig() wrote directly to the target cfg file when appending to / overriding an existing configuration. Since cfg.update() also triggers fileinstall to persist the same configuration on the CM Event Dispatcher thread, two unsynchronized writers could hit the same file at once, producing torn/corrupted content. The "new file" path already avoided this by writing to a temp file and renaming it atomically into place (KARAF-7389); this applies the same pattern to the existing-file path. Fixes apache#2805
…enameTo File.renameTo() silently fails (returns false, no exception) on Windows when the destination already exists, leaving the cfg file stale and the temp file orphaned. This broke updateExistingConfig() since it is only called when the target file already exists, causing the Windows CI job to fail (AppendTest).
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Atomic-move portability, file-permission preservation, and regression-test effectiveness remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds atomic temporary-file replacement to prevent configuration corruption during concurrent writes.
Changes:
- Replaces direct cfg writes with atomic file moves.
- Adds a regression-oriented test for existing properties files.
File summaries
| File | Description |
|---|---|
FeatureConfigInstaller.java |
Writes new and updated configurations through temporary files. |
FeatureConfigInstallerTest.java |
Tests merged properties and temporary-file cleanup. |
Review details
Suppressed comments (1)
features/core/src/main/java/org/apache/karaf/features/internal/service/FeatureConfigInstaller.java:418
- This second unconditional
ATOMIC_MOVEhas the same portability failure: an unsupported provider leaves the existing file stale after the in-memory Configuration Admin update, while the exception is merely logged. Add the establishedAtomicMoveNotSupportedExceptionfallback here as well.
Files.move(tmpCfgFile.toPath(), cfgFile.toPath(),
StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2805.
FeatureConfigInstaller.updateExistingConfig()(used both for theappendandoverrideconfig flows) read the existing cfg file, merged in the new properties in memory, then wrote the result directly back to the target file (properties.save(cfgFile)/Configurations.buildWriter().build(new FileWriter(cfgFile))...).Calling
cfg.update(...)just before that write also triggers Configuration Admin to notify its persistence manager (fileinstall), which persists the very same configuration to the very same cfg file on the CM Event Dispatcher thread. With two unsynchronized, non-atomic writers hitting the same file at once, the writes can interleave and corrupt the file (as shown in the issue, where lines from the old file content bleed into the newly written content).The "file doesn't exist yet" path already avoids this by writing to a temp file and renaming it atomically into place (KARAF-7389 / #1489), but that fix didn't cover the "file already exists" path exercised by
append/override. This PR applies the same temp-file + atomic rename pattern toupdateExistingConfig(), for both the properties and JSON formats, so a concurrent reader/writer can only ever observe a fully-old or fully-new file, never a torn one.Test plan
mvn -pl features/core test— all existing tests pass (139 tests)FeatureConfigInstallerTest#testUpdateExistingConfigWritesAtomically, which exercisesupdateExistingConfig()against a pre-existing cfg file and asserts the merged content is correct and no leftover.tmpfile remains after the rename