Skip to content

Detect concurrent modification of the library file when saving - #16646

Merged
Siedlerchr merged 13 commits into
mainfrom
concurrent-save-conflict
Aug 24, 2026
Merged

Detect concurrent modification of the library file when saving#16646
Siedlerchr merged 13 commits into
mainfrom
concurrent-save-conflict

Conversation

@koppor

@koppor koppor commented Aug 22, 2026

Copy link
Copy Markdown
Member

🤖 Summary

When two JabRef instances (e.g., on two machines sharing a .bib file on a network share) save the same library at overlapping times, the save finishing last silently overwrote the other instance's changes — and on the in-place overwrite path introduced for hard links / non-atomic file systems, two concurrent writers could even interleave into a corrupted file. AtomicFileOutputStream now snapshots the target file's existence, size, and modification time when the stream is opened and verifies it again before committing; on a mismatch it aborts with the new FileChangedException, leaving the concurrently written file intact. Instead of an exception dialog, the user gets a proper flow: SaveDatabaseAction shows a "Library was not saved: the file was modified by another program." notification, and the standard "External changes detected" review flow is offered for the concurrent write. The user's changes stay in memory: they can review/merge the external changes and save again.

This PR also integrates #16613 (co-authored by @Siedlerchr) and therefore closes it: DatabaseChangeMonitor now stays registered during saves and only defers event handling, scanning once on resume — so filesystem events arriving mid-save are no longer lost (previously the monitor was unregistered and recreated, silently dropping them). On top of #16613's approach, a cheap size+modification-time guard skips the full library parse whenever the file still matches the last state known to be consistent with memory (library load, successful save, all external changes merged) — so JabRef's own writes do not cause a re-parse of a large library after every save, and the post-save scan only runs when something external actually changed the file. As a drive-by, the different-encoding retry in SaveDatabaseAction was moved out of the writer's try-with-resources, because the outer writer's close() previously re-committed the badly encoded content after the re-encoded save (and would now trip the conflict detection).

The guard state is modeled explicitly as optimistic concurrency control: a single public value type org.jabref.logic.util.io.FileSnapshot (existence + size + mtime — the version stamp) is the one implementation of the baseline concept and flows through the layers instead of being re-derived. AtomicFileOutputStream verifies the target against an expected snapshot before committing (accepting an inherited baseline, so a follow-up writer can span a longer logical operation such as the encoding-retry) and exposes the snapshot of what it committed; SaveDatabaseAction threads that committed snapshot through the encoding dialogs into the retry writer and finally into LibraryTab.resetChangedProperties(FileSnapshot)DatabaseChangeMonitor.markConsistentWithDisk(...), so the monitor knows the post-save disk state without re-reading the file. The detection is best-effort (size + mtime, subject to file system timestamp resolution), which is documented on FileSnapshot and in a new requirement req~logic.exporter.concurrent-save-detection~1.

The encoding-problems dialogs are covered by the same mechanism: an external save landing while they are open (they can sit for hours) aborts the retry — and, without a retry ("Ignore"/cancel), aborts reporting success — through the regular conflict path, with regression tests for both.

jabref-contrib-policy:4.2:reviewed​:ok

Analogies: This PR is like honey, because a save should preserve everything the bees (both instances) collected instead of letting one hive raid the other. It is like chocolate, because atomic commits should stay whole — nobody wants two half-melted bars swirled into one. And it is like the moon, because two writers orbiting the same file must not eclipse each other's changes.

Steps to test

  1. Open a reasonably large library (the bigger, the longer the save window; tens of thousands of entries make it easy to hit).
  2. Make a change in JabRef so the library is marked as modified.
  3. Save (Ctrl+S) and, while the save is running, modify the .bib file externally (e.g. echo "% external" >> library.bib in a terminal — a loop appending every 100 ms makes the timing trivial).
  4. JabRef aborts the save and shows the notifications below: the save was not performed, and the standard "External changes detected" notification offers "Review changes" to merge the concurrent modifications. The externally modified file is left untouched, and your changes remain in memory, so after reviewing you can simply save again.
  5. Regression check: a normal save without concurrent modification still works ("Library saved").

The two notifications the aborted save produces (captured from a live run with a 60k-entry library and a shell loop appending to the file during the save):

