Skip to content

Fix Rust hook cleanup ordering and registration ownership - #114

Closed
Brian Krabach (bkrabach) wants to merge 3 commits into
mainfrom
fix/core-lifecycle-ownership-20260923
Closed

Brian Krabach (bkrabach) wants to merge 3 commits into
mainfrom
fix/core-lifecycle-ownership-20260923

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Revised candidate — 1e3ef7c

This revision keeps the original purpose: public Rust session cleanup emits session:end before 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 invoke deadline 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.

  • Exact-source gates: 396/396 manifest hashes; 1,144 Python tests passed, 1 skipped, 2 existing deprecation warnings; no closed-loop diagnostic or leaked-task output. Rust passed 491 unit tests, 13 integration tests, and 19 doctests. Clippy, formatting, and uv.lock checks passed. The strengthened cancellation probe passed 10/10 repetitions with no forbidden output.
  • GitHub Rust Core CI passed all jobs: Rust kernel, Node binding, and Python 3.11, 3.12, and 3.13. Proto Sync Check passed both jobs. CLA is green.
  • The exact-head official wheel run passed all six platform builds; the PyPI publication job was skipped. The Linux ARM64 wheel is amplifier_core-2.0.1-cp311-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.whl, SHA-256 3427a12b19458b7ffc27548754140832e6246a105d21644fed39176063002a30.

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 was f41eeb31dde8490bf553fbd78782b99b9f6a62f6d9ea95b0a73bda38bff92c31. 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:

[recipe] ✅ Recipe completed: core-release-smoke
State:          completed
Final output:   CORE_RELEASE_SMOKE_OK
[PASS]  SMOKE TEST PASSED
[PASS]  No crashes, no tool failures, no timeout

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 SessionEndEvent graph 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 remained running rather than being represented as success.

Final host/Core cleanup-ownership qualification

Against the official wheel and real installed Rust session:

  • Normal CLI handoff waited behind the earlier terminal hook, then delivered the later recorder before resource closure.
  • Cooperative Foundation handoff waited for save, terminal hook, terminal recorder, and cleanup, returning released only after that ordering completed.
  • The bounded tool-cleanup helper deliberately returned abandoned at its deadline with the expected warning; a later cleanup released resources without replaying the terminal recorder.
  • The related app-cli tests passed 35 tests.

Coverage boundary: this exercised the actual bounded helper named by amplifier tool invoke, not a full user-facing amplifier tool invoke command 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.

  • Historical Rust Core CI: all five jobs passed, including Rust, Node and 1,139 Python tests on each Python 3.11–3.13. Fourteen inert smoke-script regressions pass; the original script fails nine.
  • Historical official wheel build: all six platform builds passed; PyPI publication was skipped. The Linux ARM64 wheel was checked against the prior candidate.
  • The maintained CLI smoke passed on 23 September 2026 using that prior candidate wheel; this is historical validation only and is superseded for release qualification by the exact-head smoke above.
  • The paired lifecycle integration with Context Intelligence #127 previously accepted the natural terminal event before client closure; later graph readback matched HTTP, local, and server-spool payloads. Bad-auth and unavailable-endpoint controls retained local events, indexed none, and closed without reopening workers.
  • Fresh checks of amplifier, amplifier-app-cli, and amplifier-foundation main 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() or emit_and_collect() wrapper does not cancel/defer already-running Rust work, and CheckedCompletor can 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 in bindings/python/src/session.rs, immediate-retry assertions in bindings/python/tests/test_cleanup_ownership.py, awaitable draining in bindings/python/tests/test_coroutine_compat.py, and uv.lock 2.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 / BLOCKED in 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.

@bkrabach
Brian Krabach (bkrabach) marked this pull request as ready for review September 23, 2026 04:43
Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Release ordering note: PR #114’s qualified head 1e3ef7c3dba78cac74b1383115b143fef1db2265 still resolves affected PyO3 0.28.2. PR #115 is the separate patched candidate and is now fully qualified at 846ea25490d541661838d8cbb3788d259bfbf1f9. Recommendation: land the security candidate as 2.0.1 first, then rebase and requalify this lifecycle PR as a follow-up version. PR #114 alone is not a PyO3 security remediation.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

Combined integration is now pushed to PR #115 at 0f5181ec774ad04e97035c5ac8e66d0344a5e613 for the single approved Core 2.0.1 release. Your lifecycle commits remain separately attributed and this PR remains open; no source-head change or closure is requested. Fresh official wheel, CI, smoke, and host/pair gates are pending on #115 before any merge/tag/publication.

Brian Krabach (bkrabach) added a commit that referenced this pull request Sep 23, 2026
Core 2.0.1 combines the patched PyO3 dependency graph, lifecycle ownership fixes, and pinned Actions updates.

Refs: #113, #114, #115

Generated with Amplifier

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Included in the combined Core 2.0.1 release through PR #115, merged at 6ed28858ebb4ce3ca3939f9be96dc091f25822cb. The lifecycle changes from this PR are present in the merged release tree, alongside the security dependency and Actions updates. Closing this PR as incorporated; it was not separately merged, and its branch is retained.

@bkrabach

Copy link
Copy Markdown
Collaborator Author

Closed as incorporated in merged PR #115; no separate merge was performed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants