feat: implement project locks containing multiple job ids #3271
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package io.tolgee.api.v2.controllers.batch | ||
|
|
||
| import io.tolgee.batch.BatchJobProjectLockingManager | ||
| import io.tolgee.fixtures.andIsForbidden | ||
| import io.tolgee.fixtures.andIsOk | ||
| import io.tolgee.fixtures.waitFor | ||
|
|
@@ -14,6 +15,8 @@ import kotlinx.coroutines.ensureActive | |
| import org.junit.jupiter.api.AfterEach | ||
| import org.junit.jupiter.api.BeforeEach | ||
| import org.junit.jupiter.api.Test | ||
| import org.junit.jupiter.params.ParameterizedTest | ||
| import org.junit.jupiter.params.provider.ValueSource | ||
| import org.mockito.kotlin.* | ||
| import org.springframework.beans.factory.annotation.Autowired | ||
| import org.springframework.boot.test.context.SpringBootTest | ||
|
|
@@ -30,72 +33,91 @@ class BatchJobManagementControllerCancellationTest : | |
| @Autowired | ||
| lateinit var stuckBatchJobTestUtil: StuckBatchJobTestUtil | ||
|
|
||
| @Autowired | ||
| lateinit var batchJobProjectLockingManager: BatchJobProjectLockingManager | ||
|
|
||
| @Autowired | ||
| lateinit var batchProperties: io.tolgee.configuration.tolgee.BatchProperties | ||
|
|
||
| var simulateLongRunningChunkRun = true | ||
| var originalProjectConcurrency = 1 | ||
|
|
||
| @BeforeEach | ||
| fun setup() { | ||
| simulateLongRunningChunkRun = true | ||
| originalProjectConcurrency = batchProperties.projectConcurrency | ||
| } | ||
|
|
||
| @AfterEach | ||
| fun clean() { | ||
| simulateLongRunningChunkRun = false | ||
| batchProperties.projectConcurrency = originalProjectConcurrency | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(ints = [1, 2]) | ||
| @ProjectJWTAuthTestMethod | ||
| fun `cancels a job`(projectConcurrency: Int) { | ||
| val keys = testData.addTranslationOperationData(100) | ||
| saveAndPrepare() | ||
|
|
||
| batchProperties.projectConcurrency = projectConcurrency | ||
|
|
||
| val keyIds = keys.map { it.id }.toList() | ||
|
|
||
| val count = AtomicInteger(0) | ||
|
|
||
| doAnswer { | ||
| if (count.incrementAndGet() > 5) { | ||
| while (simulateLongRunningChunkRun) { | ||
| // this simulates long-running operation, which checks for active context | ||
| val context = it.arguments[2] as CoroutineContext | ||
| context.ensureActive() | ||
| Thread.sleep(10) | ||
| } | ||
| } | ||
| it.callRealMethod() | ||
| }.whenever(machineTranslationChunkProcessor).process(any(), any(), any(), any()) | ||
|
|
||
| performProjectAuthPost( | ||
| "start-batch-job/machine-translate", | ||
| mapOf( | ||
| "keyIds" to keyIds, | ||
| "targetLanguageIds" to | ||
| listOf( | ||
| testData.projectBuilder.getLanguageByTag("cs")!!.self.id, | ||
| ), | ||
| ), | ||
| ).andIsOk | ||
|
|
||
| waitFor { | ||
| batchJobConcurrentLauncher.runningJobs.size >= 5 | ||
| } | ||
|
|
||
| val job = util.getSingleJob() | ||
|
|
||
| // Verify the job is locked | ||
| val lockedJobs = batchJobProjectLockingManager.getLockedForProject(testData.project.id) | ||
| lockedJobs.assert.contains(job.id) | ||
|
|
||
| performProjectAuthPut("batch-jobs/${job.id}/cancel") | ||
| .andIsOk | ||
|
|
||
| waitForNotThrowing(pollTime = 100) { | ||
| executeInNewTransaction { | ||
| util.getSingleJob().status.assert.isEqualTo(BatchJobStatus.CANCELLED) | ||
| verify(batchJobActivityFinalizer, times(1)).finalizeActivityWhenJobCompleted(any()) | ||
|
|
||
| // assert activity stored | ||
| entityManager.createQuery("""from ActivityRevision ar where ar.batchJob.id = :id""") | ||
| .setParameter("id", job.id).resultList | ||
| .assert.hasSize(1) | ||
| } | ||
| } | ||
|
|
||
| // Verify the job lock was released | ||
| val lockedJobsAfterCancel = batchJobProjectLockingManager.getLockedForProject(testData.project.id) | ||
| lockedJobsAfterCancel.assert.doesNotContain(job.id) | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -135,4 +157,106 @@ class BatchJobManagementControllerCancellationTest : | |
|
|
||
| util.getSingleJob().status.assert.isEqualTo(BatchJobStatus.CANCELLED) | ||
| } | ||
|
|
||
| @Test | ||
| @ProjectJWTAuthTestMethod | ||
| fun `cancels one of two running jobs with projectConcurrency=2`() { | ||
| val keys = testData.addTranslationOperationData(100) | ||
| saveAndPrepare() | ||
|
|
||
| batchProperties.projectConcurrency = 2 | ||
|
|
||
| val keyIds = keys.map { it.id }.toList() | ||
|
|
||
| val firstJobCount = AtomicInteger(0) | ||
| val secondJobCount = AtomicInteger(0) | ||
|
|
||
| doAnswer { | ||
| val jobId = it.getArgument<io.tolgee.batch.data.BatchJobDto>(0).id | ||
| val allJobs = executeInNewTransaction { | ||
| batchJobService.getAllByProjectId(testData.project.id) | ||
| } | ||
| val isFirstJob = allJobs.size > 0 && jobId == allJobs[0].id | ||
|
|
||
| val counter = if (isFirstJob) firstJobCount else secondJobCount | ||
|
|
||
| if (counter.incrementAndGet() > 5) { | ||
| while (simulateLongRunningChunkRun) { | ||
| // this simulates long-running operation, which checks for active context | ||
| val context = it.arguments[2] as CoroutineContext | ||
| context.ensureActive() | ||
| Thread.sleep(10) | ||
| } | ||
| } | ||
| it.callRealMethod() | ||
| }.whenever(machineTranslationChunkProcessor).process(any(), any(), any(), any()) | ||
|
Comment on lines
+176
to
+194
There was a problem hiding this comment. 🧩 Analysis chainEnsure deterministic job ordering in the test. The mock logic at line 181 assumes Verify that
Example approach using stored IDs: // After line 206
val firstJobId = performProjectAuthPost(...)
.andIsOk.andReturn().response.contentAsString.let {
// parse job ID from response
}Then reference Also applies to: 228-230 🏁 Script executed: #!/bin/bash
# Find BatchJobService and its getAllByProjectId method
ast-grep --pattern 'fun getAllByProjectId($_, $_) {
$$$
}'Length of output: 0 🏁 Script executed: #!/bin/bash
# Search for getAllByProjectId usages and definitions in the codebase
rg -nP --type=kotlin 'getAllByProjectId' -B3 -A8Length of output: 25551 🏁 Script executed: #!/bin/bash
# Examine the test file to understand job ID storage and mock usage
cat backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt | head -250Length of output: 7613 🏁 Script executed: #!/bin/bash
# Find BatchJobRepository and its findAllByProjectId method
rg -nP --type=kotlin 'class BatchJobRepository|fun findAllByProjectId' -B2 -A10 | head -100Length of output: 10377 🏁 Script executed: #!/bin/bash
# Search more specifically for the repository definition and query
find . -name "BatchJobRepository.kt" -type f -exec cat {} \;Length of output: 3402 🏁 Script executed: #!/bin/bash
# Check the POST response to see if job IDs are returned
grep -n "performProjectAuthPost" backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/BatchJobManagementControllerCancellationTest.kt | head -20Length of output: 96 🏁 Script executed: #!/bin/bash
# Look at the batch job controller to see what the POST response contains
find . -path "*/controller/*BatchJob*Controller.kt" -type f | head -5Length of output: 0 🏁 Script executed: #!/bin/bash
# Check what util.getSingleJob() does and if responses contain job IDs
grep -n "getSingleJob\|startBatchJob" backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/*.kt | head -20Length of output: 1243 🏁 Script executed: #!/bin/bash
# Find the abstract test class
find . -name "AbstractBatchJobManagementControllerTest.kt" -type f -exec cat {} \;Length of output: 2604 🏁 Script executed: #!/bin/bash
# Search for the util field initialization and getSingleJob implementation
grep -n "class.*util\|val util\|fun getSingleJob" backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/batch/*.kt -A5Length of output: 0 🏁 Script executed: #!/bin/bash
# Check what the POST response looks like (check controller or DTO)
rg -nP --type=kotlin 'BatchJobDto|BatchJobResponse' backend/app/src/main/kotlin --type=kotlin -C3 | head -50Length of output: 0 🏁 Script executed: #!/bin/bash
# Find BatchJobTestUtil
find . -name "BatchJobTestUtil.kt" -type f -exec cat {} \;Length of output: 15364 🏁 Script executed: #!/bin/bash
# Search for BatchJobTestUtil and its methods
rg -nP --type=kotlin 'class BatchJobTestUtil|fun getSingleJob' -A10Length of output: 3610 🏁 Script executed: #!/bin/bash
# Check the batch job data models to see what fields are available
find . -path "*/batch/data/*" -name "*.kt" -type f | head -10Length of output: 668 🏁 Script executed: #!/bin/bash
# Find the batch job controller endpoint for POST machine-translate
rg -nP --type=kotlin '"start-batch-job"' -B5 -A15 | head -80Length of output: 0 🏁 Script executed: #!/bin/bash
# Look for BatchJobDto to see if it contains ID
cat backend/data/src/main/kotlin/io/tolgee/batch/data/BatchJobDto.ktLength of output: 1702 Deterministic job identification needed— The mock logic assumes Store and use job IDs from the POST responses (lines 197-206, 213-222) instead of relying on array indices at lines 181, 236-237. Since val firstJobIdResponse = performProjectAuthPost(
"start-batch-job/machine-translate",
mapOf(...)
).andIsOk.andReturn().response.contentAsString
// Parse firstJobId from JSON responseThen reference stored IDs directly in the mock and assertions instead of |
||
|
|
||
| // Start first job | ||
| performProjectAuthPost( | ||
| "start-batch-job/machine-translate", | ||
| mapOf( | ||
| "keyIds" to keyIds, | ||
| "targetLanguageIds" to | ||
| listOf( | ||
| testData.projectBuilder.getLanguageByTag("cs")!!.self.id, | ||
| ), | ||
| ), | ||
| ).andIsOk | ||
|
|
||
| waitFor(timeout = 5000) { | ||
| batchJobConcurrentLauncher.runningJobs.size >= 5 | ||
| } | ||
|
|
||
| // Start second job | ||
| performProjectAuthPost( | ||
| "start-batch-job/machine-translate", | ||
| mapOf( | ||
| "keyIds" to keyIds, | ||
| "targetLanguageIds" to | ||
| listOf( | ||
| testData.projectBuilder.getLanguageByTag("cs")!!.self.id, | ||
| ), | ||
| ), | ||
| ).andIsOk | ||
|
|
||
| waitFor(timeout = 5000) { | ||
| batchJobConcurrentLauncher.runningJobs.size >= 10 | ||
| } | ||
|
|
||
| val jobs = executeInNewTransaction { | ||
| batchJobService.getAllByProjectId(testData.project.id) | ||
| } | ||
| jobs.size.assert.isEqualTo(2) | ||
|
|
||
| // Verify both jobs are locked | ||
| val lockedJobs = batchJobProjectLockingManager.getLockedForProject(testData.project.id) | ||
| lockedJobs.size.assert.isEqualTo(2) | ||
| lockedJobs.assert.contains(jobs[0].id) | ||
| lockedJobs.assert.contains(jobs[1].id) | ||
|
|
||
| // Cancel the first job | ||
| val firstJob = jobs[0] | ||
| performProjectAuthPut("batch-jobs/${firstJob.id}/cancel") | ||
| .andIsOk | ||
|
|
||
| waitForNotThrowing(pollTime = 100) { | ||
| executeInNewTransaction { | ||
| batchJobService.getJobDto(firstJob.id).status.assert.isEqualTo(BatchJobStatus.CANCELLED) | ||
| } | ||
| } | ||
|
|
||
| // Verify the first job lock was released but second job is still locked | ||
| val lockedJobsAfterCancel = batchJobProjectLockingManager.getLockedForProject(testData.project.id) | ||
| lockedJobsAfterCancel.assert.doesNotContain(firstJob.id) | ||
| lockedJobsAfterCancel.assert.contains(jobs[1].id) | ||
|
|
||
| // Let the second job complete | ||
| simulateLongRunningChunkRun = false | ||
|
|
||
| waitForNotThrowing(pollTime = 100, timeout = 10000) { | ||
| executeInNewTransaction { | ||
| batchJobService.getJobDto(jobs[1].id).status.assert.isEqualTo(BatchJobStatus.SUCCESS) | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is
batchDumperremoved here? It's really convinient for investigation in case of a test failureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just maybe
batchDumpershould be extended with smth likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while playing with it i actually noticed that setting
batchProperties.projectConcurrency = 2doesn't work here. It must be passed as application properties in the@SpringBootTest(There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re
batchDumper, I missed this, will have a look.re
batchProperties.projectConcurrency = 2how did you notice it doesn't work? Spring will start it with its standard configuration, and then it is overridden before the test, and then returned to the original configuration after the tests.As the checks against the properties in
BatchJobProjectLockingManagerare always done dynamically, it should work.This approach allows "@ParameterizedTest" to be used. With spring test properties, it would require instantiating a different spring boot test context with different methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed the requested
batchDumperchanges.Unfortunately tests are all failing on my machine today (due to PG setup), but the change makes sense and it all compiles... Let's see if the CI agrees :D
@bdshadow Would you have any idea why the tests might be failing locally?
The build system seems to have launched the docker containers successfully