Repository navigation
Add cooperative safe-boundary execution suspension - #67
Conversation
Brian Krabach (bkrabach)
left a comment
There was a problem hiding this comment.
Reviewed exact successor ccd958c against accepted main e04bba7. One correction remains before source clearance.
[P2] Preserve the mounted tool's execution lock on resume. In _settle_tool_batch (lines 3570–3572), every continued call receives a distinct ResumingTool adapter. With approved dispatch enabled, _execute_tool_only passes each adapter to wrap_tools, which keys the lock by id(raw) (dispatch.py line 563) and unwraps only GuardedTool. Two resumed calls to the same mounted tool therefore receive different locks; a resumed call and a not-started sibling also differ. They all invoke the same underlying mounted instance, losing the serialization ordinary calls receive. This is a source-level call-path finding, not a newly executed reproduction.
Please retain per-call continuations and current authority checks while selecting the lock from the trusted mounted execution identity. A focused regression should cover two resumed calls and a resumed/not-started pair, requiring peak execution of one, ordered original results, correct continuations, and no execute replay of an already-started call. Preserve the existing delegation exclusions and ordinary/nested concurrency contracts.
Accepted #68 authority behavior and the workflow/dependency inputs remain preserved. The retained four real-Core probes cover the conditional provider:error observer concern, so that is not an additional finding. Current Python 3.11–3.14 CI is green. Please also refresh the description with the exact successor's 156 focused Streaming checks, paired Live checks and four Core probes, keeping older evidence and downstream host-acceptance limits distinct. Normal maintainer approval remains separate; no bypass or production acceptance is implied.
Brian Krabach (bkrabach)
left a comment
There was a problem hiding this comment.
Focused successor source review at 1c2c283: the P2 finding is addressed; no remaining actionable finding in this delta.
Lock identity now unwraps only the engine-owned GuardedTool/ResumingTool adapters to the mounted instance, while actual execution retains each per-call continuation adapter. The nested guard chain forwards resume_execution without acquiring the same execution lock twice. Existing delegation exclusions, current authority checks and accepted #68 behavior remain unchanged.
The new real-Core batch tests distinguish the original peak-two defect from peak-one serialization for resumed/resumed and resumed/not-started calls, including aliases. They separately assert original ordered result IDs, exact continuation multiplicity and no execute replay. Removing the unsupported execution-start-order expectation is correct: result publication order remains asserted. Retained evidence reports 79 focused passes; the exact-head Python 3.11–3.14 matrix and CLA are green. This review is source re-review, not a new independent runtime run.
Ready for normal maintainer review. This comment is not an approving maintainer review or authorization to bypass protection. Live's accepted-Streaming update and Unified's durable host admission/result recovery remain separate future integration work; historical qualification retains its original identities.
Review follow-up: two suspension-path regression requestsReviewed head: Independent Astra ( 1. Exceptional resume setup can let a sibling outlive batch failureSource:
A bounded stdlib probe compiled the exact source method with fake tools: the first resumed sibling waited on an Event; the second had an unavailable resume owner. Observed: The harness then released and awaited the sibling. This is method-level control-flow evidence, not an installed-Core/public-entry reproduction. Requested regression: restore a two-call Suggested minimal direction: validate all needed resume owners before scheduling any sibling, and ensure unexpected setup/dispatch failures cannot leave started tasks unobserved. Do not equate local task cancellation with remote rollback or safe replay. Please choose the implementation consistent with the started-work settlement contract. 2. True-stream overflow recovery lacks the replacement-send suspension boundarySource: After awaiting A bounded exact-method probe used a fake first stream raising No real provider or network was used. Requested regression: request hold during overflow recovery/preflight/rate-limit wait; assert that no replacement provider send occurs until a durable checkpoint and authorized resume. Retain the exact recovered request, the spent recovery allowance and call accounting; do not repeat preparation on resume. Also retain the existing rule that recovery is forbidden after the first provider chunk. Scope and limitsThese are opt-in suspension-path findings, not demonstrated default execution regressions. No assertion is made that ordinary transcript-session resume implements frame resume, or that current hosts have enabled suspension. Native-Core exception/frame transport, crash-safe admission, competing claims and live-provider behavior remain separate qualification boundaries. The lock-wait admission definition, Stop-versus-checkpoint priority and queued-steering retention were not established as blocking defects by these reviews; they should not be conflated with the two focused requests above. No source changes or new runtime qualification were made in this review. |
Response to the two suspension-path findingsAddressed in successor 1. Exceptional resume setup and sibling settlementAll required Unexpected ordinary exceptions that escape resumed dispatch are converted to an explicit unknown, non-retryable result. New public-entry tests use a real Core coordinator, create an authentic
This preserves the conservative unknown-outcome classification for any first-time 2. True-stream overflow recovery and hold admissionThe streaming retry wrapper now invokes the existing prepared-request suspension boundary after recovery/preflight/rate-limit waits and before counting or sending the replacement. The checkpoint retains the exact recovered request, Six new public-entry cases cover hold during recovery, preflight, and rate-limit waiting, each with successful or failing replacement sends. They verify one send/call before checkpoint, explicit refusal while held, two total sends/calls after authorized resume, exact request equality, no repeated hook/context/budget/recovery/rate preparation, and closed iterators. A second overflow propagates instead of triggering a third send. Evidence and limits
The new tests directly call Python engine public methods with installed native Core 2.0.1, real hooks/context, and offline fake providers/tools. They do not establish native-kernel frame/exception transport, crash-safe or competing host claims, production Stop priority, or remote rollback. Main's Unified #945 host qualification remains separate. No live provider calls, host update, protection change, merge, or admin bypass occurred. All new task-owned environments were removed with retained evidence; historical failures and cleanup caveats remain preserved. Ready for independent re-review of these two changes. Normal maintainer approval remains required. Live #16's unpublished candidate and Foundation #447 are unchanged; Live's accepted-Streaming dependency step still waits for actual Streaming merge. |
|
Readiness's bounded follow-up review of exact I read the complete three-file delta, the production settlement/retry context and all eight new public-entry cases. Required resuming owners are constructed before sibling tasks are scheduled. Ordinary resumed-dispatch errors retain an unknown/non-retryable result so gather can settle other started siblings; cancellation/control exceptions remain distinct. The recovered stream checkpoint occurs after recovery and before retry accounting/send, with Independently read exact-head CI Keep native-kernel transport, crash-safe/competing claims, real Stop and Unified #945 acceptance separate. The historical failures remain failures. Normal upstream maintainer approval/order still applies; this note grants no merge, downstream publication or live-adoption authority. No duplicate review request is being made. Handoff key: readiness-streaming67-395594-source-ci-review-v1 |
Bounded re-review:
|
Admin merge completed — October 25Squash-merged reviewed head
|
Purpose and contract
Add an opt-in public safe-boundary suspension contract for explicit maintenance pause/resume. Already-started model/tool calls settle; versioned frames retain prepared requests, ordered results, accounting, and goal/finalization progress. Resume validates current history/configuration and a host-owned one-time admission callback. Cooperating tools continue through their original
resume_executionowner, never anexecutereplay. No coroutine serialization, new scheduler, automatic crash replay, or cancellation-as-safe-pause.Accepted main
e04bba7e/ #68 scoped dispatch and ordinary nested inference are preserved. The host still owns durable persistence, current permission/budget/Stop admission, session binding, and unknown-outcome recovery.Current successor:
395594ad9da191289ff75eb126bcf550a2b93694The response to the two suspension-path review findings records resume-owner preflight/settled sibling outcomes and prepared true-stream retry checkpointing. Eight new public-entry regressions fail on the prior production head and pass on this successor. An additional mixed suspended/unknown outcome probe passes on unchanged successor code. Exact-head CI 40025223645 passes on Python 3.11–3.14; Python 3.11 reports 552 passed / one skipped. Intermediate failures and the limits of direct public-engine tests with installed native Core remain explicit in that response. This delta awaits independent re-review and normal maintainer approval.
Previously qualified lock successor:
1c2c28343816f002fdcfb5393fad06df1bd92b84Addresses the P2 review on ccd958: resumed calls previously selected separate locks for per-call
ResumingToolobjects even when they invoked the same mounted tool.Only
dispatch.pyandtests/test_approved_dispatch.pychange fromccd958c. Lock selection now unwraps the engine's knownGuardedTool/ResumingTooladapters to the mounted identity. Execution retains each original per-call adapter and continuation. Exclusions, authority callbacks, ordinary/nested concurrency, Stop/budget paths, workflow, and dependency inputs are unchanged.For a
ResumingTool(GuardedTool(x))chain, the inner guard's existing__getattr__forwardsresume_executiondirectly tox; it does not invokeGuardedTool.executeor acquire the execution lock again. The identity test covers that chain and rewrapping; actual resumed-batch execution tests cover the ordinary mounted-tool path. No claim of arbitrary custom-wrapper support is made.New discriminating checks
bd042fe(production exactlyccd958c): five failures, no collection errors. Four real-Core_settle_tool_batchcases reach peak 2: resumed/resumed and resumed/not-started, each under one name or two aliases for the same instance. The fifth shows distinct locks for adapter chains.upstream-resumed-lock-final-cycle12-20261022: 79 passed, zero skipped across approved dispatch, execution suspension, cancellation sibling preservation, and dispatch context. All four cases now have peak 1, original ordered unique result IDs, exact continuation use, and noexecutereplay of started calls. Delegation exclusions and rewrapping remain covered.record_propertyunder xunit2; counter properties were retained and parsed. No hidden skip or weakened assertion for concurrency/results.Both requested independent
second-opiniondelta reviews returned on frozen1c2c283:openai/gpt-6-astra(e17eb38b1c924b81_self) andfable/claude-fable-5-1(2a86a756f3b94b83_self). Both found the narrow correction reviewable subject to exact-head CI, now observed. They reviewed the same source/evidence brief, not an exhaustive repository audit. Normal independent maintainer approval remains required; these reviews are not merge approval or an admin exception.Earlier evidence — retained, not relabeled
On
ccd958c, isolated requestupstream-review-guards-cycle7-20261022passed 156 Streaming checks and 105 paired Live checks / one existing compaction skip, with Live candidate7c5e2d6cand Foundation5265d6cc. Guest-owned offline/no-deps wheel installation and outside-source byte verification passed. CI 39826661275 was green for that earlier head (Python 3.11: 539 passed / one skipped).Also on unchanged
ccd958c,upstream-reviewed-model-held-probe-cycle9-20261022passed four real-Core probes for serial/parallel nested-model holds with normal/raising error observers. Conversation-visible held/non-retryable results, zero provider calls, and one prior settled effect were verified. Main closed that conditional observer concern; it was not repeated as a new gate. Cycle8's native-method source-export failure and its passive cleanup-binding diagnostic caveat remain retained.The initial PR description reported 218 earlier focused/adjacent checks and composed application tests. That historical report is not requalified or substituted for the exact-head evidence above.
Remaining boundaries
This is ready for normal maintainer review, not merged/released/adopted. Main's source re-review closes the P2 on exact
1c2c283; it is a source assessment, not an approving maintainer review. One normal review request is pending withmicrosoft/octo-made-amplifier-maintainers. The repository's required independent approval remains outstanding; no admin exception or bypass is requested.Live #16's corrected local
7c5e2d6cstays unpublished until Streaming is actually accepted/merged; its stale remote dependency-failure CI is not retried early. Foundation #4475265d6ccis unchanged and ready, with its existing maintainer request and prior reviews retained.Main owns Unified #945 integration and real-host competing claims, Stop during admission, crash after claim consumption, exact session binding, and recorded child-result recovery after completion-hook failure. Module fixtures do not prove those host guarantees. Unsupported opaque execution and unknown outcomes remain explicit; no live-provider or remote-cancellation guarantee is claimed.