Skip to content

test: de-flake the concurrency/blob-store specs - #59

Merged
ProjectBay merged 1 commit into
mainfrom
fix/deflake-concurrency-specs
Jul 9, 2026
Merged

test: de-flake the concurrency/blob-store specs#59
ProjectBay merged 1 commit into
mainfrom
fix/deflake-concurrency-specs

Conversation

@ProjectBay

Copy link
Copy Markdown
Owner

Why

release-nestjs.yml deliberately skips the @openbucket/nestjs unit suite because it was "known-flaky (race, not parallelism)". That is a 1.0 blocker: the release we tag 1.0 should pass its own full suite. This PR finds, diagnoses, and fixes the flakiness at the root so the suite is deterministic and the release gate can stop skipping it.

The flakiness only reproduces under full-suite CPU saturation (many Jest workers). Looping the whole suite before this PR failed ~2/12 runs; after, 60/60 full-suite runs are green (two rounds of 30).

Root causes & fixes (all test-only — no production code changed)

1. s3/concurrency.spec.ts — two invariants were quarantined (it.skip) but their prerequisite fix has since landed

The two same-target concurrency cases were skipped in Jun 2026 (commit a8f185b) because the write path could tear under a same-key/same-part race. The hardening they explicitly waited for — per-(bucket,key) write serialization (ObjectWriterService.withKeyLock, F6) — landed afterwards in c87ef90 (Jul 2026), but the tests were never re-enabled.

  • concurrent same-key PUT: withKeyLock now serializes writers of the same (bucket,key) through a keyed async mutex, so the loser's rollback can no longer unlink the winner's committed blob. Row, blob bytes, and ETag agree on one winner.
  • same-partNumber concurrent UploadPart: each writer stages to a randomUUID-suffixed tmp file (no O_EXCL collision) and rename(2)s onto the shared <n>.part; on POSIX (Linux CI, macOS dev) that is atomic last-wins, so the final part is one whole writer's payload.

Fix: un-skip both cases and refresh the stale quarantine comment. They now pass 30/30 in isolation and in the full suite.

2. common/middleware/request-id.middleware.spec.ts — "socket hang up" under load

This was the only supertest spec that built its Express app in beforeEach and called request(app) per test, which makes supertest stand up and tear down a brand-new ephemeral server for every request (4× per run). Under full-suite CPU saturation that rapid listen/connect/close churn intermittently surfaced as a client-side socket hang up — the failing case varied run to run (case 1, case 4…), the tell-tale of a transport-level flake rather than a logic bug.

Fix: adopt the pattern every other supertest spec in the lib already uses — build the app and one persistent listening server once in beforeAll, reuse request(server), close it in afterAll. The middleware is stateless, so each request still mints a fresh id; every assertion is unchanged.

3. admin/backup/scheduled-backup.service.spec.tsENOTEMPTY during cleanup

The "run-now joins an in-flight cycle" test fires runNowOrJoin() (a fire-and-forget async snapshot cycle) and then waited a fixed wall-clock setTimeout(50ms) before asserting. Under load the cycle outlived the sleep and was still writing files into the per-test temp dir when afterEach's fs.rm(dir, { recursive }) ran — the rmdir then hit a dir that a concurrent write had just repopulated → ENOTEMPTY: directory not empty.

Fix: replace the sleep with a deterministic joinawait svc.runSnapshotCycle('manual') returns the same live inFlight promise (never starts a second cycle), so we wait for the fire-and-forget cycle to fully finish writing before cleanup. No timing assumption; the writeSnapshot-called-once assertion is unchanged.

Evidence — 25×+ green

Round 1 (concurrency un-skip + request-id fix):   PASS=29 FAIL=1/30
  → the 1 failure was scheduled-backup ENOTEMPTY (a third, distinct flake), fixed in this PR
Round 2 (all three fixes):                        PASS=30 FAIL=0/30
s3/concurrency.spec.ts, isolated:                 30/30 green

npx nx lint nestjs → 0 errors. npx nx test nestjs → all suites pass (2 previously-skipped concurrency cases now run).

Follow-up

With the suite now deterministic, release-nestjs.yml can drop the "publish gate deliberately does NOT re-run the unit suite" carve-out and gate the release on the full nx test nestjs. Intentionally left out of this PR to keep it test-only and reviewable.

🤖 Generated with Claude Code

The @openbucket/nestjs unit suite was skipped by release-nestjs.yml because
it was known-flaky under full-suite parallelism. Fix the three root causes so
the suite is deterministic (60/60 full-suite runs green):

1. s3/concurrency.spec.ts — un-skip the two same-target concurrency invariants.
   They were quarantined (a8f185b) before their prerequisite landed: per-
   (bucket,key) write serialization (ObjectWriterService.withKeyLock, F6) shipped
   in c87ef90 but the tests were never re-enabled. With the lock in place the
   concurrent same-key PUT and same-partNumber UploadPart cases are deterministic.

2. common/middleware/request-id.middleware.spec.ts — the only supertest spec that
   built its Express app in beforeEach and called request(app) per test, standing
   up + tearing down a fresh ephemeral server for every request. Under CPU
   saturation that churn intermittently surfaced as a client-side "socket hang up".
   Build one persistent server in beforeAll (as every other supertest spec does).

3. admin/backup/scheduled-backup.service.spec.ts — the run-now test waited a fixed
   setTimeout(50ms) for a fire-and-forget cycle, then afterEach fs.rm raced the
   still-writing cycle -> ENOTEMPTY. Await the shared in-flight promise via
   runSnapshotCycle('manual') instead of sleeping.

All changes are test-only; no production code changed. release-nestjs.yml can now
drop its unit-suite skip (left for a follow-up).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ProjectBay
ProjectBay merged commit a006617 into main Jul 9, 2026
6 checks passed
@ProjectBay
ProjectBay deleted the fix/deflake-concurrency-specs branch July 9, 2026 19:54
ProjectBay added a commit that referenced this pull request Jul 9, 2026
The release workflow used to skip the unit suite because a few specs were flaky;
those were de-flaked (#59), so the publish gate now runs `nx test nestjs` — the
tag you publish must pass its own tests.

Co-authored-by: Daniel Bersenkowitsch <accounts@projectbay.io>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant