From e1671469faebd62b583b91a48b74ab70faa9212f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 27 Sep 2026 04:33:57 -0700 Subject: [PATCH] fix(migrate): refuse to drop the legacy source-ref index until the pair index is live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drop is the load-bearing half of this migration, not the create. Pre-#1876 code matches podId_1_sourceRef_1_partial by name in its 11000 recovery, so dropping it while that code is live removes the only thing that converts a lost create race into an idempotent replay; a differing-title race then writes a second row per (podId, sourceRef) that the deployed code resolves arbitrarily. The header note said the ordering was "safe either way", which reads true only if you reason about the index the script creates and skip the one it drops. Guard: withhold the drop (exit 1, nothing written) unless the pair index is visible, with --force for an operator who knows better. The pair index is the evidence because models/Task.ts declares it and nothing in backend disables mongoose autoIndex, so a boot creates it. It proves some pair-aware code booted against this database, not which version — enough for the ordering hazard and said so in the comment. Fail-closed is safe because pair-aware code boots fine while the legacy index is present (the named 503 is its designed degraded state), so deploy-then-migrate stays reachable. Two things the guard needed, both measured: - Withholding returns before createIndexes(). Creating the pair index while withholding would let the next run see it, pass the guard and drop: the guard authorising itself. - mongoose.set('autoIndex', false). The script imports the same schema the app does, so its own process created declared indexes on connect — with autoIndex on, a dry run left the pair index itself behind, and on main --dry is not read-only at all. Also corrects the header note (#1959) so the file does not contradict its own guard, and covers the CLI wiring in a child process: an earlier draft parsed --force and passed only { dryRun } through, which no unit test could see. --- .../migrate-task-source-ref-identity.test.js | 135 +++++++++++++++++- .../migrate-task-source-ref-identity.ts | 81 +++++++++-- 2 files changed, 205 insertions(+), 11 deletions(-) diff --git a/backend/__tests__/service/migrate-task-source-ref-identity.test.js b/backend/__tests__/service/migrate-task-source-ref-identity.test.js index 0b9c81347..10fb2277b 100644 --- a/backend/__tests__/service/migrate-task-source-ref-identity.test.js +++ b/backend/__tests__/service/migrate-task-source-ref-identity.test.js @@ -1,5 +1,8 @@ process.env.PG_HOST = ''; +const { spawnSync } = require('child_process'); +const path = require('path'); +const mongoose = require('mongoose'); const Task = require('../../models/Task'); const { migrateTaskSourceRefIdentity, @@ -19,6 +22,13 @@ const PAIR_INDEX = 'podId_1_sourceRef_1_title_1_partial'; // be inserted at all (the route answers that with a named 503). So this // exercises the drop/create against a real mongod rather than trusting the // script's shape. +// +// The two database states below are the two sides of the ordering guard, and +// the difference between them is what makes a run safe or not: a database where +// only the legacy index exists is one the new code has not booted against, and +// dropping there removes the deployed code's 11000 backstop. `seedLegacyIndex` +// is that state; `seedDeployedIndexes` adds the pair index the deploy's boot +// creates. describe('migrate-task-source-ref-identity', () => { beforeAll(async () => { await setupMongoDb(); @@ -53,12 +63,19 @@ describe('migrate-task-source-ref-identity', () => { } }; + const seedDeployedIndexes = async () => { + // What a deploy leaves behind: boot has created the declared pair index + // while the legacy one survives, because autoIndex creates and never drops. + await seedLegacyIndex(); + await Task.createIndexes(); + }; + beforeEach(async () => { await clearMongoDb(); await seedLegacyIndex(); }); - it('reports the legacy index and changes nothing on a dry run', async () => { + it('reports the legacy index, would withhold the drop, and changes nothing on a dry run', async () => { const result = await migrateTaskSourceRefIdentity({ dryRun: true }); expect(result).toMatchObject({ @@ -67,28 +84,139 @@ describe('migrate-task-source-ref-identity', () => { pairIndexPresentBefore: false, pairIndexPresentAfter: false, droppedLegacy: false, + dropWithheld: true, }); const names = await indexNames(); expect(names).toContain(LEGACY_INDEX); expect(names).not.toContain(PAIR_INDEX); }); - it('drops the legacy index and creates the pair index', async () => { + it('withholds the drop while the pair index is absent, and writes nothing at all', async () => { const result = await migrateTaskSourceRefIdentity(); expect(result).toMatchObject({ dryRun: false, + legacyIndexPresent: true, + pairIndexPresentBefore: false, + pairIndexPresentAfter: false, + droppedLegacy: false, + dropWithheld: true, + }); + // The assertions that matter, and the reason the withheld path returns + // before createIndexes(): if this run created the pair index, the NEXT run + // would see it, pass the guard, and drop the legacy index — the guard would + // authorise itself instead of waiting for the deploy. + const names = await indexNames(); + expect(names).toContain(LEGACY_INDEX); + expect(names).not.toContain(PAIR_INDEX); + + const second = await migrateTaskSourceRefIdentity(); + expect(second.dropWithheld).toBe(true); + expect(await indexNames()).toContain(LEGACY_INDEX); + }); + + it('drops it once the pair index is visible, which is the state a deploy leaves behind', async () => { + await seedDeployedIndexes(); + + const result = await migrateTaskSourceRefIdentity(); + + expect(result).toMatchObject({ + dryRun: false, + legacyIndexPresent: true, + pairIndexPresentBefore: true, + pairIndexPresentAfter: true, + droppedLegacy: true, + dropWithheld: false, + }); + const names = await indexNames(); + expect(names).not.toContain(LEGACY_INDEX); + expect(names).toContain(PAIR_INDEX); + }); + + it('drops it on --force while the pair index is still absent', async () => { + const result = await migrateTaskSourceRefIdentity({ force: true }); + + expect(result).toMatchObject({ legacyIndexPresent: true, pairIndexPresentBefore: false, pairIndexPresentAfter: true, droppedLegacy: true, + dropWithheld: false, }); const names = await indexNames(); expect(names).not.toContain(LEGACY_INDEX); expect(names).toContain(PAIR_INDEX); }); + // The CLI is the artifact an operator runs, and its wiring is not covered by + // calling the exported function: an earlier version of this change parsed + // --force in main() and then passed only { dryRun } to the migration, which + // eslint caught and no unit test could have. Both flags are exercised in a + // child process, against the same database this suite runs on. + const runCli = (...args) => spawnSync( + process.execPath, + [ + require.resolve('ts-node/dist/bin.js'), + 'scripts/migrate-task-source-ref-identity.ts', + ...args, + ], + { + cwd: path.join(__dirname, '../..'), + encoding: 'utf8', + timeout: 120000, + env: { + ...process.env, + MONGO_URI: `mongodb://${mongoose.connection.host}:${mongoose.connection.port}/${mongoose.connection.name}`, + }, + }, + ); + + // The guard's evidence is a database fact, and the one thing positioned to + // destroy it quietly is the script's own process: `--dry` prints "no changes + // written", and `mongoose.set('autoIndex', false)` in the script is what makes + // that true. Measured with autoIndex left on, this dry run left + // `podId_1_sourceRef_1_title_1_partial` and `podId_1_taskId_1` behind — the + // script created the very index its guard reads as "the code that replaces the + // legacy index is live", and the next run would then drop. + it('leaves the database untouched on --dry when run the way an operator runs it', async () => { + // clearMongoDb deletes documents and leaves indexes, so a sibling test that + // ran Task.createIndexes() would otherwise leave the declared indexes in + // `before` and make this assertion insensitive to what the child creates. + // Which is not hypothetical: the first version of this test passed with + // autoIndex switched back on for exactly that reason. + await Task.collection.drop().catch(() => {}); + await seedLegacyIndex(); + const before = (await indexNames()).sort(); + expect(before.filter((name) => name !== '_id_')).toEqual([LEGACY_INDEX]); + + const child = runCli('--dry'); + + expect(child.error).toBeUndefined(); + expect(child.status).toBe(0); + expect(child.stdout).toContain('DRY-RUN'); + expect(child.stdout).toContain('dropWithheld=true'); + // No index this script did not intend, and in particular not the one the + // guard reads as "the deploy has booted here". + expect((await indexNames()).sort()).toEqual(before); + }); + + it('drops it on --force when run the way an operator runs it', async () => { + await Task.collection.drop().catch(() => {}); + await seedLegacyIndex(); + + const child = runCli('--force'); + + expect(child.error).toBeUndefined(); + expect(child.status).toBe(0); + expect(child.stdout).toContain('droppedLegacy=true'); + expect(child.stdout).toContain('dropWithheld=false'); + const names = await indexNames(); + expect(names).not.toContain(LEGACY_INDEX); + expect(names).toContain(PAIR_INDEX); + }); + it('is idempotent, and the pair index is what a second ask needs', async () => { + await seedDeployedIndexes(); await migrateTaskSourceRefIdentity(); const second = await migrateTaskSourceRefIdentity(); expect(second).toMatchObject({ @@ -96,9 +224,10 @@ describe('migrate-task-source-ref-identity', () => { pairIndexPresentBefore: true, pairIndexPresentAfter: true, droppedLegacy: false, + dropWithheld: false, }); - const podId = new (require('mongoose').Types.ObjectId)(); + const podId = new mongoose.Types.ObjectId(); const base = { podId, source: 'import', diff --git a/backend/scripts/migrate-task-source-ref-identity.ts b/backend/scripts/migrate-task-source-ref-identity.ts index 85e204377..8a55b3365 100644 --- a/backend/scripts/migrate-task-source-ref-identity.ts +++ b/backend/scripts/migrate-task-source-ref-identity.ts @@ -11,18 +11,41 @@ * (`task_source_ref_index_migration_pending`) instead of a silent wrong row, * and the gap is loud rather than quiet. * - * Ordering is safe either way: before the deploy it creates an index the - * deployed code never consults; after the deploy it only shortens the window in - * which a mismatched-title create is refused. + * Ordering: run this AFTER the new code is deployed, never before. The two + * orderings are not comparable, because it is the DROP that is load-bearing: + * the pre-TASK-063 code matches the legacy index by name in its 11000 recovery + * (routes/tasksApi.ts), so dropping it while that code is still live takes away + * the only thing that turns a lost create race into an idempotent replay, and a + * differing-title race then writes a SECOND row for one (podId, sourceRef) that + * the deployed code resolves arbitrarily. After the deploy the worst case is a + * named 503 (`task_source_ref_index_migration_pending`) on that narrow case for + * a bounded window, with nothing written. + * + * `migrateTaskSourceRefIdentity` enforces this rather than trusting the reader + * of a note: it refuses to drop the legacy index until the pair index is + * visible. The pair index is not a version marker, but its presence does mean + * pair-aware code has booted against this database. * * Usage (from backend/): * npm run migrate:task-source-ref-identity -- --dry * npm run migrate:task-source-ref-identity + * npm run migrate:task-source-ref-identity -- --force # drop anyway */ import mongoose from 'mongoose'; import Task from '../models/Task'; +// This process must not create indexes as a side effect of connecting. Two +// reasons, both about the ordering guard below. Whatever autoIndex creates is +// created by this script rather than by a boot, so it cannot serve as evidence +// about the deploy: measured on a database holding only the legacy index, a DRY +// RUN left `podId_1_status_1` behind, and the declared indexes are created one +// after another — so the pair index is next in the same sequence and any run +// that has not already exited leaves the guard's own evidence behind for its +// next run to pass on. And it makes `--dry` what its own output claims: no +// changes written. `Task.createIndexes()` below is explicit and unaffected. +mongoose.set('autoIndex', false); + const LEGACY_INDEX = 'podId_1_sourceRef_1_partial'; const PAIR_INDEX = 'podId_1_sourceRef_1_title_1_partial'; @@ -32,6 +55,11 @@ export interface TaskSourceRefIdentityResult { pairIndexPresentBefore: boolean; pairIndexPresentAfter: boolean; droppedLegacy: boolean; + // The legacy index was left in place because the pair index it is being + // replaced by is not visible (or, under --dry, would be left in place). + // Nothing at all was written in that case — see the guard below for why + // creating the pair index here would defeat the guard on its next run. + dropWithheld: boolean; } async function indexNames(): Promise { @@ -48,7 +76,7 @@ async function indexNames(): Promise { } export async function migrateTaskSourceRefIdentity( - options: { dryRun?: boolean } = {}, + options: { dryRun?: boolean; force?: boolean } = {}, ): Promise { const dryRun = options.dryRun === true; const names = await indexNames(); @@ -58,8 +86,34 @@ export async function migrateTaskSourceRefIdentity( pairIndexPresentBefore: names.includes(PAIR_INDEX), pairIndexPresentAfter: names.includes(PAIR_INDEX), droppedLegacy: false, + dropWithheld: false, }; - if (dryRun) return result; + + // The ordering guard. Dropping the legacy index before the code that replaces + // it is live takes the deployed code's 11000 backstop away (see the header), + // so the drop needs evidence that code has booted against THIS database. The + // pair index is that evidence: models/Task.ts declares it and nothing in + // backend turns mongoose's autoIndex off (config/db.ts passes only + // useNewUrlParser / useUnifiedTopology), so a boot creates it — the same + // reason the test for this script has to DROP it to reproduce an old database. + // + // Two properties make fail-closed safe here. It cannot deadlock: pair-aware + // code boots fine while the legacy index is present — the named 503 IS its + // designed degraded state — so deploy-then-migrate stays reachable. And the + // signal is honest about what it is: an index being present proves SOME + // pair-aware code booted here, not which version, which is enough for the + // ordering hazard and is not a version check. --force is for an operator who + // has another reason to believe the deploy is live. + result.dropWithheld = result.legacyIndexPresent + && !result.pairIndexPresentBefore + && options.force !== true; + + // Withholding returns before createIndexes(). Creating the pair index here + // would make the NEXT run of this script see it, pass the guard, and drop — + // the guard would authorise itself. Nothing is written while the drop is + // withheld, and nothing needs to be: the deploy creates the pair index at + // boot, which is exactly what the guard is waiting for. + if (dryRun || result.dropWithheld) return result; if (result.legacyIndexPresent) { await Task.collection.dropIndex(LEGACY_INDEX); @@ -76,6 +130,7 @@ export async function migrateTaskSourceRefIdentity( async function main(): Promise { const dryRun = process.argv.includes('--dry'); + const force = process.argv.includes('--force'); const mongoUri = process.env.MONGO_URI; if (!mongoUri) { console.error('MONGO_URI is required'); @@ -83,15 +138,25 @@ async function main(): Promise { } await mongoose.connect(mongoUri); try { - const result = await migrateTaskSourceRefIdentity({ dryRun }); + const result = await migrateTaskSourceRefIdentity({ dryRun, force }); console.log( `[migrate-task-source-ref-identity] ${dryRun ? 'DRY-RUN (no changes written) ' : ''}` + `legacyIndexPresent=${result.legacyIndexPresent} ` + `pairIndexPresentBefore=${result.pairIndexPresentBefore} ` + `pairIndexPresentAfter=${result.pairIndexPresentAfter} ` - + `droppedLegacy=${result.droppedLegacy}`, + + `droppedLegacy=${result.droppedLegacy} ` + + `dropWithheld=${result.dropWithheld}`, ); - if (!dryRun && !result.pairIndexPresentAfter) { + if (result.dropWithheld) { + console.error( + `[migrate-task-source-ref-identity] ${dryRun ? 'would withhold' : 'WITHHELD'} the drop of ` + + `${LEGACY_INDEX}: ${PAIR_INDEX} is not present, so the code that replaces the legacy index` + + ' does not appear to have booted against this database yet. Deploy the new code first and' + + ` re-run; --force drops it anyway, at the cost of the deployed code having no 11000` + + ' backstop for a differing-title race.', + ); + if (!dryRun) process.exitCode = 1; + } else if (!dryRun && !result.pairIndexPresentAfter) { console.error(`[migrate-task-source-ref-identity] ${PAIR_INDEX} is still missing after createIndexes()`); process.exitCode = 1; }