Repository navigation
fix(core): report a streaming step's own error, not the caller's or a hook's - #1033
xihaian251 wants to merge 2 commits into
Conversation
The streaming containers decided what error to report for a step with `sys.exc_info()[1]` inside the iterator's `finally`. `sys.exc_info()` is thread state: when the caller was itself inside an `except` block, the iterator reported *that* exception as the step's failure. A streaming action that completed successfully was persisted as `failed` -- and `_init_state_from_persister` re-runs anything not stored as `completed`, so resuming executed the step a second time. Capture the exception from this iterator's own `except` clauses instead, and use it for the post-step callback. While there, keep the step's own error when a hook fails: `post_run_step` is dispatched from the same `finally`, so a hook raising a plain `Exception` replaced the action's exception. The container now logs that hook error and keeps the step's error; a hook's `BaseException` (KeyboardInterrupt / CancelledError / SystemExit) is never caught. The application callback runs `post_end_stream` and the execute-method post hook even when `post_run_step` fails, so a single failing hook no longer truncates the lifecycle. `close()` / `aclose()` keep their own first-error-wins rule: early stop still reports `GeneratorExit` / cancellation, a cleanup error still wins, and repeated close still raises the same error. Tests: tests/core/test_streaming_exception_precedence.py, 24 cases; 14 of them fail on main (36e3ff3).
The narrow aclose race is now reproducible without sleeps: hold aclose() inside the action's cleanup, let the iterator's GeneratorExit path park on _close_future, then cancel that finalizer task. The step was stopped early, so GeneratorExit is the error to report; reading the thread's current exception reported the caller's CancelledError instead. Also pins that a failing post_run_step hook no longer skips post_end_stream: the local tracker releases its per-stream state there, so skipping it leaked stream_state and left an unterminated stream in log.jsonl.
|
Hi maintainers — this is a follow-up to #1027 (early close), already merged as
The patch records the iterator's own exception via its On the async close race we discussed last time: I can now reproduce it Verification on Three things I want to flag rather than hide:
The contributor used WorkBuddy AI (Tencent Hunyuan Hy) for analysis and drafting; |
A streaming step currently decides whether it failed by reading
sys.exc_info()[1]inside the iterator's
finally. That value is thread state, not the step's ownexception: when the caller consumes a successful stream from inside an
exceptblock, the caller's exception is reported as the step's failure, persisted as
failed, and the step is executed a second time on resume.This change records the exception from the iterator's own
exceptclauses instead,keeps the step's own error when a lifecycle hook fails, and stops a failing
post_run_stephook from skippingpost_end_stream.close()/aclose()areuntouched.
Problem
Two symptoms, one
finallyinStreamingResultContainer.__iter__/AsyncStreamingResultContainer.__aiter__:sys.exc_info()[1]inherited thecaller's in-flight exception.
PersisterHookstoresfailed,_init_state_from_persistercallsreset_to_entrypoint()for anything notcompleted, and resume re-runs the action — so a non-idempotent side effect(a charge, an email, an order) happens twice.
stream_iterate/astream_iteratecreate one container per step, so every step in the chain isaffected.
post_run_stepis dispatchedfrom the same
finally; a plainExceptionraised there overwrites theaction's exception, and the remaining lifecycle callbacks
(
post_end_stream, execute-method post) never run. That leaves thepre_start_stream/post_end_streampair unbalanced —LocalTrackingClientreleases its per-stream state only in
post_end_stream, so a telemetry hookfailure leaks
stream_stateand writes aninitialize_streamwith no matchingend_stream.Minimal reproduction (sync; async behaves the same):
Root Cause
sys.exc_info()returns the exception state of the current thread, and agenerator inherits that state when it is first resumed. After
except StopIteration: return, thefinallystill sees the inherited exception.Burr used that value as the step's
exception=argument, which feeds bothpost_run_stephooks andPersisterHook.save(..., status).The ownership boundary belongs on the iterator frame: only an exception raised
inside this iterator's own
tryand caught by its ownexceptclauses describesthis step. Reading thread state crosses that boundary.
Two consequences of the same design mistake:
caused the step to fail.
Wrapping the call in a broader
exceptwould not help: it would still be readingthread state rather than the step's own control flow, and would risk swallowing
KeyboardInterrupt/CancelledError/SystemExit.Changes
burr/core/action.pyStreamingResultContainer.__iter__andAsyncStreamingResultContainer.__aiter__now keep a localexc, assigned onlyby this iterator's own
except GeneratorExit/except BaseExceptionclauses.
except StopIteration/except StopAsyncIterationleave itNone.sys.exc_info()is no longer used in either container._realize_callback_keeping_step_error(sync + async): when the stepalready has an error, a callback
Exceptionis logged and does not replaceit; when the step succeeded, the callback error still propagates. A callback
BaseExceptionis never caught.close()/aclose()are not modified.burr/core/application.pystream_result/astream_resultcallbacks are now two-stage: ifpost_run_stepraises,post_end_streamand the execute-method post hookstill run, then the first hook error is re-raised. The container decides
whether that error is allowed to supersede the step's own error.
tests/core/test_streaming_exception_precedence.py— 30 cases (24 from theoriginal analysis + 6 added for the async close/cancel race and the stream-end
pairing).
Net: 3 files,
+851 / -40(716 lines are tests). No public API change; no newdependency.
How I tested this
All runs used isolated git worktrees with a unique
--basetempper run. Baselineis
36e3ff30; the branch is36e3ff30 -> 6a840f1c (fix) -> acee6f3b (tests only).tests/coreminus thedot-dependent display tests (3.13.14)tests/coreminus thedot-dependent display tests (3.10.22)The 15 failures are identical in both trees (
diffof the FAILED lists is empty):5 need the
dotexecutable, 6 are pre-existingtest_apache_release*templatefailures, and 4 are release/verify tool tests that fail only when run together
(they pass individually on both trees). The
+30passed is exactly the number ofnew cases.
aclose()is in flight and parked inside the action's cleanup, the iterator'sGeneratorExithandler is parked onasyncio.wait([self._close_future]), andthat finalizer task is cancelled. Built with
asyncio.Eventhandshakes andexplicit tasks (no
sleepused as a synchronisation primitive). Result: mainreports the caller's
CancelledErrortopost_run_step; with this patch itreports
GeneratorExit, which is whataclose's docstring and fix(core): propagate early close through streaming actions #1027 promise.Persisted status is
failedon both trees; the action's cleanup runs exactlyonce; no unhandled task exceptions. Pinned by
test_async_aclose_race_reports_the_early_stop_not_the_cancellation.post_run_stephook used to skippost_end_stream. Measured withLocalTrackingClient: on main the trackerkeeps
stream_state[('app','a','pk')]forever andlog.jsonlhasinitialize_streamwith noend_stream; with this patch the state is releasedand the log is balanced. Pinned by
test_a_failing_hook_does_not_leak_the_tracker_stream_state.restoring
sys.exc_info(); dropping the iterator'sBaseExceptionrecord;forcing
exc=None; removing error precedence; catchingBaseException;swallowing a hook error on success; treating early stop as success; letting a
hook error beat a cleanup error; breaking only the async path; truncating the
lifecycle; re-aborting the lifecycle on a sync hook failure; reverting only the
async
finallytosys.exc_info(); swallowing the re-raised hook error;letting a hook error replace the step error again; skipping
post_end_streamin the async path only; and not recording cancellation on the async path.
SQLitePersisterstorescompleted, three consecutiveresumes never re-run the action, sequence IDs 0 -> 1 without a UNIQUE conflict.
GeneratorExit/ cancellation reporting, cleanuperror precedence, and repeated
close()/aclose()raising the same error areunchanged (verified on both trees).
black 23.11.0,isort 5.12.0,flake8 6.1.0andscripts/check_asf_headers.pyall pass on the three changed files (versionspinned by the repo's
.pre-commit-config.yaml);git diff --checkis clean andthe patch applies to
36e3ff30withgit apply --check.Scope note (not an overclaim): the repo's CI command is
pytest tests --ignore=tests/integrations/persisters --ignore=tests/integrations/test_bip0042_bedrock.py.On this machine it stops during collection with 8 errors because optional extras
(
langchain_core,langchain_community,hamilton,haystack,opentelemetry.sdk,ray) are not installed. The table above excludes exactlythose files. CI covers Python 3.10/3.11/3.12; I ran the 30 regression cases on
3.10.22, 3.11.16, 3.12.14 and 3.13.14 with identical results, and
tests/coreon 3.10.22 and 3.13.14 (
473 -> 503on both). The full-suite comparison above wasrun on 3.13.14. The project has no mypy/pyright/ruff type configuration, so no type
check was run.
Dependabotis already failing onmain's check-runs and isunrelated to this change.
Compatibility
exceptionvalues for a successful stepstay
None; for a failed step they are now the step's own exception rather thanan unrelated one.
BaseException(KeyboardInterrupt,SystemExit,asyncio.CancelledError,GeneratorExit) is never swallowed. A hook raising aBaseExceptionstillpropagates immediately and the action error remains in
__context__.close()/aclose()are byte-for-byte unchanged, so the early-stop contractintroduced in fix(core): propagate early close through streaming actions #1027 is preserved.
observable):
post_run_stephook — including on a step thatotherwise succeeded —
post_end_streamand the execute-method post hook nowrun. Previously they were skipped. This is required to keep
pre_start_stream/post_end_streambalanced for hooks that keep per-streamstate (the local tracker, the S3 tracker, the OpenTelemetry adapter).
post_run_stephook raising a plainExceptionis now logged (with traceback) instead of replacing the action'serror. On a successful step the hook error still propagates.
post_run_stepreceivesGeneratorExitinstead ofCancelledError. Persisted status isfailedeither way.
mainand out of scope:post_run_stephook still prevents laterpost_run_stephooks inthe same dispatch loop (
burr/lifecycle/internal.py) from running;post_end_stream(the callback catchesException, notBaseException);stream_result/astream_resulthalting at a non-streaming actiondispatches
post_run_steptwice when a hook fails — the second dispatchpasses the hook's own exception as the step's
exception=, so the trackerrecords one step twice, the second entry as failed. Different code path
(
application.py, non-streaming branch), pre-existing, and it needs adispatch-level decision rather than a local fix; I have a minimal repro and
can file it as a separate issue.
Relation to #1027 and to open PRs
#1027 (
fix(core): propagate early close through streaming actions, merged as36e3ff30) reworked the streaming containers and established: early stop reportsGeneratorExit/cancellation, cleanup errors win onclose()/aclose(), andrepeated close raises the same error. This PR does not redo any of that and
does not touch
close()/aclose(); 4 regression cases specifically pin thosecontracts. It addresses two gaps #1027 left open: the
finallystill usedsys.exc_info()[1], and a hook error could still replace the action's error.This PR is independent of the separate non-streaming
_step/_astephookerror-precedence fix — different execution path, different patch; that fix is
not included here.
For orientation only (no code overlap):
_astep(non-streaming). This PRdoes not modify
_astep.CONFLICTING/DIRTY.This PR does not modify
_call_execute_method_pre_post; it only ensurescall_poststill runs when apost_run_stephook fails.outer error path in
astream_result(aroundapplication.py:2016) so that astream which never started does not get a
post_end_stream. This PR changes thecallback dispatch (
application.py:1623/1897) so that a stream which didstart does get one. They are the two sides of the same pairing contract and are
complementary; there is no textual overlap.
Notes
AI Disclosure
Analysis, test design, mutation design, and the drafts of these documents were
produced with WorkBuddy AI (Tencent Hunyuan Hy). The code changes, the
reproduction scripts, and every verification step were reviewed and executed
locally by the contributor; all commands and raw outputs are retained in the
working notes and can be re-run. The tool's exact version is not exposed in this
environment, so no version string is asserted here.
Per the ASF Generative Tooling Guidance,
contributors are responsible for their submissions and are encouraged to
identify tooling in the commit message (e.g.
Generated-by: <Tool> <Version>).The current local commits do not yet carry that token; I will add it before
publication if the maintainers prefer. The Burr PR template and
CONTRIBUTING.rsthave no dedicated AI field, so the disclosure is placed here.Out of scope
burr/lifecycle/internal.py(one failing hookblocking later hooks, including the persister).
_step/_astephook precedence fix.post_end_stream.stream_resultdispatchingpost_run_steptwicewhen a hook fails (see Compatibility above).
_close_action_generator/_aclose_action_generatorstill takesys.exc_info()[1]astheir
in_flightargument (4 call sites inapplication.py). I instrumented all fouracross 9 scenarios: the value is correct except when the caller is inside an
exceptblock while an already-completed stream's wrapper is closed by GC — there it is the
caller's exception. A controlled experiment (calling the helper with an explicit
in_flight) shows that such a value would downgrade a cleanup error from "propagates"to "logged warning"; but the wrong value and a live cleanup error are mutually exclusive
in the current code (closing a live generator always sets
GeneratorExit), so I couldnot reach it. Business results are identical either way; it is a latent diagnostic-channel
risk to clean up separately, not a live bug.
Checklist
black,isort,flake8,check-asf-headers,check-ast);frontend-lint-stagedwas not run (no frontend changes)_realize_callback_keeping_step_errorvariants carry docstrings; no new public functions)
— N/A: no public API or documented behaviour was added; the behaviour
changes are described above and covered by tests. Happy to add a short
docs/concepts/streaming-actions.rstnote if the maintainers want one.