Allow specifying failure policies

[merlinnot]

This PR:

• Makes runtime configuration validation more precise.
• Adds a FailurePolicy, which can be specified for all functions.
• Adds a warning message when FailurePolicy is used for https functions. Originally I planned to express it using more correct typings, it's doable, but not very clean. It's tricky because the runtime options are specified before the function trigger is chosen, so specifying a retry policy would result in limiting the number of options for triggers (removing https). We'd either need two classes returned conditionally based on the config (EventTriggeredFunctionBuilder when retry policy is specified, current FunctionBuilder otherwise) or we can just have this warning and ignore this option for https functions. Let me know which approach do you like better.
• Exposes FailurePolicy in __trigger, so it can be consumed by firebase-tools.
• Adds tests for setting and exposing failurePolicy.

I'm wondering how to approach releasing this functionality. Should we first implement support for it in firebase-tools, release a new version of it and start requiring it in peerDependencies? It would be weird if it's allowed to specify it here, but the setting is ignored by the CLI.

Description

Resolves #425.

Code sample

The API change is described in the issue linked above.

[merlinnot]

Travis fails on Node.js 10, because it can't install it. CI passes on Node.js 8.

[thechenky]

Hey @merlinnot just rerun the job if it fails on stuff like installing Node 10, it usually is a transient error. I just reran your last one and it passed.

[merlinnot]

Great, thanks! The PR is ready for a first pass, please not the question in the description of the PR.

[merlinnot]

@thechenky Hey, just a friendly ping for a review :)

[thechenky]

Ah, will get to this in a bit - for future reference, please add me as reviewer at the top so that I understand it's ready for review.

[thechenky]

There's a loooooooooot of code changes here that are unrelated to the actual feature which makes it very hard to review. I've made a lot of comments here, mostly around pulling these changes out into separate PRs. I'm thinking you need two extra PRs - one for formatting/comment changes, and one for refactors you made around pulling out configuration and changing opts => options.

Please do this, add me as reviewer on all the other PRs so that I can review them separately. Then I will review the code that's left in this PR, which should just be the feature. Also btw, we will be needing CLI changes to parse __trigger to pluck out the extra retry options and supply them in the API to GCF.

[merlinnot]

I'll extract opts -> options rename and comments fixes to separate PRs once the three new ones I created are merged, it will be easier for me to extract these changes.

[thechenky]

Sounds good. Reviewing the others now.

[samtstern]

@merlinnot looks like you might need to run prettier on this one more time before we can merge? I merged master into your branch so please pull (or just force-push your own thing)

[merlinnot]

@samtstern Done!

[kevinajian]

Thanks @merlinnot !

[joehan]

LGTM

[rhodgkins]

Any ideas when this is going to be re-merged back in / released?

[tinaliang]

Good call! @samtstern Is the work in the CLI complete?

[samtstern]

I know this sounds pathetic but no matter what I tried I could not get this to work, the GCF API did not seem to be taking my PATCH requests.

@tinaliang I'll bring you into the internal thread.

[andersonaddo]

Just found this PR, I'm happy this is in the works!
Just hoping it'll be merged again soon :)

[niklv]

Hi

I find workaround, you can define your functions like this:

const functions = require('firebase-functions')

const fn = functions.firestore
  .document('/collection/{item_id}')
  .onWrite(async (change, context) => {
    // duno
    return 0
  })

module.exports = new Proxy(fn, {
  get(target, phrase) {
    const obj = target[phrase]
    if (phrase === '__trigger') obj.eventTrigger.failurePolicy = { retry: {} }
    return obj
  }
})

[cjimmy]

@merlinnot I may be mistaken here, but the current firebase docs do not reflect this change, correct? https://firebase.google.com/docs/reference/functions/function_builder_.functionbuilder#runwith

[samtstern]

@cjimmy you're right looks like we need to regenerate the reference docs, thanks for the reminder!

[cjimmy]

Here are some notes, for my future self and others.

For anyone looking add retries to firebase functions, without having to use GCP console, this PR is it and enabled in firebase-functions 3.10.0. Since it's not entirely obvious, here's a quick example of the syntax:

export const myFirebaseFunc = functions
  .runWith({
    failurePolicy: {
      retry: {},
    },
    memory: '512MB',
    timeoutSeconds: 60,
  })
  .firestore.document('/path/to/some/doc')
  .onCreate(async (snap, context) => {
   /* do stuff */
 })

Right now, it looks like the failure policy is simply on or off. Thus, this is equivalent

export const myFirebaseFunc = functions
  .runWith({
    failurePolicy: true,
    memory: '512MB',
    timeoutSeconds: 60,
  })
  .firestore.document('/path/to/some/doc')
  .onCreate(async (snap, context) => {
   /* do stuff */
 })

Some caveats:

• You'll also have to deploy with --force
• You can enable retries only on triggered functions, not http-called functions.
• It would be idiotic not to build in some safeguards. Retry policy maxes out at 7 days, and bills just like any other function invocation, so if you have some unhandled error, it could repeatedly run for a full week. You can use context.eventTimestamp to know when the first attempt roughly started.

Read this: https://firebase.google.com/docs/functions/retries and make sure your function in idempotent.

It was also difficult to discover what to return to force a retry or avoid a retry.
Triggered Firebase functions must return a Promise.

A Firebase function will retry if:

• it returns a rejected promise
• throws an exception
• or the function times out

That means that if you run into an error that you know won't eventually resolve itself with a retry (i.e. you want to stop the function execution and not retry), you can return Promise.resolve({message: 'some message'});