Save aborted: "Library was not saved: the file was modified by another program." plus the standard "External changes detected" notification

The interleaved two-writers scenario is covered by unit tests in AtomicFileOutputStreamTest (interleavedSavesDoNotOverwriteEachOther, externalChangeOfTargetAbortsSave, externalCreationOfTargetAbortsSave, externalDeletionOfTargetAbortsSave).

Related issues and pull requests

Closes #16613 — its "keep external change detection active while saving" approach is integrated here (with an added size+mtime guard that avoids re-parsing the library after every save), so that PR is superseded by this one.

Related: #16610, #7718 (the in-place save fallback for hard links / non-atomic file systems enlarged the window in which concurrent writers could corrupt the file; this PR adds the missing lost-update guard). No existing issue found for the silent lost-update itself.

AI usage

Claude Code (model claude-fable-5) — investigation, implementation, and tests were AI-generated under human direction (AIL3); reviewed and owned by the contributor.

AI CHECKLIST.md walkthrough

1. Code self-review

Nullability and control flow

  • No == null / != null checks — JSpecify annotations (@NullMarked, @Nullable, @NonNull) used instead. (The only null checks are on values declared @Nullable, consistent with the existing checks in AtomicFileOutputStream.)
  • No Objects.requireNonNull(...) — nullability expressed via JSpecify annotations.
  • New classes annotated with @NullMarked (org.jspecify.annotations.NullMarked).
  • [/] Optional consumed with ifPresent / ifPresentOrElse / map / orElseThrow — never orElse(unusedValue) nor an isPresent() + get() block.
  • [/] StringUtil.isBlank(...) used instead of s == null || s.isBlank().

Exceptions

  • No catch (Exception e) — only specific exceptions are caught.
  • No throw new RuntimeException(...) / IllegalStateException(...) — these tear down the whole application.
  • Logged exceptions are passed as the last logger argument (LOGGER.info("...", e)), not concatenated into the message string.

