Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ public AttachmentPolicy(MedicalRecordPolicy medicalRecordPolicy) {
}

public void canUpload(User user, MedicalRecord record, String contentType, long fileSize) {
if (fileSize <= 0) {
throw new IllegalArgumentException("Filen kan inte vara tom (0 bytes).");
}

validateFileType(contentType);
validateFileSize(fileSize);
Expand Down
181 changes: 181 additions & 0 deletions src/main/java/org/example/vet1177/services/AttachmentService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
package org.example.vet1177.services;

import org.example.vet1177.config.AwsS3Properties;
import org.example.vet1177.dto.request.attachment.AttachmentRequest;
import org.example.vet1177.dto.response.attachment.AttachmentResponse;
import org.example.vet1177.entities.Attachment;
import org.example.vet1177.entities.MedicalRecord;
import org.example.vet1177.entities.User;
import org.example.vet1177.exception.ResourceNotFoundException;
import org.example.vet1177.policy.AttachmentPolicy;
import org.example.vet1177.repository.AttachmentRepository;
import org.example.vet1177.repository.MedicalRecordRepository;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import org.springframework.web.multipart.MultipartFile;

import java.io.IOException;
import java.util.UUID;

@Service
public class AttachmentService {

private static final Logger log = LoggerFactory.getLogger(AttachmentService.class);

private final AttachmentRepository attachmentRepository;
private final FileStorageService fileStorageService;
private final MedicalRecordRepository medicalRecordRepository;
private final AttachmentPolicy attachmentPolicy;
private final String bucketName;

public AttachmentService(AttachmentRepository attachmentRepository,
FileStorageService fileStorageService,
MedicalRecordRepository medicalRecordRepository,
AttachmentPolicy attachmentPolicy,
AwsS3Properties props) {
this.attachmentRepository = attachmentRepository;
this.fileStorageService = fileStorageService;
this.medicalRecordRepository = medicalRecordRepository;
this.attachmentPolicy = attachmentPolicy;
this.bucketName = props.getBucketName();
}


@Transactional(rollbackFor = Exception.class)
public AttachmentResponse uploadAttachment(User currentUser, MultipartFile file, AttachmentRequest request) throws IOException {
MedicalRecord record = medicalRecordRepository.findById(request.recordId())
.orElseThrow(() -> new ResourceNotFoundException("MedicalRecord", request.recordId()));

if (file.isEmpty() || file.getSize() <= 0) {
throw new IllegalArgumentException("Det går inte att ladda upp en tom fil.");
}

// Validering
attachmentPolicy.canUpload(currentUser, record, file.getContentType(), file.getSize());
Comment thread
coderabbitai[bot] marked this conversation as resolved.

String originalName = file.getOriginalFilename();
String sanitizedName = sanitizeFilename(originalName);

// Skapa unik S3-nyckel
String s3Key = String.format("records/%s/%s_%s",
record.getId(),
UUID.randomUUID(),
file.getOriginalFilename());
Comment thread
johanbriger marked this conversation as resolved.

// Anropa FileStorageService
try {
fileStorageService.upload(s3Key, file.getInputStream(), file.getSize(), file.getContentType());
} catch (Exception e) {
log.error("S3 upload failed for key: {}", s3Key);
throw new RuntimeException("Kunde inte ladda upp filen till lagringen", e);
}

// Skapa entitet
try {
Attachment attachment = new Attachment();
attachment.setMedicalRecord(record);
attachment.setUploadedBy(currentUser);
attachment.setFileName(file.getOriginalFilename());
attachment.setS3Key(s3Key);
attachment.setS3Bucket(bucketName);
attachment.setFileType(file.getContentType());
attachment.setFileSizeBytes(file.getSize());
attachment.setDescription(request.description());

attachment = attachmentRepository.saveAndFlush(attachment);

log.info("Attachment {} successfully persisted for record {}", attachment.getId(), record.getId());

return mapToResponse(attachment);

} catch (Exception e) {
// Tack vare saveAndFlush hamnar vi här om databasen nekar sparningen
log.error("Database persistence failed for attachment with S3 key: {}. Triggering S3 cleanup.", s3Key);

try {
fileStorageService.delete(s3Key);
} catch (Exception deleteEx) {
log.error("CRITICAL: Failed to cleanup S3 object {} after DB failure!", s3Key, deleteEx);
}

throw new RuntimeException("Kunde inte spara bilagans metadata. Uppladdningen avbröts.", e);
}
}

private String sanitizeFilename(String originalFilename) {
if (originalFilename == null || originalFilename.isBlank()) {
return UUID.randomUUID().toString();
}

String filename = new java.io.File(originalFilename).getName();
filename = filename.replaceAll("[^a-zA-Z0-9\\.\\-_]", "_");

filename = filename.trim();
if (filename.isEmpty() || filename.equals(".") || filename.equals("..")) {
return "file_" + System.currentTimeMillis();
}
return filename;
}


@Transactional(readOnly = true)
public AttachmentResponse getAttachment(User currentUser, UUID attachmentId) {
Attachment attachment = attachmentRepository.findById(attachmentId)
.orElseThrow(() -> new ResourceNotFoundException("Attachment", attachmentId));

attachmentPolicy.canDownload(currentUser, attachment);

return mapToResponse(attachment);
}


@Transactional
public void deleteAttachment(User currentUser, UUID attachmentId) {
Attachment attachment = attachmentRepository.findById(attachmentId)
.orElseThrow(() -> new ResourceNotFoundException("Attachment", attachmentId));

attachmentPolicy.canDelete(currentUser, attachment);

String s3Key = attachment.getS3Key();

attachmentRepository.delete(attachment);

if (TransactionSynchronizationManager.isSynchronizationActive()) {
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
@Override
public void afterCommit() {
try {
fileStorageService.delete(s3Key);
log.info("S3 object {} deleted after successful DB commit", s3Key);
} catch (Exception e) {
log.error("CRITICAL: Failed to delete S3 object {} after DB commit!", s3Key, e);
}
Comment on lines +145 to +156

@coderabbitai coderabbitai Bot Apr 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

A failed post-commit S3 delete becomes a permanent orphan.

Once the row is removed on Line 141, the afterCommit() block only logs if fileStorageService.delete(s3Key) fails. At that point the database no longer has anything to retry from, so a transient S3 issue can leave the blob undeleted indefinitely. Please hand this off to a durable retry path instead of dropping the failure in logs.

🤖 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 141 - 152, The current afterCommit() in AttachmentService deletes the S3
object but only logs failures, which creates permanent orphans; change it to
hand off failures to a durable retry path. Keep the immediate call to
fileStorageService.delete(s3Key) inside afterCommit, but in the catch block call
a durable retry enqueuer (e.g., S3DeletionRetryService.enqueue(s3Key) or
OrphanedS3ObjectRepository.save(new OrphanedS3Object(s3Key))) instead of only
logging; implement S3DeletionRetryService to persist the key (or publish to a
message queue) and have a background worker/process retry deletes until success,
and ensure the enqueue/save is idempotent and records error metadata for
debugging. Also ensure the enqueue/save call is used from the same afterCommit
registration (TransactionSynchronizationManager.registerSynchronization) so the
enqueue happens only after DB commit.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo ithsjava25/project-backend-org-random-coders \
  --assignee johanbriger \
  --title "feat: Implement durable retry logic for failed S3/MinIO deletions" \
  --body "## Summary

When a post-commit S3/MinIO deletion fails in \`AttachmentService.deleteAttachment()\`, the failure is currently only logged. This means the blob remains orphaned indefinitely with no automated recovery path.

This issue tracks the work required to implement a durable retry mechanism so that no files are left orphaned in MinIO if a post-commit deletion fails.

## Background

Discussed in PR `#112` (comment: https://github.com/ithsjava25/project-backend-org-random-coders/pull/112#discussion_r3032969422).

In \`AttachmentService\`, the \`afterCommit()\` hook attempts to delete the S3 object after the database row has been removed. If that deletion fails, there is currently no mechanism to retry it — the S3 key is lost from the database and the blob becomes a permanent orphan.

## Tasks

### 1. Create \`OrphanedS3Object\` Entity
- Create a JPA entity \`OrphanedS3Object\` with fields:
  - \`id\` (UUID, primary key)
  - \`s3Key\` (String, unique, not null)
  - \`s3Bucket\` (String, not null)
  - \`createdAt\` (Instant, auto-set on persist)
  - \`retryCount\` (int, default 0)
  - \`lastAttemptAt\` (Instant, nullable)
  - \`lastError\` (String, nullable — truncated error message for debugging)

### 2. Create \`OrphanedS3ObjectRepository\`
- Extend \`JpaRepository<OrphanedS3Object, UUID>\`
- Add a query method to fetch records eligible for retry (e.g., ordered by \`lastAttemptAt\` ascending, with a max retry cap)
- Ensure upsert/save is idempotent (unique constraint on \`s3Key\`)

### 3. Update \`AttachmentService\`
- In the \`afterCommit()\` catch block (inside \`TransactionSynchronizationManager.registerSynchronization\`), call \`OrphanedS3ObjectRepository.save(new OrphanedS3Object(s3Key, bucketName))\` instead of only logging
- The enqueue/save must happen inside the same \`afterCommit\` registration so it only fires after a successful DB commit

### 4. Create a Scheduled Background Worker
- Create a \`@Component\` (e.g., \`OrphanedS3CleanupWorker\`) annotated with \`@Scheduled\`
- Periodically query \`OrphanedS3ObjectRepository\` for pending orphaned objects
- For each, attempt \`fileStorageService.delete(s3Key)\`
- On success: delete the \`OrphanedS3Object\` row
- On failure: increment \`retryCount\`, update \`lastAttemptAt\` and \`lastError\`, save
- Implement a maximum retry cap (e.g., 10 attempts) after which the entry is flagged/alerted rather than retried endlessly

## Acceptance Criteria

- [ ] \`OrphanedS3Object\` entity and migration script created
- [ ] \`OrphanedS3ObjectRepository\` created with idempotent save
- [ ] \`AttachmentService.afterCommit()\` enqueues failed deletions to \`OrphanedS3ObjectRepository\` instead of only logging
- [ ] Background worker retries deletions on a schedule and removes entries on success
- [ ] Retry count is bounded; entries exceeding the cap are logged/alerted
- [ ] Solution is covered by unit and/or integration tests

## References
- Raised by: `@johanbriger`
- Related PR: `#112`"

Length of output: 74


🏁 Script executed:

#!/bin/bash
gh issue list --repo ithsjava25/project-backend-org-random-coders --limit 5 --json number,title,url

Length of output: 792


@johanbriger, the issue has been created and assigned to you: #114 — feat: Implement durable retry logic for failed S3/MinIO deletions

It covers all the areas discussed:

  1. OrphanedS3Object entity — with fields for s3Key, s3Bucket, retryCount, lastAttemptAt, and lastError for debugging.
  2. OrphanedS3ObjectRepository — idempotent save backed by a unique constraint on s3Key.
  3. AttachmentService update — enqueue failed deletions to the repository inside afterCommit() instead of only logging.
  4. OrphanedS3CleanupWorker — a @Scheduled background worker that retries pending deletions, removes entries on success, and enforces a retry cap to prevent endless loops.

}
});
} else {
fileStorageService.delete(s3Key);
}

log.info("Attachment {} marked for deletion in database", attachmentId);
}

private AttachmentResponse mapToResponse(Attachment attachment) {
String downloadUrl = fileStorageService.generatePresignedUrl(attachment.getS3Key());

return new AttachmentResponse(
attachment.getId(),
attachment.getMedicalRecord().getId(),
attachment.getFileName(),
attachment.getDescription(),
attachment.getFileType(),
attachment.getFileSizeBytes(),
attachment.getUploadedAt(),
attachment.getUploadedBy() != null ? attachment.getUploadedBy().getName() : "System",
downloadUrl
);
}
}