Repository navigation
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
Changes from 9 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,9 +6,28 @@ | |
|
|
||
| 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 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' && !!candidate.meta; | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| export class FsAssetStore extends FsSnapshotStore { | ||
| // --- Image Store Methods --- | ||
|
|
||
|
|
@@ -22,8 +41,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 +60,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,7 +96,29 @@ 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( | ||
|
|
@@ -85,23 +128,53 @@ export class FsAssetStore extends FsSnapshotStore { | |
| 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) { | ||
|
|
@@ -113,8 +186,12 @@ export class FsAssetStore extends FsSnapshotStore { | |
| async deleteBinderAsset(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.