From c3420b1d6c50ad04dcb45403409390b3b72083eb Mon Sep 17 00:00:00 2001 From: James Foster Date: Mon, 24 Aug 2026 16:18:40 -0700 Subject: [PATCH] Stage Grail before stamping it, and stamp only what this run created MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A user's first install died just after "Database created": Setting up GemDB failed: ENOENT: no such file or directory, open '/Users/…/GemDB/grail/.gemdb-grail-stamp' prepareFiles recorded Grail as filed into the database before anything had created /grail for the record to live in. Python support was then never staged, and every retry failed the same way. The same misordering broke upgrades, more quietly. Where the directory did exist, the stamp was written into the PREVIOUS version's copy; grailNeedsUpdate then compared the bundled stamp with the bundled stamp, found them equal, and skipped staging -- so a new version's payload never reached disk, wearing the new version's label, and writeCliScripts never re-ran to refresh bin/gemdb. It could not have worked either way, since stageGrail replaces the directory wholesale and deletes the stamp it was just handed. Neither failure reproduces on a machine that has run an earlier version, which is how both reached a release: every developer machine already had the directory, and the integration suite calls stageGrail itself rather than going through prepare. The order now lives in one named place, stageAndRecordGrail, with the reasons attached. The condition moved too: createDatabase returns {created, preloaded} so the stamp follows this call having made the database from the shipped extent, rather than the extension merely shipping one. An upgrade finds a database carrying whatever Grail was filed into it before, and stamping there would claim an install that never happened; leaving it unstamped is what sends ensureRunning to file the new Grail in. src/__tests__/grailStaging.test.ts starts from a root path that does not exist, which is the condition the existing tests could not express. Two of its five cases fail with the calls reversed -- one reproducing the reported ENOENT verbatim -- and database.test.ts pins the new return. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 9 +++ CLAUDE.md | 19 +++++ src/__integration__/database.test.ts | 14 +++- src/__tests__/grailStaging.test.ts | 114 +++++++++++++++++++++++++++ src/database.ts | 19 ++++- src/grail.ts | 30 +++++++ src/lifecycle.ts | 18 +++-- 7 files changed, 211 insertions(+), 12 deletions(-) create mode 100644 src/__tests__/grailStaging.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c94a96..af3efc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **First-run setup no longer fails right after creating the database.** On a + machine where GemDB had never been installed, setup stopped with + `ENOENT: no such file or directory, open '/grail/.gemdb-grail-stamp'` + and Python support was never staged. It recorded Python as installed before + creating the directory that record lives in — harmless on any machine that + had run an earlier version, which is why it reached a release. Upgrades were + affected too, more quietly: the same misordering made GemDB believe the + Python payload was already current, so a new version's payload was never + written to disk and the `gemdb` command kept its old contents. - **Renaming a notebook no longer costs it its session or its variables.** A notebook is identified by its file URI, so renaming the file used to leave the old session logged in with nobody to claim it — one of ten, gone until diff --git a/CLAUDE.md b/CLAUDE.md index a33481f..1dda2fa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -294,6 +294,25 @@ executeAsync loop clears the stack at the next forwarder stop and throws `Error: KeyboardInterrupt - `. Anything new that evaluates Python should go through that layer, not `execute` directly. +**Stage Grail before stamping it, and stamp only what this run created.** +`stageAndRecordGrail` in `grail.ts` owns that order. Reversed, it broke both +ways at once: on a first install `/grail` does not exist yet, so +writing the stamp threw ENOENT and setup died just after "Database created" +(reported from the field, 2026-08-24); on an upgrade the stamp landed in the +*previous* version's directory, `grailNeedsUpdate` then compared the bundled +stamp against itself and skipped staging, so the old payload stayed on disk +labelled as the new one and `writeCliScripts` never refreshed `bin/gemdb`. It +could not have worked regardless — `stageGrail` replaces the directory +wholesale, stamp included. Neither failure reproduces on a machine that has run +an earlier version, and the integration suite calls `stageGrail` itself rather +than going through `prepare`, which is why `src/__tests__/grailStaging.test.ts` +starts from a root path that does not exist. The condition is equally +load-bearing: `createDatabase` returns `{created, preloaded}` so the stamp +follows *this call having made the database from the shipped extent*, not "the +extension ships an extent" — an upgrade finds a database carrying whatever +Grail was filed into it before, and stamping there would claim an install that +never happened. + **Grail must be staged to a stable directory.** `installGrail` records Grail's own directory _inside the database_, and every session resolves modules relative to it. The extension directory is versioned (`gemdb.gemdb-/`), so it diff --git a/src/__integration__/database.test.ts b/src/__integration__/database.test.ts index 690dafc..e48909c 100644 --- a/src/__integration__/database.test.ts +++ b/src/__integration__/database.test.ts @@ -63,9 +63,19 @@ afterAll(async () => { }); describe.skipIf(!makeFixtureIsPossible())('a real database', () => { - it('creates a database from the engine', () => { - createDatabase(fixture!.engine); + it('creates a database from the engine, and says what it did', () => { + // What it did, not what the extension ships: whether Grail may be recorded + // as filed in turns on this call having made the database from the shipped + // extent. Passing no extension path here means the engine's own extent, so + // Grail is not in it and `preloaded` must say so. + const made = createDatabase(fixture!.engine); expect(databaseExists()).toBe(true); + expect(made).toEqual({ created: true, preloaded: false }); + + // Second call finds it already there and creates nothing — the state an + // upgrade arrives in, where stamping the bundled Grail would be a lie + // about a database that was filed in by some earlier version. + expect(createDatabase(fixture!.engine)).toEqual({ created: false, preloaded: false }); }); it('starts, and reports itself through gslist', async () => { diff --git a/src/__tests__/grailStaging.test.ts b/src/__tests__/grailStaging.test.ts new file mode 100644 index 0000000..f7a4652 --- /dev/null +++ b/src/__tests__/grailStaging.test.ts @@ -0,0 +1,114 @@ +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { __setSetting } from '../__mocks__/vscode'; +import { grailNeedsUpdate, stageAndRecordGrail } from '../grail'; +import { expectedEnginePath, grailPath, grailStampPath, installedGrailStamp } from '../paths'; + +/** + * Getting Grail onto disk, and saying so afterwards. + * + * This is the first-run path, and it is the one place the integration suite + * cannot speak for: those tests call `stageGrail` themselves against a fixture + * that already has a root path. What broke in the field was the *order* of two + * calls on a machine where nothing had been installed before — so these tests + * start from a root path that does not exist, which is the condition that + * matters. + */ + +let root: string; +let ext: string; + +const BUNDLED = 'grail=0.1-2172-gabc\ncommit=abc\nengine=3.7.5\n'; + +/** A stand-in extension directory carrying a Grail payload. */ +function makeExtensionDir(stamp: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemdb-ext-')); + fs.mkdirSync(path.join(dir, 'grail', 'src', 'python'), { recursive: true }); + fs.writeFileSync(path.join(dir, 'grail', 'GRAIL_VERSION'), stamp); + fs.writeFileSync(path.join(dir, 'grail', 'src', 'python', 'marker.py'), '# staged\n'); + fs.mkdirSync(path.join(dir, 'out'), { recursive: true }); + return dir; +} + +beforeEach(() => { + root = path.join(fs.mkdtempSync(path.join(os.tmpdir(), 'gemdb-run-')), 'GemDB'); + ext = makeExtensionDir(BUNDLED); + __setSetting('gemdb.rootPath', root); + // writeCliScripts, which staging calls, refuses without an engine. + fs.mkdirSync(expectedEnginePath(), { recursive: true }); +}); + +afterEach(() => { + fs.rmSync(root, { recursive: true, force: true }); + fs.rmSync(ext, { recursive: true, force: true }); +}); + +describe('staging Grail on a machine that has never had it', () => { + it('does not fail trying to stamp a directory that is not there yet', () => { + // The reported failure: "ENOENT ... open '/grail/.gemdb-grail-stamp'" + // arrived immediately after "Database created", because the stamp was + // written before anything created /grail. + expect(fs.existsSync(grailPath())).toBe(false); + + expect(() => stageAndRecordGrail(ext, true)).not.toThrow(); + + expect(fs.existsSync(path.join(grailPath(), 'src', 'python', 'marker.py'))).toBe(true); + expect(installedGrailStamp()).toBe(BUNDLED.trim()); + }); + + it('leaves no stamp when the database was not made from the shipped extent', () => { + // Then `ensureRunning` files Grail in, which is the work the stamp exists + // to skip; claiming it was done would offer Python against a database + // without any. + stageAndRecordGrail(ext, false); + + expect(fs.existsSync(path.join(grailPath(), 'src', 'python', 'marker.py'))).toBe(true); + expect(fs.existsSync(grailStampPath())).toBe(false); + }); +}); + +describe('staging Grail over a previous version', () => { + beforeEach(() => { + // What an upgrade finds: a complete older Grail, stamped as filed in. + fs.mkdirSync(path.join(grailPath(), 'src', 'python'), { recursive: true }); + fs.writeFileSync(path.join(grailPath(), 'GRAIL_VERSION'), 'grail=0.1-1570-gold\n'); + fs.writeFileSync(path.join(grailPath(), 'src', 'python', 'old-only.py'), '# stale\n'); + fs.writeFileSync(grailStampPath(), 'grail=0.1-1570-gold\n'); + }); + + it('replaces the old payload rather than declaring it current', () => { + // The subtler half of the same bug: stamping first made grailNeedsUpdate + // compare the bundled stamp against itself, so staging was skipped and the + // old Grail stayed on disk wearing the new version's label. + expect(grailNeedsUpdate(ext)).toBe(true); + + stageAndRecordGrail(ext, false); + + expect(fs.existsSync(path.join(grailPath(), 'src', 'python', 'marker.py'))).toBe(true); + expect(fs.existsSync(path.join(grailPath(), 'src', 'python', 'old-only.py'))).toBe(false); + expect(fs.readFileSync(path.join(grailPath(), 'GRAIL_VERSION'), 'utf8')).toBe(BUNDLED); + }); + + it('stages before stamping even when the stamp is warranted', () => { + // The ordering guard. With the calls reversed this passes silently on an + // upgrade — the stamp lands in the old directory, grailNeedsUpdate then + // finds the bundled stamp already installed, and staging never runs, so + // the previous Grail stays on disk labelled as the current one. + stageAndRecordGrail(ext, true); + + expect(fs.existsSync(path.join(grailPath(), 'src', 'python', 'marker.py'))).toBe(true); + expect(fs.existsSync(path.join(grailPath(), 'src', 'python', 'old-only.py'))).toBe(false); + expect(installedGrailStamp()).toBe(BUNDLED.trim()); + }); + + it('drops the old stamp, so the new Grail gets filed into the old database', () => { + // Staging replaces the directory wholesale, stamp included. That is what + // makes isInstalled() false and sends ensureRunning to install the new + // Grail — the upgrade path the stamp is for. + stageAndRecordGrail(ext, false); + + expect(fs.existsSync(grailStampPath())).toBe(false); + }); +}); diff --git a/src/database.ts b/src/database.ts index 9df2a3c..569793f 100644 --- a/src/database.ts +++ b/src/database.ts @@ -19,10 +19,24 @@ import { * is otherwise the same shape Jasper writes, because that is the shape the * engine's tooling expects to find. */ -export function createDatabase(enginePath: string, extensionPath?: string): void { +/** + * What a call to `createDatabase` actually did. + * + * `preloaded` is true only when this call made the database *and* made it from + * the shipped extent — which is the one case where Grail is already filed in + * and the stamp may be written without doing the work. "The extension ships an + * extent" is not the same question: an upgrade finds the database already + * there, carrying whatever Grail was filed into it before. + */ +export interface DatabaseCreation { + created: boolean; + preloaded: boolean; +} + +export function createDatabase(enginePath: string, extensionPath?: string): DatabaseCreation { if (databaseExists()) { log(`Database already exists at ${databasePath()}`); - return; + return { created: false, preloaded: false }; } logStep('Creating the database'); @@ -111,6 +125,7 @@ export function createDatabase(enginePath: string, extensionPath?: string): void fs.chmodSync(extentPath(), 0o644); log(`Database created at ${dbPath}`); + return { created: true, preloaded: extentSource.preloaded }; } /** Delete the database directory, extent and all. */ diff --git a/src/grail.ts b/src/grail.ts index 7062f5f..109404a 100644 --- a/src/grail.ts +++ b/src/grail.ts @@ -109,6 +109,36 @@ export function stageGrail(extensionPath: string): void { log(`Grail staged at ${dest}`); } +/** + * Put Grail on disk and, when the database was just made from the shipped + * extent, record that it is filed in. In that order, which is the whole point. + * + * The two steps were once the other way round, and each of the three things + * that went wrong is worth naming, because none of them showed up on a machine + * that had run an earlier version: + * + * - On a first install `/grail` does not exist yet, so writing the + * stamp into it threw ENOENT and setup died right after creating the + * database. Reported from the field; every developer machine already had + * the directory, and the integration suite stages Grail itself rather than + * going through this path. + * - On an upgrade the stamp *could* be written, into the previous version's + * directory — at which point `grailNeedsUpdate` compared the bundled stamp + * with the bundled stamp, found them equal, and skipped staging. The old + * Grail then stayed on disk indefinitely, labelled as the new one, with + * `writeCliScripts` never re-running to refresh `bin/gemdb`. + * - And it could not have worked anyway: `stageGrail` replaces the directory + * wholesale, so it deletes the stamp that was just written into it. + * + * Staging last also gives the upgrade case the behaviour the stamp is for: the + * fresh copy arrives without a stamp, `isInstalled()` is false, and + * `ensureRunning` files the new Grail into the existing database. + */ +export function stageAndRecordGrail(extensionPath: string, fromPreloadedExtent: boolean): void { + if (grailNeedsUpdate(extensionPath)) stageGrail(extensionPath); + if (fromPreloadedExtent) recordGrailInstalled(extensionPath); +} + /** * Record that the staged Grail is now filed into the database. * diff --git a/src/lifecycle.ts b/src/lifecycle.ts index 256a8cd..0e9eeed 100644 --- a/src/lifecycle.ts +++ b/src/lifecycle.ts @@ -8,6 +8,7 @@ import { grailLabel, grailNeedsUpdate, recordGrailInstalled, + stageAndRecordGrail, installGrail, stageGrail, bundledGrailStamp, @@ -21,7 +22,6 @@ import { grailInstalled, grailPath, grailStagedOnDisk, - bundledExtentPath, } from './paths'; import { findNetldi, @@ -80,14 +80,16 @@ async function prepareFiles( if (token.isCancellationRequested) return; progress.report({ message: 'Creating your database…' }); - createDatabase(engine, extensionPath); - // A database made from the shipped extent already has Grail in it, so record - // that here. Without the stamp `ensureRunning` would file it in again on - // first use — several minutes of work to arrive where we already are. - if (fs.existsSync(bundledExtentPath(extensionPath))) recordGrailInstalled(extensionPath); - + const database = createDatabase(engine, extensionPath); + + // Stage Grail, then — only if this run made the database from the shipped + // extent — record that it is already filed in, saving the several minutes + // `ensureRunning` would otherwise spend filing it in on first use. The order + // matters and the reasons are in stageAndRecordGrail; so does the condition, + // which asks what this call did rather than what the extension ships, since + // an upgrade finds a database carrying whatever Grail it was built with. progress.report({ message: 'Preparing Python support…' }); - if (grailNeedsUpdate(extensionPath)) stageGrail(extensionPath); + stageAndRecordGrail(extensionPath, database.created && database.preloaded); } /** Guard against a build that forgot to run `npm run bundle:grail`. */