Skip to content

fix(runtime-sdk): ensure finalize_request is alive inside wait_until call - #250

Draft
ryanking13 wants to merge 2 commits into
mainfrom
gyeongjae/finalize-proxy
Draft

fix(runtime-sdk): ensure finalize_request is alive inside wait_until call#250
ryanking13 wants to merge 2 commits into
mainfrom
gyeongjae/finalize-proxy

Conversation

@ryanking13

@ryanking13 ryanking13 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

While working on cloudflare/python-workers-examples#103, I noticed that finalizer_task is gc-ed and cleaned up improperly inside wait_until.

This fixes it by wrapping it with create_proxy.

Probably the long term solution is to land #90 and make self.ctx.waitUntil and workers.wait_until work exactly the same, but #90 requires JSPI which isn't available in 0.26.0a2, so it needs more discussion.


wait_until(run_in_background(finalize_request()))
finalizer_task = run_in_background(finalize_request())
task_proxy = create_proxy(finalizer_task)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

create_proxy() transfers ownership to the caller, so this proxy is retained permanently for every streaming response. Destroy it when the background finalizer settles, as the websocket path does.

Suggested change
task_proxy = create_proxy(finalizer_task)
finalizer_task = run_in_background(finalize_request())
task_proxy = create_proxy(finalizer_task)
finalizer_task.add_done_callback(lambda _: task_proxy.destroy())
wait_until(task_proxy)

@ask-bonk

ask-bonk Bot commented Sep 9, 2026

Copy link
Copy Markdown

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

Ensures streaming ASGI finalizers remain alive across wait_until.

  1. P2: The new finalizer proxy is never destroyed, leaking one Pyodide proxy per streaming request. Posted one inline suggestion.

github run

@dom96 dom96 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.

AI comment might have a point, but rest looks good so approving.

@hoodmane

hoodmane commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

This is a duplicate of #249. I think it should be fixed in workerd:
cloudflare/workerd#7293

@ryanking13

Copy link
Copy Markdown
Contributor Author

This is a duplicate of #249. I think it should be fixed in workerd:
cloudflare/workerd#7293

sounds good. Thanks for fixing that

@ryanking13
ryanking13 marked this pull request as draft September 10, 2026 04:40
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.

3 participants