diff --git a/CHANGELOG.md b/CHANGELOG.md index 118495a..7c94a96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,13 +10,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **Sessions now say who they belong to.** Every session GemDB opens names - itself in the database's shared cache — `gemdb nb analysis` for a notebook, - `gemdb sh 41234` for a GemDB Shell, `gemdb run backfill` for a script started - with `gemdb backfill.py`. The names are visible to anything attached to the - same database, so a second VS Code window, topaz, or an administrator's tool - can tell which of the ten sessions belongs to what, instead of seeing a row - of anonymous gems. Nothing is committed to do this and the entry disappears - with the process, so a window that crashes leaves nothing behind. + itself in the database's shared cache — `GemDB nb analysis` for a notebook, + `GemDB Shell 41234` for a GemDB Shell, `GemDB Code` for the extension's own, + and `GemDB run backfill` for a script started with `gemdb backfill.py`. The + names are visible to anything attached to the same database, so a second VS + Code window, topaz, or an administrator's tool can tell which of the ten + sessions belongs to what, instead of seeing a row of anonymous gems. Nothing + is committed to do this and the entry disappears with the process, so a + window that crashes leaves nothing behind. ### Changed @@ -32,6 +33,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **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 + the window closed — while the notebook itself started over with an empty + namespace, which looked like its variables had vanished. A rename now carries + the session, the variables and the session's published name across. - **A notebook no longer keeps a view of a database that has been replaced.** Reinstalling Python support, uninstalling, and changing the root path or engine version now log out every session rather than only the extension's diff --git a/CLAUDE.md b/CLAUDE.md index 66666be..3e39d09 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,7 +168,28 @@ characters** — 32 raises `OutOfRange` (2061), measured — so the name is a label and the sessionId remains the identifier; `cacheNameFor` truncates and strips non-ASCII rather than letting a long notebook title fail a login. File mode needs its own copy of this in `gemdb-run.tpz`, because linked topaz never -reaches `session.ts` and would otherwise show as the stock `TopazL`. +reaches `session.ts` and would otherwise show as the stock `TopazL` (an +unnamed RPC gem is `TopazR` — also measured). + +Names read `GemDB nb analysis`, `GemDB Shell 41234`, `GemDB Code`, +`GemDB run backfill`. The product names are capitalised as the product is +written, per the GemDB Shell rule above — an administrator reading a session +list is a user — while `nb` and `run` are common nouns and stay lowercase. +`nb` is abbreviated where `Shell` is spelled out because a shell's suffix is a +fixed-width pid while a notebook's is a filename, and every character the tag +takes is one the title loses: 22 against 16. GemStone's own names in that +column are PascalCase (`GcReclaim`, `SymbolGem`, `ShrPcMonitor`, `TopazR`); +the one lowercase entry is the stone's slot, which carries the stone's +configured name rather than a product's, so it is not a counter-example. + +**A notebook's URI is its session key _and_ its namespace key, so a rename +moves both.** `renameOwner` in `pythonQueries.ts` is that move — it re-keys the +scope dictionary inside the session, then `renameSession` re-keys the map and +re-publishes the cache name. Without it a rename strands the old session +(logged in, spending one of ten, owned by a URI nothing will ask for again) and +hands the notebook an empty namespace, which reads as lost variables. Wired to +`onDidRenameFiles`, which is explicit renames only; saving under a new name +makes a second document and correctly gets a session of its own. Two things the GCI headers say, so nobody goes looking again: there is no `GciInit` equivalent in the thread-safe library at all, `GciInitAppName` "has diff --git a/src/__integration__/sessions.test.ts b/src/__integration__/sessions.test.ts index 28ab7ba..be5b27a 100644 --- a/src/__integration__/sessions.test.ts +++ b/src/__integration__/sessions.test.ts @@ -4,7 +4,7 @@ import { createDatabase } from '../database'; import { stageGrail } from '../grail'; import { bundledExtentPath } from '../paths'; import { isRunning, startNetldi, startStone, stopNetldi, stopStone } from '../processes'; -import { isErrorResult, runPython } from '../pythonQueries'; +import { isErrorResult, renameOwner, runPython } from '../pythonQueries'; import { SessionOwner, cacheNameFor, execute, logoutAll, sessionRegistry } from '../session'; import { Fixture, makeFixture } from './fixture'; @@ -171,16 +171,45 @@ describe.skipIf(!havePreloaded || !canMakeFixture())('a session per notebook', ( }), ); - expect(cacheNameFor(A)).toBe('gemdb nb one'); - expect([...slots.keys()]).toEqual(expect.arrayContaining(['gemdb nb one', 'gemdb nb two'])); + expect(cacheNameFor(A)).toBe('GemDB nb one'); + expect([...slots.keys()]).toEqual(expect.arrayContaining(['GemDB nb one', 'GemDB nb two'])); // `execute` runs in the extension's own session, so it named itself too. - expect(slots.has('gemdb ext')).toBe(true); + expect(slots.has('GemDB Code')).toBe(true); // The join a dashboard needs: the cache slot's session id is the number // this side recorded at login, so a name over there resolves to an owner // over here. const held = new Map(sessionRegistry().map((s) => [s.owner.key, s])); - expect(slots.get('gemdb nb one')).toBe(held.get(A.key)?.serial); - expect(slots.get('gemdb nb two')).toBe(held.get(B.key)?.serial); + expect(slots.get('GemDB nb one')).toBe(held.get(A.key)?.serial); + expect(slots.get('GemDB nb two')).toBe(held.get(B.key)?.serial); + }); + + it('carries a renamed notebook’s session, variables and name across', async () => { + const before = notebook('before-rename'); + const after = notebook('after-rename'); + + await runPython('renamed_marker = 99', before); + const originalSerial = sessionRegistry().find((s) => s.owner.key === before.key)?.serial; + expect(typeof originalSerial).toBe('number'); + const openSessions = sessionRegistry().length; + + renameOwner(before.key, after); + + // The same session, not a second one: a rename that logged in again would + // spend one of ten and strand the first under a URI nothing asks for. + expect(sessionRegistry()).toHaveLength(openSessions); + expect(sessionRegistry().find((s) => s.owner.key === after.key)?.serial).toBe(originalSerial); + expect(sessionRegistry().some((s) => s.owner.key === before.key)).toBe(false); + + // The variables came too. Without moving the scope entry the notebook + // would get an empty namespace, which reads as "my variables disappeared". + expect((await runPython('renamed_marker', after)).value).toBe('99'); + + // And the shared cache tells the truth about who holds it now — a stale + // name points an administrator at a file that is not there. + const name = execute( + `((System cacheStatisticsForSessionId: ${originalSerial}) at: 1) encodeAsUTF8`, + ); + expect(name).toBe('GemDB nb after-rename'); }); }); diff --git a/src/__tests__/cli.test.ts b/src/__tests__/cli.test.ts index a2af630..96f0c15 100644 --- a/src/__tests__/cli.test.ts +++ b/src/__tests__/cli.test.ts @@ -79,7 +79,7 @@ describe('writeCliScripts', () => { // otherwise sit in the shared cache as the stock 'TopazL'. The truncation // is not optional: 32 characters raises OutOfRange, at login. expect(run).toContain('System cacheName: label'); - expect(run).toContain("label := 'gemdb run ', label"); + expect(run).toContain("label := 'GemDB run ', label"); expect(run).toContain('label size > 31 ifTrue:'); }); diff --git a/src/__tests__/sessions.test.ts b/src/__tests__/sessions.test.ts index 7c093fe..c25a4e7 100644 --- a/src/__tests__/sessions.test.ts +++ b/src/__tests__/sessions.test.ts @@ -78,34 +78,43 @@ describe('the name a session publishes to the shared cache', () => { }); it('names a notebook, without repeating what the tag already said', () => { - expect(cacheNameFor(nb('analysis.ipynb'))).toBe('gemdb nb analysis'); + expect(cacheNameFor(nb('analysis.ipynb'))).toBe('GemDB nb analysis'); }); - it('names a shell by its process, which is what leads back to the terminal', () => { + it('spells out the shell, which is a product name and has room', () => { + // "GemDB Shell" is what a user is told this thing is called, and an + // administrator reading a session list is a user. The pid is fixed-width, + // so spelling out the tag costs a notebook name nothing. expect(cacheNameFor({ key: 'shell', kind: 'shell', label: 'shell' }, 41234)).toBe( - 'gemdb sh 41234', + 'GemDB Shell 41234', ); }); - it('names the extension’s own session', () => { + it('names the extension’s own session after the extension', () => { expect(cacheNameFor({ key: 'gemdb.extension', kind: 'extension', label: 'GemDB' })).toBe( - 'gemdb ext', + 'GemDB Code', ); }); it('never exceeds what the cache will take', () => { const long = nb('a-notebook-with-a-really-quite-long-name.ipynb'); expect(cacheNameFor(long).length).toBeLessThanOrEqual(31); - expect(cacheNameFor(long)).toBe('gemdb nb a-notebook-with-a-real'); + expect(cacheNameFor(long)).toBe('GemDB nb a-notebook-with-a-real'); + }); + + it('leaves the notebook’s own capitalisation alone', () => { + // The tag is ours to style; the title is the user's, and altering its case + // would make the label harder to match against the file they can see. + expect(cacheNameFor(nb('Q3-Revenue.ipynb'))).toBe('GemDB nb Q3-Revenue'); }); it('drops what the cache cannot store', () => { // The cache holds 8-bit code points, so an emoji or a CJK title would be // meaningless there at best; strip rather than let login fail over it. - expect(cacheNameFor(nb('sales–📈.ipynb'))).toBe('gemdb nb sales'); + expect(cacheNameFor(nb('sales–📈.ipynb'))).toBe('GemDB nb sales'); }); it('falls back to the tag when nothing usable is left', () => { - expect(cacheNameFor(nb('📈.ipynb'))).toBe('gemdb nb'); + expect(cacheNameFor(nb('📈.ipynb'))).toBe('GemDB nb'); }); }); diff --git a/src/cli.ts b/src/cli.ts index d8a047e..39c6c64 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -171,7 +171,7 @@ SessionTemps current at: #'GrailConsole' put: (Array with: GsFile stdout). ifFalse: [(target subStrings: '/') last]]. (label size > 3 and: [(label copyFrom: label size - 2 to: label size) = '.py']) ifTrue: [label := label copyFrom: 1 to: label size - 3]. - label := 'gemdb run ', label. + label := 'GemDB run ', label. label size > 31 ifTrue: [label := label copyFrom: 1 to: 31]. [System cacheName: label] on: Error do: [:ignored | ignored return: nil]. target = '-m' diff --git a/src/extension.ts b/src/extension.ts index fb4c2ab..c9bb98f 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -18,11 +18,13 @@ import { GemDbNotebookController, newNotebook, notebookOwner, + notebookOwnerForUri, resetActiveNotebook, } from './notebook'; import { configureSharedMemory, ensureOsConfigured, isSharedMemoryConfigured } from './osConfig'; import { isSupportedPlatform, setContext } from './platform'; import { isRunning } from './processes'; +import { renameOwner } from './pythonQueries'; import { openRepl, runFile } from './repl'; import { closeSessionFor, logoutAll, setInputHandler } from './session'; import { GemDbStatusBar } from './statusBar'; @@ -59,6 +61,26 @@ export function activate(context: vscode.ExtensionContext): void { }), ); + // A renamed notebook keeps its session and its variables. Both are keyed by + // the notebook's URI, so without this a rename would strand the old session + // — still logged in, spending one of ten, owned by a URI nothing will ask + // for again — and hand the notebook an empty namespace, which reads as "my + // variables disappeared". It also re-publishes the session's name, because a + // stale one points an administrator at a file that is not there. + // + // Only explicit renames arrive here: this is `onDidRenameFiles`, which VS + // Code raises for a rename it performed (the explorer, a refactor). Saving + // under a new name is a different act — it makes a second document — and is + // correctly left to log in a session of its own. + context.subscriptions.push( + vscode.workspace.onDidRenameFiles((event) => { + for (const { oldUri, newUri } of event.files) { + if (!newUri.path.endsWith('.ipynb')) continue; + renameOwner(oldUri.toString(), notebookOwnerForUri(newUri)); + } + }), + ); + // Python's input() in a notebook cell becomes an input box — the same move // Jupyter makes for stdin requests. Escape cancels the read, and the cell's // interrupt button closes the box; either way input() raises diff --git a/src/notebook.ts b/src/notebook.ts index cf863ff..1cb4ecb 100644 --- a/src/notebook.ts +++ b/src/notebook.ts @@ -7,17 +7,29 @@ import { SessionOwner, interruptSessionFor } from './session'; /** * Which session a notebook owns. * - * The URI is the identity — stable across renames of the window, unique per - * document — and the file name is what a person should see in a message about - * sessions being scarce. `resetScope` keys the notebook's globals by the same - * string, so a notebook's namespace and its session are named alike. + * The URI is the identity — unique per document — and the file name is what a + * person should see in a message about sessions being scarce. `resetScope` + * keys the notebook's globals by the same string, so a notebook's namespace + * and its session are named alike. + * + * Renaming the file therefore changes both, which is why `renameOwner` exists: + * a rename must carry the session and the globals across to the new URI rather + * than abandon them under the old one. */ export function notebookOwner(notebook: vscode.NotebookDocument): SessionOwner { - const uri = notebook.uri.toString(); + return notebookOwnerForUri(notebook.uri); +} + +/** + * The same, from a URI alone — what a rename has, since the event reports + * files rather than open documents. + */ +export function notebookOwnerForUri(uri: vscode.Uri): SessionOwner { + const key = uri.toString(); return { - key: uri, + key, kind: 'notebook', - label: uri.split('/').pop() || 'notebook', + label: key.split('/').pop() || 'notebook', }; } diff --git a/src/pythonQueries.ts b/src/pythonQueries.ts index b4c2b42..2ffced3 100644 --- a/src/pythonQueries.ts +++ b/src/pythonQueries.ts @@ -5,6 +5,7 @@ import { SessionOwner, execute, executeAsync, + renameSession, sessionFor, sessionForIfOpen, } from './session'; @@ -183,6 +184,34 @@ scopes ifNotNil: [scopes removeKey: '${scope}' ifAbsent: []]. ); } +/** + * Follow a renamed notebook: its session, and the variables inside it. + * + * A notebook's URI is both its session key and the name of its globals, so + * renaming the file would otherwise strand the session and hand the notebook + * a fresh, empty namespace — the rename would read as "my variables + * disappeared". Moving the scope entry keeps `x` defined across a rename, and + * `renameSession` keeps the session and re-publishes its name. + * + * Does nothing when that notebook has no session yet, which is the common + * case: renaming a notebook nobody has run has nothing to carry over. + */ +export function renameOwner(oldKey: string, newOwner: SessionOwner): void { + const session = sessionForIfOpen(oldKey); + if (!session) return; + session.execute( + `| scopes moved | +scopes := SessionTemps current at: #'__gemdbScopes' ifAbsent: [nil]. +scopes ifNotNil: [ + moved := scopes at: '${escapeString(oldKey)}' ifAbsent: [nil]. + moved ifNotNil: [ + scopes removeKey: '${escapeString(oldKey)}' ifAbsent: []. + scopes at: '${escapeString(newOwner.key)}' put: moved]]. +'renamed' encodeAsUTF8`, + ); + renameSession(oldKey, newOwner); +} + /** Is Grail installed in this database? */ export function isGrailInstalled(): boolean { return ( diff --git a/src/session.ts b/src/session.ts index c62bc01..f39229e 100644 --- a/src/session.ts +++ b/src/session.ts @@ -78,11 +78,29 @@ export interface SessionOwner { */ const CACHE_NAME_LIMIT = 31; -/** How each kind of owner introduces itself in that list. */ +/** + * How each kind of owner introduces itself in that list. + * + * Capitalised the way the product is written, because this is a string a + * person reads: "GemDB Shell" is the name of a thing (CLAUDE.md: write it that + * way wherever a user can see it, and an administrator reading a session list + * is a user), while "nb" and "run" are common nouns describing a role. It also + * sits better beside GemStone's own `GcReclaim`, `SymbolGem` and `TopazR` than + * an all-lowercase name would — the one lowercase entry in that column is the + * stone's slot, which carries the stone's configured name rather than a + * product's. + * + * `nb` stays abbreviated where `Shell` was spelled out, and the asymmetry is + * deliberate: a shell's suffix is a pid, five or six characters and fixed, so + * spelling out the tag costs nothing. A notebook's suffix is a filename of + * unknown length, and every character the tag takes is one the name loses. + * `GemDB nb ` leaves 22 for the title; `GemDB notebook ` would leave 16, which + * ordinary names exceed. + */ const CACHE_NAME_TAGS: Record = { - notebook: 'gemdb nb', - shell: 'gemdb sh', - extension: 'gemdb ext', + notebook: 'GemDB nb', + shell: 'GemDB Shell', + extension: 'GemDB Code', }; /** @@ -95,8 +113,8 @@ const CACHE_NAME_TAGS: Record = { * GemDB session is anonymous, and "which of these ten is worth closing" has no * answer from outside the window that opened it. * - * The `gemdb` prefix earns its six characters on a database this extension did - * not install alone — it is the only thing saying which client a session + * The `GemDB` prefix earns its five characters on a database this extension + * did not install alone — it is the only thing saying which client a session * belongs to. Shells report their pid because a shell is its own process and * that is what leads back to the terminal; in the extension host `process.pid` * would be the same number for every notebook, which is why only 'shell' uses @@ -305,12 +323,47 @@ export class GciSession { private constructor( private readonly gci: GciLibrary, private handle: unknown | undefined, - readonly owner: SessionOwner, + private currentOwner: SessionOwner, ) {} + /** Who this session belongs to — a notebook's URI, or the extension itself. */ + get owner(): SessionOwner { + return this.currentOwner; + } + /** What the logs call this session. */ get label(): string { - return this.owner.label; + return this.currentOwner.label; + } + + /** + * Take on a new owner — a renamed notebook — keeping the session. The name + * in the shared cache is re-sent, since the old one now points at a file + * that is not there. + */ + rename(owner: SessionOwner): void { + this.currentOwner = owner; + this.publishName(); + } + + /** + * Publish who holds this session, where anything on the host can read it. + * + * Everything the registry knows is trapped in one extension host; + * `System cacheName:` is what makes it visible to another window, to topaz, + * and to whatever ends up reporting on sessions. It writes shared memory + * rather than the repository, so it costs no commit and — the reason it + * beats a committed registry — the entry dies with the process instead of + * outliving a window that crashed. Best-effort: an unnamed session works. + */ + private publishName(): void { + try { + this.execute( + `System cacheName: '${cacheNameFor(this.currentOwner).replace(/'/g, "''")}'. true`, + ); + } catch (e) { + log(`Could not name the session (${this.label}): ${e instanceof Error ? e.message : e}`); + } } /** @@ -393,19 +446,7 @@ export class GciSession { } catch (e) { log(`Could not read the session serial: ${e instanceof Error ? e.message : e}`); } - // Publish who this session belongs to, where anything on the host can read - // it. Everything above this point is knowledge trapped in one extension - // host; `System cacheName:` is what makes it visible to another window, to - // topaz, and to whatever ends up reporting on sessions. It writes shared - // memory rather than the repository, so it costs no commit and — the - // reason it beats a committed registry — the entry dies with the process - // instead of outliving a window that crashed. Best-effort, like the serial: - // an unnamed session still works. - try { - session.execute(`System cacheName: '${cacheNameFor(resolved).replace(/'/g, "''")}'. true`); - } catch (e) { - log(`Could not name the session (${resolved.label}): ${e instanceof Error ? e.message : e}`); - } + session.publishName(); log( `Connected to GemDB as ${DB_USER} (${resolved.label}` + `${session.sessionSerial === undefined ? '' : `, session ${session.sessionSerial}`})`, @@ -773,6 +814,29 @@ export function sessionRegistry(): SessionInfo[] { .sort((a, b) => b.idleMs - a.idleMs); } +/** + * Follow a session to its owner's new identity. + * + * Sessions are keyed by a notebook's URI, so renaming the file changes the + * key. Without this the old session would be stranded in the map — still + * logged in, spending one of ten, owned by a URI nothing will ask for again — + * and the notebook would silently log in a second one. Re-keying keeps the + * session, and re-sending the name keeps the shared cache honest about who + * holds it; a stale name is worse than none, because it points an + * administrator at a notebook that no longer exists. + * + * Answers whether there was a session to move, so the caller can skip the + * work that only matters if there was. + */ +export function renameSession(oldKey: string, newOwner: SessionOwner): boolean { + const session = sessions.get(oldKey); + if (!session) return false; + sessions.delete(oldKey); + sessions.set(newOwner.key, session); + session.rename(newOwner); + return true; +} + /** One owner's session if it is already open, without logging one in. */ export function sessionForIfOpen(key: string): GciSession | undefined { const session = sessions.get(key);