fix(functions): clean up generated artifacts when kit install fails by wandamora · Pull Request #11082 · firebase/firebase-tools

wandamora · 2026-09-11T22:41:48Z

Description

When firebase functions:kits:install fails partway through execution (e.g. npm install failure, TypeScript compile error, parameter resolution cancellation/error, or build discovery failure), the command previously left behind partially generated artifacts on disk and, in some flows, a modified firebase.json referencing an uninstalled kit or instance. Users had to manually delete files inside function-kits/ and hand-edit firebase.json before retrying.

This PR makes function kit installation and instance addition transactional. If any step fails during execution, all artifacts created during that invocation are rolled back and firebase.json is restored to its pre-install state before rethrowing the original error.

Key Changes:

Cleanup Helpers (src/functions/kits/install.ts):
- safeRemove(targetPath): Safely removes files or directories, catching and logging any filesystem errors via logger.debug so cleanup failures never swallow or mask the root cause.
- cleanUpEmptyDir(dirPath): Recursively removes directory trees only if they exist and are empty, pruning orphaned parent directories (e.g., function-kits/<kitId> and function-kits/).
- revertFunctionsConfig({ config, originalFunctions }): Restores in-memory config.src.functions to a snapshot taken prior to mutation and, if changed, persists the reverted state back to firebase.json.
Transactional Error Handling across Install & Configure Paths:
- installKitOrInstance: Scaffolding, dependency installation, build, parameter prompt, and config mutations are wrapped in a try/catch. On failure, removes generated kit/config directories (while preserving user-provided directories when installed via --directory) and reverts firebase.json.
- addKitInstanceOrConfigureProject: Wraps adding a new instance or project environment. On failure, removes the created instance config directory or .env files and cleans up any empty parent kit directories.
- addInstanceToKit: Preserves existing config snapshot; rolls back the instance config directory, empty parent directories, and firebase.json on failure.
- scaffoldKit: Cleans up the target kit directory on unpack or scaffold failure before propagating the error.
Comprehensive Unit Testing (src/functions/kits/install.spec.ts):
- Tests rollback of firebase.json and directory structures on failed npm install.
- Tests cleanup on build discovery / parameter prompt failures.
- Tests rollback when configuring existing kits or adding additional instances.
- Verifies that external source directories specified via --directory are preserved while CLI-generated config directories are pruned.
- Asserts on the exact serialized disk state of firebase.json during write calls to ensure configuration rollbacks are written to disk.

Scenarios Tested

Unit tests via npm run mocha:fast / npm test.
Package kit install failure (npm install / build failure): verified function-kits/<kitId> and function-kits/ are pruned and firebase.json remains untouched.
Local kit install failure via --directory: verified user source code directory is preserved, generated config directory is deleted, and firebase.json remains untouched.
Adding a second instance to an existing kit: verified failed instance config directory is deleted and existing kit definition in firebase.json retains only the original instance.
Adding project configuration (.env.<projectId>) failure: verified newly created .env file is deleted without modifying existing project files.

Sample Commands

# Install a package kit (rolls back function-kits/ and firebase.json on failure)
firebase functions:kits:install @firebase/kit-template

# Install a local directory kit (cleans up config dirs on failure, preserves local source)
firebase functions:kits:install --directory ./my-kit

# Add instance to existing kit (cleans up only failed instance artifacts on failure)
firebase functions:kits:install --instance another-instance

gemini-code-assist

Code Review

This pull request introduces robust error handling and rollback mechanisms during function kit installation, configuration, and instance addition by adding cleanup utilities (safeRemove, cleanUpEmptyDir, and revertFunctionsConfig) and wrapping key operations in try-catch blocks. The review feedback correctly identifies a critical bug where pre-emptively mutating the in-memory configuration in the catch blocks of addInstanceToKit and addKitInstanceOrConfigureProject prevents revertFunctionsConfig from detecting changes and writing the reverted configuration back to firebase.json on disk. The reviewer provides actionable suggestions to remove these pre-emptive deletions and update the corresponding unit test assertions to verify the reverted configuration.

Remove premature in-memory instance deletions in addInstanceToKit and addKitInstanceOrConfigureProject catch blocks so revertFunctionsConfig detects the difference and writes the reverted config back to disk. Update unit tests to assert serialized content on disk rather than object references.

ajperel

I hope I left a suggestion that will make this simpler.

ajperel · 2026-09-12T02:04:45Z

+}
+
+/**
+ * Removes an empty directory if it exists and contains no files or subdirectories.


You could also note this suppresses any errors and logging to debug.

ajperel · 2026-09-12T02:06:15Z

+/**
+ * Safely removes a file or directory, suppressing any errors and logging to debug.
+ */
+export async function safeRemove(targetPath: string): Promise<void> {


This and cleanUpEmptyDir seem fairly generic and might be well served moving to fsutils.ts

ajperel · 2026-09-12T02:16:54Z

+/**
+ * Removes an empty directory if it exists and contains no files or subdirectories.
+ */
+export async function cleanUpEmptyDir(absDirPath: string): Promise<void> {


maybe an even clearer name would be removeDirectoryIfEmpty

ajperel · 2026-09-12T02:20:36Z

+    if (await fs.pathExists(absDirPath)) {
+      const stat = await fs.stat(absDirPath);
+      if (stat.isDirectory()) {
+        const files = await fs.readdir(absDirPath);


nit: I'd call this something more generic like entries since it could be a file or a directory.

ajperel · 2026-09-12T02:23:18Z

+ */
+export function revertFunctionsConfig(options: RevertFunctionsConfigOptions): void {
+  try {
+    const originalFunctionsJson = JSON.stringify(options.originalFunctions);


You should use deepEqual from utils.ts probably. Not that it should happen in this case but it will not care about say reordering things and can fail fast.

ajperel · 2026-09-12T02:24:17Z

+/**
+ * Reverts the functions configuration in firebase.json to its original state if it was modified.
+ */
+export function revertFunctionsConfig(options: RevertFunctionsConfigOptions): void {


This is a cool pattern we might want to expand elsewhere in future PRs and make a shared config thing. But I wouldn't do that in this PR.

ajperel · 2026-09-12T02:28:39Z

+    return { configDirPath, absConfigDirPath };
+  } catch (err: unknown) {
+    await safeRemove(absConfigDirPath);
+    await cleanUpEmptyDir(options.config.path(path.join(FUNCTION_KITS_DIR, options.kitId)));


Won't these directories never be empty because you have a source directory already if this is the addInstance case and this is a second instance? I think you just need to clean up configDir and firebase.json

ajperel · 2026-09-12T02:35:41Z

+  try {
+    await fs.remove(targetPath);
+  } catch (err: unknown) {
+    logger.debug(`Failed to remove path '${targetPath}': ${getErrMsg(err)}`);


This is OK in in our time crunch... but we should think through the UX a bit more. I wonder if on a failure like this we should at the end instead be like:

Warning: Kit install failed. Unable to remove partial install. To clean up this kit/instance:

Remove X

Edit Y

ajperel · 2026-09-12T03:35:26Z

+  let absSourcePathToPreserve: string | undefined;

-  await source.buildAndInstall(absSourcePath);
+  try {


You have a lot of cleanup logic spread all throughout this file and it's just getting more and more complicated. Why don't we take a simple approach. This file orchestrates everything right?

At the beginning of this you:

Snapshot firebase.json

Create an array like const createdPaths: string[] = [];

We can pass createdPaths in options to all the sub methods (required?) Anytime they create a new file or directory the path is added to createdPaths... one shared object.

Then we have only this try/catch look for cleanup. Any failure we catch here and run:

await Promise.all(this.createdPaths.map(safeRemove)); await cleanUpEmptyDir(this.options.config.path(FUNCTION_KITS_DIR)); revertFunctionsConfig({ config: this.options.config, originalFunctions: this.originalFunctions });

and then can re-throw the error.

Actually, shouldn't we really even only need to have one path to remove. It'll be either:

Entire Kit Directory (kit install)

Entire Config Directory (add instance)

Single .env file (add Env)

We just need to pipe that one path back here, though maybe still easiest using createdPaths?

Cleanup if failed install

0424ddf

gemini-code-assist Bot reviewed Sep 11, 2026

View reviewed changes

Comment thread src/functions/kits/install.ts

Comment thread src/functions/kits/install.ts

Comment thread src/functions/kits/install.spec.ts Outdated

wandamora requested a review from ajperel September 11, 2026 23:35

ajperel reviewed Sep 12, 2026

View reviewed changes

Uh oh!

Conversation

wandamora commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key Changes:

Scenarios Tested

Sample Commands

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajperel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wandamora commented Sep 11, 2026 •

edited

Loading