From 2d6f655e8232e3595e234211fd6ff23605d84cc6 Mon Sep 17 00:00:00 2001 From: "Leslie P. Polzer" Date: Mon, 8 Dec 2025 11:53:23 +0800 Subject: [PATCH 1/7] Fix stale lockfile cleanup for function extension builds When a function build process crashes or is killed, the `.build-lock` directory can be left behind. Previously, this caused subsequent builds to hang for an extended period (with 20 retries using exponential backoff) before failing, making development frustrating. This change: - Adds proactive stale lock detection before attempting to acquire a lock - Uses `lockfile.check()` to verify if a lock is actively held - Removes orphaned lock directories from crashed builds automatically - Reduces retries from 20 to 3 with shorter timeouts (faster failure) - Sets explicit 10-second stale threshold for lock files - Improves error message to show the actual lockfile path The fix handles three scenarios: 1. Stale lock exists (no active process) - auto-removes and proceeds 2. Active lock held by another process - waits normally 3. Corrupted lock (check fails) - attempts cleanup and proceeds --- .../src/cli/services/build/extension.test.ts | 96 ++++++++++++++++++- .../app/src/cli/services/build/extension.ts | 48 +++++++++- 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/packages/app/src/cli/services/build/extension.test.ts b/packages/app/src/cli/services/build/extension.test.ts index 4889dbd803c..6fb322b4968 100644 --- a/packages/app/src/cli/services/build/extension.test.ts +++ b/packages/app/src/cli/services/build/extension.test.ts @@ -7,7 +7,7 @@ import {beforeEach, describe, expect, test, vi} from 'vitest' import {exec} from '@shopify/cli-kit/node/system' import lockfile from 'proper-lockfile' import {AbortError} from '@shopify/cli-kit/node/error' -import {fileExistsSync} from '@shopify/cli-kit/node/fs' +import {fileExistsSync, fileExists, rmdir} from '@shopify/cli-kit/node/fs' vi.mock('@shopify/cli-kit/node/system') vi.mock('../function/build.js') @@ -294,4 +294,98 @@ describe('buildFunctionExtension', () => { expect(releaseLock).toHaveBeenCalled() expect(runWasmOpt).toHaveBeenCalled() }) + + test('cleans up stale lock before acquiring new lock', async () => { + // Given + vi.mocked(fileExists).mockResolvedValue(true) // Lock file exists + vi.mocked(lockfile.check).mockResolvedValue(false) // Lock is stale (not actively held) + vi.mocked(rmdir).mockResolvedValue() + + // When + await expect( + buildFunctionExtension(extension, { + stdout, + stderr, + signal, + app, + environment: 'production', + }), + ).resolves.toBeUndefined() + + // Then + expect(lockfile.check).toHaveBeenCalled() + expect(rmdir).toHaveBeenCalledWith(expect.stringContaining('.build-lock'), {recursive: true}) + expect(lockfile.lock).toHaveBeenCalled() + expect(releaseLock).toHaveBeenCalled() + }) + + test('does not clean up lock that is actively held', async () => { + // Given + vi.mocked(fileExists).mockResolvedValue(true) // Lock file exists + vi.mocked(lockfile.check).mockResolvedValue(true) // Lock is active (held by another process) + vi.mocked(rmdir).mockResolvedValue() + + // When + await expect( + buildFunctionExtension(extension, { + stdout, + stderr, + signal, + app, + environment: 'production', + }), + ).resolves.toBeUndefined() + + // Then + expect(lockfile.check).toHaveBeenCalled() + expect(rmdir).not.toHaveBeenCalled() // Should not remove active lock + expect(lockfile.lock).toHaveBeenCalled() + expect(releaseLock).toHaveBeenCalled() + }) + + test('cleans up corrupted lock when check fails', async () => { + // Given + vi.mocked(fileExists).mockResolvedValue(true) // Lock file exists + vi.mocked(lockfile.check).mockRejectedValue(new Error('ENOENT or corrupted lock')) + vi.mocked(rmdir).mockResolvedValue() + + // When + await expect( + buildFunctionExtension(extension, { + stdout, + stderr, + signal, + app, + environment: 'production', + }), + ).resolves.toBeUndefined() + + // Then + expect(lockfile.check).toHaveBeenCalled() + expect(rmdir).toHaveBeenCalledWith(expect.stringContaining('.build-lock'), {recursive: true}) + expect(lockfile.lock).toHaveBeenCalled() + expect(releaseLock).toHaveBeenCalled() + }) + + test('proceeds normally when no stale lock exists', async () => { + // Given + vi.mocked(fileExists).mockResolvedValue(false) // No lock file exists + + // When + await expect( + buildFunctionExtension(extension, { + stdout, + stderr, + signal, + app, + environment: 'production', + }), + ).resolves.toBeUndefined() + + // Then + expect(lockfile.check).not.toHaveBeenCalled() // Should not check if file doesn't exist + expect(rmdir).not.toHaveBeenCalled() + expect(lockfile.lock).toHaveBeenCalled() + expect(releaseLock).toHaveBeenCalled() + }) }) diff --git a/packages/app/src/cli/services/build/extension.ts b/packages/app/src/cli/services/build/extension.ts index 0e992326a25..cc613df3011 100644 --- a/packages/app/src/cli/services/build/extension.ts +++ b/packages/app/src/cli/services/build/extension.ts @@ -10,7 +10,7 @@ import {AbortError, AbortSilentError} from '@shopify/cli-kit/node/error' import lockfile from 'proper-lockfile' import {dirname, joinPath} from '@shopify/cli-kit/node/path' import {outputDebug} from '@shopify/cli-kit/node/output' -import {readFile, touchFile, writeFile, fileExistsSync} from '@shopify/cli-kit/node/fs' +import {readFile, touchFile, writeFile, fileExistsSync, rmdir, fileExists} from '@shopify/cli-kit/node/fs' import {Writable} from 'stream' export interface ExtensionBuildOptions { @@ -126,6 +126,36 @@ export async function buildUIExtension(extension: ExtensionInstance, options: Ex type BuildFunctionExtensionOptions = ExtensionBuildOptions +// Duration in milliseconds after which a lock is considered stale (default: 10 seconds) +// This should be long enough to allow for slow builds but short enough to recover from crashes +const LOCK_STALE_MS = 10000 + +/** + * Removes a stale lockfile if it exists and is not actively held. + * This handles cases where a previous build process crashed without releasing the lock. + */ +async function cleanupStaleLock(lockfilePath: string): Promise { + if (await fileExists(lockfilePath)) { + try { + // Check if the lock is stale (no active process holding it) + const isLocked = await lockfile.check(lockfilePath, {stale: LOCK_STALE_MS, lockfilePath}) + if (!isLocked) { + // Lock exists but is stale - remove it + outputDebug(`Removing stale build lock: ${lockfilePath}`) + await rmdir(lockfilePath, {recursive: true}) + } + } catch (error) { + // If check fails, try to remove the lock anyway - it's likely corrupted + outputDebug(`Failed to check lock status, attempting cleanup: ${lockfilePath}`) + try { + await rmdir(lockfilePath, {recursive: true}) + } catch { + // Ignore cleanup errors - the lock acquisition will fail with a clear message if needed + } + } + } +} + /** * Builds a function extension * @param extension - The function extension to build. @@ -136,15 +166,27 @@ export async function buildFunctionExtension( options: BuildFunctionExtensionOptions, ): Promise { const lockfilePath = joinPath(extension.directory, '.build-lock') + + // Clean up any stale locks from crashed builds before attempting to acquire + await cleanupStaleLock(lockfilePath) + let releaseLock try { - releaseLock = await lockfile.lock(extension.directory, {retries: 20, lockfilePath}) + releaseLock = await lockfile.lock(extension.directory, { + retries: { + retries: 3, + minTimeout: 100, + maxTimeout: 1000, + }, + stale: LOCK_STALE_MS, + lockfilePath, + }) // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (error: any) { outputDebug(`Failed to acquire function build lock: ${error.message}`) throw new AbortError('Failed to build function.', 'This is likely due to another in-progress build.', [ 'Ensure there are no other function builds in-progress.', - 'Delete the `.build-lock` file in your function directory.', + `If no other build is running, delete the \`${lockfilePath}\` directory and try again.`, ]) } From 6ab528020a64ecc36072c56131d1722ec60665e9 Mon Sep 17 00:00:00 2001 From: "Leslie P. Polzer" Date: Mon, 8 Dec 2025 12:09:45 +0800 Subject: [PATCH 2/7] Add signal handlers to release lock on SIGINT/SIGTERM/SIGHUP This prevents stale lockfiles from being left behind when the build process is interrupted by a signal (Ctrl+C, kill, terminal close). The signal handlers: - Register after the lock is acquired - Release the lock before re-emitting the signal to allow normal termination - Are cleaned up after normal build completion to avoid memory leaks Also adds a test to verify signal handlers are properly registered and removed during the build lifecycle. --- .../src/cli/services/build/extension.test.ts | 32 ++++++++++++++ .../app/src/cli/services/build/extension.ts | 43 ++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/build/extension.test.ts b/packages/app/src/cli/services/build/extension.test.ts index 6fb322b4968..65a3b0f4243 100644 --- a/packages/app/src/cli/services/build/extension.test.ts +++ b/packages/app/src/cli/services/build/extension.test.ts @@ -388,4 +388,36 @@ describe('buildFunctionExtension', () => { expect(lockfile.lock).toHaveBeenCalled() expect(releaseLock).toHaveBeenCalled() }) + + test('registers signal handlers after acquiring lock and removes them after release', async () => { + // Given + const processOnSpy = vi.spyOn(process, 'on') + const processRemoveListenerSpy = vi.spyOn(process, 'removeListener') + + // When + await expect( + buildFunctionExtension(extension, { + stdout, + stderr, + signal, + app, + environment: 'production', + }), + ).resolves.toBeUndefined() + + // Then + // Verify signal handlers were registered for SIGINT, SIGTERM, SIGHUP + expect(processOnSpy).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(processOnSpy).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + expect(processOnSpy).toHaveBeenCalledWith('SIGHUP', expect.any(Function)) + + // Verify signal handlers were removed after build completed + expect(processRemoveListenerSpy).toHaveBeenCalledWith('SIGINT', expect.any(Function)) + expect(processRemoveListenerSpy).toHaveBeenCalledWith('SIGTERM', expect.any(Function)) + expect(processRemoveListenerSpy).toHaveBeenCalledWith('SIGHUP', expect.any(Function)) + + // Cleanup spies + processOnSpy.mockRestore() + processRemoveListenerSpy.mockRestore() + }) }) diff --git a/packages/app/src/cli/services/build/extension.ts b/packages/app/src/cli/services/build/extension.ts index cc613df3011..3c56a7551fc 100644 --- a/packages/app/src/cli/services/build/extension.ts +++ b/packages/app/src/cli/services/build/extension.ts @@ -130,6 +130,42 @@ type BuildFunctionExtensionOptions = ExtensionBuildOptions // This should be long enough to allow for slow builds but short enough to recover from crashes const LOCK_STALE_MS = 10000 +// Signals that should trigger lock cleanup +const CLEANUP_SIGNALS: NodeJS.Signals[] = ['SIGINT', 'SIGTERM', 'SIGHUP'] + +/** + * Registers signal handlers to release the lock on abnormal termination. + * Returns a cleanup function to remove the handlers when the lock is released normally. + */ +function registerLockCleanupHandlers(releaseLock: () => Promise): () => void { + const handlers: Map = new Map() + + for (const signal of CLEANUP_SIGNALS) { + const handler: NodeJS.SignalsListener = () => { + outputDebug(`Received ${signal}, releasing build lock...`) + // Use sync-style cleanup since we're in a signal handler + // The lock release is async but we need to at least initiate it + releaseLock() + .catch((err) => outputDebug(`Failed to release lock on ${signal}: ${err}`)) + .finally(() => { + // Re-emit the signal after cleanup so the process can terminate normally + // Remove our handler first to avoid infinite loop + process.removeListener(signal, handler) + process.kill(process.pid, signal) + }) + } + handlers.set(signal, handler) + process.on(signal, handler) + } + + // Return cleanup function to remove handlers + return () => { + for (const [signal, handler] of handlers) { + process.removeListener(signal, handler) + } + } +} + /** * Removes a stale lockfile if it exists and is not actively held. * This handles cases where a previous build process crashed without releasing the lock. @@ -170,7 +206,8 @@ export async function buildFunctionExtension( // Clean up any stale locks from crashed builds before attempting to acquire await cleanupStaleLock(lockfilePath) - let releaseLock + let releaseLock: () => Promise + let removeSignalHandlers: () => void try { releaseLock = await lockfile.lock(extension.directory, { retries: { @@ -181,6 +218,8 @@ export async function buildFunctionExtension( stale: LOCK_STALE_MS, lockfilePath, }) + // Register signal handlers to release lock on SIGINT, SIGTERM, SIGHUP + removeSignalHandlers = registerLockCleanupHandlers(releaseLock) // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (error: any) { outputDebug(`Failed to acquire function build lock: ${error.message}`) @@ -231,6 +270,8 @@ export async function buildFunctionExtension( newError.errors = error.errors throw newError } finally { + // Remove signal handlers before releasing lock (normal termination path) + removeSignalHandlers() await releaseLock() } } From 8b744d12052cf831992b7e0a4ffc348743ab9420 Mon Sep 17 00:00:00 2001 From: "Leslie P. Polzer" Date: Mon, 8 Dec 2025 12:18:19 +0800 Subject: [PATCH 3/7] Trigger CI From cafaee13e88013cac6d81ed914f87f9989770a2d Mon Sep 17 00:00:00 2001 From: "Leslie P. Polzer" Date: Mon, 8 Dec 2025 12:23:05 +0800 Subject: [PATCH 4/7] Fix rmdir options: use force instead of recursive The @shopify/cli-kit rmdir function uses the 'del' library which handles recursion automatically. The RmDirOptions interface only supports 'force', not 'recursive'. --- packages/app/src/cli/services/build/extension.test.ts | 4 ++-- packages/app/src/cli/services/build/extension.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/app/src/cli/services/build/extension.test.ts b/packages/app/src/cli/services/build/extension.test.ts index 65a3b0f4243..4bc34c1c034 100644 --- a/packages/app/src/cli/services/build/extension.test.ts +++ b/packages/app/src/cli/services/build/extension.test.ts @@ -314,7 +314,7 @@ describe('buildFunctionExtension', () => { // Then expect(lockfile.check).toHaveBeenCalled() - expect(rmdir).toHaveBeenCalledWith(expect.stringContaining('.build-lock'), {recursive: true}) + expect(rmdir).toHaveBeenCalledWith(expect.stringContaining('.build-lock'), {force: true}) expect(lockfile.lock).toHaveBeenCalled() expect(releaseLock).toHaveBeenCalled() }) @@ -362,7 +362,7 @@ describe('buildFunctionExtension', () => { // Then expect(lockfile.check).toHaveBeenCalled() - expect(rmdir).toHaveBeenCalledWith(expect.stringContaining('.build-lock'), {recursive: true}) + expect(rmdir).toHaveBeenCalledWith(expect.stringContaining('.build-lock'), {force: true}) expect(lockfile.lock).toHaveBeenCalled() expect(releaseLock).toHaveBeenCalled() }) diff --git a/packages/app/src/cli/services/build/extension.ts b/packages/app/src/cli/services/build/extension.ts index 3c56a7551fc..4c2fc0a92a2 100644 --- a/packages/app/src/cli/services/build/extension.ts +++ b/packages/app/src/cli/services/build/extension.ts @@ -178,13 +178,13 @@ async function cleanupStaleLock(lockfilePath: string): Promise { if (!isLocked) { // Lock exists but is stale - remove it outputDebug(`Removing stale build lock: ${lockfilePath}`) - await rmdir(lockfilePath, {recursive: true}) + await rmdir(lockfilePath, {force: true}) } } catch (error) { // If check fails, try to remove the lock anyway - it's likely corrupted outputDebug(`Failed to check lock status, attempting cleanup: ${lockfilePath}`) try { - await rmdir(lockfilePath, {recursive: true}) + await rmdir(lockfilePath, {force: true}) } catch { // Ignore cleanup errors - the lock acquisition will fail with a clear message if needed } From a4a56be5bd090abf8b603c7631465254e1c46b42 Mon Sep 17 00:00:00 2001 From: "Leslie P. Polzer" Date: Mon, 8 Dec 2025 12:28:20 +0800 Subject: [PATCH 5/7] Fix lint errors: generic constructor and no-catch-all --- packages/app/src/cli/services/build/extension.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/app/src/cli/services/build/extension.ts b/packages/app/src/cli/services/build/extension.ts index 4c2fc0a92a2..73f8eee8f41 100644 --- a/packages/app/src/cli/services/build/extension.ts +++ b/packages/app/src/cli/services/build/extension.ts @@ -138,7 +138,7 @@ const CLEANUP_SIGNALS: NodeJS.Signals[] = ['SIGINT', 'SIGTERM', 'SIGHUP'] * Returns a cleanup function to remove the handlers when the lock is released normally. */ function registerLockCleanupHandlers(releaseLock: () => Promise): () => void { - const handlers: Map = new Map() + const handlers = new Map() for (const signal of CLEANUP_SIGNALS) { const handler: NodeJS.SignalsListener = () => { @@ -180,11 +180,13 @@ async function cleanupStaleLock(lockfilePath: string): Promise { outputDebug(`Removing stale build lock: ${lockfilePath}`) await rmdir(lockfilePath, {force: true}) } + // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { - // If check fails, try to remove the lock anyway - it's likely corrupted + // If check fails (e.g., ENOENT, corrupted lock), try to remove the lock anyway outputDebug(`Failed to check lock status, attempting cleanup: ${lockfilePath}`) try { await rmdir(lockfilePath, {force: true}) + // eslint-disable-next-line no-catch-all/no-catch-all } catch { // Ignore cleanup errors - the lock acquisition will fail with a clear message if needed } From 346343669512326ffa26171e5a42c09c3b138953 Mon Sep 17 00:00:00 2001 From: "Leslie P. Polzer" Date: Mon, 8 Dec 2025 12:32:47 +0800 Subject: [PATCH 6/7] Fix lint: move inline comments above code --- .../src/cli/services/build/extension.test.ts | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/packages/app/src/cli/services/build/extension.test.ts b/packages/app/src/cli/services/build/extension.test.ts index 4bc34c1c034..7a316c2b3cf 100644 --- a/packages/app/src/cli/services/build/extension.test.ts +++ b/packages/app/src/cli/services/build/extension.test.ts @@ -297,8 +297,10 @@ describe('buildFunctionExtension', () => { test('cleans up stale lock before acquiring new lock', async () => { // Given - vi.mocked(fileExists).mockResolvedValue(true) // Lock file exists - vi.mocked(lockfile.check).mockResolvedValue(false) // Lock is stale (not actively held) + // Lock file exists + vi.mocked(fileExists).mockResolvedValue(true) + // Lock is stale (not actively held) + vi.mocked(lockfile.check).mockResolvedValue(false) vi.mocked(rmdir).mockResolvedValue() // When @@ -321,8 +323,10 @@ describe('buildFunctionExtension', () => { test('does not clean up lock that is actively held', async () => { // Given - vi.mocked(fileExists).mockResolvedValue(true) // Lock file exists - vi.mocked(lockfile.check).mockResolvedValue(true) // Lock is active (held by another process) + // Lock file exists + vi.mocked(fileExists).mockResolvedValue(true) + // Lock is active (held by another process) + vi.mocked(lockfile.check).mockResolvedValue(true) vi.mocked(rmdir).mockResolvedValue() // When @@ -338,14 +342,16 @@ describe('buildFunctionExtension', () => { // Then expect(lockfile.check).toHaveBeenCalled() - expect(rmdir).not.toHaveBeenCalled() // Should not remove active lock + // Should not remove active lock + expect(rmdir).not.toHaveBeenCalled() expect(lockfile.lock).toHaveBeenCalled() expect(releaseLock).toHaveBeenCalled() }) test('cleans up corrupted lock when check fails', async () => { // Given - vi.mocked(fileExists).mockResolvedValue(true) // Lock file exists + // Lock file exists + vi.mocked(fileExists).mockResolvedValue(true) vi.mocked(lockfile.check).mockRejectedValue(new Error('ENOENT or corrupted lock')) vi.mocked(rmdir).mockResolvedValue() @@ -369,7 +375,8 @@ describe('buildFunctionExtension', () => { test('proceeds normally when no stale lock exists', async () => { // Given - vi.mocked(fileExists).mockResolvedValue(false) // No lock file exists + // No lock file exists + vi.mocked(fileExists).mockResolvedValue(false) // When await expect( @@ -383,7 +390,8 @@ describe('buildFunctionExtension', () => { ).resolves.toBeUndefined() // Then - expect(lockfile.check).not.toHaveBeenCalled() // Should not check if file doesn't exist + // Should not check if file doesn't exist + expect(lockfile.check).not.toHaveBeenCalled() expect(rmdir).not.toHaveBeenCalled() expect(lockfile.lock).toHaveBeenCalled() expect(releaseLock).toHaveBeenCalled() From c80cbafb0fca8c93d4a391f220ce553b8d5e6f00 Mon Sep 17 00:00:00 2001 From: "Leslie P. Polzer" Date: Mon, 8 Dec 2025 12:48:31 +0800 Subject: [PATCH 7/7] Add changeset for stale lockfile fix --- .changeset/fix-stale-function-build-lock.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fix-stale-function-build-lock.md diff --git a/.changeset/fix-stale-function-build-lock.md b/.changeset/fix-stale-function-build-lock.md new file mode 100644 index 00000000000..eed046350a1 --- /dev/null +++ b/.changeset/fix-stale-function-build-lock.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Fixed stale lockfile handling for function extension builds. When a build process crashes or is interrupted (SIGINT/SIGTERM/SIGHUP), the `.build-lock` directory could be left behind causing subsequent builds to hang. The fix adds proactive stale lock detection on startup and signal handlers to release locks on abnormal termination.