Skip to content

test(helpers): share one bounded waiter and Postgres lock-wait probe across tests - #3165

Merged
freeqaz merged 4 commits into
mainfrom
test-dry-1022
Oct 22, 2026
Merged

freeqaz merged 4 commits into
mainfrom
test-dry-1022

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Integration and conformance tests copy the same polling code into each file: const deadline = Date.now() + N; while (...) { assert.ok(Date.now() < deadline, ...); await delay(M); }. A survey of tests/integration/ and tests/conformance/ found 66 hand-rolled wait loops. Three of them (while (!release) await delay(10)) had no deadline at all. Lock-order tests also copied the same pg_blocking_pids probe ("wait until a backend waits on this lock") into 8 files, 11 times. tests/helpers/wait-for.mjs already had a bounded waitFor, but its fixed 20 ms interval and string-only description kept most callers hand-rolling their own loops.

Survey top (copies x lines, of what this PR addresses):

Cluster Copies Status
Hand-rolled deadline/poll loops 66 loops in 40 files 33 migrated here; 33 left (real Docker/k3d files, files held for the Credential Proxy stack, ci-runner.test.mjs, workspace-node-supervisor which runs inside the runtime image, intentional fixed-duration probes)
pg_blocking_pids lock-wait probes 11 in 8 files + 1 helper all outside held files migrated

The full survey, including clusters left for later, is kept outside the repository.

Change

  • tests/helpers/wait-for.mjs: waitFor(description, read, timeoutMs | { timeoutMs, intervalMs }). The description can be a function that runs at the deadline, so a timeout reports the state still wrong (backend pids, child output). New waitUntil for boolean checks. Existing callers are unchanged.
  • New tests/helpers/postgres-lock-waits.mjs: waitForLockWaiter(pool, holderPid, description, { count, queryLike, ...}) and waitForLockHolder(pool, waiterPid, description, ...). postgres-namespace-lock.mjs now uses it.
  • 33 loops in 21 test files moved onto these helpers, keeping each site's timeout, poll interval, early exits (child exited, ps errors) and cleanup on timeout (the held provider responses in repository-credentials-descriptions). The three unbounded while (!release) loops are now bounded.
  • tests/conformance/workspace-node-supervisor.test.mjs stays self-contained: runtime-image-native-worker copies it into the runtime image with only available-port.mjs, so it cannot import the waiter (its first migration failed Image Runtime Startup 3 on 1cbfe12 and was reverted in fc85bdf).
  • 24 files, net -16 lines. Test-only; no product code changes.

Verification

All runs on a remote test VM, on f4b810c unless noted. 1cbfe12 adds only review nits, fc85bdf restores workspace-node-supervisor, and c298ff5 merges main:

  • CI lanes static, checks-baseline-1, checks-baseline-2, postgres-auth, postgres-platform and postgres-application all green.
  • Wall time of the touched test files, one lane run each: main 5bd29fc 367 s, this branch 354 s (both including workspace-node-supervisor, 14.5 s and 14.3 s). Per file within noise, none slower.
  • Mutants planted in the code under test, run against main and against this branch. The kill sets are identical; each mutant was killed by the same test cases:
    • M1 credential service reports uncertain: 0: 3 cases in repository-credentials-service-loss.
    • M2 metadata lookups never leave pending: 8 cases in repository-credentials-descriptions.
    • M3 catalog reader never escalates to SIGKILL: plugin-driver "kills a Codex process that ignores SIGTERM after an abort".
    • M4 runtime supervisor signals the child, not its group: workspace-node-supervisor "retire their process groups". That file was later restored to main's version.
    • M5 state-lock tickets renamed: ci-prepare "serves waiters in arrival order".
    • M7 native-admin denial audit dropped: native-admin-upgrade-reset "still records the denial", and native-admin-session-postgres "closes at the next renewal after IAM administer restriction".
    • M10 lockNamespace without FOR UPDATE: 3 postgres-agent-provisioning Stop/handoff lock-order cases.
    • M11 findServiceAccount without its row lock: the service-account-deletion forced-delete case and the service-account-issuance-recovery reclaim case.
    • M10 survived in production-wireup and password-default-activation on both heads; their waits block on a different lock.
  • Pins at 2 CPUs: 20/20 for the 11 checks-lane files on f4b810c, and 20/20 for the three files the nits touched on 1cbfe12. Postgres files pinned through their lanes, restricted to the touched files, 20 runs each: postgres-auth 20/20, postgres-application 20/20, postgres-platform 20/20.
  • eslint --max-warnings 0 and prettier on the touched files.

Open PRs touching these files (#115, #906, #1557, #1707, #2258, #2259, #2268, #2398, #2445, #2633, #2982): git merge-tree against this head gives the same conflicted files as against main. No new conflicts.

Risks

Low; test-only. Failure messages change wording ("Timed out waiting for ."). A few deadlines changed shape. Two descriptions waits that shared one 5 s deadline now get 5 s each. A 300 x 10 ms attempt loop is now a 5 s deadline. One workspace-node-supervisor retry wait polls every 25 ms instead of 50 ms. Real Docker/k3d files were not migrated because they could not be proven here. They are listed in the survey for a follow-up, along with a retryErrors option their waitFor copies would need.

…iter

Extend tests/helpers/wait-for.mjs with an options form ({ timeoutMs, intervalMs }),
a lazily built description and waitUntil for boolean checks, and add
tests/helpers/postgres-lock-waits.mjs for the pg_blocking_pids probes that
lock-order tests copied into each file. Migrate the hand-rolled deadline loops
in checks and Postgres test files onto them; three unbounded 'while (!release)'
loops become bounded.
Treat an empty pid file as not yet written in plugin-driver, word the
processGone description as the awaited outcome, document why the lock-wait
helper needs no wait_event_type filter and which waiter it returns, accept a
null options argument, and keep helper imports after the app imports.
@freeqaz
freeqaz requested a review from a team as a code owner October 22, 2026 02:29
@clawsweeper

clawsweeper Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

runtime-image-native-worker copies this file and available-port.mjs into the
runtime image and runs it there, so it cannot import the shared waiter without
that test also shipping wait-for.mjs (Image Runtime Startup 3 failed on
1cbfe12). Restore the file; its four loops stay listed in the survey.
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