From a1cde5727294c0969a05b6d863a2ac835f186f1d Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 11:49:26 +0200 Subject: [PATCH 01/10] feat(web): enable database editing in VS Code for Web (1.5.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The web build was read-only since v1.0.0 (every edit threw 'Document is read-only') — a conservative original choice from when the web engine path was unreliable. Now that the in-process WASM engine works in the web extension host (1.3.9), enable read-write there. - editorController.ts: drop the !VSCODE_BROWSER_EXT term from the provider gate; rely on SupportsWriteMode (now web-aware) so the browser host gets the read-write DatabaseEditorProvider. - databaseModel.ts: SupportsWriteMode includes IsBrowserExtensionHost. save() persists web edits by serializing the DB and writing back via workspace.fs.writeFile (works on github.dev). - SAFETY: createCheckpoint() now runs only AFTER a successful write (was before), and the web write is wrapped so a rejected write throws a clear 'Failed to save database' and leaves the modification tracker uncommitted — no silent data loss; edits remain recoverable/retriable. Tests: +4 (provider selection in browser mode; SupportsWriteMode; save serialize+writeFile for non-file uri; failed-write-stays-uncommitted). The data-loss safeguard test is revert-proof-verified (fails if the checkpoint reorder is undone). Verified: build OK (extension-browser.js exports activate, worker-free); tsc --noEmit clean; npm test 338/338. NOT yet verified edit->save->persist in real vscode.dev — that requires publishing; will confirm before calling done. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 10 ++ package.json | 2 +- src/databaseModel.ts | 18 ++- src/editorController.ts | 4 +- tests/unit/databaseModel.test.ts | 197 +++++++++++++++++++++++++++- tests/unit/editorController.test.ts | 64 +++++++++ 6 files changed, 287 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f698c5c..c1c8352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## 1.5.0 + +### Features + +- **Editing databases in VS Code for Web (vscode.dev / github.dev)**. The web build was previously read-only — any edit reported "Document is read-only". Editing had been disabled since the original release because the web engine path was unreliable. The in-process WASM engine now works in the web extension host (since 1.3.9), so the read-write editor is enabled in web mode. Edits apply to the in-memory database, then persist on save: the database is serialized and written back through the VS Code filesystem (for example, committing via github.dev). + +### Safety + +- **No silent data loss on web save failure**. When the underlying filesystem rejects a write (for example, a read-only web filesystem provider), save now surfaces a clear "Failed to save database" error. The edit history is kept uncommitted so changes can be retried or recovered, rather than being marked as saved. + ## 1.4.0 ### Changed diff --git a/package.json b/package.json index 3ed7015..2723ab7 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "name": "sqlite-explorer", "displayName": "SQLite Explorer", "description": "A powerful SQLite database viewer and editor for VS Code", - "version": "1.4.0", + "version": "1.5.0", "publisher": "zknpr", "license": "MIT", "repository": { diff --git a/src/databaseModel.ts b/src/databaseModel.ts index 412cf20..ef87f15 100644 --- a/src/databaseModel.ts +++ b/src/databaseModel.ts @@ -44,13 +44,16 @@ const CurrentExtension = vsc.extensions.getExtension(FullExtensionId); /** Running on local machine (not remote) */ const IsLocalMode = !vsc.env.remoteName; +/** Running inside the browser extension host used by VS Code for Web */ +const IsBrowserExtensionHost = !!import.meta.env?.VSCODE_BROWSER_EXT; + /** Running on remote with workspace extension */ export const IsRemoteWorkspaceMode = !!vsc.env.remoteName && CurrentExtension?.extensionKind === vsc.ExtensionKind.Workspace; /** Editor supports read-write operations */ -export const SupportsWriteMode = IsLocalMode || IsRemoteWorkspaceMode; +export const SupportsWriteMode = IsLocalMode || IsRemoteWorkspaceMode || IsBrowserExtensionHost; // ============================================================================ // Configuration @@ -322,7 +325,6 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { */ async save(cancellation?: vsc.CancellationToken): Promise { await this.ensureWritable(); - await this.#modificationTracker.createCheckpoint(); // Check if using native engine - changes are already on disk const engineKind = await this.databaseOperations.engineKind; @@ -336,6 +338,7 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { // Log at debug level for troubleshooting if needed GlobalOutputChannel?.appendLine(`[WAL checkpoint skipped] ${err instanceof Error ? err.message : String(err)}`); } + await this.#modificationTracker.createCheckpoint(); return; } @@ -344,6 +347,7 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { if (this.uri.scheme === 'file') { try { await this.databaseOperations.writeToFile(this.uri.fsPath); + await this.#modificationTracker.createCheckpoint(); return; } catch (e) { // Fallback if direct write fails @@ -353,7 +357,15 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { const { filename } = this.fileParts; const binaryContent = await this.databaseOperations.serializeDatabase(filename); - await vsc.workspace.fs.writeFile(this.uri, binaryContent); + try { + await vsc.workspace.fs.writeFile(this.uri, binaryContent); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to save database: ${message}`); + } + // Only mark the tracker clean after bytes are persisted. If a web filesystem + // rejects writeFile, the edit history remains uncommitted for backup/retry. + await this.#modificationTracker.createCheckpoint(); } /** diff --git a/src/editorController.ts b/src/editorController.ts index 4101601..b9d0156 100644 --- a/src/editorController.ts +++ b/src/editorController.ts @@ -385,7 +385,9 @@ export function registerEditorProvider( // Optional chaining is required: `import.meta.env` is undefined when this module is require()'d // under tsx (unit tests); esbuild's `define` substitutes the value in real builds. Do not make // this a bare access to match workerFactory.ts — that module is never required raw in tests. - const enableReadWrite = !import.meta.env?.VSCODE_BROWSER_EXT && verified && !readOnly && SupportsWriteMode; + // SupportsWriteMode includes the browser extension host only after the + // in-process WASM engine is available, so the provider gate can stay shared. + const enableReadWrite = verified && !readOnly && SupportsWriteMode; const Provider = enableReadWrite ? DatabaseEditorProvider : DatabaseViewerProvider; return vsc.window.registerCustomEditorProvider( viewType, diff --git a/tests/unit/databaseModel.test.ts b/tests/unit/databaseModel.test.ts index bd596ec..785a77b 100644 --- a/tests/unit/databaseModel.test.ts +++ b/tests/unit/databaseModel.test.ts @@ -1,7 +1,51 @@ import './vscode_mock_setup'; import { describe, it, before, after, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert'; +import path from 'node:path'; +import fs from 'node:fs'; +import Module from 'node:module'; +import esbuild from 'esbuild'; import { mockVscode } from './mocks/vscode'; +import { ModificationTracker } from '../../src/core/undo-history'; + +const databaseModelPath = path.resolve(__dirname, '../../src/databaseModel.ts'); +const databaseModelSource = fs.readFileSync(databaseModelPath, 'utf8'); + +function loadBrowserDatabaseModel() { + const jsCode = esbuild.transformSync(databaseModelSource, { + loader: 'ts', + format: 'cjs', + define: { + 'import.meta.env.VSCODE_BROWSER_EXT': 'true' + } + }).code; + + const scriptModule = new Module(databaseModelPath, module as unknown as Module); + scriptModule.filename = databaseModelPath; + scriptModule.paths = (Module as unknown as { _nodeModulePaths(dirname: string): string[] }) + ._nodeModulePaths(path.dirname(databaseModelPath)); + + const originalRequire = Module.prototype.require; + Module.prototype.require = function(request: string) { + if (request === 'vscode') return mockVscode; + if (request.endsWith('workerFactory')) { + return { createDatabaseConnection: async () => ({}) }; + } + if (request.endsWith('main')) { + return { GlobalOutputChannel: null }; + } + return originalRequire.call(this, request); + }; + + try { + (scriptModule as unknown as { _compile(code: string, filename: string): void }) + ._compile(jsCode, databaseModelPath); + } finally { + Module.prototype.require = originalRequire; + } + + return scriptModule.exports as typeof import('../../src/databaseModel'); +} // Setup environment definitions that match VS Code ExtensionKind (mockVscode as any).ExtensionKind = { Workspace: 2, UI: 1 }; @@ -10,6 +54,51 @@ mockVscode.env.remoteName = 'remote'; getExtension: () => ({ extensionKind: 2 }) }; +describe('SupportsWriteMode', () => { + it('allows the browser extension host even when remoteName is set and the extension is UI-kind', () => { + const originalRemoteName = mockVscode.env.remoteName; + const originalExtensionKind = (mockVscode as any).ExtensionKind; + const originalExtensions = (mockVscode as any).extensions; + + Object.defineProperty(mockVscode, 'ExtensionKind', { + value: { Workspace: 2, UI: 1 }, + writable: true, + configurable: true + }); + Object.defineProperty(mockVscode.env, 'remoteName', { + value: 'github', + writable: true, + configurable: true + }); + Object.defineProperty(mockVscode, 'extensions', { + value: { getExtension: () => ({ extensionKind: 1 }) }, + writable: true, + configurable: true + }); + + try { + const { SupportsWriteMode } = loadBrowserDatabaseModel(); + assert.strictEqual(SupportsWriteMode, true); + } finally { + Object.defineProperty(mockVscode, 'ExtensionKind', { + value: originalExtensionKind, + writable: true, + configurable: true + }); + Object.defineProperty(mockVscode.env, 'remoteName', { + value: originalRemoteName, + writable: true, + configurable: true + }); + Object.defineProperty(mockVscode, 'extensions', { + value: originalExtensions, + writable: true, + configurable: true + }); + } + }); +}); + describe('isAutoCommitEnabled', () => { let originalGetConfiguration: any; let configMap: Map; @@ -161,6 +250,21 @@ describe('DatabaseDocument save/saveAs fallback', () => { DatabaseDocument = dbModel.DatabaseDocument; }); + const createUri = (scheme: string, path: string) => { + const uri = { + scheme, + authority: '', + path: path, + query: '', + fragment: '', + fsPath: scheme === 'file' ? path : '', + with: (changes: { path?: string }) => createUri(scheme, changes.path ?? path), + toString: () => `${scheme}://${path}`, + toJSON: () => ({}) + }; + return uri; + }; + const createFileUri = (path: string) => { return { scheme: 'file', @@ -174,7 +278,7 @@ describe('DatabaseDocument save/saveAs fallback', () => { }; }; - const createDocBypassingFactory = (dbOps: any) => { + const createDocBypassingFactory = (dbOps: any, uri: any = createFileUri('/test/db.sqlite')) => { const mockViewerProvider = { reporter: {}, isVerified: true, @@ -182,11 +286,10 @@ describe('DatabaseDocument save/saveAs fallback', () => { forceReadOnly: false, outputChannel: undefined }; - const fileUri = createFileUri('/test/db.sqlite'); return new (DatabaseDocument as any)( mockViewerProvider, - fileUri, + uri, null, // tracker false, // autoCommitEnabled { databaseOps: dbOps, isReadOnly: false }, @@ -300,6 +403,94 @@ describe('DatabaseDocument save/saveAs fallback', () => { console.warn = originalConsoleWarn; } }); + + it('save: serializes and writes WASM database content for non-file URI', async () => { + const sourceUri = createUri('vscode-vfs', '/github/user/repo/test.db'); + let serializedName: string | undefined; + let writeFileCalled = false; + + const dbOps = { + engineKind: Promise.resolve('wasm'), + writeToFile: async () => { throw new Error('writeToFile should not be called for non-file URIs'); }, + serializeDatabase: async (name: string) => { + serializedName = name; + return new Uint8Array([4, 5, 6]); + } + }; + + const doc = createDocBypassingFactory(dbOps, sourceUri); + + const originalFs = mockVscode.workspace.fs; + mockVscode.workspace.fs = { + ...originalFs, + writeFile: async (uri: any, content: any) => { + writeFileCalled = true; + assert.strictEqual(uri, sourceUri); + assert.deepStrictEqual(content, new Uint8Array([4, 5, 6])); + }, + readFile: async () => new Uint8Array([]) + } as any; + + try { + await doc.save(); + + assert.strictEqual(serializedName, 'test.db'); + assert.strictEqual(writeFileCalled, true, 'fs.writeFile should be called'); + } finally { + mockVscode.workspace.fs = originalFs; + } + }); + + it('save: keeps failed non-file WASM writes uncommitted for backup and retry', async () => { + const sourceUri = createUri('vscode-vfs', '/github/user/repo/test.db'); + let backupContent: Uint8Array | undefined; + + const dbOps = { + engineKind: Promise.resolve('wasm'), + serializeDatabase: async () => new Uint8Array([7, 8, 9]) + }; + + const doc = createDocBypassingFactory(dbOps, sourceUri); + doc.recordModification({ + label: 'Update Cell', + description: 'Update items.name', + modificationType: 'cell_update', + targetTable: 'items', + targetRowId: 1, + targetColumn: 'name', + newValue: 'after', + priorValue: 'before' + }); + + const originalFs = mockVscode.workspace.fs; + mockVscode.workspace.fs = { + ...originalFs, + writeFile: async (uri: any, content: any) => { + if (uri.toString() === sourceUri.toString()) { + throw new Error('NoPermissions: read-only filesystem'); + } + backupContent = content; + }, + readFile: async () => new Uint8Array([]) + } as any; + + try { + await assert.rejects( + () => doc.save(), + /NoPermissions: read-only filesystem/ + ); + + await doc.backup(createUri('vscode-userdata', '/backups/test.db'), undefined); + + assert.ok(backupContent, 'backup should be written after failed save'); + const restored = ModificationTracker.deserialize(backupContent); + const uncommitted = restored.getUncommittedEntries(); + assert.strictEqual(uncommitted.length, 1); + assert.strictEqual(uncommitted[0].label, 'Update Cell'); + } finally { + mockVscode.workspace.fs = originalFs; + } + }); }); diff --git a/tests/unit/editorController.test.ts b/tests/unit/editorController.test.ts index bfc5d56..9fad549 100644 --- a/tests/unit/editorController.test.ts +++ b/tests/unit/editorController.test.ts @@ -2,7 +2,12 @@ import './vscode_mock_setup'; import { describe, it, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert'; +import path from 'node:path'; +import fs from 'node:fs'; +import Module from 'node:module'; +import esbuild from 'esbuild'; import { mockVscode } from './mocks/vscode'; +import { HostBridge } from '../../src/hostBridge'; // SupportsWriteMode in databaseModel.ts is evaluated at module-load time from // `vsc.env.remoteName` and `CurrentExtension?.extensionKind`. Set both so the @@ -35,6 +40,47 @@ moduleCache[workerFactoryPath] = { const editorControllerModule = require('../../src/editorController'); const { registerEditorProvider, DatabaseViewerProvider, DatabaseEditorProvider } = editorControllerModule; +const editorControllerPath = path.resolve(__dirname, '../../src/editorController.ts'); +const editorControllerSource = fs.readFileSync(editorControllerPath, 'utf8'); + +function loadBrowserEditorController(supportsWriteMode = true) { + const jsCode = esbuild.transformSync(editorControllerSource, { + loader: 'ts', + format: 'cjs', + define: { + 'import.meta.env.VSCODE_BROWSER_EXT': 'true' + } + }).code; + + const scriptModule = new Module(editorControllerPath, module as unknown as Module); + scriptModule.filename = editorControllerPath; + scriptModule.paths = (Module as unknown as { _nodeModulePaths(dirname: string): string[] }) + ._nodeModulePaths(path.dirname(editorControllerPath)); + + const originalRequire = Module.prototype.require; + Module.prototype.require = function(request: string) { + if (request === 'vscode') return mockVscode; + if (request.endsWith('databaseModel')) { + return { + SupportsWriteMode: supportsWriteMode, + IsRemoteWorkspaceMode: false, + DatabaseDocument: class DatabaseDocument {}, + isAutoCommitEnabled: () => false + }; + } + return originalRequire.call(this, request); + }; + + try { + (scriptModule as unknown as { _compile(code: string, filename: string): void }) + ._compile(jsCode, editorControllerPath); + } finally { + Module.prototype.require = originalRequire; + } + + return scriptModule.exports as typeof import('../../src/editorController'); +} + describe('registerEditorProvider', () => { type RegisterCall = { viewType: string; provider: unknown; options: unknown }; let calls: RegisterCall[]; @@ -85,6 +131,24 @@ describe('registerEditorProvider', () => { assert.ok(calls[0].provider instanceof DatabaseEditorProvider); }); + it('registers the read-write provider in browser mode when write mode is supported', () => { + const { + registerEditorProvider: registerBrowserEditorProvider, + DatabaseEditorProvider: BrowserDatabaseEditorProvider + } = loadBrowserEditorController(true); + + registerBrowserEditorProvider('sqlite-explorer.edit', ctx, undefined, null, { + verified: true, + readOnly: false + }); + + assert.strictEqual(calls.length, 1); + assert.ok(calls[0].provider instanceof BrowserDatabaseEditorProvider); + + const bridge = new HostBridge(calls[0].provider as any, {} as any); + assert.strictEqual(bridge.isReadOnly, false); + }); + it('passes retainContextWhenHidden=false in webview options', () => { registerEditorProvider('sqlite-explorer.view', ctx, undefined, null, { verified: true, readOnly: true }); From 52168984fe3fa3ddb963587bbaeffbccf3d04939 Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 12:14:36 +0200 Subject: [PATCH 02/10] =?UTF-8?q?fix(web):=20harden=20web=20edit=20against?= =?UTF-8?q?=20data=20loss=20=E2=80=94=203=20P1=20fixes=20(#424=20review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review found 3 confirmed data-integrity bugs that made web editing unsafe. All fixed: P1-A — WAL pages dropped on save. sql.js opens only the main DB image and cannot merge a separate -wal, so saving a WAL-mode DB would overwrite the main file and drop committed WAL pages. Now: when walContent is non-empty, the browser connection opens read-only (viewable, not silently corruptible). P1-B — undo/redo/revert/hot-exit were no-ops. The in-process facade wired applyModifications/undoModification/redoModification/flushChanges/ discardModifications as async()=>{}, so VS Code Undo/Revert/backup-restore marked document state handled while the in-memory DB was unchanged (zombie/lost edits). createWorkerEndpoint() now exposes these ops, and the facade delegates to the already-implemented WasmDatabaseEngine methods. P1-C — checkpoint raced an in-flight save. createCheckpoint() ran after the async writeFile, marking clean any edit that arrived during the write (dropped on reload). ModificationTracker gains getCurrentPosition()/createCheckpointAt(); save() snapshots the position right after serializeDatabase() and only commits that snapshot after a successful write. Position is absolute (timelineOffset) so it survives front-eviction during the write. Tests +3, each revert-proof-verified (fail when its fix is undone): WAL-read-only, undo-delegation, save-race-stays-dirty. Verified: build OK (extension-browser.js exports activate, worker-free); tsc --noEmit clean; npm test 341/341. Desktop path unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/sqlite-db.ts | 25 ++++- src/core/undo-history.ts | 24 ++++ src/databaseModel.ts | 8 +- src/workerFactory.ts | 22 +++- tests/unit/databaseModel.test.ts | 72 ++++++++++++ tests/unit/workerFactory_browser.test.ts | 136 ++++++++++++++++++++++- 6 files changed, 278 insertions(+), 9 deletions(-) diff --git a/src/core/sqlite-db.ts b/src/core/sqlite-db.ts index 8d8928a..9b06a47 100644 --- a/src/core/sqlite-db.ts +++ b/src/core/sqlite-db.ts @@ -21,7 +21,8 @@ import type { TableCountOptions, SchemaSnapshot, ColumnMetadata, - ColumnDefinition + ColumnDefinition, + ModificationEntry } from './types'; import { getNodeFs } from './platform/fs'; import { @@ -174,6 +175,28 @@ export function createWorkerEndpoint() { return requireEngine().serializeDatabase(name); }, + // Expose undo/history operations for the browser in-process facade, which + // calls this endpoint directly instead of going through worker RPC. + async applyModifications(mods: ModificationEntry[], signal?: AbortSignal): Promise { + return requireEngine().applyModifications(mods, signal); + }, + + async undoModification(mod: ModificationEntry): Promise { + return requireEngine().undoModification(mod); + }, + + async redoModification(mod: ModificationEntry): Promise { + return requireEngine().redoModification(mod); + }, + + async flushChanges(signal?: AbortSignal): Promise { + return requireEngine().flushChanges(signal); + }, + + async discardModifications(mods: ModificationEntry[], signal?: AbortSignal): Promise { + return requireEngine().discardModifications(mods, signal); + }, + async updateCell(table: string, rowId: RecordId, column: string, value: CellValue): Promise { return requireEngine().updateCell(table, rowId, column, value); }, diff --git a/src/core/undo-history.ts b/src/core/undo-history.ts index 3ca36e5..4d0dc29 100644 --- a/src/core/undo-history.ts +++ b/src/core/undo-history.ts @@ -142,6 +142,8 @@ interface TrackerState { export class ModificationTracker { private timeline: T[] = []; private timelineSizes: number[] = []; + /** Number of entries that were evicted from the front of the retained timeline. */ + private timelineOffset: number = 0; private futureStack: T[] = []; private futureStackSizes: number[] = []; @@ -208,6 +210,7 @@ export class ModificationTracker { + const relativePosition = position - this.timelineOffset; + this.checkpointIndex = Math.max(0, Math.min(this.timeline.length, relativePosition)); + } + /** * Get all modifications since last checkpoint. * diff --git a/src/databaseModel.ts b/src/databaseModel.ts index ef87f15..4bfab61 100644 --- a/src/databaseModel.ts +++ b/src/databaseModel.ts @@ -357,6 +357,10 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { const { filename } = this.fileParts; const binaryContent = await this.databaseOperations.serializeDatabase(filename); + // Capture the tracker position immediately after serialization. The bytes + // below represent edits up to this position only; edits recorded while the + // asynchronous workspace write is pending must remain dirty. + const serializedCheckpoint = this.#modificationTracker.getCurrentPosition(); try { await vsc.workspace.fs.writeFile(this.uri, binaryContent); } catch (err) { @@ -365,7 +369,9 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { } // Only mark the tracker clean after bytes are persisted. If a web filesystem // rejects writeFile, the edit history remains uncommitted for backup/retry. - await this.#modificationTracker.createCheckpoint(); + // The saved checkpoint is limited to the serialized snapshot so concurrent + // edits are not acknowledged before their bytes reach storage. + await this.#modificationTracker.createCheckpointAt(serializedCheckpoint); } /** diff --git a/src/workerFactory.ts b/src/workerFactory.ts index 90c6e0a..26a79f9 100644 --- a/src/workerFactory.ts +++ b/src/workerFactory.ts @@ -214,6 +214,11 @@ async function createInProcessWasmDatabaseConnection( // There is no file-path fast path because the web extension host cannot // access local disk paths directly. const [dbContent, walContent] = await loadDatabaseFiles(fileUri); + const hasActiveWal = !!walContent && walContent.byteLength > 0; + // sql.js opens one main database image and cannot merge a separate WAL + // file, so browser editing is disabled when committed WAL pages may be + // absent from the main database bytes that save() would later overwrite. + const readOnlyMode = (forceReadOnly ?? false) || hasActiveWal; // Preload sql.js WASM bytes from the extension assets directory so // WebAssembly instantiation does not depend on worker-relative URLs. @@ -226,7 +231,7 @@ async function createInProcessWasmDatabaseConnection( maxSize: getMaximumFileSizeBytes(), resourceMap: {}, wasmBinary: wasmContent, - readOnlyMode: forceReadOnly ?? false, + readOnlyMode, queryTimeout: getQueryTimeout() }; @@ -237,11 +242,16 @@ async function createInProcessWasmDatabaseConnection( executeQuery: (sql: string, params?: CellValue[]) => endpoint.runQuery(sql, params), serializeDatabase: (name: string) => endpoint.exportDatabase(name), - applyModifications: async () => {}, - undoModification: async () => {}, - redoModification: async () => {}, - flushChanges: async () => {}, - discardModifications: async () => {}, + applyModifications: (mods: ModificationEntry[], signal?: AbortSignal) => + endpoint.applyModifications(mods, signal), + undoModification: (mod: ModificationEntry) => + endpoint.undoModification(mod), + redoModification: (mod: ModificationEntry) => + endpoint.redoModification(mod), + flushChanges: (signal?: AbortSignal) => + endpoint.flushChanges(signal), + discardModifications: (mods: ModificationEntry[], signal?: AbortSignal) => + endpoint.discardModifications(mods, signal), updateCell: (table: string, rowId: string | number, column: string, value: CellValue) => endpoint.updateCell(table, rowId, column, value), insertRow: (table: string, data: Record) => diff --git a/tests/unit/databaseModel.test.ts b/tests/unit/databaseModel.test.ts index 785a77b..82fff4e 100644 --- a/tests/unit/databaseModel.test.ts +++ b/tests/unit/databaseModel.test.ts @@ -491,6 +491,78 @@ describe('DatabaseDocument save/saveAs fallback', () => { mockVscode.workspace.fs = originalFs; } }); + + it('save: does not checkpoint edits recorded while writeFile is still pending', async () => { + const sourceUri = createUri('vscode-vfs', '/github/user/repo/test.db'); + let resolveWrite: () => void = () => {}; + let markWriteStarted: () => void = () => {}; + const writeMayFinish = new Promise(resolve => { + resolveWrite = resolve; + }); + const writeStarted = new Promise(resolve => { + markWriteStarted = resolve; + }); + + const firstModification = { + label: 'First Update', + description: 'Update first item', + modificationType: 'cell_update' as const, + targetTable: 'items', + targetRowId: 1, + targetColumn: 'name', + priorValue: 'before', + newValue: 'after' + }; + const concurrentModification = { + label: 'Concurrent Update', + description: 'Update concurrent item', + modificationType: 'cell_update' as const, + targetTable: 'items', + targetRowId: 2, + targetColumn: 'name', + priorValue: 'old', + newValue: 'new' + }; + let discardedModifications: unknown[] | undefined; + + const dbOps = { + engineKind: Promise.resolve('wasm'), + serializeDatabase: async () => new Uint8Array([7, 8, 9]), + discardModifications: async (mods: unknown[]) => { + discardedModifications = mods; + } + }; + + const doc = createDocBypassingFactory(dbOps, sourceUri); + doc.recordModification(firstModification); + + const originalFs = mockVscode.workspace.fs; + mockVscode.workspace.fs = { + ...originalFs, + writeFile: async (uri: any, content: any) => { + assert.strictEqual(uri, sourceUri); + assert.deepStrictEqual(content, new Uint8Array([7, 8, 9])); + markWriteStarted(); + await writeMayFinish; + }, + readFile: async () => new Uint8Array([]) + } as any; + + try { + const savePromise = doc.save(); + await writeStarted; + + doc.recordModification(concurrentModification); + resolveWrite(); + await savePromise; + + await doc.revert(undefined); + + assert.deepStrictEqual(discardedModifications, [concurrentModification]); + } finally { + mockVscode.workspace.fs = originalFs; + } + }); }); diff --git a/tests/unit/workerFactory_browser.test.ts b/tests/unit/workerFactory_browser.test.ts index 5a5d01d..790bb4e 100644 --- a/tests/unit/workerFactory_browser.test.ts +++ b/tests/unit/workerFactory_browser.test.ts @@ -7,7 +7,7 @@ import fs from 'node:fs'; import Module from 'node:module'; import esbuild from 'esbuild'; import { mockVscode } from './mocks/vscode'; -import type { CellUpdate, CellValue, DatabaseInitConfig } from '../../src/core/types'; +import type { CellUpdate, CellValue, DatabaseInitConfig, ModificationEntry } from '../../src/core/types'; const workerFactoryPath = path.resolve(__dirname, '../../src/workerFactory.ts'); const workerFactorySource = fs.readFileSync(workerFactoryPath, 'utf8'); @@ -16,6 +16,11 @@ interface FakeEndpoint { initializeDatabase(filename: string, config: DatabaseInitConfig): Promise<{ isReadOnly: boolean }>; runQuery(sql: string, params?: CellValue[]): Promise; exportDatabase(name: string): Promise; + applyModifications?(mods: ModificationEntry[], signal?: AbortSignal): Promise; + undoModification?(mod: ModificationEntry): Promise; + redoModification?(mod: ModificationEntry): Promise; + flushChanges?(signal?: AbortSignal): Promise; + discardModifications?(mods: ModificationEntry[], signal?: AbortSignal): Promise; updateCell(table: string, rowId: string | number, column: string, value: CellValue): Promise; insertRow(table: string, data: Record): Promise; updateCellBatch(table: string, updates: CellUpdate[]): Promise; @@ -167,4 +172,133 @@ describe('workerFactory browser WASM connection', () => { assert.strictEqual(insertRowValue, blobValue); assert.strictEqual(updateBatchValue, blobValue); }); + + it('opens browser WAL databases read-only instead of silently writing without WAL pages', async () => { + const dbContent = new Uint8Array([1, 2, 3]); + const walContent = new Uint8Array([9, 9, 9]); + const wasmContent = new Uint8Array([4, 5, 6]); + let initConfig: DatabaseInitConfig | undefined; + + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + stat: async () => ({ size: dbContent.byteLength }), + readFile: async (uri: { path?: string; fsPath?: string }) => { + const pathValue = uri.path ?? uri.fsPath ?? ''; + if (pathValue.endsWith('-wal')) { + return walContent; + } + if (pathValue.endsWith('sqlite3.wasm')) { + return wasmContent; + } + return dbContent; + } + }, + writable: true, + configurable: true + }); + + const endpoint: FakeEndpoint = { + initializeDatabase: async (_filename, config) => { + initConfig = config; + return { isReadOnly: config.readOnlyMode ?? false }; + }, + runQuery: async () => [], + exportDatabase: async () => new Uint8Array(), + updateCell: async () => {}, + insertRow: async () => 1, + updateCellBatch: async () => {}, + ping: async () => true + }; + + const workerFactory = loadBrowserWorkerFactory(endpoint); + const extensionUri = { scheme: 'vscode-vfs', fsPath: '/ext', path: '/ext' } as any; + const fileUri = { + scheme: 'vscode-vfs', + fsPath: '/workspace/test.db', + path: '/workspace/test.db', + with: ({ path: nextPath }: { path: string }) => ({ + scheme: 'vscode-vfs', + fsPath: nextPath, + path: nextPath + }) + } as any; + + const bundle = await workerFactory.createDatabaseConnection(extensionUri, null as any); + const connection = await bundle.establishConnection(fileUri, 'test.db'); + + assert.strictEqual(initConfig?.walContent, walContent); + assert.strictEqual(initConfig?.readOnlyMode, true); + assert.strictEqual(connection.isReadOnly, true); + }); + + it('delegates in-process modification operations through the endpoint', async () => { + const calls: string[] = []; + const mod = { + label: 'Update', + description: 'Update item', + modificationType: 'cell_update' as const, + targetTable: 'items', + targetRowId: 1, + targetColumn: 'name', + priorValue: 'before', + newValue: 'after' + }; + const abortController = new AbortController(); + + const endpoint: FakeEndpoint = { + initializeDatabase: async () => ({ isReadOnly: false }), + runQuery: async () => [], + exportDatabase: async () => new Uint8Array(), + applyModifications: async (mods, signal) => { + assert.deepStrictEqual(mods, [mod]); + assert.strictEqual(signal, abortController.signal); + calls.push('apply'); + }, + undoModification: async (entry) => { + assert.strictEqual(entry, mod); + calls.push('undo'); + }, + redoModification: async (entry) => { + assert.strictEqual(entry, mod); + calls.push('redo'); + }, + flushChanges: async (signal) => { + assert.strictEqual(signal, abortController.signal); + calls.push('flush'); + }, + discardModifications: async (mods, signal) => { + assert.deepStrictEqual(mods, [mod]); + assert.strictEqual(signal, abortController.signal); + calls.push('discard'); + }, + updateCell: async () => {}, + insertRow: async () => 1, + updateCellBatch: async () => {}, + ping: async () => true + }; + + const workerFactory = loadBrowserWorkerFactory(endpoint); + const extensionUri = { scheme: 'vscode-vfs', fsPath: '/ext', path: '/ext' } as any; + const fileUri = { + scheme: 'vscode-vfs', + fsPath: '/workspace/test.db', + path: '/workspace/test.db', + with: ({ path: nextPath }: { path: string }) => ({ + scheme: 'vscode-vfs', + fsPath: nextPath, + path: nextPath + }) + } as any; + + const bundle = await workerFactory.createDatabaseConnection(extensionUri, null as any); + const connection = await bundle.establishConnection(fileUri, 'test.db'); + + await connection.databaseOps.applyModifications([mod], abortController.signal); + await connection.databaseOps.undoModification(mod); + await connection.databaseOps.redoModification(mod); + await connection.databaseOps.flushChanges(abortController.signal); + await connection.databaseOps.discardModifications([mod], abortController.signal); + + assert.deepStrictEqual(calls, ['apply', 'undo', 'redo', 'flush', 'discard']); + }); }); From 768c67996773d203f356dd2e886cc61d1183f386 Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 12:37:34 +0200 Subject: [PATCH 03/10] fix(web): close 2 more web-edit data-integrity holes (#424 review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the P1 fixes found two more issues, both in those fixes: 1. WAL read-only guard was incomplete (P1). The browser WAL guard marked the connection/document read-only, but hostBridge.isReadOnly only checked the provider (DatabaseEditorProvider, isReadOnly=false in web), so the webview still allowed edits on a WAL database that save()/revert() then rejected — a dead-end with unsavable changes. hostBridge.isReadOnly now also honors document.isReadOnlyMode, so the edit gate is consistent with the connection. 2. undo/redo DURING an in-flight save could still lose data (P2). The serialized-position snapshot handled edits ADDED during the async write but not timeline SHRINKING: if the user undoes an edit while writeFile is pending, createCheckpointAt clamps to the shortened timeline and marks bytes clean that no longer match the in-memory state, silently losing the undo on reload. ModificationTracker now tracks a checkpointInvalidationRevision (bumped by undo/rollback/eviction); save() captures it before the write and only commits the checkpoint if it is unchanged afterward — otherwise the document stays dirty so the next save reconciles. Normal desktop save (no concurrent undo) is unaffected. Also fixed a stale test: editorController.test.ts passed {} as the mock document; with hostBridge.isReadOnly now reading document.isReadOnlyMode, the mock must expose it (isReadOnlyMode: false). Tests +2, both revert-proof-verified. tsc clean; npm test 343/343. Desktop unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/undo-history.ts | 50 +++++++++++++++++ src/databaseModel.ts | 15 +++++- src/hostBridge.ts | 9 +++- tests/unit/databaseModel.test.ts | 83 +++++++++++++++++++++++++++++ tests/unit/editorController.test.ts | 4 +- tests/unit/hostBridge.test.ts | 47 ++++++++++++++++ 6 files changed, 205 insertions(+), 3 deletions(-) diff --git a/src/core/undo-history.ts b/src/core/undo-history.ts index 4d0dc29..6c86022 100644 --- a/src/core/undo-history.ts +++ b/src/core/undo-history.ts @@ -152,6 +152,10 @@ export class ModificationTracker { mockVscode.workspace.fs = originalFs; } }); + + it('save: stays dirty when undo shrinks the timeline while writeFile is still pending', async () => { + const sourceUri = createUri('vscode-vfs', '/github/user/repo/test.db'); + let resolveWrite: () => void = () => {}; + let markWriteStarted: () => void = () => {}; + const writeMayFinish = new Promise(resolve => { + resolveWrite = resolve; + }); + const writeStarted = new Promise(resolve => { + markWriteStarted = resolve; + }); + + const firstModification = { + label: 'First Update', + description: 'Update first item', + modificationType: 'cell_update' as const, + targetTable: 'items', + targetRowId: 1, + targetColumn: 'name', + priorValue: 'before-a', + newValue: 'after-a' + }; + const undoneModification = { + label: 'Second Update', + description: 'Update second item', + modificationType: 'cell_update' as const, + targetTable: 'items', + targetRowId: 2, + targetColumn: 'name', + priorValue: 'before-b', + newValue: 'after-b' + }; + let undoSecondUpdate: (() => Promise) | undefined; + let discardedModifications: unknown[] | undefined; + + const dbOps = { + engineKind: Promise.resolve('wasm'), + serializeDatabase: async () => new Uint8Array([10, 11, 12]), + undoModification: async (modification: unknown) => { + assert.strictEqual(modification, undoneModification); + }, + discardModifications: async (mods: unknown[]) => { + discardedModifications = mods; + } + }; + + const doc = createDocBypassingFactory(dbOps, sourceUri); + doc.recordModification(firstModification); + doc.onDidChange((modification: any) => { + if (modification.label === 'Second Update') { + undoSecondUpdate = modification.undo; + } + }); + doc.recordModification(undoneModification); + assert.ok(undoSecondUpdate, 'second update undo action should be emitted'); + + const originalFs = mockVscode.workspace.fs; + mockVscode.workspace.fs = { + ...originalFs, + writeFile: async (uri: any, content: any) => { + assert.strictEqual(uri, sourceUri); + assert.deepStrictEqual(content, new Uint8Array([10, 11, 12])); + markWriteStarted(); + await writeMayFinish; + }, + readFile: async () => new Uint8Array([]) + } as any; + + try { + const savePromise = doc.save(); + await writeStarted; + + await undoSecondUpdate!(); + resolveWrite(); + await savePromise; + + await doc.revert(undefined); + + assert.deepStrictEqual(discardedModifications, [firstModification]); + } finally { + mockVscode.workspace.fs = originalFs; + } + }); }); diff --git a/tests/unit/editorController.test.ts b/tests/unit/editorController.test.ts index 9fad549..cdd17a7 100644 --- a/tests/unit/editorController.test.ts +++ b/tests/unit/editorController.test.ts @@ -145,7 +145,9 @@ describe('registerEditorProvider', () => { assert.strictEqual(calls.length, 1); assert.ok(calls[0].provider instanceof BrowserDatabaseEditorProvider); - const bridge = new HostBridge(calls[0].provider as any, {} as any); + // HostBridge.isReadOnly now also reflects the document's connection-level + // read-only state, so the mock document must expose isReadOnlyMode. + const bridge = new HostBridge(calls[0].provider as any, { isReadOnlyMode: false } as any); assert.strictEqual(bridge.isReadOnly, false); }); diff --git a/tests/unit/hostBridge.test.ts b/tests/unit/hostBridge.test.ts index 8991774..dfafcd2 100644 --- a/tests/unit/hostBridge.test.ts +++ b/tests/unit/hostBridge.test.ts @@ -148,6 +148,53 @@ describe('HostBridge', () => { assert.strictEqual(dbOps.deleteRows.mock.callCount(), 1); }); + it('treats connection-level read-only documents as read-only for web mutators', async () => { + const dbOps = { + updateCell: mock.fn(async () => {}), + insertRow: mock.fn(async () => 1), + deleteRows: mock.fn(async () => {}), + updateCellBatch: mock.fn(async () => {}), + executeQuery: mock.fn(async () => []) + }; + const mockDocument = { + uri: vscode.Uri.parse('file:///test.db'), + documentKey: Promise.resolve('test-key'), + databaseOperations: dbOps, + isReadOnlyMode: true, + recordExternalModification: mock.fn() + }; + const mockProvider = { + webviews: new Map(), + context: {}, + isReadOnly: false + }; + const bridge = new HostBridge(mockProvider as any, mockDocument as any); + + assert.strictEqual(bridge.isReadOnly, true); + await assert.rejects( + () => bridge.updateCell('table1', 1, 'name', 'after', 'before'), + /Document is read-only/ + ); + await assert.rejects( + () => bridge.insertRow('table1', { name: 'new' }), + /Document is read-only/ + ); + await assert.rejects( + () => bridge.deleteRows('table1', [1]), + /Document is read-only/ + ); + await assert.rejects( + () => bridge.updateCellBatch('table1', [{ rowId: 1, column: 'name', value: 'after' }], 'Batch update'), + /Document is read-only/ + ); + + assert.strictEqual(dbOps.updateCell.mock.callCount(), 0); + assert.strictEqual(dbOps.insertRow.mock.callCount(), 0); + assert.strictEqual(dbOps.deleteRows.mock.callCount(), 0); + assert.strictEqual(dbOps.updateCellBatch.mock.callCount(), 0); + assert.strictEqual(mockDocument.recordExternalModification.mock.callCount(), 0); + }); + it('should catch and log error if fetch columns for undo history fails in deleteColumns', async () => { const consoleWarnMock = mock.method(console, 'warn', () => {}); const error = new Error('Database disconnected during column info fetch'); From 136ef2329cdc1b3efd3776251e18314604ff5e79 Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 13:00:38 +0200 Subject: [PATCH 04/10] fix(web): implement applyModifications replay + guard file:// save race (#424 review round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more confirmed data-integrity issues, both in/under the prior fixes: 1. (P1) WasmDatabaseEngine.applyModifications was an empty no-op, so the round-1 'fix' that delegated the in-process facade to it did nothing real: hot-exit backup restore (DatabaseDocument.create replays tracker.getUncommittedEntries() via applyModifications) silently dropped the recovered edits — the doc looked restored but the in-memory DB held the last saved bytes, and the next save discarded the edits. applyModifications now replays each entry forward. Extracted a shared private forwardApply(mod, strict) used by BOTH applyModifications (strict: throws on malformed/ unsupported entries so restore fails loud) and redoModification (non-strict: unchanged historical behavior). Covers all ModificationType values; respects the abort signal between entries. 2. (P2) The file:// direct-save branch (writeToFile) still used a plain createCheckpoint after the async write, so a concurrent edit/undo during an in-flight direct save could mark mismatched bytes clean. It now uses the same capture-position + checkpointInvalidationRevision guard as the workspace.fs.writeFile branch (commit checkpoint only if the timeline didn't change during the write; else stay dirty). Tests +4 (applyModifications actually replays; restore replays a dirty backup; restore aborts on a pre-aborted signal; file:// save stays dirty on concurrent undo). Both fixes revert-proof-verified. tsc clean; npm test 347/347. redo and desktop behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/engine/wasm/WasmDatabaseEngine.ts | 65 ++++++-- src/databaseModel.ts | 14 +- tests/unit/databaseModel.test.ts | 184 +++++++++++++++++++++ tests/unit/sqlite_db.test.ts | 68 +++++++- 4 files changed, 315 insertions(+), 16 deletions(-) diff --git a/src/core/engine/wasm/WasmDatabaseEngine.ts b/src/core/engine/wasm/WasmDatabaseEngine.ts index 2685152..ab9b32d 100644 --- a/src/core/engine/wasm/WasmDatabaseEngine.ts +++ b/src/core/engine/wasm/WasmDatabaseEngine.ts @@ -183,13 +183,21 @@ export class WasmDatabaseEngine implements DatabaseOperations { /** * Apply a batch of modifications. - * Currently no-op as modifications are applied via executeQuery. + * + * Hot-exit restore opens the last saved database bytes first, then replays + * the serialized uncommitted edit entries into that fresh in-memory database. + * Each entry is applied through the same forward path used by redo so restore + * and redo cannot drift apart. */ async applyModifications( - _mods: ModificationEntry[], - _signal?: AbortSignal + mods: ModificationEntry[], + signal?: AbortSignal ): Promise { - // Modifications applied directly through executeQuery + signal?.throwIfAborted(); + for (const mod of mods) { + await this.forwardApply(mod, true); + signal?.throwIfAborted(); + } } /** @@ -331,16 +339,24 @@ export class WasmDatabaseEngine implements DatabaseOperations { } /** - * Redo a modification. + * Apply one modification in the forward direction. + * + * `strict` is enabled for hot-exit restore so malformed or unsupported + * entries fail loudly instead of silently dropping recovered edits. Redo uses + * the historical non-strict behavior so existing undo/redo semantics are not + * changed for entries that lack enough data to replay. */ - async redoModification(mod: ModificationEntry): Promise { + private async forwardApply(mod: ModificationEntry, strict: boolean): Promise { const { modificationType, targetTable, targetRowId, targetColumn, newValue, affectedCells, affectedRowIds, rowData, tableDef, columnDef, deletedColumns } = mod; - if (!targetTable) return; + if (!targetTable) { + if (strict) throw new Error(`Cannot apply ${modificationType}: missing target table`); + return; + } switch (modificationType) { case 'cell_update': if (affectedCells) { - // Batch redo + // Batch cell updates preserve the original per-cell order and values. const updates = affectedCells.map(cell => ({ rowId: cell.rowId, column: cell.columnName, @@ -349,51 +365,72 @@ export class WasmDatabaseEngine implements DatabaseOperations { await this.updateCellBatch(targetTable, updates); } else if (targetRowId !== undefined && targetColumn) { await this.updateCell(targetTable, targetRowId, targetColumn, newValue ?? null); + } else if (strict) { + throw new Error('Cannot apply cell_update: missing target cell or affected cells'); } break; case 'row_insert': - // Redo insert = insert again if (rowData) { - // If we have the original rowId, enforce it to maintain history consistency + // If the history captured a rowid, include it so restored rows + // keep the same identity they had before shutdown. const dataToInsert = targetRowId !== undefined ? { ...rowData, rowid: targetRowId } : rowData; await this.insertRow(targetTable, dataToInsert); + } else if (strict) { + throw new Error('Cannot apply row_insert: missing row data'); } break; case 'row_delete': - // Redo delete = delete rows if (affectedRowIds) { await this.deleteRows(targetTable, affectedRowIds); + } else if (strict) { + throw new Error('Cannot apply row_delete: missing affected row ids'); } break; case 'column_add': - // Redo add column = add column if (targetColumn && columnDef) { await this.addColumn(targetTable, targetColumn, columnDef.type, columnDef.defaultValue); + } else if (strict) { + throw new Error('Cannot apply column_add: missing column definition'); } break; case 'column_drop': - // Redo drop column = drop column if (deletedColumns) { const colNames = deletedColumns.map(c => c.name); await this.deleteColumns(targetTable, colNames); + } else if (strict) { + throw new Error('Cannot apply column_drop: missing deleted column data'); } break; case 'table_create': - // Redo create table if (tableDef && tableDef.columns) { await this.createTable(targetTable, tableDef.columns); + } else if (strict) { + throw new Error('Cannot apply table_create: missing table definition'); + } + break; + + case 'table_drop': + if (strict) { + throw new Error('Cannot apply table_drop: forward replay is not supported'); } break; } } + /** + * Redo a modification. + */ + async redoModification(mod: ModificationEntry): Promise { + await this.forwardApply(mod, false); + } + /** * Flush changes to storage. * No-op for in-memory database; actual persistence via serializeDatabase. diff --git a/src/databaseModel.ts b/src/databaseModel.ts index dcd42f7..1a37354 100644 --- a/src/databaseModel.ts +++ b/src/databaseModel.ts @@ -346,8 +346,20 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { // We always do this for WASM, regardless of auto-commit setting, because WASM is in-memory. if (this.uri.scheme === 'file') { try { + // Capture the tracker position that matches the database snapshot + // exported by writeToFile(). If undo, rollback, or history eviction + // changes the retained timeline while the async filesystem write is + // pending, the saved bytes no longer match the live tracker state. + const fileCheckpoint = this.#modificationTracker.getCurrentPosition(); + const fileCheckpointInvalidationRevision = + this.#modificationTracker.getCheckpointInvalidationRevision(); await this.databaseOperations.writeToFile(this.uri.fsPath); - await this.#modificationTracker.createCheckpoint(); + if ( + this.#modificationTracker.getCheckpointInvalidationRevision() === + fileCheckpointInvalidationRevision + ) { + await this.#modificationTracker.createCheckpointAt(fileCheckpoint); + } return; } catch (e) { // Fallback if direct write fails diff --git a/tests/unit/databaseModel.test.ts b/tests/unit/databaseModel.test.ts index 515397f..e7ba60b 100644 --- a/tests/unit/databaseModel.test.ts +++ b/tests/unit/databaseModel.test.ts @@ -7,6 +7,8 @@ import Module from 'node:module'; import esbuild from 'esbuild'; import { mockVscode } from './mocks/vscode'; import { ModificationTracker } from '../../src/core/undo-history'; +import { createDatabaseEngine } from '../../src/core/sqlite-db'; +import type { DatabaseOperations, LabeledModification } from '../../src/core/types'; const databaseModelPath = path.resolve(__dirname, '../../src/databaseModel.ts'); const databaseModelSource = fs.readFileSync(databaseModelPath, 'utf8'); @@ -646,6 +648,188 @@ describe('DatabaseDocument save/saveAs fallback', () => { mockVscode.workspace.fs = originalFs; } }); + + it('save: stays dirty when undo shrinks the timeline while file writeToFile is still pending', async () => { + const sourceUri = createFileUri('/test/db.sqlite'); + let resolveWrite: () => void = () => {}; + let markWriteStarted: () => void = () => {}; + const writeMayFinish = new Promise(resolve => { + resolveWrite = resolve; + }); + const writeStarted = new Promise(resolve => { + markWriteStarted = resolve; + }); + + const firstModification = { + label: 'First File Update', + description: 'Update first file item', + modificationType: 'cell_update' as const, + targetTable: 'items', + targetRowId: 1, + targetColumn: 'name', + priorValue: 'before-a', + newValue: 'after-a' + }; + const undoneModification = { + label: 'Second File Update', + description: 'Update second file item', + modificationType: 'cell_update' as const, + targetTable: 'items', + targetRowId: 2, + targetColumn: 'name', + priorValue: 'before-b', + newValue: 'after-b' + }; + let undoSecondUpdate: (() => Promise) | undefined; + let discardedModifications: unknown[] | undefined; + + const dbOps = { + engineKind: Promise.resolve('wasm'), + writeToFile: async (filePath: string) => { + assert.strictEqual(filePath, '/test/db.sqlite'); + markWriteStarted(); + await writeMayFinish; + }, + serializeDatabase: async () => { + throw new Error('serializeDatabase should not be called when file writeToFile succeeds'); + }, + undoModification: async (modification: unknown) => { + assert.strictEqual(modification, undoneModification); + }, + discardModifications: async (mods: unknown[]) => { + discardedModifications = mods; + } + }; + + const doc = createDocBypassingFactory(dbOps, sourceUri); + doc.recordModification(firstModification); + doc.onDidChange((modification: any) => { + if (modification.label === 'Second File Update') { + undoSecondUpdate = modification.undo; + } + }); + doc.recordModification(undoneModification); + assert.ok(undoSecondUpdate, 'second update undo action should be emitted'); + + const savePromise = doc.save(); + await writeStarted; + + await undoSecondUpdate!(); + resolveWrite(); + await savePromise; + + await doc.revert(undefined); + + assert.deepStrictEqual(discardedModifications, [firstModification]); + }); +}); + +describe('DatabaseDocument hot-exit restore', () => { + it('replays backup modifications into the restored WASM database', async () => { + const result = await createDatabaseEngine({ + content: null, + maxSize: 0, + readOnlyMode: false + }); + const engine = result.operations as DatabaseOperations & { shutdown?: () => void }; + await engine.executeQuery("CREATE TABLE restored_items (id INTEGER PRIMARY KEY, name TEXT)"); + + const restoredEntries: LabeledModification[] = [ + { + label: 'Insert Restored Item', + description: 'Insert restored item', + modificationType: 'row_insert', + targetTable: 'restored_items', + targetRowId: 1, + rowData: { id: 1, name: 'Draft' } + }, + { + label: 'Update Restored Item', + description: 'Update restored item', + modificationType: 'cell_update', + targetTable: 'restored_items', + targetRowId: 1, + targetColumn: 'name', + priorValue: 'Draft', + newValue: 'Recovered' + } + ]; + const tracker = new ModificationTracker(100); + for (const entry of restoredEntries) { + tracker.record(entry); + } + + const backupData = tracker.serialize(); + let applyWasCalled = false; + let appliedModificationCount = 0; + const originalApplyModifications = engine.applyModifications.bind(engine); + engine.applyModifications = async (mods, signal) => { + applyWasCalled = true; + appliedModificationCount = mods.length; + return originalApplyModifications(mods, signal); + }; + + const originalFs = mockVscode.workspace.fs; + const moduleCache = require('module')._cache; + const workerFactoryPath = require.resolve('../../src/workerFactory'); + const originalWorkerFactoryCacheEntry = moduleCache[workerFactoryPath]; + const databaseModelModulePath = require.resolve('../../src/databaseModel'); + + mockVscode.workspace.fs = { + ...originalFs, + readFile: async () => backupData + } as any; + + moduleCache[workerFactoryPath] = { + id: workerFactoryPath, + filename: workerFactoryPath, + loaded: true, + exports: { + createDatabaseConnection: async () => ({ + establishConnection: async () => ({ + databaseOps: engine, + isReadOnly: false + }), + workerMethods: { + [Symbol.dispose]: () => engine.shutdown?.() + } + }) + } + }; + delete moduleCache[databaseModelModulePath]; + + try { + const { DatabaseDocument } = require('../../src/databaseModel'); + await DatabaseDocument.create( + { + reporter: undefined, + isVerified: true, + context: { extensionUri: mockVscode.Uri.file('/ext') }, + forceReadOnly: false, + outputChannel: undefined + }, + mockVscode.Uri.file('/test/restored.db'), + { backupId: 'vscode-userdata:///backup/restored.db' } + ); + + const restoredRows = await engine.executeQuery( + "SELECT id, name FROM restored_items ORDER BY id" + ); + + assert.strictEqual(applyWasCalled, true); + assert.strictEqual(appliedModificationCount, 2); + assert.deepStrictEqual(restoredRows[0].rows, [[1, 'Recovered']]); + } finally { + mockVscode.workspace.fs = originalFs; + if (originalWorkerFactoryCacheEntry) { + moduleCache[workerFactoryPath] = originalWorkerFactoryCacheEntry; + } else { + delete moduleCache[workerFactoryPath]; + } + delete moduleCache[databaseModelModulePath]; + engine.shutdown?.(); + } + }); }); diff --git a/tests/unit/sqlite_db.test.ts b/tests/unit/sqlite_db.test.ts index 626abb0..50d889e 100644 --- a/tests/unit/sqlite_db.test.ts +++ b/tests/unit/sqlite_db.test.ts @@ -2,7 +2,7 @@ import { describe, it, before, after } from 'node:test'; import assert from 'node:assert'; import { createDatabaseEngine } from '../../src/core/sqlite-db'; -import type { DatabaseOperations, CellUpdate } from '../../src/core/types'; +import type { DatabaseOperations, CellUpdate, ModificationEntry } from '../../src/core/types'; describe('WasmDatabaseEngine', () => { let engine: DatabaseOperations; @@ -118,4 +118,70 @@ describe('WasmDatabaseEngine', () => { const result = await engine.executeQuery("SELECT name FROM users WHERE id = 1"); assert.strictEqual(result[0].rows[0][0], 'Frank'); // Should remain original value }); + + it('should replay modification entries when applyModifications restores a dirty backup', async () => { + // A hot-exit backup stores edit history, not full database bytes. Restoring + // that backup must replay each pending entry into the freshly opened + // in-memory database so later saves contain the recovered edits. + await engine.executeQuery("DROP TABLE IF EXISTS restored_users"); + await engine.executeQuery("CREATE TABLE restored_users (id INTEGER PRIMARY KEY, name TEXT, age INTEGER)"); + + const modifications: ModificationEntry[] = [ + { + description: 'Insert restored row', + modificationType: 'row_insert', + targetTable: 'restored_users', + targetRowId: 1, + rowData: { id: 1, name: 'Draft', age: 30 } + }, + { + description: 'Update restored name', + modificationType: 'cell_update', + targetTable: 'restored_users', + targetRowId: 1, + targetColumn: 'name', + priorValue: 'Draft', + newValue: 'Recovered' + }, + { + description: 'Update restored age', + modificationType: 'cell_update', + targetTable: 'restored_users', + affectedCells: [ + { rowId: 1, columnName: 'age', priorValue: 30, newValue: 31 } + ] + } + ]; + + await engine.applyModifications(modifications); + + const result = await engine.executeQuery("SELECT id, name, age FROM restored_users ORDER BY id"); + assert.deepStrictEqual(result[0].rows, [[1, 'Recovered', 31]]); + }); + + it('should stop replaying modifications when the restore signal is already aborted', async () => { + // The restore caller passes an AbortSignal from VS Code cancellation. + // A pre-aborted signal must fail loudly before any entry is applied. + await engine.executeQuery("DROP TABLE IF EXISTS aborted_restore"); + await engine.executeQuery("CREATE TABLE aborted_restore (id INTEGER PRIMARY KEY, name TEXT)"); + + const controller = new AbortController(); + controller.abort(); + + await assert.rejects( + () => engine.applyModifications([ + { + description: 'Insert aborted row', + modificationType: 'row_insert', + targetTable: 'aborted_restore', + targetRowId: 1, + rowData: { id: 1, name: 'Should not exist' } + } + ], controller.signal), + /aborted/i + ); + + const result = await engine.executeQuery("SELECT COUNT(*) FROM aborted_restore"); + assert.strictEqual(result[0].rows[0][0], 0); + }); }); From 3172d6dd6706ef0d01d6a3a834e74d0c08bb3aaa Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 14:10:59 +0200 Subject: [PATCH 05/10] =?UTF-8?q?fix(web):=20faithful=20+=20atomic=20hot-e?= =?UTF-8?q?xit=20replay=20=E2=80=94=203=20P1s=20(#424=20review=20round=204?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three were lossy/incorrect forward-replay of serialized edit history during hot-exit restore (the round-3 replay path): 1. (P1) JSON-patch batch/single cell updates replayed as raw value SETS, not patches, because the history entry recorded no per-cell operation and the pre-processed value. ModificationEntry + affectedCells now carry an 'operation' ('set'|'json_patch'); HostBridge.updateCell/updateCellBatch record it (and the patch as newValue) from the processed updates; forwardApply replays json_patch cells via updateCell's patch arg / CellUpdate.operation, so restore reproduces the merged JSON instead of overwriting. 2. (P1) column_drop replay omitted dependent indexes → replay threw when an index still referenced the column. ModificationEntry now records droppedIndexes; HostBridge.deleteColumns stores them; forwardApply passes them to deleteColumns(table, cols, droppedIndexes). 3. (P2) Partial restore wasn't atomic: a malformed/aborted entry mid-loop left a half-applied DB. applyModifications now wraps the whole replay in a SAVEPOINT (RELEASE on success, ROLLBACK TO + RELEASE on any failure/abort) so restore is all-or-nothing, leaving the DB at the last-saved bytes on failure. Required converting deleteColumns' inner BEGIN TRANSACTION to a SAVEPOINT so it nests inside the restore transaction (SQLite can't nest BEGIN). insertRowBatch / undoColumnDrop BEGINs are not reachable from forwardApply, so left as-is. Backward-compatible: missing operation -> 'set', missing droppedIndexes -> none. redo (forwardApply non-strict) + desktop/native + worker paths unchanged. Tests +3 (json_patch batch round-trip; column_drop+index replay; atomic rollback on malformed entry), all revert-proof-verified. tsc clean; npm test 350/350. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/engine/wasm/WasmDatabaseEngine.ts | 80 +++++++++-- src/core/types.ts | 13 +- src/hostBridge.ts | 12 +- tests/unit/hot_exit_restore_replay.test.ts | 153 +++++++++++++++++++++ 4 files changed, 242 insertions(+), 16 deletions(-) create mode 100644 tests/unit/hot_exit_restore_replay.test.ts diff --git a/src/core/engine/wasm/WasmDatabaseEngine.ts b/src/core/engine/wasm/WasmDatabaseEngine.ts index ab9b32d..9fe728d 100644 --- a/src/core/engine/wasm/WasmDatabaseEngine.ts +++ b/src/core/engine/wasm/WasmDatabaseEngine.ts @@ -97,6 +97,39 @@ export class WasmDatabaseEngine implements DatabaseOperations { } } + /** + * Build a quoted SAVEPOINT name that is unique enough for nested engine work. + */ + private createSavepointName(prefix: string): string { + return escapeIdentifier(`${prefix}_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`); + } + + /** + * Roll back and release a SAVEPOINT without masking the original failure. + */ + private async safeRollbackSavepoint(savepointName: string, context: string): Promise { + try { + await this.executeQuery(`ROLLBACK TO ${savepointName}`); + await this.executeQuery(`RELEASE ${savepointName}`); + } catch (rollbackErr) { + console.warn(`Failed to rollback savepoint (${context}):`, rollbackErr); + } + } + + /** + * Normalize serialized cell replay operations for old and malformed history. + */ + private normalizeReplayCellOperation( + operation: unknown, + strict: boolean, + context: string + ): 'set' | 'json_patch' { + if (operation === undefined || operation === null) return 'set'; + if (operation === 'set' || operation === 'json_patch') return operation; + if (strict) throw new Error(`Cannot apply ${context}: unsupported cell operation ${String(operation)}`); + return 'set'; + } + /** * Execute a SQL query and return structured results. * @@ -194,9 +227,20 @@ export class WasmDatabaseEngine implements DatabaseOperations { signal?: AbortSignal ): Promise { signal?.throwIfAborted(); - for (const mod of mods) { - await this.forwardApply(mod, true); + if (mods.length === 0) return; + + const savepointName = this.createSavepointName('sp_apply_modifications'); + await this.executeQuery(`SAVEPOINT ${savepointName}`); + try { + for (const mod of mods) { + await this.forwardApply(mod, true); + signal?.throwIfAborted(); + } signal?.throwIfAborted(); + await this.executeQuery(`RELEASE ${savepointName}`); + } catch (err) { + await this.safeRollbackSavepoint(savepointName, 'applyModifications'); + throw err; } } @@ -347,7 +391,7 @@ export class WasmDatabaseEngine implements DatabaseOperations { * changed for entries that lack enough data to replay. */ private async forwardApply(mod: ModificationEntry, strict: boolean): Promise { - const { modificationType, targetTable, targetRowId, targetColumn, newValue, affectedCells, affectedRowIds, rowData, tableDef, columnDef, deletedColumns } = mod; + const { modificationType, targetTable, targetRowId, targetColumn, newValue, operation, affectedCells, affectedRowIds, rowData, tableDef, columnDef, deletedColumns, droppedIndexes } = mod; if (!targetTable) { if (strict) throw new Error(`Cannot apply ${modificationType}: missing target table`); return; @@ -360,11 +404,18 @@ export class WasmDatabaseEngine implements DatabaseOperations { const updates = affectedCells.map(cell => ({ rowId: cell.rowId, column: cell.columnName, - value: cell.newValue ?? null + value: cell.newValue ?? null, + operation: this.normalizeReplayCellOperation(cell.operation, strict, 'cell_update') })); await this.updateCellBatch(targetTable, updates); } else if (targetRowId !== undefined && targetColumn) { - await this.updateCell(targetTable, targetRowId, targetColumn, newValue ?? null); + const replayOperation = this.normalizeReplayCellOperation(operation, strict, 'cell_update'); + if (replayOperation === 'json_patch') { + const patch = typeof newValue === 'string' ? newValue : JSON.stringify(newValue ?? null); + await this.updateCell(targetTable, targetRowId, targetColumn, null, patch); + } else { + await this.updateCell(targetTable, targetRowId, targetColumn, newValue ?? null); + } } else if (strict) { throw new Error('Cannot apply cell_update: missing target cell or affected cells'); } @@ -402,7 +453,7 @@ export class WasmDatabaseEngine implements DatabaseOperations { case 'column_drop': if (deletedColumns) { const colNames = deletedColumns.map(c => c.name); - await this.deleteColumns(targetTable, colNames); + await this.deleteColumns(targetTable, colNames, droppedIndexes ?? undefined); } else if (strict) { throw new Error('Cannot apply column_drop: missing deleted column data'); } @@ -421,6 +472,12 @@ export class WasmDatabaseEngine implements DatabaseOperations { throw new Error('Cannot apply table_drop: forward replay is not supported'); } break; + + default: + if (strict) { + throw new Error(`Cannot apply unsupported modification type: ${String(modificationType)}`); + } + break; } } @@ -659,9 +716,10 @@ export class WasmDatabaseEngine implements DatabaseOperations { const escapedTable = escapeIdentifier(table); - // Now drop the columns within a single transaction for better performance - // This avoids N+1 query transaction overhead for multiple columns - await this.executeQuery('BEGIN TRANSACTION'); + // Use a SAVEPOINT so column drops remain atomic on their own and can also + // participate in the outer hot-exit restore transaction. + const savepointName = this.createSavepointName('sp_delete_columns'); + await this.executeQuery(`SAVEPOINT ${savepointName}`); try { // Drop specified dependent indexes first inside the transaction if (dropDependentIndexes && dropDependentIndexes.length > 0) { @@ -675,9 +733,9 @@ export class WasmDatabaseEngine implements DatabaseOperations { .map((col) => `ALTER TABLE ${escapedTable} DROP COLUMN ${escapeIdentifier(col)};`) .join('\n'); await this.executeQuery(dropColumnStatements); - await this.executeQuery('COMMIT'); + await this.executeQuery(`RELEASE ${savepointName}`); } catch (e) { - await this.safeRollback('deleteColumns'); + await this.safeRollbackSavepoint(savepointName, 'deleteColumns'); throw e; } } diff --git a/src/core/types.ts b/src/core/types.ts index 7f3dced..d56d58e 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -135,6 +135,11 @@ export type ModificationType = | 'column_drop' | 'table_drop'; +/** + * How a cell value should be applied when replaying a cell update. + */ +export type CellUpdateOperation = 'set' | 'json_patch'; + /** * Record of a single database modification for undo/redo. */ @@ -153,6 +158,8 @@ export interface ModificationEntry { priorValue?: CellValue; /** Value after modification */ newValue?: CellValue; + /** Cell update operation; missing values from older backups are treated as set. */ + operation?: CellUpdateOperation; /** Raw SQL executed */ executedQuery?: string; /** Multiple affected rows */ @@ -163,6 +170,8 @@ export interface ModificationEntry { columnName: string; priorValue?: CellValue; newValue?: CellValue; + /** Per-cell operation; missing values from older backups are treated as set. */ + operation?: CellUpdateOperation; }[]; /** Row data for insert/delete undo/redo */ rowData?: Record; @@ -178,6 +187,8 @@ export interface ModificationEntry { type: string; data: { rowId: RecordId; value: CellValue }[]; }[]; + /** Indexes dropped before a column_drop; missing values from older backups mean none. */ + droppedIndexes?: string[]; } /** @@ -280,7 +291,7 @@ export interface CellUpdate { column: string; value: CellValue; originalValue?: CellValue; - operation?: 'set' | 'json_patch'; + operation?: CellUpdateOperation; } /** diff --git a/src/hostBridge.ts b/src/hostBridge.ts index e02fba6..ef4dd62 100644 --- a/src/hostBridge.ts +++ b/src/hostBridge.ts @@ -121,6 +121,7 @@ export class HostBridge implements ToastService { } const patch = this.tryGeneratePatch(value, originalValue); + const operation = patch ? 'json_patch' as const : 'set' as const; // Use specific method instead of generic exec // This allows the backend to handle safe SQL construction @@ -139,7 +140,8 @@ export class HostBridge implements ToastService { targetTable: table, targetRowId: rowId, targetColumn: column, - newValue: value, + newValue: patch ?? value, + operation, priorValue: originalValue }); } @@ -342,7 +344,8 @@ export class HostBridge implements ToastService { description: `Delete columns ${columns.join(', ')} from ${table}`, modificationType: 'column_drop', targetTable: table, - deletedColumns: deletedColumnsData + deletedColumns: deletedColumnsData, + droppedIndexes: dependentIndexes.length > 0 ? dependentIndexes : undefined }); } @@ -429,11 +432,12 @@ export class HostBridge implements ToastService { description: `Update ${updates.length} cells in ${table}`, modificationType: 'cell_update', targetTable: table, - affectedCells: updates.map(u => ({ + affectedCells: processedUpdates.map(u => ({ rowId: u.rowId, columnName: u.column, newValue: u.value, - priorValue: u.originalValue + priorValue: u.originalValue, + operation: u.operation ?? 'set' })) }); } diff --git a/tests/unit/hot_exit_restore_replay.test.ts b/tests/unit/hot_exit_restore_replay.test.ts new file mode 100644 index 0000000..6f855ed --- /dev/null +++ b/tests/unit/hot_exit_restore_replay.test.ts @@ -0,0 +1,153 @@ +import './vscode_mock_setup'; +import { afterEach, describe, it, mock } from 'node:test'; +import assert from 'node:assert'; +import * as vscode from 'vscode'; +import { HostBridge } from '../../src/hostBridge'; +import { createDatabaseEngine } from '../../src/core/sqlite-db'; +import type { DatabaseOperations, LabeledModification, ModificationEntry } from '../../src/core/types'; + +function createRecordingBridge( + databaseOperations: DatabaseOperations, + recorded: LabeledModification[] +): HostBridge { + const document = { + uri: vscode.Uri.parse('file:///hot-exit-replay.db'), + documentKey: Promise.resolve('hot-exit-replay'), + databaseOperations, + isReadOnlyMode: false, + recordExternalModification: (modification: LabeledModification) => { + recorded.push(modification); + } + }; + const provider = { + webviews: new Map(), + context: { globalState: { update: () => Promise.resolve() } }, + isReadOnly: false + }; + + return new HostBridge(provider as any, document as any); +} + +async function createEngine(): Promise { + const result = await createDatabaseEngine({ + content: null, + maxSize: 0, + readOnlyMode: false + }); + assert.ok(result.operations, 'database operations should be initialized'); + + return result.operations; +} + +describe('hot-exit restore forward replay', () => { + afterEach(() => { + mock.reset(); + }); + + it('round-trips recorded json_patch batch cells as merge patches', async () => { + const originalJson = JSON.stringify({ + keep: true, + nested: { before: 1 }, + remove: 'old' + }); + const patchJson = JSON.stringify({ + nested: { after: 2 }, + remove: null, + added: 3 + }); + const expectedJson = { + keep: true, + nested: { before: 1, after: 2 }, + added: 3 + }; + + const writer = await createEngine(); + await writer.executeQuery('CREATE TABLE patch_docs (id INTEGER PRIMARY KEY, content TEXT)'); + await writer.insertRow('patch_docs', { id: 1, content: originalJson }); + + const recorded: LabeledModification[] = []; + const bridge = createRecordingBridge(writer, recorded); + + await bridge.updateCellBatch( + 'patch_docs', + [{ rowId: 1, column: 'content', value: patchJson, operation: 'json_patch' }], + 'Patch JSON' + ); + + assert.strictEqual(recorded.length, 1); + + const replay = await createEngine(); + await replay.executeQuery('CREATE TABLE patch_docs (id INTEGER PRIMARY KEY, content TEXT)'); + await replay.insertRow('patch_docs', { id: 1, content: originalJson }); + + await replay.applyModifications([recorded[0]]); + + const result = await replay.executeQuery('SELECT content FROM patch_docs WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(result[0].rows[0][0] as string), expectedJson); + assert.strictEqual(recorded[0].affectedCells?.[0].operation, 'json_patch'); + }); + + it('replays column_drop history with dependent indexes dropped first', async () => { + const writer = await createEngine(); + await writer.executeQuery('CREATE TABLE indexed_docs (id INTEGER PRIMARY KEY, payload TEXT, keep TEXT)'); + await writer.executeQuery('CREATE INDEX idx_indexed_docs_payload ON indexed_docs(payload)'); + await writer.insertRow('indexed_docs', { id: 1, payload: 'drop me', keep: 'keep me' }); + + mock.method( + vscode.window, + 'showWarningMessage', + async (_message: string, _options: unknown, continueButton: unknown) => continueButton + ); + + const recorded: LabeledModification[] = []; + const bridge = createRecordingBridge(writer, recorded); + + await bridge.deleteColumns('indexed_docs', ['payload']); + + assert.strictEqual(recorded.length, 1); + + const replay = await createEngine(); + await replay.executeQuery('CREATE TABLE indexed_docs (id INTEGER PRIMARY KEY, payload TEXT, keep TEXT)'); + await replay.executeQuery('CREATE INDEX idx_indexed_docs_payload ON indexed_docs(payload)'); + await replay.insertRow('indexed_docs', { id: 1, payload: 'drop me', keep: 'keep me' }); + + await replay.applyModifications([recorded[0]]); + + const columns = await replay.getTableInfo('indexed_docs'); + assert.deepStrictEqual(columns.map(column => column.identifier), ['id', 'keep']); + + const indexes = await replay.executeQuery( + "SELECT name FROM sqlite_master WHERE type = 'index' AND tbl_name = 'indexed_docs'" + ); + assert.deepStrictEqual(indexes[0]?.rows ?? [], []); + assert.deepStrictEqual(recorded[0].droppedIndexes, ['idx_indexed_docs_payload']); + }); + + it('rolls back all replayed entries when strict restore hits a malformed entry', async () => { + const engine = await createEngine(); + await engine.executeQuery('CREATE TABLE atomic_restore (id INTEGER PRIMARY KEY, name TEXT)'); + + const modifications: ModificationEntry[] = [ + { + description: 'Insert draft row', + modificationType: 'row_insert', + targetTable: 'atomic_restore', + targetRowId: 1, + rowData: { name: 'Draft' } + }, + { + description: 'Malformed cell update', + modificationType: 'cell_update', + targetTable: 'atomic_restore' + } + ]; + + await assert.rejects( + () => engine.applyModifications(modifications), + /Cannot apply cell_update: missing target cell or affected cells/ + ); + + const result = await engine.executeQuery('SELECT COUNT(*) FROM atomic_restore'); + assert.strictEqual(result[0].rows[0][0], 0); + }); +}); From 6b3bc064823998eac3b9dd8ed7bfd8aebfd4063d Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 15:10:23 +0200 Subject: [PATCH 06/10] fix(undo): unify forward-apply replay across web + native redo (#424 review round 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ModificationEntry was forward-applied by three divergent implementations (live edit via HostBridge, web redo via WasmDatabaseEngine.forwardApply, native redo via nativeWorker.redoModification) that interpreted the same fields differently — the root cause of recurring replay data bugs across review rounds. Native redoModification reimplemented raw SQL and ignored both `operation` and `droppedIndexes`. Combined with the earlier branch change that records `newValue: patch` + `operation` for json_patch cells, this corrupted desktop redo: a json_patch cell redo wrote the raw patch string as a literal value. Unify by delegating native redo to the same operationsFacade primitives the live-edit path uses, so ModificationEntry has ONE interpretation everywhere: - cell_update batch -> updateCellBatch (honors per-cell operation) - cell_update single -> updateCell (json_patch -> patch arg; else value) - row_insert/delete -> insertRow / deleteRows - column_add -> addColumn (same DEFAULT-value handling) - column_drop -> deleteColumns(table, cols, droppedIndexes) drops dependent indexes first - table_create -> createTable (same primaryKey/notNull handling) Native redo now structurally mirrors WasmDatabaseEngine.forwardApply (non-strict); paired-contract comments in both point at each other so they cannot drift again. Tests: 3 regression tests driving the real native length-prefixed V8 IPC, asserting json_patch single (COALESCE patch SQL, not a literal SET), batch (operation-aware execBatch grouping), and column_drop (DROP INDEX before ALTER DROP COLUMN). Revert-proof: all 3 fail against the old raw-SQL redo. Full suite 353/353 (+3); build + tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/engine/wasm/WasmDatabaseEngine.ts | 3 + src/nativeWorker.ts | 81 +++------ tests/unit/nativeWorker.test.ts | 191 +++++++++++++++++++++ 3 files changed, 221 insertions(+), 54 deletions(-) diff --git a/src/core/engine/wasm/WasmDatabaseEngine.ts b/src/core/engine/wasm/WasmDatabaseEngine.ts index 9fe728d..9ab3203 100644 --- a/src/core/engine/wasm/WasmDatabaseEngine.ts +++ b/src/core/engine/wasm/WasmDatabaseEngine.ts @@ -389,6 +389,9 @@ export class WasmDatabaseEngine implements DatabaseOperations { * entries fail loudly instead of silently dropping recovered edits. Redo uses * the historical non-strict behavior so existing undo/redo semantics are not * changed for entries that lack enough data to replay. + * + * Keep the non-strict redo shape paired with nativeWorker.redoModification so + * ModificationEntry fields keep one interpretation across web and desktop. */ private async forwardApply(mod: ModificationEntry, strict: boolean): Promise { const { modificationType, targetTable, targetRowId, targetColumn, newValue, operation, affectedCells, affectedRowIds, rowData, tableDef, columnDef, deletedColumns, droppedIndexes } = mod; diff --git a/src/nativeWorker.ts b/src/nativeWorker.ts index 236c858..f3fa242 100644 --- a/src/nativeWorker.ts +++ b/src/nativeWorker.ts @@ -613,91 +613,64 @@ export async function createNativeDatabaseConnection( * Redo a modification by re-executing the original change. */ redoModification: async (mod: ModificationEntry) => { - const { modificationType, targetTable, targetRowId, targetColumn, newValue, affectedCells, affectedRowIds, rowData, tableDef, columnDef, deletedColumns } = mod; + const { modificationType, targetTable, targetRowId, targetColumn, newValue, operation, affectedCells, affectedRowIds, rowData, tableDef, columnDef, deletedColumns, droppedIndexes } = mod; if (!targetTable) return; + // Keep this non-strict forward replay switch paired with + // WasmDatabaseEngine.forwardApply(..., false) so ModificationEntry + // fields keep one interpretation across desktop and web redo. + const normalizeReplayCellOperation = (replayOperation: unknown): 'set' | 'json_patch' => { + return replayOperation === 'json_patch' ? 'json_patch' : 'set'; + }; + switch (modificationType) { case 'cell_update': if (affectedCells) { - const updates = affectedCells.map(c => ({ - rowId: c.rowId, - column: c.columnName, - value: c.newValue - } as CellUpdate)); - await worker.call('execBatch', [ - updates.map(u => ({ - sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(u.column)} = ? WHERE rowid = ?`, - params: [u.value, Number(u.rowId)] - })) - ]); + await operationsFacade.updateCellBatch(targetTable, affectedCells.map(c => ({ + rowId: c.rowId, + column: c.columnName, + value: c.newValue ?? null, + operation: normalizeReplayCellOperation(c.operation) + }))); } else if (targetRowId !== undefined && targetColumn) { - const rowIdNum = Number(targetRowId); - const sql = `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(targetColumn)} = ? WHERE rowid = ?`; - await worker.call('run', [sql, [newValue, rowIdNum]]); + const replayOperation = normalizeReplayCellOperation(operation); + if (replayOperation === 'json_patch') { + const patch = typeof newValue === 'string' ? newValue : JSON.stringify(newValue ?? null); + await operationsFacade.updateCell(targetTable, targetRowId, targetColumn, null, patch); + } else { + await operationsFacade.updateCell(targetTable, targetRowId, targetColumn, newValue ?? null); + } } break; case 'row_insert': if (rowData) { const dataToInsert = targetRowId !== undefined ? { ...rowData, rowid: targetRowId } : rowData; - const columns = Object.keys(dataToInsert); - const colNames = columns.map(escapeIdentifier).join(', '); - const placeholders = columns.map(() => '?').join(', '); - const params = columns.map(c => dataToInsert[c]); - await worker.call('run', [`INSERT INTO ${escapeIdentifier(targetTable)} (${colNames}) VALUES (${placeholders})`, params]); + await operationsFacade.insertRow(targetTable, dataToInsert); } break; case 'row_delete': - if (affectedRowIds && affectedRowIds.length > 0) { - const ids = affectedRowIds.map(id => Number(id)); - const placeholders = ids.map(() => '?').join(', '); - await worker.call('run', [`DELETE FROM ${escapeIdentifier(targetTable)} WHERE rowid IN (${placeholders})`, ids]); + if (affectedRowIds) { + await operationsFacade.deleteRows(targetTable, affectedRowIds); } break; case 'column_add': if (targetColumn && columnDef) { - validateSqlType(columnDef.type); - let sql = `ALTER TABLE ${escapeIdentifier(targetTable)} ADD COLUMN ${escapeIdentifier(targetColumn)} ${columnDef.type}`; - if (columnDef.defaultValue !== undefined && columnDef.defaultValue !== null && columnDef.defaultValue !== '') { - // Strict numeric validation for default values - if (columnDef.defaultValue.toLowerCase() === 'null') { - sql += ' DEFAULT NULL'; - } else if (/^-?(\d+\.?\d*|\.\d+)([eE][+-]?\d+)?$/.test(columnDef.defaultValue)) { - // Strict numeric pattern: optional sign, digits with optional decimal, optional exponent - sql += ` DEFAULT ${columnDef.defaultValue}`; - } else { - sql += ` DEFAULT '${columnDef.defaultValue.replace(/'/g, "''")}'`; - } - } - await worker.call('run', [sql]); + await operationsFacade.addColumn(targetTable, targetColumn, columnDef.type, columnDef.defaultValue); } break; case 'column_drop': if (deletedColumns) { - const batch = []; - for (const col of deletedColumns) { - batch.push({ sql: `ALTER TABLE ${escapeIdentifier(targetTable)} DROP COLUMN ${escapeIdentifier(col.name)}` }); - } - if (batch.length > 0) { - await worker.call('execBatch', [batch]); - } + await operationsFacade.deleteColumns(targetTable, deletedColumns.map(c => c.name), droppedIndexes ?? undefined); } break; case 'table_create': if (tableDef && tableDef.columns) { - // Re-use createTable logic - const colDefs = tableDef.columns.map(col => { - validateSqlType(col.type); - let def = `${escapeIdentifier(col.name)} ${col.type}`; - if (col.primaryKey) def += ' PRIMARY KEY'; - if (col.notNull && !col.primaryKey) def += ' NOT NULL'; - return def; - }); - await worker.call('run', [`CREATE TABLE ${escapeIdentifier(targetTable)} (${colDefs.join(', ')})`]); + await operationsFacade.createTable(targetTable, tableDef.columns); } break; } diff --git a/tests/unit/nativeWorker.test.ts b/tests/unit/nativeWorker.test.ts index 6cd0839..57a840b 100644 --- a/tests/unit/nativeWorker.test.ts +++ b/tests/unit/nativeWorker.test.ts @@ -3,8 +3,74 @@ import { describe, it, mock, afterEach, beforeEach } from 'node:test'; import assert from 'node:assert'; import * as fs from 'node:fs'; import * as path from 'node:path'; +import * as v8 from 'node:v8'; +import { EventEmitter } from 'node:events'; import * as vscode from 'vscode'; import { isNativeAvailable, NativeWorkerProcess } from '../../src/nativeWorker'; +import type { DatabaseOperations } from '../../src/core/types'; + +interface RecordedNativeCall { + id: number; + method: string; + args: unknown[]; +} + +function encodeNativeMessage(message: unknown): Buffer { + // Native worker messages are length-prefixed V8 payloads, matching NativeWorkerProcess.writeMessage. + const payload = v8.serialize(message); + const header = Buffer.alloc(4); + header.writeUInt32BE(payload.byteLength, 0); + return Buffer.concat([header, payload]); +} + +function createRecordingNativeProcess(recordedCalls: RecordedNativeCall[]) { + const mockProcess = new EventEmitter() as any; + mockProcess.stdout = new EventEmitter(); + mockProcess.stderr = new EventEmitter(); + mockProcess.kill = mock.fn(); + + let inputBuffer = Buffer.alloc(0); + + const emitMessage = (message: unknown) => { + mockProcess.stdout.emit('data', encodeNativeMessage(message)); + }; + + const readInboundMessages = () => { + while (inputBuffer.length >= 4) { + const payloadLength = inputBuffer.readUInt32BE(0); + const frameLength = 4 + payloadLength; + if (inputBuffer.length < frameLength) { + return; + } + + const payload = inputBuffer.subarray(4, frameLength); + inputBuffer = inputBuffer.subarray(frameLength); + + const call = v8.deserialize(payload) as RecordedNativeCall; + recordedCalls.push(call); + + // Every operation under test only needs a successful native response. + queueMicrotask(() => { + emitMessage({ id: call.id, result: { changes: 1, lastInsertRowId: 1 } }); + }); + } + }; + + mockProcess.stdin = { + write: mock.fn((chunk: Buffer) => { + // NativeWorkerProcess writes the header and payload separately, so buffer until a full frame arrives. + inputBuffer = Buffer.concat([inputBuffer, Buffer.from(chunk)]); + readInboundMessages(); + return true; + }) + }; + + queueMicrotask(() => { + emitMessage({ ready: true }); + }); + + return mockProcess; +} describe('isNativeAvailable', () => { let originalPlatform: string; @@ -131,6 +197,25 @@ describe('createNativeDatabaseConnection', () => { mock.restoreAll(); }); + async function createRecordingConnection(): Promise<{ + databaseOps: DatabaseOperations; + calls: RecordedNativeCall[]; + dispose: () => void; + }> { + const calls: RecordedNativeCall[] = []; + mock.method(child_process, 'spawn', () => createRecordingNativeProcess(calls)); + + const { createNativeDatabaseConnection } = require('../../src/nativeWorker'); + const bundle = await createNativeDatabaseConnection({ fsPath: tempDir } as any); + const connection = await bundle.establishConnection({ fsPath: '/db/path.sqlite' } as any, 'TestDB'); + + return { + databaseOps: connection.databaseOps, + calls, + dispose: () => bundle.workerMethods[Symbol.dispose]() + }; + } + it('should throw an error with context when database opening fails', async () => { let mockProcess: any; const EventEmitter = require('node:events').EventEmitter; @@ -198,6 +283,112 @@ describe('createNativeDatabaseConnection', () => { bundle.workerMethods[Symbol.dispose](); }); + + it('replays single json_patch cell redo through the patch-aware updateCell primitive', async () => { + const connection = await createRecordingConnection(); + + try { + const patch = JSON.stringify({ meta: { reviewed: true } }); + connection.calls.length = 0; + + await connection.databaseOps.redoModification({ + modificationType: 'cell_update', + description: 'Patch payload', + targetTable: 'docs', + targetRowId: 7, + targetColumn: 'payload', + newValue: patch, + operation: 'json_patch' + }); + + assert.strictEqual(connection.calls.length, 1); + const call = connection.calls[0]; + assert.strictEqual(call.method, 'run'); + + const [sql, params] = call.args as [string, unknown[]]; + assert.strictEqual( + sql, + `UPDATE "docs" SET "payload" = json_patch(COALESCE("payload", '{}'), ?) WHERE rowid = ?` + ); + assert.notStrictEqual(sql, `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`); + assert.deepStrictEqual(params, [patch, 7]); + } finally { + connection.dispose(); + } + }); + + it('replays batch json_patch cell redo through operation-aware updateCellBatch', async () => { + const connection = await createRecordingConnection(); + + try { + const firstPatch = JSON.stringify({ status: 'reviewed' }); + const secondPatch = JSON.stringify({ status: 'approved' }); + connection.calls.length = 0; + + await connection.databaseOps.redoModification({ + modificationType: 'cell_update', + description: 'Batch patch payloads', + targetTable: 'docs', + affectedCells: [ + { rowId: 3, columnName: 'payload', newValue: firstPatch, operation: 'json_patch' }, + { rowId: 4, columnName: 'payload', newValue: secondPatch, operation: 'json_patch' }, + { rowId: 5, columnName: 'title', newValue: 'Plain title' } + ] + }); + + assert.strictEqual(connection.calls.length, 1); + const call = connection.calls[0]; + assert.strictEqual(call.method, 'execBatch'); + + const batch = call.args[0] as { + sql: string; + paramsList?: unknown[][]; + params?: unknown[]; + }[]; + + assert.strictEqual(batch.length, 2); + assert.strictEqual( + batch[0].sql, + `UPDATE "docs" SET "payload" = json_patch("payload", ?) WHERE rowid = ?` + ); + assert.deepStrictEqual(batch[0].paramsList, [[firstPatch, 3], [secondPatch, 4]]); + assert.strictEqual(batch[1].sql, `UPDATE "docs" SET "title" = ? WHERE rowid = ?`); + assert.deepStrictEqual(batch[1].paramsList, [['Plain title', 5]]); + } finally { + connection.dispose(); + } + }); + + it('replays column_drop redo by dropping recorded dependent indexes first', async () => { + const connection = await createRecordingConnection(); + + try { + connection.calls.length = 0; + + await connection.databaseOps.redoModification({ + modificationType: 'column_drop', + description: 'Drop indexed payload', + targetTable: 'docs', + deletedColumns: [{ name: 'payload', type: 'TEXT', data: [] }], + droppedIndexes: ['idx_docs_payload'] + }); + + assert.strictEqual(connection.calls.length, 1); + const call = connection.calls[0]; + assert.strictEqual(call.method, 'execBatch'); + + const batch = call.args[0] as { sql: string }[]; + assert.deepStrictEqual( + batch.map(item => item.sql), + [ + `DROP INDEX IF EXISTS "idx_docs_payload"`, + `ALTER TABLE "docs" DROP COLUMN "payload"` + ] + ); + } finally { + connection.dispose(); + } + }); }); describe('NativeWorkerProcess', () => { From 9e1bf376253c205c71af411b081821d85236b7d9 Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 15:29:26 +0200 Subject: [PATCH 07/10] =?UTF-8?q?fix(undo):=20address=20review=20round=205?= =?UTF-8?q?=20=E2=80=94=20batch=20json=5Fpatch=20NULL=20+=20sync=20checkpo?= =?UTF-8?q?int=20(#424)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both bots flagged the same data bug independently (CodeRabbit major, Gemini): - nativeWorker.updateCellBatch json_patch used `json_patch(col, ?)` without COALESCE — the SOLE json_patch site missing it (native single-cell, WASM single, and WASM batch all use `json_patch(COALESCE(col,'{}'), ?)`). A batch json_patch on a NULL cell returned NULL, silently discarding the patch. This affects the LIVE desktop batch path, not just redo. Now matches the other three sites. Batch redo test updated to assert the COALESCE form (+ a notStrictEqual guard); revert-proof verified. Gemini (medium): - ModificationTracker.createCheckpointAt was `async`/`Promise` but does only synchronous work (arithmetic + array bounds). Made synchronous and dropped the two unnecessary `await`s in databaseModel (the only callers). CodeRabbit (refactor): - tests/unit/sqlite_db.test.ts now imports `./vscode_mock_setup` first per the documented unit-test convention (defensive: it passes standalone today since sqlite-db has no vscode dependency, but this removes suite-order fragility). tsc clean; build OK; full suite 353/353. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/undo-history.ts | 2 +- src/databaseModel.ts | 4 ++-- src/nativeWorker.ts | 6 ++++-- tests/unit/nativeWorker.test.ts | 6 ++++++ tests/unit/sqlite_db.test.ts | 2 ++ 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/core/undo-history.ts b/src/core/undo-history.ts index 6c86022..34e0f1a 100644 --- a/src/core/undo-history.ts +++ b/src/core/undo-history.ts @@ -329,7 +329,7 @@ export class ModificationTracker { + createCheckpointAt(position: number): void { const relativePosition = position - this.timelineOffset; this.checkpointIndex = Math.max(0, Math.min(this.timeline.length, relativePosition)); } diff --git a/src/databaseModel.ts b/src/databaseModel.ts index 1a37354..f3c30ce 100644 --- a/src/databaseModel.ts +++ b/src/databaseModel.ts @@ -358,7 +358,7 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { this.#modificationTracker.getCheckpointInvalidationRevision() === fileCheckpointInvalidationRevision ) { - await this.#modificationTracker.createCheckpointAt(fileCheckpoint); + this.#modificationTracker.createCheckpointAt(fileCheckpoint); } return; } catch (e) { @@ -395,7 +395,7 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { this.#modificationTracker.getCheckpointInvalidationRevision() === serializedCheckpointInvalidationRevision ) { - await this.#modificationTracker.createCheckpointAt(serializedCheckpoint); + this.#modificationTracker.createCheckpointAt(serializedCheckpoint); } } diff --git a/src/nativeWorker.ts b/src/nativeWorker.ts index f3fa242..cedaa20 100644 --- a/src/nativeWorker.ts +++ b/src/nativeWorker.ts @@ -1110,8 +1110,10 @@ export async function createNativeDatabaseConnection( let sql: string; if (op === 'json_patch') { - // json_patch(col, patch) - sql = `UPDATE ${escapedTable} SET ${escapedColumn} = json_patch(${escapedColumn}, ?) WHERE rowid = ?`; + // COALESCE handles NULL columns: json_patch(NULL, x) returns NULL per SQL + // semantics, but a patch on a NULL JSON cell must be applied to '{}' (matching + // the single-cell updateCell path and both WasmDatabaseEngine json_patch sites). + sql = `UPDATE ${escapedTable} SET ${escapedColumn} = json_patch(COALESCE(${escapedColumn}, '{}'), ?) WHERE rowid = ?`; } else { // Standard set sql = `UPDATE ${escapedTable} SET ${escapedColumn} = ? WHERE rowid = ?`; diff --git a/tests/unit/nativeWorker.test.ts b/tests/unit/nativeWorker.test.ts index 57a840b..13d3c03 100644 --- a/tests/unit/nativeWorker.test.ts +++ b/tests/unit/nativeWorker.test.ts @@ -347,7 +347,13 @@ describe('createNativeDatabaseConnection', () => { }[]; assert.strictEqual(batch.length, 2); + // COALESCE so a batch patch on a NULL JSON cell applies to '{}' instead of + // returning NULL — must match single-cell updateCell and both WASM json_patch sites. assert.strictEqual( + batch[0].sql, + `UPDATE "docs" SET "payload" = json_patch(COALESCE("payload", '{}'), ?) WHERE rowid = ?` + ); + assert.notStrictEqual( batch[0].sql, `UPDATE "docs" SET "payload" = json_patch("payload", ?) WHERE rowid = ?` ); diff --git a/tests/unit/sqlite_db.test.ts b/tests/unit/sqlite_db.test.ts index 50d889e..f8f7aee 100644 --- a/tests/unit/sqlite_db.test.ts +++ b/tests/unit/sqlite_db.test.ts @@ -1,4 +1,6 @@ +import './vscode_mock_setup'; + import { describe, it, before, after } from 'node:test'; import assert from 'node:assert'; import { createDatabaseEngine } from '../../src/core/sqlite-db'; From 0a15b1616fcb3e46bb3a5d8f091bbe0c5b7dadd5 Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 16:07:13 +0200 Subject: [PATCH 08/10] fix(web): forward json_patch arg, enforce read-only VFS, replay cancellation (#424 review round 6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three still-valid findings from the round-6 bot review (CodeRabbit + Codex connector); other flagged items were verified already-fixed in earlier rounds. P2 — Forward the json_patch arg through the in-process / browser facades. WasmDatabaseEngine.updateCell accepts (…, value, patch?) and merges when patch is set, but the patch was silently dropped at createWorkerEndpoint.updateCell (sqlite-db.ts) and the browser in-process facade (workerFactory.ts). The worker-backed desktop facade dropped it too. All now forward patch (interface WorkerMethods.updateCell widened). Without this, a single-cell JSON edit in vscode.dev (and desktop wasm mode) overwrites the whole — possibly stale — document instead of applying the merge patch. P2 — Enforce read-only in the cell-editor (VFS) write path. SQLiteFileSystemProvider.writeFile called updateCell with no read-only check, so a WAL-backed (read-only) browser database could be mutated via the cell editor, recording dirty edits save()/revert() later reject. Now throws FileSystemError.NoPermissions when document.isReadOnlyMode, before decoding content or mutating SQLite. Minor — Check cancellation before each replay step. applyModifications now calls signal?.throwIfAborted() at the top of the replay loop as well, so an abort between entries is observed before the next forwardApply begins. Tests (+3, all revert-proof): endpoint forwards patch → engine merges (not stale overwrite); browser facade forwards patch; read-only writeFile rejects without calling updateCell; cancellation observed at loop boundary before the next entry starts. Also converts databaseModel.test.ts workspace.fs overrides to Object.defineProperty per convention. Deferred (own PR): hot-exit does not persist the redo/futureStack — pre-existing on main, affects desktop too. tsc clean; build OK; full suite 356/356. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/engine/wasm/WasmDatabaseEngine.ts | 3 + src/core/sqlite-db.ts | 6 +- src/virtualFileSystem.ts | 6 + src/workerFactory.ts | 14 +- tests/unit/databaseModel.test.ts | 202 +++++++++++++-------- tests/unit/sqlite_db.test.ts | 57 ++++++ tests/unit/virtualFileSystem.test.ts | 22 +++ tests/unit/workerFactory_browser.test.ts | 11 +- tests/unit/worker_endpoint.test.ts | 36 ++++ 9 files changed, 274 insertions(+), 83 deletions(-) diff --git a/src/core/engine/wasm/WasmDatabaseEngine.ts b/src/core/engine/wasm/WasmDatabaseEngine.ts index 9ab3203..a485a8e 100644 --- a/src/core/engine/wasm/WasmDatabaseEngine.ts +++ b/src/core/engine/wasm/WasmDatabaseEngine.ts @@ -233,6 +233,9 @@ export class WasmDatabaseEngine implements DatabaseOperations { await this.executeQuery(`SAVEPOINT ${savepointName}`); try { for (const mod of mods) { + // Check cancellation at the replay boundary so an aborted restore stops + // before the next modification starts mutating the in-memory database. + signal?.throwIfAborted(); await this.forwardApply(mod, true); signal?.throwIfAborted(); } diff --git a/src/core/sqlite-db.ts b/src/core/sqlite-db.ts index 9b06a47..ccbd067 100644 --- a/src/core/sqlite-db.ts +++ b/src/core/sqlite-db.ts @@ -197,8 +197,10 @@ export function createWorkerEndpoint() { return requireEngine().discardModifications(mods, signal); }, - async updateCell(table: string, rowId: RecordId, column: string, value: CellValue): Promise { - return requireEngine().updateCell(table, rowId, column, value); + async updateCell(table: string, rowId: RecordId, column: string, value: CellValue, patch?: string): Promise { + // Forward the optional JSON merge patch so browser/in-process cell edits + // can update the current document instead of replacing it with stale data. + return requireEngine().updateCell(table, rowId, column, value, patch); }, async insertRow(table: string, data: Record): Promise { diff --git a/src/virtualFileSystem.ts b/src/virtualFileSystem.ts index 46e0460..0ab1280 100644 --- a/src/virtualFileSystem.ts +++ b/src/virtualFileSystem.ts @@ -107,6 +107,12 @@ export class SQLiteFileSystemProvider implements vsc.FileSystemProvider { throw vsc.FileSystemError.NoPermissions('Cannot edit CREATE statement directly'); } + if (document.isReadOnlyMode) { + // Read-only database documents may expose cell files for viewing, but + // writes must stop before decoding content or recording dirty edits. + throw vsc.FileSystemError.NoPermissions('Database is read-only'); + } + try { const rowIdNum = Number(rowId); if (isNaN(rowIdNum)) { diff --git a/src/workerFactory.ts b/src/workerFactory.ts index 26a79f9..73204ed 100644 --- a/src/workerFactory.ts +++ b/src/workerFactory.ts @@ -66,7 +66,7 @@ interface WorkerMethods { ): Promise; runQuery(sql: string, params?: CellValue[]): Promise; exportDatabase(name: string): Promise; - updateCell(table: string, rowId: string | number, column: string, value: CellValue): Promise; + updateCell(table: string, rowId: string | number, column: string, value: CellValue, patch?: string): Promise; insertRow(table: string, data: Record): Promise; insertRowBatch(table: string, rows: Record[]): Promise; deleteRows(table: string, rowIds: (string | number)[]): Promise; @@ -252,8 +252,10 @@ async function createInProcessWasmDatabaseConnection( endpoint.flushChanges(signal), discardModifications: (mods: ModificationEntry[], signal?: AbortSignal) => endpoint.discardModifications(mods, signal), - updateCell: (table: string, rowId: string | number, column: string, value: CellValue) => - endpoint.updateCell(table, rowId, column, value), + // Preserve JSON merge patches when the browser facade calls the + // in-process endpoint directly. + updateCell: (table: string, rowId: string | number, column: string, value: CellValue, patch?: string) => + endpoint.updateCell(table, rowId, column, value, patch), insertRow: (table: string, data: Record) => endpoint.insertRow(table, data), insertRowBatch: (table: string, rows: Record[]) => @@ -444,8 +446,10 @@ async function createWorkerBackedWasmDatabaseConnection( redoModification: async () => {}, flushChanges: async () => {}, discardModifications: async () => {}, - updateCell: (table: string, rowId: string | number, column: string, value: CellValue) => - workerProxy.updateCell(table, rowId, column, wrapForTransfer(value)), + // Preserve JSON merge patches through worker RPC while still + // transferring Uint8Array cell values without copying. + updateCell: (table: string, rowId: string | number, column: string, value: CellValue, patch?: string) => + workerProxy.updateCell(table, rowId, column, wrapForTransfer(value), patch), insertRow: (table: string, data: Record) => { // Wrap any Uint8Array values in the data object for zero-copy transfer const wrappedData: Record = {}; diff --git a/tests/unit/databaseModel.test.ts b/tests/unit/databaseModel.test.ts index e7ba60b..8c9f57a 100644 --- a/tests/unit/databaseModel.test.ts +++ b/tests/unit/databaseModel.test.ts @@ -320,18 +320,22 @@ describe('DatabaseDocument save/saveAs fallback', () => { let writeFileCalled = false; const originalFs = mockVscode.workspace.fs; - mockVscode.workspace.fs = { - ...originalFs, - stat: async () => { - statCalled = true; - return { size: 100 }; // Less than getFileSizeLimit() - }, - writeFile: async (uri: any, content: any) => { - writeFileCalled = true; - assert.deepStrictEqual(content, new Uint8Array([1, 2, 3])); - }, - readFile: async () => new Uint8Array([]) - } as any; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + ...originalFs, + stat: async () => { + statCalled = true; + return { size: 100 }; // Less than getFileSizeLimit() + }, + writeFile: async (uri: any, content: any) => { + writeFileCalled = true; + assert.deepStrictEqual(content, new Uint8Array([1, 2, 3])); + }, + readFile: async () => new Uint8Array([]) + } as any, + writable: true, + configurable: true + }); const originalConsoleWarn = console.warn; let consoleWarnCalled = false; @@ -351,7 +355,11 @@ describe('DatabaseDocument save/saveAs fallback', () => { assert.strictEqual(serialized, true, 'serializeDatabase should be called'); assert.strictEqual(writeFileCalled, true, 'fs.writeFile should be called'); } finally { - mockVscode.workspace.fs = originalFs; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: originalFs, + writable: true, + configurable: true + }); console.warn = originalConsoleWarn; } }); @@ -372,17 +380,21 @@ describe('DatabaseDocument save/saveAs fallback', () => { let writeFileCalled = false; const originalFs = mockVscode.workspace.fs; - mockVscode.workspace.fs = { - ...originalFs, - stat: async () => { - return { size: 100 }; - }, - writeFile: async (uri: any, content: any) => { - writeFileCalled = true; - assert.deepStrictEqual(content, new Uint8Array([1, 2, 3])); - }, - readFile: async () => new Uint8Array([]) - } as any; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + ...originalFs, + stat: async () => { + return { size: 100 }; + }, + writeFile: async (uri: any, content: any) => { + writeFileCalled = true; + assert.deepStrictEqual(content, new Uint8Array([1, 2, 3])); + }, + readFile: async () => new Uint8Array([]) + } as any, + writable: true, + configurable: true + }); const originalConsoleWarn = console.warn; let consoleWarnCalled = false; @@ -401,7 +413,11 @@ describe('DatabaseDocument save/saveAs fallback', () => { assert.strictEqual(serialized, true, 'serializeDatabase should be called'); assert.strictEqual(writeFileCalled, true, 'fs.writeFile should be called'); } finally { - mockVscode.workspace.fs = originalFs; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: originalFs, + writable: true, + configurable: true + }); console.warn = originalConsoleWarn; } }); @@ -423,15 +439,19 @@ describe('DatabaseDocument save/saveAs fallback', () => { const doc = createDocBypassingFactory(dbOps, sourceUri); const originalFs = mockVscode.workspace.fs; - mockVscode.workspace.fs = { - ...originalFs, - writeFile: async (uri: any, content: any) => { - writeFileCalled = true; - assert.strictEqual(uri, sourceUri); - assert.deepStrictEqual(content, new Uint8Array([4, 5, 6])); - }, - readFile: async () => new Uint8Array([]) - } as any; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + ...originalFs, + writeFile: async (uri: any, content: any) => { + writeFileCalled = true; + assert.strictEqual(uri, sourceUri); + assert.deepStrictEqual(content, new Uint8Array([4, 5, 6])); + }, + readFile: async () => new Uint8Array([]) + } as any, + writable: true, + configurable: true + }); try { await doc.save(); @@ -439,7 +459,11 @@ describe('DatabaseDocument save/saveAs fallback', () => { assert.strictEqual(serializedName, 'test.db'); assert.strictEqual(writeFileCalled, true, 'fs.writeFile should be called'); } finally { - mockVscode.workspace.fs = originalFs; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: originalFs, + writable: true, + configurable: true + }); } }); @@ -465,16 +489,20 @@ describe('DatabaseDocument save/saveAs fallback', () => { }); const originalFs = mockVscode.workspace.fs; - mockVscode.workspace.fs = { - ...originalFs, - writeFile: async (uri: any, content: any) => { - if (uri.toString() === sourceUri.toString()) { - throw new Error('NoPermissions: read-only filesystem'); - } - backupContent = content; - }, - readFile: async () => new Uint8Array([]) - } as any; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + ...originalFs, + writeFile: async (uri: any, content: any) => { + if (uri.toString() === sourceUri.toString()) { + throw new Error('NoPermissions: read-only filesystem'); + } + backupContent = content; + }, + readFile: async () => new Uint8Array([]) + } as any, + writable: true, + configurable: true + }); try { await assert.rejects( @@ -490,7 +518,11 @@ describe('DatabaseDocument save/saveAs fallback', () => { assert.strictEqual(uncommitted.length, 1); assert.strictEqual(uncommitted[0].label, 'Update Cell'); } finally { - mockVscode.workspace.fs = originalFs; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: originalFs, + writable: true, + configurable: true + }); } }); @@ -539,16 +571,20 @@ describe('DatabaseDocument save/saveAs fallback', () => { doc.recordModification(firstModification); const originalFs = mockVscode.workspace.fs; - mockVscode.workspace.fs = { - ...originalFs, - writeFile: async (uri: any, content: any) => { - assert.strictEqual(uri, sourceUri); - assert.deepStrictEqual(content, new Uint8Array([7, 8, 9])); - markWriteStarted(); - await writeMayFinish; - }, - readFile: async () => new Uint8Array([]) - } as any; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + ...originalFs, + writeFile: async (uri: any, content: any) => { + assert.strictEqual(uri, sourceUri); + assert.deepStrictEqual(content, new Uint8Array([7, 8, 9])); + markWriteStarted(); + await writeMayFinish; + }, + readFile: async () => new Uint8Array([]) + } as any, + writable: true, + configurable: true + }); try { const savePromise = doc.save(); @@ -562,7 +598,11 @@ describe('DatabaseDocument save/saveAs fallback', () => { assert.deepStrictEqual(discardedModifications, [concurrentModification]); } finally { - mockVscode.workspace.fs = originalFs; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: originalFs, + writable: true, + configurable: true + }); } }); @@ -622,16 +662,20 @@ describe('DatabaseDocument save/saveAs fallback', () => { assert.ok(undoSecondUpdate, 'second update undo action should be emitted'); const originalFs = mockVscode.workspace.fs; - mockVscode.workspace.fs = { - ...originalFs, - writeFile: async (uri: any, content: any) => { - assert.strictEqual(uri, sourceUri); - assert.deepStrictEqual(content, new Uint8Array([10, 11, 12])); - markWriteStarted(); - await writeMayFinish; - }, - readFile: async () => new Uint8Array([]) - } as any; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + ...originalFs, + writeFile: async (uri: any, content: any) => { + assert.strictEqual(uri, sourceUri); + assert.deepStrictEqual(content, new Uint8Array([10, 11, 12])); + markWriteStarted(); + await writeMayFinish; + }, + readFile: async () => new Uint8Array([]) + } as any, + writable: true, + configurable: true + }); try { const savePromise = doc.save(); @@ -645,7 +689,11 @@ describe('DatabaseDocument save/saveAs fallback', () => { assert.deepStrictEqual(discardedModifications, [firstModification]); } finally { - mockVscode.workspace.fs = originalFs; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: originalFs, + writable: true, + configurable: true + }); } }); @@ -775,10 +823,14 @@ describe('DatabaseDocument hot-exit restore', () => { const originalWorkerFactoryCacheEntry = moduleCache[workerFactoryPath]; const databaseModelModulePath = require.resolve('../../src/databaseModel'); - mockVscode.workspace.fs = { - ...originalFs, - readFile: async () => backupData - } as any; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: { + ...originalFs, + readFile: async () => backupData + } as any, + writable: true, + configurable: true + }); moduleCache[workerFactoryPath] = { id: workerFactoryPath, @@ -820,7 +872,11 @@ describe('DatabaseDocument hot-exit restore', () => { assert.strictEqual(appliedModificationCount, 2); assert.deepStrictEqual(restoredRows[0].rows, [[1, 'Recovered']]); } finally { - mockVscode.workspace.fs = originalFs; + Object.defineProperty(mockVscode.workspace, 'fs', { + value: originalFs, + writable: true, + configurable: true + }); if (originalWorkerFactoryCacheEntry) { moduleCache[workerFactoryPath] = originalWorkerFactoryCacheEntry; } else { diff --git a/tests/unit/sqlite_db.test.ts b/tests/unit/sqlite_db.test.ts index f8f7aee..944b9f3 100644 --- a/tests/unit/sqlite_db.test.ts +++ b/tests/unit/sqlite_db.test.ts @@ -186,4 +186,61 @@ describe('WasmDatabaseEngine', () => { const result = await engine.executeQuery("SELECT COUNT(*) FROM aborted_restore"); assert.strictEqual(result[0].rows[0][0], 0); }); + + it('should check cancellation before each restored modification is replayed', async () => { + // This signal flips to aborted after the first replay entry has finished. + // applyModifications must observe that state at the next loop boundary + // before starting another forward replay operation. + await engine.executeQuery("DROP TABLE IF EXISTS boundary_abort_restore"); + await engine.executeQuery("CREATE TABLE boundary_abort_restore (id INTEGER PRIMARY KEY, name TEXT)"); + + const originalInsertRow = engine.insertRow.bind(engine); + const startedRowIds: unknown[] = []; + engine.insertRow = async (table, data) => { + startedRowIds.push(data.id); + return originalInsertRow(table, data); + }; + + let checkCount = 0; + let isAborted = false; + const boundarySignal = { + throwIfAborted() { + checkCount++; + if (isAborted) { + throw new Error('Replay aborted at loop boundary'); + } + if (checkCount === 2) { + isAborted = true; + } + } + } as AbortSignal; + + try { + await assert.rejects( + () => engine.applyModifications([ + { + description: 'Insert first row before abort boundary', + modificationType: 'row_insert', + targetTable: 'boundary_abort_restore', + targetRowId: 1, + rowData: { id: 1, name: 'First' } + }, + { + description: 'Insert second row after abort boundary', + modificationType: 'row_insert', + targetTable: 'boundary_abort_restore', + targetRowId: 2, + rowData: { id: 2, name: 'Second' } + } + ], boundarySignal), + /Replay aborted at loop boundary/ + ); + + assert.deepStrictEqual(startedRowIds, [1]); + const result = await engine.executeQuery("SELECT COUNT(*) FROM boundary_abort_restore"); + assert.strictEqual(result[0].rows[0][0], 0); + } finally { + engine.insertRow = originalInsertRow; + } + }); }); diff --git a/tests/unit/virtualFileSystem.test.ts b/tests/unit/virtualFileSystem.test.ts index 850277d..f6e3b9e 100644 --- a/tests/unit/virtualFileSystem.test.ts +++ b/tests/unit/virtualFileSystem.test.ts @@ -226,6 +226,28 @@ describe('SQLiteFileSystemProvider', () => { }, (err: any) => err.message.includes('Database write error')); }); + it('should reject read-only documents before updating a cell', async () => { + const dbOps = { + updateCell: mock.fn(async () => {}) + }; + setupMockDocument(docKey, dbOps); + const document = DocumentRegistry.get(docKey) as any; + Object.defineProperty(document, 'isReadOnlyMode', { + value: true, + configurable: true + }); + + const uri = vscode.Uri.parse(`vscode-sqlite://${docKey}/users/group/1/col.txt`); + const content = new TextEncoder().encode('blocked write'); + + // Read-only database documents are immutable from the cell editor, + // so writeFile must stop before decoding content or mutating SQLite. + await assert.rejects(async () => { + await provider.writeFile(uri, content, { create: false, overwrite: true }); + }, (err: any) => err.message.includes('Database is read-only')); + assert.strictEqual(dbOps.updateCell.mock.callCount(), 0); + }); + it('should write text content correctly', async () => { const dbOps = { updateCell: mock.fn(async () => {}) diff --git a/tests/unit/workerFactory_browser.test.ts b/tests/unit/workerFactory_browser.test.ts index 790bb4e..5d5227a 100644 --- a/tests/unit/workerFactory_browser.test.ts +++ b/tests/unit/workerFactory_browser.test.ts @@ -21,7 +21,7 @@ interface FakeEndpoint { redoModification?(mod: ModificationEntry): Promise; flushChanges?(signal?: AbortSignal): Promise; discardModifications?(mods: ModificationEntry[], signal?: AbortSignal): Promise; - updateCell(table: string, rowId: string | number, column: string, value: CellValue): Promise; + updateCell(table: string, rowId: string | number, column: string, value: CellValue, patch?: string): Promise; insertRow(table: string, data: Record): Promise; updateCellBatch(table: string, updates: CellUpdate[]): Promise; ping(): Promise; @@ -119,6 +119,7 @@ describe('workerFactory browser WASM connection', () => { it('uses an in-process endpoint and passes raw Uint8Array values directly', async () => { let initConfig: DatabaseInitConfig | undefined; let updateCellValue: CellValue | undefined; + let updateCellPatch: string | undefined; let insertRowValue: CellValue | undefined; let updateBatchValue: CellValue | undefined; @@ -129,8 +130,9 @@ describe('workerFactory browser WASM connection', () => { }, runQuery: async () => [], exportDatabase: async () => new Uint8Array(), - updateCell: async (_table, _rowId, _column, value) => { + updateCell: async (_table, _rowId, _column, value, patch) => { updateCellValue = value; + updateCellPatch = patch; }, insertRow: async (_table, data) => { insertRowValue = data.blob; @@ -164,11 +166,14 @@ describe('workerFactory browser WASM connection', () => { assert.strictEqual(connection.isReadOnly, false); const blobValue = new Uint8Array([9, 8, 7]); - await connection.databaseOps.updateCell('items', 1, 'blob', blobValue); + // The browser facade calls the in-process endpoint directly, so the patch + // argument must survive this delegation layer alongside the raw Uint8Array. + await connection.databaseOps.updateCell('items', 1, 'blob', blobValue, '{"merged":true}'); await connection.databaseOps.insertRow('items', { blob: blobValue }); await connection.databaseOps.updateCellBatch('items', [{ rowId: 1, column: 'blob', value: blobValue }]); assert.strictEqual(updateCellValue, blobValue); + assert.strictEqual(updateCellPatch, '{"merged":true}'); assert.strictEqual(insertRowValue, blobValue); assert.strictEqual(updateBatchValue, blobValue); }); diff --git a/tests/unit/worker_endpoint.test.ts b/tests/unit/worker_endpoint.test.ts index 755b93f..464580f 100644 --- a/tests/unit/worker_endpoint.test.ts +++ b/tests/unit/worker_endpoint.test.ts @@ -163,4 +163,40 @@ describe('Worker Endpoint', () => { await endpoint.writeToFile('/tmp/test_dump.db'); }); + + it('should forward JSON merge patches through updateCell to the WASM engine', async () => { + await endpoint.initializeDatabase('test.db', { + content: null, + maxSize: 0, + readOnlyMode: false, + wasmBinary + }); + + await endpoint.createTable('json_items', [ + { name: 'id', type: 'INTEGER', primaryKey: true, notNull: false }, + { name: 'data', type: 'TEXT', primaryKey: false, notNull: false } + ]); + await endpoint.insertRow('json_items', { + id: 1, + data: '{"preserved":true,"changed":"before"}' + }); + + // The fourth argument represents the stale full-cell value. When the + // fifth patch argument reaches WasmDatabaseEngine.updateCell, SQLite + // merges it into the existing document instead of writing this stale value. + await endpoint.updateCell( + 'json_items', + 1, + 'data', + '{"changed":"stale-overwrite"}', + '{"changed":"after","added":42}' + ); + + const tableData = await endpoint.fetchTableData('json_items', { offset: 0, limit: 10 }); + assert.deepStrictEqual(JSON.parse(tableData.rows[0][1] as string), { + preserved: true, + changed: 'after', + added: 42 + }); + }); }); From 1db571a1c5122b0f6c19322d639c02fe1b7c145c Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 16:32:27 +0200 Subject: [PATCH 09/10] fix(save): honor cancellation token in DatabaseDocument.save (#424 review round 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gemini review: save(cancellation?) accepted a CancellationToken but ignored it. The WASM web-save path (serialize in-memory db + workspace.fs.writeFile over a possibly-remote filesystem) can be slow, so a user-requested cancel should abort it gracefully rather than always running to completion. save() now throws vsc.CancellationError when the token is already cancelled — once on entry, and again before the WASM serialize/write begins (after the quick native-engine branch). The native branch returns fast and needs no mid-op check. Adds CancellationError to the vscode test mock and a revert-proof test: a pre-cancelled token makes save() reject with a Canceled error and perform no serialize/writeToFile work. tsc clean; build OK; full suite 357/357. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/databaseModel.ts | 9 +++++++++ tests/unit/databaseModel.test.ts | 28 ++++++++++++++++++++++++++++ tests/unit/mocks/vscode.ts | 6 ++++++ 3 files changed, 43 insertions(+) diff --git a/src/databaseModel.ts b/src/databaseModel.ts index f3c30ce..fdb0368 100644 --- a/src/databaseModel.ts +++ b/src/databaseModel.ts @@ -324,6 +324,9 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { * For WASM engine: we need to serialize the in-memory database and write to disk. */ async save(cancellation?: vsc.CancellationToken): Promise { + if (cancellation?.isCancellationRequested) { + throw new vsc.CancellationError(); + } await this.ensureWritable(); // Check if using native engine - changes are already on disk @@ -342,6 +345,12 @@ export class DatabaseDocument extends Disposable implements vsc.CustomDocument { return; } + // The WASM serialize + filesystem write can be slow for large databases or + // remote web filesystems; re-check cancellation before starting that work. + if (cancellation?.isCancellationRequested) { + throw new vsc.CancellationError(); + } + // Export in-memory database to file (WASM engine only) // We always do this for WASM, regardless of auto-commit setting, because WASM is in-memory. if (this.uri.scheme === 'file') { diff --git a/tests/unit/databaseModel.test.ts b/tests/unit/databaseModel.test.ts index 8c9f57a..91031b2 100644 --- a/tests/unit/databaseModel.test.ts +++ b/tests/unit/databaseModel.test.ts @@ -301,6 +301,34 @@ describe('DatabaseDocument save/saveAs fallback', () => { ); }; + it('save: throws CancellationError and writes nothing when the token is already cancelled', async () => { + let serializeCalled = false; + let writeToFileCalled = false; + const dbOps = { + engineKind: Promise.resolve('wasm'), + writeToFile: async () => { writeToFileCalled = true; }, + serializeDatabase: async () => { + serializeCalled = true; + return new Uint8Array([1, 2, 3]); + } + }; + + const doc = createDocBypassingFactory(dbOps); + + // A pre-cancelled token must abort save before any serialize/write work. + const cancelledToken = { + isCancellationRequested: true, + onCancellationRequested: () => ({ dispose() {} }) + } as any; + + await assert.rejects( + () => doc.save(cancelledToken), + (err: any) => err instanceof Error && err.name === 'Canceled' + ); + assert.strictEqual(serializeCalled, false, 'serializeDatabase must not run when cancelled'); + assert.strictEqual(writeToFileCalled, false, 'writeToFile must not run when cancelled'); + }); + it('saveAs: falls back to buffer transfer when writeToFile fails for file URI', async () => { let serialized = false; const dbOps = { diff --git a/tests/unit/mocks/vscode.ts b/tests/unit/mocks/vscode.ts index 2a67f86..4d7fcc9 100644 --- a/tests/unit/mocks/vscode.ts +++ b/tests/unit/mocks/vscode.ts @@ -73,6 +73,12 @@ export const mockVscode = { NoPermissions: (msg?: string) => new Error(msg || 'NoPermissions'), Unavailable: (msg: string) => new Error(msg || 'Unavailable') }, + CancellationError: class CancellationError extends Error { + constructor() { + super('Canceled'); + this.name = 'Canceled'; + } + }, FileChangeType: { Changed: 1, Created: 2, From 8b67d8e0bf435ff6e88aab1dffc3a3303fead4dc Mon Sep 17 00:00:00 2001 From: zknpr Date: Mon, 1 Jun 2026 16:58:24 +0200 Subject: [PATCH 10/10] docs(changelog): note web-edit safety hardening in 1.5.0 (WAL RO, JSON merge, atomic recovery) Round 5-7 review hardening was user-facing but missing from the 1.5.0 notes. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1c8352..2ad1225 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ ### Safety - **No silent data loss on web save failure**. When the underlying filesystem rejects a write (for example, a read-only web filesystem provider), save now surfaces a clear "Failed to save database" error. The edit history is kept uncommitted so changes can be retried or recovered, rather than being marked as saved. +- **WAL databases open read-only in web mode**. When a database has an active write-ahead log (`-wal`), the web build opens it read-only instead of risking a save that writes back a main image missing committed WAL pages. Read-only state is enforced on the cell-editor write path too, not just on save. +- **JSON cell edits apply as merge patches**. Editing a JSON cell applies an RFC 7396 merge patch to the stored value (web and desktop), so a concurrent change to a different key is preserved instead of being overwritten by a stale full-document write. +- **Faithful, atomic recovery of unsaved edits**. Hot-exit restore replays uncommitted edits into a freshly opened in-memory database inside a single SAVEPOINT, rolled back on any error or cancellation, so a partially replayed database is never left behind. ## 1.4.0