session: remove legacy teardown aliases - #413
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughSession teardown was consolidated around ChangesSession teardown contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant SessionDebugBackends
participant Journal
participant BackendStores
Session->>SessionDebugBackends: _finalize_debug_backends()
SessionDebugBackends->>Journal: write clean-close marker
SessionDebugBackends->>BackendStores: close backend resources
SessionDebugBackends-->>Session: preserve read-only postmortem view
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the lifecycle teardown consolidation and its repository references. The context-manager force-stop behavior, unified backend finalization, read-only post-stop inspection, and retry coverage are aligned with the new single-verb API. No blocking findings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/docs/test_route_contracts.py (1)
181-189: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that removed lifecycle aliases are absent from the reference page.
The new positive marker only proves that the replacement text exists; stale
shutdown(),close(), ordestroy()examples could remain and the test would still pass.Proposed assertions
for marker in ( "`stop(force=True)`", "There are no separate public close/destroy phases", "`session.journal.read()`", "`session.export_debug_bundle(path)`", "`async with session:`", "record_to", ): assert marker in text, f"docs/reference/session-lifecycle.md missing {marker!r}" + for stale in ( + "session.shutdown()", + "Session.shutdown()", + "session.close()", + "Session.close()", + "session.destroy()", + "Session.destroy()", + ): + assert stale not in textAs per coding guidelines, add or update tests for every behavior change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/docs/test_route_contracts.py` around lines 181 - 189, Update the lifecycle documentation contract test around the marker loop to also assert that removed aliases and examples—such as `shutdown()`, `close()`, and `destroy()`—are absent from the reference page, while retaining the existing positive marker checks.Source: Coding guidelines
tests/test_public_api.py (1)
222-234: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProtect the machine-visible deprecation guarantee.
docs/public-api.mdstill states that stable symbols normally carry a machine-visible deprecation signal, but this contract test does not assert that wording. A future edit could weaken that guarantee while all current assertions still pass.Proposed test assertions
assert "`settings=`" in section + assert "machine-visible deprecation signal" in section assert "pre-release" in section assert "ownership ambiguity" in section + assert "params=" in sectionAs per coding guidelines, add or update tests for every behavior change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_public_api.py` around lines 222 - 234, Update test_public_api_documents_deprecation_and_removal_policy to assert that the Deprecation & Removal Policy section explicitly states the machine-visible deprecation signal guarantee for stable symbols. Add an assertion matching the documented wording, alongside the existing settings, pre-release, and ownership ambiguity checks.Source: Coding guidelines
src/easycat/session/_session.py (1)
1212-1226: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSession-level retry contract for backend finalization failure is untested.
_finalize_debug_backends()documents itself as "safe to call multiple times," which holds at theSessionDebugBackends.destroy()level (guarded by_destroyed, verified by the newtest_debug_backends_destroy_retries_after_backend_close_failure). However, ifdestroy()raises insidestop()(line 1194), the exception propagates pastself._mark_closed()(line 1195) without being caught, leaving_closed=Falsewhile_stoppingis still reset toFalsein thefinallyblock. A caller must know to retrystop()itself to eventually flip_closedand unblockwait_closed()waiters — this full-stop()retry path (as opposed to the isolatedSessionDebugBackendsretry) isn't exercised by any test in this changeset.Given this PR is establishing
stop(force=...)as the sole, guaranteed teardown contract, an integration test that forces_finalize_debug_backends()to fail once (e.g. via aTrackingJournalsubclass like the ones intest_debug_backends.py) and asserts that a secondawait session.stop()succeeds and unblockswait_closed()would close this coverage gap.Do you want me to draft this test?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/easycat/session/_session.py` around lines 1212 - 1226, Add an integration test covering the full Session.stop() retry contract: force _finalize_debug_backends() to fail once using a tracking backend/journal fixture, assert the first stop raises while the session remains open, then verify a second await session.stop() succeeds, marks the session closed, and unblocks wait_closed().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/easycat/session/_session.py`:
- Around line 1212-1226: Add an integration test covering the full
Session.stop() retry contract: force _finalize_debug_backends() to fail once
using a tracking backend/journal fixture, assert the first stop raises while the
session remains open, then verify a second await session.stop() succeeds, marks
the session closed, and unblocks wait_closed().
In `@tests/docs/test_route_contracts.py`:
- Around line 181-189: Update the lifecycle documentation contract test around
the marker loop to also assert that removed aliases and examples—such as
`shutdown()`, `close()`, and `destroy()`—are absent from the reference page,
while retaining the existing positive marker checks.
In `@tests/test_public_api.py`:
- Around line 222-234: Update
test_public_api_documents_deprecation_and_removal_policy to assert that the
Deprecation & Removal Policy section explicitly states the machine-visible
deprecation signal guarantee for stable symbols. Add an assertion matching the
documented wording, alongside the existing settings, pre-release, and ownership
ambiguity checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a9e064e-af1a-4fc8-a5fb-b038e5ec264d
📒 Files selected for processing (21)
AGENTS.mdCLAUDE.mdREADME.mddocs/public-api.mddocs/reference/session-lifecycle.mddocs/teaching/15-operate-in-production/README.mddocs/teaching/15-operate-in-production/main.pysrc/easycat/_provider_helpers.pysrc/easycat/runtime/DURABILITY.mdsrc/easycat/session/_session.pysrc/easycat/transports/_base.pytests/docs/test_route_contracts.pytests/session/test_debug_backends.pytests/session/test_journal_sink.pytests/session/test_record_to.pytests/session/test_session_journal_accounting.pytests/session/test_session_lifecycle_teardown.pytests/teaching/test_ladder_index.pytests/test_deprecations.pytests/test_dx_helpers.pytests/test_public_api.py
|
Review findings — three references to the removed aliases survived the sweep:
Everything else checked out: CLAUDE.md / AGENTS.md / session-lifecycle reference / DURABILITY.md / README / public-api docs are all updated, no functional callers of the removed methods remain anywhere in the tree, and the |
1 participant
Problem
Sessionexposed four teardown vocabularies:stop(),shutdown(),close(), anddestroy(), plus two low-level wrapper names (_close()/_destroy()). The deprecated aliases preserved ambiguity over cancellation strategy, clean-close markers, physical backend teardown, and whether post-stop inspection remained valid.EasyCat is still pre-release, so retaining these compatibility paths costs more architectural clarity than it provides.
Changes
Session.shutdown(),Session.close(), andSession.destroy().Session._close()andSession._destroy()wrappers.stop(force=False|True)the single public teardown verb._finalize_debug_backends()to describe its ownership.SessionDebugBackendscollaborator.Breaking change
Callers must replace:
await session.shutdown()withawait session.stop(force=True)session.close()/session.destroy()withawait session.stop()orasync with session:There is intentionally no compatibility alias during the pre-release period.
Validation
ruff format --check .ruff check .A local all-
tests/sessionserial run reached an unrelated executor-using session-action test and hit the existing Python 3.12.13 default-executor fixture-teardown hang. The directly affected suites pass; repository CI provides the clean-environment full validation.Summary by CodeRabbit
shutdown(),close(), anddestroy().await session.stop()the single public teardown entry point (includingforcesemantics andasync withbehavior).settings=vsparams=guidance.