From 7511c6b2f4a18b391336a48f23b1814640ea4a04 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 8 Jan 2026 01:34:04 +0000 Subject: [PATCH] fix: clean up stale lock files during extension activation to prevent hangs on btrfs On btrfs and other copy-on-write filesystems, the proper-lockfile library's stale lock detection may not work correctly. This can cause the extension to hang indefinitely during activation after updates or crashes. This fix adds a cleanupStaleLocks() function that: - Scans for .lock directories/files created by proper-lockfile - Removes locks older than the stale timeout (31 seconds) - Handles both directory and file locks - Supports recursive scanning The cleanup is called early in extension activation, before any safeWriteJson operations, to ensure a clean state. Fixes #10436 --- src/extension.ts | 17 ++ src/utils/__tests__/safeWriteJson.test.ts | 218 +++++++++++++++++++++- src/utils/safeWriteJson.ts | 82 +++++++- 3 files changed, 315 insertions(+), 2 deletions(-) diff --git a/src/extension.ts b/src/extension.ts index 76f02af6de2..354517a9817 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -20,6 +20,7 @@ import { customToolRegistry } from "@roo-code/core" import "./utils/path" // Necessary to have access to String.prototype.toPosix. import { createOutputChannelLogger, createDualLogger } from "./utils/outputChannelLogger" import { initializeNetworkProxy } from "./utils/networkProxy" +import { cleanupStaleLocks } from "./utils/safeWriteJson" import { Package } from "./shared/package" import { formatLanguage } from "./shared/language" @@ -77,6 +78,22 @@ export async function activate(context: vscode.ExtensionContext) { // Set extension path for custom tool registry to find bundled esbuild customToolRegistry.setExtensionPath(context.extensionPath) + // Clean up stale lock files early, before any file operations that use safeWriteJson. + // This prevents hangs on btrfs and other filesystems where lock detection may not + // work correctly after extension updates or crashes. See issue #10436. + try { + const globalStoragePath = context.globalStorageUri.fsPath + const removedCount = await cleanupStaleLocks(globalStoragePath) + if (removedCount > 0) { + outputChannel.appendLine(`Cleaned up ${removedCount} stale lock file(s) from previous session`) + } + } catch (error) { + // Don't block activation on cleanup failures + outputChannel.appendLine( + `Warning: Failed to cleanup stale locks: ${error instanceof Error ? error.message : String(error)}`, + ) + } + // Migrate old settings to new await migrateSettings(context, outputChannel) diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index e18b86a02d8..7c8521d39dd 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -4,7 +4,7 @@ import { Writable } from "stream" import * as path from "path" import * as os from "os" -import { safeWriteJson } from "../safeWriteJson" +import { safeWriteJson, cleanupStaleLocks, LOCK_STALE_MS } from "../safeWriteJson" const originalFsPromisesRename = actualFsPromises.rename const originalFsPromisesUnlink = actualFsPromises.unlink @@ -478,3 +478,219 @@ describe("safeWriteJson", () => { consoleErrorSpy.mockRestore() }) }) + +describe("cleanupStaleLocks", () => { + let tempDir: string + + beforeEach(async () => { + // Create a temporary directory for each test + tempDir = await actualFsPromises.mkdtemp(path.join(os.tmpdir(), "cleanupStaleLocks-test-")) + }) + + afterEach(async () => { + // Clean up the temporary directory after each test + await actualFsPromises.rm(tempDir, { recursive: true, force: true }) + }) + + test("should return 0 when directory does not exist", async () => { + const nonExistentPath = path.join(tempDir, "non-existent") + const result = await cleanupStaleLocks(nonExistentPath) + expect(result).toBe(0) + }) + + test("should return 0 when directory is empty", async () => { + const result = await cleanupStaleLocks(tempDir) + expect(result).toBe(0) + }) + + test("should return 0 when no lock files exist", async () => { + // Create some regular files (not locks) + await actualFsPromises.writeFile(path.join(tempDir, "file1.json"), "{}") + await actualFsPromises.writeFile(path.join(tempDir, "file2.txt"), "test") + + const result = await cleanupStaleLocks(tempDir) + expect(result).toBe(0) + + // Verify regular files still exist + await expect(actualFsPromises.access(path.join(tempDir, "file1.json"))).resolves.toBeUndefined() + await expect(actualFsPromises.access(path.join(tempDir, "file2.txt"))).resolves.toBeUndefined() + }) + + test("should remove stale .lock directory", async () => { + // Create a stale lock directory + const lockPath = path.join(tempDir, "file.json.lock") + await actualFsPromises.mkdir(lockPath) + + // Set mtime to be older than LOCK_STALE_MS + const staleTime = new Date(Date.now() - LOCK_STALE_MS - 5000) + await actualFsPromises.utimes(lockPath, staleTime, staleTime) + + const result = await cleanupStaleLocks(tempDir) + expect(result).toBe(1) + + // Verify lock directory was removed + await expect(actualFsPromises.access(lockPath)).rejects.toThrow() + }) + + test("should remove stale .lock file", async () => { + // Create a stale lock file + const lockPath = path.join(tempDir, "file.json.lock") + await actualFsPromises.writeFile(lockPath, "") + + // Set mtime to be older than LOCK_STALE_MS + const staleTime = new Date(Date.now() - LOCK_STALE_MS - 5000) + await actualFsPromises.utimes(lockPath, staleTime, staleTime) + + const result = await cleanupStaleLocks(tempDir) + expect(result).toBe(1) + + // Verify lock file was removed + await expect(actualFsPromises.access(lockPath)).rejects.toThrow() + }) + + test("should NOT remove recent .lock directory", async () => { + // Create a recent lock directory (not stale) + const lockPath = path.join(tempDir, "file.json.lock") + await actualFsPromises.mkdir(lockPath) + + // mtime will be recent (now) + + const result = await cleanupStaleLocks(tempDir) + expect(result).toBe(0) + + // Verify lock directory still exists + await expect(actualFsPromises.access(lockPath)).resolves.toBeUndefined() + }) + + test("should remove multiple stale locks", async () => { + // Create multiple stale lock directories + const lock1 = path.join(tempDir, "file1.json.lock") + const lock2 = path.join(tempDir, "file2.json.lock") + const lock3 = path.join(tempDir, "settings.lock") + + await actualFsPromises.mkdir(lock1) + await actualFsPromises.mkdir(lock2) + await actualFsPromises.writeFile(lock3, "") + + // Set mtime to be older than LOCK_STALE_MS + const staleTime = new Date(Date.now() - LOCK_STALE_MS - 5000) + await actualFsPromises.utimes(lock1, staleTime, staleTime) + await actualFsPromises.utimes(lock2, staleTime, staleTime) + await actualFsPromises.utimes(lock3, staleTime, staleTime) + + const result = await cleanupStaleLocks(tempDir) + expect(result).toBe(3) + + // Verify all locks were removed + await expect(actualFsPromises.access(lock1)).rejects.toThrow() + await expect(actualFsPromises.access(lock2)).rejects.toThrow() + await expect(actualFsPromises.access(lock3)).rejects.toThrow() + }) + + test("should recursively clean subdirectories by default", async () => { + // Create nested directory structure with locks + const subDir = path.join(tempDir, "subdir") + await actualFsPromises.mkdir(subDir) + + const lock1 = path.join(tempDir, "file1.json.lock") + const lock2 = path.join(subDir, "file2.json.lock") + + await actualFsPromises.mkdir(lock1) + await actualFsPromises.mkdir(lock2) + + // Set mtime to be older than LOCK_STALE_MS + const staleTime = new Date(Date.now() - LOCK_STALE_MS - 5000) + await actualFsPromises.utimes(lock1, staleTime, staleTime) + await actualFsPromises.utimes(lock2, staleTime, staleTime) + + const result = await cleanupStaleLocks(tempDir, { recursive: true }) + expect(result).toBe(2) + + // Verify both locks were removed + await expect(actualFsPromises.access(lock1)).rejects.toThrow() + await expect(actualFsPromises.access(lock2)).rejects.toThrow() + + // Verify subdir still exists (only locks removed) + await expect(actualFsPromises.access(subDir)).resolves.toBeUndefined() + }) + + test("should not recursively clean when recursive is false", async () => { + // Create nested directory structure with locks + const subDir = path.join(tempDir, "subdir") + await actualFsPromises.mkdir(subDir) + + const lock1 = path.join(tempDir, "file1.json.lock") + const lock2 = path.join(subDir, "file2.json.lock") + + await actualFsPromises.mkdir(lock1) + await actualFsPromises.mkdir(lock2) + + // Set mtime to be older than LOCK_STALE_MS + const staleTime = new Date(Date.now() - LOCK_STALE_MS - 5000) + await actualFsPromises.utimes(lock1, staleTime, staleTime) + await actualFsPromises.utimes(lock2, staleTime, staleTime) + + const result = await cleanupStaleLocks(tempDir, { recursive: false }) + expect(result).toBe(1) + + // Verify only top-level lock was removed + await expect(actualFsPromises.access(lock1)).rejects.toThrow() + + // Lock in subdir should still exist + await expect(actualFsPromises.access(lock2)).resolves.toBeUndefined() + }) + + test("should use custom staleDurationMs when provided", async () => { + // Create a lock directory + const lockPath = path.join(tempDir, "file.json.lock") + await actualFsPromises.mkdir(lockPath) + + // Set mtime to 10 seconds ago + const tenSecondsAgo = new Date(Date.now() - 10000) + await actualFsPromises.utimes(lockPath, tenSecondsAgo, tenSecondsAgo) + + // Should not remove with default stale duration (31s) + const result1 = await cleanupStaleLocks(tempDir) + expect(result1).toBe(0) + + // Should remove with custom stale duration (5s) + const result2 = await cleanupStaleLocks(tempDir, { staleDurationMs: 5000 }) + expect(result2).toBe(1) + + // Verify lock was removed + await expect(actualFsPromises.access(lockPath)).rejects.toThrow() + }) + + test("should handle errors gracefully and continue", async () => { + // Create a stale lock directory + const lockPath = path.join(tempDir, "file.json.lock") + await actualFsPromises.mkdir(lockPath) + + // Set mtime to be older than LOCK_STALE_MS + const staleTime = new Date(Date.now() - LOCK_STALE_MS - 5000) + await actualFsPromises.utimes(lockPath, staleTime, staleTime) + + // Suppress console output during this test + const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + + // Mock rm to fail for this specific lock + const rmSpy = vi.spyOn(actualFsPromises, "rm").mockRejectedValueOnce(new Error("Permission denied")) + + const result = await cleanupStaleLocks(tempDir) + + // Should return 0 because removal failed + expect(result).toBe(0) + + // Verify console.warn was called + expect(consoleWarnSpy).toHaveBeenCalled() + + consoleWarnSpy.mockRestore() + consoleLogSpy.mockRestore() + rmSpy.mockRestore() + }) + + test("should export LOCK_STALE_MS constant", () => { + expect(LOCK_STALE_MS).toBe(31000) + }) +}) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 719bbd72167..f52d80f75f2 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -5,6 +5,12 @@ import * as lockfile from "proper-lockfile" import Disassembler from "stream-json/Disassembler" import Stringer from "stream-json/Stringer" +/** + * The stale timeout for locks in milliseconds (must match the value in safeWriteJson). + * Locks older than this are considered abandoned and can be safely removed. + */ +const LOCK_STALE_MS = 31000 + /** * Safely writes JSON data to a file. * - Creates parent directories if they don't exist @@ -232,4 +238,78 @@ async function _streamDataToFile(targetPath: string, data: any): Promise { }) } -export { safeWriteJson } +/** + * Cleans up stale lock files/directories from a directory. + * + * The `proper-lockfile` library creates `.lock` directories for advisory locking. + * On some filesystems (particularly btrfs with Copy-on-Write), these locks may not + * be properly detected as stale, causing hangs during extension activation after + * updates or crashes. + * + * This function scans a directory for `.lock` entries and removes any that are + * older than the stale timeout, ensuring clean startup. + * + * @param directoryPath - The directory to scan for stale locks + * @param options - Optional configuration + * @param options.recursive - If true, scan subdirectories as well (default: true) + * @param options.staleDurationMs - Custom stale duration in ms (default: LOCK_STALE_MS) + * @returns Promise - Number of stale locks removed + */ +async function cleanupStaleLocks( + directoryPath: string, + options: { recursive?: boolean; staleDurationMs?: number } = {}, +): Promise { + const { recursive = true, staleDurationMs = LOCK_STALE_MS } = options + let removedCount = 0 + + try { + // Check if directory exists + const dirStat = await fs.stat(directoryPath).catch(() => null) + if (!dirStat || !dirStat.isDirectory()) { + return 0 + } + + const entries = await fs.readdir(directoryPath, { withFileTypes: true }) + const now = Date.now() + + for (const entry of entries) { + const entryPath = path.join(directoryPath, entry.name) + + // Check for .lock files/directories created by proper-lockfile + if (entry.name.endsWith(".lock")) { + try { + const stat = await fs.stat(entryPath) + const age = now - stat.mtimeMs + + // Remove if older than stale duration + // Use a slightly larger threshold to account for filesystem timing quirks + if (age > staleDurationMs) { + if (stat.isDirectory()) { + await fs.rm(entryPath, { recursive: true, force: true }) + } else { + await fs.unlink(entryPath) + } + removedCount++ + console.log( + `[cleanupStaleLocks] Removed stale lock: ${entryPath} (age: ${Math.round(age / 1000)}s)`, + ) + } + } catch (err) { + // Ignore errors for individual lock files - they may have been + // cleaned up by another process or the original operation completed + console.warn(`[cleanupStaleLocks] Could not process ${entryPath}:`, err) + } + } else if (recursive && entry.isDirectory()) { + // Recursively clean subdirectories (but not .lock directories themselves) + removedCount += await cleanupStaleLocks(entryPath, options) + } + } + } catch (err) { + // Log but don't throw - cleanup failures shouldn't block extension activation + console.error(`[cleanupStaleLocks] Error scanning ${directoryPath}:`, err) + } + + return removedCount +} + +export { safeWriteJson, cleanupStaleLocks, LOCK_STALE_MS }