Skip to content

docs(sdk): document FIFOLock held across arun()'s await (Q4 of #3341) #3400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
VascoSch92 wants to merge 1 commit into
base: main
Choose a base branch
from
+12 −0
Open

docs(sdk): document FIFOLock held across arun()'s await (Q4 of #3341) #3400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions openhands-sdk/openhands/sdk/conversation/fifo_lock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ class FIFOLock:
- FIFO ordering: Threads get lock in request order
- Context manager support: Use with 'with' statement
- Thread-safe: Safe for concurrent access

Note: reentrancy is keyed by OS thread id, not by asyncio task, so the
lock does NOT serialize concurrent coroutines on the same event-loop
thread - each re-enters it. Only hold it across an ``await`` when no
other task on that thread can acquire it.
"""

_mutex: threading.Lock
Expand Down
7 changes: 7 additions & 0 deletions openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,6 +1005,13 @@ async def arun(self) -> None:
ConversationExecutionStatus.RUNNING
)

# The state lock is held across this await. This is safe
# only because every other state mutator is dispatched via
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot May 27, 2026

Choose a reason for hiding this comment

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

🟠 Important: This says every other state mutator is dispatched via run_in_executor, but native async Agent.astep() and ACPAgent.astep() both run state/on-event mutations on the event-loop thread after async awaits. That makes the documented invariant too broad and contradictory. Please narrow this to the actual rule: no unrelated/concurrent same-loop state mutator may run while arun() holds the lock; intentional mutations inside the awaited astep() are part of this critical section.

# run_in_executor onto a worker thread: FIFOLock is thread-
# (not task-) reentrant, so a state-mutating coroutine
# awaited on this event-loop thread would silently re-enter
# the lock and corrupt history. Do not await state mutations
# on the event-loop thread.
await self.agent.astep(
self,
on_event=self._on_event,
Expand Down
Loading