Fix race condition in functions discovery shutdown

[tilgovi]

Attach event handlers to subprocesses before yielding to calling code to ensure that promises of shutting down discovery processes are fulfilled.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gemini-code-assist]

Summary of Changes

Hello @tilgovi, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical race condition in the function discovery shutdown mechanism for both Node.js and Python runtimes. By proactively attaching exit and error event listeners to child processes as soon as they are created, the system can now reliably detect and respond to process terminations. This change guarantees that shutdown promises are always honored, leading to a more stable and predictable termination sequence for discovery processes and preventing potential resource leaks or hanging processes.

Highlights

• Race Condition Fix: Addressed a race condition during the shutdown of function discovery processes by attaching exit and error event handlers to child processes immediately after they are spawned, rather than within the asynchronous shutdown routine.
• Reliable Shutdown: Ensured that the promise for a child process's shutdown is fulfilled consistently by moving the event listener attachment to an earlier stage, preventing scenarios where a process might exit before its shutdown promise is properly registered.
• Timeout Management: Improved timeout handling by ensuring that the quitTimeout is cleared reliably when the child process exits, regardless of whether it exits gracefully or with an error.
• Cross-Runtime Consistency: Applied the fix consistently across both Node.js and Python function runtimes, ensuring robust shutdown behavior for both.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to fix a race condition in shutting down functions discovery processes by attaching event handlers to subprocesses earlier. The approach of creating the exit promise (pExit) before returning the shutdown function is correct and effectively addresses the race condition.

However, I've found critical issues in both modified files that would lead to runtime errors:

• In node/index.ts, there's a ReferenceError because quitTimeout is not defined, along with a syntax error. It seems to be a copy-paste issue.
• In python/index.ts, there's a syntax error in the finally block.

I've left specific comments with suggestions to fix these problems. Once these are addressed, the change should be good to go.

[tilgovi]

Without this, failures of the emulator to start a function (such as do to runtime error in the function's entrypoint) seem to just hang for a number of seconds and then the deploy command exits with code 0. I can't figure out where that timeout is happening or where the error is getting swallowed. With the change, a failure to start the function immediately results in the deploy command exiting with a non-zero exit code.

[taeold]

/gemini review

[gemini-code-assist]

Code Review

The changes in this pull request correctly address a race condition in the shutdown logic for both Node.js and Python function discovery processes. By attaching the 'exit' and 'error' event handlers immediately after the child process is spawned, it ensures that the shutdown promise will always be settled, even if the process terminates unexpectedly early. Additionally, the updated timeout handling, which now correctly clears the SIGKILL timeout when the process exits, is a great improvement that prevents potential resource leaks and fixes a bug in the Python runtime where the timeout was cleared immediately. The code is clean and the fix is well-implemented.

[gemini-code-assist]

Since serveAdmin is an async function, it already returns a Promise. You can remove the explicit Promise.resolve wrapper for simplicity.

Suggested change

	return Promise.resolve(async () => {
	try {
	await fetch(`http://127.0.0.1:${port}/__/quitquitquit`);
	} catch (e) {
	logger.debug("Failed to call quitquitquit. This often means the server failed to start", e);
	}
	const quitTimeout = setTimeout(() => {
	if (!childProcess.killed) {
	childProcess.kill("SIGKILL");
	}
	}, 10_000);
	clearTimeout(quitTimeout);
	return new Promise<void>((resolve, reject) => {
	childProcess.once("exit", resolve);
	childProcess.once("error", reject);
	return pExit.finally(() => {
	clearTimeout(quitTimeout);
	});
	});
	return async () => {
	try {
	await fetch(`http://127.0.0.1:${port}/__/quitquitquit`);
	} catch (e) {
	logger.debug("Failed to call quitquitquit. This often means the server failed to start", e);
	}
	const quitTimeout = setTimeout(() => {
	if (!childProcess.killed) {
	childProcess.kill("SIGKILL");
	}
	}, 10_000);
	return pExit.finally(() => {
	clearTimeout(quitTimeout);
	});
	};