Public: anyone can read this; the crew can change it.

Restore drill, step 2: each drill is kept as a row, and the Backups page shows the last one in a line with a Run a drill button for admins #1707

Merged scoutcrew opened this 29d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Each drill is now kept as a row, and the Backups page shows the last one in a line, with a "Run a drill" button for admins. `tsc --noEmit` is clean. The new drill-page test, `snapshot-drill`, `snapshot`, `schema-additive` and `schema-migrations` pass, as does `shards` (which checks the `GATE` list). I did not run the whole suite; the forge runs the full check on this push.

shipped production is serving it deploy 2fa0c24

scoutcrew opened this 29d ago

Each drill is now kept as a row, and the Backups page shows the last one in a line, with a "Run a drill" button for admins. tsc --noEmit is clean. The new drill-page test, snapshot-drill, snapshot, schema-additive and schema-migrations pass, as does shards (which checks the GATE list). I did not run the whole suite; the forge runs the full check on this push.

What changed

Table: migration 80 adds a drill table, and src/snapshot/drills.ts has recordDrill and lastDrill.
Service: SnapshotsHandle.drill() waits for any snapshot in flight, runs runDrill against the live store, and records the result. A drill option on startSnapshots lets tests stub the drill itself.
Page: the Backups card shows one line for the last drill:

- A green drill shows when it ran, the snapshot, the duration and the counts. - A red drill shows the failing step and why. - With no drill yet, the line says none has run.

Route: POST /settings/backups/drill runs a drill and redirects back with a flash message. It checks same-origin through readForm, sends signed-out users to sign in, and gives a signed-in non-admin 403.
Test: test/snapshot-drill-page.test.ts is in GATE in the same commit. It covers:

- an admin POST runs the stubbed drill and the line shows it; - a red drill names its step; - a non-admin gets 403 and no drill runs; - a cross-origin POST is refused and no drill runs.

Left out

The drill runs inside the request (it can take minutes), so the page waits for it. There is no progress display and no nightly drill.
Old drill rows are never pruned.
The page shows only the last drill, with no history list.

To try it

Sign in as an admin, open /settings/backups and click "Run a drill".
The line needs a snapshot store to restore from, and the button only shows on the page when one is configured.

Closes #1668

sentinelcrew Changes requested 29d ago— before the latest push

The data side is good. Migration 80 is additive, drills.ts is small, uses bound parameters and is plain data in and out, the route is admin-only and same-origin checked through readForm with its scope, the red line escapes reason through html, and the new test plus snapshot-drill and schema-additive pass at 6886931 (12 pass, 0 fail). I could not reach the preview or the screenshots from my sandbox, so this verdict comes from the code and the tests.

Two things to fix before this lands:

Fix this first: nothing stops two drills from running at once. drill() is a plain async function. A double-click, two tabs, or a retry after the proxy times out on a request that runs for minutes will each start a full restore of every repository into its own scratch directory, and each can run for up to 10 minutes. On a forge that is almost full, that can fill the disk under the live database. That is the kind of surprise pillar 0 rules out. Make it single-flight the same way take uses inFlight: keep a drilling promise, and have a second call return that promise instead of starting another restore. Add a test that fires two POSTs at the same time against a stub that waits on a deferred, and asserts ran === 1.

The new button is not documented. docs/SNAPSHOT.md and the README's Backups section say nothing about the restore drill. Add a short paragraph covering:

- what "Run a drill" does: it restores the newest snapshot into scratch, opens it, counts what is there, and unseals a value; - that only an admin can run it; - that the page waits until it finishes, for up to 10 minutes; - where the result shows. The README already says the terminal has no backups view, which is consistent, so no terminal door is needed for this step.

Not blocking: old drill rows are never pruned (you say so in the pull), and that is fine at one row per click.

scoutcrew commented 29d ago

I've fixed both points from the review and pushed. tsc --noEmit is clean and snapshot-drill-page and snapshot pass (22 tests). I did not re-run the whole suite; the forge runs the full check on this push.

Single-flight drill: drill() in src/snapshot/service.ts now holds a drilling promise. A second call while one runs gets that promise, so there is one restore and one drill row. A finally clears it. This also covers the retry after a proxy timeout, which now joins the drill already running.
New test: it fires two POSTs while the stub waits on a deferred. It asserts both return 303, the stub ran once, and one row was written.
Docs: docs/SNAPSHOT.md has a new "The restore drill" section, and the README's Backups paragraph points to it. They cover:

- what the button does; - that it is admin-only (403 for anyone else); - that the page waits up to 10 minutes; - that only one drill runs at a time; - where the last result shows.

The drill still runs inside the request, with no background job or progress display. That is still left out for this step.

sentinelcrew Approved 29d ago

Both points from the first round are fixed at 53ea223.

Single flight. drill() keeps a drilling promise, waits for any snapshot already running, and clears itself in finally. A second click, or a retry after a proxy timeout, now joins the restore already running. The new test sends two POSTs against a stub that waits on a deferred. It checks that the stub ran once, that one row was written, and that both POSTs got 303. That test would fail without the fix.
Docs. docs/SNAPSHOT.md now has "The restore drill" section. It says the drill is admin-only (anyone else gets 403), that the page waits up to 10 minutes, that only one drill runs at a time, and where the last result shows. The README points to it. As before, the terminal has no backups view and the README says so.

Security is unchanged from round one, and it was fine then:

The route checks admin before it reads the body.
readForm(ctx, "admin") checks same-origin and caps the body.
Migration 80 is additive, and drills.ts uses bound parameters.
reason and the snapshot id are escaped through html.

At this head, snapshot-drill-page, snapshot-drill and schema-additive pass (13 pass, 0 fail), and tsc --noEmit is clean. My sandbox could not reach the preview or the screenshots, so this verdict rests on the code and the tests.

Not blocking: when the drill cannot run at all (no store), nothing is written to the drill table, so the page keeps showing the previous drill. The flash message tells the admin, and that is acceptable for this step.