Skip to content

fix(v2): validate timeoutSeconds per trigger type#1874

Open
kyungseopk1m wants to merge 3 commits intofirebase:masterfrom
kyungseopk1m:fix/v2-timeout-validation
Open

fix(v2): validate timeoutSeconds per trigger type#1874
kyungseopk1m wants to merge 3 commits intofirebase:masterfrom
kyungseopk1m:fix/v2-timeout-validation

Conversation

@kyungseopk1m
Copy link
Copy Markdown

Fixes #1737

Description

The v2 SDK currently accepts any timeoutSeconds value and only the Cloud Functions control plane rejects invalid ones at deploy time with errors like cannot exceed 540 seconds. v1 has had assertRuntimeOptionsValid for a long time but v2 was missing the equivalent, so users only find out they configured the wrong limit after a multi-minute deploy round-trip.

This PR adds per-trigger-type validation that throws at function-definition time (or at manifest-extraction time for providers whose __endpoint is a lazy getter, such as storage).

Limits enforced:

  • event-handling — 540s (firestore, database, pubsub, storage, scheduler, eventarc, testLab, remoteConfig, alerts, dataconnect)
  • HTTPS and callable — 3600s (onRequest, onCall, onCallGenkit, identity, dataconnect/graphql, ai)
  • task queue — 1800s (onTaskDispatched)

Implementation:

  • New internal helper assertTimeoutSecondsValid(opts, kind) and MAX_{EVENT,HTTPS,TASK}_TIMEOUT_SECONDS constants in src/v2/options.ts.
  • optionsToEndpoint and optionsToTriggerAnnotations gained an optional kind: TimeoutKind argument. When omitted they behave exactly as before — validation is opt-in at the call site, so existing consumers of these helpers are unaffected.
  • Each v2 provider passes its own kind when converting the function-specific options. Global-option calls stay unannotated so they keep working exactly as before (no regression for a project that sets timeoutSeconds globally on a function type that can tolerate it).
  • Expression<number>, RESET_VALUE, and undefined are deliberately skipped. The SDK cannot know the concrete value for parametrized expressions, and RESET_VALUE is the user explicitly asking us to clear the global.
  • onCallGenkit delegates to onCall(opts, …) and is therefore covered transitively.

Error messages follow the v1 shape but include the per-kind label:

timeoutSeconds must be between 0 and 540 for event-handling functions. Got 3600.
timeoutSeconds must be between 0 and 3600 for HTTPS and callable functions. Got 3601.
timeoutSeconds must be between 0 and 1800 for task queue functions. Got 1801.

Tests

  • spec/v2/options.spec.ts — 13 unit tests for assertTimeoutSecondsValid covering each kind's limit, negative values, Expression, RESET_VALUE, global fallback, function-level override, and function-level RESET_VALUE clearing an out-of-range global. Plus 6 integration tests for optionsToEndpoint / optionsToTriggerAnnotations (omitted kind = no-op for backwards compatibility, provided kind = enforced).
  • Per-provider regressions:
    • spec/v2/providers/https.spec.tsonRequest and onCall both reject 3601s.
    • spec/v2/providers/pubsub.spec.ts — function-level 3600s and a global 3600s (via setGlobalOptions) are both rejected.
    • spec/v2/providers/tasks.spec.ts — 1801s rejected.
    • spec/v2/providers/storage.spec.ts — validation fires when __endpoint is accessed (lazy-getter path).

Full suite: 900 / 900 passing, npm run build clean, npm run lint 0 errors on the touched files, prettier --check clean.

Backwards compatibility

  • The new kind argument is optional. Calls to optionsToEndpoint / optionsToTriggerAnnotations that don't pass it keep their current behavior, so downstream code that reaches into these helpers is unaffected.
  • In-range timeouts (0–540 for events, 0–3600 for HTTPS, 0–1800 for tasks) continue to work identically.
  • 0 is still accepted, matching v1's assertRuntimeOptionsValid. Enforcing the documented 1s minimum can be a separate follow-up if desired.
  • Users who were silently shipping invalid timeouts will now see the error while running locally instead of at deploy time — this is the intended fix.

Code sample

Before: this deploys, hits gcloud beta functions deploy, and fails after ~30 seconds:

import { onDocumentWritten } from "firebase-functions/v2/firestore";

export const fn = onDocumentWritten(
  { document: "users/{id}", timeoutSeconds: 3600 },
  (event) => { /* ... */ },
);

After: fails immediately when the function module is loaded with:

Error: timeoutSeconds must be between 0 and 540 for event-handling functions. Got 3600.

The v2 SDK accepts any timeoutSeconds value at function-definition
time and leaves the rejection to the Cloud Functions control plane
at deploy time. v1 has had assertRuntimeOptionsValid for a long time
but v2 was missing the equivalent.

Add per-trigger-type validation so misconfigured values fail locally
instead of mid-deploy:

  - 540s for event-handling functions (firestore, database, pubsub,
    storage, scheduler, eventarc, testLab, remoteConfig, alerts,
    dataconnect)
  - 3600s for HTTPS and callable functions (onRequest, onCall,
    onCallGenkit, identity, dataconnect/graphql, ai)
  - 1800s for task queue functions (onTaskDispatched)

Implementation notes:

  - New internal helper assertTimeoutSecondsValid(opts, kind) and
    MAX_{EVENT,HTTPS,TASK}_TIMEOUT_SECONDS constants in
    src/v2/options.ts.
  - optionsToEndpoint and optionsToTriggerAnnotations gained an
    optional kind argument. When omitted they behave exactly as
    before, so existing callers are unaffected.
  - Expression<number> and RESET_VALUE are passed through untouched;
    the SDK cannot know the concrete value in those cases.
  - For providers whose __endpoint is a lazy getter (storage) the
    throw happens the first time the manifest is read instead of at
    function-definition time; a regression test covers this path.

Fixes firebase#1737
Copy link
Copy Markdown
Contributor

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

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 validation for the timeoutSeconds parameter based on the specific Cloud Functions v2 trigger type (event-handling, HTTPS/callable, or task queue). The validation is performed at function-definition or manifest-extraction time to prevent deployment failures due to misconfigured timeout values. The changes include the addition of a centralized validation function, assertTimeoutSecondsValid, and updates across all v2 providers to pass the correct trigger category during endpoint and trigger annotation generation. Extensive unit tests were added to verify the validation logic for each trigger type and ensure correct fallback behavior with global options. I have no feedback to provide.

Copy link
Copy Markdown

@IzaakGough IzaakGough left a comment

Choose a reason for hiding this comment

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

Hi @kyungseopk1m, thank you for contributing! This looks good overall. The only issue I found is the failing lint check from CHANGELOG.md formatting. Could you please fix that?

@kyungseopk1m
Copy link
Copy Markdown
Author

@IzaakGough Thanks for the review! Fixed in 750b2e9 — formatted CHANGELOG.md with prettier.

Comment thread spec/v2/providers/pubsub.spec.ts
Comment thread src/v2/options.ts
kind: TimeoutKind
): void {
const timeoutSeconds = opts?.timeoutSeconds ?? getGlobalOptions().timeoutSeconds;
if (typeof timeoutSeconds !== "number") {
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.

if timeoutSeconds is NaN here, i think we might have an issue, i think it might slip through?

expect(hello).to.equal("world");
});

it("rejects timeoutSeconds above the 3600s HTTPS limit", () => {
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.

I like that we're testing some of the providers, but I think we need more direct test of the wiring across providers

a parameterized suite like "each v2 provider rejects out-of-range timeout" would be a cheap way to get coverage

Comment thread src/v2/options.ts
* @internal
*/
export function assertTimeoutSecondsValid(
opts: GlobalOptions | EventHandlerOptions | HttpsOptions | undefined,
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.

nit: I don't know if we need the | undefined

Copy link
Copy Markdown
Contributor

@cabljac cabljac left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I've added some comments. @IzaakGough we may be able to pick this up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot Deploy Firestore Trigger Function with timeoutSeconds: 3600 (Error: Max 540s)

3 participants