-
Notifications
You must be signed in to change notification settings - Fork 5
fix(desktop): atomic writes for all filesystem-backed stores #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
29231c2
af1ae9b
e004c23
e892ed8
05a20ff
e3f0273
aadcd42
a921910
b3de72a
39ea4be
c3c9931
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,10 +6,120 @@ | |
|
|
||
| import { logger } from '../logger'; | ||
| import type { BinderAssetMeta, BinderAssetPayload } from '../storageBackend'; | ||
| import { retryFs, sanitizePathSegment } from './fsCore'; | ||
| import { retryFs, sanitizePathSegment, writeFileAtomic, writeTextFileAtomic } from './fsCore'; | ||
| import { FsSnapshotStore } from './snapshotFsStore'; | ||
|
|
||
| interface BinderAssetManifest { | ||
| version: 1; | ||
| dataFile: string; | ||
| meta: BinderAssetMeta; | ||
| } | ||
|
|
||
| function isBinderAssetMeta(value: unknown): value is BinderAssetMeta { | ||
| if (!value || typeof value !== 'object') return false; | ||
| const candidate = value as Partial<BinderAssetMeta>; | ||
| return ( | ||
| typeof candidate.mimeType === 'string' && | ||
| typeof candidate.originalFileName === 'string' && | ||
| typeof candidate.byteSize === 'number' && | ||
| Number.isFinite(candidate.byteSize) && | ||
| candidate.byteSize >= 0 | ||
| ); | ||
| } | ||
|
|
||
| function createBinderRevision(): string { | ||
| if (typeof crypto.randomUUID === 'function') return crypto.randomUUID(); | ||
| return Array.from(crypto.getRandomValues(new Uint8Array(16)), (byte) => | ||
| byte.toString(16).padStart(2, '0'), | ||
| ).join(''); | ||
| } | ||
|
|
||
| function isBinderAssetManifest(value: unknown): value is BinderAssetManifest { | ||
| if (!value || typeof value !== 'object') return false; | ||
| const candidate = value as Partial<BinderAssetManifest>; | ||
| return ( | ||
| candidate.version === 1 && | ||
| typeof candidate.dataFile === 'string' && | ||
| isBinderAssetMeta(candidate.meta) | ||
| ); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| const BINDER_REVISION_FILE_PATTERN = /^(.+)\.([0-9a-f]{32}|[0-9a-f-]{36})\.bin$/i; | ||
|
|
||
| export class FsAssetStore extends FsSnapshotStore { | ||
| private readonly binderOperationTails = new Map<string, Promise<void>>(); | ||
|
|
||
| private enqueueBinderOperation<T>( | ||
| projectId: string, | ||
| assetId: string, | ||
| operation: () => Promise<T>, | ||
| ): Promise<T> { | ||
| const key = `${projectId}\u0000${assetId}`; | ||
| const previous = this.binderOperationTails.get(key) ?? Promise.resolve(); | ||
|
Comment on lines
+57
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The binder operation queue uses raw IDs while the filesystem paths use sanitized IDs. For example, IDs that differ only by path-invalid characters can map to the same Severity Level: Major
|
||
| const result = previous.catch(() => {}).then(operation); | ||
| const tail = result.then( | ||
| () => undefined, | ||
| () => undefined, | ||
| ); | ||
| this.binderOperationTails.set(key, tail); | ||
| void tail.then(() => { | ||
| if (this.binderOperationTails.get(key) === tail) this.binderOperationTails.delete(key); | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| override async initialize(): Promise<void> { | ||
| await super.initialize(); | ||
| await this.cleanupOrphanedBinderRevisions(); | ||
| } | ||
|
|
||
| private async cleanupOrphanedBinderRevisions(): Promise<void> { | ||
| try { | ||
| const apis = await this.getApis(); | ||
| const appDataPath = await this.ensureAppDataPath(); | ||
| const projectsPath = await apis.join(appDataPath, 'projects'); | ||
| if (!(await apis.exists(projectsPath))) return; | ||
| const projects = await retryFs(() => apis.readDir(projectsPath)); | ||
| for (const project of projects) { | ||
| if (!project.name || !project.isDirectory) continue; | ||
| const binderPath = await apis.join(projectsPath, project.name, 'binder'); | ||
| if (!(await apis.exists(binderPath))) continue; | ||
| const entries = await retryFs(() => apis.readDir(binderPath)); | ||
| const committedFiles = new Set<string>(); | ||
| const protectedAssets = new Set<string>(); | ||
| for (const entry of entries) { | ||
| const metaName = entry.name; | ||
| if (!metaName?.endsWith('.meta.json')) continue; | ||
| const safeAsset = metaName.replace(/\.meta\.json$/, ''); | ||
| const metaFile = await apis.join(binderPath, metaName); | ||
| try { | ||
| const raw = JSON.parse(await retryFs(() => apis.readTextFile(metaFile))) as unknown; | ||
| if (raw && typeof raw === 'object' && ('version' in raw || 'dataFile' in raw)) { | ||
| const manifest = await this.readBinderManifest(apis, metaFile, safeAsset); | ||
| if (manifest) committedFiles.add(manifest.dataFile); | ||
| else protectedAssets.add(safeAsset); | ||
| } | ||
| } catch { | ||
| protectedAssets.add(safeAsset); | ||
| } | ||
| } | ||
| for (const entry of entries) { | ||
| const match = entry.name?.match(BINDER_REVISION_FILE_PATTERN); | ||
| if (!match) continue; | ||
| const [, safeAsset] = match; | ||
| if (!safeAsset || committedFiles.has(entry.name!) || protectedAssets.has(safeAsset)) | ||
|
Comment on lines
+107
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On startup, a legacy asset whose sanitized ID ends in a dot plus 32 hex characters or a 36-character UUID-like value is misclassified here as an orphaned revision; for example, AGENTS.md reference: AGENTS.md:L9-L9 Useful? React with 👍 / 👎. |
||
| continue; | ||
| const revisionFile = await apis.join(binderPath, entry.name!); | ||
| await retryFs(() => apis.remove(revisionFile)).catch((error) => { | ||
| logger.warn('Failed to remove orphaned binder asset revision:', error); | ||
| }); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| logger.warn('Failed to clean up orphaned binder asset revisions:', error); | ||
| } | ||
| } | ||
|
|
||
| // --- Image Store Methods --- | ||
|
|
||
| async saveImage(id: string, base64Data: string): Promise<void> { | ||
|
|
@@ -22,8 +132,8 @@ export class FsAssetStore extends FsSnapshotStore { | |
| } | ||
|
|
||
| const imageFile = await apis.join(imagesPath, `${sanitizePathSegment(id, 'image')}.png`); | ||
| const cleanBase64 = base64Data.replace(/^data:image\/png;base64,/, ''); | ||
| await retryFs(() => apis.writeTextFile(imageFile, cleanBase64)); | ||
| // QNBS-v3: preserve the original data URL so JPEG/WebP uploads keep their MIME type; legacy raw base64 remains readable below. | ||
| await writeTextFileAtomic(apis, imageFile, base64Data); | ||
|
Comment on lines
+135
to
+136
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The filesystem backend treats an empty string as image content, but delete flows use Severity Level: Major
|
||
| } | ||
|
|
||
| async getImage(id: string): Promise<string | null> { | ||
|
|
@@ -41,7 +151,9 @@ export class FsAssetStore extends FsSnapshotStore { | |
| } | ||
|
|
||
| const base64Data = await retryFs(() => apis.readTextFile(imageFile)); | ||
| return `data:image/png;base64,${base64Data}`; | ||
| return base64Data.startsWith('data:image/') | ||
| ? base64Data | ||
| : `data:image/png;base64,${base64Data}`; | ||
| } catch (error) { | ||
| logger.error('Failed to load image:', error); | ||
| return null; | ||
|
|
@@ -75,33 +187,96 @@ export class FsAssetStore extends FsSnapshotStore { | |
| const dir = await apis.join(appDataPath, 'projects', safeId, 'binder'); | ||
| const binFile = await apis.join(dir, `${safeAsset}.bin`); | ||
| const metaFile = await apis.join(dir, `${safeAsset}.meta.json`); | ||
| return { apis, dir, binFile, metaFile }; | ||
| return { apis, dir, binFile, metaFile, safeAsset }; | ||
| } | ||
|
|
||
| private async readBinderManifest( | ||
| apis: Awaited<ReturnType<FsAssetStore['binderAssetPaths']>>['apis'], | ||
| metaFile: string, | ||
| safeAsset: string, | ||
| ): Promise<BinderAssetManifest | null> { | ||
| try { | ||
| const parsed = JSON.parse(await retryFs(() => apis.readTextFile(metaFile))) as unknown; | ||
| if (!isBinderAssetManifest(parsed)) return null; | ||
| if ( | ||
| !parsed.dataFile.startsWith(`${safeAsset}.`) || | ||
| !parsed.dataFile.endsWith('.bin') || | ||
| parsed.dataFile.includes('/') || | ||
| parsed.dataFile.includes('\\') | ||
| ) { | ||
| throw new Error('Binder asset manifest references an invalid data file'); | ||
| } | ||
| return parsed; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| async saveBinderAsset( | ||
| projectId: string, | ||
| assetId: string, | ||
| data: ArrayBuffer, | ||
| meta: BinderAssetMeta, | ||
| ): Promise<void> { | ||
| return this.enqueueBinderOperation(projectId, assetId, () => | ||
| this.saveBinderAssetLocked(projectId, assetId, data, meta), | ||
| ); | ||
| } | ||
|
|
||
| private async saveBinderAssetLocked( | ||
| projectId: string, | ||
| assetId: string, | ||
| data: ArrayBuffer, | ||
| meta: BinderAssetMeta, | ||
| ): Promise<void> { | ||
| const apis = await this.getApis(); | ||
| const { dir, binFile, metaFile } = await this.binderAssetPaths(projectId, assetId); | ||
| const { dir, binFile, metaFile, safeAsset } = await this.binderAssetPaths(projectId, assetId); | ||
| if (!(await apis.exists(dir))) await apis.mkdir(dir, { recursive: true }); | ||
| const metaOut: BinderAssetMeta = { ...meta, byteSize: data.byteLength }; | ||
| await retryFs(() => apis.writeFile(binFile, new Uint8Array(data))); | ||
| await retryFs(() => apis.writeTextFile(metaFile, JSON.stringify(metaOut))); | ||
| const prior = await this.readBinderManifest(apis, metaFile, safeAsset); | ||
| const dataFileName = `${safeAsset}.${createBinderRevision()}.bin`; | ||
| const dataFile = await apis.join(dir, dataFileName); | ||
| await writeFileAtomic(apis, dataFile, new Uint8Array(data)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If this binary write succeeds but the following manifest write fails (for example, ENOSPC) or the process exits between them, the new Useful? React with 👍 / 👎. |
||
| // QNBS-v3: publishing this manifest is the binder pair's commit point, so readers never combine new bytes with stale metadata. | ||
| try { | ||
| await writeTextFileAtomic( | ||
| apis, | ||
| metaFile, | ||
| JSON.stringify({ version: 1, dataFile: dataFileName, meta: metaOut }), | ||
| ); | ||
|
Comment on lines
+242
to
+246
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The writer publishes a revision manifest without ensuring that Severity Level: Major
|
||
| } catch (error) { | ||
| // QNBS-v3: the revision is unreachable until its manifest commits, so failed publication must not leak a new binary on every retry. | ||
| await retryFs(() => apis.remove(dataFile)).catch((cleanupError) => { | ||
|
Comment on lines
+247
to
+249
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the native manifest replacement succeeds but the subsequent parent-directory AGENTS.md reference: AGENTS.md:L9-L9 Useful? React with 👍 / 👎. |
||
| logger.warn('Failed to remove unpublished binder asset revision:', cleanupError); | ||
| }); | ||
| throw error; | ||
| } | ||
| if (prior) { | ||
| const priorFile = await apis.join(dir, prior.dataFile); | ||
| if (priorFile !== dataFile && (await apis.exists(priorFile))) { | ||
| await retryFs(() => apis.remove(priorFile)).catch((error) => { | ||
| logger.warn('Failed to remove superseded binder asset revision:', error); | ||
| }); | ||
| } | ||
| } else if (await apis.exists(binFile)) { | ||
| await retryFs(() => apis.remove(binFile)).catch((error) => { | ||
| logger.warn('Failed to remove superseded legacy binder asset:', error); | ||
| }); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| async getBinderAsset(projectId: string, assetId: string): Promise<BinderAssetPayload | null> { | ||
| try { | ||
| const apis = await this.getApis(); | ||
| const { binFile, metaFile } = await this.binderAssetPaths(projectId, assetId); | ||
| if (!(await apis.exists(binFile)) || !(await apis.exists(metaFile))) return null; | ||
| const [bytes, metaRaw] = await Promise.all([ | ||
| retryFs(() => apis.readFile(binFile)), | ||
| retryFs(() => apis.readTextFile(metaFile)), | ||
| ]); | ||
| const meta = JSON.parse(metaRaw) as BinderAssetMeta; | ||
| const { binFile, metaFile, dir, safeAsset } = await this.binderAssetPaths(projectId, assetId); | ||
| if (!(await apis.exists(metaFile))) return null; | ||
| const manifest = await this.readBinderManifest(apis, metaFile, safeAsset); | ||
| const dataFile = manifest ? await apis.join(dir, manifest.dataFile) : binFile; | ||
| if (!(await apis.exists(dataFile))) return null; | ||
| const bytes = await retryFs(() => apis.readFile(dataFile)); | ||
| const meta = manifest | ||
| ? manifest.meta | ||
| : (JSON.parse(await retryFs(() => apis.readTextFile(metaFile))) as BinderAssetMeta); | ||
| const copy = bytes.buffer.slice(bytes.byteOffset, bytes.byteOffset + bytes.byteLength); | ||
| return { data: copy, meta }; | ||
| } catch (error) { | ||
|
|
@@ -111,10 +286,20 @@ export class FsAssetStore extends FsSnapshotStore { | |
| } | ||
|
|
||
| async deleteBinderAsset(projectId: string, assetId: string): Promise<void> { | ||
| return this.enqueueBinderOperation(projectId, assetId, () => | ||
| this.deleteBinderAssetLocked(projectId, assetId), | ||
| ); | ||
| } | ||
|
|
||
| private async deleteBinderAssetLocked(projectId: string, assetId: string): Promise<void> { | ||
| try { | ||
| const apis = await this.getApis(); | ||
| const { binFile, metaFile } = await this.binderAssetPaths(projectId, assetId); | ||
| if (await apis.exists(binFile)) await retryFs(() => apis.remove(binFile)); | ||
| const { binFile, metaFile, dir, safeAsset } = await this.binderAssetPaths(projectId, assetId); | ||
| const manifest = await this.readBinderManifest(apis, metaFile, safeAsset); | ||
| const dataFile = manifest ? await apis.join(dir, manifest.dataFile) : binFile; | ||
| if (await apis.exists(dataFile)) await retryFs(() => apis.remove(dataFile)); | ||
| if (dataFile !== binFile && (await apis.exists(binFile))) | ||
| await retryFs(() => apis.remove(binFile)); | ||
| if (await apis.exists(metaFile)) await retryFs(() => apis.remove(metaFile)); | ||
| } catch (error) { | ||
| logger.warn('deleteBinderAsset failed:', error); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.