fix(sessions): make session teardown atomic so tmux and the registry cannot diverge

[tedswinyar]

delete_session could leave the tmux session and the terminal registry out of sync. Two separate symptoms were observed: a tmux session surviving a shutdown with no registry row behind it, and a window where a session showed as present in one call and gone in the next. Both trace back to the same teardown logic.

The old code captured whether the session was alive before the per-terminal cleanup loop, then killed the tmux session only if that stale snapshot said it was alive, and did not verify the kill worked. Killing the last terminal window can itself make tmux drop the whole session, so the pre-loop snapshot is stale by the time it is used. And because kill_session returned success without confirming the session was actually gone, a failed or slow kill was reported as a clean shutdown while the tmux session lived on.

The fix. TmuxClient.kill_session now polls until tmux confirms the session is gone (bounded to two seconds) and returns true only then, so its return value is trustworthy. delete_session re-checks liveness after the terminal loop instead of trusting a stale snapshot, and relies on the verified kill result rather than polling a second time. If the kill cannot be confirmed, it raises and deliberately leaves the registry rows in place, so the operation reports a real failure instead of a false success and a re-run can reconcile the surviving session. Only once the session is confirmed gone does it sweep any leftover registry rows, so a caller can never observe a registry entry for a dead session. Deleting an already-dead session remains a safe no-op, the existing return shape and raise-on-error contract are preserved, and re-running on a half-torn-down session reconciles rather than erroring.

A known limitation. This verified-kill guarantee holds for the tmux backend. The herdr backend's kill_session returns its subprocess exit code without a liveness check, so on that backend the teardown still trusts the bool without confirmation. That is documented in the code and tracked as a separate follow-up. The tmux backend is the default and the one these symptoms were seen on.

Tests. A faithful in-memory tmux backend models the real failure modes: killing the last window dropping the session, a kill that lags before tmux reaps the session, and a kill that fails silently. The suite asserts that after a successful delete_session the tmux session is gone and no registry rows remain, that a failed kill is surfaced rather than swallowed, and that re-running on a half-torn-down session reconciles. The regression tests were confirmed to fail against the pre-fix code and pass against this change, so they exercise the real bug rather than mocking around it. Relevant suites pass (session teardown, session service, tmux client, plugin events).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 90.75908% with 28 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@752be53). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...li_agent_orchestrator/services/terminal_service.py	84.61%	14 Missing ⚠️
src/cli_agent_orchestrator/clients/tmux.py	93.65%	8 Missing ⚠️
...cli_agent_orchestrator/services/session_service.py	92.72%	4 Missing ⚠️
src/cli_agent_orchestrator/backends/base.py	50.00%	1 Missing ⚠️
...rc/cli_agent_orchestrator/backends/tmux_backend.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #498   +/-   ##
=======================================
  Coverage        ?   90.97%           
=======================================
  Files           ?      180           
  Lines           ?    23504           
  Branches        ?        0           
=======================================
  Hits            ?    21383           
  Misses          ?     2121           
  Partials        ?        0           

Flag	Coverage Δ
unittests	90.97% <90.75%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fanhongy]

Summary

The happy-path and focused regression tests pass, but the changed teardown still has three paths that can leave a live tmux session without registry rows, plus one race that reports failure after the session is already gone. These contradict the PR's core verified-kill and reconciliation contract.

Findings

P1: Lookup errors are accepted as proof that tmux is gone

At /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/clients/tmux.py:519, the new verification loop treats not self.session_exists(...) as confirmed absence. However, /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/clients/tmux.py:555 catches every lookup exception and returns False. The post-terminal check at /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/services/session_service.py:182 has the same ambiguity. A transient tmux socket/libtmux error either before the kill or during its first verification lookup therefore skips/accepts the kill, after which line 196 deletes registry rows and the API reports success. I reproduced the verification case with server.sessions.get returning a session for the kill and then raising OSError; kill_session returned True.

Use a strict lookup for teardown verification that distinguishes confirmed absence from query failure, and return failure/raise on lookup errors. Add tests for an exception during the post-loop liveness check and during the verification poll.

P1: A failed kill has already deleted the live terminals' registry rows

