feat: add Flyway migrations and S3 orphaned object cleanup#269
Merged
johanbriger merged 12 commits intoApr 27, 2026
Merged
Conversation
6 tasks
📝 WalkthroughWalkthroughThe PR adds durable retry logic for failed S3/MinIO deletions by introducing an Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Svc as AttachmentService
participant S3 as FileStorageService
participant DB as Database
participant Enq as OrphanedS3Enqueuer
App->>Svc: deleteAttachment(...)
activate Svc
Svc->>DB: registerAfterCommit callback
Svc->>DB: commit (delete attachment record)
deactivate Svc
DB-->>Svc: afterCommit triggered
activate Svc
Svc->>S3: delete(s3Key)
S3--xSvc: deletion fails
Svc->>Enq: enqueue(s3Key, bucket, reason)
activate Enq
Enq->>DB: findByS3Key(s3Key)
alt existing
Enq->>DB: update lastError, reset lastAttemptAt
else not found
Enq->>DB: create new OrphanedS3Object
end
Enq->>DB: save()
deactivate Enq
deactivate Svc
sequenceDiagram
participant Worker as OrphanedS3CleanupWorker
participant Repo as OrphanedS3ObjectRepository
participant Proc as OrphanedS3Processor
participant S3 as FileStorageService
participant DB as Database
Worker->>Repo: findNextBatchToProcess(maxRetries, pageable)
Repo-->>Worker: batch of OrphanedS3Objects
loop for each orphan
Worker->>Proc: processOrphan(orphan, maxRetries)
activate Proc
Proc->>S3: delete(s3Key)
alt success
S3-->>Proc: deleted
Proc->>DB: delete OrphanedS3Object
Proc-->>Worker: log success
else failure
S3--xProc: exception
Proc->>DB: increment retryCount, set lastAttemptAt, update lastError
Proc->>DB: save()
Proc-->>Worker: log warning
end
deactivate Proc
end
Worker->>Repo: countByRetryCountGreaterThanEqual(maxRetries)
Repo-->>Worker: count
alt count > 0
Worker->>Worker: log ERROR alert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/org/example/vet1177/services/AttachmentServiceTest.java (1)
51-78:⚠️ Potential issue | 🟡 MinorWiring is correct, but add test coverage for S3 deletion failures.
The PR introduces persistence of
OrphanedS3Objectrecords when S3 delete fails. The service usesTransactionSynchronizationManager.afterCommit()to defer the delete operation (lines 132–139 in the service), which provides the asynchronous behavior mentioned in the PR. However, the existing delete tests only cover the happy path. Add test(s) that:
- Mock
fileStorageService.delete()to throw an exception- Verify that
orphanedS3ObjectRepository.save()is called with anOrphanedS3Objectcontaining the expecteds3Keyands3Bucket- Ensure the delete operation does not propagate the exception (graceful handling)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/services/AttachmentServiceTest.java` around lines 51 - 78, Add a test in AttachmentServiceTest that simulates an S3 deletion failure by mocking fileStorageService.delete(...) to throw, then call AttachmentService.deleteAttachment(recordId, attachmentId) (same method used by existing delete tests); assert that the exception is not propagated (call completes normally) and verify orphanedS3ObjectRepository.save(...) was called with an OrphanedS3Object whose s3Key equals the attachment's S3 key and s3Bucket equals props.getBucketName(); ensure the test runs after setUp so AttachmentService, attachment and record fixtures are used and rely on TransactionSynchronizationManager.afterCommit() behavior to trigger the orphan save.src/main/java/org/example/vet1177/services/AttachmentService.java (1)
130-140:⚠️ Potential issue | 🟡 MinorAdd null check and fallback for
s3Bucket, plus error handling around orphan save.Two gaps in the
afterCommithook:
Null safety for
s3Bucket: AlthoughAttachment.s3Bucketisnullable = false, legacy rows created before this constraint was added could have null values. WhenorphanedS3ObjectRepository.save()is called at line 139, a null value will violate theOrphanedS3Object.s3Bucketconstraint and throw an exception that Spring silently swallows into aTransactionSystemException. Add a null check and fall back tobucketNamewhen null.Error handling for
save()itself: IforphanedS3ObjectRepository.save(orphan)fails (line 139), the exception is not explicitly logged. Wrap it in try-catch with a clearlog.error()statement; the same applies to line 147 in the non-transactional path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around lines 130 - 140, Handle null s3Bucket and errors when persisting orphan records: in the afterCommit() hook (and the non-transactional branch that also calls orphanedS3ObjectRepository.save), check if s3Bucket is null and fall back to bucketName before constructing the OrphanedS3Object; then wrap orphanedS3ObjectRepository.save(orphan) in its own try-catch and log.error the exception with a clear message if saving fails (include the s3Key and chosen bucket value). Ensure you reference the same OrphanedS3Object creation path used when fileStorageService.delete(...) fails and apply identical error logging in both places.
🧹 Nitpick comments (4)
src/main/java/org/example/vet1177/entities/OrphanedS3Object.java (1)
22-40: Optional cleanup: redundant initialization and@PrePersistforcreatedAt.A few small refinements:
createdAt = Instant.now()is set both in the field initializer (line 22) and in the convenience constructor (line 38) — drop the duplicate.- Same for
retryCount = 0in the constructor (line 39) — already the field default.- Prefer
@PrePersistover a field initializer forcreatedAt. As written,createdAtis fixed at object construction time, so an entity built and then held in memory beforesave()will record a timestamp earlier than the actual insert. Not a correctness bug for this orphan-tracking flow, but@PrePersistis the idiomatic JPA pattern.♻️ Proposed refactor
`@Column`(name = "created_at", nullable = false, updatable = false) - private Instant createdAt = Instant.now(); + private Instant createdAt; @@ public OrphanedS3Object(String s3Key, String s3Bucket) { this.s3Key = s3Key; this.s3Bucket = s3Bucket; - this.createdAt = Instant.now(); - this.retryCount = 0; + } + + `@PrePersist` + void onCreate() { + if (this.createdAt == null) { + this.createdAt = Instant.now(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/entities/OrphanedS3Object.java` around lines 22 - 40, Remove the redundant initializations in the OrphanedS3Object class: delete the field initializer "createdAt = Instant.now()" and the constructor assignments "this.createdAt = Instant.now()" and "this.retryCount = 0" in the OrphanedS3Object(String s3Key, String s3Bucket) constructor (keep the field declaration for retryCount with default 0). Add a `@PrePersist` lifecycle method (e.g., prePersist()) that sets createdAt = Instant.now() if createdAt is null so the timestamp is set at persist time rather than construction.src/main/resources/db/migration/V2__init_orphaned_table.sql (1)
2-14: Minor: align idempotency style with V1.V1 uses
CREATE TABLE IF NOT EXISTS/CREATE INDEX IF NOT EXISTS; V2 doesn't. For Flyway-versioned migrations either style is fine (each version runs once), but consistency reduces surprise during local re-bootstraps andflyway repair.-CREATE TABLE orphaned_s3_objects ( +CREATE TABLE IF NOT EXISTS orphaned_s3_objects ( @@ -CREATE INDEX idx_orphaned_s3_retry ON orphaned_s3_objects (retry_count, last_attempt_at); +CREATE INDEX IF NOT EXISTS idx_orphaned_s3_retry ON orphaned_s3_objects (retry_count, last_attempt_at);Also worth considering: the secondary index
(retry_count, last_attempt_at)matches the repository'sfindTop20ByRetryCountLessThanOrderByLastAttemptAtAscquery — make sure the ordering of the worker's filter (retry_count < N) and ORDER BY are kept in sync if you ever change the column order here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V2__init_orphaned_table.sql` around lines 2 - 14, The migration lacks idempotent CREATE ... IF NOT EXISTS style used in V1; update the V2 SQL to use "CREATE TABLE IF NOT EXISTS orphaned_s3_objects" and "CREATE INDEX IF NOT EXISTS idx_orphaned_s3_retry" so local reboots and flyway repair behave consistently, and while editing confirm the index column order on idx_orphaned_s3_retry (retry_count, last_attempt_at) remains compatible with the repository query findTop20ByRetryCountLessThanOrderByLastAttemptAtAsc (i.e., retry_count as the first column and last_attempt_at as the second) to preserve query performance and ordering semantics.src/main/resources/db/migration/V1__initial_schema.sql (1)
69-87: Remove the redundantALTER TABLE ... ADD COLUMN IF NOT EXISTSstatements.
comment_type(line 64) anddescription(line 76) are already declared inline in the sameCREATE TABLEblock above. The trailingALTER TABLEstatements at lines 69 and 87 are dead in a freshV1migration —CREATE TABLE IF NOT EXISTSeither creates the table with these columns or finds an existing table; in both cases theALTERis either a no-op or, if the table existed without the column, an unrelated legacy concern that does not belong in the initial schema.The comment at lines 84–86 acknowledges this is for "legacy databases", but Flyway's migration history is what tracks that — those one-off fixups should live in their own versioned migration (e.g.
V1.1__...), not wedged into V1.♻️ Proposed cleanup
-ALTER TABLE comment ADD COLUMN IF NOT EXISTS comment_type VARCHAR(32) NOT NULL DEFAULT 'OWNER_MESSAGE'; - CREATE TABLE IF NOT EXISTS attachment ( @@ ); - --- Migration för befintliga databaser som skapades innan description-kolumnen lades till. --- CREATE TABLE IF NOT EXISTS ovan är no-op om tabellen redan finns, så kolumnen måste --- läggas till explicit. Samma mönster som comment_type ovan. -ALTER TABLE attachment ADD COLUMN IF NOT EXISTS description VARCHAR(500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V1__initial_schema.sql` around lines 69 - 87, The ALTER TABLE ADD COLUMN IF NOT EXISTS statements for comment_type and description are redundant because comment_type is already declared in the comment CREATE TABLE and description is declared in the attachment CREATE TABLE in this V1 migration; remove the two ALTER TABLE lines that reference comment_type and attachment.description (the ALTER TABLE comment ADD COLUMN IF NOT EXISTS comment_type ... and ALTER TABLE attachment ADD COLUMN IF NOT EXISTS description ...) from V1__initial_schema.sql, and if you need to support legacy databases that may lack those columns, create a new separate versioned migration (e.g. V1_1__add_missing_columns.sql) that performs those ALTER TABLE operations instead.src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java (1)
22-22: Consider what happens afterMAX_RETRIESis reached.Once
retryCounthits 10 the row is silently ignored by the query forever — no alert, no metric, no dead-letter. Operators have no signal that data in S3 is permanently orphaned. Worth either emitting a counter/log when a row is skipped because it has exhausted retries, or moving such rows to a separate "dead" table/state for manual review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java` at line 22, The code currently drops rows once retryCount reaches MAX_RETRIES (10) with no signal; update OrphanedS3CleanupWorker to handle exhausted retries by detecting when retryCount >= MAX_RETRIES and then (a) emit a clear log via process/logger (include row id and reason) and increment a dedicated metric/counter (e.g., orphaned_s3_deadletter_count), and/or (b) move the row to a dead-letter state/table (insert into an OrphanedS3DeadLetter table or set a status column like 'DEAD') and remove it from the retry set so operators can review; ensure changes are applied where retry logic updates retryCount and retries are evaluated (refer to MAX_RETRIES, retryCount, and the method handling the cleanup loop in OrphanedS3CleanupWorker).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/vet1177/repository/OrphanedS3ObjectRepository.java`:
- Line 12: The derived query method
findTop20ByRetryCountLessThanOrderByLastAttemptAtAsc in
OrphanedS3ObjectRepository orders by last_attempt_at which is nullable and will
place NULLs last in PostgreSQL, starving new rows; fix by replacing that
ordering with a non-null column (e.g., order by created_at), or replace the
method with an explicit `@Query` that uses "ORDER BY last_attempt_at ASC NULLS
FIRST", or ensure last_attempt_at is initialized to created_at on insert so
NULLs do not occur; update the repository method signature and/or add the `@Query`
accordingly and adjust any insert logic that sets last_attempt_at.
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 143-149: The else branch in deleteAttachment is unreachable and
unsafe — don't attempt to delete S3 objects outside the transaction; remove the
else block that calls fileStorageService.delete(s3Key) and
orphanedS3ObjectRepository.save(...). Instead keep the transaction-aware path
that registers a TransactionSynchronization (using
TransactionSynchronizationManager.isSynchronizationActive() and
afterCommit/afterCompletion) to delete the S3 object with
fileStorageService.delete(s3Key) and only record
orphanedS3ObjectRepository.save(...) from within the synchronization if deletion
fails; if you prefer a fail-fast behavior, drop any fallback and let the method
fail loudly when called without an active transaction.
- Around line 103-112: The orphan enqueue is being saved inside the original
`@Transactional` uploadAttachment transaction so it gets rolled back; extract the
save into a new Spring bean (e.g. class OrphanedS3Enqueuer) with a method
enqueue(String s3Key, String bucket, String reason) annotated
`@Transactional`(propagation = Propagation.REQUIRES_NEW) which builds the
OrphanedS3Object (truncate reason to 1024, set lastAttemptAt) and
repo.save(...); replace direct calls to orphanedS3ObjectRepository.save(...) in
AttachmentService (both the catch block in uploadAttachment and the else branch
in deleteAttachment) with calls to OrphanedS3Enqueuer.enqueue(...) to ensure the
orphan record is persisted outside the failing transaction (note: do not try to
call a `@Transactional` method on this from the same class via self-invocation).
In `@src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java`:
- Line 38: The repository query used by OrphanedS3CleanupWorker.retryDeletions
(findTop20ByRetryCountLessThanOrderByLastAttemptAtAsc) returns plain rows that
concurrent workers can read; change the repository method to acquire row-level
locks (e.g., add `@Lock`(LockModeType.PESSIMISTIC_WRITE) and a JPA query hint for
SKIP LOCKED / lock timeout or replace with a native query using FOR UPDATE SKIP
LOCKED) so only one worker processes each OrphanedS3Object at a time;
alternatively, if you prefer optimistic handling add `@Version` to the
OrphanedS3Object entity to surface lost-update races via
OptimisticLockException.
- Line 38: The current query used by OrphanedS3CleanupWorker
(repository.findTop20ByRetryCountLessThanOrderByLastAttemptAtAsc with
MAX_RETRIES) lets rows with null lastAttemptAt sort after non-null values,
starving newly inserted orphans; fix this by either initializing lastAttemptAt
in the OrphanedS3Object constructor (set lastAttemptAt = createdAt or
Instant.now() in the constructor where fields are set) so new rows have a
deterministic timestamp, or change the repository query to an explicit `@Query`
that orders by lastAttemptAt NULLS FIRST (e.g., "ORDER BY last_attempt_at ASC
NULLS FIRST") to ensure new/null-lastAttemptAt rows are prioritized in the
top-20 selection.
- Around line 34-66: The batch method retryDeletions() currently has
`@Transactional` for the whole loop; remove that annotation from retryDeletions()
and extract the per-item logic into a new bean method (e.g.
processOrphanOrRetry(OrphanedS3Object orphan)) annotated
`@Transactional`(propagation = Propagation.REQUIRES_NEW) so each orphan's work
(call fileStorageService.delete(orphan.getS3Key()), repository.delete(orphan) on
success, and orphan.setRetryCount/... + repository.save(orphan) on failure) is
committed per-item; keep the `@Scheduled` on retryDeletions() in
OrphanedS3CleanupWorker, have it fetch the pending list with
repository.findTop20ByRetryCountLessThanOrderByLastAttemptAtAsc(MAX_RETRIES) and
call the new REQUIRES_NEW method for each orphan, and ensure imports for
Propagation are added.
- Around line 62-63: The warn log in OrphanedS3CleanupWorker that currently
calls log.warn("Retry failed for S3 object {}. Attempt {}/{}",
orphan.getS3Key(), orphan.getRetryCount(), MAX_RETRIES) drops the exception;
update the call to pass the Throwable (the caught exception variable, e) as the
trailing argument so SLF4J will print the stack trace and underlying cause (keep
the existing message and placeholders and still set orphan.setLastError(...) as
before). Ensure you reference the same variables (orphan.getS3Key(),
orphan.getRetryCount(), MAX_RETRIES, lastError) and only append the exception
object to the log call.
In `@src/main/resources/application-dev.properties`:
- Around line 16-17: The application-dev.properties currently forces Spring to
run schema/data SQL after startup (spring.sql.init.mode=always) which can
conflict with Flyway-managed migrations; update application-dev.properties to
stop Spring SQL initialization by removing the spring.sql.init.mode property or
set spring.sql.init.mode=never so Flyway alone manages schema/seed changes (also
ensure spring.jpa.hibernate.ddl-auto remains set to validate to prevent
Hibernate from altering schema).
In `@src/main/resources/db/migration/V1__initial_schema.sql`:
- Around line 1-9: The Flyway migration V1__initial_schema.sql uses
PostgreSQL-only syntax (gen_random_uuid(), TIMESTAMPTZ) which breaks H2 in CI;
either switch CI to run migrations against a real PostgreSQL test instance
(e.g., Testcontainers) so V1__initial_schema.sql can stay unchanged, or make the
migration H2-compatible by changing the clinic table defaults/types: replace
gen_random_uuid() with RANDOM_UUID() (or remove DB default and set UUID in JPA)
and replace TIMESTAMPTZ with a H2-compatible type such as TIMESTAMP WITH TIME
ZONE (or TIMESTAMP) and ensure the H2 JDBC URL uses MODE=PostgreSQL; update
V1__initial_schema.sql accordingly and keep the table name clinic and its
columns (id, created_at, updated_at) consistent.
---
Outside diff comments:
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 130-140: Handle null s3Bucket and errors when persisting orphan
records: in the afterCommit() hook (and the non-transactional branch that also
calls orphanedS3ObjectRepository.save), check if s3Bucket is null and fall back
to bucketName before constructing the OrphanedS3Object; then wrap
orphanedS3ObjectRepository.save(orphan) in its own try-catch and log.error the
exception with a clear message if saving fails (include the s3Key and chosen
bucket value). Ensure you reference the same OrphanedS3Object creation path used
when fileStorageService.delete(...) fails and apply identical error logging in
both places.
In `@src/test/java/org/example/vet1177/services/AttachmentServiceTest.java`:
- Around line 51-78: Add a test in AttachmentServiceTest that simulates an S3
deletion failure by mocking fileStorageService.delete(...) to throw, then call
AttachmentService.deleteAttachment(recordId, attachmentId) (same method used by
existing delete tests); assert that the exception is not propagated (call
completes normally) and verify orphanedS3ObjectRepository.save(...) was called
with an OrphanedS3Object whose s3Key equals the attachment's S3 key and s3Bucket
equals props.getBucketName(); ensure the test runs after setUp so
AttachmentService, attachment and record fixtures are used and rely on
TransactionSynchronizationManager.afterCommit() behavior to trigger the orphan
save.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/entities/OrphanedS3Object.java`:
- Around line 22-40: Remove the redundant initializations in the
OrphanedS3Object class: delete the field initializer "createdAt = Instant.now()"
and the constructor assignments "this.createdAt = Instant.now()" and
"this.retryCount = 0" in the OrphanedS3Object(String s3Key, String s3Bucket)
constructor (keep the field declaration for retryCount with default 0). Add a
`@PrePersist` lifecycle method (e.g., prePersist()) that sets createdAt =
Instant.now() if createdAt is null so the timestamp is set at persist time
rather than construction.
In `@src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java`:
- Line 22: The code currently drops rows once retryCount reaches MAX_RETRIES
(10) with no signal; update OrphanedS3CleanupWorker to handle exhausted retries
by detecting when retryCount >= MAX_RETRIES and then (a) emit a clear log via
process/logger (include row id and reason) and increment a dedicated
metric/counter (e.g., orphaned_s3_deadletter_count), and/or (b) move the row to
a dead-letter state/table (insert into an OrphanedS3DeadLetter table or set a
status column like 'DEAD') and remove it from the retry set so operators can
review; ensure changes are applied where retry logic updates retryCount and
retries are evaluated (refer to MAX_RETRIES, retryCount, and the method handling
the cleanup loop in OrphanedS3CleanupWorker).
In `@src/main/resources/db/migration/V1__initial_schema.sql`:
- Around line 69-87: The ALTER TABLE ADD COLUMN IF NOT EXISTS statements for
comment_type and description are redundant because comment_type is already
declared in the comment CREATE TABLE and description is declared in the
attachment CREATE TABLE in this V1 migration; remove the two ALTER TABLE lines
that reference comment_type and attachment.description (the ALTER TABLE comment
ADD COLUMN IF NOT EXISTS comment_type ... and ALTER TABLE attachment ADD COLUMN
IF NOT EXISTS description ...) from V1__initial_schema.sql, and if you need to
support legacy databases that may lack those columns, create a new separate
versioned migration (e.g. V1_1__add_missing_columns.sql) that performs those
ALTER TABLE operations instead.
In `@src/main/resources/db/migration/V2__init_orphaned_table.sql`:
- Around line 2-14: The migration lacks idempotent CREATE ... IF NOT EXISTS
style used in V1; update the V2 SQL to use "CREATE TABLE IF NOT EXISTS
orphaned_s3_objects" and "CREATE INDEX IF NOT EXISTS idx_orphaned_s3_retry" so
local reboots and flyway repair behave consistently, and while editing confirm
the index column order on idx_orphaned_s3_retry (retry_count, last_attempt_at)
remains compatible with the repository query
findTop20ByRetryCountLessThanOrderByLastAttemptAtAsc (i.e., retry_count as the
first column and last_attempt_at as the second) to preserve query performance
and ordering semantics.
🪄 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: 59bd74ce-d6f9-4ddd-875d-e5bf2db13713
📒 Files selected for processing (10)
pom.xmlsrc/main/java/org/example/vet1177/Vet1177Application.javasrc/main/java/org/example/vet1177/entities/OrphanedS3Object.javasrc/main/java/org/example/vet1177/repository/OrphanedS3ObjectRepository.javasrc/main/java/org/example/vet1177/services/AttachmentService.javasrc/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.javasrc/main/resources/application-dev.propertiessrc/main/resources/db/migration/V1__initial_schema.sqlsrc/main/resources/db/migration/V2__init_orphaned_table.sqlsrc/test/java/org/example/vet1177/services/AttachmentServiceTest.java
persist even if main transactions roll back. - Updated AttachmentService to use the new enqueuer and refined post-commit deletion logic using TransactionSynchronization. - Optimized cleanup queue with 'NULLS FIRST' ordering to prevent retries from starving new deletion tasks. - Refactored AttachmentServiceTest to reflect new service architecture and verified error-handling edge cases.
- Remove unsafe fallback for S3 deletion outside of active transactions. - Refactor deleteAttachment to rely solely on TransactionSynchronization. - Implement fail-fast behavior by requiring an active transaction context. - Update AttachmentServiceTest to verify post-commit synchronization using Mockito static mocks and ArgumentCaptors.
- Move demo data from data.sql to V3__insert_demo_data.sql. - Disable spring.sql.init.mode to let Flyway handle all data seeding. - Set spring.jpa.hibernate.ddl-auto=none to prevent validation errors during Flyway initialization (missing activity_log table). - Clean up V3 script to include explicit timestamps and default values.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/test/java/org/example/vet1177/services/AttachmentServiceTest.java (1)
270-295: Nit: inconsistent indentation inside thetry-with-resources blocks.In
delete_shouldDeleteFromRepository(lines 272-281) anddelete_shouldCallPolicyCanDelete(lines 285-294), only the newmockedStaticlines are indented relative to the try block; the existingwhen(...), action, andverify(...)lines remain at the original indentation level. The block indelete_shouldCallFileStorageDelete(lines 299-314) is consistently indented and is much easier to read — recommend re-indenting the other two for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/vet1177/services/AttachmentServiceTest.java` around lines 270 - 295, Re-indent the contents of the two test methods delete_shouldDeleteFromRepository and delete_shouldCallPolicyCanDelete so all statements inside the try-with-resources block using mockedStatic are consistently indented (including the mockedStatic.when(...), when(attachmentRepository.findById(...)), attachmentService.deleteAttachment(...), verify(attachmentRepository).delete(...), verify(attachmentPolicy).canDelete(...), and mockedStatic.verify(...) lines) to match the style used in delete_shouldCallFileStorageDelete; locate the try(...) using mockStatic(TransactionSynchronizationManager.class) and adjust the inner block indentation accordingly.src/main/java/org/example/vet1177/services/OrphanedS3Enqueuer.java (1)
28-29: Nit: redundantsetLastAttemptAt(null).
lastAttemptAthas no default initializer inOrphanedS3Object, so it is alreadynullafter construction. The explicit setter call (and the extra blank line at line 28) can be removed without behavior change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/OrphanedS3Enqueuer.java` around lines 28 - 29, Remove the redundant explicit null assignment to lastAttemptAt in OrphanedS3Enqueuer: delete the call orphan.setLastAttemptAt(null) (and the extra blank line) since OrphanedS3Object.lastAttemptAt is null by default after construction; this keeps behavior unchanged and cleans up the code.src/main/java/org/example/vet1177/services/AttachmentService.java (1)
124-136: Always-register-afterCommitflow LGTM, but indentation is leftover from the oldif/else.Addresses the prior concern about the unreachable
elsebranch.registerSynchronizationis now always called, which is correct sincedeleteAttachmentis@Transactionaland synchronization is guaranteed to be active. Stack trace at line 131 also now propagateseproperly.Nit: line 124 keeps an extra level of indentation (8 extra spaces) inherited from the removed
if (...isSynchronizationActive())branch. Worth re-aligning to the method's normal block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/AttachmentService.java` around lines 124 - 136, The block that calls TransactionSynchronizationManager.registerSynchronization(...) inside AttachmentService (the anonymous TransactionSynchronization.afterCommit implementation that calls fileStorageService.delete and orphanedS3Enqueuer.enqueue) has leftover extra indentation from a removed if/else; reformat the registerSynchronization call and its anonymous class to match the method's normal indentation level (remove the 8-space overindent) so the block aligns with the surrounding method code and visual style.src/main/java/org/example/vet1177/services/OrphanedS3Processor.java (1)
26-51: External S3 call held inside the DB transaction.
processOrphanopens aREQUIRES_NEWtransaction on entry and only commits afterfileStorageService.delete(...)returns. That means the JDBC connection is held for the entire duration of the S3/MinIO RTT — which is exactly the failure mode this worker exists to handle (slow/unreachable S3). Under load (20 orphans × slow S3), this can pin connections from the pool while doing zero useful DB work.Recommended pattern: do the S3 call outside the transaction, then open a short-lived transaction to either delete the row or persist the failure metadata.
♻️ Sketch of the refactor
// in OrphanedS3CleanupWorker, no `@Transactional` on the loop for (OrphanedS3Object orphan : pending) { boolean deleted; String errorMessage = null; try { fileStorageService.delete(orphan.getS3Key()); deleted = true; } catch (Exception e) { deleted = false; errorMessage = e.getMessage(); log.warn("Retry failed for S3 object {}", orphan.getS3Key(), e); } processor.recordOutcome(orphan.getId(), deleted, errorMessage, MAX_RETRIES); } // in OrphanedS3Processor `@Transactional`(propagation = Propagation.REQUIRES_NEW) public void recordOutcome(UUID id, boolean deleted, String error, int maxRetries) { // re-load by id, then either delete or update retryCount/lastAttemptAt/lastError }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/OrphanedS3Processor.java` around lines 26 - 51, processOrphan currently performs the external call fileStorageService.delete(...) inside a REQUIRES_NEW transaction which holds a DB connection during S3/MinIO RTT; move the S3 call out of the transaction and convert processOrphan into two steps: perform fileStorageService.delete(orphan.getS3Key()) outside any transaction, capture success/exception and error message, then call a new transactional method (e.g. recordOutcome(UUID id, boolean deleted, String error, int maxRetries) on OrphanedS3Processor) annotated with `@Transactional`(propagation = Propagation.REQUIRES_NEW) that reloads the OrphanedS3Object by id and either repository.delete(orphan) on success or updates retryCount, lastAttemptAt, lastError (truncate to 1024 chars) and repository.save(orphan) on failure; update the caller (OrphanedS3CleanupWorker loop) to call the delete first and then processor.recordOutcome(...) with the result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 100-109: The catch block loses stack traces and uses a hardcoded
enqueue reason; update the log.error calls to pass the exception objects so
SLF4J includes stack traces (e.g., log.error("Database persistence failed for S3
key: {}. Triggering cleanup.", s3Key, e) and log.error("Upload cleanup failed.
Enqueuing {} for background retry.", s3Key, deleteEx)), and change the
orphanedS3Enqueuer.enqueue call to include a descriptive reason built from the
actual exceptions (e.g., combine deleteEx.getMessage() and e.getMessage()/class
names) instead of the literal "Upload transaction failed" so lastError contains
the real cause; keep the RuntimeException thrown but preserve the original
exception as its cause.
In `@src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java`:
- Around line 31-44: The worker currently ignores rows with retryCount >=
MAX_RETRIES (found via repository.findNextBatchToProcess) so permanently-failed
orphans never get surfaced; add a repository method (e.g.,
OrphanedS3ObjectRepository.countByRetryCountGreaterThanEqual(int retries)) and,
after processing the pending batch in OrphanedS3CleanupWorker.retryDeletions (or
the loop that calls processor.processOrphan(orphan, MAX_RETRIES)), query that
count and emit an ERROR-level log (or metric) like "ALERT: {} orphaned S3
objects have exceeded MAX_RETRIES={}" when the count > 0 so operators are
alerted to stuck records.
In `@src/main/java/org/example/vet1177/services/OrphanedS3Enqueuer.java`:
- Around line 25-40: The enqueue method can throw
DataIntegrityViolationException on duplicate s3_key; make it idempotent by
implementing a find-or-create flow: add Optional<OrphanedS3Object>
findByS3Key(String s3Key) to OrphanedS3ObjectRepository, then change
OrphanedS3Enqueuer.enqueue to call repository.findByS3Key(s3Key) and if present
update that entity's lastError (with the same truncation logic) and
lastAttemptAt=null and save, otherwise create and save a new OrphanedS3Object;
this avoids unique-constraint inserts and ensures upsert semantics without
propagating constraint exceptions from repository.save.
In `@src/main/resources/db/migration/V3__insert_demo_data.sql`:
- Around line 10-13: The demo seed inserts currently reuse the same bcrypt hash
('$2a$10$mxL7BIFi9S2jdA867vmuDeRe4SZ/krIiHsCzvOdh/3j5nviJ0G2xy') for all users
(including 'Sara Admin' / role 'ADMIN'); replace with distinct bcrypt hashes per
user (generate unique salts/hashes) and remove or disable privileged accounts in
seed data by either omitting the 'Sara Admin' ADMIN row or setting its
is_active/active flag to false (and similarly set is_active=false for non-OWNER
roles like 'VET'/'ADMIN'), and if your schema supports it set a
password_must_change flag for seeded accounts; document any cleartext dev
passwords only in a non-deployable developer README rather than in the
migration.
- Around line 1-32: Demo seed migration V3__insert_demo_data.sql is currently in
the default Flyway location and will run in production; move it to a dev-only
location and update dev profile Flyway locations to avoid executing in prod.
Move V3__insert_demo_data.sql into a dev migrations folder (e.g.,
classpath:db/migration/dev), add or update application-dev.properties to set
spring.flyway.locations=classpath:db/migration,classpath:db/migration/dev so
Flyway picks up dev seeds only when the dev profile is active, and ensure
production config does not include the dev path; also remove or rotate any
deterministic/demo credentials in the seed (e.g., the ADMIN sara@admin.se bcrypt
hash) to avoid shipping known passwords.
In `@src/test/java/org/example/vet1177/services/AttachmentServiceTest.java`:
- Around line 297-315: Add a test that exercises the post-commit failure path in
AttachmentService.deleteAttachment: mock
TransactionSynchronizationManager.isSynchronizationActive() to true and
attachmentRepository.findById(...) to return the attachment, stub
fileStorageService.delete(s3Key) to throw (e.g., RuntimeException), invoke
attachmentService.deleteAttachment(vetUser, attachmentId), capture the
TransactionSynchronization registered via
TransactionSynchronizationManager.registerSynchronization(...) (use
ArgumentCaptor<TransactionSynchronization>), call captured.afterCommit(), and
verify that orphanedS3Enqueuer.enqueue(attachment.getS3Key(), the expected
bucket name, any retry metadata) is invoked; ensure you reference the same
attachmentId/attachment used elsewhere in the test class and the real bucket
string used by AttachmentService.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/services/AttachmentService.java`:
- Around line 124-136: The block that calls
TransactionSynchronizationManager.registerSynchronization(...) inside
AttachmentService (the anonymous TransactionSynchronization.afterCommit
implementation that calls fileStorageService.delete and
orphanedS3Enqueuer.enqueue) has leftover extra indentation from a removed
if/else; reformat the registerSynchronization call and its anonymous class to
match the method's normal indentation level (remove the 8-space overindent) so
the block aligns with the surrounding method code and visual style.
In `@src/main/java/org/example/vet1177/services/OrphanedS3Enqueuer.java`:
- Around line 28-29: Remove the redundant explicit null assignment to
lastAttemptAt in OrphanedS3Enqueuer: delete the call
orphan.setLastAttemptAt(null) (and the extra blank line) since
OrphanedS3Object.lastAttemptAt is null by default after construction; this keeps
behavior unchanged and cleans up the code.
In `@src/main/java/org/example/vet1177/services/OrphanedS3Processor.java`:
- Around line 26-51: processOrphan currently performs the external call
fileStorageService.delete(...) inside a REQUIRES_NEW transaction which holds a
DB connection during S3/MinIO RTT; move the S3 call out of the transaction and
convert processOrphan into two steps: perform
fileStorageService.delete(orphan.getS3Key()) outside any transaction, capture
success/exception and error message, then call a new transactional method (e.g.
recordOutcome(UUID id, boolean deleted, String error, int maxRetries) on
OrphanedS3Processor) annotated with `@Transactional`(propagation =
Propagation.REQUIRES_NEW) that reloads the OrphanedS3Object by id and either
repository.delete(orphan) on success or updates retryCount, lastAttemptAt,
lastError (truncate to 1024 chars) and repository.save(orphan) on failure;
update the caller (OrphanedS3CleanupWorker loop) to call the delete first and
then processor.recordOutcome(...) with the result.
In `@src/test/java/org/example/vet1177/services/AttachmentServiceTest.java`:
- Around line 270-295: Re-indent the contents of the two test methods
delete_shouldDeleteFromRepository and delete_shouldCallPolicyCanDelete so all
statements inside the try-with-resources block using mockedStatic are
consistently indented (including the mockedStatic.when(...),
when(attachmentRepository.findById(...)),
attachmentService.deleteAttachment(...),
verify(attachmentRepository).delete(...),
verify(attachmentPolicy).canDelete(...), and mockedStatic.verify(...) lines) to
match the style used in delete_shouldCallFileStorageDelete; locate the try(...)
using mockStatic(TransactionSynchronizationManager.class) and adjust the inner
block indentation accordingly.
🪄 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: fa3c6e98-d151-43c9-9e58-e3fdf2d1ad86
📒 Files selected for processing (9)
src/main/java/org/example/vet1177/repository/OrphanedS3ObjectRepository.javasrc/main/java/org/example/vet1177/services/AttachmentService.javasrc/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.javasrc/main/java/org/example/vet1177/services/OrphanedS3Enqueuer.javasrc/main/java/org/example/vet1177/services/OrphanedS3Processor.javasrc/main/resources/application-dev.propertiessrc/main/resources/db/migration/V3__insert_demo_data.sqlsrc/test/java/org/example/vet1177/services/AttachmentServiceTest.javasrc/test/resources/application-test.properties
✅ Files skipped from review due to trivial changes (1)
- src/test/resources/application-test.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/application-dev.properties
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/resources/db/migration/dev/V3__insert_demo_data.sql (1)
10-13:⚠️ Potential issue | 🟠 MajorShared bcrypt hash across all four demo users (including
ADMIN) — still unchanged.Even scoped to the dev profile, all four rows reuse the identical hash
$2a$10$mxL7BIFi9S2jdA867vmuDeRe4SZ/krIiHsCzvOdh/3j5nviJ0G2xy, and thesara@admin.seADMIN account is seeded in the same batch. Once the cleartext is known to anyone on the team it unlocks every demo identity at once, and shared CI/dev environments inherit the same credential surface. Consider: distinct hashes per row, omitting the ADMIN account from seed data (provision out-of-band), and/or settingis_active = falseonVET/ADMINrows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/dev/V3__insert_demo_data.sql` around lines 10 - 13, The seeded demo users in V3__insert_demo_data.sql all share the same bcrypt hash ('$2a$10$mxL7BIFi9S2jdA867vmuDeRe4SZ/krIiHsCzvOdh/3j5nviJ0G2xy') (rows for 'anna@test.se', 'lars@test.se', 'erik@klinik.se', 'sara@admin.se'), which leaks a single credential for all accounts; fix by generating and using unique bcrypt hashes per row (replacing the shared hash for 'anna@test.se', 'lars@test.se', 'erik@klinik.se', and 'sara@admin.se'), and either remove the ADMIN seed row for 'sara@admin.se' or set its is_active = false (and optionally set is_active = false for the 'VET' row 'erik@klinik.se') so privileged accounts are not immediately usable in dev/CI.
🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java (1)
46-51: ERROR log will fire every 10 minutes for the same stuck rows — alert fatigue.
countByRetryCountGreaterThanEqual(MAX_RETRIES)runs on every tick, so as soon as one orphan permanently fails, the sameERRORline repeats every 10 minutes forever (until somebody manually intervenes). That's noisy on log/alerting backends that page on ERROR rate, and the message is identical so dedup may suppress real new failures.Consider one of:
- Emit a gauge metric (e.g. Micrometer
MeterRegistry.gauge("s3.orphaned.permanent_failures", …)) instead of (or in addition to) an ERROR log, so dashboards/alerts handle thresholding.- Log at
WARNand let an alerting rule on the metric drive paging.- Track the last-reported count and only re-log when it changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java` around lines 46 - 51, The ERROR log currently emitted every run using repository.countByRetryCountGreaterThanEqual(MAX_RETRIES) causes alert fatigue; fix by recording a class-level long lastReportedPermanentFailures (default 0), update a Micrometer gauge via MeterRegistry.gauge("s3.orphaned.permanent_failures", ...) with the current count, and only call log.warn/log.error (use WARN or ERROR per team policy) when permanentFailures != lastReportedPermanentFailures or when permanentFailures increases; update lastReportedPermanentFailures after logging so identical repeated counts do not re-log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/resources/db/migration/dev/V3__insert_demo_data.sql`:
- Around line 10-13: The seeded demo users in V3__insert_demo_data.sql all share
the same bcrypt hash
('$2a$10$mxL7BIFi9S2jdA867vmuDeRe4SZ/krIiHsCzvOdh/3j5nviJ0G2xy') (rows for
'anna@test.se', 'lars@test.se', 'erik@klinik.se', 'sara@admin.se'), which leaks
a single credential for all accounts; fix by generating and using unique bcrypt
hashes per row (replacing the shared hash for 'anna@test.se', 'lars@test.se',
'erik@klinik.se', and 'sara@admin.se'), and either remove the ADMIN seed row for
'sara@admin.se' or set its is_active = false (and optionally set is_active =
false for the 'VET' row 'erik@klinik.se') so privileged accounts are not
immediately usable in dev/CI.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.java`:
- Around line 46-51: The ERROR log currently emitted every run using
repository.countByRetryCountGreaterThanEqual(MAX_RETRIES) causes alert fatigue;
fix by recording a class-level long lastReportedPermanentFailures (default 0),
update a Micrometer gauge via
MeterRegistry.gauge("s3.orphaned.permanent_failures", ...) with the current
count, and only call log.warn/log.error (use WARN or ERROR per team policy) when
permanentFailures != lastReportedPermanentFailures or when permanentFailures
increases; update lastReportedPermanentFailures after logging so identical
repeated counts do not re-log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dddf6509-b701-4845-a1ff-fbe961e9fae0
📒 Files selected for processing (6)
src/main/java/org/example/vet1177/repository/OrphanedS3ObjectRepository.javasrc/main/java/org/example/vet1177/services/AttachmentService.javasrc/main/java/org/example/vet1177/services/OrphanedS3CleanupWorker.javasrc/main/java/org/example/vet1177/services/OrphanedS3Enqueuer.javasrc/main/resources/application-dev.propertiessrc/main/resources/db/migration/dev/V3__insert_demo_data.sql
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/resources/application-dev.properties
- src/main/java/org/example/vet1177/services/OrphanedS3Enqueuer.java
- src/main/java/org/example/vet1177/repository/OrphanedS3ObjectRepository.java
- src/main/java/org/example/vet1177/services/AttachmentService.java
lindaeskilsson
approved these changes
Apr 26, 2026
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
2 participants
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.
Sammanfattning
Denna PR introducerar två stora förbättringar för projektets stabilitet och dataintegritet: Versionshanterad databas (Flyway) och ett självläkande system för fillagring (S3/MinIO).
Ändringar
1. Database Migrations (Flyway)
Vi har gått ifrån att låta Hibernate skapa tabeller till att använda Flyway.
V1__initial_schema.sql: Skapar grundstrukturen för kliniker, användare, djur och journaler.
V2__init_orphaned_table.sql: Skapar tabellen för att hantera radering av filer.
Fördel: Alla utvecklare har nu exakt samma databasstruktur och vi kan säkert rulla ut ändringar till produktion.
2. S3 Orphaned Object Cleanup
För att undvika att "föräldralösa" filer (filer som inte längre har en referens i databasen) blir liggande i MinIO/S3 och tar upp plats, har jag implementerat en asynkron städmekanism.
OrphanedS3Object Entity: En "att-göra-lista" i databasen för filer som ska raderas.
CleanupWorker: En schemalagd bakgrundstjänst som försöker radera filer var 10:e minut.
Retry-logik: Inkluderar felhantering med retryCount och loggning av felmeddelanden (lastError) om S3-tjänsten tillfälligt skulle vara nere.
Testning
Verifierat att Flyway-migreringar körs korrekt vid start.
Testat att manuellt lägga till rader i orphaned_s3_objects och sett att CleanupWorker raderar filen från MinIO och städar upp i tabellen.
Kontrollerat att retry_count ökar om man simulerar anslutningsfel till S3.
Noteringar
spring.jpa.hibernate.ddl-auto är nu satt till validate i dev-profilen för att säkerställa att koden matchar databasschemat.
S3-raderingar sker asynkront för att inte blockera användarens request-flöde.
Stängt av Flyway i testmiljön för att undvika syntaxfel i H2 med: spring.flyway.enabled=false
closes #114
Summary by CodeRabbit
Release Notes
New Features
Infrastructure