(motir-core) The GitLab concurrent-redelivery test asserts an outcome its OWN comment says is timing-dependent — `is idempotent under concurrent redelivery` flakes red on a loaded CI runner

Repo: motir-core. One PR. Surfaced as a CI flake on an unrelated PR (#1817 / MOTIR-2070, a planning-canvas diff with zero GitLab files). Logged per the flake rule and filed rather than patched in that PR (a drive-by fix gets its own card AND its own PR).

Observed

Vitest (1/3) red with a single failure out of 3440:

FAIL tests/gitlab/gitlabWebhookEdges.test.ts
  > gitlabWebhookService — concurrent redelivery + degenerate states (MOTIR-1479)
  > is idempotent under concurrent redelivery of the same MR (race-safe)
AssertionError: expected false to be true
  at tests/gitlab/gitlabWebhookEdges.test.ts:307:81

Root cause — the test contradicts itself (verified against origin/main)

The test fires two handleEvent calls through Promise.all and then asserts, at tests/gitlab/gitlabWebhookEdges.test.ts:307:

expect([a, b].every((r) => 'outcome' in r && r.outcome === 'transitioned')).toBe(true);

Its own comment, three lines above, states the opposite:

"Both callers race; depending on timing, both may return 'transitioned' (the second reads the item status before the first's write commits) or one may return 'noop' (the commit finished first). Either way the item ends up at in_review and there is exactly one MR row — idempotent under race."

And the service agrees with the comment, not the assertion — lib/services/changeRequestStatusSync.ts (~line 236) short-circuits:

// Idempotent: already in the target (a redelivery) — updateStatus no-ops, but
// short-circuit so the outcome reads `noop` rather than `transitioned`.
if (resolved.currentStatus === targetKey)
  return { event: 'pull_request', outcome: 'noop', ... };

So the second handler returns noop exactly when the first transaction commits before the second's status read — a legal, documented interleaving. On an idle box both reads land before either write (green); on a loaded CI runner the interleaving flips and the assertion fails. This is not network noise and not a service defect: it is an assertion strictly narrower than the contract it was written to describe — the same family as a test pinned to a wall-clock boundary.

The two assertions that actually encode idempotency are correct and hold under both interleavings:

• expect(await statusOf(s.item.id)).toBe('in_review') — the end state is the same either way.
• expect(mrRows).toHaveLength(1) — the upsert retried on P2002 and survived the race.

Fix

Assert the outcome is one of the two legal values and keep the two real invariants — the point of the test is idempotence under race, which neither weakens:

expect([a, b].every((r) => 'outcome' in r && (r.outcome === 'transitioned' || r.outcome === 'noop'))).toBe(true);

While in the file, check the sibling race/concurrency cases for the same shape (an assertion pinned to one interleaving of several the code permits) rather than patching this one line blind.

Acceptance criteria

• The test accepts transitioned or noop from each of the two racing callers, and still asserts the final status is in_review and that exactly one MR row exists.
• A change that broke real idempotence still fails it — e.g. mutate the service to skip the noop short-circuit's status check, or to drop the P2002 upsert retry, and confirm the test goes red.
• Any sibling assertion in the same file pinned to one legal interleaving is fixed in the same pass, or explicitly reported as not applicable.
• No production code changes: the service's behaviour is correct as shipped.

Context refs

• tests/gitlab/gitlabWebhookEdges.test.ts:307 — the over-strict assertion and the comment it contradicts.
• lib/services/changeRequestStatusSync.ts — the noop short-circuit that makes the second outcome timing-dependent.
• Flake occurrence: PR #1817 run 30905328633, job 91978967038 (Vitest (1/3), 1 failed / 3439 passed; shards 2 and 3 green; main green on the two preceding commits; the file passes 3/3 locally on that branch).