The failure branch at /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/services/session_service.py:185 says rows are left intact, but line 173 has already called delete_terminal for each row. That function ignores a False kill_window result at /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/services/terminal_service.py:1262 and unconditionally deletes the row at line 1275. When both the window kill and subsequent session kill fail, delete_session raises but leaves the live agent/session unregistered. A real-SQLite reproduction ended with tmux_session_alive=True and registry_rows_after_failure=[].

Defer registry deletion until the session kill is confirmed, or retain/restore the original rows on failure. Extend the failed-kill regression test to assert that rows for surviving windows remain.

P1: The new session-wide sweep can delete a concurrently recreated session's rows

The unconditional bulk delete at /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/services/session_service.py:196 is not synchronized with creation. POST /sessions can reuse the name after teardown observes/confirms the old session absent, create the new tmux session at /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/services/terminal_service.py:231, and persist its row at line 300 before the old delete reaches the sweep. The sweep then removes the new row while leaving the new tmux session alive. The delete endpoint runs in a worker thread, and there is no shared session lifecycle lock. A deterministic interleaving reproduced a successful delete with new_tmux_session_alive=True and no registry rows.

Serialize create/add/delete operations with a per-session lifecycle lock, or use a session generation/identity so cleanup can only delete rows belonging to the lifecycle it started tearing down. Add a concurrent same-name recreate test.

P2: Disappearance between the existence check and kill is reported as a failed kill

At /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/services/session_service.py:183, any False from kill_session is interpreted as "still exists." The backend contract at /tmp/cao-pr-review-awslabs-cli-agent-orchestrator-pr-498/repo/src/cli_agent_orchestrator/backends/base.py:92 explicitly also uses False for "not found." If tmux drops the session after line 182 but before TmuxClient.kill_session looks it up, the service raises HTTP 500 and skips the residual-row/env cleanup even though teardown has succeeded. A reproduction ended with the tmux session absent, one residual registry row, and the new RuntimeError.

Make the kill operation idempotently succeed when the target is already absent, or perform a strict follow-up existence check before raising. Add a test for disappearance between the post-loop check and kill lookup.

Validation

• uv run pytest test/clients/test_tmux_client.py test/services/test_session_service.py test/services/test_session_teardown_atomic.py test/services/test_plugin_event_emission.py -q -> 103 passed.
• git diff --check 77befe84e69c0785c25ec024bb7e6391202cf305...HEAD -> passed.
• Deterministic in-memory/SQLite harnesses reproduced all four findings without modifying the checkout.
• Checkout remained clean at f1eececf7c08c4b5ce43232922ad472076c6feee.

[Copilot]

Pull request overview

This PR hardens session teardown so the tmux session lifecycle and the terminal registry can’t diverge during delete_session, addressing races where killing windows can implicitly drop sessions and where session kills may lag or fail silently.

Changes:

• Make TmuxClient.kill_session verify session disappearance (bounded polling) and return True only when tmux confirms the session is gone.
• Update delete_session to re-check liveness after per-terminal teardown, rely on the verified kill_session result, and sweep any leftover registry rows only after the session is confirmed gone.
• Add/extend tests to model real teardown failure modes (last-window drop, kill lag, silent kill failure) and assert invariants across tmux + SQLite registry.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/cli_agent_orchestrator/clients/tmux.py	Makes kill_session return a trustworthy success value by polling until tmux reports the session is gone (bounded).
src/cli_agent_orchestrator/services/session_service.py	Re-checks session liveness after terminal loop, raises on unverified kill, and reconciles registry rows post-confirmed kill.
test/clients/test_tmux_client.py	Updates and adds tests for verified-kill behavior (success path + “session survives” path).
test/services/test_session_service.py	Updates mocked unit tests to account for the new registry sweep and verified kill_session semantics.
test/services/test_plugin_event_emission.py	Updates delete-session event emission test wiring to include the new sweep step.
test/services/test_session_teardown_atomic.py	Adds integration-style regression tests with a faithful in-memory tmux model plus a real SQLite registry.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gutosantos82]

PR Review: #498 — fix(sessions): make session teardown atomic so tmux and the registry cannot diverge

Summary

This PR fixes a real teardown bug — delete_session trusted a pre-loop liveness snapshot and an unverified kill_session bool, letting tmux and the terminal registry diverge. The fix (verified kill with a bounded 2s poll, post-loop liveness re-check, raise-on-unconfirmed-kill, sweep-only-after-confirmed-gone) is well designed, tightly scoped, and its regression tests genuinely fail against pre-fix code — all six PR-body claims were dynamically verified. However, a maintainer review at this exact head requested changes with three P1 findings plus one P2, none of which are addressed (no commits since), and our independent probes confirmed the two we tested. Recommendation: address the open maintainer findings; the additional items below are smaller and mostly documentation/contract accuracy.

