From ae83fd875676eee852a93bf257686589c574d015 Mon Sep 17 00:00:00 2001 From: "qodo-code-review[bot]" <151058649+qodo-code-review[bot]@users.noreply.github.com> Date: Wed, 29 Jul 2026 02:01:40 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix:=203=20findings=20=E2=80=94=20Remove=20?= =?UTF-8?q?reentrant=20lock=20bypass;=20Bound=20write=20lock=20acquisi?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove reentrant lock bypass - Bound write lock acquisition - Keep transfer result type private --- QuickShell.Raycast/src/lib/storage.ts | 121 +++++++++++++++----------- 1 file changed, 68 insertions(+), 53 deletions(-) diff --git a/QuickShell.Raycast/src/lib/storage.ts b/QuickShell.Raycast/src/lib/storage.ts index 877751d7..0a5b34f1 100644 --- a/QuickShell.Raycast/src/lib/storage.ts +++ b/QuickShell.Raycast/src/lib/storage.ts @@ -32,7 +32,7 @@ export type StorageAdapter = { setItem: (key: string, value: string) => Promise; }; -export type StorageTransferResult = { +type StorageTransferResult = { success: true; message: string; }; @@ -48,7 +48,7 @@ export class QuickShellStorage { private recentWriteDirty = false; /** Serializes load-modify-save mutations so overlapping commands cannot clobber each other. */ private writeTail: Promise = Promise.resolve(); - private writeDepth = 0; + private static readonly WRITE_LOCK_TIMEOUT_MS = 5000; constructor( private readonly adapter: StorageAdapter, @@ -75,7 +75,7 @@ export class QuickShellStorage { async undo(): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); if (this.undoHistory.length === 0) { return false; } @@ -97,7 +97,7 @@ export class QuickShellStorage { async redo(): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); if (this.redoHistory.length === 0) { return false; } @@ -128,10 +128,10 @@ export class QuickShellStorage { async importJson(raw: string, mode: "merge" | "replace" = "merge"): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const existing = mode === "merge" ? await this.load() : createEmptyStoredData(); const result = parseImportPayload(raw, existing); - await this.save(result.data, { preserveSecurity: false, allowSubmittedSecurity: true }); + await this.saveUnlocked(result.data, { preserveSecurity: false, allowSubmittedSecurity: true }); return result; }); } @@ -142,7 +142,7 @@ export class QuickShellStorage { */ async resetAll(): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); await this.ensureLoaded(); if (this.cache!.workspaces.length === 0) { return { success: true, message: "No workspaces to reset." }; @@ -153,7 +153,7 @@ export class QuickShellStorage { const emptied = createEmptyStoredData(); emptied.settings = { ...this.cache!.settings }; - await this.save(emptied, { preserveSecurity: false, allowSubmittedSecurity: true }); + await this.saveUnlocked(emptied, { preserveSecurity: false, allowSubmittedSecurity: true }); const itemsLabel = count === 1 ? "workspace" : "workspaces"; return { @@ -166,7 +166,7 @@ export class QuickShellStorage { /** Restores the durable reset-all backup into the live store. */ async restoreFromBackup(): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const raw = await this.adapter.getItem(BACKUP_STORAGE_KEY); if (!raw) { return { success: true, message: "No workspace backup found." }; @@ -180,7 +180,7 @@ export class QuickShellStorage { } const restored = migrateStoredData(parsed); - await this.save(restored, { preserveSecurity: false, allowSubmittedSecurity: true }); + await this.saveUnlocked(restored, { preserveSecurity: false, allowSubmittedSecurity: true }); return { success: true, message: `Restored ${restored.workspaces.length} workspace${restored.workspaces.length === 1 ? "" : "s"} from backup.`, @@ -193,7 +193,7 @@ export class QuickShellStorage { return summarizeImportConflicts(raw, existing); } - async save( + private async saveUnlocked( data: StoredData, options?: { recordHistory?: boolean; @@ -201,7 +201,6 @@ export class QuickShellStorage { allowSubmittedSecurity?: boolean; }, ): Promise { - return this.withWriteLock(async () => { const recordHistory = options?.recordHistory ?? true; const preserveSecurity = options?.preserveSecurity ?? true; const allowSubmittedSecurity = options?.allowSubmittedSecurity ?? false; @@ -250,7 +249,17 @@ export class QuickShellStorage { this.cache = normalized; await this.persistCache({ recordHistory: false }); - }); + } + + async save( + data: StoredData, + options?: { + recordHistory?: boolean; + preserveSecurity?: boolean; + allowSubmittedSecurity?: boolean; + }, + ): Promise { + return this.withWriteLock(() => this.saveUnlocked(data, options)); } async getWorkspaces(): Promise { @@ -294,7 +303,7 @@ export class QuickShellStorage { reviewToken: WorkspaceReviewToken, ): Promise<"granted" | "already" | "changed" | "invalid" | "missing"> { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const workspace = data.workspaces.find((candidate) => candidate.id === workspaceId); if (!workspace) { @@ -314,14 +323,14 @@ export class QuickShellStorage { } data.workspaceSecurity = { ...(data.workspaceSecurity ?? {}) }; data.workspaceSecurity[workspaceId] = { isTrusted: true, revision: security.revision + 1 }; - await this.save(data, { preserveSecurity: false, allowSubmittedSecurity: true }); + await this.saveUnlocked(data, { preserveSecurity: false, allowSubmittedSecurity: true }); return "granted"; }); } async revokeTrust(workspaceId: string): Promise<"revoked" | "already" | "missing"> { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const workspace = data.workspaces.find((candidate) => candidate.id === workspaceId); if (!workspace) { @@ -333,7 +342,7 @@ export class QuickShellStorage { } data.workspaceSecurity = { ...(data.workspaceSecurity ?? {}) }; data.workspaceSecurity[workspaceId] = { isTrusted: false, revision: security.revision + 1 }; - await this.save(data, { preserveSecurity: false, allowSubmittedSecurity: true }); + await this.saveUnlocked(data, { preserveSecurity: false, allowSubmittedSecurity: true }); return "revoked"; }); } @@ -349,7 +358,7 @@ export class QuickShellStorage { async upsertWorkspace(workspace: Workspace): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const normalized = normalizeWorkspace({ ...workspace, @@ -384,26 +393,26 @@ export class QuickShellStorage { data.layoutEntries = [...(data.layoutEntries ?? []), { type: "workspace", workspaceId: normalized.id }]; } - await this.save(data, { allowSubmittedSecurity: true }); + await this.saveUnlocked(data, { allowSubmittedSecurity: true }); return normalized; }); } async deleteWorkspace(workspaceId: string): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); data.workspaces = data.workspaces.filter((workspace) => workspace.id !== workspaceId); data.layoutEntries = (data.layoutEntries ?? []).filter( (entry) => entry.type !== "workspace" || entry.workspaceId !== workspaceId, ); - await this.save(data); + await this.saveUnlocked(data); }); } async duplicateWorkspace(workspaceId: string): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const source = data.workspaces.find((workspace) => workspace.id === workspaceId); if (!source) { @@ -429,7 +438,7 @@ export class QuickShellStorage { data.workspaceSecurity = { ...(data.workspaceSecurity ?? {}) }; data.workspaceSecurity[duplicate.id] = { isTrusted: sourceSecurity.isTrusted, revision: 1 }; data.layoutEntries = [...(data.layoutEntries ?? []), { type: "workspace", workspaceId: duplicate.id }]; - await this.save(data, { preserveSecurity: false, allowSubmittedSecurity: true }); + await this.saveUnlocked(data, { preserveSecurity: false, allowSubmittedSecurity: true }); return duplicate; }); } @@ -446,7 +455,7 @@ export class QuickShellStorage { async setBranchTarget(worktreeKey: string, branch: string): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const key = worktreeKey.trim().toLowerCase(); const value = branch.trim(); @@ -457,13 +466,13 @@ export class QuickShellStorage { throw new Error("Invalid branch name."); } data.branchTargets = { ...(data.branchTargets ?? {}), [key]: value }; - await this.save(data); + await this.saveUnlocked(data); }); } async clearBranchTarget(worktreeKey: string): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const key = worktreeKey.trim().toLowerCase(); if (!data.branchTargets || !(key in data.branchTargets)) { @@ -472,7 +481,7 @@ export class QuickShellStorage { const next = { ...data.branchTargets }; delete next[key]; data.branchTargets = next; - await this.save(data); + await this.saveUnlocked(data); }); } @@ -483,7 +492,7 @@ export class QuickShellStorage { async insertSeparator(title?: string | null, beforeWorkspaceId?: string): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const separator: LayoutEntry = { type: "separator", @@ -500,23 +509,23 @@ export class QuickShellStorage { layout.push(separator); } data.layoutEntries = layout; - await this.save(data); + await this.saveUnlocked(data); return separator; }); } async removeLayoutEntry(entryId: string): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); data.layoutEntries = (data.layoutEntries ?? []).filter((entry) => !layoutEntryMatchesId(entry, entryId)); - await this.save(data); + await this.saveUnlocked(data); }); } async moveLayoutEntry(entryId: string, direction: "up" | "down"): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const layout = [...(data.layoutEntries ?? [])]; const index = layout.findIndex((entry) => layoutEntryMatchesId(entry, entryId)); @@ -531,13 +540,13 @@ export class QuickShellStorage { layout[index] = layout[swapIndex]; layout[swapIndex] = current; data.layoutEntries = layout; - await this.save(data); + await this.saveUnlocked(data); }); } async setFavorite(workspaceId: string, isPinned: boolean): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const workspace = data.workspaces.find((item) => item.id === workspaceId); if (!workspace) { @@ -554,7 +563,7 @@ export class QuickShellStorage { workspace.pinOrder = null; } - await this.save(data); + await this.saveUnlocked(data); return { ...workspace }; }); } @@ -562,7 +571,7 @@ export class QuickShellStorage { /** Returns the moved workspace, or `null` when the move is a boundary no-op. */ async moveFavorite(workspaceId: string, direction: "up" | "down" | "top" | "bottom"): Promise { return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); const workspace = data.workspaces.find((item) => item.id === workspaceId); if (!workspace || !workspace.isPinned) { @@ -601,7 +610,7 @@ export class QuickShellStorage { item.pinOrder = orderIndex + 1; }); - await this.save(data); + await this.saveUnlocked(data); return { ...favorites[targetIndex] }; }); } @@ -619,8 +628,7 @@ export class QuickShellStorage { }); } - async flushRecentWrites(): Promise { - return this.withWriteLock(async () => { + private async flushRecentWritesUnlocked(): Promise { if (this.recentWriteTimer) { clearTimeout(this.recentWriteTimer); this.recentWriteTimer = null; @@ -630,7 +638,10 @@ export class QuickShellStorage { } this.recentWriteDirty = false; await this.persistCache({ recordHistory: false }); - }); + } + + async flushRecentWrites(): Promise { + return this.withWriteLock(() => this.flushRecentWritesUnlocked()); } async updateSettings(settings: QuickShellSettings): Promise { @@ -639,22 +650,15 @@ export class QuickShellStorage { } return this.withWriteLock(async () => { - await this.flushRecentWrites(); + await this.flushRecentWritesUnlocked(); const data = await this.load(); data.settings = { ...settings }; - await this.save(data); + await this.saveUnlocked(data); }); } - /** - * Runs `operation` exclusively against other writers. - * Re-entrant so nested save/flush calls from an outer mutator do not deadlock. - */ + /** Runs `operation` exclusively against other writers, with bounded acquisition. */ private async withWriteLock(operation: () => Promise): Promise { - if (this.writeDepth > 0) { - return operation(); - } - let release!: () => void; const gate = new Promise((resolve) => { release = resolve; @@ -664,12 +668,23 @@ export class QuickShellStorage { () => gate, () => gate, ); - await previous; - this.writeDepth += 1; + + let timeoutHandle: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timeoutHandle = setTimeout(() => { + console.warn(`lock-timeout: write lock acquisition exceeded ${QuickShellStorage.WRITE_LOCK_TIMEOUT_MS}ms`); + reject(new Error(`Write lock acquisition timed out after ${QuickShellStorage.WRITE_LOCK_TIMEOUT_MS}ms.`)); + }, QuickShellStorage.WRITE_LOCK_TIMEOUT_MS); + }); + try { + await Promise.race([previous, timeout]); + } finally { + if (timeoutHandle) clearTimeout(timeoutHandle); + } + try { return await operation(); } finally { - this.writeDepth -= 1; release(); } } From 37de04bc2da9a6ed188cfb77b25744b01fc4d0f8 Mon Sep 17 00:00:00 2001 From: "qodo-code-review[bot]" <151058649+qodo-code-review[bot]@users.noreply.github.com> Date: Wed, 29 Jul 2026 02:01:48 +0000 Subject: [PATCH 2/2] fix: Document Raycast backup and locking --- docs/architecture/hosts.md | 2 ++ docs/architecture/parity-matrix.md | 1 + docs/architecture/persistence.md | 2 +- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/architecture/hosts.md b/docs/architecture/hosts.md index 3cd7fbba..a329f59b 100644 --- a/docs/architecture/hosts.md +++ b/docs/architecture/hosts.md @@ -64,6 +64,8 @@ Important libs: | `suggest-commands.ts` | shells out to Core Suggest CLI | | `settings.ts` | settings prefs | +Raycast `storage.ts` serializes mutations with a bounded write lock. Its reset-all flow owns a durable backup key, allowing restore after restart (with Undo available in-session). + Parity goals: multi-launch tabs (no `-w` on tab segments), similar settings keys, similar workspace shape, Suggest.exe pills with local heuristic fallback, multi-companion form + installed presets + folder-marker seed, trust/import contracts aligned with Core, copyable launch diagnostics, companions before terminals. Gaps: shared LocalAppData store, git worktree targets / dirty gate, full Core health (ports/process), Adaptive Card forms. ## Shared Core (desktop only) diff --git a/docs/architecture/parity-matrix.md b/docs/architecture/parity-matrix.md index c9abb0e5..ffa1cdca 100644 --- a/docs/architecture/parity-matrix.md +++ b/docs/architecture/parity-matrix.md @@ -41,6 +41,7 @@ Data stores: | Import / export JSON | Full | Via settings / shared file | Full (extension import-export) | Desktop shares one file; Raycast is separate blob; import always untrusted | | Layout undo / redo | Full | Partial (plugin + editor) | Full (storage history) | | | Section separators | Full | Partial | Full (Raycast-local `layoutEntries`) | Desktop shared layout file; Raycast blob parallel | +| Reset all / backup restore | Full | Partial | Full (durable Raycast backup) | Raycast backup is local to the extension and survives restart; mutations are serialized | --- diff --git a/docs/architecture/persistence.md b/docs/architecture/persistence.md index 7b1df0c7..5d8e046a 100644 --- a/docs/architecture/persistence.md +++ b/docs/architecture/persistence.md @@ -141,7 +141,7 @@ Also first-time import from `shortcuts.json.bak` or old product path `TerminalSh ## Raycast -`QuickShellStorage` mirrors layout/undo/recent debounce in Raycast storage API — **does not** share the desktop JSON path unless the user imports or exports. +`QuickShellStorage` mirrors layout/undo/recent debounce in Raycast storage API and serializes all mutations behind a bounded write lock — **does not** share the desktop JSON path unless the user imports or exports. Reset-all writes a durable, restart-safe backup snapshot under the Raycast-owned backup key; users can restore it after restart (or use in-session Undo). ## Key files