Skip to content

Chore: Add warn in fal run if the app startup time exceeds startup timeout.#958

Open
mittal-ishaan wants to merge 8 commits intomainfrom
SERV-3/warn-in-fal-run-when-startup-timeout-would-trigger
Open

Chore: Add warn in fal run if the app startup time exceeds startup timeout.#958
mittal-ishaan wants to merge 8 commits intomainfrom
SERV-3/warn-in-fal-run-when-startup-timeout-would-trigger

Conversation

@mittal-ishaan
Copy link
Copy Markdown

@mittal-ishaan mittal-ishaan commented Apr 6, 2026

To check for startup timeout in fal run, we wrap a timer around setup function and check how much time is it taking
Screenshot 2026-04-09 at 9 15 26 PM

@linear
Copy link
Copy Markdown

linear bot commented Apr 6, 2026

@mittal-ishaan mittal-ishaan force-pushed the SERV-3/warn-in-fal-run-when-startup-timeout-would-trigger branch from ba0dcb9 to 3f1c4b7 Compare April 8, 2026 17:58
@mittal-ishaan mittal-ishaan marked this pull request as ready for review April 8, 2026 19:13
@mittal-ishaan mittal-ishaan changed the title Add warn in fal run if the app startup time exceeds startup timeout. Chore: Add warn in fal run if the app startup time exceeds startup timeout. Apr 8, 2026
@mittal-ishaan mittal-ishaan requested a review from Copilot April 9, 2026 15:46
@mittal-ishaan
Copy link
Copy Markdown
Author

@claude review

@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

Claude finished @mittal-ishaan's task in 1m 34s —— View job


Code Review

  • Read changed files and gather context
  • Review app.py changes
  • Review test_app.py changes
  • Post final review

Overall the change is clean and well-tested. A few observations below.

projects/fal/src/fal/app.py

1. Use logger.warning instead of print

The module already uses a structured logger (logger = get_logger(__name__) at module level). Using print for this warning is inconsistent with the rest of the codebase. Prefer:

logger.warning(
    "App setup exceeded startup_timeout (%s.1fs > %ss). "
    "`fal deploy` would have triggered the startup timeout.",
    setup_elapsed, effective_startup_timeout,
)

Fix this →

2. or vs is not None for falsy zero

effective_startup_timeout = (
    self.startup_timeout or DEFAULT_PLATFORM_STARTUP_TIMEOUT
)

If someone ever sets startup_timeout = 0, this silently falls back to 600. While 0 is an unlikely value in practice, the semantically correct check is:

effective_startup_timeout = (
    self.startup_timeout if self.startup_timeout is not None
    else DEFAULT_PLATFORM_STARTUP_TIMEOUT
)

3. Verify the 600s constant

DEFAULT_PLATFORM_STARTUP_TIMEOUT = 600 — worth confirming this matches the actual platform default. The wait_for_app helper in the same file uses startup_timeout: int = 60 (line 230), though that appears to be a local health-check timeout rather than the server-side startup timeout, so the values being different is likely intentional. A brief comment explaining the source of 600 would make this clearer for future readers.


projects/fal/tests/unit/test_app.py

The tests look solid:

  • The parametrize matrix covers the key cases: explicit timeout over/under, default timeout over/under, and the exact-equal boundary ((1, 1.0, False)).
  • Patching fal.app.time.perf_counter with side_effect=[0.0, elapsed_seconds] correctly simulates the two calls without actually sleeping.
  • The TestApp class is defined inside the test function, so the direct assignment TestApp.startup_timeout = startup_timeout doesn't bleed between parametrize iterations.

One minor nit: the printed_lines construction

printed_lines = ["".join(map(str, call.args)) for call in mock_print.call_args_list]

works correctly here because the warning uses an f-string (single positional arg), but a more idiomatic way to join call.args with a space would be " ".join(map(str, call.args)). Not a bug, just a style note.


Summary: Two actionable items worth addressing before merge — switch print to logger.warning, and consider the is not None guard. The rest is cosmetic.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a warning mechanism to fal run to inform users when their application's setup time exceeds the configured startup timeout. This helps users catch performance issues early during local development that would later cause failures in production deployments.

Changes:

  • Added a constant DEFAULT_PLATFORM_STARTUP_TIMEOUT = 600 seconds (10 minutes) as the default startup timeout when none is specified
  • Wrapped the setup() function call in the lifespan() method with timing measurements using time.perf_counter()
  • Added logic to check if the measured setup time exceeds the effective startup timeout and print a warning message if it does
  • Added comprehensive parametrized test test_lifespan_startup_timeout_warning with five test cases covering different timeout configurations and elapsed times

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
projects/fal/src/fal/app.py Added startup timeout measurement and warning logic to the lifespan() method, with a new constant for the default platform timeout
projects/fal/tests/unit/test_app.py Added parametrized test to verify warning behavior across different timeout and elapsed time combinations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants