From 1e07d5b9aae43ef58444604683099a8fc7185321 Mon Sep 17 00:00:00 2001 From: Tonio Date: Wed, 12 Aug 2026 21:30:50 -0700 Subject: [PATCH] fix(db): give the last two embedded-Postgres migration tests a timeout (#11313) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The `@paperclipai/db` package owns the database schema and its migrations > - Some migration tests start an embedded Postgres server and replay a migration against it > - An embedded Postgres server needs 7 to 12 seconds to start on a CI runner > - Vitest stops a test after 5 seconds unless the test sets its own timeout > - Two of these tests do not set a timeout, so they fail on CI before they assert anything > - This pull request gives both tests a 30 second timeout > - The benefit is that unrelated pull requests stop failing on a test they did not change ## Linked Issues or Issue Description No public issue exists for this. The problem follows. **What happened?** The test `packages/db/src/company-secret-proposals-migration.test.ts` fails on CI. The error is `Test timed out in 5000ms`. The test never reaches its assertions. The suite reports `1 failed | 104 passed`. The failure is not caused by the branch under test. It appeared on three different branches in a few hours: | Run | Head | Failing jobs | | --- | --- | --- | | 31630781317 | `95622fa3` | `General tests (workspaces-b)`, `verify`, `e2e shard (2/3)`, `e2e` | | 31652020976 | `feba90c9` | `General tests (workspaces-b)`, `verify`, `e2e shard (3/3)`, `e2e` | | 31651467721 | `f2115207` | `General tests (workspaces-a (1/2))`, `verify` | The `verify` job reads the result of the general tests. One timeout therefore turns into two red checks. A reviewer sees two failures and reads them as a regression. **Expected behavior** The test starts an embedded Postgres server, replays the migration, and asserts the schema. It must pass on a normal CI runner. **Steps to reproduce** 1. Open any pull request against `master`. 2. Wait for the job `General tests (workspaces-b)`. 3. Read the failure. The test times out after 5000 ms. The failure needs a slow runner. A fast development machine starts embedded Postgres in less than 5 seconds, so the test passes there. **Paperclip version or commit** `master` at `a09d7dcc0`. **Deployment mode** CI only. GitHub Actions, `ubuntu24` runner image. ## What Changed - `packages/db/src/company-secret-proposals-migration.test.ts` — the test now uses a 30 second timeout. The migration suites in this package already use 20 to 60 seconds. 30 seconds is the most common value. - `packages/db/src/status-card-migrations.test.ts` — the same change. This test has the same defect. It does not fail yet because it replays fewer statements. A fix to only one test moves the problem instead of removing it. - Both tests get a comment. The comment tells the next author why the 5 second default is too short. These two tests were the only embedded-Postgres migration tests in the package without a timeout. ## Verification - Run `pnpm vitest run src/company-secret-proposals-migration.test.ts src/status-card-migrations.test.ts` in `packages/db`. Both tests pass. - These suites skip themselves when the Postgres binaries are absent. A pass alone therefore proves nothing. Run the command with `--reporter=verbose`. The output contains Postgres `NOTICE` messages, for example `relation "status_cards" already exists, skipping`. These messages prove the tests ran real SQL. - Run the same command with `--testTimeout=1`. Both tests still pass. This proves the per-test timeout overrides the global timeout. Before this change, the same command fails immediately. - All CI jobs on this pull request pass. The job `General tests (workspaces-b)` passes. This job failed on the three runs listed above. Not done: no attempt to reproduce the timeout on a development machine. A fast machine starts embedded Postgres in less than 5 seconds, so the failure does not occur there. ## Risks Low risk. The change adds two timeout arguments to tests. It changes no source code, no schema, and no dependency. A longer timeout cannot hide a regression here. The tests assert the same conditions as before. A migration that truly hangs now fails after 30 seconds. Before, it failed after 5 seconds with a message that pointed at the wrong cause. The `e2e` failures on the runs above have a different cause. The spec `mcp-user-stories.spec.ts › US-9` fails with `502 — fetch failed` and `fetch failed: bad port`. These errors come from MCP tool-connection health checks. The failures hit different shards on different runs. This pull request does not change that behavior. `e2e shard (2/3)` passes here, which supports the view that those failures are unstable infrastructure. To revert, remove the two timeout arguments. ## Model Used Claude Opus 5 (`claude-opus-5`), through Claude Code. Extended thinking enabled. Tool use enabled: file read and edit, shell command execution for the local test runs, and the GitHub CLI to read the failing CI logs. ## 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 - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 5 Co-authored-by: Paperclip --- packages/db/src/company-secret-proposals-migration.test.ts | 7 ++++++- packages/db/src/status-card-migrations.test.ts | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/db/src/company-secret-proposals-migration.test.ts b/packages/db/src/company-secret-proposals-migration.test.ts index 52d9a96a4a..b358551ccc 100644 --- a/packages/db/src/company-secret-proposals-migration.test.ts +++ b/packages/db/src/company-secret-proposals-migration.test.ts @@ -24,6 +24,11 @@ if (!embeddedPostgresSupport.supported) { describeEmbeddedPostgres("company secret proposals migration", () => { afterEach(async () => Promise.all(cleanups.splice(0).map((cleanup) => cleanup()))); + // Starting an embedded Postgres and replaying a migration against it does not + // fit vitest's 5s default: the neighbouring replay suites measure 7-12s on + // CI. Every other embedded-Postgres migration test in this package carries an + // explicit timeout for that reason; this one did not, so it failed on any + // runner that was not unusually fast. it("can be reapplied after its migration journal entry is removed", async () => { const database = await startEmbeddedPostgresTestDatabase("paperclip-secret-proposals-migration-"); cleanups.push(database.cleanup); @@ -40,5 +45,5 @@ describeEmbeddedPostgres("company secret proposals migration", () => { (SELECT count(*)::int FROM pg_indexes WHERE tablename = 'company_secret_proposals') AS indexes `; expect(result).toEqual({ constraints: 13, indexes: 5 }); - }); + }, 30_000); }); diff --git a/packages/db/src/status-card-migrations.test.ts b/packages/db/src/status-card-migrations.test.ts index 87b0f5072e..6e049d2a70 100644 --- a/packages/db/src/status-card-migrations.test.ts +++ b/packages/db/src/status-card-migrations.test.ts @@ -22,6 +22,10 @@ describeEmbeddedPostgres("status card migrations", () => { await Promise.all(cleanups.splice(0).map((cleanup) => cleanup())); }); + // Same reason as every other embedded-Postgres migration test here: starting + // the server and replaying migrations against it does not fit vitest's 5s + // default. This suite has not tripped yet only because it replays fewer + // statements than its neighbours — it is the same latent failure. it("can be reapplied after the schema already exists", async () => { const database = await startEmbeddedPostgresTestDatabase("paperclip-status-card-migrations-"); cleanups.push(database.cleanup); @@ -35,5 +39,5 @@ describeEmbeddedPostgres("status card migrations", () => { ); await sql.unsafe(migrationSql); } - }); + }, 30_000); });