From decf83454b124d5195d7e0dec279418b56e40a9d Mon Sep 17 00:00:00 2001 From: Daniel Bersenkowitsch Date: Thu, 9 Jul 2026 21:43:18 +0200 Subject: [PATCH] test: de-flake the concurrency/blob-store specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../backup/scheduled-backup.service.spec.ts | 9 +++-- .../middleware/request-id.middleware.spec.ts | 31 ++++++++++++----- libs/nestjs/src/lib/s3/concurrency.spec.ts | 33 ++++++++++--------- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/libs/nestjs/src/lib/admin/backup/scheduled-backup.service.spec.ts b/libs/nestjs/src/lib/admin/backup/scheduled-backup.service.spec.ts index aca5293..7ddb8bb 100644 --- a/libs/nestjs/src/lib/admin/backup/scheduled-backup.service.spec.ts +++ b/libs/nestjs/src/lib/admin/backup/scheduled-backup.service.spec.ts @@ -217,8 +217,13 @@ describe('ScheduledBackupService', () => { const second = svc.runNowOrJoin(); expect(first).toEqual({ started: true }); expect(second).toEqual({ started: false }); - // let the in-flight cycle settle - await new Promise((r) => setTimeout(r, 50)); + // Deterministically JOIN the same in-flight cycle rather than sleeping a + // fixed wall-clock 50ms: runSnapshotCycle returns the live `inFlight` promise + // (never starts a second), so awaiting it waits for the fire-and-forget cycle + // to fully finish writing. The old setTimeout(50) let the cycle outlive the + // wait under CPU load, so afterEach's `fs.rm(dir)` raced the still-writing + // cycle and intermittently threw ENOTEMPTY (rmdir on a non-empty dir). + await svc.runSnapshotCycle('manual'); expect((backup.writeSnapshot as jest.Mock).mock.calls.length).toBe(1); }); diff --git a/libs/nestjs/src/lib/common/middleware/request-id.middleware.spec.ts b/libs/nestjs/src/lib/common/middleware/request-id.middleware.spec.ts index de9c786..4b1e677 100644 --- a/libs/nestjs/src/lib/common/middleware/request-id.middleware.spec.ts +++ b/libs/nestjs/src/lib/common/middleware/request-id.middleware.spec.ts @@ -1,4 +1,6 @@ -import express, { type Express, type Request, type Response } from 'express'; +import type { Server } from 'node:http'; + +import express, { type Request, type Response } from 'express'; import request from 'supertest'; import { RequestIdMiddleware } from './request-id.middleware'; @@ -10,19 +12,32 @@ const UUID_V7 = /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; describe('RequestIdMiddleware', () => { - let app: Express; + // One express app + one persistent ephemeral server for the whole suite, + // matching every other supertest spec in the lib (they all listen once in + // beforeAll). The middleware is stateless — each request still mints a fresh + // id — so there is no per-test state to reset. Passing a fresh `express()` + // app to supertest per test makes it stand up AND tear down a brand-new + // ephemeral server for every request; under full-suite CPU saturation that + // rapid listen/connect/close churn intermittently surfaced as a client-side + // "socket hang up", the sole source of this suite's flakiness (TEST-0006). + let server: Server; - beforeEach(() => { + beforeAll((done) => { const mw = new RequestIdMiddleware(); - app = express(); + const app = express(); app.use((req: Request, res: Response, next) => mw.use(req, res, next)); app.get('/probe', (req: Request, res: Response) => { res.json({ openbucket: req.openbucket }); }); + server = app.listen(0, done); + }); + + afterAll((done) => { + server.close(done); }); it('case 1: mints a UUIDv7 and sets both response headers', async () => { - const res = await request(app).get('/probe'); + const res = await request(server).get('/probe'); expect(res.status).toBe(200); const id = res.body.openbucket.requestId; @@ -33,21 +48,21 @@ describe('RequestIdMiddleware', () => { it('case 2: reuses a syntactically valid upstream X-Request-Id', async () => { const upstream = '0190d9c1-7f32-7c0c-bea5-1f51d1c0b2c4'; - const res = await request(app).get('/probe').set('X-Request-Id', upstream); + const res = await request(server).get('/probe').set('X-Request-Id', upstream); expect(res.body.openbucket.requestId).toBe(upstream); expect(res.headers['x-request-id']).toBe(upstream); }); it('case 3: discards a malformed upstream X-Request-Id and mints fresh', async () => { - const res = await request(app).get('/probe').set('X-Request-Id', 'not-a-uuid'); + const res = await request(server).get('/probe').set('X-Request-Id', 'not-a-uuid'); expect(res.body.openbucket.requestId).not.toBe('not-a-uuid'); expect(res.body.openbucket.requestId).toMatch(UUID_V7); }); it('case 4: initializes the placeholder context (kind=s3, receivedAt=0)', async () => { - const res = await request(app).get('/probe'); + const res = await request(server).get('/probe'); expect(res.body.openbucket.kind).toBe('s3'); expect(res.body.openbucket.receivedAt).toBe(0); diff --git a/libs/nestjs/src/lib/s3/concurrency.spec.ts b/libs/nestjs/src/lib/s3/concurrency.spec.ts index 0b10365..4d6a522 100644 --- a/libs/nestjs/src/lib/s3/concurrency.spec.ts +++ b/libs/nestjs/src/lib/s3/concurrency.spec.ts @@ -93,20 +93,23 @@ describe('concurrency invariants (TEST-0317)', () => { await fs.rm(dataDir, { recursive: true, force: true }); }); - // QUARANTINED (flaky) — these two cases assert concurrency invariants the write - // path does not yet guarantee for writers racing on the SAME target, so they - // fail intermittently / platform-dependently and destabilise CI: - // • same-partNumber UploadPart: both writers rename(2) onto the same `.part`. - // POSIX overwrites atomically (last-wins), but Windows rejects rename-over- - // existing, so the call rejects on a dev box. - // • concurrent first-time same-key PUT: the writer renames the blob BEFORE it - // commits the row; if the losing writer's row commit conflicts, its rollback - // unlinks the shared final blob and tears the winner's result. - // Re-enable after hardening concurrent same-target writes (e.g. per-(bucket,key) - // serialization in ObjectWriterService + rename-over-existing tolerance in - // BlobStore.atomicRename). The deterministic sequential case below stays active. - // Follow-up: harden concurrent same-target writes (see s3/CONCURRENCY.md §4.8). - it.skip('same-partNumber concurrent UploadPart does not throw EEXIST; the part is one whole writer', async () => { + // These two cases assert concurrency invariants for writers racing on the SAME + // target. Both were quarantined (it.skip) while the write path could tear under + // that race; the hardening they waited on has since landed, so they are now + // active and deterministic: + // • same-partNumber UploadPart: each writer stages to a randomUUID-suffixed + // tmp file (no O_EXCL collision) and rename(2)s onto the shared `.part`. + // On POSIX (Linux CI, macOS dev) rename-over-existing is atomic last-wins, + // so the final part is exactly one whole writer's payload — no tear, no + // EEXIST. (Windows rename-over is the only remaining platform caveat; the + // CI runner is Linux.) + // • concurrent first-time same-key PUT: ObjectWriterService now serializes + // writers of the same (bucket,key) through a keyed async mutex + // (`withKeyLock`, F6, commit c87ef90), so the two PUTs run strictly one + // after the other — the loser's rollback can no longer unlink the winner's + // committed blob. Row, blob bytes, and ETag all agree on one winner. + // See s3/CONCURRENCY.md §4.8. + it('same-partNumber concurrent UploadPart does not throw EEXIST; the part is one whole writer', async () => { const uploadId = 'concurrent-upload'; const a = 'A'.repeat(4096); const b = 'B'.repeat(8192); @@ -136,7 +139,7 @@ describe('concurrency invariants (TEST-0317)', () => { expect((await fs.readFile(blobs.paths.blobPath('b', 'seq'))).toString()).toBe('second-wins'); }); - it.skip('concurrent PUT same key: SQLite serializes the writers; row + blob agree on one winner', async () => { + it('concurrent PUT same key: the per-key write lock serializes the writers; row + blob agree on one winner', async () => { const x = 'X'.repeat(500); const y = 'Y'.repeat(700);