Skip to content

Fix: [for cherry-picking] Removed the shared reentrancy-depth bypass by #134

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.

Sign up for GitHub

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

Closed
qodo-code-review wants to merge 2 commits into from
+72 −54
Closed

Fix: [for cherry-picking] Removed the shared reentrancy-depth bypass by #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae83fd8
fix: 3 findings — Remove reentrant lock bypass; Bound write lock acquisi
qodo-code-review[bot] Jul 29, 2026
37de04b
fix: Document Raycast backup and locking
qodo-code-review[bot] Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 68 additions & 53 deletions QuickShell.Raycast/src/lib/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export type StorageAdapter = {
setItem: (key: string, value: string) => Promise<void>;
};

export type StorageTransferResult = {
type StorageTransferResult = {
success: true;
message: string;
};
Expand All @@ -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<void> = Promise.resolve();
private writeDepth = 0;
private static readonly WRITE_LOCK_TIMEOUT_MS = 5000;

constructor(
private readonly adapter: StorageAdapter,
Expand All @@ -75,7 +75,7 @@ export class QuickShellStorage {

async undo(): Promise<boolean> {
return this.withWriteLock(async () => {
await this.flushRecentWrites();
await this.flushRecentWritesUnlocked();
if (this.undoHistory.length === 0) {
return false;
}
Expand All @@ -97,7 +97,7 @@ export class QuickShellStorage {

async redo(): Promise<boolean> {
return this.withWriteLock(async () => {
await this.flushRecentWrites();
await this.flushRecentWritesUnlocked();
if (this.redoHistory.length === 0) {
return false;
}
Expand Down Expand Up @@ -128,10 +128,10 @@ export class QuickShellStorage {

async importJson(raw: string, mode: "merge" | "replace" = "merge"): Promise<ImportResult> {
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;
});
}
Expand All @@ -142,7 +142,7 @@ export class QuickShellStorage {
*/
async resetAll(): Promise<StorageTransferResult> {
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." };
Expand All @@ -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 {
Expand All @@ -166,7 +166,7 @@ export class QuickShellStorage {
/** Restores the durable reset-all backup into the live store. */
async restoreFromBackup(): Promise<StorageTransferResult> {
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." };
Expand All @@ -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.`,
Expand All @@ -193,15 +193,14 @@ export class QuickShellStorage {
return summarizeImportConflicts(raw, existing);
}

async save(
private async saveUnlocked(
data: StoredData,
options?: {
recordHistory?: boolean;
preserveSecurity?: boolean;
allowSubmittedSecurity?: boolean;
},
): Promise<void> {
return this.withWriteLock(async () => {
const recordHistory = options?.recordHistory ?? true;
const preserveSecurity = options?.preserveSecurity ?? true;
const allowSubmittedSecurity = options?.allowSubmittedSecurity ?? false;
Expand Down Expand Up @@ -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<void> {
return this.withWriteLock(() => this.saveUnlocked(data, options));
}

async getWorkspaces(): Promise<Workspace[]> {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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";
});
}
Expand All @@ -349,7 +358,7 @@ export class QuickShellStorage {

async upsertWorkspace(workspace: Workspace): Promise<Workspace> {
return this.withWriteLock(async () => {
await this.flushRecentWrites();
await this.flushRecentWritesUnlocked();
const data = await this.load();
const normalized = normalizeWorkspace({
...workspace,
Expand Down Expand Up @@ -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<void> {
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<Workspace> {
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) {
Expand All @@ -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;
});
}
Expand All @@ -446,7 +455,7 @@ export class QuickShellStorage {

async setBranchTarget(worktreeKey: string, branch: string): Promise<void> {
return this.withWriteLock(async () => {
await this.flushRecentWrites();
await this.flushRecentWritesUnlocked();
const data = await this.load();
const key = worktreeKey.trim().toLowerCase();
const value = branch.trim();
Expand All @@ -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<void> {
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)) {
Expand All @@ -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);
});
}

Expand All @@ -483,7 +492,7 @@ export class QuickShellStorage {

async insertSeparator(title?: string | null, beforeWorkspaceId?: string): Promise<LayoutEntry> {
return this.withWriteLock(async () => {
await this.flushRecentWrites();
await this.flushRecentWritesUnlocked();
const data = await this.load();
const separator: LayoutEntry = {
type: "separator",
Expand All @@ -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<void> {
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<void> {
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));
Expand All @@ -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<Workspace> {
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) {
Expand All @@ -554,15 +563,15 @@ export class QuickShellStorage {
workspace.pinOrder = null;
}

await this.save(data);
await this.saveUnlocked(data);
return { ...workspace };
});
}

/** Returns the moved workspace, or `null` when the move is a boundary no-op. */
async moveFavorite(workspaceId: string, direction: "up" | "down" | "top" | "bottom"): Promise<Workspace | null> {
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) {
Expand Down Expand Up @@ -601,7 +610,7 @@ export class QuickShellStorage {
item.pinOrder = orderIndex + 1;
});

await this.save(data);
await this.saveUnlocked(data);
return { ...favorites[targetIndex] };
});
}
Expand All @@ -619,8 +628,7 @@ export class QuickShellStorage {
});
}

async flushRecentWrites(): Promise<void> {
return this.withWriteLock(async () => {
private async flushRecentWritesUnlocked(): Promise<void> {
if (this.recentWriteTimer) {
clearTimeout(this.recentWriteTimer);
this.recentWriteTimer = null;
Expand All @@ -630,7 +638,10 @@ export class QuickShellStorage {
}
this.recentWriteDirty = false;
await this.persistCache({ recordHistory: false });
});
}

async flushRecentWrites(): Promise<void> {
return this.withWriteLock(() => this.flushRecentWritesUnlocked());
}

async updateSettings(settings: QuickShellSettings): Promise<void> {
Expand All @@ -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<T>(operation: () => Promise<T>): Promise<T> {
Comment on lines +660 to 661

@sourcery-ai sourcery-ai Bot Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Dropping re-entrancy on the write lock introduces a deadlock risk if future code accidentally calls locked APIs from inside a locked section.

The previous writeDepth logic allowed nested save/flush calls without blocking. Now, correctness depends on always using saveUnlocked/flushRecentWritesUnlocked inside withWriteLock. A future nested call to save or flushRecentWrites from within withWriteLock would queue behind the current lock and then wait on itself, causing a deadlock until timeout. Please either reintroduce a re-entrancy mechanism (e.g., per-instance depth/flag that bypasses the queue) or add runtime assertions that prevent calling the locked variants from within withWriteLock, so misuse fails fast.

Fix in Cursor

if (this.writeDepth > 0) {
return operation();
}

let release!: () => void;
const gate = new Promise<void>((resolve) => {
release = resolve;
Expand All @@ -664,12 +668,23 @@ export class QuickShellStorage {
() => gate,
() => gate,
);
await previous;
this.writeDepth += 1;

let timeoutHandle: ReturnType<typeof setTimeout> | undefined;
const timeout = new Promise<never>((_, 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();
}
}
Expand Down
Loading
Loading