Skip to content

fix: support persistent lifespans in asgi - #231

Open
dom96 wants to merge 1 commit into
mainfrom
dominik/persistent-lifespan
Open

fix: support persistent lifespans in asgi#231
dom96 wants to merge 1 commit into
mainfrom
dominik/persistent-lifespan

Conversation

@dom96

@dom96 dom96 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Ensures that mutable lifespans are persisted across requests. Adds fastapi and asgi-specific tests.

@dom96
dom96 requested review from hoodmane and ryanking13 August 27, 2026 17:19
Comment on lines +485 to +491
async def lifespan_state():
nonlocal lifespan
if lifespan is None:
# Assign before awaiting so concurrent first requests share startup.
lifespan = create_task(start_application(app))
_, state = await lifespan
return state

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A cancelled first request propagates cancellation into this shared task, leaving lifespan permanently cancelled and causing every later request in the isolate to be cancelled. Shield the shared startup task from an individual request cancellation.

Suggested change
async def lifespan_state():
nonlocal lifespan
if lifespan is None:
# Assign before awaiting so concurrent first requests share startup.
lifespan = create_task(start_application(app))
_, state = await lifespan
return state
async def lifespan_state():
from asyncio import shield
nonlocal lifespan
if lifespan is None:
# Assign before awaiting so concurrent first requests share startup.
lifespan = create_task(start_application(app))
_, state = await shield(lifespan)
return state

@ask-bonk

ask-bonk Bot commented Aug 27, 2026

Copy link
Copy Markdown

I'm Bonk, and I've done a quick review of your PR.

Makes ASGI entrypoint lifespan state persist across requests.

  1. High: Posted 1 actionable inline suggestion.

github run

Comment thread packages/runtime-sdk/src/workers/asgi.py Outdated
Comment thread packages/runtime-sdk/src/workers/asgi.py Outdated
@dom96
dom96 force-pushed the dominik/persistent-lifespan branch from 7fdc066 to 864cb45 Compare August 28, 2026 15:54
@dom96
dom96 force-pushed the dominik/persistent-lifespan branch from 864cb45 to 5b8cb5e Compare August 28, 2026 16:00
ctx: Context | None = None,
state: dict[str, Any] | None = None,
) -> js.Response:
if (req.headers.get("upgrade") or "").lower() == "websocket":

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.

Suggested change
if (req.headers.get("upgrade") or "").lower() == "websocket":
if req.headers.get("upgrade", "").lower() == "websocket":

Comment on lines +479 to +480
entrypoint_type = type(self)
start_future = entrypoint_type.__dict__.get("_start_future")

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.

This seems weird to me, shouldn't it be an instance attribute not a class attribute? Also, why not just:

Suggested change
entrypoint_type = type(self)
start_future = entrypoint_type.__dict__.get("_start_future")
start_future = self._start_future

@hoodmane hoodmane left a comment

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.

Generally looks reasonable, though I'd set _start_future on the AsgiWorkerEntrypoint instance rather than the class and initialize _start_future to None in AsgiWorkerEntrypoint.__init__(). Could adjust it in a followup if you like though.

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