Skip to content

Commit 753b8d1

Browse files
Brooooooklynclaude
Brooooooklyn
and
claude
committed
fix(cloudflare): name only the migrations a restore point precedes
Two overlapping deploys could both see a migration pending. If the other deploy applied it before this one took its bookmark, this deploy still printed that bookmark as the way to undo the migration, though restoring it would leave the migration in place. Deploy now reads the pending set again after it takes the bookmark and names only the migrations still pending then. When none remain, it prints no restore command. It refuses to migrate if that second read fails. The interactive `void project` list also offers `--platform cloudflare` as a rollback flag, instead of reading it as part of the deployment ID. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 3d27796 commit 753b8d1

5 files changed

Lines changed: 137 additions & 24 deletions

File tree

‎packages/void/src/cli/deploy-migrations.ts‎

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -626,22 +626,29 @@ export async function applyAndVerifyRemoteD1Migrations(
626626
}
627627
// A Worker rollback does not undo a migration. Time Travel does, and a bookmark taken right
628628
// before the apply restores exactly the state the migrations started from.
629-
const before = await Promise.resolve(
630-
runWrangler(
631-
{ type: 'd1.migrations.list', database: d1MigrateToken, configPath },
632-
{ capture: true },
633-
),
634-
);
635-
const pendingBefore = (before.data as { pending?: unknown } | undefined)?.pending;
636-
if (!isCleanCloudflareExit(before) || !Array.isArray(pendingBefore)) {
629+
const readPending = async (): Promise<Array<unknown> | undefined> => {
630+
const listed = await Promise.resolve(
631+
runWrangler(
632+
{ type: 'd1.migrations.list', database: d1MigrateToken, configPath },
633+
{ capture: true },
634+
),
635+
);
636+
const pending = (listed.data as { pending?: unknown } | undefined)?.pending;
637+
if (isCleanCloudflareExit(listed) && Array.isArray(pending)) {
638+
return pending;
639+
}
637640
log.error(
638641
`deploy: could not read the pending D1 migrations for "${d1MigrateToken}" before applying them ` +
639-
(isCleanCloudflareExit(before)
642+
(isCleanCloudflareExit(listed)
640643
? '(Cloudflare returned no pending migration list). '
641-
: `(exit ${String(before.status)}). `) +
644+
: `(exit ${String(listed.status)}). `) +
642645
'Refusing to migrate without knowing whether a restore point is needed. Rerun once Cloudflare can be reached.' +
643-
cloudflareReason(before),
646+
cloudflareReason(listed),
644647
);
648+
return undefined;
649+
};
650+
const pendingBefore = await readPending();
651+
if (!pendingBefore) {
645652
return false;
646653
}
647654
let restoreHint = '';
@@ -662,12 +669,20 @@ export async function applyAndVerifyRemoteD1Migrations(
662669
);
663670
return false;
664671
}
665-
restoreHint =
666-
'\nTo undo these migrations, restore this point (discards every later write):\n' +
667-
` void db restore${restorePlatformFlag ? ' --platform cloudflare' : ''} --bookmark=${bookmark}`;
668-
log.info(
669-
`Saved a D1 restore point before applying ${formatCount(pendingBefore.length, 'migration')}.${restoreHint}`,
670-
);
672+
// An overlapping deploy may have applied some of them before the bookmark. Whatever is still
673+
// pending now is applied after it, so the bookmark undoes exactly those.
674+
const pendingAfterBookmark = await readPending();
675+
if (!pendingAfterBookmark) {
676+
return false;
677+
}
678+
if (pendingAfterBookmark.length > 0) {
679+
restoreHint =
680+
'\nTo undo these migrations, restore this point (discards every later write):\n' +
681+
` void db restore${restorePlatformFlag ? ' --platform cloudflare' : ''} --bookmark=${bookmark}`;
682+
log.info(
683+
`Saved a D1 restore point before applying ${formatCount(pendingAfterBookmark.length, 'migration')}.${restoreHint}`,
684+
);
685+
}
671686
if (!assertUploaderStillPinned()) {
672687
return false;
673688
}