Important (should fix)

• [consistency] src/cli_agent_orchestrator/backends/base.py:85-93 — The TerminalBackend.kill_session ABC docstring still reads "True if session was killed, False if not found," but the tmux implementation now also returns False when a dispatched kill cannot be confirmed within the 2s bound, and delete_session branches on that new meaning. A future backend author implementing the documented (old) contract would silently break the atomicity guarantee. Update the docstring to: "True once the session is confirmed gone; False if not found OR the kill could not be confirmed."
• [conventions] src/cli_agent_orchestrator/services/session_service.py:132, test/services/test_session_teardown_atomic.py:1, test/services/test_session_service.py (TestDeleteSession docstring) — References to internal tracker slug caom-9k8 leak a non-public issue id. The same function references issues in the repo's public form (issue #248 at lines 58 and 198), and caom-9k8 appears nowhere else in the repo. Replace with the public GitHub issue number or describe the follow-up inline; also consider linking the herdr follow-up issue so the disclosed limitation is verifiably tracked.
• [conventions] CHANGELOG.md — No [Unreleased] → ### Fixed bullet for a user-facing teardown fix that also changes the error contract (unconfirmable kill now raises). Recent fixes all carry a (#NNN) bullet; add e.g. "session teardown is now atomic; TmuxClient.kill_session confirms the session is gone before returning True (#498)."

Nits (optional)

• [tests] test/services/test_session_teardown_atomic.py:129 — Dead import from ...registry import _backend # noqa: F401; the fixture only calls set_backend(None). Remove the line (the noqa masks it).
• [tests] src/cli_agent_orchestrator/clients/tmux.py (~line 532) — The time.sleep retry iteration inside the verify loop is the one uncovered changed line; the new unit tests hit only the immediate-success and timeout=0 branches. Add a TestKillSession case with side_effect=[mock_session, mock_session, None] and a patched sleep so the bounded retry loop — the heart of "True only once confirmed gone" — is exercised against the real primitive.
• [correctness] src/cli_agent_orchestrator/clients/tmux.py:514-532 — kill_session now blocks synchronously up to 2s on a lagging/failed kill for all callers (terminal_service.py:461, flow_service.py:260, herdr_inbox_service.py:469/716), not just delete_session. Happy path is unaffected; worth confirming none of those call sites run directly on the asyncio event loop.
• [conventions] test/services/test_session_teardown_atomic.py:14 — Internal jargon "the bead"/"the bead demands" won't be understood by OSS contributors; reword to "the invariant the fix requires."
• [consistency] docs/terminal-lifecycle.md:19-22 — Pre-existing drift (not introduced here): the doc claims session-level shutdown "kills windows directly and does not snapshot," but delete_session tears down via delete_terminal, which does snapshot. Fine to fix in a follow-up.

Tests

Strong. The new test_session_teardown_atomic.py (7 tests) drives the real delete_session against a faithful in-memory tmux backend plus a real per-test SQLite registry, covering every fix invariant: happy path leaves no orphan in either store, kill-lag is confirmed before success, silent kill failure surfaces as RuntimeError, re-run reconciles a half-torn-down session, leftover rows are swept, killing the last window dropping the session is handled without a double kill, and deleting an already-dead session is a safe no-op. Existing test_session_service.py tests were correctly updated for the new sweep, mock fidelity in test_kill_session_success matches the real double-lookup, markers are correct under asyncio_mode=strict, and changed-line coverage is 100% on session_service.py / 95% on tmux.py (only the retry-sleep line uncovered — see Nits).

Verification

Baseline: the four change-selected suites pass on PR code — 103 passed, 0 failed. Per-claim:

