Fix flaky tests caused by leaked job threads and signal timing#766
Merged
Conversation
The tests only waited for jobs to be claimed before signaling, so on a slow runner the signal's exit! could land after the instant job was claimed, or even after it had written its result, but before its finished_at was committed, leaving it unfinished and failing the test. Wait until the instant job has actually finished and the paused job has actually started before signaling, so the signal always arrives while only the paused job is in-flight, which is the scenario these tests exist to check. The async QUIT test's pause also becomes 60 seconds like the forked variant's: with only 1 second, a slow QUIT could land after the paused job finished, breaking the assertion that it remains claimed. Nothing waits for the pause, so test runtime is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A worker stopped while a job is running gives up on its pool after SolidQueue.shutdown_timeout and releases the claim, but it can't stop the thread running the job. A forked worker exits right after, killing the thread; a worker running in-process in a test leaks it instead, sleeping until the job's pause is over, long after its test finished. Inside a transactional test this corrupts later tests: the leaked job's inserts roll back with the test, and on SQLite the AUTOINCREMENT sequence bump rolls back too, so later tests' rows get the same ids and the leaked thread writes over them when it wakes up. This was behind several flaky failures on SQLite: a job result overwritten with "completed" mid-assertion, and a claimed execution stolen by a leaked finalize call on a recycled claim id, leaving a job neither finished nor ready. It can't happen in production: ids are only re-issued when the insert rolls back, and a real worker's threads die with its process. Kill the leaked threads in the tests that create them, like a forked worker's exit would, and fix the explanation of SQLite id reuse in the concurrency controls test setup: with AUTOINCREMENT, committed deletes never free ids; it's the transactional rollback that recycles them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Fixes the integration test flakes that have been hitting CI lately (seen on #764 and #765 runs), all test-side — no product changes. Two independent root causes:
1. QUIT/KILL signal timing (
AsyncProcessesLifecycleTest/ForkedProcessesLifecycleTest): the tests only waited for jobs to be claimed before signaling, so on a slow runner theexit!could land after the instant job was claimed — or even after it wrote its result — but beforefinished_atcommitted, leaving it unfinished. Reproduced deterministically by injecting a small delay before the finish write. The tests now wait until the instant job has finished and the paused job has started (via its JobResult row) before signaling, so the signal always arrives in exactly the scenario the tests exist to check. Verified 25/25 stable on mysql for each previously-flaky test.2. Leaked job threads from in-process workers (
InstrumentationTest,JobTest): a worker stopped in-process while a job is running can't stop the thread running it (ThreadPoolExecutor#shutdowndoesn't kill running tasks) — a forked worker's exit kills it, but in tests the thread leaks and wakes up seconds later, after its test finished. Inside a transactional test this corrupts later tests: the leaked job's inserts roll back, and on SQLite the AUTOINCREMENT sequence bump rolls back too, so later tests' rows get the same ids and the leaked thread writes over them when it wakes. Confirmed via CI log artifacts: this is what overwrote aJobResultaccumulator with"completed"mid-assertion, and what stole a claimed execution via a leakedfinalizeon a recycled claim id (leaving a job neither finished nor ready). It cannot happen in production — ids are only re-issued on transactional rollback, and a real worker's threads die with its process. The fix kills the leaked threads in the tests that create them, mirroring what a forked worker's exit does, and corrects the SQLite id-reuse explanation in the concurrency controls setup (with AUTOINCREMENT, committed deletes never free ids; it's the rollback that recycles them).This should also clear several other recent sqlite CI failures with the same signature (
discard_on_conflict_and_release_semaphore,run_several_jobs_over_the_same_record_limiting_concurrency).Stability evidence: adversarial orderings that failed pre-fix pass 20/20 and 50/50 post-fix; all affected files green on sqlite and mysql.
🤖 Generated with Claude Code