test: stabilize queues worker-recovery and pgvector read-replica CI flakes - #18078
Conversation
📦 esbuild Bundle Analysis for payloadThis analysis was generated by esbuild-bundle-analyzer. 🤖
Largest pathsThese visualization shows top 20 largest paths in the bundle.Meta file: packages/next/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_index.json, Out file: esbuild/index.js
Meta file: packages/payload/meta_shared.json, Out file: esbuild/exports/shared.js
Meta file: packages/richtext-lexical/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_client.json, Out file: esbuild/exports/client_optimized/index.js
Meta file: packages/ui/meta_shared.json, Out file: esbuild/exports/shared_optimized/index.js
DetailsNext to the size is how much the size has increased or decreased compared with the base branch of this PR.
|
…very test The "healthy long-running job" worker recovery test used a 600ms lease duration with only a 50ms safety buffer, leaving ~550ms for heartbeats to renew the lease. On a loaded CI runner a single delayed heartbeat can exceed that margin, causing the worker's own completion update to be rejected as lease-expired (JobRunAbortedError), which gets silently filtered out of jobStatus. Widen the margin to 5000ms/300ms so a single CI-scale scheduling stall can't exceed it, while keeping the same duration/safetyBuffer ratio.
05c459f to
9d946ac
Compare
… flake The vector column tests read query results through the raw payload.db.drizzle handle right after payload.create(). That handle is wrapped by withReplicas(), so its reads default to a replica and bypass payload's read-after-write routing, returning stale/empty rows on the postgres-read-replica adapter. Read through payload.db.primaryDrizzle (falling back to drizzle when no replicas are configured) so the assertions see the just-written rows.
…overy-flaky-timing # Conflicts: # test/database/postgres-vector.int.spec.ts
Replace the three duplicated read-through-primary blocks in the vector column tests with a single primaryDb() helper, so the read-replica handling and its explanatory comment live in one place.
|
/ai-review |
There was a problem hiding this comment.
Issues
Minor (nice to have)
- [test/database/postgres-vector.int.spec.ts:14] — The comment "Read through the primary.
payload.db.drizzleis the withReplicas() handle whose reads default to a replica, which races replication lag right after a write." explains what the code does rather than why the constraint exists at a deeper level. It's reasonably informative, but "which races replication lag right after a write" could be tightened — the key "why" is that the test writes a record and immediately queries it, so reading from a replica risks returning stale data before replication catches up. The current wording conveys this adequately, so this is purely cosmetic.
Recommendations
The primaryDb helper in postgres-vector.int.spec.ts is a well-scoped fix for a genuine race condition: after a write, reading from a replica-routed handle can return stale data before replication catches up. Accessing adapter.primaryDrizzle ?? adapter.drizzle directly from tests does reach into an adapter internal, but there's no public API for this scenario and the fallback makes it safe when primaryDrizzle is absent.
The queue test timing changes (duration 600 → 5000, safetyBuffer 50 → 300, wait 900 → 6000) substantially lengthen a CI test (6 seconds of wait alone). If flakiness at the old values was the motivation, the change is justified, but it may be worth documenting in a comment why the specific values were chosen and whether they were the minimum needed to eliminate the flake.
Assessment
Ready to merge? Yes
Reasoning: Both changes are targeted bug/flakiness fixes with no correctness, security, or convention violations. The primaryDb helper correctly handles the replica-lag race condition, and the queue lease timing values are straightforward numerical increases to prevent test flakiness.
Summary
I'm trying to fix some flakey int tests
Why
Two int suites failed intermittently in CI across different adapters:
int-sqlite-uuidv7, and the same flake onmongodb-atlasatqueues:1469). The "healthy long-running job" test set a 600ms lease with a 50ms safety buffer and waited 900ms. That left almost no room for heartbeats to renew the lease. On a loaded CI runner, one delayed heartbeat passed the margin. The worker's own completion update was then rejected as lease-expired (JobRunAbortedError) and removed fromjobStatus, so the test sawundefinedinstead ofsuccess.int-postgres-read-replica). The three vector-column tests read query results through the rawpayload.db.drizzlehandle right afterpayload.create(). That handle is wrapped bywithReplicas(), so its reads go to a replica by default. Payload routes its own reads to the primary for 2s after a write, but a raw handle read skips that routing. The read hit the lagging replica and returned stale or empty rows (expected length 2, got 0/1).How
duration=600 / safetyBuffer=50toduration=5000 / safetyBuffer=300, and wait 6000ms. This keeps the same duration-to-buffer ratio, so a single CI-scale scheduling stall no longer passes the margin. This resolves the flake on bothsqlite-uuidv7andmongodb-atlas.payload.db.primaryDrizzle, and fall back topayload.db.drizzlewhen no replica is configured. This gives the test read-after-write consistency without changing framework behavior. It mirrors how the shared reset helper already reads from the primary.Validation
postgres-read-replica. After the fix, the file passed 8 out of 8 runs onpostgres-read-replicaand stayed green on plainpostgresthrough the fallback.