fix: unblock STAR REPL under asyncio — recursion-safe __repr__, loud sync guard, async converse - #24
Closed
ngoclam9415 wants to merge 4 commits into
Closed
fix: unblock STAR REPL under asyncio — recursion-safe __repr__, loud sync guard, async converse#24ngoclam9415 wants to merge 4 commits into
ngoclam9415 wants to merge 4 commits into
Conversation
Timeline.__repr__ eagerly deep-formatted the last 10 entries, including metadata that can hold cyclic or non-serializable tool/resource objects. That sent repr() into infinite recursion whenever a tracer (LangSmith str() fallback on a non-serializable CompressedTimeline) or an error handler touched the timeline. The cascade exhausted the interpreter's recursion budget and crashed build_prompt — inspect.signature (recursive internally) died first as a victim, masking the real underlying error. Bounded __repr__ to entries count + token budget; CompressedTimeline inherits it. Detail remains available via get_timeline_summary()/to_dict(). Also harden Misc.parse_method_signature: wrap inspect.signature so a pathological __wrapped__ chain or low recursion budget falls back to follow_wrapped=False instead of failing build_prompt on one bad method. Adds two regression tests (cyclic entry metadata, large timeline).
chat_response_sync ran the async chat_response via loop.run_until_complete, which calls _check_running() and raises "Cannot run the event loop while another loop is running" whenever the caller already has a running loop on its thread — i.e. any sync agent API call (query/compression/reactive_compact) made from inside async code. The pre-existing async-context guard was dead: its own `raise RuntimeError` was caught by the surrounding `except RuntimeError: pass`, so the cryptic asyncio error surfaced instead. Fix: detect a running loop; when one is present, ship the coroutine to a dedicated background event loop on a daemon thread via run_coroutine_threadsafe and block on the result. The background loop spins independently of the caller's thread, so the coroutine progresses while the caller blocks — no deadlock. When no loop is running, the existing run_until_complete path is unchanged. Trade-off: under a running loop the caller's loop is blocked for the call duration. Acceptable for interactive/single-tenant use (the reported case); concurrent servers should use the async chat_response()/aquery() path. Adds three regression tests (no-loop, inside-running-loop, exception propagation through the bridge).
ngoclam9415
changed the title
The async path (chat_response / agent.aquery / _think_async / call_llm_async) is complete and is the correct entry point from async code. Bridging inside chat_response_sync via a background daemon-thread loop was the wrong layer: it masked sync-from-async misuse and blocked the caller's loop, instead of routing callers onto the async interface. Revert the background-loop bridge. chat_response_sync now fails loud when called from within a running event loop, with guidance toward the async API. This also fixes the pre-existing dead guard (its `raise RuntimeError` was caught by the surrounding `except RuntimeError: pass`, so the cryptic "Cannot run the event loop while another loop is running" surfaced instead). Legit sync callers are unaffected: sync query() from a sync context and the reflection daemon thread both run with no event loop on their thread, so they take the run_until_complete path as before. Tests updated to assert the loud-guard behavior (raises with guidance) plus the no-loop baseline and exception propagation.
converse() (the sync interactive REPL entry) now delegates to aconverse(), which awaits agent.aquery() — so REPL turns use the fully-async STAR path (aquery → _think_async → call_llm_async → chat_response) instead of the sync query()/chat_response_sync path that crashed under a running event loop. _run_coroutine_blocking() runs aconverse via asyncio.run when no loop is running, and on a short-lived worker thread when a loop is already running, so the sync entry point remains usable from any caller. The bridge lives at the REPL entry (one place), keeping everything below it purely async. Adds test_communicator_converse_async.py covering the sync-wrapper-runs- async-path-from-running-loop regression.
ngoclam9415
changed the title
Contributor
Author
|
This PR fix and existing issue but introduce whole new family of errors |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unblocks running the dana-librarian interactive REPL under a running asyncio loop. Three coordinated changes; together they route REPL turns through the fully-async STAR path and stop the recursion cascade that masked the underlying error.
1.
fix(timeline): recursion-safe__repr__(4808797)Timeline.__repr__eagerly deep-formatted entries whosemetadatacan hold cyclic/non-serializable objects → infinite recursion. LangSmith's@observableonbuild_prompttraces the wholeCompressedTimeline; itsstr()fallback hit__repr__→ recursion → exhausted the interpreter budget →inspect.signaturedied first, masking the real error. Bounded__repr__toTimeline(entries=N, max_context_tokens=M)(CompressedTimelineinherits). HardenedMisc.parse_method_signatureto fall back tofollow_wrapped=Falseon recursion/type errors.2.
refactor(llm): loud guard inchat_response_sync(9de161c, reverts 1b49234)The async path (
aquery→_think_async→call_llm_async→chat_response) is complete and is the correct entry from async code. An earlier attempt bridgedchat_response_syncvia a background daemon-thread loop — wrong layer: it masked sync-from-async misuse and blocked the caller's loop. Reverted to a loud guard:chat_response_syncnow raises a clear, actionable error pointing to the async API when called under a running loop. This also fixes the pre-existing dead guard (itsraise RuntimeErrorwas swallowed by the surroundingexcept RuntimeError: pass). Legit sync callers (syncquery()from a sync context, the reflection daemon thread) are unaffected — no running loop on their thread.3.
feat(communicator): route syncconversethrough the async STAR path (bff7d50)converse()(sync REPL entry) now delegates toaconverse()→await agent.aquery(...), so REPL turns use the fully-async path instead of syncquery()/chat_response_sync._run_coroutine_blocking()runsaconverseviaasyncio.runwhen no loop is running, and on a short-lived worker thread when a loop is already running — so the sync entry stays usable from any caller. The bridge lives at the REPL entry (one place), keeping everything below purely async.Tests
tests/unit/test_timeline.py: +2 (cyclic metadata, large timeline). 41 passed.tests/unit/test_llm_chat_response_sync_bridge.py: 3 (no-loop baseline, loud-guard raises with guidance, exception propagation). Replaces the prior bridge-works tests.tests/unit/core/test_communicator_converse_async.py: 1 (sync wrapper runs the async path from within a running loop).Verified in the consumer venv
Reinstalled the branch into the dana-librarian venv and confirmed at the code level: loud guard present, background bridge absent,
communicator.converseroutes toaconverse, async banner present.Install
```bash
uv pip install --reinstall "git+ssh://git@github.com/aitomatic/dana-runtime.git@fix/timeline-repr-recursion-cascade"
```
Known follow-up (not in this PR)
Orphaned tool call integrity —
_remove_forward_orphansonly prunes orphaned tool results, not orphaned tool calls. After this PR unblocks the async path, a persisted timeline with a dangling assistanttool_calls(missing its tool result) will still 400 from the provider. Needs symmetric sanitization at the message-build boundary; tracked separately.