Skip to content

Commit 5a93cff

Browse files
committed
fix(migrations): Address migration recovery gaps
Allow edited TypeScript migrations to recover from stale running rows once the migration lock is reacquired. Pass the host Redis capability into plugin data migrations and cover both paths with tests.
1 parent db6a3ff commit 5a93cff

5 files changed

Lines changed: 150 additions & 66 deletions

File tree

packages/junior-migrations/src/runner.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,12 @@ async function runTypeScriptMigration(args: {
234234
const qualified = qualifiedTable(args.table);
235235
const sourceChanged =
236236
args.row !== undefined && args.row.hash !== args.migration.hash;
237-
if (args.row && sourceChanged && args.row.status !== "failed") {
237+
if (
238+
args.row &&
239+
sourceChanged &&
240+
args.row.status !== "failed" &&
241+
args.row.status !== "running"
242+
) {
238243
throw new Error(`Migration ${args.migration.tag} changed after it started`);
239244
}
240245
if (args.row) {
@@ -354,7 +359,10 @@ export async function runMigrationJournal(
354359
if (
355360
row &&
356361
row.hash !== migration.hash &&
357-
!(migration.kind === "typescript" && row.status === "failed")
362+
!(
363+
migration.kind === "typescript" &&
364+
(row.status === "failed" || row.status === "running")
365+
)
358366
) {
359367
throw new Error(
360368
`Migration ${migration.tag} changed after it started`,

packages/junior-migrations/tests/runner.test.ts

Lines changed: 60 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -304,60 +304,68 @@ describe("runMigrationJournal", () => {
304304
]);
305305
});
306306

307-
it("restarts a failed TypeScript migration after its source changes", async () => {
308-
const folder = await mixedFolder();
309-
const executor = new FakeExecutor();
310-
let attempts = 0;
311-
let resumedProgress: unknown;
312-
const migration: MigrationV1 = {
313-
apiVersion: 1,
314-
async up(context) {
315-
attempts += 1;
316-
if (attempts === 1) {
317-
await context.progress.save({ cursor: 1 });
318-
throw new Error("needs a source fix");
319-
}
320-
resumedProgress = await context.progress.load();
321-
},
322-
};
323-
const options = {
324-
executor,
325-
migrationsFolder: folder,
326-
migrationsTable: "__drizzle_test",
327-
loadTypeScript: async () => ({ default: migration }),
328-
createContext: ({
329-
progress,
330-
}: {
331-
progress: MigrationContextV1["progress"];
332-
}) => ({
333-
database: executor,
334-
log: () => {},
335-
progress,
336-
state: fakeMigrationState(),
337-
}),
338-
};
307+
it.each(["failed", "running"] as const)(
308+
"restarts a %s TypeScript migration after its source changes",
309+
async (retryStatus) => {
310+
const folder = await mixedFolder();
311+
const executor = new FakeExecutor();
312+
let attempts = 0;
313+
let resumedProgress: unknown;
314+
const migration: MigrationV1 = {
315+
apiVersion: 1,
316+
async up(context) {
317+
attempts += 1;
318+
if (attempts === 1) {
319+
await context.progress.save({ cursor: 1 });
320+
throw new Error("needs a source fix");
321+
}
322+
resumedProgress = await context.progress.load();
323+
},
324+
};
325+
const options = {
326+
executor,
327+
migrationsFolder: folder,
328+
migrationsTable: "__drizzle_test",
329+
loadTypeScript: async () => ({ default: migration }),
330+
createContext: ({
331+
progress,
332+
}: {
333+
progress: MigrationContextV1["progress"];
334+
}) => ({
335+
database: executor,
336+
log: () => {},
337+
progress,
338+
state: fakeMigrationState(),
339+
}),
340+
};
339341

340-
await expect(runMigrationJournal(options)).rejects.toThrow(
341-
"needs a source fix",
342-
);
343-
const failedHash = executor.rows.get(2_001)?.hash;
344-
await writeFile(
345-
join(folder, "0001_data.ts"),
346-
"export default { apiVersion: 1, async up() { return { fixed: true }; } };\n",
347-
);
342+
await expect(runMigrationJournal(options)).rejects.toThrow(
343+
"needs a source fix",
344+
);
345+
const interruptedRow = executor.rows.get(2_001);
346+
expect(interruptedRow?.status).toBe("failed");
347+
if (interruptedRow) {
348+
interruptedRow.status = retryStatus;
349+
}
350+
const failedHash = interruptedRow?.hash;
351+
await writeFile(
352+
join(folder, "0001_data.ts"),
353+
"export default { apiVersion: 1, async up() { return { fixed: true }; } };\n",
354+
);
348355

349-
await expect(runMigrationJournal(options)).resolves.toEqual({
350-
existing: 1,
351-
migrated: 2,
352-
scanned: 3,
353-
skipped: 0,
354-
});
355-
expect(resumedProgress).toBeUndefined();
356-
expect(executor.rows.get(2_001)).toMatchObject({
357-
status: "completed",
358-
});
359-
expect(executor.rows.get(2_001)?.hash).not.toBe(failedHash);
360-
});
356+
await expect(runMigrationJournal(options)).resolves.toEqual({
357+
existing: 1,
358+
migrated: 2,
359+
scanned: 3,
360+
skipped: 0,
361+
});
362+
expect(resumedProgress).toBeUndefined();
363+
expect(executor.rows.get(2_001)).toMatchObject({
364+
status: "completed",
365+
});
366+
expect(executor.rows.get(2_001)?.hash).not.toBe(failedHash);
367+
},
368+
);
361369

362370
it("rejects source changes after a TypeScript migration completes", async () => {
363371
const folder = await mixedFolder();

packages/junior/src/chat/plugins/migrations.ts

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
type TypeScriptMigrationLoader,
88
} from "@sentry/junior-migrations";
99
import type { StateAdapter } from "chat";
10+
import type { RedisStateAdapter } from "@chat-adapter/state-redis";
1011
import { createPluginMigrationStateV1 } from "@/chat/plugins/migration-state";
1112
import type { JuniorSqlMigrationExecutor } from "@/db/db";
1213

@@ -26,7 +27,10 @@ type PluginMigrationResult = {
2627
type PluginMigrationOptions =
2728
| { mode: "schema-bootstrap" }
2829
| {
29-
getStateAdapter: () => Promise<StateAdapter>;
30+
getStateContext: () => Promise<{
31+
redisStateAdapter?: RedisStateAdapter;
32+
stateAdapter: StateAdapter;
33+
}>;
3034
loadTypeScript: TypeScriptMigrationLoader;
3135
log?: MigrationContextV1["log"];
3236
mode: "all";
@@ -165,15 +169,29 @@ export async function migratePluginSchemas(
165169
const pluginResult = runAll
166170
? await runMigrationJournal({
167171
...baseOptions,
168-
createContext: async ({ progress }): Promise<MigrationContextV1> => ({
169-
database: executor,
170-
log: options.log ?? (() => {}),
171-
progress,
172-
state: createPluginMigrationStateV1(
173-
root.pluginName,
174-
await options.getStateAdapter(),
175-
),
176-
}),
172+
createContext: async ({ progress }): Promise<MigrationContextV1> => {
173+
const { redisStateAdapter, stateAdapter } =
174+
await options.getStateContext();
175+
return {
176+
database: executor,
177+
log: options.log ?? (() => {}),
178+
progress,
179+
...(redisStateAdapter
180+
? {
181+
redis: {
182+
sendCommand: async <T>(args: readonly string[]) =>
183+
await redisStateAdapter
184+
.getClient()
185+
.sendCommand<T>([...args]),
186+
},
187+
}
188+
: {}),
189+
state: createPluginMigrationStateV1(
190+
root.pluginName,
191+
stateAdapter,
192+
),
193+
};
194+
},
177195
loadTypeScript: options.loadTypeScript,
178196
mode: "all",
179197
})

packages/junior/src/cli/upgrade/migrations/plugin-journal.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,7 @@ export async function migratePluginJournals(
2424
executor,
2525
pluginCatalogRuntime.getMigrationRoots(),
2626
{
27-
getStateAdapter: async () =>
28-
(await context.getStateContext()).stateAdapter,
27+
getStateContext: context.getStateContext,
2928
loadTypeScript: async (path) =>
3029
await migrationLoader.import<Record<string, unknown>>(path),
3130
log: context.io.info,

packages/junior/tests/component/migrations/mixed-runner-database.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import {
66
type MigrationContextV1,
77
type MigrationStateV1,
88
} from "@sentry/junior-migrations";
9+
import type { RedisStateAdapter } from "@chat-adapter/state-redis";
10+
import type { StateAdapter } from "chat";
11+
import { migratePluginSchemas } from "@/chat/plugins/migrations";
912
import { createJiti } from "jiti";
1013
import { afterEach, describe, expect, it, vi } from "vitest";
1114
import {
@@ -92,6 +95,54 @@ afterEach(async () => {
9295
});
9396

9497
describe("mixed migration runner database contract", () => {
98+
it("provides the host Redis capability to plugin data migrations", async () => {
99+
const fixture = await createLocalJuniorSqlFixture();
100+
const folder = await createMigrationFolder({
101+
source: `
102+
export default {
103+
apiVersion: 1,
104+
async up(context) {
105+
return {
106+
response: await context.redis.sendCommand(["PING"]),
107+
};
108+
},
109+
};
110+
`,
111+
tag: "0000_plugin_redis",
112+
when: 2_026_071_600_000,
113+
});
114+
const sendCommand = vi.fn(async () => "PONG");
115+
116+
try {
117+
await expect(
118+
migratePluginSchemas(
119+
fixture.sql,
120+
[{ dir: folder, pluginName: "redis-test" }],
121+
{
122+
getStateContext: async () => ({
123+
redisStateAdapter: {
124+
getClient: () => ({ sendCommand }),
125+
} as unknown as RedisStateAdapter,
126+
stateAdapter: {} as StateAdapter,
127+
}),
128+
loadTypeScript: async (migrationPath) =>
129+
await migrationLoader.import<Record<string, unknown>>(
130+
migrationPath,
131+
),
132+
mode: "all",
133+
},
134+
),
135+
).resolves.toEqual({
136+
existing: 0,
137+
migrated: 1,
138+
scanned: 1,
139+
});
140+
expect(sendCommand).toHaveBeenCalledWith(["PING"]);
141+
} finally {
142+
await fixture.close();
143+
}
144+
});
145+
95146
it("persists failed progress and resumes a TypeScript migration", async () => {
96147
const fixture = await createLocalJuniorSqlFixture();
97148
const folder = await createMigrationFolder({

0 commit comments

Comments
 (0)