test(server): deflake postgres teardown and pinned exposure port (#11667)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The server test suite gates every merge and every release cut. > - Three server tests each failed exactly once on markdown-only or unrelated diffs, then passed on rerun. > - One of the three (the git-operation-scheduler owner/joiner race) was fixed on master independently by [#11671](https://github.com/paperclipai/paperclip/pull/11671) while this PR was open, so after rebasing this pull request carries the remaining two. > - A flaky gate makes release operators rerun CI and stop trusting red results. > - Each remaining flake has a real nondeterminism: a teardown race and a hard-coded host port. > - This pull request removes the nondeterminism from the two tests without weakening what they prove. > - The benefit is a test gate that fails only when the product is broken. ## Linked Issues or Issue Description - [x] I searched open and closed issues and pull requests for these test files and for these failures. I found no duplicate report or fix. **What happened?** Three one-off CI failures occurred during release operations, each on a diff that could not have caused it, and each passed on rerun: 1. Run [32086355930](https://github.com/paperclipai/paperclip/actions/runs/32086355930): `server/src/__tests__/interaction-resolution-cross-issue-cap-postgres.test.ts` — all 7 tests passed, but vitest recorded an Unhandled Error and failed the run: `TypeError: Cannot read properties of null (reading 'write')` at `postgres@3.4.9/src/connection.js:255 Immediate.nextWrite`. 2. Run [32096743814](https://github.com/paperclipai/paperclip/actions/runs/32096743814): `server/src/services/workspace-git-operation-scheduler.test.ts` — the test "coalesces the same canonical key and cleans single-flight state after success and failure" failed with an AssertionError: the two concurrent calls came back with the `singleFlightJoined` values swapped. *(Fixed on master by [#11671](https://github.com/paperclipai/paperclip/pull/11671) with an equivalent single-flight barrier while this PR was open; the fix was dropped from this PR on rebase and the file is no longer touched here.)* 3. Run [32196201529](https://github.com/paperclipai/paperclip/actions/runs/32196201529): `server/src/services/workspace-runtime-exposure.test.ts` — the test "keeps an existing runtime port that is already inside the dedicated range" failed once out of 610 recorded runs because the runtime came back on a relocated port instead of the pinned 42500. **Expected behavior** The tests pass on every run when the code under test is correct. A red result means a product defect, not scheduling luck on the CI host. **Steps to reproduce** Each flake is a low-probability race, but both remaining mechanisms reproduce deterministically: 1. Postgres teardown: the suite never ends the postgres.js pool behind `createDb`; `afterAll` only stops the embedded server. postgres.js batches small writes and flushes them with `setImmediate` (`connection.js` `nextWrite`), and `close()` nulls the socket. Stop the server while the pool is open and a pending flush can run after the socket is gone. 2. Exposure pinned port: hold any loopback socket on 42500 or 52500 (both are inside the default Linux ephemeral port range, 32768–60999) and run the test. The allocator correctly relocates, and the assertion fails with `expected 42000 to be 42500`. The client side of any loopback connection on the CI host can land on those ports. **Paperclip version or commit** Branched from `master` at `4b968d8c0`; rebased onto `5a1ce7aed`. **Privacy checklist** I reviewed this description and removed private instance URLs, internal task identifiers, credentials, and user paths. ## What Changed Both fixes are test-side. I found no product race. - `interaction-resolution-cross-issue-cap-postgres.test.ts`: `afterAll` now ends the drizzle/postgres.js pool (`db.$client.end()`) before it stops the embedded Postgres server. `end()` waits for in-flight queries, including a fire-and-forget wake that lands just after a response, and closes the sockets from the client side first. Sibling suites (for example `heartbeat-plugin-environment.test.ts`) already use this order; this suite had skipped the pool shutdown. - `workspace-runtime-exposure.test.ts`: the pinned-port test no longer hard-codes 42500. It scans the dedicated range with the suite's real loopback probe, finds the lowest free app/HMR pair, then pins the next free pair strictly above it. If the keep-preferred-port path broke, the ascending fallback scan would return the lower pair, so the assertion keeps its discriminating power while no longer betting on one fixed host port staying free. - *(Dropped on rebase: the `workspace-git-operation-scheduler.test.ts` coalescing fix, superseded by the equivalent barrier merged in [#11671](https://github.com/paperclipai/paperclip/pull/11671).)* ## Verification - Reproduced the exposure flake exactly: with a listener held on `127.0.0.1:52500`, the pre-fix test fails with `expected 42000 to be 42500`; the fixed test passes with the port still held. - The postgres flake is a probabilistic teardown race and I could not trigger it on demand. The mechanism is established from `postgres@3.4.9` source (`setImmediate`-batched `nextWrite` versus `close()` nulling the socket) and the fix removes the whole class by closing the pool before the server. - Repeat runs after the fix: the pinned-port exposure test 20/20 green while the Postgres suite looped concurrently for loopback churn; `interaction-resolution-cross-issue-cap-postgres.test.ts` 15/15 green with no unhandled errors. - Re-verified after rebasing onto `5a1ce7aed`: both changed test files pass and `tsc --noEmit` passes in `server/`. - Environment note: three unrelated tests in `workspace-runtime-exposure.test.ts` (the wildcard-bind diagnosis tests) fail on macOS before and after this change because they read `/proc`; they are untouched and pass on Linux CI. ## Risks - Low risk: both changes are test-only; no product code changed. - The pinned-port test keeps a tiny time-of-check/time-of-use window between its own probe and the runtime's bind. The window shrinks from "one fixed port must stay free across the whole CI fleet" to milliseconds on a pair just verified free. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used Claude Fable 5 (Claude Code) — model ID `claude-fable-5`, with repository tools and local code execution for reproduction and repeat-run verification. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge
This commit is contained in:
parent
54b8bec444
commit
933749e01f
|
|
@ -67,6 +67,15 @@ describeEmbeddedPostgres("cross-issue interaction resolution cap (routes + postg
|
|||
});
|
||||
|
||||
afterAll(async () => {
|
||||
// End the postgres.js pool before stopping the embedded server. Stopping
|
||||
// the server first tears the TCP connection down under the still-open
|
||||
// client, and a batched write the driver scheduled via setImmediate can
|
||||
// then fire after the connection dropped its socket — an unhandled
|
||||
// `Cannot read properties of null (reading 'write')` that fails the run
|
||||
// even though every test passed. `end()` waits for in-flight queries
|
||||
// (including a fire-and-forget wake landing just after a response) and
|
||||
// closes the sockets from the client side first.
|
||||
await db.$client.end();
|
||||
await tempDb?.cleanup();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -5,6 +5,12 @@ import path from "node:path";
|
|||
|
||||
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest";
|
||||
|
||||
import {
|
||||
deriveViteHmrPort,
|
||||
RUNTIME_EXPOSURE_APP_PORT_MAX,
|
||||
RUNTIME_EXPOSURE_APP_PORT_MIN,
|
||||
} from "@paperclipai/shared";
|
||||
|
||||
import type { BrokerClient, BrokerListenerRequest } from "./runtime-exposure/broker-client.js";
|
||||
import {
|
||||
diagnoseRuntimeListenerBinds,
|
||||
|
|
@ -182,6 +188,25 @@ async function isLoopbackPortFree(port: number): Promise<boolean> {
|
|||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* The first app port at or above `startAt` whose HMR companion is also free on
|
||||
* the real host, mirroring the allocator's own scan. The pinned-port test needs
|
||||
* a port that is *verifiably* free right now rather than a hard-coded one: the
|
||||
* whole dedicated range sits inside the Linux ephemeral port range, so any
|
||||
* transient loopback socket on the host (the client side of a readiness probe,
|
||||
* a TIME_WAIT remnant from another suite) can occupy a fixed constant at probe
|
||||
* time and make the allocator relocate for a reason unrelated to the behaviour
|
||||
* under test.
|
||||
*/
|
||||
async function findFreeExposureAppPort(startAt: number): Promise<number> {
|
||||
for (let appPort = startAt; appPort <= RUNTIME_EXPOSURE_APP_PORT_MAX; appPort += 1) {
|
||||
if (await isLoopbackPortFree(appPort) && await isLoopbackPortFree(deriveViteHmrPort(appPort))) {
|
||||
return appPort;
|
||||
}
|
||||
}
|
||||
throw new Error("no free app/HMR port pair available in the dedicated runtime exposure range");
|
||||
}
|
||||
|
||||
function installDeps(overrides: {
|
||||
broker: BrokerClient;
|
||||
probeHealth?: () => Promise<boolean>;
|
||||
|
|
@ -316,14 +341,22 @@ describe("automatic tailscale_https default for managed worktree runtimes", () =
|
|||
const { broker } = createBroker();
|
||||
installDeps({ broker });
|
||||
|
||||
// Pin a pair that is free right now, strictly above the lowest free pair.
|
||||
// If the keep-the-preferred-port path were broken, the ascending fallback
|
||||
// scan would return that lower pair, so coming back with the pinned port
|
||||
// still proves the behaviour — without betting the test on one hard-coded
|
||||
// host port (formerly 42500) staying unoccupied for the whole run.
|
||||
const lowestFreeAppPort = await findFreeExposureAppPort(RUNTIME_EXPOSURE_APP_PORT_MIN);
|
||||
const pinnedPort = await findFreeExposureAppPort(lowestFreeAppPort + 1);
|
||||
|
||||
const [runtime] = await startRuntimeServicesForWorkspaceControl(startInput({
|
||||
serviceName: "paperclip-dev",
|
||||
expose: LEGACY_HTTP_EXPOSE,
|
||||
port: 42_500,
|
||||
port: pinnedPort,
|
||||
}));
|
||||
|
||||
expect(runtime.port).toBe(42_500);
|
||||
expect(runtime.url).toBe("https://runner.tail123.ts.net:42500");
|
||||
expect(runtime.port).toBe(pinnedPort);
|
||||
expect(runtime.url).toBe(`https://runner.tail123.ts.net:${pinnedPort}`);
|
||||
}, 15_000);
|
||||
|
||||
it("preserves a deliberate opt-out and leaves the service on plain HTTP", async () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue