Fixes source token expiration by token refresh #5198

blidd-google · 2022-10-31T23:11:58Z

Developers have reported a variety of issues while attempting large function deployments (see #4266). Specifically, developers were encountering an Invalid source token error message after successfully deploying a subset of the functions, indicating that at least part of the issue stemmed from the source token re-use hack (used to expedite deployments by re-using the same container for each function).

On particularly large deployments (~100+), the process can take >30 minutes. The source token acquired after the first function deployment expires at 30 minutes, resulting in the failure of subsequent function deployments that included the expired source token.

This change tracks the expiration of acquired source tokens, and after about 25 minutes, discards the old token and instructs the next function deployment to request a new source token. The remaining functions wait until the new source token is acquired before starting deployment. A 5 minute buffer is provided to basically guarantee that no subsequent function deployments will include the expired token. After about 25 minutes, the second token also expires and the next function requests a new token, etc., etc.

taeold · 2022-11-01T23:49:10Z

src/deploy/functions/release/sourceTokenScraper.ts

+enum TokenState {
+  "NONE",
+  "VALID",
+  "INVALID",
+}


tangent: enums are controversial in TS for some reason.

https://blog.logrocket.com/why-typescript-enums-suck/

Internally, I think we prefer string literals:

type TokenState = "none" | "valid" | "invalid"

taeold · 2022-11-01T23:51:08Z

src/deploy/functions/release/sourceTokenScraper.ts

+  }
+
+  calculateTokenExpiry(): bigint {
+    const now = process.hrtime.bigint();


Personally, I think using hrtime with its bigint silliness is unnecessarily precise, especially if the time range we care about is in MINUTES not MICROSECONDS.

I'd suggest we stick to Date.now() and forget about big ints.

taeold · 2022-11-01T23:52:55Z

src/deploy/functions/release/sourceTokenScraper.ts

+    if (this.tokenState === TokenState.NONE) {
+      this.tokenState = TokenState.INVALID;
+      return undefined;
+    } else if (this.tokenState === TokenState.INVALID) {


I found that token states as described here bit confusing. tokenState == INVALID makes me think that token is invalid, but token doesn't actually exist at this point.

What about another name that closely maps to the state of the scraper, like tokenFetchState instead of tokenState and FETCHING instead of INVALID?

taeold · 2022-11-01T23:56:34Z

src/deploy/functions/release/sourceTokenScraper.ts

-    return this.promise;
+  }
+
+  checkTokenExpired(): boolean {


nit* I usually name my boolean function with "is" like isTokenExpired

taeold · 2022-11-01T23:57:05Z

src/deploy/functions/release/sourceTokenScraper.ts

+
+  checkTokenExpired(): boolean {
+    if (this.expiration === undefined) {
+      throw new Error();


This error is user visible - we should try our best to describe the situation and also call out what users should do when they see the issue.

taeold · 2022-11-03T15:33:25Z

src/deploy/functions/release/fabricator.ts

@@ -210,9 +210,11 @@ export class Fabricator {
    if (apiFunction.httpsTrigger) {
      apiFunction.httpsTrigger.securityLevel = "SECURE_ALWAYS";
    }
-    apiFunction.sourceToken = await scraper.tokenPromise();
+    // apiFunction.sourceToken = await scraper.tokenPromise();


nit - stray comment?

taeold · 2022-11-03T15:35:17Z

src/deploy/functions/release/sourceTokenScraper.ts

-      return Promise.resolve(undefined);
+  isTokenExpired(): boolean {
+    if (this.expiry === undefined) {
+      throw new FirebaseError("failed to check expiry: no token exists");


This error message makes sense to you and me but won't to almost all users trying to deploy their function.

I think this is a good example of a user facing error message - describe the situation in coarsely and suggest next steps:

firebase-tools/src/deploy/functions/prepareFunctionsUpload.ts

Lines 36 to 40 in 2ebefe2

throw new FirebaseError(

"Cloud Runtime Config is currently experiencing issues, " +

"which is preventing your functions from being deployed. " +

"Please wait a few minutes and then try to deploy your functions again." +

"\nRun `firebase deploy --except functions` if you want to continue deploying the rest of your project."

In our case, hitting this if statement should never happen and would be considered a bug. Maybe we can suggest that they file a issue.

ah gotcha. working on a more descriptive message.

throw new FirebaseError( "Your deployment is checking the expiration of a source token that has not yet been polled. " + "Hitting this case should never happen and should be considered a bug. " + "Please file an issue at https://github.com/firebase/firebase-tools/issues " + "and try deploying your functions again." );

What about something like this?

src/deploy/functions/release/sourceTokenScraper.ts

src/deploy/functions/release/timer.ts

codecov-commenter · 2022-11-03T15:53:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ea2a9ec). Click here to learn what that means.
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #5198   +/-   ##
=========================================
  Coverage          ?   56.26%           
=========================================
  Files             ?      308           
  Lines             ?    20814           
  Branches          ?     4225           
=========================================
  Hits              ?    11710           
  Misses            ?     8092           
  Partials          ?     1012

Impacted Files	Coverage Δ
src/deploy/functions/release/sourceTokenScraper.ts	`83.33% <81.81%> (ø)`
src/deploy/functions/release/fabricator.ts	`83.59% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

taeold

Lgtm

taeold · 2022-11-03T18:02:11Z

src/deploy/functions/release/sourceTokenScraper.ts

+        this.promise = new Promise((resolve) => (this.resolve = resolve));
+        return undefined;
+      }
+      return this.promise;


Should we throw an error here instead of silently succeeding? Can see argument for either.

By throwing an error, do you mean throwing an error and then catching it in an enclosing try { await getToken(); ... } catch (expiredErr) {...} block, and then retrying?

I mean that in theory we should never reach this codepath - fetchState can only be in one of three states, and we have if condition for all of them. So we could throw (which could happen if we misunderstood this code for somee reason) or just return the promise (which might leave the CLI in an undefined state). I have slight preference for the former.

taeold · 2022-11-03T18:03:24Z

src/test/deploy/functions/release/sourceTokenScraper.spec.ts

+    await expect(scraper.getToken()).to.eventually.equal("magic token #2");
+  });
+
+  it("concurrent requests for source token", async () => {


Nice test!!

inlined · 2022-12-13T01:50:30Z

src/deploy/functions/release/sourceTokenScraper.ts

-      return Promise.resolve(undefined);
+  isTokenExpired(): boolean {
+    if (this.expiry === undefined) {
+      throw new FirebaseError(


Can we update this to throw with code: 2? We track this in Google Analytics for internal assertion errors.

inlined · 2022-12-13T01:51:56Z

src/test/deploy/functions/release/sourceTokenScraper.spec.ts

+    const timeout = (duration: number): Promise<void> => {
+      return new Promise<void>((resolve) => setTimeout(resolve, duration));
+    };
+    await timeout(50);


In the future, try to instrument code without sleeping in your test. You can inject a clock for example.

refresh source token

236446f

blidd-google requested review from taeold and colerogers October 31, 2022 23:12

add changelog entry

08f87c0

blidd-google changed the title ~~Refreshes source token upon expiration~~ Fixes source token expiration by token refresh Oct 31, 2022

check if token is expired right before deploy call

6409aba

blidd-google self-assigned this Nov 1, 2022

taeold reviewed Nov 1, 2022

View reviewed changes

blidd-google requested a review from inlined November 3, 2022 13:55

blidd-google added 2 commits November 3, 2022 11:12

use Date lib, rename states & remove enums

0935361

clean up, revert timer changes

af6a82d

taeold reviewed Nov 3, 2022

View reviewed changes

more descriptive error message

f3b4729

update error msg

101425e

taeold marked this pull request as ready for review November 3, 2022 18:00

taeold approved these changes Nov 3, 2022

View reviewed changes

blidd-google and others added 2 commits November 4, 2022 18:05

add type guard to token states

303bc30

Merge branch 'master' into bl-refresh-src-token

3c5f2a8

blidd-google enabled auto-merge (squash) November 4, 2022 22:09

Update CHANGELOG.md

493e215

blidd-google merged commit 98e23ed into master Nov 4, 2022

inlined reviewed Dec 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes source token expiration by token refresh #5198

Fixes source token expiration by token refresh #5198

blidd-google commented Oct 31, 2022 •

edited

taeold Nov 1, 2022

taeold Nov 1, 2022

taeold Nov 1, 2022 •

edited

taeold Nov 1, 2022

taeold Nov 1, 2022

taeold Nov 3, 2022

taeold Nov 3, 2022

blidd-google Nov 3, 2022

blidd-google Nov 3, 2022 •

edited

codecov-commenter commented Nov 3, 2022 •

edited

taeold left a comment

taeold Nov 3, 2022

blidd-google Nov 3, 2022

taeold Nov 3, 2022

taeold Nov 3, 2022

inlined Dec 13, 2022

inlined Dec 13, 2022

	throw new FirebaseError(
	"Cloud Runtime Config is currently experiencing issues, " +
	"which is preventing your functions from being deployed. " +
	"Please wait a few minutes and then try to deploy your functions again." +
	"\nRun `firebase deploy --except functions` if you want to continue deploying the rest of your project."

Fixes source token expiration by token refresh #5198

Fixes source token expiration by token refresh #5198

Conversation

blidd-google commented Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taeold Nov 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blidd-google Nov 3, 2022 • edited

Choose a reason for hiding this comment

codecov-commenter commented Nov 3, 2022 • edited

Codecov Report

taeold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blidd-google commented Oct 31, 2022 •

edited

taeold Nov 1, 2022 •

edited

blidd-google Nov 3, 2022 •

edited

codecov-commenter commented Nov 3, 2022 •

edited