• ✓ VERIFIED — kill_session returns True only after confirmed absence, bounded 2s (never-dying mock polled exactly 2.00s → False; immediately-dying → True in <1ms).
• ✓ VERIFIED — delete_session re-checks liveness after the terminal loop (pre-fix snapshotted before it; kill_session_calls==0 test fails pre-fix).
• ✓ VERIFIED as coded, wording overstated — a failed kill raises and skips the sweep, but "registry rows left intact" only holds for rows the terminal loop didn't already delete (see Prior feedback).
• ✓ VERIFIED — after a successful delete, the tmux session is gone and no registry rows remain (real SQLite).
• ✓ VERIFIED — re-run on a half-torn-down session reconciles.
• ✓ VERIFIED — regression tests genuinely fail against pre-fix code (4 of 7 atomic tests + 1 new tmux unit test fail on the parent commit).
  Both maintainer concerns probed were CONFIRMED with deterministic reproductions (lookup-error-as-absence; rows-deleted-before-failed-kill). Net: the PR is a strict improvement over pre-fix code, but the guarantee has real gaps at its edges exactly where the maintainer said.

Verdict

Request changes — the core fix is sound and dynamically verified, but a maintainer's CHANGES_REQUESTED review at this exact head lists three P1 teardown gaps plus one P2 race that remain unaddressed (two independently reproduced here), and the ABC contract docstring no longer matches the new kill_session semantics.

[gutosantos82]

PR Review: #498 — fix(sessions): make session teardown atomic so tmux and the registry cannot diverge (re-review at fd74114)

Summary

This head is a substantial rework (13 files, +3414/−222, squashed force-push on 2026-08-04) responding to fanhongy's CHANGES_REQUESTED (3×P1 + 1×P2) and our prior Request-changes report. All four maintainer findings are genuinely fixed and all of our prior items are addressed — each verified dynamically, not just by reading (see Verification). The design is now considerably stronger: a strict session_exists_strict that distinguishes "gone" from "couldn't tell" (fail-closed via TmuxLookupError), a per-session-name lifecycle lock serializing create vs teardown, an id-scoped reconciliation sweep, and a destructive-work-only-after-confirmed-kill teardown ordering. Tests are excellent — 22 atomic-teardown tests (18 fail on pre-fix code) plus 27 strict-lookup tests that drive real tmux servers on private sockets. However, the create-path refactor that moved the tmux create + registry write into one locked closure dropped the incremental session_created/window_created flag assignment: if db_create_terminal (or set_session_env) raises inside the closure after the tmux session is created, the flags are never assigned and the failure path skips the session kill, leaving a live orphan tmux session with no registry row. I reproduced this with a probe test against the PR head; pre-PR code killed the session in this case. The PR's own comments call SQLite database is locked "an ordinary outcome under CAO's concurrent writers," so the trigger is realistic. One targeted fix and this is merge-ready.

Important (should fix)

• [correctness — regression] src/cli_agent_orchestrator/services/terminal_service.py (_create_session_or_window_locked, ~line 355-425) — Pre-PR, session_created = True was set immediately after get_backend().create_session(...), so any later failure (env persist, DB row write) triggered the cleanup path's kill_session. Now the closure returns (window_name, created_session, created_window) only on full success; an exception from set_session_env or db_create_terminal after the tmux create propagates out of asyncio.to_thread with the outer flags still False, so the except cleanup neither kills the new session nor (for the new_session=False path, harness-control#186) the new window. Probe-confirmed: with db_create_terminal patched to raise, the fake backend still reported the session alive after create_terminal failed — a live tmux session invisible to the registry, i.e. the PR's own headline divergence. Fix options: roll back inside the closure (kill the session/window under the lock before re-raising), or report partial progress via a mutable holder assigned incrementally.
• [docs] docs/terminal-lifecycle.md:11-23 — The deletion-paths table and the paragraph below it say session-level shutdown "kills windows directly and does not snapshot" (cao shutdown --session → "Snapshot saved? No"). This head makes snapshotting an explicit, documented step 2 of delete_session (capture_terminal_snapshot before the kill), so the doc now contradicts the code it describes. This was pre-existing drift we previously waved to a follow-up, but the PR now restructures exactly this behavior, and CODEBASE.md's documentation-maintenance rule makes the update in-scope here.

Nits (optional)

