fix(sessions): checkpoint a resumed handoff before the Session append - #4689
fix(sessions): checkpoint a resumed handoff before the Session append#4689rajarshidattapy wants to merge 5 commits into
Conversation
…attaches BackendSpanExporter._shutdown_event is one-way: BatchTraceProcessor.shutdown() sets it through _request_shutdown() so an in-flight export abandons its retry backoff, and nothing ever clears it. default_exporter() caches a module-level singleton, so that instance outlives the processor that shut it down. Every later export through it gave up on the first 5xx or network error instead of retrying, and logged a warning blaming a shutdown that was already over. Clear the request as a new processor attaches to the exporter, which is the point where the exporter is known to be in service again. Kept duck-typed like the existing _request_shutdown call, since TracingExporter requires neither. Fixes openai#4683
…topped Clearing the exporter's request as a new processor attached could take the cancellation away from a worker that still owned it: if shutdown(timeout=...) timed out while a worker was exporting a failed batch, and a replacement BatchTraceProcessor was constructed before that worker reached _sleep_before_retry, the worker slept through its backoff and kept retrying after the shutdown that was meant to stop it had already returned. Make ownership explicit instead. The processor that requests the shutdown remembers it, and releases it only when its own worker is gone: at the end of _run for a worker that stopped, or after the synchronous drain when no worker was ever started. A worker abandoned by a timed-out shutdown therefore keeps the exporter cancelled until it exits, and hands it back on the way out, so the next processor still gets its retries. The request is now made before the processor's own shutdown event is set, so a worker that exits immediately always sees who owns it.
Processors share an exporter, so a single exporter-wide flag let one release speak for all of them: a provider shutting its processors down in turn could time out on a processor whose worker was still inside an HTTP request, then shut down a second processor whose worker exited and cleared the shared cancellation. When the blocked request finally returned a 5xx, its worker found the request gone and retried, after its own shutdown had returned. Count the outstanding requests instead. _request_shutdown() increments and sets, _reset_shutdown() decrements and only clears the event when the count reaches zero, both under a lock. Each processor asks at most once, so its single release balances its request and never takes the cancellation away from another processor that still needs it.
BatchTraceProcessor.shutdown is safe to call from more than one thread, and the check that kept the exporter request to one per processor was an unsynchronized read-then-set: two concurrent callers could both find the flag unset and both increment the exporter's request count, while the worker released only one of them on its way out. The count never reached zero, so the shared cancellation stayed in force and every later processor gave up on its first transient failure instead of retrying. Move the request behind a per-processor lock, alongside the release that balances it, and release after a successful join as well -- covering a worker that had already exited when the request was made, which would otherwise leave nobody to release it.
A resumed turn that resolves an approved function tool and a queued handoff from the same model response committed the target agent, then appended the batch to the Session. `update_run_state_after_resume()` stored the `NextStepHandoff` transition, which `save_resumed_turn_items()` does not accept as a resumable checkpoint, so a failing append raised without leaving a `pending_session_write`. Retrying continued with the target agent but never repaired the missing `function_call_output` and handoff output, leaving the Session and `RunResult.to_input_list()` permanently divergent. Normalize the transition to `NextStepRunAgain` once the target agent is committed, so the existing pending-write checkpoint carries the batch with no RunState schema change, and publish the completed tool guardrail results before the append can raise.
FU-max-boop
left a comment
FU-max-boop
left a comment
There was a problem hiding this comment.
Thanks for taking on #4685. I reviewed the exact head 8f2985a9379b3a91ca024b8a62a6788c7d6c4a94 against its stated handoff scope and found two things to resolve before this is ready for behavioral review.
-
The branch currently includes the four tracing-exporter commits from #4684 (
4fb0dd57,8760e90e,560d2b1b, and73879e3d) and therefore addssrc/agents/tracing/processors.pyplus 211 lines intests/test_trace_processor.py. Those changes are unrelated to #4685 and are not disclosed in this PR body. The handoff commit alone (73879e3d..8f2985a9) applies cleanly to the stated basea40ae980, so please either rebuild this PR from that single commit or make the stacked dependency/base explicit. As currently shaped, merging this PR would also merge #4684. -
The new 16-row resume matrix defines no tool input/output guardrails and explicitly asserts zero tool guardrail results. However, the runtime change also moves publication of
_tool_input_guardrail_resultsand_tool_output_guardrail_resultsbefore the fallible Session append. That claimed durability behavior is therefore untested. Please add tool guardrails to at least one representative fail-before-commit row and assert that the completed results survive the failed live state, JSON round-trip, and cross-mode retry without re-running the guardrails. That would directly protect the new ordering rather than only the Session batch repair.
The core normalization of the committed NextStepHandoff to the existing NextStepRunAgain checkpoint is consistent with the narrow #4685 contract; this review is about isolating that change and covering the additional guardrail state it publishes.
seratch
changed the title
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Please remove unrelated commits
3 participants
Fixes #4685.
Problem
When a resumed
RunStateresolves an approved local function tool and a queued handoff from the same model response, the handoff target is committed before the resumed turn is persisted. If the client-managedSession.add_items()call then fails, the error propagates but nopending_session_writeis retained.Retrying the same live or JSON-restored state correctly continues with the target agent and does not re-execute the tool or the handoff, but it never repairs the missing outputs either. The result is two durable conversation views that never converge:
Sessionholds thechargeand handofffunction_callitems with neither output;RunStateandRunResult.to_input_list()hold both complete call/output pairs.Cause
update_run_state_after_resume()stores the raw transition, so_current_stepbecomesNextStepHandoff.save_resumed_turn_items()only suppliesresumed_write_stateforNextStepRunAgainorNextStepInterruption, so the handoff append bypasses the pending-write checkpoint introduced in #4630 and extended in #4650.NextStepHandoffis also outside theNextStepInterruption | NextStepRunAgain | NonetypeRunStatedeclares and serializes, so the failed state carries no resumable step at all.The same boundary drops the turn's tool input/output guardrail results: both runners publish them to
RunStateonly forNextStepInterruption.Fix
Treat the handoff as committed once the target agent is recorded, then normalize the remaining resumable transition to the existing
NextStepRunAgaincheckpoint before the fallible append. The currentpending_session_writemechanism then carries the exact function-tool and handoff output batch, andresume_pending_session_write()settles it on the next resume — before any further model call or local side effect. No new persisted step type and noRunStateschema change.At the same boundary, both runners now publish the completed tool guardrail results to
RunStatebefore the append can raise, so a retry reports them through the existing pre-model reconciliation instead of losing them.The streaming loop already set
NextStepRunAgainafter its handoff append; that assignment is now redundant and removed, since the shared helper normalizes it before the append.Behavior
RunResult.to_input_list(), the restoredRunState, and the next model input each contain exactly one ordered call/output pair forcharge-1andhandoff-1.resume_pending_session_write()checks.Tests
test_resumed_handoff_session_append_is_recovered_before_next_modelcovers the full sync/stream, same-mode/cross-mode, live/JSON, fail-before-commit/commit-then-raise matrix (16 rows). It fails onmainand passes here.