mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-22 23:47:28 +00:00
f91f958a6c
* fix(supersync): retry lock-bounded migrations by shape, not by name A production deploy on 2026-07-21 failed twice on the fastupdate migration with P3018/55P03: ALTER INDEX ... SET (...) needs ACCESS EXCLUSIVE on the index, and the planner takes AccessShareLock on every index of a table for any query planning against it, held to end of transaction — so a single slow query on `operations` starves the deliberate 1s window. One deploy burned both its allowed attempts; the next won only on its second. One retry is a coin flip. - Gate the native retry on SHAPE (SET LOCAL lock_timeout, then a single ALTER INDEX ... SET (...), no third statement) instead of the literal SQL text with a hardcoded index name. Restores the script's documented "hardcodes no migration names" invariant, which name-hardcoding had already broken a production deploy over once. - Raise the budget from 1 retry to 10 attempts, 5s apart (~70s worst case, far below both the step timeout and MIGRATION_TIMEOUT). The cap is a literal, not an env knob: a non-numeric value would make the `-ge` test error inside an `if`, which `set -e` does not catch, and the Helm initContainer path has no outer timeout at all. - Do NOT widen the gate to "any 55P03". A single-statement migration gets no implicit transaction, so a lock timeout during a bare CREATE INDEX CONCURRENTLY leaves an INVALID index unrolled-back, and a generic branch would short-circuit that deliberate fail-loud path (the 2026-05-18 missing-entity-index outage, #7654). Tests: replace the three fastupdate-specific refusal cases (two of which now correctly retry) with cases isolating each gate condition, and add coverage for a differently-named index and for more than one retry. Every new assertion was sabotage-verified to fail without its guard. Also fix two tests that could not fail: the migration-ordering tautology (compared two adjacent string literals) now reads the real migrations directory, and the no-hardcoded-names invariant enforced only a denylist of three legacy index names, so it passed vacuously over the new hardcode it was meant to catch. README: correct the claim that editing an applied migration "breaks migrate deploy" — verified against PG16/Prisma 5.22.0, migrate deploy never re-reads an applied migration and exits 0. The real reasons not to are that the edit reaches nobody who already applied it, and that migrate dev shadow-replays it. * refactor(supersync): drop the lock-retry sleep knob Prisma CLI start already spaces retries several seconds apart, so the explicit sleep bought little: ~25% duty cycle of self-inflicted lock queueing without it vs ~11% with. Removing it deletes an operator-facing env knob, its validation, and its tests, and shortens the worst case from ~80-160s to ~45-60s. Gate behaviour verified identical under the shipped node:24-alpine BusyBox sh and grep, not just dash/bash. * test(supersync): prove the lock-retry path against real PostgreSQL The existing spec drives a FAKE npx prisma, so it can only prove control flow. It cannot prove the gate matches SQL Postgres accepts, that Prisma's real 55P03 output matches the script's log anchors, or that a plain seq-scan reader is enough to starve the ALTER's lock window. This holds a real reader open on a real table, deploys the lock-bounded migration through the real script against real Prisma, and asserts it loses the race, retries, wins, and leaves reloptions set. Runs in the existing test:integration:postgres suite; skips without DATABASE_URL. Sabotage-verified: with MAX_LOCK_ATTEMPTS=1 the test fails. * fix(supersync): address delta review of the lock-retry work A three-agent review of the post-review fixes found one CRITICAL and several real gaps. All verified before fixing. - CRITICAL: the new integration spec was not in vitest.config.ts's exclude list, so `npm test` would run it in CI — where DATABASE_URL is set at job level — before `prisma db push` and in parallel workers, twice per run. - The lock_timeout bound was asymmetric: '9999ms' (~10s) was accepted while '6s' was refused, so a conforming migration could still queue traffic for ~10s, retried 10x. Capped both branches at 5s. - Dropped the ';' exclusions from both patterns: proven redundant (the anchored paren-free option list is what excludes a smuggled statement) and they false-rejected legal exotic index names. Corrected two comments that credited the wrong mechanism, and 'all four are enforced below' when only three are. - Removed unmeasured timing claims from the MAX_LOCK_ATTEMPTS comment; the measured CLI cost was ~0.45s, not the ~1.6s asserted, making the stated duty cycle wrong in the unsafe direction. Inter-attempt spacing is incidental now, and says so. - migration-sql.spec.ts: deleted the legacy three-name denylist (subsumed by the derived list), added a sentinel + reloption-keyword check to close the coverage hole left by dropping the 'fastupdate' assertion. - Integration spec: release the blocker on the first observed lock timeout instead of a 12s timer, so contention is guaranteed at any runner speed (also 13.6s -> 3.3s); drop the schema in beforeAll too, since a killed run poisoned later runs; make cleanup failure-robust; assert the retry count stays well under the budget rather than merely non-zero. Sabotage-verified: widening the ms cap, hardcoding a reloption or index name, and making the gate case-sensitive each kill a test; MAX_LOCK_ATTEMPTS=1 still kills the integration test. Gate re-probed under the shipped node:24-alpine BusyBox sh/grep. * fix(supersync): cover the lock-timeout refusal guard and strip ANSI Round-4 review, three agents. The headline finding was a coverage hole, not a code defect: deleting the entire lock-timeout refusal guard left 72/72 green. That guard is the only thing stopping a 55P03 on a CONCURRENTLY migration from falling through to out-of-band execution + 'resolve --applied' — the shape of the 2026-05-18 missing-index outage (#7654). Deleting it now fails 10 tests. - Assert the REASON in the refusal cases, not just a non-zero exit; every other fail_loudly satisfied the old assertions equally. - New test: a CONCURRENTLY migration that lock-times-out is never auto-recovered. - Strip ANSI from the migrate log at capture. Prisma colorizes 'Error: P3018' under FORCE_COLOR, and that anchor is the first conjunct of all three failure detectors, so one stray env var disabled every recovery path — and poisoned parse_failing_migration too, which a log_has_line-only fix would have missed. Stripping once at capture covers both readers. Verified under BusyBox sed. - Accept-side coverage for the millisecond bound: dropping the whole ms branch used to leave the suite green while refusing every documented ms migration. - Cover the per-migration counter reset, and correct the comment that implied MAX_LOCK_ATTEMPTS bounds a whole deploy — it is N x 10 for N migrations. - Print the INVALID-index recovery hint when a lock timeout aborts a bare CONCURRENTLY build, matching the other fail-loud branches. - Validate MIGRATE_MAX_ATTEMPTS: the file argued that hazard to justify hardcoding MAX_LOCK_ATTEMPTS while leaving its sibling unguarded. - Sentinel for the reloption denylist; it had exactly one entry, so a drifting regex would have gone vacuous. - Integration spec: 60s hook timeout on beforeAll (it does DB work now), 120s backstop, failure output on the assertions that rendered none, drop a dead assertion. - execFileSync timeout so a runaway loop fails instead of hanging CI. Sabotage-verified: each of the four new guards kills at least one test. |
||
|---|---|---|
| .. | ||
| clean-slate-atomicity-sql.integration.spec.ts | ||
| conflict-detection-sql.integration.spec.ts | ||
| health-alert-db-probe.integration.spec.ts | ||
| migrate-deploy-db-timeout.integration.spec.ts | ||
| migrate-deploy-lock-retry.integration.spec.ts | ||
| multi-client-sync.integration.spec.ts | ||
| registration-races.integration.spec.ts | ||
| repair-causality.integration.spec.ts | ||
| snapshot-skip-optimization.integration.spec.ts | ||
| snapshot-vector-clock-sql.integration.spec.ts | ||
| websocket-storm.integration.spec.ts | ||