Skip to content

fix: release source token scraper before retry to prevent deadlock#10758

Open
IzaakGough wants to merge 12 commits into
mainfrom
@invertase/fix-source-token-scraper
Open

fix: release source token scraper before retry to prevent deadlock#10758
IzaakGough wants to merge 12 commits into
mainfrom
@invertase/fix-source-token-scraper

Conversation

@IzaakGough

@IzaakGough IzaakGough commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #10755 and #10661

Summary

Prevent function deploy retries from deadlocking when source-token acquisition is still in flight.

Problem/Root Cause

If a createFunction or updateFunction attempt 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

  • Added unit coverage for the scraper re-arm behavior.
  • Added regression tests for retrying createFunction and updateFunction after a retryable 409.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@IzaakGough IzaakGough marked this pull request as ready for review July 3, 2026 14:33

@cabljac cabljac left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cabljac cabljac left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hold on, just noticed something

@cabljac cabljac left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@IzaakGough IzaakGough requested a review from cabljac July 16, 2026 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants