From 0baa3338092640fe0bf5dac3611ccf533e6835f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Sun, 14 Jun 2026 20:44:54 +0200 Subject: [PATCH] feat(lint): forbid data mutations in fast instance command up() (#21547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Fast instance commands run in the ArgoCD **PreSync** hook, before the new pods roll. A bulk `UPDATE`/`INSERT`/`DELETE` held in the **same transaction** as an `ADD COLUMN`/`ALTER` keeps an `ACCESS EXCLUSIVE` lock on the table for the whole write, blocking every read of it. That is what froze prod during the 2.13 `isUIReadOnly → isUIEditable` rename — a bulk `UPDATE "fieldMetadata"` inside the same `up()` transaction as the `ADD COLUMN`s → read timeouts → failed PreSync → aborted sync. @charlesBochet already caught this exact pattern by hand on #21527 ("data migration => make a slow instance command :)"). This turns that manual review into something CI enforces. ## What New oxlint rule **`twenty/no-data-mutation-in-fast-instance-command`**: - Flags statement-leading `UPDATE`/`INSERT`/`DELETE`/`MERGE` passed to `.query(...)` **inside `up()`** of a `*-instance-command-fast-*` file. - Allows: schema DDL (`ALTER`/`CREATE`/`DROP`); `ON DELETE CASCADE` / a column named `updatedAt` (not statement-leading, so never matched); rollback DML in `down()`; and data migrations in **slow** commands' `runDataMigration()`. - The error message points the author straight at the slow-command pattern. Enabled as `error` in `twenty-server`. ## Grandfathering Scoping to `up()` means **only one** existing file violates the rule: the already-shipped 2.13 rename command. It's recorded complete in cloud and must not be rewritten, so it's grandfathered with a documented file-level `oxlint-disable` (the comment makes clear it's an exception, not a precedent). The four other fast commands that contain DML keep theirs in `down()` and are correctly unaffected. ## Tests - 9 RuleTester cases — valid: DDL, FK cascade, `updatedAt`, `down()` DML, slow-command DML, non-upgrade files; invalid: `UPDATE`/`INSERT`/`DELETE` in `up()`. - Verified end-to-end with oxlint: a throwaway violating file → 1 error; all 141 upgrade-command files → 0 errors; full oxlint-rules suite 225/225; typecheck clean. Part of the v2.13 deploy post-mortem follow-ups. https://claude.ai/code/session_013Az1etaGyxWRRVhgjhPWeB --- _Generated by [Claude Code](https://claude.ai/code/session_013Az1etaGyxWRRVhgjhPWeB)_ Review in cubic --------- Co-authored-by: Claude --- packages/twenty-oxlint-rules/oxlint-plugin.ts | 6 + ...-mutation-in-fast-instance-command.spec.ts | 102 +++++++++++++ ...-data-mutation-in-fast-instance-command.ts | 143 ++++++++++++++++++ packages/twenty-server/.oxlintrc.json | 1 + ...ename-is-ui-read-only-to-is-ui-editable.ts | 7 + 5 files changed, 259 insertions(+) create mode 100644 packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.spec.ts create mode 100644 packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.ts diff --git a/packages/twenty-oxlint-rules/oxlint-plugin.ts b/packages/twenty-oxlint-rules/oxlint-plugin.ts index 058503f240..062c5d0477 100644 --- a/packages/twenty-oxlint-rules/oxlint-plugin.ts +++ b/packages/twenty-oxlint-rules/oxlint-plugin.ts @@ -32,6 +32,10 @@ import { rule as maxConstsPerFile, RULE_NAME as maxConstsPerFileName, } from './rules/max-consts-per-file'; +import { + rule as noDataMutationInFastInstanceCommand, + RULE_NAME as noDataMutationInFastInstanceCommandName, +} from './rules/no-data-mutation-in-fast-instance-command'; import { rule as noDirectAtomFamilyInSelector, RULE_NAME as noDirectAtomFamilyInSelectorName, @@ -84,6 +88,8 @@ export default definePlugin({ [injectWorkspaceRepositoryName]: injectWorkspaceRepository, [matchingStateVariableName]: matchingStateVariable, [maxConstsPerFileName]: maxConstsPerFile, + [noDataMutationInFastInstanceCommandName]: + noDataMutationInFastInstanceCommand, [noDirectAtomFamilyInSelectorName]: noDirectAtomFamilyInSelector, [noHardcodedColorsName]: noHardcodedColors, [noJotaiStoreInSelectorName]: noJotaiStoreInSelector, diff --git a/packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.spec.ts b/packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.spec.ts new file mode 100644 index 0000000000..62a8c566da --- /dev/null +++ b/packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.spec.ts @@ -0,0 +1,102 @@ +import { RuleTester } from 'oxlint/plugins-dev'; + +import { rule, RULE_NAME } from './no-data-mutation-in-fast-instance-command'; + +const ruleTester = new RuleTester(); + +const BASE = + '/project/packages/twenty-server/src/database/commands/upgrade-version-command'; + +const FAST_FILE = `${BASE}/2-13/2-13-instance-command-fast-1781277453604-rename-flag.ts`; +const SLOW_FILE = `${BASE}/2-13/2-13-instance-command-slow-1781277480000-backfill-flag.ts`; + +ruleTester.run(RULE_NAME, rule, { + valid: [ + // Schema changes are exactly what a fast command is for. + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query('ALTER TABLE "core"."x" ADD COLUMN IF NOT EXISTS "y" boolean NOT NULL DEFAULT true'); } }`, + }, + // A foreign key clause contains DELETE/UPDATE but is not statement-leading DML. + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query('ALTER TABLE "core"."a" ADD CONSTRAINT "fk" FOREIGN KEY ("bId") REFERENCES "core"."b"("id") ON DELETE CASCADE ON UPDATE NO ACTION'); } }`, + }, + // A column named "updatedAt" must not trip the UPDATE keyword. + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query('ALTER TABLE "core"."x" ADD COLUMN "updatedAt" timestamptz'); } }`, + }, + // A read-only CTE has no data mutation. + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query(\`WITH recent AS (SELECT "id" FROM "core"."x") SELECT count(*) FROM recent\`); } }`, + }, + // A read-only CTE that only mentions a keyword in a string is not flagged. + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query(\`WITH note AS (SELECT 'needs update later' AS msg) SELECT msg FROM note\`); } }`, + }, + // A keyword after a parenthesis inside a string literal is not flagged. + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query(\`WITH note AS (SELECT 'reminder: (UPDATE later)' AS msg) SELECT msg FROM note\`); } }`, + }, + // Rollback DML lives in down() and is allowed (incl. via a CTE). + { + filename: FAST_FILE, + code: `class C { async down(q) { await q.query(\`UPDATE "core"."x" SET "y" = false WHERE "z" = true\`); } }`, + }, + { + filename: FAST_FILE, + code: `class C { async down(q) { await q.query(\`WITH ids AS (SELECT "id" FROM "core"."x") UPDATE "core"."x" SET "y" = true\`); } }`, + }, + // down() written as an arrow-function property is also recognized. + { + filename: FAST_FILE, + code: `class C { down = async (q) => { await q.query('DELETE FROM "core"."x" WHERE 1 = 1'); }; }`, + }, + // A slow command is the correct home for a data migration. + { + filename: SLOW_FILE, + code: `class C { async runDataMigration(ds) { await ds.query(\`UPDATE "core"."x" SET "y" = false\`); } }`, + }, + // Files outside the upgrade-command tree are ignored. + { + filename: '/project/packages/twenty-front/src/foo.ts', + code: `class C { async up(q) { await q.query(\`UPDATE "x" SET "y" = 1\`); } }`, + }, + ], + invalid: [ + // The incident pattern: bulk UPDATE in up(). + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query(\`UPDATE "core"."fieldMetadata" SET "isUIEditable" = false WHERE "isUIReadOnly" = true\`); } }`, + errors: [{ messageId: 'dataMutationInFastInstanceCommand' }], + }, + // INSERT in up(). + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query('INSERT INTO "core"."x" ("id") VALUES (1)'); } }`, + errors: [{ messageId: 'dataMutationInFastInstanceCommand' }], + }, + // DELETE in up(). + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query('DELETE FROM "core"."x" WHERE "t" = 1'); } }`, + errors: [{ messageId: 'dataMutationInFastInstanceCommand' }], + }, + // Data mutation moved into a helper called by up() is still forbidden. + { + filename: FAST_FILE, + code: `class C { async backfill(q) { await q.query('DELETE FROM "core"."x" WHERE 1 = 1'); } }`, + errors: [{ messageId: 'dataMutationInFastInstanceCommand' }], + }, + // CTE-wrapped data mutation in up(). + { + filename: FAST_FILE, + code: `class C { async up(q) { await q.query(\`WITH ids AS (SELECT "id" FROM "core"."x") UPDATE "core"."x" SET "y" = false\`); } }`, + errors: [{ messageId: 'dataMutationInFastInstanceCommand' }], + }, + ], +}); diff --git a/packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.ts b/packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.ts new file mode 100644 index 0000000000..f9cb040631 --- /dev/null +++ b/packages/twenty-oxlint-rules/rules/no-data-mutation-in-fast-instance-command.ts @@ -0,0 +1,143 @@ +import { defineRule } from '@oxlint/plugins'; + +export const RULE_NAME = 'no-data-mutation-in-fast-instance-command'; + +const UPGRADE_COMMAND_MARKER = 'upgrade-version-command/'; +const FAST_INSTANCE_COMMAND_SEGMENT = 'instance-command-fast-'; +const SKIPPED_FILE_REGEX = /\.(spec|test)\.ts$/; + +const DATA_MUTATION_STATEMENT_REGEX = /^(UPDATE|INSERT|DELETE|MERGE)\b/i; +const CTE_DATA_MUTATION_REGEX = + /^WITH\b[\s\S]*[()]\s*(UPDATE|INSERT|DELETE|MERGE)\b/i; + +const isFastInstanceCommandFile = (filename: string): boolean => { + const markerIndex = filename.indexOf(UPGRADE_COMMAND_MARKER); + + if (markerIndex === -1) { + return false; + } + + const basename = + filename + .slice(markerIndex + UPGRADE_COMMAND_MARKER.length) + .split('/') + .pop() ?? ''; + + if (SKIPPED_FILE_REGEX.test(basename)) { + return false; + } + + return basename.includes(FAST_INSTANCE_COMMAND_SEGMENT); +}; + +const isInsideDownMethod = (node: any): boolean => { + let current = node.parent; + + while (current) { + if ( + (current.type === 'MethodDefinition' || + current.type === 'PropertyDefinition') && + current.key?.type === 'Identifier' && + current.key.name === 'down' + ) { + return true; + } + + current = current.parent; + } + + return false; +}; + +const getSqlText = (argument: any): string | null => { + if (argument.type === 'Literal' && typeof argument.value === 'string') { + return argument.value; + } + + if (argument.type === 'TemplateLiteral') { + return argument.quasis.map((quasi: any) => quasi.value.raw).join(' '); + } + + return null; +}; + +const findDataMutationKeyword = (sql: string): string | null => { + const normalized = sql + .replace(/--[^\n]*/g, '') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/'(?:[^']|'')*'/g, "''") + .replace(/"(?:[^"]|"")*"/g, '""'); + + for (const statement of normalized.split(';')) { + const trimmed = statement.trim(); + const match = + trimmed.match(DATA_MUTATION_STATEMENT_REGEX) ?? + trimmed.match(CTE_DATA_MUTATION_REGEX); + + if (match) { + return match[1].toUpperCase(); + } + } + + return null; +}; + +export const rule = defineRule({ + meta: { + type: 'problem', + docs: { + description: + 'Disallow data mutations (UPDATE/INSERT/DELETE) in a fast instance command outside down(); backfills belong in a slow instance command', + }, + schema: [], + messages: { + dataMutationInFastInstanceCommand: + "Fast instance commands must not run data mutations outside down() (found '{{ keyword }}'). A bulk write held in the same transaction as ADD/ALTER COLUMN keeps an ACCESS EXCLUSIVE lock and can stall reads during the deploy. Put backfills in a slow instance command's runDataMigration(); rollback writes belong in down().", + }, + }, + create: (context) => { + if (!isFastInstanceCommandFile(context.filename)) { + return {}; + } + + return { + CallExpression: (node: any) => { + const callee = node.callee; + + if ( + callee?.type !== 'MemberExpression' || + callee.property?.type !== 'Identifier' || + callee.property.name !== 'query' + ) { + return; + } + + if (isInsideDownMethod(node)) { + return; + } + + const sqlArgument = node.arguments?.[0]; + + if (!sqlArgument) { + return; + } + + const sql = getSqlText(sqlArgument); + + if (sql === null) { + return; + } + + const keyword = findDataMutationKeyword(sql); + + if (keyword) { + context.report({ + node: sqlArgument, + messageId: 'dataMutationInFastInstanceCommand', + data: { keyword }, + }); + } + }, + }; + }, +}); diff --git a/packages/twenty-server/.oxlintrc.json b/packages/twenty-server/.oxlintrc.json index bed13e34f6..1ec6c4c250 100644 --- a/packages/twenty-server/.oxlintrc.json +++ b/packages/twenty-server/.oxlintrc.json @@ -82,6 +82,7 @@ "twenty/rest-api-methods-should-be-guarded": "error", "twenty/graphql-resolvers-should-be-guarded": "error", "twenty/upgrade-command-filename": "error", + "twenty/no-data-mutation-in-fast-instance-command": "error", "twenty/enforce-module-boundaries": [ "error", { diff --git a/packages/twenty-server/src/database/commands/upgrade-version-command/2-13/2-13-instance-command-fast-1781277453604-rename-is-ui-read-only-to-is-ui-editable.ts b/packages/twenty-server/src/database/commands/upgrade-version-command/2-13/2-13-instance-command-fast-1781277453604-rename-is-ui-read-only-to-is-ui-editable.ts index 98c99f563c..e1a6a2a92d 100644 --- a/packages/twenty-server/src/database/commands/upgrade-version-command/2-13/2-13-instance-command-fast-1781277453604-rename-is-ui-read-only-to-is-ui-editable.ts +++ b/packages/twenty-server/src/database/commands/upgrade-version-command/2-13/2-13-instance-command-fast-1781277453604-rename-is-ui-read-only-to-is-ui-editable.ts @@ -1,3 +1,10 @@ +// Grandfathered: this 2.13 command already shipped to cloud and is recorded +// complete there, so per "never rewrite a committed instance command" it is +// frozen as-is. Its up() does ADD COLUMN + bulk UPDATE in one transaction — the +// exact pattern that held an ACCESS EXCLUSIVE lock and stalled prod reads, and +// the reason no-data-mutation-in-fast-instance-command exists. This is an +// exception, not a precedent: new backfills go in a slow instance command. +/* oxlint-disable twenty/no-data-mutation-in-fast-instance-command */ import { QueryRunner } from 'typeorm'; import { RegisteredInstanceCommand } from 'src/engine/core-modules/upgrade/decorators/registered-instance-command.decorator';