diff --git a/CHANGELOG.md b/CHANGELOG.md index df952bb..827dbab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 no setter. A method's receiver (`self`, or whatever the def calls it), a tuple's items, group rows and a saved stack's values cannot be changed, and VS Code does not offer Set Value on them. +### Fixed + +- **Starting GemDB from two places at once starts it once.** A cell run while the database was + starting on its own, cells in two notebooks, or a cell and **GemDB: Start** each ran the whole + start: two stone starts, two installs of Python support, two MCP servers. Now whatever asks while + a start is under way in the same window waits for that start and gets its result, unless the + database was stopped while it started: then it waits for the stop and starts the database again. +- **GemDB asks about shared memory once, even when two things need it.** A cell run while the + first-run question about shared memory was still open queued a second copy of it, and answering + both could open two `sudo` terminals. Now the second waits for the first answer. Running + **GemDB: Configure Shared Memory** while that setup is already running uses its terminal instead + of opening another, and so does answering the question while the command's setup is running. +- **The MCP server is installed and started once when GemDB starts.** Opening a window on a running + database while a cell started it could install the MCP server's classes twice and start two + servers on one port. Now the second waits for the first. + ## [1.6.0] - 2026-10-07 The engine is unchanged at GemStone 4.0.0.a4, but start with a fresh database: delete diff --git a/CLAUDE.md b/CLAUDE.md index 2e6fb3a..cbdf434 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,7 +49,11 @@ measured about when to ask for shared memory. whether the user pressed Start or just ran a notebook cell. It finishes any outstanding preparation, prompts for shared memory, starts the processes, files Grail in, and brings the MCP server up. New entry points that need a database -should call it rather than checking and asking. +should call it rather than checking and asking. Within a window it is +single-flight (`singleFlight.ts`): a second caller waits for the call already +in progress and gets its result, and only the first caller reports telemetry — +except after a Stop: a caller other than auto-start arriving once a Stop began +during that bring-up waits for the Stop and starts again. ## CI diff --git a/docs/telemetry.md b/docs/telemetry.md index feaca80..2a9fa9f 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -153,8 +153,15 @@ Memory** while shared memory was still too low, or when they ran **GemDB: Keep the Database Running After Logout** while RemoveIPC was unset. The dialog comes back every time the database is needed until the user agrees, but a failure is sent once for each trigger, outcome and `missing` until setup works or the -database next starts. Running either command is not deduped: every run is -sent. +database next starts. That rule does not apply to either command: every run +made while the setting was still unchanged is sent. A request that arrives +while the dialog, or the shared-memory setup it started, is already under way +in the same window waits for it and sends nothing, so one dialog is one event, +carrying the trigger of whatever opened it. The command is not such a request: +run during the dialog's setup, **GemDB: Configure Shared Memory** shares its +terminal but sends its own event, and a dialog that comes up while the +command's setup is running is shown and sent as usual, its "Configure" joining +that terminal rather than opening a second one. | Property | Values | | --------- | ----------------------------------------------------------------------------- | @@ -167,7 +174,11 @@ sent. The database was brought up, or failed to come up. Sent only when something actually happened. Running a cell when the database is already up sends nothing, and a failure is sent once for each trigger until the database next -starts. +starts. A request that arrives while a start is already under way in the same +window waits for it and sends nothing, so one start is one event, carrying the +trigger of whatever began it. The exception is a request that arrives after +the user stopped the database during that start: it waits for the stop and +then asks again, so the start that follows is an event of its own. | Property / measure | Values | | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------- | diff --git a/src/__mocks__/vscode.ts b/src/__mocks__/vscode.ts index d88502f..bee747c 100644 --- a/src/__mocks__/vscode.ts +++ b/src/__mocks__/vscode.ts @@ -251,7 +251,16 @@ export const window = { dispose(): void; } { __terminals.push(nameOrOptions as FakeTerminalOptions); - return { show: () => {}, sendText: () => {}, dispose: () => {} }; + const terminal = { show: () => {}, sendText: () => {}, dispose: () => {} }; + __openTerminals.push(terminal); + return terminal; + }, + /** Fires when a test calls `__closeTerminal`, as the editor does when the user closes one. */ + onDidCloseTerminal(listener: (terminal: unknown) => void): Disposable { + __closeTerminalListeners.push(listener); + return new Disposable(() => { + __closeTerminalListeners.splice(__closeTerminalListeners.indexOf(listener), 1); + }); }, /** * Progress is a pass-through here: run the callback and return its result. @@ -489,6 +498,20 @@ export interface FakeTerminalOptions { }; } export const __terminals: FakeTerminalOptions[] = []; +/** The terminal objects `createTerminal` handed back, oldest first. */ +export const __openTerminals: object[] = []; +const __closeTerminalListeners: Array<(terminal: unknown) => void> = []; +/** Close a terminal the way the editor reports it to `onDidCloseTerminal`. */ +export function __closeTerminal(terminal: object): void { + for (const listener of [...__closeTerminalListeners]) listener(terminal); +} +/** + * Close every terminal still open, so a test that failed half way does not + * leave a setup script waiting for the next test to finish it. + */ +export function __closeAllTerminals(): void { + for (const terminal of __openTerminals.splice(0)) __closeTerminal(terminal); +} export const workspace = { notebookDocuments: [] as unknown[], diff --git a/src/__tests__/oneBringUp.test.ts b/src/__tests__/oneBringUp.test.ts new file mode 100644 index 0000000..a8ddf55 --- /dev/null +++ b/src/__tests__/oneBringUp.test.ts @@ -0,0 +1,398 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as vscode from 'vscode'; +import { __resetSettings, __setSetting } from '../__mocks__/vscode'; +import { eventsNamed, fakeExtensionContext } from './telemetryTestSupport'; + +// `ensureRunning` is single-flight within a window: two notebooks and Start +// pressed together would otherwise each run the whole bring-up, with two stone +// starts, two Grail file-ins and two events. These tests hold the stone start +// open on a promise so the calls genuinely overlap, then check that one run +// served them all and that only the caller that started it reported. +vi.mock('../cli', () => ({ writeCliScripts: () => {}, ensureCliCurrent: () => true })); +// Creating the database account logs in over GCI; that is integration's. +vi.mock('../account', () => ({ ensureDatabaseAccount: () => {} })); + +const bundledGrailStamp = vi.fn(() => 'grail=0.1-1-gabc\n'); +const grailNeedsUpdate = vi.fn(() => false); +const fileInGrail = vi.fn(async () => {}); +vi.mock('../grail', () => ({ + grailLabel: () => 'grail 0.1', + grailNeedsUpdate: () => grailNeedsUpdate(), + stageGrail: () => {}, + fileInGrail: () => fileInGrail(), + bundledGrailStamp: () => bundledGrailStamp(), +})); + +const grailInstalled = vi.fn(() => true); +const isInstalled = vi.fn(() => true); +vi.mock('../paths', () => ({ + databaseExists: () => isInstalled(), + databasePath: () => '/db', + enginePath: () => (isInstalled() ? '/engine' : undefined), + grailInstalled: () => grailInstalled(), + grailPath: () => '/grail', + grailStagedOnDisk: () => isInstalled(), + mcpPath: () => '/mcp', +})); + +const ensureOsConfigured = vi.fn( + async (_extensionPath: string, _trigger: string): Promise => 'alreadyConfigured', +); +const isSharedMemoryConfigured = vi.fn(async () => true); +vi.mock('../osConfig', async (importOriginal) => { + const actual = await importOriginal(); + return { + OS_CONFIG_RESULT: actual.OS_CONFIG_RESULT, + osConfigAllowsStart: actual.osConfigAllowsStart, + ensureOsConfigured: (extensionPath: string, trigger: string) => + ensureOsConfigured(extensionPath, trigger), + isSharedMemoryConfigured: () => isSharedMemoryConfigured(), + }; +}); + +const findStone = vi.fn(() => true); +const findNetldi = vi.fn(() => true); +const startStone = vi.fn(async () => {}); +const startNetldi = vi.fn(async () => {}); +const stopStone = vi.fn(async (_force: boolean) => { + findStone.mockReturnValue(false); +}); +vi.mock('../processes', () => ({ + listProcesses: () => [], + findStone: () => findStone(), + findNetldi: () => findNetldi(), + startStone: () => startStone(), + startNetldi: () => startNetldi(), + isListening: () => true, + isRunning: () => findStone(), + stopNetldi: async () => {}, + stopStone: (force: boolean) => stopStone(force), +})); + +// The MCP server is off unless a test turns `gemdb.mcp.enabled` on. +const mcpNeedsUpdate = vi.fn(() => false); +const installMcp = vi.fn(async () => {}); +const isMcpRunning = vi.fn(async () => false); +const startMcpServer = vi.fn(async () => true); +vi.mock('../mcp', async (importOriginal) => ({ + ...(await importOriginal()), + bundledMcpStamp: () => 'mcp=abc\n', + mcpNeedsUpdate: () => mcpNeedsUpdate(), + stageMcp: () => {}, + installMcp: () => installMcp(), + recordMcpInstalled: () => {}, + isMcpRunning: () => isMcpRunning(), + startMcpServer: () => startMcpServer(), + stopMcpServer: async () => {}, +})); + +vi.mock('../platform', () => ({ isSupportedPlatform: () => true, setContext: () => {} })); +const allowAutoStart = vi.fn(); +vi.mock('../autoStart', () => ({ + allowAutoStart: () => allowAutoStart(), + autoStartSuppressed: () => false, + initAutoStart: () => {}, + suppressAutoStart: () => {}, +})); + +const { ensureRunning, resumeMcpServing, stop } = await import('../lifecycle'); +const { TRIGGER, initTelemetry } = await import('../telemetry'); +const { initUnattendedSetupMarker } = await import('../unattendedSetupMarker'); + +/** Every deferred a test made, so `afterEach` can release whatever a failed test left waiting. */ +const deferreds: Array<{ resolve: () => void }> = []; + +/** A promise the test settles by hand, so a bring-up stays in progress until it chooses. */ +function deferred() { + let resolve!: () => void; + let reject!: (error: Error) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + deferreds.push({ resolve }); + return { promise, resolve, reject }; +} + +// The bring-up and the MCP step are module-level single flights. A test that +// fails with one still held open would hand it to every later test, which +// would join it and time out — so release every deferred and let it drain. +afterEach(async () => { + for (const d of deferreds.splice(0)) d.resolve(); + for (let round = 0; round < 5; round++) await new Promise((resolve) => setTimeout(resolve, 0)); +}); + +describe('ensureRunning', () => { + beforeEach(() => { + __resetSettings(); + bundledGrailStamp.mockReturnValue('grail=0.1-1-gabc\n'); + grailNeedsUpdate.mockReturnValue(true); + grailInstalled.mockReturnValue(false); + isInstalled.mockReturnValue(true); + ensureOsConfigured.mockReset().mockResolvedValue('alreadyConfigured'); + isSharedMemoryConfigured.mockReset().mockResolvedValue(true); + fileInGrail.mockClear(); + findStone.mockReturnValue(false); + findNetldi.mockReturnValue(true); + startStone.mockReset(); + + const context = fakeExtensionContext(); + initTelemetry(context, false); + initUnattendedSetupMarker(context.globalStorageUri.fsPath); + }); + + describe('when called while a bring-up is in progress', () => { + it('serves every caller with one run, reported once under the first caller', async () => { + const stoneStart = deferred(); + startStone.mockImplementation(async () => { + await stoneStart.promise; + findStone.mockReturnValue(true); + }); + + const calls = [ + ensureRunning('/ext', TRIGGER.autoStart), + ensureRunning('/ext', TRIGGER.notebook), + ensureRunning('/ext', TRIGGER.startCommand), + ]; + stoneStart.resolve(); + + expect(await Promise.all(calls)).toEqual([true, true, true]); + expect(startStone).toHaveBeenCalledTimes(1); + expect(fileInGrail).toHaveBeenCalledTimes(1); + const events = eventsNamed('databaseStarted'); + expect(events).toHaveLength(1); + expect(events[0].properties).toMatchObject({ + trigger: 'autoStart', + outcome: 'started', + filedGrail: 'firstTime', + }); + }); + + it('fails every caller together when the run fails, reporting the failure once', async () => { + const stoneStart = deferred(); + startStone.mockImplementation(() => stoneStart.promise); + + const calls = [ + ensureRunning('/ext', TRIGGER.notebook), + ensureRunning('/ext', TRIGGER.notebook), + ensureRunning('/ext', TRIGGER.startCommand), + ]; + stoneStart.reject(new Error('stone would not start')); + + expect(await Promise.all(calls)).toEqual([false, false, false]); + expect(startStone).toHaveBeenCalledTimes(1); + const events = eventsNamed('databaseStarted'); + expect(events).toHaveLength(1); + expect(events[0].properties).toMatchObject({ trigger: 'notebook', outcome: 'startFailed' }); + }); + }); + + it('starts a new run for a call made after the previous one has settled', async () => { + startStone.mockImplementation(async () => {}); + ensureOsConfigured.mockResolvedValue('configured'); + + await ensureRunning('/ext', TRIGGER.notebook); + await ensureRunning('/ext', TRIGGER.startCommand); + + expect(ensureOsConfigured).toHaveBeenCalledTimes(2); + expect(startStone).toHaveBeenCalledTimes(2); + }); +}); + +// Activation's `resumeMcpServing` and a cell's `ensureRunning` both reach +// `ensureMcpServing`; on a running stone they would otherwise install the +// classes twice and fork two routers at one port. +describe('ensureMcpServing', () => { + beforeEach(() => { + __resetSettings(); + __setSetting('gemdb.mcp.enabled', true); + grailNeedsUpdate.mockReturnValue(false); + grailInstalled.mockReturnValue(true); + isInstalled.mockReturnValue(true); + ensureOsConfigured.mockReset().mockResolvedValue('alreadyConfigured'); + findStone.mockReturnValue(true); + findNetldi.mockReturnValue(true); + mcpNeedsUpdate.mockReturnValue(true); + installMcp.mockReset(); + isMcpRunning.mockReset().mockResolvedValue(false); + startMcpServer.mockReset().mockResolvedValue(true); + + const context = fakeExtensionContext(); + initTelemetry(context, false); + initUnattendedSetupMarker(context.globalStorageUri.fsPath); + }); + + it('installs and starts the MCP server once when resume and a start overlap', async () => { + const install = deferred(); + installMcp.mockImplementation(() => install.promise); + + const resumed = resumeMcpServing('/ext'); + const started = ensureRunning('/ext', TRIGGER.notebook); + // Let both callers reach the MCP step before the install finishes. + await new Promise((resolve) => setTimeout(resolve, 20)); + install.resolve(); + + expect(await Promise.all([resumed, started])).toEqual([true, true]); + expect(installMcp).toHaveBeenCalledTimes(1); + expect(startMcpServer).toHaveBeenCalledTimes(1); + }); +}); + +// Stop takes no lock, so it can land on a bring-up under way. A caller who +// arrives after that Stop is asking for the database back: it must not be +// handed a result that described a database Stop has since taken down. +describe('ensureRunning after a Stop', () => { + beforeEach(() => { + __resetSettings(); + __setSetting('gemdb.mcp.enabled', true); + grailNeedsUpdate.mockReturnValue(false); + grailInstalled.mockReturnValue(true); + isInstalled.mockReturnValue(true); + ensureOsConfigured.mockReset().mockResolvedValue('alreadyConfigured'); + fileInGrail.mockReset(); + findStone.mockReturnValue(false); + findNetldi.mockReturnValue(true); + startStone.mockReset().mockImplementation(async () => { + findStone.mockReturnValue(true); + }); + stopStone.mockClear(); + mcpNeedsUpdate.mockReturnValue(true); + installMcp.mockReset(); + isMcpRunning.mockReset().mockResolvedValue(false); + startMcpServer.mockReset().mockResolvedValue(true); + allowAutoStart.mockClear(); + + const context = fakeExtensionContext(); + initTelemetry(context, false); + initUnattendedSetupMarker(context.globalStorageUri.fsPath); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('starts the database again for a cell run after Stop, while auto-start is in the MCP step', async () => { + const install = deferred(); + installMcp.mockImplementationOnce(() => install.promise); + + const auto = ensureRunning('/ext', TRIGGER.autoStart); + await vi.waitFor(() => expect(installMcp).toHaveBeenCalledTimes(1)); + await stop(); + expect(findStone()).toBeFalsy(); + + const cell = ensureRunning('/ext', TRIGGER.notebook); + install.resolve(); + + expect(await auto).toBe(true); + expect(await cell).toBe(true); + expect(findStone()).toBe(true); + expect(startStone).toHaveBeenCalledTimes(2); + // The cell retracted the Stop, once per pass; auto-start, GemDB's own doing, did not. + expect(allowAutoStart).toHaveBeenCalledTimes(2); + }); + + it('starts again for a cell run after Stop even when the bring-up it joined failed', async () => { + // Stop lands during the Grail file-in, which then fails for want of a stone. + grailNeedsUpdate.mockReturnValue(true); + grailInstalled.mockReturnValue(false); + const fileIn = deferred(); + fileInGrail.mockImplementationOnce(() => fileIn.promise); + + const auto = ensureRunning('/ext', TRIGGER.autoStart); + await vi.waitFor(() => expect(fileInGrail).toHaveBeenCalledTimes(1)); + await stop(); + const cell = ensureRunning('/ext', TRIGGER.notebook); + fileIn.reject(new Error('no stone')); + + expect(await auto).toBe(false); + expect(await cell).toBe(true); + expect(findStone()).toBe(true); + expect(fileInGrail).toHaveBeenCalledTimes(2); + }); + + it('joins without starting again when no Stop came between', async () => { + const install = deferred(); + installMcp.mockImplementationOnce(() => install.promise); + + const first = ensureRunning('/ext', TRIGGER.notebook); + await vi.waitFor(() => expect(installMcp).toHaveBeenCalledTimes(1)); + const second = ensureRunning('/ext', TRIGGER.startCommand); + install.resolve(); + + expect(await Promise.all([first, second])).toEqual([true, true]); + expect(startStone).toHaveBeenCalledTimes(1); + expect(allowAutoStart).toHaveBeenCalledTimes(2); + }); + + it('lets a Stop pressed after the cell joined win', async () => { + const install = deferred(); + installMcp.mockImplementationOnce(() => install.promise); + + const auto = ensureRunning('/ext', TRIGGER.autoStart); + await vi.waitFor(() => expect(installMcp).toHaveBeenCalledTimes(1)); + const cell = ensureRunning('/ext', TRIGGER.notebook); + await stop(); + install.resolve(); + + await auto; + await cell; + expect(startStone).toHaveBeenCalledTimes(1); + expect(findStone()).toBeFalsy(); + }); + + it('lets a Stop stand for an auto-start that joins after it', async () => { + const install = deferred(); + installMcp.mockImplementationOnce(() => install.promise); + + const cell = ensureRunning('/ext', TRIGGER.notebook); + await vi.waitFor(() => expect(installMcp).toHaveBeenCalledTimes(1)); + await stop(); + const auto = ensureRunning('/ext', TRIGGER.autoStart); + install.resolve(); + + await cell; + await auto; + expect(startStone).toHaveBeenCalledTimes(1); + expect(findStone()).toBeFalsy(); + }); + + it('leaves the database up when the Stop the cell came after is cancelled at its modal', async () => { + const install = deferred(); + installMcp.mockImplementationOnce(() => install.promise); + // A session holds the stone up, so Stop asks before forcing it; the user + // dismisses the modal, which is the same answer as Cancel. + stopStone.mockImplementationOnce(async () => { + throw new Error('sessions are still logged in'); + }); + const modal = deferred(); + const warning = vi + .spyOn(vscode.window, 'showWarningMessage') + .mockImplementationOnce(() => modal.promise.then(() => undefined)); + + const auto = ensureRunning('/ext', TRIGGER.autoStart); + await vi.waitFor(() => expect(installMcp).toHaveBeenCalledTimes(1)); + const stopping = stop(); + await vi.waitFor(() => expect(warning).toHaveBeenCalledTimes(1)); + + let cellDone = false; + const cell = ensureRunning('/ext', TRIGGER.notebook).then((running) => { + cellDone = true; + return running; + }); + install.resolve(); + expect(await auto).toBe(true); + // The cell goes after the Stop it followed, so it waits on that modal. + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(cellDone).toBe(false); + + modal.resolve(); + await stopping; + expect(await cell).toBe(true); + // Nothing was stopped, so nothing was started again: the second pass + // found the stone up and only brought back the MCP server Stop had taken down. + expect(stopStone).toHaveBeenCalledTimes(1); + expect(startStone).toHaveBeenCalledTimes(1); + expect(startMcpServer).toHaveBeenCalledTimes(2); + expect(findStone()).toBe(true); + }); +}); diff --git a/src/__tests__/oneOsPrompt.test.ts b/src/__tests__/oneOsPrompt.test.ts new file mode 100644 index 0000000..c336238 --- /dev/null +++ b/src/__tests__/oneOsPrompt.test.ts @@ -0,0 +1,277 @@ +import { promises as fsPromises } from 'fs'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as vscode from '../__mocks__/vscode'; +import { eventsNamed, fakeExtensionContext } from './telemetryTestSupport'; + +/** + * The vscode-facing wrappers are one at a time in a window: the first-run + * prompt, a cell's, and "GemDB: Configure Shared Memory" must not each open a + * modal or a `sudo` terminal. They live apart from osConfig.test.ts because + * they need the real wrappers, with the operating system faked underneath. + */ + +// The limits `isSharedMemoryConfigured` reads — as macOS's sysctl prints them, +// or as Linux's /proc holds them — and what `busctl` says about RemoveIPC. +let enoughMemory = false; +let removeIpcOff = true; +const sharedMemoryBytes = (): number => Math.round((enoughMemory ? 1024 : 0.004) * 2 ** 30); +vi.mock('child_process', async (original) => ({ + ...(await original()), + execFile: ( + file: string, + _args: string[], + _options: unknown, + callback: (error: Error | null, stdout: string) => void, + ) => { + if (file === 'busctl') { + callback(null, `b ${removeIpcOff ? 'false' : 'true'}\n`); + return; + } + const bytes = sharedMemoryBytes(); + callback(null, `kern.sysv.shmmax: ${bytes}\nkern.sysv.shmall: ${Math.round(bytes / 4096)}\n`); + }, +})); + +const { ensureOsConfigured, configureSharedMemory } = await import('../osConfig'); +const { TRIGGER, initTelemetry } = await import('../telemetry'); + +const SHARED_MEMORY_TERMINAL = 'GemDB: Shared Memory Setup'; +const REMOVE_IPC_TERMINAL = 'GemDB: RemoveIPC Setup'; + +const originalPlatform = process.platform; +const sent = (): Record[] => + eventsNamed('osConfigPrompted').map((e) => e.properties as Record); + +let answer: ((choice: string | undefined) => void) | undefined; +let modal: ReturnType; +let information: ReturnType; +let error: ReturnType; + +/** Terminals opened so far with this name, in order. */ +const terminalsNamed = (name: string): number => + vscode.__terminals.filter((t) => (t as unknown) === name).length; + +/** Close the most recent terminal with this name, as the user finishing it. */ +function closeTerminal(name: string): void { + const index = (vscode.__terminals as unknown[]).lastIndexOf(name); + vscode.__closeTerminal(vscode.__openTerminals[index]); +} + +/** The user finishing the shared-memory terminal: the script has run, so memory is as given. */ +function finishScript(options: { took: boolean }): void { + enoughMemory = options.took; + closeTerminal(SHARED_MEMORY_TERMINAL); +} + +/** Let every pending promise chain run, so a test can assert that something is still waiting. */ +const settle = (): Promise => new Promise((resolve) => setTimeout(resolve, 0)); + +function onLinux(): void { + Object.defineProperty(process, 'platform', { value: 'linux' }); + vi.spyOn(fsPromises, 'readFile').mockImplementation((async (file: unknown) => { + if (String(file).startsWith('/proc/sys/kernel/shmmax')) return String(sharedMemoryBytes()); + if (String(file).startsWith('/proc/sys/kernel/shmall')) + return String(Math.round(sharedMemoryBytes() / 4096)); + throw new Error(`unexpected read of ${String(file)}`); + }) as typeof fsPromises.readFile); +} + +beforeEach(() => { + Object.defineProperty(process, 'platform', { value: 'darwin' }); + enoughMemory = false; + removeIpcOff = true; + vscode.__resetSettings(); + vscode.__terminals.length = 0; + vscode.__openTerminals.length = 0; + initTelemetry(fakeExtensionContext(), false); + answer = undefined; + modal = vi.spyOn(vscode.window, 'showWarningMessage').mockImplementation( + () => + new Promise((resolve) => { + answer = resolve; + }), + ); + information = vi.spyOn(vscode.window, 'showInformationMessage'); + error = vi.spyOn(vscode.window, 'showErrorMessage'); +}); + +// The prompt and the script are module-level single flights. A test that fails +// with one still open would hand it to every later test, which would join it +// and time out, so settle whatever is left — cancel the modal, close the +// terminals — before the spies are restored. +afterEach(async () => { + for (let round = 0; round < 5; round++) { + answer?.(undefined); + vscode.__closeAllTerminals(); + await settle(); + } + Object.defineProperty(process, 'platform', { value: originalPlatform }); + vi.restoreAllMocks(); +}); + +describe('one OS prompt at a time', () => { + it('shares one modal, one terminal and one event between callers with different triggers', async () => { + const first = ensureOsConfigured('/ext', TRIGGER.firstRun); + const second = ensureOsConfigured('/ext', TRIGGER.notebook); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(1)); + + answer?.('Configure'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + finishScript({ took: true }); + + expect(await first).toBe('configured'); + expect(await second).toBe('configured'); + expect(modal).toHaveBeenCalledTimes(1); + expect(vscode.__terminals).toHaveLength(1); + expect(sent()).toHaveLength(1); + expect(sent()[0]).toMatchObject({ trigger: 'firstRun', outcome: 'configured' }); + }); + + it('starts afresh once the first prompt has settled', async () => { + const first = ensureOsConfigured('/ext', TRIGGER.firstRun); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(1)); + answer?.(undefined); + expect(await first).toBe('declined'); + + const second = ensureOsConfigured('/ext', TRIGGER.notebook); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(2)); + answer?.(undefined); + expect(await second).toBe('declined'); + }); +}); + +describe('the command and the prompt share one script, and nothing else', () => { + it('lets the command join the script a prompt started, without a second terminal', async () => { + const prompt = ensureOsConfigured('/ext', TRIGGER.firstRun); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(1)); + answer?.('Configure'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + + const command = configureSharedMemory('/ext'); + await settle(); + finishScript({ took: true }); + await command; + + expect(await prompt).toBe('configured'); + expect(vscode.__terminals).toHaveLength(1); + // One event each: the modal's answer, and the command run while memory was short. + expect(sent()).toEqual([ + expect.objectContaining({ trigger: 'firstRun', outcome: 'configured' }), + expect.objectContaining({ trigger: 'sharedMemoryCommand', outcome: 'configured' }), + ]); + expect(information).toHaveBeenCalledTimes(1); + expect(information).toHaveBeenCalledWith('Shared memory configured.'); + expect(error).not.toHaveBeenCalled(); + }); + + it('runs the script for the command even when the modal it overlapped is cancelled', async () => { + const prompt = ensureOsConfigured('/ext', TRIGGER.notebook); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(1)); + + const command = configureSharedMemory('/ext'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + answer?.(undefined); + expect(await prompt).toBe('declined'); + + finishScript({ took: true }); + await command; + expect(vscode.__terminals).toHaveLength(1); + expect(information).toHaveBeenCalledWith('Shared memory configured.'); + expect(error).not.toHaveBeenCalled(); + }); + + it('lets a modal answered "Configure" join the script the command already opened', async () => { + const command = configureSharedMemory('/ext'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + + // Raising shared memory always asks, even with the command's terminal open. + const prompt = ensureOsConfigured('/ext', TRIGGER.notebook); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(1)); + answer?.('Configure'); + await settle(); + expect(vscode.__terminals).toHaveLength(1); + + finishScript({ took: false }); + expect(await prompt).toBe('stillUnconfigured'); + await command; + expect(vscode.__terminals).toHaveLength(1); + expect(sent()).toEqual([ + expect.objectContaining({ trigger: 'sharedMemoryCommand', outcome: 'stillUnconfigured' }), + expect.objectContaining({ trigger: 'notebook', outcome: 'stillUnconfigured' }), + ]); + }); + + it("answers alreadyConfigured at once when memory is enough, even with the command's terminal open", async () => { + enoughMemory = true; + const command = configureSharedMemory('/ext'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + + expect(await ensureOsConfigured('/ext', TRIGGER.notebook)).toBe('alreadyConfigured'); + expect(modal).not.toHaveBeenCalled(); + // Memory was never short, so neither the prompt nor the command reports. + finishScript({ took: true }); + await command; + expect(sent()).toHaveLength(0); + }); + + it('reports nothing for a cell whose modal was never shown', async () => { + enoughMemory = true; + const command = configureSharedMemory('/ext'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + + // The cell overlaps the command's script but shows no modal, so it must + // not answer `configured`: that would count as GemDB having prompted. + const prompt = ensureOsConfigured('/ext', TRIGGER.notebook); + finishScript({ took: true }); + await command; + expect(await prompt).toBe('alreadyConfigured'); + expect(modal).not.toHaveBeenCalled(); + expect(sent()).toHaveLength(0); + }); +}); + +describe('on Linux, with RemoveIPC unset', () => { + beforeEach(() => { + onLinux(); + removeIpcOff = false; + }); + + it("still offers RemoveIPC to a prompt that joined the command's script", async () => { + const command = configureSharedMemory('/ext'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + + const prompt = ensureOsConfigured('/ext', TRIGGER.notebook); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(1)); + answer?.('Configure'); + await settle(); + finishScript({ took: true }); + await command; + + await vi.waitFor(() => expect(terminalsNamed(REMOVE_IPC_TERMINAL)).toBe(1)); + closeTerminal(REMOVE_IPC_TERMINAL); + expect(await prompt).toBe('removeIpcUnset'); + expect(terminalsNamed(SHARED_MEMORY_TERMINAL)).toBe(1); + }); + + it('lets the command finish without waiting on the RemoveIPC terminal that follows', async () => { + const prompt = ensureOsConfigured('/ext', TRIGGER.firstRun); + await vi.waitFor(() => expect(modal).toHaveBeenCalledTimes(1)); + answer?.('Configure'); + await vi.waitFor(() => expect(vscode.__openTerminals).toHaveLength(1)); + + let commandDone = false; + const command = configureSharedMemory('/ext').then(() => { + commandDone = true; + }); + await settle(); + finishScript({ took: true }); + await vi.waitFor(() => expect(terminalsNamed(REMOVE_IPC_TERMINAL)).toBe(1)); + await settle(); + + expect(commandDone).toBe(true); + await command; + expect(information).toHaveBeenCalledWith('Shared memory configured.'); + closeTerminal(REMOVE_IPC_TERMINAL); + expect(await prompt).toBe('removeIpcUnset'); + }); +}); diff --git a/src/__tests__/singleFlight.test.ts b/src/__tests__/singleFlight.test.ts new file mode 100644 index 0000000..d60fd2a --- /dev/null +++ b/src/__tests__/singleFlight.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it, vi } from 'vitest'; +import { singleFlight } from '../singleFlight'; + +function deferred(): { + promise: Promise; + resolve: (value: T) => void; + reject: (error: unknown) => void; +} { + let resolve!: (value: T) => void; + let reject!: (error: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; +} + +describe('singleFlight', () => { + it('joins a call already in flight instead of starting another', async () => { + const flight = singleFlight(); + const work = deferred(); + const start = vi.fn(() => work.promise); + const onJoin = vi.fn(); + + const first = flight.run(start, onJoin); + const second = flight.run(start, onJoin); + work.resolve('done'); + + expect(await Promise.all([first, second])).toEqual(['done', 'done']); + expect(start).toHaveBeenCalledTimes(1); + expect(onJoin).toHaveBeenCalledTimes(1); + }); + + it('starts afresh once the call has resolved', async () => { + const flight = singleFlight(); + const start = vi.fn(() => Promise.resolve(1)); + + await flight.run(start); + await flight.run(start); + + expect(start).toHaveBeenCalledTimes(2); + }); + + it('starts afresh once the call has rejected', async () => { + const flight = singleFlight(); + const start = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error('first')) + .mockResolvedValueOnce(2); + + await expect(flight.run(start)).rejects.toThrow('first'); + expect(await flight.run(start)).toBe(2); + }); + + it('hands a joiner the same rejection', async () => { + const flight = singleFlight(); + const work = deferred(); + + const first = flight.run(() => work.promise); + const second = flight.run(() => Promise.resolve(0)); + work.reject(new Error('failed')); + + await expect(first).rejects.toThrow('failed'); + await expect(second).rejects.toThrow('failed'); + }); +}); diff --git a/src/lifecycle.ts b/src/lifecycle.ts index c539271..7d90eba 100644 --- a/src/lifecycle.ts +++ b/src/lifecycle.ts @@ -70,6 +70,7 @@ import { isOnNfs } from './networkFileSystem'; import { logoutAll } from './session'; import { ensureStatmonitor } from './statmonitor'; import { allowAutoStart } from './autoStart'; +import { singleFlight } from './singleFlight'; import { readUnattendedSetupMarker, writeUnattendedSetupMarker } from './unattendedSetupMarker'; import { DATABASE_OUTCOME, @@ -227,20 +228,18 @@ function paused(): void { * say so instead of offering a Set Up GemDB button that would only join it. */ export function runSetup(extensionPath: string, trigger: Trigger): Promise { - if (setupInFlight) { - log('Setup is already under way; waiting for it to finish.'); - return setupInFlight; - } - setContext('gemdb.settingUp', true); - const run = runSetupOnce(extensionPath, trigger).finally(() => { - setupInFlight = undefined; - setContext('gemdb.settingUp', false); - }); - setupInFlight = run; - return run; + return setupFlight.run( + () => { + setContext('gemdb.settingUp', true); + return runSetupOnce(extensionPath, trigger).finally(() => { + setContext('gemdb.settingUp', false); + }); + }, + () => log('Setup is already under way; waiting for it to finish.'), + ); } -let setupInFlight: Promise | undefined; +const setupFlight = singleFlight(); async function runSetupOnce(extensionPath: string, trigger: Trigger): Promise { reportSetupStarted(trigger); @@ -504,19 +503,68 @@ export async function start(extensionPath: string): Promise { * pointed at a new root path — and here the prompt is justified because the * user has asked for something that cannot happen without it. * + * At most one runs at a time in this window. Auto-start and a cell, two + * notebooks, or a cell and Start would otherwise each run the whole bring-up + * at once — two stone starts, two Grail file-ins, two MCP servers. A caller + * arriving while one is in progress joins it, from the shared-memory prompt to + * the last step, and gets its result. A joiner reports nothing: + * `databaseStarted` is sent once, by the caller that started the call, under + * that caller's trigger. + * + * Except after a Stop: a caller other than auto-start that arrives once Stop + * has been pressed on the bring-up under way is asking for the database back, so it waits for that + * bring-up and the Stop to finish and then starts again, rather than taking a + * result that described a database Stop has since taken down. A Stop pressed + * after the caller joined wins: it was pressed later, so it is the latest word. + * * Returns true when the database is up and Python will run. */ export async function ensureRunning(extensionPath: string, trigger: Trigger): Promise { + // Asking for a running database is the clearest possible retraction of an + // earlier "stop it". Running a cell counts: `ensureRunning` is the one path + // to a running database, so it is the one place this belongs — here rather + // than in the bring-up, so that a caller who joins one counts too. Auto-start + // is GemDB's own doing, not a request from anybody, so it retracts nothing. + if (trigger !== TRIGGER.autoStart) allowAutoStart(); + let stoppedBeforeJoining = false; + const running = await bringUp.run( + () => { + stopsWhenBringUpBegan = stopsBegun; + return ensureRunningOnce(extensionPath, trigger); + }, + () => { + stoppedBeforeJoining = stopsBegun !== stopsWhenBringUpBegan; + log('GemDB is already starting; waiting for it.'); + }, + ); + // Whatever the shared result, true or false, it predates this caller's + // request. Each rerun needs another Stop to have begun, so this cannot loop. + // Auto-start asks for nothing, so it takes the result and lets the Stop stand. + if (!stoppedBeforeJoining || trigger === TRIGGER.autoStart) return running; + await stopInFlight; + log('GemDB was stopped while it was starting; starting it again.'); + return ensureRunning(extensionPath, trigger); +} + +const bringUp = singleFlight(); +/** + * How many times `stop` has begun in this window, and that count when the + * bring-up in flight began. Counted rather than asked of the processes: + * `isRunning` blocks the extension host, and a stone that is up says nothing + * about whether a Stop is on its way to it. + */ +let stopsBegun = 0; +let stopsWhenBringUpBegan = 0; +/** The Stop under way, if any, so a caller starting again goes after it. */ +let stopInFlight: Promise | undefined; + +async function ensureRunningOnce(extensionPath: string, trigger: Trigger): Promise { const stopwatch = Stopwatch.start(); const failed = (outcome: Exclude): false => { reportDatabaseStarted(trigger, outcome, FILED_GRAIL.no, stopwatch.elapsedMs(), false); return false; }; - // Asking for a running database is the clearest possible retraction of an - // earlier "stop it". Running a cell counts: `ensureRunning` is the one path - // to a running database, so it is the one place this belongs. - allowAutoStart(); if (!requireSupportedPlatform()) return failed(DATABASE_OUTCOME.unsupportedPlatform); if (!requireGrailPayload(extensionPath)) return failed(DATABASE_OUTCOME.missingPayload); @@ -640,6 +688,12 @@ function grailFileInDue(extensionPath: string): boolean { * Installing is separate from running, as with Grail: the classes are filed * into the database once per payload build, and the router is forked whenever * one is not already listening. + * + * At most one runs at a time in this window. `ensureRunning` and activation's + * `resumeMcpServing` both come here, and two at once would install the + * classes twice and race to fork two routers onto one port. A caller arriving + * while one is in progress waits for it and gets its result; its progress + * notification says nothing about the MCP server meanwhile. */ async function ensureMcpServing( extensionPath: string, @@ -652,7 +706,18 @@ async function ensureMcpServing( log('This build of GemDB ships no MCP server payload, so there is none to run.'); return false; } + return mcpServing.run( + () => ensureMcpServingOnce(extensionPath, progress), + () => log('The MCP server is already starting; waiting for it.'), + ); +} + +const mcpServing = singleFlight(); +async function ensureMcpServingOnce( + extensionPath: string, + progress?: vscode.Progress<{ message?: string }>, +): Promise { try { if (mcpNeedsUpdate(extensionPath)) { const stamp = bundledMcpStamp(extensionPath); @@ -887,6 +952,17 @@ export async function stop(): Promise { ); return; } + stopsBegun++; + const stopping = stopOnce(); + stopInFlight = stopping; + try { + await stopping; + } finally { + if (stopInFlight === stopping) stopInFlight = undefined; + } +} + +async function stopOnce(): Promise { await vscode.window.withProgress( { location: vscode.ProgressLocation.Notification, title: 'Stopping GemDB' }, async () => { diff --git a/src/osConfig.ts b/src/osConfig.ts index 55e7c3f..24325f5 100644 --- a/src/osConfig.ts +++ b/src/osConfig.ts @@ -4,6 +4,7 @@ import { execFile } from 'child_process'; import * as vscode from 'vscode'; import { REQUIRED_SHARED_MEMORY_GB } from './config'; import { log } from './log'; +import { singleFlight } from './singleFlight'; import { OS_CONFIG_MISSING, OS_CONFIG_OUTCOME, @@ -342,10 +343,31 @@ export async function runEnsureOsConfigured(world: OsConfigWorld): Promise { + return promptFlight.run( + () => ensureOsConfiguredOnce(extensionPath, trigger), + () => log('GemDB is already asking to configure shared memory; waiting for the answer.'), + ); +} + +const promptFlight = singleFlight(); + +async function ensureOsConfiguredOnce( + extensionPath: string, + trigger: Trigger, ): Promise { return runEnsureOsConfigured({ sharedMemoryOk: isSharedMemoryConfigured, @@ -353,15 +375,7 @@ export function ensureOsConfigured( confirm: async (message) => (await vscode.window.showWarningMessage(message, { modal: true }, 'Configure')) === 'Configure', - runSharedMemoryScript: () => - runSetupScript( - SHARED_MEMORY_TERMINAL, - path.join( - extensionPath, - 'resources', - process.platform === 'linux' ? 'setSharedMemoryLinux.sh' : 'setSharedMemoryDarwin.sh', - ), - ), + runSharedMemoryScript: () => runSharedMemoryScript(extensionPath), runRemoveIpcScript: () => runSetupScript(REMOVE_IPC_TERMINAL, path.join(extensionPath, 'resources', 'setRemoveIPC.sh')), showError: (message) => void vscode.window.showErrorMessage(message), @@ -377,17 +391,16 @@ export function ensureOsConfigured( * else tells the user whether it worked. Reported as `osConfigPrompted` when * shared memory was actually short, because this is the path back after * declining the modal, and without it that recovery is invisible. + * + * When the script is already running in this window — the prompt opened it, + * or this command did a moment ago — this joins it instead of opening a second + * terminal, then checks and reports as usual. It never waits on the prompt + * itself: not on a modal nobody has answered, nor on the RemoveIPC step that + * follows the script. */ export async function configureSharedMemory(extensionPath: string): Promise { const wasShort = !(await isSharedMemoryConfigured()); - await runSetupScript( - SHARED_MEMORY_TERMINAL, - path.join( - extensionPath, - 'resources', - process.platform === 'linux' ? 'setSharedMemoryLinux.sh' : 'setSharedMemoryDarwin.sh', - ), - ); + await runSharedMemoryScript(extensionPath); const configured = await isSharedMemoryConfigured(); if (wasShort) { reportOsConfigPrompted( @@ -405,6 +418,28 @@ export async function configureSharedMemory(extensionPath: string): Promise { + return sharedMemoryScriptFlight.run( + () => + runSetupScript( + SHARED_MEMORY_TERMINAL, + path.join( + extensionPath, + 'resources', + process.platform === 'linux' ? 'setSharedMemoryLinux.sh' : 'setSharedMemoryDarwin.sh', + ), + ), + () => log('The shared-memory setup is already open in a terminal; waiting for it.'), + ); +} + +const sharedMemoryScriptFlight = singleFlight(); + /** * Open the RemoveIPC setup on its own, from the status view's "Survives * logout" row. diff --git a/src/singleFlight.ts b/src/singleFlight.ts new file mode 100644 index 0000000..32026a7 --- /dev/null +++ b/src/singleFlight.ts @@ -0,0 +1,33 @@ +/** + * At most one call of something in flight in this window, shared by everyone + * who asks while it runs. + * + * The first caller starts the work; a caller arriving before it settles joins + * it and gets the same result, rejection included, instead of starting a + * second copy. The slot empties when the work settles, so the next caller + * starts afresh. Only the caller that started the work runs `start`, so + * anything `start` reports — telemetry above all — is reported once. + * + * This is in-window only. Between windows, the locks in `lock.ts` decide. + */ +export interface SingleFlight { + /** Start `start` unless a call is already in flight; then join that one. */ + run(start: () => Promise, onJoin?: () => void): Promise; +} + +export function singleFlight(): SingleFlight { + let inFlight: Promise | undefined; + return { + run(start, onJoin) { + if (inFlight) { + onJoin?.(); + return inFlight; + } + const run = start().finally(() => { + if (inFlight === run) inFlight = undefined; + }); + inFlight = run; + return run; + }, + }; +}