• [portability] src/cli_agent_orchestrator/clients/tmux.py (_SOCKET_MISSING_STDERR_MARKERS) — "no such file or directory" matches strerror(ENOENT) inside tmux's "error connecting to %s (%s)" message, and strerror output is locale-dependent. Under a non-English LC_MESSAGES the marker misses and the missing-socket case lands in the final raise branch — fail-closed (safe direction, per the module's own philosophy), but teardown reconciliation of a dead-server/missing-socket host would raise forever there. Consider forcing LC_ALL=C for the classification path or noting the limitation in the (already thorough) comment block.
• [tracking] backends/base.py, session_service.py, CHANGELOG.md — The herdr kill_session/session_exists_strict limitation is now well documented in code (good), but I could find no GitHub issue tracking the follow-up (gh issue search came up empty). Prior review asked for a verifiable tracker; a one-line issue + reference would close that.
• [concurrency — observation] terminal_service.delete_terminal — Single-terminal deletion takes no lifecycle lock, so it can interleave with a delete_session of the same session (double-dismantle is idempotent; a last-window kill mid-teardown resolves as confirmed-gone). Interleavings look benign, but the lock's coverage boundary is worth one sentence in session_lock.py's docstring.
• [performance] flow_service.py:300, terminal_service.py:586 — kill_session now blocks up to ~2s (verify poll, subprocess per iteration) and both call sites run directly on the asyncio event loop (execute_flow's recycle path; create_terminal's failure cleanup). Bounded and rare (error/recycle paths only), but a to_thread wrap would be cheap. Carried over from prior review; unaddressed.

Tests

Outstanding — the strongest part of the PR. test_session_teardown_atomic.py (22 tests, 1229 lines) drives the real delete_session/create_terminal against a faithful in-memory tmux backend + real per-test SQLite, including real-thread concurrency tests with entry-gate events (create-vs-teardown both orders, teardown-vs-teardown overlap detection, different-names concurrency, self-deadlock guard, no-plugin-code-under-lock, tail-failure event preservation). test_tmux_session_exists_strict.py (27 tests, 670 lines) is genuine integration: it spawns real tmux servers on private sockets and exercises live/absent/killed-server/unlinked-socket/unreadable-dir cases plus the platform detector ladder (proc, lsof, ps), lsof time-bounding, and exception translation. 18 of 22 atomic tests fail against pre-fix code — they exercise the real bug. Coverage on changed modules: session_lock 100%, session_service 95%, tmux.py 92% (codecov patch 92.96%). The gap: no test covers a mid-closure failure in the create path — exactly where the regression above hides; the fix should land with one.

Verification

Worktree at fd74114, main-repo venv with PR src on PYTHONPATH, CAO_HOME_DIR redirected (sandbox ~/.aws is read-only).

• ✓ Five change-selected suites: 169/169 pass (atomic teardown, strict-exists, tmux client, session service, plugin events).
• ✓ Broad sweep test/services test/clients test/backends: 2418 passed, 5 failed — all 5 in test_learning_enabled_flag.py, reproduced identically on merge-base 752be53 → pre-existing/environmental, unrelated. GitHub CI fully green at this head (3.10/3.11/3.12, security, quality).
• ✓ Regression tests genuinely bite: 18/22 atomic tests fail against pre-fix source.
• ✓ fanhongy's P1 repro re-run against new code: lookup error during verify → False → raise, rows intact (fail-closed confirmed for OSError, ENOENT+unknown-liveness, unlinked-socket+live-server; ECONNREFUSED correctly confirmed absent).
• ✗ NEW regression probe: db_create_terminal raising inside the locked closure → create_terminal fails but the created tmux session survives with no registry row (assert failed on PR head). Pre-PR code set session_created = True before the DB write and killed the session on this path.
• ✓ dispatch_plugin_event(None, ...) confirmed a no-op, so registry=None row-deletes plus deferred emission cannot double-fire events.

Verdict

Request changes — one probe-confirmed regression: a DB-write failure inside the new locked create closure leaves an orphan live tmux session (the failure path loses the session_created/window_created signal), which is precisely the tmux/registry divergence this PR eliminates elsewhere. Everything else — all four maintainer findings and every prior-report item — is fixed and dynamically verified; with a rollback inside the closure (plus a doc touch-up to terminal-lifecycle.md), this flips to Approve.

[tedswinyar]

Thanks @gutosantos82 — the create-path regression was real and is fixed. Four commits on top of fd74114, all in terminal_service.py plus its test file and one doc.

I reproduced it empirically before changing anything, since a probe is more convincing than reading the diff. Driving the real create_terminal with db_create_terminal raising database is locked:

code	result
fd74114 (this PR's previous head)	LIVE TMUX SESSIONS AFTER FAILURE: {'cao-probe'} — orphan, no registry row
752be53 (pre-PR base)	(none) — session correctly killed
2f0baee (now)	(none)

So it was a genuine regression, exactly as you described: the closure returned its flags only on success, the outer except keyed teardown off those flags, and a raise after the backend create left a live session invisible to the registry.

The fix (cfe32b6) rolls back inside the closure, still holding the lifecycle lock — your option (a). We chose it over reporting partial progress outward because rolling back after releasing the lock reopens the window the lock exists to close: between release and kill, another thread can acquire the name and legitimately succeed (a new_session=False create adding a window to what it sees as a live session, or a teardown plus a fresh create rebuilding the name). The late kill would then destroy an incarnation this call doesn't own, leaving its row pointing at nothing — the same divergence with the polarity flipped. Under the lock the name goes free → free with no observable intermediate state.

Both branches are handled and they aren't the same teardown: new_session=True kills the session and clears its forwarded env (so a --env secret can't linger or bleed into a reuse of the name); new_session=False kills only the window this call added, since the session pre-existed and its other terminals must survive. set_session_env also moved inside the guarded region — it previously ran before the row write with no rollback covering it, so a failure there leaked both the session and the env mapping.

Two regression tests in test_session_teardown_atomic.py, one per branch, in that file's existing idiom (real create_terminal against the in-memory backend plus a real per-test SQLite registry). Both fail against fd74114 and pass now — verified in both directions.

Adversarial review found two things worth telling you about, and both turned out to be pre-existing gaps my commits had overclaimed away rather than new bugs. I've corrected the prose (1bd108d) and filed the underlying issues rather than quietly expanding this PR:

1. A failure inside the backend create isn't covered. TmuxClient.create_session lands the session at server.new_session(...) and only then reads session.windows[0].name — a fresh list-windows fetch that can raise IndexError, or the method's own ValueError. That leaks a session the closure never learns about. Pre-fix(sessions): make session teardown atomic so tmux and the registry cannot diverge #498 had the same shape (it also set its flag only after the create returned), so this isn't a regression — but my docstring had asserted it couldn't happen, which is worse than saying nothing. Now named explicitly as out of scope.

2. The comment claiming that killing the added window "cannot collapse" a pre-existing session is false. tmux drops a session when its last window dies, and the peer window that made the session non-empty at the session_exists check can be reaped by its own process exiting before the rollback runs — the lock serializes CAO's transitions, not a pane's exit. Documented as a real (narrow) race. Killing the whole session instead would be strictly worse, since it would destroy peers that are alive.

A second review round then caught a defect in my own correction (2f0baee): I'd claimed the forwarded env is dropped "even when the kill failed", but sequencing the clear after the kill's try meant a BaseException — precisely what the call site catches — skipped it, leaving a secret in the process-global map keyed to a dead session name. Now in a finally, which keeps the guarantee while still letting Ctrl-C propagate. Measured across all six paths (falsy/None/True return, Exception, KeyboardInterrupt, SystemExit).

Also 7294558: docs/terminal-lifecycle.md's deletion-paths table contradicted the code once delete_session began snapshotting, per your doc-maintenance note. Corrected, including the caveat that capture is best-effort on every path — so "Yes" there means the path attempts a snapshot, not that one is guaranteed.

Verification

• test_session_teardown_atomic.py + test_tmux_session_exists_strict.py: 51 passed (your 49 plus the 2 new)
• test/services/ + test/clients/ excluding agui: 1885 passed, 6 skipped, 0 failed
• One intermittent test_config_service.py failure is a pre-existing flake, not this change: get_server_settings invalidates its cache on st_mtime_ns alone, and same-tick writes to different tmp_path files compare equal, so a test gets the previous test's config. Filed separately; happy to point you at the trace.

Not rebased onto the latest main (4 commits landed while this was in flight); the branch still reports MERGEABLE, but say the word if you'd prefer it rebased.

On review provenance, so you can calibrate how much to trust the above: the implementation and both correction rounds were reviewed by independent reviewers on a different model from the author, each executing the code rather than reading it. The final finally commit is the fix the second reviewer explicitly endorsed, verified by measurement, but it did not itself get a further independent round.

[gutosantos82]

PR Review: #498 — fix(sessions): make session teardown atomic so tmux and the registry cannot diverge

Summary