‎packages/void/src/cli/project-args.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,15 @@ export const projectSubcommands: Array<SubcommandDef> = [
3232
{ name: 'requests', description: 'Show request-level traffic (status, method, timing)' },
3333
{
3434
name: 'rollback',
35-
arg: '[deployId] [--platform cloudflare]',
35+
arg: '[deployId]',
3636
description: 'Roll back to a previous deployment',
37+
flags: [
38+
{
39+
name: '--platform',
40+
arg: 'cloudflare',
41+
description: 'Roll back after a one-off deploy with --platform cloudflare',
42+
},
43+
],
3744
},
3845
{ name: 'cancel', arg: '[deployId]', description: 'Cancel an active deployment' },
3946
{

‎packages/void/test/unit/cloudflare-deploy.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7948,6 +7948,7 @@ describe('runCloudflareDeploy — remote migration gates', () => {
79487948
expect(localTrace.filter((entry) => entry.startsWith('wrangler:d1:'))).toEqual([
79497949
'wrangler:d1:list',
79507950
'wrangler:d1:info',
7951+
'wrangler:d1:list',
79517952
'wrangler:d1:apply',
79527953
'wrangler:d1:list',
79537954
]);

‎packages/void/test/unit/deploy-migrations-restore-point.test.ts‎

Lines changed: 81 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,11 @@ beforeEach(() => {
3838
describe('the D1 restore point a deploy takes before migrating', () => {
3939
it('saves a restore point before applying pending migrations, and names how to restore it', async () => {
4040
const runner = recordingRunner({
41-
'd1.migrations.list': [pending(['0001_add_column.sql']), pending([])],
41+
'd1.migrations.list': [
42+
pending(['0001_add_column.sql']),
43+
pending(['0001_add_column.sql']),
44+
pending([]),
45+
],
4246
'd1.time-travel.bookmark': [{ ...clean, data: { bookmark } }],
4347
});
4448

@@ -53,6 +57,7 @@ describe('the D1 restore point a deploy takes before migrating', () => {
5357
expect(runner.calls).toEqual([
5458
'd1.migrations.list',
5559
'd1.time-travel.bookmark',
60+
'd1.migrations.list',
5661
'd1.migrations.apply',
5762
'd1.migrations.list',
5863
]);
@@ -113,7 +118,7 @@ describe('the D1 restore point a deploy takes before migrating', () => {
113118

114119
it('checks the uploader again after the restore point, before applying', async () => {
115120
const runner = recordingRunner({
116-
'd1.migrations.list': [pending(['0001_add_column.sql'])],
121+
'd1.migrations.list': [pending(['0001_add_column.sql']), pending(['0001_add_column.sql'])],
117122
'd1.time-travel.bookmark': [{ ...clean, data: { bookmark } }],
118123
});
119124
const stillPinned = vi.fn().mockReturnValueOnce(true).mockReturnValueOnce(false);
@@ -126,7 +131,11 @@ describe('the D1 restore point a deploy takes before migrating', () => {
126131
);
127132

128133
expect(applied).toBe(false);
129-
expect(runner.calls).toEqual(['d1.migrations.list', 'd1.time-travel.bookmark']);
134+
expect(runner.calls).toEqual([
135+
'd1.migrations.list',
136+
'd1.time-travel.bookmark',
137+
'd1.migrations.list',
138+
]);
130139
});
131140

132141
it.each([
@@ -137,7 +146,11 @@ describe('the D1 restore point a deploy takes before migrating', () => {
137146
{ name: 'the uploader changed after the apply', pinnedChecks: 2 },
138147
])('repeats the restore command when $name', async ({ apply, verify, pinnedChecks }) => {
139148
const runner = recordingRunner({
140-
'd1.migrations.list': [pending(['0001_add_column.sql']), verify ?? pending([])],
149+
'd1.migrations.list': [
150+
pending(['0001_add_column.sql']),
151+
pending(['0001_add_column.sql']),
152+
verify ?? pending([]),
153+
],
141154
'd1.time-travel.bookmark': [{ ...clean, data: { bookmark } }],
142155
'd1.migrations.apply': [apply ?? clean],
143156
});
@@ -156,7 +169,11 @@ describe('the D1 restore point a deploy takes before migrating', () => {
156169

157170
it('names the platform in the restore command after a one-off Cloudflare deploy', async () => {
158171
const runner = recordingRunner({
159-
'd1.migrations.list': [pending(['0001_add_column.sql']), pending([])],
172+
'd1.migrations.list': [
173+
pending(['0001_add_column.sql']),
174+
pending(['0001_add_column.sql']),
175+
pending([]),
176+
],
160177
'd1.time-travel.bookmark': [{ ...clean, data: { bookmark } }],
161178
});
162179

@@ -167,6 +184,65 @@ describe('the D1 restore point a deploy takes before migrating', () => {
167184
);
168185
});
169186

187+
it('names only the migrations still pending after the restore point', async () => {
188+
// An overlapping deploy applied 0001 between the first read and the bookmark.
189+
const runner = recordingRunner({
190+
'd1.migrations.list': [
191+
pending(['0001_add_column.sql', '0002_add_index.sql']),
192+
pending(['0002_add_index.sql']),
193+
pending([]),
194+
],
195+
'd1.time-travel.bookmark': [{ ...clean, data: { bookmark } }],
196+
});
197+
198+
await applyAndVerifyRemoteD1Migrations('app-db', 'wrangler.json', runner.run, () => true);
199+
200+
expect(vi.mocked(log.info)).toHaveBeenCalledWith(
201+
expect.stringContaining('Saved a D1 restore point before applying 1 migration.'),
202+
);
203+
});
204+
205+
it('offers no restore point when an overlapping deploy applied every pending migration', async () => {
206+
const runner = recordingRunner({
207+
'd1.migrations.list': [pending(['0001_add_column.sql']), pending([]), pending([])],
208+
'd1.time-travel.bookmark': [{ ...clean, data: { bookmark } }],
209+
'd1.migrations.apply': [{ status: 1, stdout: '', stderr: '' }],
210+
});
211+
212+
const applied = await applyAndVerifyRemoteD1Migrations(
213+
'app-db',
214+
'wrangler.json',
215+
runner.run,
216+
() => true,
217+
);
218+
219+
// The bookmark is later than those migrations, so restoring it would not undo them.
220+
expect(applied).toBe(false);
221+
expect(vi.mocked(log.info)).not.toHaveBeenCalled();
222+
expect(errorText()).not.toContain('void db restore');
223+
});
224+
225+
it('refuses to migrate when the pending list cannot be read after the restore point', async () => {
226+
const runner = recordingRunner({
227+
'd1.migrations.list': [
228+
pending(['0001_add_column.sql']),
229+
{ status: 1, stdout: '', stderr: 'Authentication error' },
230+
],
231+
'd1.time-travel.bookmark': [{ ...clean, data: { bookmark } }],
232+
});
233+
234+
const applied = await applyAndVerifyRemoteD1Migrations(
235+
'app-db',
236+
'wrangler.json',
237+
runner.run,
238+
() => true,
239+
);
240+
241+
expect(applied).toBe(false);
242+
expect(runner.calls).not.toContain('d1.migrations.apply');
243+
expect(errorText()).toContain('Cloudflare: Authentication error');
244+
});
245+
170246
it('saves no restore point when no migration is pending', async () => {
171247
const runner = recordingRunner({ 'd1.migrations.list': [pending([]), pending([])] });
172248

‎packages/void/test/unit/project-args.test.ts‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, expect, it } from 'vite-plus/test';
2-
import { parseProjectArgs } from '../../src/cli/project-args.ts';
2+
import { parseProjectArgs, projectSubcommands } from '../../src/cli/project-args.ts';
33

44
describe('parseProjectArgs', () => {
55
describe('token', () => {
@@ -332,6 +332,20 @@ describe('parseProjectArgs', () => {
332332
});
333333
});
334334

335+
it('offers --platform as a rollback flag in the interactive list', () => {
336+
const rollback = projectSubcommands.find((entry) => entry.name === 'rollback');
337+
expect(rollback?.arg).toBe('[deployId]');
338+
expect(rollback?.flags).toEqual([
339+
expect.objectContaining({ name: '--platform', arg: 'cloudflare' }),
340+
]);
341+
// The prompt passes the id and each flag value as separate argv items.
342+
expect(parseProjectArgs(['rollback', 'abc123', '--platform', 'cloudflare'])).toEqual({
343+
subcommand: 'rollback',
344+
deployId: 'abc123',
345+
platform: 'cloudflare',
346+
});
347+
});
348+
335349
it.each([['void'], ['']])('rejects rollback --platform %o', (value) => {
336350
expect(() => parseProjectArgs(['rollback', `--platform=${value}`])).toThrow(
337351
"accepts only 'cloudflare'",

0 commit comments

Comments
 (0)