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`. */