diff --git a/.changeset/encrypt-backfill-chunk-size.md b/.changeset/encrypt-backfill-chunk-size.md new file mode 100644 index 000000000..175bf5289 --- /dev/null +++ b/.changeset/encrypt-backfill-chunk-size.md @@ -0,0 +1,5 @@ +--- +'stash': patch +--- + +`stash encrypt backfill` now rejects a `--chunk-size` that is not a positive whole number (`abc`, `0`, `-5`, `1.5`) with a clear error and exit code 1, before loading the encryption client or opening a database connection. Previously the value reached the backfill engine unchecked: a non-numeric value became `NaN` and failed inside Postgres after the dual-write gate had already been recorded, under the generic "Backfill failed" message, and `0` reported a completed backfill having encrypted nothing. The default is unchanged at 1000 and is now shown in `--help`. diff --git a/languages/typescript/packages/cli/src/bin/main.ts b/languages/typescript/packages/cli/src/bin/main.ts index a74b99a0e..f738aa804 100644 --- a/languages/typescript/packages/cli/src/bin/main.ts +++ b/languages/typescript/packages/cli/src/bin/main.ts @@ -19,6 +19,7 @@ import { readFileSync } from 'node:fs' import { dirname, join } from 'node:path' import { fileURLToPath } from 'node:url' import * as p from '@clack/prompts' +import { InvalidChunkSizeError, parseChunkSize } from '../cli/chunk-size.js' import { CliExit } from '../cli/exit.js' import { renderCommandHelp } from '../cli/help.js' import { validateInstallFlags } from '../commands/db/install.js' @@ -429,9 +430,7 @@ async function runEncryptCommand( table, column, pkColumn: values['pk-column'], - chunkSize: values['chunk-size'] - ? Number(values['chunk-size']) - : undefined, + chunkSize: parseChunkSizeFlag(values['chunk-size']), encryptedColumn: values['encrypted-column'], schemaColumnKey: values['schema-column-key'], confirmDualWritesDeployed: flags['confirm-dual-writes-deployed'], @@ -475,6 +474,24 @@ function requireValue(values: Record, key: string): string { return v } +/** + * Parse `--chunk-size` at the argv boundary, where the raw text is still in + * hand, so the error can quote exactly what was typed. The handler validates + * the number again through the same helper, for callers that bypass argv. + * Returns `undefined` when the flag is absent so the handler applies its + * own default. + */ +function parseChunkSizeFlag(raw: string | undefined): number | undefined { + if (raw === undefined) return undefined + try { + return parseChunkSize(raw) + } catch (error) { + if (!(error instanceof InvalidChunkSizeError)) throw error + p.log.error(error.message) + throw new CliExit(1) + } +} + async function runSchemaCommand( sub: string | undefined, flags: Record, diff --git a/languages/typescript/packages/cli/src/cli/__tests__/chunk-size.test.ts b/languages/typescript/packages/cli/src/cli/__tests__/chunk-size.test.ts new file mode 100644 index 000000000..89827b7c9 --- /dev/null +++ b/languages/typescript/packages/cli/src/cli/__tests__/chunk-size.test.ts @@ -0,0 +1,86 @@ +/** + * `--chunk-size` is the one backfill flag whose bad values do not fail at the + * prompt: `Number('abc')` is `NaN`, and the engine binds whatever it is given + * straight into `LIMIT $n`. These tests pin the single decision — what counts + * as a chunk size — that both `encrypt backfill` and `decrypt backfill` defer + * to, in both the forms it is reached with: raw argv text and a coerced + * number. + */ + +import { describe, expect, it } from 'vitest' +import { + InvalidChunkSizeError, + parseChunkSize, + resolveChunkSize, +} from '../chunk-size.js' + +describe('parseChunkSize', () => { + it.each([ + ['500', 500], + ['1', 1], + [' 42 ', 42], + ['007', 7], + [500, 500], + [1, 1], + ])('accepts %j as %d', (input, expected) => { + expect(parseChunkSize(input)).toBe(expected) + }) + + it.each([ + 'abc', + '', + ' ', + '0', + '-5', + '1.5', + '1e3', + '0x10', + 'Infinity', + 'NaN', + '12abc', + ])('rejects the text %j', (input) => { + expect(() => parseChunkSize(input)).toThrow(InvalidChunkSizeError) + }) + + it.each([ + Number('abc'), + 0, + -5, + 1.5, + Number.POSITIVE_INFINITY, + Number.MAX_SAFE_INTEGER + 1, + ])('rejects the number %s', (input) => { + expect(() => parseChunkSize(input)).toThrow(InvalidChunkSizeError) + }) + + it('names the flag and quotes exactly what was typed', () => { + expect(() => parseChunkSize('abc')).toThrow( + '--chunk-size must be a positive whole number of rows (got "abc").', + ) + }) + + it('reports a coerced number as the number it became', () => { + // What a handler sees after argv's `Number(...)` — the raw text is gone, + // so the message says what arrived rather than inventing a spelling. + expect(() => parseChunkSize(Number('abc'))).toThrow('(got NaN)') + expect(() => parseChunkSize(0)).toThrow('(got 0)') + expect(() => parseChunkSize(1.5)).toThrow('(got 1.5)') + }) +}) + +describe('resolveChunkSize', () => { + it('applies the fallback only when the flag was not given', () => { + expect(resolveChunkSize(undefined, 1000)).toBe(1000) + expect(resolveChunkSize(250, 1000)).toBe(250) + expect(resolveChunkSize('250', 1000)).toBe(250) + }) + + it('does not let a bad value fall back to the default', () => { + // A typo must be an error, not a silent 1000-row run. + expect(() => resolveChunkSize(Number('abc'), 1000)).toThrow( + InvalidChunkSizeError, + ) + expect(() => resolveChunkSize('', 1000)).toThrow(InvalidChunkSizeError) + expect(() => resolveChunkSize(0, 1000)).toThrow(InvalidChunkSizeError) + }) +}) diff --git a/languages/typescript/packages/cli/src/cli/chunk-size.ts b/languages/typescript/packages/cli/src/cli/chunk-size.ts new file mode 100644 index 000000000..85f58b6c2 --- /dev/null +++ b/languages/typescript/packages/cli/src/cli/chunk-size.ts @@ -0,0 +1,69 @@ +/** + * `--chunk-size` validation, shared by every backfill-shaped command. + * + * The flag arrives as text from argv and is coerced with `Number(...)`, so + * `--chunk-size abc` used to reach the backfill engine as `NaN`. The engine + * binds it straight into the page query's `LIMIT $n`, so Postgres refused it + * — but only after the dual-write gate had been recorded and the manifest + * written, and under the generic "Backfill failed" message that deliberately + * hides the database's text. Negatives and fractions failed the same way. + * `0` was worse: `LIMIT 0` returns no rows, which the engine reads as + * "nothing left to do" and reports as a completed backfill. + * + * This module is the one place that decides what a chunk size is, so + * `encrypt backfill` and `decrypt backfill` cannot disagree. + */ + +/** + * Thrown by {@link parseChunkSize} for a value that is not a positive whole + * number. The message is author-controlled, names the flag and quotes exactly + * what was received, and carries no row data — safe to print verbatim. + */ +export class InvalidChunkSizeError extends Error { + constructor(message: string) { + super(message) + this.name = 'InvalidChunkSizeError' + } +} + +/** + * Parse a `--chunk-size` value into the number of rows per chunk. + * + * Accepts the flag's raw argv text (so the error can quote what the user + * typed) or an already-coerced number (what a command handler receives). + * Anything that is not a positive whole number — `abc`, `0`, `-5`, `1.5`, + * `NaN`, `1e3`, an empty string — throws {@link InvalidChunkSizeError}. + * + * Text is matched as plain decimal digits rather than through `Number(...)`, + * so `0x10`, `1e3` and `Infinity` are rejected: a chunk size is a row count, + * and the only sensible spelling is one a user would read back the same way. + */ +export function parseChunkSize(input: string | number): number { + const value = + typeof input === 'string' + ? /^\s*\d+\s*$/.test(input) + ? Number(input) + : Number.NaN + : input + + if (!Number.isSafeInteger(value) || value < 1) { + const received = + typeof input === 'string' ? JSON.stringify(input) : String(input) + throw new InvalidChunkSizeError( + `--chunk-size must be a positive whole number of rows (got ${received}).`, + ) + } + + return value +} + +/** + * {@link parseChunkSize} with a default: `undefined` (the flag was not + * given) yields `fallback`, anything else is validated. + */ +export function resolveChunkSize( + input: string | number | undefined, + fallback: number, +): number { + return input === undefined ? fallback : parseChunkSize(input) +} diff --git a/languages/typescript/packages/cli/src/cli/registry.ts b/languages/typescript/packages/cli/src/cli/registry.ts index 746c68e1c..0c02dd190 100644 --- a/languages/typescript/packages/cli/src/cli/registry.ts +++ b/languages/typescript/packages/cli/src/cli/registry.ts @@ -596,7 +596,8 @@ export const registry: CommandGroup[] = [ { name: '--chunk-size', value: '', - description: 'Rows encrypted per batch.', + description: 'Rows encrypted per batch; a positive whole number.', + default: '1000', }, { name: '--encrypted-column', diff --git a/languages/typescript/packages/cli/src/commands/encrypt/__tests__/backfill-chunk-size.test.ts b/languages/typescript/packages/cli/src/commands/encrypt/__tests__/backfill-chunk-size.test.ts new file mode 100644 index 000000000..9a5a70e4e --- /dev/null +++ b/languages/typescript/packages/cli/src/commands/encrypt/__tests__/backfill-chunk-size.test.ts @@ -0,0 +1,169 @@ +/** + * Pins that `stash encrypt backfill` refuses a bad `--chunk-size` BEFORE it + * does anything else. + * + * The order matters as much as the refusal. The handler loads the stash + * config, loads the user's encryption client (jiti, credentials), opens a + * pg pool, and runs the dual-write gate — which RECORDS its acceptance — + * before the engine ever sees the chunk size. A `NaN` that was only caught + * at `LIMIT $n` had already paid for all of that and left a `dual_writing` + * event behind. So each test asserts the exit AND that none of those steps + * ran. + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const poolCtor = vi.hoisted(() => vi.fn()) +vi.mock('pg', () => ({ + default: { + Pool: class { + constructor(config: unknown) { + poolCtor(config) + } + connect = vi.fn(async () => { + throw new Error('the pool must never be reached') + }) + end = vi.fn(async () => {}) + }, + }, +})) + +// Only `installMigrationsSchema` and `runBackfill` need to be observable; +// everything else stays real so a renamed export fails loudly. +const migrateMocks = vi.hoisted(() => ({ + installMigrationsSchema: vi.fn(async () => {}), + runBackfill: vi.fn(async () => { + throw new Error('the engine must never be reached') + }), +})) +vi.mock('@cipherstash/migrate', async (importOriginal) => ({ + ...(await importOriginal()), + ...migrateMocks, +})) + +vi.mock('@clack/prompts', () => ({ + intro: vi.fn(), + outro: vi.fn(), + note: vi.fn(), + confirm: vi.fn(async () => true), + isCancel: vi.fn(() => false), + log: { + info: vi.fn(), + success: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + step: vi.fn(), + }, +})) + +const loadStashConfig = vi.hoisted(() => + vi.fn(async () => ({ databaseUrl: 'postgres://test' })), +) +vi.mock('@/config/index.js', () => ({ loadStashConfig })) + +const loadEncryptionContext = vi.hoisted(() => + vi.fn(async () => ({ client: {}, tables: new Map() })), +) +vi.mock('../context.js', () => ({ + loadEncryptionContext, + requireTable: vi.fn(), +})) + +vi.mock('@/commands/init/utils.js', () => ({ + detectPackageManager: vi.fn(() => 'npm'), + runnerCommand: vi.fn((_pm: string, ref: string) => `npx ${ref}`), +})) + +import * as p from '@clack/prompts' +import { backfillCommand } from '../backfill.js' + +/** + * `process.exit` must THROW here, not return: the real one never returns, + * and the handler's control flow relies on that. A no-op stub would let the + * run continue past the guard and fail the "nothing else ran" assertions for + * the wrong reason. + */ +function spyExit() { + return vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + throw new Error(`exit ${code}`) + }) as never) +} + +describe('encrypt backfill — --chunk-size validation', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it.each([ + ['the NaN that `--chunk-size abc` coerces to', Number('abc')], + ['zero', 0], + ['a negative', -5], + ['a fraction', 1.5], + ])( + 'rejects %s with exit code 1 before touching anything', + async (_, chunkSize) => { + const exitSpy = spyExit() + + await expect( + backfillCommand({ table: 'users', column: 'email', chunkSize }), + ).rejects.toThrow('exit 1') + + expect(p.log.error).toHaveBeenCalledWith( + expect.stringContaining('--chunk-size must be a positive whole number'), + ) + // Nothing paid for, nothing recorded. + expect(loadStashConfig).not.toHaveBeenCalled() + expect(loadEncryptionContext).not.toHaveBeenCalled() + expect(poolCtor).not.toHaveBeenCalled() + expect(migrateMocks.installMigrationsSchema).not.toHaveBeenCalled() + expect(migrateMocks.runBackfill).not.toHaveBeenCalled() + + exitSpy.mockRestore() + }, + ) + + it('quotes the offending value so the typo is recognisable', async () => { + const exitSpy = spyExit() + + await expect( + backfillCommand({ table: 'users', column: 'email', chunkSize: 0 }), + ).rejects.toThrow('exit 1') + + expect(p.log.error).toHaveBeenCalledWith(expect.stringContaining('(got 0)')) + exitSpy.mockRestore() + }) + + it('lets a valid chunk size through to the rest of the command', async () => { + const exitSpy = spyExit() + + // Past the guard, the first thing the handler does is load the config; + // reaching it proves the guard accepted the value. The pool's `connect` + // then throws, which the handler reports and exits on — that path is not + // under test here, only that the guard did not fire. + await expect( + backfillCommand({ table: 'users', column: 'email', chunkSize: 250 }), + ).rejects.toThrow('exit 1') + + expect(p.log.error).not.toHaveBeenCalledWith( + expect.stringContaining('--chunk-size'), + ) + expect(loadStashConfig).toHaveBeenCalledTimes(1) + expect(loadEncryptionContext).toHaveBeenCalledTimes(1) + expect(poolCtor).toHaveBeenCalledTimes(1) + exitSpy.mockRestore() + }) + + it('applies the default when the flag is absent', async () => { + const exitSpy = spyExit() + + await expect( + backfillCommand({ table: 'users', column: 'email' }), + ).rejects.toThrow('exit 1') + + expect(p.log.error).not.toHaveBeenCalledWith( + expect.stringContaining('--chunk-size'), + ) + expect(loadStashConfig).toHaveBeenCalledTimes(1) + exitSpy.mockRestore() + }) +}) diff --git a/languages/typescript/packages/cli/src/commands/encrypt/backfill.ts b/languages/typescript/packages/cli/src/commands/encrypt/backfill.ts index 21d73a5fc..8c710bbda 100644 --- a/languages/typescript/packages/cli/src/commands/encrypt/backfill.ts +++ b/languages/typescript/packages/cli/src/commands/encrypt/backfill.ts @@ -15,11 +15,18 @@ import { } from '@cipherstash/stack/schema' import * as p from '@clack/prompts' import pg from 'pg' +import { InvalidChunkSizeError, resolveChunkSize } from '@/cli/chunk-size.js' import { detectPackageManager, runnerCommand } from '@/commands/init/utils.js' import { loadStashConfig } from '@/config/index.js' import { buildPgClientConfig, explainTlsError } from '@/db/client.js' import { loadEncryptionContext, requireTable } from './context.js' +/** + * Default rows per chunk/transaction. Mirrors `runBackfill`'s own default so + * the number the CLI prints is the number the engine uses. + */ +export const DEFAULT_BACKFILL_CHUNK_SIZE = 1000 + /** * Options accepted by `stash encrypt backfill`. Each field maps 1:1 to a * CLI flag of the same name (kebab-case at the CLI boundary). @@ -70,9 +77,12 @@ export interface BackfillCommandOptions { */ pkColumn?: string /** - * Rows per chunk/transaction. Default `1000`. Lower for lock-sensitive - * tables or very wide rows; higher for tables with tiny encrypted - * payloads. Also bounds the most work lost to a `Ctrl-C` mid-chunk. + * Rows per chunk/transaction. Default {@link DEFAULT_BACKFILL_CHUNK_SIZE}. + * Lower for lock-sensitive tables or very wide rows; higher for tables + * with tiny encrypted payloads. Also bounds the most work lost to a + * `Ctrl-C` mid-chunk. Must be a positive whole number — anything else + * (including the `NaN` that `Number('abc')` yields) is rejected before + * the database is touched. */ chunkSize?: number /** @@ -112,6 +122,19 @@ export interface BackfillCommandOptions { export async function backfillCommand(options: BackfillCommandOptions) { p.intro(runnerCommand(detectPackageManager(), 'stash encrypt backfill')) + // Validated before anything is loaded or opened: a bad chunk size is a + // typo at the prompt, and the right time to say so is before the config, + // the encryption client and a database connection have been paid for — + // and well before the dual-write gate records anything. + let chunkSize: number + try { + chunkSize = resolveChunkSize(options.chunkSize, DEFAULT_BACKFILL_CHUNK_SIZE) + } catch (error) { + if (!(error instanceof InvalidChunkSizeError)) throw error + p.log.error(error.message) + process.exit(1) + } + const stashConfig = await loadStashConfig() const ctx = await loadEncryptionContext() const tableSchema = requireTable(ctx, options.table) @@ -263,7 +286,7 @@ export async function backfillCommand(options: BackfillCommandOptions) { } p.log.info( - `Backfilling ${options.table}.${plaintextColumn} → ${encryptedColumn} (pk: ${pkColumn}, chunk: ${options.chunkSize ?? 1000}, schema cast_as: ${detectedCastAs ?? '(unknown, passing through)'}).`, + `Backfilling ${options.table}.${plaintextColumn} → ${encryptedColumn} (pk: ${pkColumn}, chunk: ${chunkSize}, schema cast_as: ${detectedCastAs ?? '(unknown, passing through)'}).`, ) let lastLogged = 0 @@ -278,7 +301,7 @@ export async function backfillCommand(options: BackfillCommandOptions) { plaintextColumn, encryptedColumn, pkColumn, - chunkSize: options.chunkSize, + chunkSize, signal: controller.signal, transformPlaintext, force: options.force, diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index d5dd7cf73..74a4b1092 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -626,7 +626,7 @@ Backfill requires a `public.eql_v3_*` target column, records version 3 and the ` | Flag | Description | |---|---| | `--table` / `--column` | Required | -| `--chunk-size ` | Default 1000. Lower for lock contention, raise for wide rows. | +| `--chunk-size ` | Default 1000. Lower for lock contention, raise for wide rows. Must be a positive whole number; anything else (`abc`, `0`, `-5`, `1.5`) is rejected with exit code 1 before the database is touched. | | `--pk-column ` | Override primary-key detection. Required for composite PKs. | | `--encrypted-column ` | Override the `_encrypted` target name | | `--schema-column-key ` | Override the schema lookup key |