fix(api): merge processFile() transactions with calling transactions#287
Conversation
- swaps propagation mode to REQUIRED to merge transactions so that multiple transactions do not modify the same book at the same time and conflict
📝 WalkthroughWalkthroughA single annotation modification changed the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@booklore-api/src/main/java/org/booklore/service/fileprocessor/AbstractFileProcessor.java`:
- Line 56: processFile currently uses `@Transactional`(propagation =
Propagation.REQUIRED) which causes it to join the caller transaction (e.g.
FileAsBookProcessor.processLibraryFilesGrouped) so a swallowed exception in
processGroupWithErrorHandling will mark the shared transaction rollback-only and
later cause a global rollback; fix by either changing
AbstractFileProcessor.processFile to use Propagation.REQUIRES_NEW so each file
runs in its own transaction, or alter
FileAsBookProcessor.processGroupWithErrorHandling to rethrow the exception after
logging so the outer transaction is aware and can fail fast — locate
AbstractFileProcessor.processFile and
FileAsBookProcessor.processLibraryFilesGrouped/processGroupWithErrorHandling and
implement one of these two remedies consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd6d3bc9-4b82-4007-b9e4-b647a4b98615
📒 Files selected for processing (1)
booklore-api/src/main/java/org/booklore/service/fileprocessor/AbstractFileProcessor.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
🧰 Additional context used
📓 Path-based instructions (1)
booklore-api/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
booklore-api/src/**/*.java: Use 4-space indentation in Java files and match surrounding Java style
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection
Use MapStruct for entity/DTO mapping
Files:
booklore-api/src/main/java/org/booklore/service/fileprocessor/AbstractFileProcessor.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: pedronave
Repo: grimmory-tools/grimmory PR: 173
File: booklore-api/src/main/java/org/booklore/service/hardcover/HardcoverSyncService.java:188-199
Timestamp: 2026-03-24T18:46:47.249Z
Learning: In grimmory-tools/grimmory (HardcoverSyncService.java), when a `hardcoverBookId` is stored in book metadata but is malformed (non-numeric), the preferred behavior is to return null and skip the sync rather than falling back to ISBN-based lookup. The reasoning is that an explicitly set bookId represents deliberate user intent, and silently resolving via ISBN could sync progress to a different book than intended.
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| @Transactional(propagation = Propagation.REQUIRED) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification of transactional boundaries + swallowed exception pattern.
set -euo pipefail
echo "== processFile transaction annotation =="
rg -n --type=java 'class\s+AbstractFileProcessor|@Transactional\(.*Propagation\.(REQUIRED|REQUIRES_NEW)|public\s+FileProcessResult\s+processFile\s*\(' -C3
echo
echo "== FileAsBookProcessor transaction + catch-and-continue flow =="
rg -n --type=java 'class\s+FileAsBookProcessor|@Transactional|processLibraryFilesGrouped\s*\(|processGroupWithErrorHandling\s*\(|catch\s*\(Exception\s+\w+\)|processor\.processFile\s*\(' -C3
echo
echo "== Additional processFile call sites =="
rg -n --type=java '\bprocessFile\s*\(' -C2Repository: grimmory-tools/grimmory
Length of output: 50380
🏁 Script executed:
fd -i 'FileAsBookProcessor' --type fRepository: grimmory-tools/grimmory
Length of output: 232
🏁 Script executed:
wc -l booklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.javaRepository: grimmory-tools/grimmory
Length of output: 151
🏁 Script executed:
cat -n booklore-api/src/main/java/org/booklore/service/library/FileAsBookProcessor.javaRepository: grimmory-tools/grimmory
Length of output: 15490
REQUIRED here can silently poison the parent transaction when callers swallow exceptions.
At Line 56, processFile(...) now joins the caller transaction. In FileAsBookProcessor.java (lines 54–82), processLibraryFilesGrouped(...) is transactional and processGroupWithErrorHandling(...) catches exceptions without rethrowing. If one processFile(...) call fails, the shared transaction is marked rollback-only; subsequent groups continue, then commit fails and all groups roll back.
Consider either rethrowing after logging in processGroupWithErrorHandling(...), or moving transaction boundaries to per-group/per-file units while preserving consistency guarantees.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@booklore-api/src/main/java/org/booklore/service/fileprocessor/AbstractFileProcessor.java`
at line 56, processFile currently uses `@Transactional`(propagation =
Propagation.REQUIRED) which causes it to join the caller transaction (e.g.
FileAsBookProcessor.processLibraryFilesGrouped) so a swallowed exception in
processGroupWithErrorHandling will mark the shared transaction rollback-only and
later cause a global rollback; fix by either changing
AbstractFileProcessor.processFile to use Propagation.REQUIRES_NEW so each file
runs in its own transaction, or alter
FileAsBookProcessor.processGroupWithErrorHandling to rethrow the exception after
logging so the outer transaction is aware and can fail fast — locate
AbstractFileProcessor.processFile and
FileAsBookProcessor.processLibraryFilesGrouped/processGroupWithErrorHandling and
implement one of these two remedies consistently.
balazs-szucs
left a comment
There was a problem hiding this comment.
Many thanks for looking into this!
…rimmory-tools#287) - swaps propagation mode to REQUIRED to merge transactions so that multiple transactions do not modify the same book at the same time and conflict
…rimmory-tools#287) - swaps propagation mode to REQUIRED to merge transactions so that multiple transactions do not modify the same book at the same time and conflict
…287) - swaps propagation mode to REQUIRED to merge transactions so that multiple transactions do not modify the same book at the same time and conflict
…287) - swaps propagation mode to REQUIRED to merge transactions so that multiple transactions do not modify the same book at the same time and conflict
Description
If processFile was called within an ongoing transaction, it would previously have generated a new transaction, which could leave the database in a state inconsistent with the prior transaction's expectations, causing errors and database corruption. This fixes that so that transactions are merged at the cost of errors propagating upwards. If processFile() throws an uncaught error, it taints the entire transaction and causes a rollback of the entire transaction.
Linked Issue: Fixes #261
Changes
Summary by CodeRabbit