Skip to content

ENG-9348: Lifespan tasks execute in registration order#6334

Open
masenf wants to merge 13 commits intomainfrom
masenf/lifespan-ordering
Open

ENG-9348: Lifespan tasks execute in registration order#6334
masenf wants to merge 13 commits intomainfrom
masenf/lifespan-ordering

Conversation

@masenf
Copy link
Copy Markdown
Collaborator

@masenf masenf commented Apr 15, 2026

Summary

Lifespan tasks now execute in registration order instead of arbitrary set order. This ensures internally registered tasks deterministically run before user-defined lifespan tasks.

  • Changed lifespan_tasks backing store from set to dict (insertion-ordered) to preserve registration order.
  • Added get_lifespan_tasks() method returning a tuple of registered tasks.
  • Added integration test for app.modify_state usage inside a lifespan task.

Deprecations

  • LifespanMixin.lifespan_tasks (the public set attribute): deprecated in 0.9.0, removal in 1.0. Use get_lifespan_tasks() instead, which returns an ordered tuple.

Test plan

  • Existing test_lifespan continues to pass
  • New test_lifespan_modify_state verifies a lifespan task can use app.modify_state to push state updates to connected clients

Avoid indeterminism when lifespan tasks are registered. This allows internally
registered tasks to deterministically execute prior to any user defined
lifespan tasks.

Added test case for originally reported issue.
@linear
Copy link
Copy Markdown

linear bot commented Apr 15, 2026

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 15, 2026

Merging this PR will not alter performance

✅ 9 untouched benchmarks


Comparing masenf/lifespan-ordering (e437699) with main (c3f684c)

Open in CodSpeed

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 15, 2026

Greptile Summary

Changes the lifespan_tasks backing store from a set to an insertion-ordered dict, guaranteeing that internally registered tasks run before user-defined ones. The old lifespan_tasks attribute is replaced by a deprecated property (returning a set copy for backward compatibility) and a new get_lifespan_tasks() method returning an ordered tuple. An integration test verifies that app.modify_state works correctly inside a lifespan task.

Confidence Score: 5/5

Safe to merge; implementation is correct and the single remaining finding is a minor style suggestion.

No P0/P1 issues found. The set→dict migration correctly preserves insertion order and deduplication semantics. The deprecated property returns a copy (intentional design). The only open finding is a P2 style suggestion to mark _lifespan_tasks with init=False to prevent accidental use as a constructor argument.

No files require special attention.

Important Files Changed

Filename Overview
reflex/app_mixins/lifespan.py Backing store changed from set to dict for ordered insertion, deprecated lifespan_tasks property added returning a set copy, and new get_lifespan_tasks() method returning an ordered tuple; logic is correct.
tests/integration/test_lifespan.py Added modify_state_task lifespan fixture and test_lifespan_modify_state test; the second poll_for_content return value is used but not asserted (noted in prior threads); otherwise logic looks correct.
docs/utility_methods/lifespan_tasks.md Documentation updated to reflect ordered execution, Starlette (instead of FastAPI) app parameter, and new get_lifespan_tasks() API; changes are accurate.

Sequence Diagram

sequenceDiagram
    participant User as User / Internal Code
    participant RM as register_lifespan_task()
    participant Store as _lifespan_tasks (dict)
    participant Runner as _run_lifespan_tasks()
    participant Task as Lifespan Task

    User->>RM: register_lifespan_task(task, **kwargs)
    RM->>Store: _lifespan_tasks[task] = None (insertion-ordered)

    Note over Store: Order preserved: internal tasks first, then user tasks

    User->>Runner: app startup (Starlette lifespan)
    Runner->>Store: iterate keys in insertion order
    loop For each task (in order)
        Store-->>Runner: task callable
        Runner->>Task: create asyncio.Task or enter AsyncContextManager
    end
    Runner-->>User: yield (app is running)

    User->>Runner: app shutdown
    Runner->>Task: cancel all running tasks
Loading

Reviews (2): Last reviewed commit: "little cleaner test assertion, for grept..." | Re-trigger Greptile

Comment thread reflex/app_mixins/lifespan.py Outdated
Comment thread tests/integration/test_lifespan.py Outdated
@masenf
Copy link
Copy Markdown
Collaborator Author

masenf commented Apr 15, 2026

@greptile-apps re-review this PR, my dog

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

Updates the Reflex app lifespan task system to run deterministically in registration order, ensuring framework-internal lifespan tasks execute before user-registered ones and documenting the new behavior.

Changes:

  • Switches lifespan task storage from an unordered set to an insertion-ordered dict to preserve registration order.
  • Adds get_lifespan_tasks() API and deprecates the public lifespan_tasks attribute.
  • Adds an integration test covering app.modify_state usage from within a lifespan task and updates docs to reflect ordering and inspection.

Reviewed changes

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

File Description
reflex/app_mixins/lifespan.py Stores tasks in insertion order, adds get_lifespan_tasks(), and deprecates lifespan_tasks.
tests/integration/test_lifespan.py Adds an integration test and a new lifespan task that pushes state updates via modify_state.
docs/utility_methods/lifespan_tasks.md Documents registration-order execution and the new task inspection API.

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

Comment thread reflex/app_mixins/lifespan.py Outdated
Comment thread reflex/app_mixins/lifespan.py Outdated
Comment thread tests/integration/test_lifespan.py Outdated
masenf and others added 3 commits April 15, 2026 12:00
Return frozenset instead of set from the deprecated lifespan_tasks
property so callers cannot mistakenly mutate the returned collection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move the @deprecated decorator to a TYPE_CHECKING-only stub so that
IDEs and type checkers still surface the deprecation warning, but
runtime access only triggers console.deprecate (avoiding duplicate
deprecation signals).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove blanket exception handling in the test's modify_state_task so
any unexpected errors propagate immediately rather than being hidden.
The outer CancelledError handler remains for clean shutdown.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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

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


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

Comment thread reflex/app_mixins/lifespan.py
Comment thread reflex/app_mixins/lifespan.py
Comment thread tests/integration/test_lifespan.py
masenf and others added 3 commits April 15, 2026 12:40
Extract _get_task_name helper that uses task.get_name() for
asyncio.Task objects and __name__ for callables. This fixes an
AttributeError when registering a pre-created asyncio.Task, which
was already accepted by the type signature but crashed at runtime.

Add integration test that creates an asyncio.Task during lifespan
startup and registers it, verifying it runs and gets cancelled on
shutdown.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add comments clarifying that test_lifespan must be the last test in
the file since it shuts down the session-scoped backend.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
unfortunate, but changing these to `nonlocal` fixes the typing, but ultimately
breaks the compiled app because these _do_ end up being module globals.
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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


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

Comment on lines 88 to 92
async with contextlib.AsyncExitStack() as stack:
for task in self.lifespan_tasks:
run_msg = f"Started lifespan task: {task.__name__} as {{type}}" # pyright: ignore [reportAttributeAccessIssue]
for task in self._lifespan_tasks:
task_name = _get_task_name(task)
run_msg = f"Started lifespan task: {task_name} as {{type}}"
if isinstance(task, asyncio.Task):
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

Iterating directly over self._lifespan_tasks will raise RuntimeError: dictionary changed size during iteration if a lifespan task registers additional lifespan tasks while startup is in progress (e.g., an async contextmanager that calls register_lifespan_task). Even if it doesn’t crash, tasks added mid-iteration won’t be visited and therefore won’t be tracked in running_tasks for cancellation. Consider iterating over a snapshot and/or using an index-based loop that can safely pick up newly-registered tasks (preserving registration order) so dynamically-registered asyncio.Tasks are also cancelled on shutdown.

Copilot uses AI. Check for mistakes.
masenf and others added 2 commits April 15, 2026 13:21
Registering a task from within a running lifespan task would mutate the
dict during iteration. Add a _lifespan_tasks_started flag that causes
register_lifespan_task to raise RuntimeError once _run_lifespan_tasks
has entered.

The previous test that registered an asyncio.Task from inside a
lifespan context manager is now a negative test asserting the
RuntimeError. The raw_asyncio_task_coro is registered directly as a
coroutine function (the framework wraps it in asyncio.create_task).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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