This head (2f0baee, four commits on top of the fd74114 rework) fixes the create-path orphan regression flagged in the previous round: the rollback now runs inside the locked closure, covers both the new-session and add-window branches, and clears the forwarded session env in a finally so even a KeyboardInterrupt cannot leave a secret behind — all of which we verified dynamically, including differential probes showing the parent commits leak where this head does not. The design (strict fail-closed existence check, verified kill, per-name lifecycle lock, dismantle-only-after-confirmed-kill) is sound and the test suite is unusually faithful. Two things still warrant changes: the CHANGELOG entry is filed under the already-tagged [2.4.1] release, which advertises the fix in a release that does not contain it, and the new capture/dismantle split introduces a path where a failed snapshot leaks the FIFO reader thread and status-monitor buffer while the registry row is still swept — a small runtime/registry divergence of exactly the kind this PR exists to close. Additionally, reviewDecision is still CHANGES_REQUESTED: the prior maintainer findings are addressed at this head, but the review has not been re-run or dismissed.

Blocking (must fix before merge)

• [conventions] CHANGELOG.md:12 — The new entry sits under ## [2.4.1] - 2026-08-04, but all five of this PR's commits land after the v2.4.1 tag (verified: git log v2.4.1..HEAD returns exactly these five, and git show v2.4.1:CHANGELOG.md lacks the entry). As written, the changelog claims 2.4.1 shipped a fix it does not contain. Move it to an ## [Unreleased] section — and note the file is regenerated by git-cliff from commit history at release time, so the fix(sessions): subject will be auto-picked anyway; if a manual entry is kept it should be a single concise line to match the file's generated style, not the current multi-sentence paragraph. 🆕
• [correctness] src/cli_agent_orchestrator/services/terminal_service.py:~1587 (dismantle_terminal_runtime) + services/session_service.py:~270 — When capture_terminal_snapshot raises (realistically database is locked on get_terminal_metadata — a condition this PR itself treats as ordinary), metadata is None and the entire if metadata: block is skipped, including fifo_manager.stop_reader(terminal_id) and status_monitor.clear_terminal(terminal_id) — two cleanups that need only terminal_id, not metadata. The by-id sweep still deletes the row, so the leaked reader thread becomes permanently orphaned with no registry row left to find it by. Run the two terminal_id-only cleanups regardless of whether the snapshot succeeded. This outcome is newly reachable via the capture/dismantle split, and it lands in services/ where we weight severity up. 🆕

Important (should fix)

• [tests] test/services/test_session_teardown_atomic.py vs commit 2f0baee's message — The claim that "all six env-clearing paths (falsy/None/True return, Exception, KeyboardInterrupt, SystemExit)" are measured is overstated as a statement about the suite: coverage shows the rollback helper's falsy-return warning branches, its except Exception arms, and the finally env-clear failure branch are not exercised by any test, and no test injects a BaseException from kill_session. Our own probe confirms the behavior is correct (env cleared, KeyboardInterrupt propagates; the parent commit demonstrably leaks), so this is a test-gap/claim-accuracy issue, not a behavior bug — either add a kill-raises-BaseException rollback test (our probe shape is simple to adopt) or soften the commit-message/PR claim. 🆕
• [consistency] PR description — The body describes only the verified-kill/re-check/raise teardown and omits the concurrency half of the diff: the new services/session_lock.py per-name lifecycle lock, the create_terminal rewrite into a locked closure with under-lock rollback, and the id-scoped delete_terminals_by_ids sweep. The CHANGELOG does mention the mutual exclusion, so the two disagree on scope, and a reviewer reading only the body would miss roughly 60% of the diff. Update the body to cover the lock and create-path changes. 🆕
• [consistency] src/cli_agent_orchestrator/backends/base.py:111 vs backends/herdr_backend.py:365 — The ABC now mandates that kill_session must not return True on a merely-dispatched kill, but HerdrBackend.kill_session returns True on subprocess exit 0 with no liveness check and inherits the fail-open session_exists_strict default, so the atomicity guarantee silently degrades to pre-fix behavior on that backend. This is disclosed in the code and PR body, so it is a documented gap rather than a hidden one — but the contract-vs-implementation divergence ships in this PR; please link the tracked follow-up issue in the code comment/PR so backend parity does not get lost. 🆕

Nits (optional)

• [correctness] services/session_service.py:~294 — A terminal whose delete_terminal_row raises never gets a post_kill_terminal event even though the by-id sweep reconciles its row; the existing regression test asserts reconciliation but not the missing event. 🆕
• [correctness] services/session_service.py:~383 — The trailing post_kill_session dispatch is unguarded inside the outer try/except, so a raise during event construction/dispatch discards an already-complete teardown result as an HTTP 500 — inconsistent with the per-terminal loop, which wraps event construction deliberately. 🆕
• [correctness] services/session_lock.py:~78 — The holder refcount is incremented under the registry guard but lock.acquire() is called outside the try/finally; an interrupt between the two pins the name's entry in _session_locks forever (bounded, per-name). Begin the try immediately after the increment. 🆕
• [consistency] services/session_service.py delete_session docstring — "raise having changed nothing but two snapshot files" undercounts: the snapshot writes two files per terminal, so a multi-terminal session leaves 2×N files. 🆕
• [consistency] docs/terminal-lifecycle.md — The "Snapshot JSON schema" block omits caller_id, which capture_terminal_snapshot actually writes. Pre-existing, but this PR edits that exact file, so it is a cheap fix while here.
• [tests] test/clients/test_tmux_session_exists_strict.py:51 — The real-tmux suite (spawns/SIGKILLs real tmux servers, shells out to lsof/ps) is guarded only by a tmux-present skipif, not the repo's integration marker, so -m "not integration" cannot exclude it. Defensible, but make it deliberate. 🆕
• [conventions] docs/api.md — DELETE /sessions/{name} gains a new failure mode (HTTP 500 when the kill cannot be confirmed); api.md is route-family-level so per-endpoint codes are likely out of scope, but flagging so the omission is a choice. 🆕

Tests

Strong, faithful suite — it drives the real delete_session/create_terminal against an in-memory backend that models the actual divergence-causing tmux behaviors (last-window-drops-session, laggy reap, silent-fail kill), a real per-test SQLite registry, and real threads with barriers tripped at exact race windows. The strict-existence tests pin behavior against real tmux servers on private sockets, which is the right call since the underlying bug was libtmux swallowing errors. Measured coverage on changed modules: session_lock.py 100%, session_service.py 96%, clients/tmux.py 92%, terminal_service.py 73% (dominated by pre-existing untested code; the new split functions are well covered). The one substantive gap is the rollback helper's failure arms (see Important above). Regression framing is accurate: core teardown tests demonstrably fail at the pre-PR base, and the two create-path tests fail at the intermediate commit they guard.

Verification

Baseline at head: the five targeted suites (test_session_teardown_atomic.py, test_tmux_session_exists_strict.py, test_tmux_client.py, test_session_service.py, test_plugin_event_emission.py) pass — 171 passed, 0 failed. Per-claim:

• ✓ VERIFIED — a raise from db_create_terminal inside the locked closure no longer orphans tmux state, both branches: new_session=True kills the just-created session (1 kill_session call, zero rows, env cleared); new_session=False kills only the added window (peer window and row survive, zero kill_session calls). The same probes fail at fd74114, confirming cfe32b6 is the fix.
• ✓ VERIFIED — forwarded env cleared even when kill_session raises KeyboardInterrupt during rollback: at this head the env map is empty afterward and the interrupt still propagates; at the parent commit (1bd108d) the identical probe leaves the secret in the map. The finally claim holds in behavior (though no suite test covers it — see Important).
• ✓ VERIFIED — regression tests genuinely encode the pre-fix bug: at base 752be53, test_silent_kill_session_failure_is_surfaced_not_swallowed, test_rerun_reconciles_half_torn_down_session, and test_failed_kill_leaves_session_whole_not_a_zombie all fail.
• Security probe review: no injection surface in the new strict lookup (session name never passed as a tmux argument; exact per-line matching), fixed-argv timeout-bounded subprocess calls, and the finally-based env clear is a genuine secret-lifetime improvement. No blocking or important security findings.

Verdict

Request changes — the engineering at this head is strong and every previously-raised maintainer finding is verifiably fixed, but the CHANGELOG entry misfiles the change into the already-shipped 2.4.1 release, the snapshot-failure path leaks the FIFO reader and status-monitor state while sweeping the row (a small instance of the divergence class this PR targets), and the standing CHANGES_REQUESTED review has not been resolved. All three are cheap to address relative to the size of the fix.