Fix flaky CI: stop the gateway restore tests from poisoning the shared test database - #2844
Conversation
The restoreBackupEvent tests restored real backup fixtures over the worker's live SQLite file. Old fixtures carry schema drift (a t_session client_id column created by OAuth migrations later removed from the repo), and since no migration drops it, db.umzug.up() could not repair the schema: after the restore, the positional INSERT ... SELECT * of the fast snapshot reset failed with "table t_session has 15 columns but 14 values were supplied" for every test running after this file on the same worker. Whether tests run after it depends on how mocha --parallel distributes files across workers, hence the intermittent CI failures. Restore into a throwaway /tmp file instead, exactly like gateway.restoreBackup.test.js already does, and copy the shared config object returned by getConfig() instead of mutating it (the previous in-place storage override leaked to every other consumer of the config). Also make resetDb self-healing: when the snapshot copy fails because the live schema no longer matches the snapshot's, rebuild the seeded state and a fresh snapshot instead of failing the remainder of the worker. A regression test covers both drift directions (column added, column dropped). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xj8nWW9CWrNUvw5C6wd5yp
github-actions
Bot
added
area:server
📝 WalkthroughWalkthroughThe test database reset helper now rebuilds seeded snapshots after schema drift or snapshot-copy failures. Reset statistics and regression coverage track rebuilds. Gateway restore tests now use process-specific temporary SQLite files instead of shared test storage. ChangesDatabase reset recovery
Gateway restore test isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — this correctly fixes the intermittent t_session column-count CI flake.
Why this works
- Root cause is accurate:
gateway.restoreBackupEvent.test.jswas restoring fixture DBs (with leftovert_session.client_id) over the worker’s live SQLite file;db.umzug.up()cannot drop that column, so the positionalINSERT … SELECT *snapshot reset from #2802/#2818 then fails for every later test on that worker. - Restoring into a throwaway
/tmppath (matchinggateway.restoreBackup.test.js) stops the poison at the source. - Spreading
getConfig()before overridingstorageis the right fix —getConfig()returns the sharedconfig[env]object, so the previous in-place mutation leaked across the process. - The
resetDbrescue path + regression test are solid defense-in-depth for ADD/DROP-column drift;DETACHbefore rebuilding the snapshot file is necessary.
Not flagged
- Production / Gladys runtime untouched (test helpers only).
- No device categories/types.
- Not
risk:high, notneeds:human-review.
One soft limitation noted inline on the rebuild path (non-blocking).
Sent by Cursor Automation: Automatic PR review
| // The fast copy assumes the live schema still matches the snapshot's. | ||
| // A test that alters the schema (e.g. restoring a real backup over the | ||
| // database file) would otherwise fail every later test of this worker: | ||
| // rebuild the seeded state and a fresh snapshot instead of giving up. | ||
| logger.warn(`resetDb: snapshot reset failed (${e.message}), rebuilding the seed snapshot`); | ||
| resetDbStats.snapshotRebuilds += 1; | ||
| await rebuildSeededSnapshot(); |
There was a problem hiding this comment.
Soft note (non-blocking): this rescue re-seeds data then VACUUM INTOs whatever schema is currently live. Extra columns (the original backup-poison case) stay baked into the new snapshot — later resets succeed because column counts match again, but the schema is not returned to pristine migrations.
That’s fine here because the primary fix stops restore tests from touching the live DB; this path mainly covers in-process DDL drift like the new regression test. Worth keeping in mind if anything else starts mutating schema mid-suite.
|
🐳 A Docker image has been built for this branch and pushed to the GitHub Container Registry. You can test this pull request (AMD64 only) by pulling the image below: For example, run it with: sudo docker run -d \
--log-driver json-file \
--log-opt max-size=10m \
--cgroupns=host \
--restart=always \
--privileged \
--network=host \
--name gladys-claude-slow-unit-tests-ci-4j4nks \
-e NODE_ENV=production \
-e SERVER_PORT=80 \
-e TZ=Europe/Paris \
-e SQLITE_FILE_PATH=/var/lib/gladysassistant/gladys-production.db \
-v /var/run/docker.sock:/var/run/docker.sock \
-v /var/lib/gladysassistant:/var/lib/gladysassistant \
-v /dev:/dev \
-v /run/udev:/run/udev:ro \
ghcr.io/gladysassistant/gladys-preview:claude-slow-unit-tests-ci-4j4nksThis comment and the image are automatically updated on every new commit pushed to this pull request. Need an ARM64 image (Raspberry Pi, Apple Silicon, …)? Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2844 +/- ##
=======================================
Coverage 99.48% 99.48%
=======================================
Files 1219 1219
Lines 85399 85399
=======================================
Hits 84963 84963
Misses 436 436 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/test/helpers/db.test.js`:
- Around line 95-112: Update rebuildSeededSnapshot in
server/test/helpers/db.test.js to restore the canonical schema before running
seeders and initSnapshotReset, and assert that the first recovery removes
test_drift_column. Remove the later manual DROP COLUMN cleanup in
server/test/helpers/dbReset.test.js at lines 72-75, and add coverage for
recovering both added and missing columns.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d720b2d4-24f1-41d5-b47e-55cc155b37dd
📒 Files selected for processing (4)
server/test/helpers/db.test.jsserver/test/helpers/dbReset.test.jsserver/test/lib/gateway/gateway.restoreBackup.test.jsserver/test/lib/gateway/gateway.restoreBackupEvent.test.js
2 participants
Description
Fixes the intermittent CI failures seen since the test-speedup series (e.g. this run), where hundreds of tests failed in
beforeEachwith:Root cause.
gateway.restoreBackupEvent.test.jsrestores real backup fixtures over the worker's live SQLite database. The old fixture carries schema drift: at_session.client_idcolumn created by OAuth migrations that were later removed from the repo, so no migration drops it and thedb.umzug.up()in the test'safterEachcannot repair it. After that test, the livet_sessionhas 15 columns while a freshly migrated schema has 14. The fast snapshot reset introduced in #2818 rebuilds tables with a positionalINSERT INTO ... SELECT *, which then fails for every test running after this file on the same worker. Whether any tests run after it depends on howmocha --paralleldistributes files across workers — hence the flakiness, and why re-running the job usually goes green.Fix.
gateway.restoreBackupEvent.test.jsnow restores into a throwaway/tmpfile instead of the worker's live database — exactly whatgateway.restoreBackup.test.jswas already doing. Both files now copy the object returned bygetConfig()instead of mutating it in place (it is a shared object, so the previousgateway.config.storage = ...override leaked to every other consumer of the config). Thedb.umzug.up()+cleanDb()repair inafterEachbecomes unnecessary and is removed.resetDbis now self-healing. If the snapshot copy fails because the live schema no longer matches the snapshot's, it logs a warning, rebuilds the seeded state with the real seeders and takes a fresh snapshot, instead of failing the remainder of the worker. A regression test covers both drift directions (column added, column dropped).Reproduced deterministically before the fix (5/5 locally by running any test file after the gateway restore file in the same mocha process); after the fix the same scenario passes and the full suite shows zero
t_sessionerrors.Checklist
cd server && npm run coverage(Codecov requires 100% coverage on changed lines) and Cypress (npm run cypress:run) if the UI changednpm run eslint,npm run prettier)🤖 Generated with Claude Code
https://claude.ai/code/session_01Xj8nWW9CWrNUvw5C6wd5yp
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests