fix: make storage, apphosting, and dataconnect emulator startup resilient - #10934
fix: make storage, apphosting, and dataconnect emulator startup resilient#10934christhompsongoogle wants to merge 4 commits into
Conversation
…ient ### Description Prevent missing configurations for Storage, App Hosting, and Data Connect emulators from crashing emulators:start and blocking the rest of the emulator suite: - Fall back to default open rules with a warning for Storage emulator when rules are unconfigured on non-demo projects. - Gracefully skip Data Connect emulator with a warning if no service configurations are present in firebase.json or startup fails. - Gracefully skip App Hosting emulator with a warning if package manager / start command auto-detection fails or startup fails. - Clean up failed instances in EmulatorRegistry when start() fails. Fixes b/546204399 ### Scenarios Tested - Unit tests in storage/rules/config.spec.ts for missing storage config, missing rules, and demo/non-demo projects. - Unit tests in controller.spec.ts for Data Connect shouldStart handling. - Unit tests in registry.spec.ts for failed emulator startup cleanup. - App Hosting serve unit tests. ### Sample Commands - firebase init emulators (select all emulators) - firebase emulators:start --project=ctfdc1
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents blocking startup errors for the entire emulator suite when configurations for Storage, App Hosting, or Data Connect emulators are missing or fail to start. Instead of throwing fatal errors, the emulators now log warnings and gracefully skip startup. The review feedback highlights a potential resource leak in the emulator registry if a port check fails after starting, and suggests extracting duplicated logging logic in the storage rules configuration to improve maintainability.
| } catch (err: unknown) { | ||
| this.clear(instance.getName()); | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
If instance.start() succeeds but the subsequent port check portUtils.waitForPortUsed fails (e.g., due to a timeout), the emulator process is left running in the background. Since this.clear(instance.getName()) is called in the catch block, the registry loses track of the instance, meaning a subsequent cleanShutdown() or stopAll() will not clean up the orphaned process.
To prevent resource leaks, attempt to stop the emulator instance in the catch block before clearing it from the registry.
} catch (err: unknown) {
try {
await instance.stop();
} catch (stopErr) {
// Ignore stop errors to avoid masking the original startup error
}
this.clear(instance.getName());
throw err;
}| } else { | ||
| storageLogger.logLabeled( | ||
| "WARN", | ||
| "storage", | ||
| "Did not find a Storage rules file specified in a firebase.json config file. The emulator will default to allowing all reads and writes. Learn more about this option: https://firebase.google.com/docs/emulator-suite/install_and_configure#security_rules_configuration.", | ||
| ); | ||
| } | ||
| throw new FirebaseError( | ||
| "Cannot start the Storage emulator without rules file specified in firebase.json: run 'firebase init' and set up your Storage configuration", | ||
| ); | ||
| return defaultStorageRules(); |
There was a problem hiding this comment.
The logic for logging the default rules warning/bullet is duplicated between the case where storageConfig is missing (lines 28-41) and where storageConfig.rules is missing (lines 48-60).
To adhere to the repository style guide on reducing nesting and improving maintainability, consider extracting this logging logic into a helper function.
Example:
function logDefaultRulesWarning(projectId: string, storageLogger: EmulatorLogger): void {
if (Constants.isDemoProject(projectId)) {
storageLogger.logLabeled(
"BULLET",
"storage",
`Detected demo project ID "${projectId}", using a default (open) rules configuration.`
);
} else {
storageLogger.logLabeled(
"WARN",
"storage",
"Did not find a Storage rules file specified in a firebase.json config file. The emulator will default to allowing all reads and writes. Learn more about this option: https://firebase.google.com/docs/emulator-suite/install_and_configure#security_rules_configuration."
);
}
}References
- Reduce nesting as much as possible and consider helper functions to encapsulate complex branching. (link)
|
/joe-review |
| EmulatorLogger.forEmulator(Emulators.DATACONNECT).logLabeled( | ||
| "ERROR", | ||
| "dataconnect", | ||
| `Failed to start Data Connect emulator: No valid Data Connect configuration detected in firebase.json`, |
There was a problem hiding this comment.
🔴 [Error Remediation] Missing remediation instructions
Rationale: CLI error messages should provide explicit instructions on how to resolve the issue. The warning for missing Data Connect configuration should advise running firebase init dataconnect (similar to how Hosting emulator advises running firebase init hosting).
Suggested Fix:
EmulatorLogger.forEmulator(Emulators.DATACONNECT).logLabeled(
"ERROR",
"dataconnect",
`Failed to start Data Connect emulator: No valid Data Connect configuration detected in firebase.json. Run ${clc.bold("firebase init dataconnect")} to configure it.`,
);| const options = createMockOptions("apphosting", { | ||
| apphosting: { startCommand: "npm run dev" }, | ||
| }); | ||
| expect(shouldStart(options, Emulators.APPHOSTING)).to.be.true; |
There was a problem hiding this comment.
🔴 [Testing Discipline] Missing test for lockfile auto-detection
Rationale: The PR implements auto-detection for the App Hosting start command by looking for a lockfile in the backend root. However, there is no test verifying that the emulator starts when a lockfile is present.
Suggested Fix: Add a unit test stubbing fs.existsSync using sinon to return true for a lockfile path.
it("should start apphosting emulator if start command is not set but lockfile is present", () => {
const existsStub = sinon.stub(fs, "existsSync");
existsStub.withArgs(sinon.match(/package-lock.json/)).returns(true);
try {
const options = createMockOptions("apphosting", {
apphosting: { rootDirectory: "./my-app" },
});
expect(shouldStart(options, Emulators.APPHOSTING)).to.be.true;
} finally {
existsStub.restore();
}
});| return { | ||
| only, | ||
| config, | ||
| cwd: process.cwd(), |
There was a problem hiding this comment.
🟡 Nit: [TypeScript Precision] Legacy as any in test helper
Rationale: The helper createMockOptions uses as any on return and any for the configValues map. Typing this more precisely (e.g. Record<string, unknown>) would improve type safety in tests.
Suggested Fix:
function createMockOptions(
only: string | undefined,
configValues: Record<string, unknown>,
): Options {
...
return {
only,
config,
cwd: process.cwd(),
project: "test-project",
} as unknown as Options;
}3 participants
Description
Prevent missing configurations for Storage, App Hosting, and Data Connect emulators from crashing emulators:start and blocking the rest of the emulator suite:
Fixes b/546204399
Scenarios Tested
Sample Commands
Description
Scenarios Tested
Sample Commands