Skip to content

test(node): Retry initial tedious connection until the DB is ready#22050

Merged
mydea merged 1 commit into
developfrom
test/fix-flaky-tedious-integration-test
Jul 8, 2026
Merged

test(node): Retry initial tedious connection until the DB is ready#22050
mydea merged 1 commit into
developfrom
test/fix-flaky-tedious-integration-test

Conversation

@mydea

@mydea mydea commented Jul 8, 2026

Copy link
Copy Markdown
Member

The tedious node-integration-test flakes on CI for the same reason the postgres suites did: it opens its DB connection before the database is actually reachable.

Root cause

docker compose up -d --wait blocks only on the container's internal healthcheck. On busy CI the host-side port forward (1433) can lag behind, and MSSQL is especially slow to start accepting connections even after the container reports healthy (the compose file uses start_period: 20s). The scenario called await connect() at the top of run(), before Sentry.startSpan(...) — so a failed initial connect rejected run() outright, no transaction was ever sent, and the test timed out.

Fix

Wrap the initial connect in a small retry loop (connectWithRetry) that keeps trying with a fresh Connection per attempt until the DB accepts it or a 60s deadline elapses. connect() now also closes the failed connection before rejecting so retries don't leak sockets. This is the tedious analogue of the readiness gate the postgres/postgresjs suites use.

Verified locally against the MSSQL container: the suite passes in both ESM and CJS.

Fixes #21620
Fixes #21765
Fixes #21766

🤖 Generated with Claude Code

The `tedious` scenario called `await connect()` immediately after
`docker compose up --wait`, which only blocks on the container's internal
healthcheck. On busy CI the host port-forward can lag behind, and MSSQL is
especially slow to start accepting connections (note the compose file's
`start_period: 20s`). A failed initial connect rejected `run()` before the span
was opened, so no transaction was sent and the test flaked.

Wrap the initial connect in a retry loop that keeps trying (with a fresh
Connection per attempt) until the DB accepts it or a deadline elapses.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mydea mydea requested a review from a team as a code owner July 8, 2026 08:28
@mydea mydea requested review from JPeer264 and andreiborza and removed request for a team July 8, 2026 08:28
if (err) {
connection.close();
reject(err);
} else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Calling connection.close() in the connect event's error handler is risky, as it may trigger an unhandled error and crash the process if the connection state is unstable.
Severity: MEDIUM

Suggested Fix

Remove the connection.close() call from the 'connect' event's error handler. The existing retry logic should be sufficient to handle connection failures by attempting to create a new connection, which is a safer and more consistent pattern.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: dev-packages/node-integration-tests/suites/tracing/tedious/scenario.mjs#L31

Potential issue: In the `tedious` integration test, `connection.close()` is called
within the error callback of the `connect` event. This pattern is known to cause state
machine issues in some versions of the library. If `close()` is called while the
connection is in a transitional state, it may emit its own error. Since there is no
`connection.on('error', ...)` listener attached, this would become an unhandled error
event, causing the Node.js process to crash. This could lead to intermittent test
failures in the CI environment.

Did we get this right? 👍 / 👎 to inform future reviews.

@JPeer264 JPeer264 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mydea mydea merged commit dee4874 into develop Jul 8, 2026
57 checks passed
@mydea mydea deleted the test/fix-flaky-tedious-integration-test branch July 8, 2026 09:00
mydea added a commit that referenced this pull request Jul 8, 2026
…tForConnection helper (#22058)

Follow-up to #22045 (postgres) and #22050 (tedious): extracts the
DB-readiness workaround into a single reusable helper and applies it
consistently across every Docker-backed DB integration suite, including
the mysql family that was still flaking.

### Root cause (recap)

`docker compose up -d --wait` only gates on the container's internal
healthcheck, but databases keep finalizing for a short window
afterwards: the host port-forward can lag (`ECONNREFUSED`/`ECONNRESET`),
MySQL drops early handshakes (*"server closed the connection"*), and
MSSQL is slow to start accepting connections. A scenario whose first
connection lands in that window fails before it opens its span, so no
transaction is sent and the test times out. A pure TCP probe is **not**
enough — the port accepts the socket while the handshake is still
refused — so the gate must perform a real driver connection.

### What changed

- Add `waitForConnection(probe)` to the test-utils package
(`dev-packages/node-integration-tests/src/index.ts`): a driver-agnostic
retry loop that keeps running a probe until it succeeds or a deadline
elapses. Each scenario passes a tiny connect-only probe using its own
driver, run before the span is opened (so the connect is uninstrumented
and emits no spans).
- Because scenarios already import from the workspace package, **no
`copyPaths` is needed** — this removes the two per-suite
`wait-for-postgres.js` helpers and all the `copyPaths` wiring that
#22045 introduced, and drops the bespoke retry loops in the tedious and
mysql2-tracing-channel scenarios.
- Applied consistently to: **postgres**, **postgres-streamed**,
**tedious**, **mysql2**, **knex (mysql2)**, and
**mysql2-tracing-channel**.

Net: **+339 / −379**, three ad-hoc styles collapsed into one helper.

All six suites verified locally (ESM + CJS, OTel + orchestrion paths).

Note: the `mysql` suite uses an in-process test server (no Docker), so
its flakes (#21854, #21808, #21767) have a different root cause and are
intentionally out of scope here.

Fixes #21947
Fixes #21730
Fixes #21725
Fixes #21695
Fixes #21961
Fixes #21948
Fixes #21694
Fixes #22044

🤖 Generated with [Claude Code](https://claude.com/claude-code)

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