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; }