Repository navigation
fix(langchain): clean up streaming run memo on error - #1978
Conversation
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
chrikrah
left a comment
There was a problem hiding this comment.
@dhionson2 approving at 11ac8a0. This fixes #1977, and the new test fails when I put back the old on_llm_error.
$ python -m pytest tests/unit/test_langchain.py -q -p no:cacheprovider # Python 3.14, uv sync --frozen
# head 11ac8a0
24 passed
# head tests, CallbackHandler.py from base bf11ec1
1 failed, 23 passed
FAILED tests/unit/test_langchain.py::test_streamed_llm_error_cleans_only_its_completion_start_time_memo
@hassiebp could you review this one?
|
Thanks a lot for your contribution, @dhionson2 |
|
Thanks for reviewing and merging the PR! I appreciate your time and feedback. Glad I could contribute to the project! |
Summary
Fixes #1977.
This PR fixes stale run ID retention in
LangchainCallbackHandlerwhen a streamed LLM execution terminates with an error.Root cause
on_llm_new_token()adds the run ID to_updated_completion_start_time_memoafter recording the first-token timestamp.While
on_llm_end()removes the run ID in itsfinallyblock,on_llm_error()did not perform the same cleanup.This could cause incremental UUID retention when long-lived callback handlers are reused across failed streaming executions.
Changes
finallyblock ofon_llm_error().The change preserves existing public SDK behavior and LangGraph control-flow handling.
Verification
Regression test
Targeted validation
tests/unit/test_langchain.py: 24 passed.git diff --check: passed.Full unit suite on Windows
pydanticand one Windows/POSIX path serialization mismatch.The full suite has not yet been confirmed green on Linux. CI validation is pending.
Scope
This is a minimal, backward-compatible fix limited to the LangChain callback implementation and its regression test.
No dependency, generated API, or public interface changes.

Confidence Score: 5/5
This PR appears safe to merge; no actionable issues were found.Summary
This PR removes a streamed run’s first-token memo entry when
on_llm_errorfinishes.Reviews (1) · Last reviewed commit: "fix(langchain): clean up streaming run m..." · Reviewed by Greptile