chore(backend): add task lock annotation#6728
Conversation
corneliusroemer
force-pushed
the
chore/backend-task-lock-annotation
branch
from
4f0a0a4 to
2103989
Compare
|
Claude finished @corneliusroemer's task in 2m 5s —— View job Code Review
Good refactor — the boilerplate reduction is clear and the aspect is implemented correctly. A couple of observations: Duplicated interval string (minor maintainability concern) Each task callsite now has the same property placeholder in both Missing test coverage for error paths
Spring AOP self-call limitation
The four existing test cases are well-structured and cover the happy path and important edge cases (exception propagation, time-unit conversion). The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| try { | ||
| return joinPoint.proceed() | ||
| } finally { |
There was a problem hiding this comment.
The intervalSeconds is computed before acquireLock is called, so if resolveInterval throws (unresolvable or non-numeric property), the lock is never acquired and there's nothing to release — correct. Just noting this ordering is load-bearing.
| taskLockService.releaseLock(taskLock.name, frequencyIntervalSeconds = intervalSeconds) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Spring AOP proxies only intercept calls coming through the proxy (i.e., from external callers). If task() were ever called from within the same bean, the lock annotation would be silently ignored. This is the standard Spring AOP limitation — worth a brief comment since it's non-obvious that @TaskLock won't fire on self-calls.
| ) { | ||
| @Scheduled(fixedRateString = "\${${BackendSpringProperty.CLEAN_UP_RUN_EVERY_SECONDS}}", timeUnit = TimeUnit.SECONDS) | ||
| @TaskLock( | ||
| name = CLEAN_UP_STALE_SEQUENCES_IN_PROCESSING_TASK_NAME, |
There was a problem hiding this comment.
The interval property is specified twice — once in @Scheduled(fixedRateString = ...) and again in @TaskLock(intervalString = ...). They must stay in sync; a silent mismatch would cause the lock to be held for the wrong duration without any error. A comment here that both refer to the same property would help, or document this in the @TaskLock KDoc.
| private val joinPoint = mockk<ProceedingJoinPoint>() | ||
| private val aspect = TaskLockAspect(taskLockService).also { | ||
| it.setEmbeddedValueResolver { value -> if (value == "\${task.interval}") "10" else value } | ||
| } |
There was a problem hiding this comment.
Missing tests for the two error paths in resolveInterval:
- Property placeholder that resolves to
null(should throwIllegalArgumentException) - Property that resolves to a non-numeric string like
"10s"(should throwIllegalArgumentException)
These paths are in the implementation but have no coverage. Fix this →
There was a problem hiding this comment.
Pull request overview
This PR introduces a reusable @TaskLock annotation and TaskLockAspect to remove repeated acquire/release boilerplate from scheduled backend tasks, while keeping lock configuration (name + interval + unit) at the task method callsite.
Changes:
- Added
@TaskLockand an AOP aspect to wrap task execution withTaskLockServiceacquire/release behavior. - Migrated several scheduled tasks to use
@TaskLockinstead of inline lock handling. - Enabled Spring Boot AOP in the backend build and added a focused unit test for the aspect.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/main/kotlin/org/loculus/backend/service/scheduler/TaskLock.kt | Adds the new @TaskLock annotation for scheduled methods. |
| backend/src/main/kotlin/org/loculus/backend/service/scheduler/TaskLockAspect.kt | Implements the AOP wrapper around task execution with placeholder resolution and lock lifecycle management. |
| backend/src/test/kotlin/org/loculus/backend/service/scheduler/TaskLockAspectTest.kt | Adds unit coverage for lock skip/run/release behavior and minute-to-second conversion. |
| backend/src/main/kotlin/org/loculus/backend/service/submission/CleanUpStaleSequencesInProcessingTask.kt | Replaces inline lock boilerplate with @TaskLock. |
| backend/src/main/kotlin/org/loculus/backend/service/submission/UseNewerProcessingPipelineVersionTask.kt | Replaces inline lock boilerplate with @TaskLock. |
| backend/src/main/kotlin/org/loculus/backend/service/seqsetcitations/SeqSetCrossRefCitationsTask.kt | Replaces inline lock boilerplate with @TaskLock (minutes-based interval). |
| backend/build.gradle | Adds spring-boot-starter-aop dependency for aspect support. |
| backend/gradle.lockfile | Locks AOP-related dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class TaskLockAspect(private val taskLockService: TaskLockService) : EmbeddedValueResolverAware { | ||
| private lateinit var embeddedValueResolver: StringValueResolver |
| fun lockTask(joinPoint: ProceedingJoinPoint, taskLock: TaskLock): Any? { | ||
| val intervalSeconds = taskLock.timeUnit.toSeconds(resolveInterval(taskLock)) | ||
| if (!taskLockService.acquireLock(taskLock.name, frequencyIntervalSeconds = intervalSeconds)) return null | ||
|
|
Summary
This PR replaces the repeated task-lock acquire/release boilerplate in scheduled backend tasks with a reusable
@TaskLockannotation. The annotation keeps the lock name and schedule interval at the method callsite, whileTaskLockAspectwraps the method execution with the existingTaskLockServiceacquire/release behavior.The affected scheduled tasks now keep their bodies focused on task logic: cleaning stale sequences, checking for newer preprocessing pipeline versions, and updating SeqSet CrossRef citations. The aspect resolves Spring property placeholders from the annotation, converts non-second intervals when needed, skips the task body if the lock is already held, and releases the lock in a
finallyblock after execution.This also adds Spring Boot AOP support and locks the new AOP dependencies in
gradle.lockfile. A focused unit test covers the aspect behavior for skipped execution, successful execution, exception handling, and minute-to-second interval conversion.Validation
Ran with Docker-backed test infrastructure:
./gradlew test --tests org.loculus.backend.service.submission.CleanUpStaleSequencesInProcessingTaskTest --tests org.loculus.backend.service.submission.UseNewerProcessingPipelineVersionTaskTest --tests org.loculus.backend.service.scheduler.TaskLockAspectTest --console=plain ./gradlew ktlintFormat --console=plainBoth commands completed successfully.
🚀 Preview: Add
previewlabel to enable