Fix sync_compatible resolution in synchronous transaction hooks#22626
Fix sync_compatible resolution in synchronous transaction hooks#22626hsusul wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68b38476f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await hook(self) | ||
| else: | ||
| await anyio.to_thread.run_sync(hook, self) | ||
| await run_sync_in_worker_thread(hook, self) |
There was a problem hiding this comment.
Keep synchronous transaction hooks shielded from cancellation
When an AsyncTransaction is unwound inside an active cancellation scope (for example, during a flow or task timeout), this helper uses abandon_on_cancel=True, so cancellation stops awaiting the synchronous commit or rollback hook while its worker thread continues running. The previous anyio.to_thread.run_sync call retained the default shielding behavior and waited for the hook to finish; this change can therefore abort rollback()/commit() and reset() while cleanup side effects continue after the transaction context has unwound. Preserve the prior non-abandoning cancellation behavior while propagating the synchronous context flag.
Useful? React with 👍 / 👎.
Merging this PR will not alter performance
Comparing Footnotes
|
Summary
This PR fixes an issue where
@sync_compatiblefunctions (such asBlock.loadorprefect.variables.get) returned unawaited coroutine objects instead of synchronous values when called inside synchronous transaction hooks (on_rollback,on_commit,on_completion) during an asynchronous flow run.Problem
When a synchronous transaction hook (e.g.
@task.on_rollback) is executed during anasyncflow, the transaction runner executes the hook in a synchronous thread (or directly as a sync function call). However,@sync_compatiblefunctions inspect the enclosing run context (FlowRunContext), see that the parent flow is asynchronous (flow.isasync == True), and assume that the caller willawaitthe returned coroutine. Because synchronous hook functions are notasync defand cannotawaitcoroutines, the hook receives a raw unawaited coroutine object (causingAttributeErrororTypeErrorwhen accessing properties on the returned value).Root cause
sync_compatiblecheckedparent_obj.isasyncwithout checking if the current thread has an active event loop or ifRUNNING_ASYNC_FLAGwas explicitly set toFalse.RUNNING_ASYNC_FLAGdefaulted toFalse, makingRUNNING_ASYNC_FLAG.get() is Falseindistinguishable from the default unset state.RUNNING_ASYNC_FLAGtoFalsewhen invoking synchronous hook callables.Fix
RUNNING_ASYNC_FLAGdefault toNoneso thatRUNNING_ASYNC_FLAG.get() is Falseuniquely identifies contexts explicitly marked as synchronous.sync_compatibleto verify that an active event loop exists in the calling thread and respectRUNNING_ASYNC_FLAG.get() is False.Transaction.run_hookandAsyncTransaction.run_hookto execute synchronous hooks withRUNNING_ASYNC_FLAG.set(False)(usingrun_sync_in_worker_threadfor async transaction runners).Testing
uv run pytest tests/test_transactions.py tests/utilities/test_asyncutils.py— Passed (148 passed, 8 skipped).repros/17656.pyfails before the fix and passes cleanly after the fix.Compatibility
Preserves backward compatibility across all client and server APIs.
Related issue
Closes #17656