Fix Rust hook cleanup ordering and registration ownership - #114
Brian Krabach (bkrabach) wants to merge 3 commits into
Conversation
Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
|
Release ordering note: PR #114’s qualified head |
|
Combined integration is now pushed to PR #115 at |
|
Included in the combined Core 2.0.1 release through PR #115, merged at |
|
Closed as incorporated in merged PR #115; no separate merge was performed. |
Revised candidate — 1e3ef7c
This revision keeps the original purpose: public Rust session cleanup emits
session:endbefore serialized resource cleanup, unregister handles retain exact-registration ownership, and cancellation of a Rust hook waiter cancels its owned Python hook task. Runtime behavior is unchanged by this revision; it clarifies and tests the existing contract, drains two compatibility-test awaitables, and synchronizes the proposed 2.0.1 lock metadata.The terminal event is attempted once per successfully initialized lifetime, including when a cleanup waiter is cancelled. Cancellation is attempt-not-delivery: an asynchronous handler may be cancelled, handlers not yet reached may not receive the event, and a later cleanup does not replay it. An immediate retry may run resource callbacks before handler cancellation is observed; there is no handler-drain or ordering guarantee. The caller/host owns the cleanup deadline and any decision to retain or abandon the task. Normal CLI shutdown awaits cleanup; the deliberate
amplifier tool invokedeadline preserves completed tool output and may abandon cleanup. The contract does not promise that cancelled cleanup callbacks drain.Final exact-head qualification — engineering-qualified
All evidence below is for exactly
1e3ef7c3dba78cac74b1383115b143fef1db2265. This replaces the earlier pending status; the PR head has not drifted.uv.lockchecks passed. The strengthened cancellation probe passed 10/10 repetitions with no forbidden output.amplifier_core-2.0.1-cp311-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl, SHA-2563427a12b19458b7ffc27548754140832e6246a105d21644fed39176063002a30.Final official wheel smoke
The maintained smoke used the exact official wheel with
--skip-build; the pristine import preflight passed, and the installed native payload SHA-256 wasf41eeb31dde8490bf553fbd78782b99b9f6a62f6d9ea95b0a73bda38bff92c31. The fresh Foundation + real delegate + recipes execution completed successfully with the required marker. The installed-wheel Python suite was 1,144 passed, 1 skipped, 2 warnings, with no lifecycle-loop diagnostics.Short output from the exact smoke log:
Final paired lifecycle qualification
The paired run used the exact Core head with the paired Context Intelligence candidate and an actual backend at the recorded public revisions. 19/19 assertions passed. The natural lifecycle returned HTTP 202, emitted the terminal event before client close, and the specific
SessionEndEventgraph readback matched the accepted session. Wrong-auth and unavailable-endpoint controls completed locally, did not reopen workers, and did not falsely deliver. The cancelled-attempt plus retry followed the documented no-replay rule; its metadata remainedrunningrather than being represented as success.Final host/Core cleanup-ownership qualification
Against the official wheel and real installed Rust session:
releasedonly after that ordering completed.abandonedat its deadline with the expected warning; a later cleanup released resources without replaying the terminal recorder.Coverage boundary: this exercised the actual bounded helper named by
amplifier tool invoke, not a full user-facingamplifier tool invokecommand run. It makes no claim about provider/configuration side effects, tool execution, or outcome printing beyond the helper's documented post-result cleanup behavior.Historical validation retained
Public Rust session cleanup could close telemetry before
session:end, and duplicate hook names let unregister handles remove the wrong registration. Cleanup now awaits the terminal event before serialized module cleanup, each unregister handle owns its exact registration, and cancellation of a Rust hook waiter cancels its owned Python hook task.The terminal event is attempted once per initialized lifetime, including concurrent or cancelled cleanup. Partial initialization still permits resource cleanup; successful reinitialization starts a new lifetime. Provider/tool bridges and name-based unregister semantics remain unchanged.
amplifier,amplifier-app-cli, andamplifier-foundationmain found no Core Git overrides under[tool.uv.sources].Follow-up: eager coroutine wrapper gap
The broader pre-existing compatibility gap is intentionally not claimed fixed here. Source construction can create the Rust future before the Python wrapper is returned; closing an unstarted
emit()oremit_and_collect()wrapper does not cancel/defer already-running Rust work, andCheckedCompletorcan later target a closed loop. The shape tests now await their returned awaitables to drain that work.A narrow follow-up issue could not be filed because GitHub Issues are disabled for this repository. The follow-up scope is separate and is not a blocker introduced by this PR. Acceptance should cover closing an unstarted
emit()/emit_and_collect()with a registered handler, then advancing the loop and proving no dispatch and no closed-loop trace.Scope and release gate
Only these five files changed in the revision:
CONTRACTS.md, comments inbindings/python/src/session.rs, immediate-retry assertions inbindings/python/tests/test_cleanup_ownership.py, awaitable draining inbindings/python/tests/test_coroutine_compat.py, anduv.lock2.0.1 metadata. No new public content class was introduced; no downstream repository was changed.The evidence is engineering-qualified for the Core owner’s release decision. The PR is still open, not merged, not tagged, not published, and not deployed. Repository guidance reserves merging for the Core owner, and the actual wheel workflow is tag-only for publication; the successful non-tag wheel run explicitly skipped PyPI publication. No automatic release or downstream adoption is claimed.
The remaining merge gate is the required approving review (
REVIEW_REQUIRED/BLOCKEDin the current GitHub PR state), followed by the Core owner’s explicit merge decision. Do not treat this evidence publication as an approval, merge, tag, or release action.