fix: release source token scraper before retry to prevent deadlock#10758
fix: release source token scraper before retry to prevent deadlock#10758IzaakGough wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces error handling and timeout mechanisms to prevent deployment processes from hanging indefinitely when fetching source tokens. Specifically, it wraps GCFv2 function creation and update operations in try-catch blocks to abort the token scraper on failure. Additionally, it implements a 10-minute timeout race in SourceTokenScraper.getToken() to degrade gracefully to a token-less deployment if a token producer fails without aborting. A unit test has also been added to verify that aborting before a concurrent waiter re-arms the scraper prevents deadlocks. No review comments were provided, so there is no additional feedback.
cabljac
left a comment
There was a problem hiding this comment.
@IzaakGough the deadlock fix itself looks right to me, moving abort() into the per-attempt catch covers the v1 paths that never released the scraper, and the regression tests explain the mechanism nicely.
Two things I'd like fixed.
1. timeoutFallback never clears its timer.
return Promise.race([
promise,
new Promise<V>((resolve) => setTimeout(() => resolve(value), timeoutMillis)),
]);The helper predates your change, but with its old 2 second callers a leaked timer didn't matter. When the token promise wins the race here, the losing 10 minute setTimeout stays scheduled, and a pending timer keeps Node's event loop alive. Every getToken() call that hits the FETCHING branch schedules one. The CLI hides this because command.ts calls process.exit() after each command, but anything that doesn't hard-exit eats the full 10 minutes. You can see it in this PR's own CI: the unit jobs run roughly 13.5 to 14.5 minutes vs roughly 4m on #10759, and that delta matches SOURCE_TOKEN_FETCH_TIMEOUT_MS. The mocha process is sitting idle after the last test waiting for the timers to fire. Any consumer that imports the CLI in-process would see the same lingering.
Fix is small, capture the timer and clear it when the race settles:
export async function timeoutFallback<T, V>(
promise: Promise<T>,
value: V,
timeoutMillis = 2000,
): Promise<T | V> {
let timer: NodeJS.Timeout | undefined;
try {
return await Promise.race([
promise,
new Promise<V>((resolve) => {
timer = setTimeout(() => resolve(value), timeoutMillis);
}),
]);
} finally {
clearTimeout(timer);
}
}General rule worth keeping: whenever we race a promise against a timer, the timer needs clearing on settle, otherwise it outlives the race. The neighbouring timeoutError has the same pattern, no need to fix it here, but happy for you to do it in passing since you're in the file.
2. Please raise SOURCE_TOKEN_FETCH_TIMEOUT_MS to 25 minutes to match the poller's masterTimeout.
At 10 minutes the backstop fires on healthy deploys: builds can legitimately run up to 25 minutes (that's why masterTimeout is 25 in fabricator.ts), so waiters on a slow but healthy first build give up early and each run a full rebuild, making working deploys slower and more expensive for exactly the projects that benefit most from token reuse. Now that abort() releases waiters immediately on every real failure path, the timeout only exists to catch a hypothetical future path that forgets to abort, and a 25 minute backstop covers that just as well without ever firing on a healthy build.
…com/firebase/firebase-tools into @invertase/fix-source-token-scraper
Fixes #10755 and #10661
Summary
Prevent function deploy retries from deadlocking when source-token acquisition is still in flight.
Problem/Root Cause
If a
createFunctionorupdateFunctionattempt failed before the source-token poller completed, the next retry could wait on a token promise that would never resolve.Solution/Changes
Abort and re-arm the source-token scraper when create/update calls fail, add a timeout fallback for stalled token fetches, and apply the fix to both v1 and v2 function deploy paths.
Testing
createFunctionandupdateFunctionafter a retryable 409.