Repository navigation
Conversation
…t rolls back The createNTestAdministrators2 test mutation starts 10 writes in withTransaction() and returned Promise.all(). The first failure rejected Promise.all, so withTransaction() rolled back and released the query runner while other writes were still running. A write queued on the connection after the ROLLBACK ran outside the transaction, and the database kept its user row. The test then counted one user too many, and the next tests of the suite failed with the same extra row. The work function now waits for all writes to settle before it returns or throws, so the rollback includes every write. The error that the test expects is now always "Failed!".
…s jobs The test runs six product and variant mutations, and each one queues an apply-collection-filters job for all Collections. The job queue runs the jobs one at a time, so on a slow runner the jobs need more than the 5 seconds that the test waited. The test now waits up to 20 seconds. awaitRunningJobs() returns when the queue is empty, so a fast run does not wait longer.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Dashboard Preview: https://admin-dashboard-jt6hdzx70-vendure.vercel.app |
|
Description
Fixes two core e2e tests that fail at random on CI and pass on a re-run. Both are test problems, so I made no core change.
database-transactions.e2e-spec.ts: concurrent writes inwithTransaction()Seen on
majorand on PRs into it (MySQL): runs 43024393448, 43025720370 and 43232946603. The test "failing mutation inside connection.withTransaction() wrapper with context and promise concurrent execution" failed withexpected 4 to be 3. The two tests after it then failed too, because the extra row stays in the database.Cause. The
createNTestAdministrators2test mutation starts 10 writes insidewithTransaction(), 10 ms apart, and returnsPromise.all(). The first failing write rejectsPromise.all(), sowithTransaction()rolls back and releases the query runner while other writes are still running. A write that the driver queues on the connection after theROLLBACKruns outside the transaction, and the database keeps its row. On a fast host the rollback ends before the next write starts, so later writes fail with "Query runner already released" and nothing leaks. On a slow runner the rollback overlaps writes that are still running.Fix. The work function now waits for all writes to settle before it returns or throws, so the rollback includes every write. This matches the
@Transaction()variant of the same test, which already waits. The expected error is now alwaysFailed!, so the test no longer accepts "Query runner already released".Reproduction. I used a temporary spec (not committed) that calls the mutation 40 times and counts leaked user rows. To get the same overlap as on a slow runner, the gap between writes was 1 ms instead of 10 ms:
collection.e2e-spec.ts:awaitRunningJobstimeoutSeen in runs 42792699214 (Postgres and MySQL) and 42800763951 (MySQL). The test "combining an AND-ed and an OR-ed filter > does not duplicate variants pulled in by the OR-ed filter when they have multiple translations" failed with
awaitRunningJobs timed out after 5000ms.Cause. The test runs six product and variant mutations before it creates the collection, and each one queues an
apply-collection-filtersjob for all Collections. The queue runs the jobs one at a time and takes the next one about 200 ms after the previous one ends. On an idle host, Postgres needs 2.6 s for this test's jobs. With the Postgres container limited to 0.1 CPU it needs 4.5 s, and with 0.05 CPU it needs 12.6 s. At 0.05 CPU the unchanged test fails with the CI error (timed out after 5000ms with 6 job(s) still running). The jobs it leaves behind then also make the next test time out.Fix. The test waits up to 20 seconds.
awaitRunningJobs()returns as soon as the queue is empty, so a fast run does not wait longer. With the fix, the whole spec passes at 0.05 CPU.Proof
20 consecutive local runs, with
CI=trueand the database images and settings from the CI workflow:database-transactions.e2e-spec.ts: 20 of 20 on Postgres 16, 20 of 20 on MySQL 8.collection.e2e-spec.ts: 20 of 20 on Postgres 16.Breaking changes
None. Test changes only.
Checklist
📌 Always:
👍 Most of the time:
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.