Style and idioms

  • [/] New BibEntry objects built with withers (withField, not setField).
  • Modern Java used: List.of() / Map.of() / Set.of(), Path.of(), SequencedCollection / SequencedSet, text blocks. (New snapshot type is a record; Set.of() used in SaveDatabaseAction.)
  • [/] Regexes use a precompiled Pattern.compile(...) constant, not String.matches(...).
  • [/] Background work uses org.jabref.logic.util.BackgroundTask, not new Thread().
  • No commented-out code, no trivial comments restating the code, no AI-disclosure comments in source.
  • Markdown Javadoc (///) uses Markdown syntax, not JavaDoc inline tags: `code` instead of {@code}, [ClassName] instead of {@link}.

User-facing text

  • All user-facing text localized (Localization.lang in Java, % prefix in FXML).
  • Sentence case (not Title Case); no trailing !; labels do not end with :.
  • [/] Variance expressed with placeholders ("...: %0"), not string concatenation. (The new message has no variable parts.)

Security

  • [/] User-controlled data (request params, entry fields, file contents) is HTML-escaped before being written into any text/html response — including exception/error messages, not just the success body (XSS).

Tests

  • Behavior changes in org.jabref.model / org.jabref.logic have added or updated tests.
  • Tests assert object contents (assertEquals), use plain JUnit asserts (not AssertJ), have no @DisplayName, do not catch exceptions (let them propagate so JUnit reports setup/teardown failures directly), and use @TempDir instead of manual temp directories.

2. Verification commands

  • ./gradlew :jablib:check (or ./gradlew check for all modules).
  • ./gradlew checkstyleMain checkstyleTest checkstyleJmh.
  • ./gradlew modernizer (run for the touched modules jablib and jabgui).
  • ./gradlew --no-configuration-cache :rewriteDryRun reports no changes (run ./gradlew rewriteRun to fix).
  • ./gradlew javadoc (run for the touched modules jablib and jabgui).
  • npx markdownlint-cli2 "docs/**/*.md" "*.md" (only if Markdown changed — run on the changed CHANGELOG.md and docs/requirements/save.md).
  • [/] Only if formatting is still off after rewriteRun: docker run ... intellij-format (no Docker available; formatted with local IntelliJ IDEA using .idea/codeStyles/Project.xml instead).

3. Documentation

4. Pull request

  • PR body built from .github/PULL_REQUEST_TEMPLATE.md, every section filled.
  • All checklist items kept and marked [x], [ ], or [/].
  • All HTML comments removed from the PR body.
  • PR created with gh pr create --body-file <file> (not --body).
  • If CHANGELOG.md used a TODO placeholder, it was replaced with the real PR-number link after PR creation, then committed and pushed.

Checklist

  • I own the copyright of the code submitted and I license it under the MIT license
  • If AI tools were used, I disclosed them in the "AI usage" section and reviewed, understood, and take full ownership of all AI-generated code
  • I manually tested my changes in running JabRef (always required)
  • I added JUnit tests for changes (if applicable)
  • I added screenshots in the PR description (if change is visible to the user)
  • [/] I added a screenshot in the PR description showing a library with a single entry with me as author and as title the issue number
  • I described the change in CHANGELOG.md in a way that can be understood by the average user (if change is visible to the user)
  • [/] I checked the user documentation for up to dateness and submitted a pull request to our user documentation repository (no user documentation describes concurrent saving; the new error message is self-explanatory)

🤖 Generated with Claude Code

koppor and others added 3 commits August 22, 2026 13:43
Two JabRef instances (or any other program) writing the same .bib file
could silently lose one side's changes: AtomicFileOutputStream committed
whatever was written last, and on the in-place overwrite path (hard
links, non-atomic file systems) two concurrent writers could even
interleave into a corrupted file.

AtomicFileOutputStream now snapshots the target file's existence, size,
and modification time when the stream is opened and verifies it again
before committing (once before creating the backup, once right before
the move/overwrite). On a mismatch it throws the new
FileChangedException, keeping the concurrently written file intact;
SaveDatabaseAction surfaces this as a "file was modified by another
program" error to the user, whose changes remain in memory.

The encoding-problem retry in SaveDatabaseAction is moved out of the
writer's try-with-resources: previously the outer writer's close()
committed the badly encoded content after the re-encoded save, silently
clobbering it (and it would now trip the conflict detection).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
…write

Instead of surfacing the FileChangedException in an exception dialog,
SaveDatabaseAction now shows a short notification that the library was
not saved and triggers the standard "External changes detected" review
flow. The change monitor is suspended during saving and thus never sees
the concurrent write itself, so LibraryTab gains scanForExternalChanges()
to start that scan manually.

Also adds the requirement req~logic.exporter.concurrent-save-detection~1
with impl/utest traces.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Abort save when the library file changes during write (concurrent-save guard)

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Detect concurrent modifications of the target .bib during save and abort the commit.
• Show a user-friendly notification and trigger the existing external-changes review flow.
• Add tests plus requirement/changelog updates documenting the best-effort detection.
Diagram

graph TD
A["SaveDatabaseAction"] --> B["AtomicFileWriter"] --> C["AtomicFileOutputStream"] --> D[("Target .bib file")]
C --> E{{"FileChangedException"}} --> H["SaveDatabaseAction (conflict handling)"] --> F["LibraryTab"] --> G["DatabaseChangeMonitor"]

subgraph Legend
  direction LR
  _ui["UI/Action"] ~~~ _svc["Logic component"] ~~~ _file[("File")] ~~~ _ex{{"Exception"}}
end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use OS-level file locking (FileChannel/FileLock) during save
  • ➕ Can prevent concurrent writers instead of detecting after the fact
  • ➕ Avoids best-effort timestamp limitations
  • ➖ Unreliable/variable semantics on network shares and across platforms
  • ➖ Risk of blocking UI or leaving stale locks after crashes
2. Compare a content hash (or full byte compare) before commit
  • ➕ More robust than size+mtime (detects same-size edits within timestamp resolution)
  • ➕ Still avoids locking
  • ➖ Requires reading the entire target file; expensive for large libraries
  • ➖ Still has a check-to-commit race unless combined with other mechanisms
3. Write a sidecar version marker (e.g., .bib.jabref-save-token) and validate it
  • ➕ Can be more reliable than filesystem timestamp resolution
  • ➕ Keeps main file unchanged until commit
  • ➖ Introduces extra files and cleanup concerns
  • ➖ Potentially surprising for users and external tooling

Recommendation: The PR’s approach (best-effort size+mtime snapshot with a late re-check) is a pragmatic, low-overhead guard that works across common filesystems and directly fixes the lost-update/corruption risk introduced by overlapping saves. Given cross-platform and network-share constraints, locking is likely to cause more usability issues than it solves; hashing would be stronger but too costly for large libraries. The chosen solution is appropriate, especially with the added UI flow that preserves in-memory changes and funnels users into the existing external-change review/merge workflow.

Files changed (8) +162 / -8

Bug fix (4) +96 / -5
LibraryTab.javaExpose scanForExternalChanges hook for post-save conflict handling +7/-0

Expose scanForExternalChanges hook for post-save conflict handling

• Adds a method that triggers the change monitor’s fileUpdated scan manually. This enables the standard external-changes notification/review flow even when the monitor was suspended during save.

jabgui/src/main/java/org/jabref/gui/LibraryTab.java

SaveDatabaseAction.javaHandle FileChangedException with notification + external-change review flow +22/-5

Handle FileChangedException with notification + external-change review flow

• Detects concurrent-save conflicts (via FileChangedException wrapped in SaveException) and avoids showing a generic error dialog. After resuming the change monitor, it notifies the user and triggers an explicit external-change scan so users can review/merge while keeping in-memory edits. Also moves the encoding-problem retry outside try-with-resources to avoid the outer writer re-committing problematic content.

jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java

AtomicFileOutputStream.javaSnapshot target file and abort commit if it changed during save +51/-0

Snapshot target file and abort commit if it changed during save

• Captures target existence/size/mtime when the stream opens and verifies it again before backup creation and again right before commit. On mismatch, throws FileChangedException to prevent overwriting or interleaving concurrent writes, while leaving the on-disk file intact and avoiding stray backup/temp artifacts.

jablib/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java

FileChangedException.javaIntroduce exception signaling concurrent modification during atomic save +16/-0

Introduce exception signaling concurrent modification during atomic save

• Adds a dedicated IOException subtype thrown when the target file changes between opening and committing an AtomicFileOutputStream. Used to distinguish concurrent-save conflicts from generic IO failures.

jablib/src/main/java/org/jabref/logic/exporter/FileChangedException.java

Tests (1) +49 / -3
AtomicFileOutputStreamTest.javaAdd tests for concurrent-save conflict detection and cleanup behavior +49/-3

Add tests for concurrent-save conflict detection and cleanup behavior

• Expands tests to assert that interleaved saves do not overwrite each other and that external create/change/delete of the target aborts commit. Also verifies that aborted saves do not leave temporary or backup files behind.

jablib/src/test/java/org/jabref/logic/exporter/AtomicFileOutputStreamTest.java

Documentation (2) +16 / -0
CHANGELOG.mdDocument concurrent-save overwrite fix in changelog +1/-0

Document concurrent-save overwrite fix in changelog

• Adds a Fixed entry describing that concurrent saves no longer silently overwrite each other. Notes that the last-finishing save now aborts and offers the external-changes review flow.

CHANGELOG.md

save.mdAdd requirement for concurrent-save detection on commit +15/-0

Add requirement for concurrent-save detection on commit

• Introduces a new requirement specifying that saves must abort if the target file changes during the save window. Documents best-effort detection based on size and modification time and traces to implementation/tests.

docs/requirements/save.md

Other (1) +1 / -0
JabRef_en.propertiesAdd localized message for aborted save due to external modification +1/-0

Add localized message for aborted save due to external modification

• Introduces the user-facing notification string: "Library was not saved: the file was modified by another program."

jablib/src/main/resources/l10n/JabRef_en.properties

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Stray backup on conflict ✓ Resolved 🐞 Bug ☼ Reliability
Description
AtomicFileOutputStream can throw FileChangedException on the second ensureTargetFileUnchanged()
after createBackup() already ran, and in that error path the newly created .sav backup is never
deleted. This can leave misleading/unused backup files behind when a concurrent modification is
detected late in close().
Code

jablib/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java[R312-315]

+            // Re-check right before the commit: creating the backup of a large file can take a while, so the first
+            // check may be long in the past by now
+            ensureTargetFileUnchanged();
+
Evidence
The new second re-check is executed after createBackup(); if it throws, execution skips the normal
path that deletes backups, and the finally block only removes the temporary file (not the backup).

jablib/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java[289-345]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`AtomicFileOutputStream.close()` performs a second concurrent-change check (`ensureTargetFileUnchanged()`) *after* `createBackup()` has potentially created/overwritten the `.sav` file. If that second check throws `FileChangedException`, the method exits via exception and never reaches the normal backup cleanup (`Files.deleteIfExists(backupFile)`), leaving a stray backup behind even though the save was aborted.
## Issue Context
This shows up when the target file is modified *during* the backup-copy window (or after the first check but before the second). The current code’s comment says the pre-backup check avoids leaving backups behind, but the post-backup check can still abort after the backup exists.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/exporter/AtomicFileOutputStream.java[289-345]
## Suggested change
Wrap the section after `createBackup()` in a `try { ... } catch (FileChangedException e) { ... }` and, when aborting due to `FileChangedException`, delete the backup if it was created by this attempt and `!keepBackup` (or otherwise track whether the backup was created/overwritten in this close). Ensure this cleanup does not interfere with the existing semantics of keeping backups for other I/O failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Wrong file scanned after Save As ✓ Resolved 🐞 Bug ≡ Correctness
Description
On FileChangedException, SaveDatabaseAction always calls libraryTab.scanForExternalChanges(), but
the DatabaseChangeMonitor is recreated from bibDatabaseContext.getDatabasePath() (the currently-open
library path), not the save target path. During “Save as” failures the context path is not updated
(only set on success), so the triggered review flow may scan the original library file instead of
the conflicted targetPath.
Code

jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[R259-264]

+            if (fileChangedDuringSave) {
+                dialogService.notify(Localization.lang("Library was not saved: the file was modified by another program."));
+                // The change monitor was suspended during the save and thus never saw the concurrent write; trigger
+                // the scan manually so the user gets the standard external-changes review flow
+                libraryTab.scanForExternalChanges();
+            }
Evidence
The scan call is unconditional on conflict; resumeChangeMonitor() rebuilds the monitor based on
the context’s current database path, and saveAs only sets the context path on success, so conflict
during Save As leaves the monitor/scan targeting the old path.

jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[213-265]
jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[135-170]
jabgui/src/main/java/org/jabref/gui/LibraryTab.java[820-833]
jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[43-70]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When a save is aborted due to concurrent modification, `SaveDatabaseAction.save(...)` unconditionally triggers `libraryTab.scanForExternalChanges()`. But that scan operates through `DatabaseChangeMonitor`, which is created based on `bibDatabaseContext.getDatabasePath()` (the current library path). In `saveAs(...)`, the context database path is only updated after a successful save; therefore, on a Save As conflict/abort, the scan is performed against the *old* library path (or does nothing), not the `targetPath` that actually conflicted.
## Issue Context
This can produce confusing or incorrect behavior: the user gets the “file modified by another program” flow, but the follow-up “review changes” is not necessarily comparing against the file that triggered the conflict.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[213-265]
- jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java[135-170]
- jabgui/src/main/java/org/jabref/gui/LibraryTab.java[808-833]
- jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java[43-70]
## Suggested change
In `save(Path targetPath, ...)` only call `scanForExternalChanges()` when `libraryTab.getBibDatabaseContext().getDatabasePath()` is present and equals `targetPath` (the normal “Save” case). For Save As conflicts (targetPath differs), skip the external-change scan and instead keep the existing notification (or implement a dedicated scan/review path that compares against `targetPath`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

koppor and others added 3 commits August 22, 2026 14:10
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
… aborts

The re-check after createBackup() could throw FileChangedException with
the .sav file of the aborted attempt left on disk, although no save
happened and the target is untouched. Backups are still kept for actual
commit failures (where they have recovery value) and with keepBackup,
where a backup file is expected to persist across saves.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
The DatabaseChangeMonitor is created from the context's database path,
which is only updated after a successful "Save as". A conflict abort in
that flow would therefore scan the old library file instead of the path
that actually conflicted. The scan is now only triggered for plain saves
(target path equals the context's database path); a "Save as" conflict
keeps just the notification.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
Integrates the approach of PR #16613: instead of unregistering the
DatabaseChangeMonitor around a save and recreating it afterwards (losing
every event in between), the watcher stays registered and only defers
handling; one scan runs on resume. On top of that, a cheap size+mtime
guard skips the full library parse when the file matches the last state
known to be consistent with memory (library load, successful save, all
external changes merged), so JabRef's own writes do not trigger scans.

This makes the explicit post-conflict scan in SaveDatabaseAction (and
its Save-As guard) redundant: resuming the monitor now performs it,
against the correct (monitored) path.

Co-authored-by: Siedlerchr <siedlerkiller@gmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
@Siedlerchr

Copy link
Copy Markdown
Member

Save-path review findings

Scope: current branch compared with upstream/main, with emphasis on AtomicFileWriter and the library save flow.

F-01: Encoding retry can overwrite an external save

  • Severity: blocker
  • Location: jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java:299
  • Impact: a library saved with an encoding that cannot represent one or more characters can silently overwrite another program's completed save. The user's external changes are lost.

Failure sequence

  1. JabRef writes and commits a first version using the selected encoding, replacing unsupported characters.
  2. The encoding dialog remains open while the user chooses a different encoding.
  3. Another program or JabRef instance saves the same library successfully.
  4. JabRef starts a new AtomicFileWriter for the encoding retry. Its concurrent-change snapshot now treats the external version as the expected starting state.
  5. The re-encoded retry commits and replaces that external version without detecting a conflict.

Required correction

Do not commit the first, lossy write when encoding problems are found. Abort it before prompting for a different encoding, or preserve and verify a save-generation token from before the first write across the retry. Add a regression test that performs an external save while the encoding-selection dialog is open and verifies that the retry aborts rather than overwriting it.

@qodo what do you think about this?

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Save-path review findings

Scope: current branch compared with upstream/main, with emphasis on AtomicFileWriter and the library save flow.

F-01: Encoding retry can overwrite an external save

  • Severity: blocker
  • Location: jabgui/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java:299
  • Impact: a library saved with an encoding that cannot represent one or more characters can silently overwrite another program's completed save. The user's external changes are lost.

Failure sequence

  1. JabRef writes and commits a first version using the selected encoding, replacing unsupported characters.
  2. The encoding dialog remains open while the user chooses a different encoding.
  3. Another program or JabRef instance saves the same library successfully.
  4. JabRef starts a new AtomicFileWriter for the encoding retry. Its concurrent-change snapshot now treats the external version as the expected starting state.
  5. The re-encoded retry commits and replaces that external version without detecting a conflict.

Required correction

Do not commit the first, lossy write when encoding problems are found. Abort it before prompting for a different encoding, or preserve and verify a save-generation token from before the first write across the retry. Add a regression test that performs an external save while the encoding-selection dialog is open and verifies that the retry aborts rather than overwriting it.

@qodo what do you think about this?

F-01 is valid and should be treated as a blocker. The current flow reads encodingProblems only after the AtomicFileWriter try-with-resources has closed, so the lossy first encoding has already been committed before saveWithDifferentEncoding(...) prompts and retries. Because the outer save keeps change detection suspended, an external save during that dialog is not reviewed; the retry creates a new snapshot from that external version and can overwrite it successfully.

The safest correction is to make encoding selection happen before any target-file commit: write/validate the candidate encoding to a temporary output, inspect its encoding problems, prompt if necessary, and commit only the final encoding. If that is impractical, the retry must carry forward and revalidate the original pre-save file-generation token rather than taking a fresh snapshot. A regression test should place an external replacement between the first write and the retry, then assert that the retry aborts and the external content remains intact.

koppor and others added 4 commits August 23, 2026 20:09
Addresses review finding F-01: the encoding-retry writer took its
concurrent-change snapshot only after the encoding dialogs were closed,
so an external save landing while they were open became the retry's
baseline and was overwritten without detection. The retry now verifies
that the file still matches the state committed by the first write and
otherwise aborts through the regular concurrent-save-conflict path
(notification plus external-changes review flow on monitor resume).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
The guard from the previous commit only protected the "Try different
encoding" path. When the user instead clicks "Ignore" (or cancels the
encoding choice) after the dialog sat open for a long time, an external
save in that window had replaced the committed first write — yet the
save was reported successful, marking the library clean and recording
the external state as consistent: no review flow, and closing JabRef
would discard the in-memory changes without any prompt. Every exit of
the dialog flow now verifies the file still matches the committed write
and otherwise aborts through the concurrent-save-conflict path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
… type

The size+mtime baseline concept existed as three private copies
(AtomicFileOutputStream, DatabaseChangeMonitor, SaveDatabaseAction).
It is now a single public value type, org.jabref.logic.util.io
.FileSnapshot — a version stamp for optimistic concurrency control on
files — and the guard information flows through the layers instead of
being re-derived:

- AtomicFileOutputStream accepts an inherited expected baseline (so a
  follow-up writer can span a longer logical operation) and exposes the
  state it committed, captured immediately after the commit.
- SaveDatabaseAction passes the committed state of the first write as
  the encoding retry's baseline (the retry writer now detects the
  conflict itself) and hands the final committed state to LibraryTab.
- LibraryTab.resetChangedProperties(FileSnapshot) forwards it to
  DatabaseChangeMonitor.markConsistentWithDisk, which no longer has to
  re-read the file after a save — closing the window in which a
  concurrent write could be mistaken for the consistent state.

The monitor now also correctly treats "file absent before and after" as
unchanged (no scan), which the adapted tests cover.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
@koppor

koppor commented Aug 23, 2026

Copy link
Copy Markdown
Member Author

🤖 Generated with Claude Code

TL;DR: F-01 confirmed and fixed — for the retry path and the "Ignore"/cancel paths — via the suggested save-generation token, generalized into a single guard mechanism.

Problem. The retry writer took its conflict baseline after the encoding dialogs closed, so an external save landing while they were open (minutes to hours) became the baseline and was overwritten undetected. The same window also affected "Ignore"/cancel: the save reported success although the committed write had already been replaced, marking the library clean — no review flow, and closing JabRef would have discarded the in-memory changes.

Fix. One concept, one implementation, enforced in one place:

  • FileSnapshot (org.jabref.logic.util.io) — existence + size + mtime as an optimistic-concurrency version stamp.
  • AtomicFileOutputStream — the sole enforcement point: verifies the target against an expected snapshot before committing, accepts an inherited baseline (so a writer can guard an operation that started before its construction), and exposes the snapshot of what it committed (captured directly after the commit).
  • SaveDatabaseAction — threads the first write's committed snapshot through the dialogs: as the retry writer's baseline (retry conflict detected by the writer itself), and as an explicit check when no retry happens. Both abort through the regular conflict path: save fails, library stays dirty, external-changes review flow is offered.
  • The final committed snapshot flows on to DatabaseChangeMonitor.markConsistentWithDisk(...), so the monitor knows the post-save disk state without re-reading the file.

Tests. SaveDatabaseActionTest#encodingRetryAbortsWhenFileWasSavedExternallyWhileDialogWasOpen (required regression test: external save during dialog → retry aborts, external content intact) and #ignoredEncodingProblemsReportFailureWhenFileWasSavedExternallyWhileDialogWasOpen; writer-level coverage in AtomicFileOutputStreamTest (inherited baseline, committed snapshot).

Not chosen: aborting the first, lossy write before prompting — it would require holding the writer open across modal dialogs and would change the long-standing "Ignore commits the replacement characters" behavior, without adding protection beyond the token.

koppor and others added 2 commits August 23, 2026 21:23
The pinned intellij-format container (IDEA 2025.3.1) aligns wrapped
@PARAM descriptions differently than local IDEA; this is its exact
suggested diff. IDEA 2026.2.1 accepts both forms (verified locally), so
this stays stable across a future formatter bump.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
The CI formatter (IDEA 2025.3.1) re-indents wrapped @PARAM continuation
lines deeper on every run (IDEA-383594), so no wrapped form can ever
pass: applying the format job's suggested diff made the next run demand
yet deeper indentation. Unwrapped single-line descriptions are stable
across IDEA 2025.3.1 and 2026.2.1.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pZNCbTYVPumAQAKcnHRht
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 23, 2026
@Siedlerchr
Siedlerchr enabled auto-merge August 24, 2026 14:11
@Siedlerchr
Siedlerchr added this pull request to the merge queue Aug 24, 2026
@github-actions github-actions Bot added the status: to-be-merged PRs which are accepted and should go into the merge-queue. label Aug 24, 2026
Merged via the queue into main with commit 99b4bae Aug 24, 2026
90 checks passed
@Siedlerchr
Siedlerchr deleted the concurrent-save-conflict branch August 24, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: export-or-save status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers status: to-be-merged PRs which are accepted and should go into the merge-queue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants