Skip to content

Commit 1ae0d7c

Browse files
dkrizanclaude
andcommitted
fix: async branch cleanup - fix test races, deadlock, and SQL bug
- Add BackgroundCleanupTracker interface so CleanDbTestListener can wait for in-flight async cleanup workers before truncating tables, preventing the PostgreSQL deadlock between BranchCleanupWorker's DELETE row locks and CleanDbTestListener's ALTER TABLE DISABLE TRIGGER ALL. - Refactor BranchCleanupWorker: scheduleCleanup() is now a plain method (called within the active transaction) that registers the tracker and hooks TransactionSynchronization to dispatch the @async executeCleanup() after commit (and deregisters on rollback). BranchServiceImpl.deleteBranch is reduced to a single branchCleanupWorker.scheduleCleanup() call with no transaction sync boilerplate. Self-injection via @lazy enables the afterCommit callback to go through the Spring proxy for @async dispatch. - Fix SQL bug: ScreenshotRepository.findOrphanFilenamesByBranchId used native SQL selecting s.filename, but Screenshot.filename is a Kotlin computed property (not a DB column). Changed to JPQL returning Screenshot entities; updated ScreenshotService.deleteFilesByBranch accordingly. - Update BranchControllerTest and BranchCopyIntegrationTest to use branchCleanupWorker.waitForPendingCleanups() instead of waitForNotThrowing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 6baf1bf commit 1ae0d7c

9 files changed

Lines changed: 419 additions & 107 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package io.tolgee.component
2+
3+
/**
4+
* Implemented by services that perform background (async) data cleanup.
5+
*
6+
* [CleanDbTestListener] collects all beans of this type and calls
7+
* [waitForPendingCleanups] before truncating the database between tests,
8+
* preventing deadlocks between the cleanup worker's row-level locks and
9+
* the listener's ALTER TABLE … DISABLE TRIGGER ALL statements.
10+
*/
11+
interface BackgroundCleanupTracker {
12+
/**
13+
* Block until all in-flight cleanups started by this service have finished
14+
* (either successfully or with an error).
15+
*/
16+
fun waitForPendingCleanups(timeoutMs: Long = 30_000)
17+
}

backend/data/src/main/kotlin/io/tolgee/repository/ScreenshotRepository.kt

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import io.tolgee.model.key.screenshotReference.KeyScreenshotReference
66
import org.springframework.context.annotation.Lazy
77
import org.springframework.data.jpa.repository.JpaRepository
88
import org.springframework.data.jpa.repository.Query
9+
import org.springframework.data.repository.query.Param
910
import org.springframework.stereotype.Repository
1011

1112
@Repository
@@ -75,4 +76,29 @@ interface ScreenshotRepository : JpaRepository<Screenshot, Long> {
7576
""",
7677
)
7778
fun getAllKeyScreenshotReferences(key: Key): List<KeyScreenshotReference>
79+
80+
/**
81+
* Returns screenshots that will become orphans after all key_screenshot_reference
82+
* rows for [branchId]'s keys are deleted — i.e., screenshots referenced ONLY by branch keys
83+
* and by no key on any other branch.
84+
*
85+
* Uses JPQL (not nativeQuery) because Screenshot.filename is a Kotlin computed property,
86+
* not a DB column — so it cannot be selected in native SQL.
87+
*/
88+
@Query(
89+
"""
90+
SELECT s FROM Screenshot s
91+
WHERE s.id IN (
92+
SELECT ksr.screenshot.id FROM KeyScreenshotReference ksr
93+
WHERE ksr.key.id IN (SELECT k.id FROM Key k WHERE k.branch.id = :branchId)
94+
)
95+
AND s.id NOT IN (
96+
SELECT ksr.screenshot.id FROM KeyScreenshotReference ksr
97+
WHERE ksr.key.id NOT IN (SELECT k.id FROM Key k WHERE k.branch.id = :branchId)
98+
)
99+
""",
100+
)
101+
fun findOrphansByBranchId(
102+
@Param("branchId") branchId: Long,
103+
): List<Screenshot>
78104
}

backend/data/src/main/kotlin/io/tolgee/service/key/ScreenshotService.kt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,17 @@ class ScreenshotService(
351351
removeScreenshotReferences(all)
352352
}
353353

354+
/**
355+
* Deletes storage files for screenshots that will become orphans after all
356+
* key_screenshot_reference rows for [branchId]'s keys are removed.
357+
* The actual DB rows are deleted by the caller via bulk SQL.
358+
*/
359+
fun deleteFilesByBranch(branchId: Long) {
360+
screenshotRepository.findOrphansByBranchId(branchId).forEach { screenshot ->
361+
deleteFile(screenshot)
362+
}
363+
}
364+
354365
private fun deleteFile(screenshot: Screenshot) {
355366
fileStorage.deleteFile(screenshot.getFilePath())
356367
}

backend/testing/src/main/kotlin/io/tolgee/CleanDbTestListener.kt

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package io.tolgee
22

33
import io.tolgee.batch.BatchJobChunkExecutionQueue
44
import io.tolgee.batch.BatchJobConcurrentLauncher
5+
import io.tolgee.component.BackgroundCleanupTracker
56
import kotlinx.coroutines.TimeoutCancellationException
67
import org.postgresql.util.PSQLException
78
import org.slf4j.LoggerFactory
@@ -36,6 +37,13 @@ class CleanDbTestListener : TestExecutionListener {
3637
val batchJobConcurrentLauncher = appContext.getBean(BatchJobConcurrentLauncher::class.java)
3738
val batchJobQueue = appContext.getBean(BatchJobChunkExecutionQueue::class.java)
3839

40+
// Wait for any background cleanup workers (e.g. BranchCleanupWorker) to finish
41+
// before truncating tables. Without this, their long-running transactions hold
42+
// row-level locks that deadlock with ALTER TABLE … DISABLE TRIGGER ALL below.
43+
appContext.getBeansOfType(BackgroundCleanupTracker::class.java).values.forEach {
44+
it.waitForPendingCleanups()
45+
}
46+
3947
batchJobConcurrentLauncher.pause = true
4048
batchJobQueue.clear()
4149

@@ -54,7 +62,9 @@ class CleanDbTestListener : TestExecutionListener {
5462
}
5563
}
5664

57-
else -> throw e
65+
else -> {
66+
throw e
67+
}
5868
}
5969
logger.info(
6070
"Failed to clean DB, retrying in 1s. Attempt ${i + 1}, " +

0 commit comments

Comments
 (0)