Fix/16679 ocr failure command output - #16682
Conversation
Surface the executed command and its captured stdout/stderr in the OCR failure dialog, so failures give something to debug instead of just "OCR process failed" or similar. OcrResult.Failure now carries the command line and output; OcrUtils.performOcr captures the process output into a buffer and joins the reader thread before returning, to avoid a race between it and the failure result being read. Fixes JabRef#16679
Surface the executed command and its captured stdout/stderr in the OCR failure dialog, so failures give something to debug instead of just "OCR process failed" or similar. OcrResult.Failure now carries the command line and output; OcrUtils.performOcr captures the process output into a buffer and waits for the output-draining task via HeadlessExecutorService before returning, avoiding a race between it and the failure result being read. Also switched the output-draining task from a raw new Thread() to HeadlessExecutorService after re-checking CHECKLIST.md caught that this project avoids new Thread() for background work. Fixes JabRef#16679
Adds unit tests for the IO_ERROR, NON_ZERO_EXIT, and success paths of OcrUtils.performOcr, verifying the captured command line and output. Also adds the CHANGELOG.md entry required by CONTRIBUTING.md, which the previous commits were missing.
github-actions
Bot
added
component: internationalization
|
Hey @Fayupable! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
github-actions
Bot
added
first contrib
good first issue
PR Summary by QodoSurface OCR command output in failure dialogs
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Gobbler interruption permits racy read
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
1. Gobbler interruption permits racy read 📎 Requirement gap ☼ Reliability
awaitGobblerQuietly returns after an interrupted Future.get without ensuring the gobbler stopped, so performOcr can read the unsynchronized StringBuilder while output is still appended. The resulting OCR failure dialog can contain incomplete or corrupted diagnostic output, and the caught interruption is not logged.
Agent Prompt
## Issue description
An interruption while awaiting the output gobbler is swallowed after restoring the interrupt flag, allowing callers to read `outputBuilder` before the gobbler has completed and leaving the caught exception unlogged.
## Issue Context
The failure result must contain deterministic captured output, and every caught exception must be logged with the throwable as the final argument. Refactor the waiting/cleanup flow so output is never read concurrently, while preserving interruption semantics.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ocr/OcrUtils.java[99-119]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// Contains the reason why the failure occurred that the GUI part can localize it and output to the user, | ||
| /// plus the command line that was executed and its captured output, when available, so the GUI can show | ||
| /// them for debugging. Both are empty strings when the failure occurred before a command could be run. | ||
| record Failure(OcrFailureReason reason, String commandLine, String output) implements OcrResult { |
There was a problem hiding this comment.
2. Failure fields lack null contracts 📘 Rule violation ≡ Correctness
The new public commandLine and output record components are added in code without a null-marked scope or explicit JSpecify annotations. Callers therefore cannot rely on the non-null contract required before invoking methods such as isEmpty().
Agent Prompt
## Issue description
The newly introduced `commandLine` and `output` public record components have unspecified nullability.
## Issue Context
The implementation constructs non-null strings and GUI callers immediately dereference them, so encode that contract using the repository's JSpecify conventions, preferably by establishing an appropriate null-marked scope.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ocr/OcrResult.java[5-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Command=Command | ||
| Output=Output |
There was a problem hiding this comment.
3. Localization keys miss locale bundles 📘 Rule violation ⚙ Maintainability
The new Command and Output keys were added only to the English bundle, while every translated JabRef_<lang>.properties bundle lacks them. This violates the required key synchronization and causes non-English locales to fall back instead of providing corresponding translations.
Agent Prompt
## Issue description
The new localization keys exist in the English source bundle but are absent from translated locale bundles.
## Issue Context
Keep the exact `Command` and `Output` keys synchronized across all locale property files according to the repository's localization workflow.
## Fix Focus Areas
- jablib/src/main/resources/l10n/JabRef_en.properties[785-786]
- jablib/src/main/resources/l10n/JabRef_ar.properties[1-1]
- jablib/src/main/resources/l10n/JabRef_de.properties[1-1]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return; | ||
| } | ||
| try { | ||
| gobblerFuture.get(5, TimeUnit.SECONDS); |
There was a problem hiding this comment.
5. Gobbler race remains 🐞 Bug ☼ Reliability
awaitGobblerQuietly suppresses its five-second timeout, after which performOcr immediately calls toString() on the same non-thread-safe StringBuilder the still-running gobbler may be appending to. A slow or pipe-inheriting process can therefore produce incomplete or inconsistent failure output despite this change's stated race avoidance.
Agent Prompt
## Issue description
The bounded gobbler wait can return while the asynchronous task is still mutating `outputBuilder`, so subsequent reads remain racy and may return incomplete output.
## Issue Context
`StreamGobbler` invokes the append callback from the shared executor. Swallowing `TimeoutException` does not establish task completion or safe publication.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/ocr/OcrUtils.java[65-82]
- jablib/src/main/java/org/jabref/logic/ocr/OcrUtils.java[85-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
github-actions
Bot
added
the
status: changes-required
subhramit
left a comment
subhramit
left a comment
There was a problem hiding this comment.
@koppor can you try out this PR and see what error message you get?
@ZiadAbdElFatah this needs a deeper look, perhaps. Seems more complex than needed?
|
|
||
| /// Waits briefly for the output-gobbler task to finish, so its buffered output is safe to | ||
| /// read afterward. A null future (the process never started) is a no-op. | ||
| private static void awaitGobblerQuietly(Future<?> gobblerFuture) { |
There was a problem hiding this comment.
This seems really unnecessary for a synchronous task.
I addressed feedback from Qodo's automated review and @subhramit on PR JabRef#16682: - I collapsed the two separate waits (process.waitFor() + a fixed 5s gobbler wait) into a single wait on the gobbler future using the real timeout, closing the race window and answering the "why is this async for a synchronous task" question. - I capped captured output at 3000 characters. The finding didn't specify a number, so I picked one generous enough for a typical failure log without letting it grow unbounded. - I quoted command arguments containing whitespace so paths with spaces don't read as extra arguments. - I shortened the CHANGELOG.md entry to fit the 20-word limit. I left two things unaddressed, with my reasoning in PR comments: - I didn't add JSpecify @NullMarked on OcrResult. This is an existing file I didn't write, and I couldn't confirm NullAway actually enforces it for this package, so I didn't want to add an annotation I couldn't verify the effect of. - I didn't add the missing translations for the new localization keys in non-English bundles. I think this is expected, since this project pulls translations from Crowdin rather than contributors adding them by hand, but I'll confirm with a maintainer.
|
You ticked that you modified If you made changes that are visible to the user, please add a brief description to the |
|
Your code currently does not meet JabRef's code guidelines. IntelliJ auto format covers some cases. There seem to be issues with your code style and autoformat configuration. Please reformat your code (Ctrl+Alt+L) and commit, then push. |
3 participants
Summary
Surfaces the executed command and its captured stdout/stderr in the OCR failure dialog, so a failure gives something to debug instead of a generic message like "OCR process failed".
OcrResult.Failurenow carries the command line and output;OcrUtils.performOcrcaptures the process output into a buffer and waits for the output-draining task viaHeadlessExecutorServicebefore reading it, avoiding a race between that read and the still-running output-draining task.Steps to test
Covered by
OcrUtilsTest(added), which exercises theIO_ERRORandNON_ZERO_EXITpaths and asserts the command line and output are captured correctly.Related issues and pull requests
Closes #16679
AI usage
Claude Code (model claude-sonnet-5), AIL3 — AI wrote the implementation and test under my direction; I reviewed the diff, understand it, and take ownership of it.
AI CHECKLIST.md walkthrough
1. Code self-review
Nullability and control flow
==null/!=null— still present (gobblerFuture == null, mirrors the pre-existingProcess process = nullpattern in this file; this file isn't@NullMarkedyet, so converting only the touched lines to JSpecify would be inconsistent with the rest of the class)Objects.requireNonNull(...)@NullMarked— no new classes addedOptionalhandling — not used in this diffStringUtil.isBlank(...)— we use.isEmpty()on a non-nullStringfield (never null by construction), not a null-or-blank checkExceptions
catch (Exception e)— onlyIOException/InterruptedException/ExecutionException/TimeoutExceptionRuntimeException/IllegalStateExceptionthrownStyle and idioms
BibEntrywithers — not applicableList.of()used in the test)Pattern— no regexHeadlessExecutorService(project's executor wrapper), not rawnew Thread()///Markdown Javadoc used, no inline{@code}/{@link}User-facing text
Localization.langused for the new "Command"/"Output" labels!, no trailing:Security
Tests
OcrUtilsTestcovering theIO_ERROR,NON_ZERO_EXIT, and success paths@DisplayName, exceptions not caught in tests2. Verification commands
./gradlew :jablib:test --tests "org.jabref.logic.ocr.OcrUtilsTest"— 3/3 passed./gradlew checkstyleMain checkstyleTest checkstyleJmh— not run./gradlew modernizer— not run./gradlew --no-configuration-cache :rewriteDryRun— not run./gradlew javadoc— not run3. Documentation
CHANGELOG.mdentry added under### Fixed, linked to If OCR failed, there should be the command line shown #16679docs/requirements/— bug fix, not a new feature4. Pull request
gh pr create --body-file— will be used to open the PRCHANGELOG.mdTODOplaceholder used — a real entry was added directlyChecklist
CHANGELOG.md