Skip to content

MOR-1187: bind the managed TX facade inside the unkey teardown guard - #2114

Merged
morozsm merged 1 commit into
mainfrom
codex/mor-1187-bind-in-try
Jul 30, 2026
Merged

MOR-1187: bind the managed TX facade inside the unkey teardown guard#2114
morozsm merged 1 commit into
mainfrom
codex/mor-1187-bind-in-try

Conversation

@morozsm

@morozsm morozsm commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Blocks MOR-1016. 2 files, +76 net LOC. Production change is one line moved.

The defect

ManagedTxApi.bind runs an isinstance against a runtime_checkable Protocol, which reads the managed_tx property — backend code that is free to fail. In case PttOff(): the bind sat above the try whose finally runs _stop_tx_audio_leg(), so a backend with a raising accessor lost the teardown entirely.

At base the teardown always ran. This narrowed exactly the invariant MOR-1013 Slices 1 and 2 established — a failed de-key with the TX audio leg still pumping modulation into the rig is the worst outcome available — and it did so silently, because nothing in src/ publishes managed_tx yet.

The fix

Move the bind to be the first statement inside the try. Sandbox, both interpreters:

3.11 3.12
base poller 1 failed, 14 passed 1 failed, 14 passed
head poller 15 passed 15 passed

The single failure is the teardown assertion — assert [] == ['stop_tx', 'restart_rx'] — and nothing else; pytest.raises matched at base too, so the delta is exactly the lost teardown.

The original exception still propagates unwrapped. Verified as the same object with __cause__ and __context__ both None, across TimeoutError, ConnectionError and RuntimeError. _mark_queued_command_failed uses only str(exc) plus a timed_out flag set by except-clause ordering, so classification is unchanged.

The mechanism differs by interpreter, and the test covers both

The verifier instrumented the getter and dumped frames:

  • 3.11isinstance itself raises; the getter is invoked once from typing.py hasattr inside __instancecheck__.
  • 3.12isinstance returns True with the getter invoked zero times; the raise comes from bind's own radio.managed_tx read.

Two genuinely different mechanisms, both landing inside _managed_tx, so one test is valid on both.

Two behaviours pinned

Both survived mutation in the Slice 4 review; the implementations were already correct.

  • IDEMPOTENT in _KEY_ACCEPTED — without it a same-owner re-key reads as a refusal and tears down its own live TX audio leg mid-transmission while the rig stays keyed. The verifier checked the failure shape matches: CommandError: managed TX rejected PTT ON: idempotent, raised only after _stop_tx_audio_leg().
  • The not session_id guard. A ticket premise of mine was wrong here and the author corrected it: a Slice 5 test already exercises websocket + None and already kills that mutation. The verifier then built the experiment that settles whether the new test still earns its lines — mutating the guard plus removing TxOwner.__post_init__'s empty-id ValueError: the pre-existing test stops killing it entirely, because it only ever killed it incidentally via a validator two modules away. The new test still kills it, running a real supervisor and asserting entries == [].

Evidence

10 mutations, all killed — 5 re-derived, 5 written by the verifier. The "exactly one test" claim was confirmed against the full suite in isolated sandboxes:

M1 (bind moved back outside the try): 1 failed, 8536 passed
M3 (IDEMPOTENT removed):              1 failed, 8536 passed

Neither hole was catchable anywhere else in 8537 tests.

Slice 1/2/4/5 behaviours re-checked by mutation: the finally teardown, the _managed_tx source gate, _refuse_key_from_gone_session first in PttOn, start_tx-before-key, and the refused-key disarm — all killed.

gate 3.11 3.12
pytest tests/ --ignore=tests/integration — base 8534 passed 8534 passed
same — head 8537 passed 8537 passed
ruff check / format --check clean clean
lint-imports 5 kept, 0 broken 5 kept, 0 broken
mypy src/ 15 errors, diff vs base clean same

PttOn is deliberately untouched

Probed rather than reasoned, both interpreters:

A: managed_tx accessor raises -> RuntimeError; calls=[]
B: set_ptt(True) raises       -> ConnectionError; calls=['start_tx', 'set_ptt(True)']

(A) No exposure — the bind precedes start_tx, so a raising accessor arms nothing and fails closed. (B) is the real PttOn leak and is MOR-1178's shape, out of scope here.

Non-blocking

  1. MOR-1193ManagedTxApi.bind diverges across interpreters for AttributeError specifically: 3.11's hasattr swallows it and the poller silently takes the unmanaged path, 3.12+ raises. Pre-existing in bind; this change strictly improves the 3.12 side.
  2. When the bind fails, the rig stays keyed — audio torn down, rig still on the air. Strictly better than base by MOR-1013's own worst-outcome reasoning, and the new test's docstring says so plainly. A raw set_ptt(False) fallback would bypass the supervisor and is well outside this ticket.
  3. The comment's "can never replace it" holds for Exception, not BaseException — a CancelledError from the teardown does demote the original to __context__. Pre-existing unchanged context, and propagating a cancellation is arguably correct.

Hardware boundary

Software only. No hardware was run and no hardware claim is made. MOR-1033 remains the FTX-1 physical acceptance gate.

Linear: MOR-1187

ManagedTxApi.bind runs an isinstance against a runtime_checkable Protocol,
which reads the managed_tx property — backend code that is free to fail. The
bind sat above the try whose finally tears down the TX audio leg, so a
backend with a raising accessor lost the teardown entirely: the failure
propagated before the guard was entered.

At base the teardown always ran. This narrowed exactly the invariant slices 1
and 2 of MOR-1013 established — a failed de-key with the TX audio leg still
pumping modulation into the rig is the worst outcome available — and it did
so silently, because nothing in src/ publishes managed_tx yet.

Move the bind to be the first statement inside the try. A raise now reaches
the finally, the teardown runs, and the original exception propagates
unwrapped: verified as the same object, with __cause__ and __context__ both
None, across TimeoutError, ConnectionError and RuntimeError, so the caller's
_mark_queued_command_failed classification is unchanged.

The failure mechanism differs by interpreter and the test covers both: on
3.11 the isinstance itself raises, because __instancecheck__ uses hasattr; on
3.12+ isinstance returns True with the getter never invoked and the raise
comes from bind's own read. Both land inside _managed_tx.

Also pin two behaviours that survived mutation in the slice 4 review. Losing
IDEMPOTENT from _KEY_ACCEPTED makes a same-owner re-key look like a refusal,
which tears down its own live TX audio leg mid-transmission while the rig
stays keyed. The not-session_id guard was already killed incidentally by a
slice 5 test, but only via a TxOwner validator two modules away; the new test
states the invariant directly against a real supervisor.

PttOn is deliberately untouched. Its bind already precedes start_tx, so a
raising accessor arms nothing and fails closed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@morozsm

morozsm commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Agent Review: PASS

Independent non-author verification at max effort. Both files sha256-pinned before and after; unchanged, and the commit introduced no delta:

5ffca1c31694ba904ad92b8c2a6f312a4bfa53c234d3d57d452f561acec26e35  src/rigplane/web/radio_poller.py
f8db5378a4631cf98f605add56d3b5b44a9487d026e8289a7c144c4624a8d807  tests/test_web_managed_tx_owner.py

All mutation work ran in git archive HEAD sandboxes with their own venvs; every venv path was asserted and printed before any result was trusted, and every mutation run printed its poller hash before executing.

Scope

2 files, net 76 by four methods (numstat, awk line classification, shortstat, per-file length delta). The naive grep -c '^+[^+]' returns 59 — the undercount reproduces. Source hunks confined to the PttOff arm; control.py and _poller_types.py untouched.

Correction to my review brief: I gave the reviewer a full HEAD SHA that does not exist. The first eight characters were right; I fabricated the remaining thirty-two rather than copying them. Actual HEAD is 5b7aaa3c039ffa1b1f4ddca2c1493e97af6e5e0d. The reviewer caught it with git cat-file and confirmed the real value is current origin/main. Nothing about the change is affected, but the error was mine and is worth recording.

The fix, symptom then mechanism

3.11 3.12
base poller 1 failed, 14 passed 1 failed, 14 passed
head poller 15 passed 15 passed

The single failure is the teardown assertion only — pytest.raises matched at base too, so the delta is exactly the lost teardown.

The reviewer then instrumented the getter and dumped frames rather than reasoning about it:

  • 3.11isinstance itself raises; getter invoked once from typing.py:2025 (hasattr inside __instancecheck__); raise originates at radio_protocol.py:564.
  • 3.12isinstance returns True with the getter invoked zero times; raise comes from radio_protocol.py:566.

Two different mechanisms, both inside _managed_tx, so one test is valid on both.

Propagation unchanged

Same object, unwrapped and unchained, across three exception classes on both interpreters:

TimeoutError    -> same-object=True  __cause__=None __context__=None  teardown ran
ConnectionError -> same-object=True  __cause__=None __context__=None  teardown ran
RuntimeError    -> same-object=True  __cause__=None __context__=None  teardown ran

_mark_queued_command_failed uses only str(exc) plus a timed_out flag set by except-clause ordering, which dispatches purely on type. Classification unchanged. Every managed token in the file was enumerated; all three PttOff uses are inside the try, the finally does not reference it, no UnboundLocalError path.

The comment, claim by claim

Each of the extended comment's assertions was checked against the code, including that dropping the word "unkey" from "the original unkey exception" is required by the change, since the exception can now be the bind's.

My ticket premise was wrong, and the reviewer settled what follows from that

I wrote that existing tests hit session_id=None only with source="http". The author corrected me: a Slice 5 test already exercises websocket + None and already kills that mutation. The reviewer verified this independently at base line 234.

They then built the experiment that decides whether the new test still earns its lines — mutating the guard plus removing TxOwner.__post_init__'s empty-id ValueError:

FAILED test_a_websocket_unkey_without_a_session_id_stays_unmanaged
1 failed, 14 passed

The pre-existing test stops killing it entirely; it only ever killed it incidentally, via a dataclass validator two modules away. The new test still kills it because it runs a real supervisor and asserts entries == [] — it states the invariant directly. That is a better argument for the lines than the one in my ticket.

Vacuity — 10 mutations, all killed

5 re-derived plus 5 of the reviewer's own. The "exactly one test" claim was confirmed against the full suite in isolated sandboxes:

M1 (bind moved back outside the try): 1 failed, 8536 passed
M3 (IDEMPOTENT removed):              1 failed, 8536 passed

Neither hole was catchable anywhere else in 8537 tests. They also checked M3's failure shape matches its stated stake: CommandError: managed TX rejected PTT ON: idempotent, reached only after _stop_tx_audio_leg() — a live leg torn down while the lease stays held.

Slice 1/2/4/5 behaviours re-checked by mutation and by direct reading: finally teardown, source gate, _refuse_key_from_gone_session first in PttOn, start_tx-before-key, refused-key disarm — all killed.

PttOn — both claims confirmed

A: managed_tx accessor raises -> RuntimeError; calls=[]
B: set_ptt(True) raises       -> ConnectionError; calls=['start_tx', 'set_ptt(True)']

(A) fails closed, no exposure. (B) is the real leak, MOR-1178's shape, untouched — the diff's hunks never reach that arm.

Gates — run by the reviewer, both interpreters

gate 3.11 3.12
pytest tests/ --ignore=tests/integration — base 8534 8534
same — head 8537 8537
ruff check / format --check clean clean
lint-imports 5 kept, 0 broken 5 kept, 0 broken
mypy src/ 15 errors, diff vs base clean identical

The reviewer established the 3.11 base themselves rather than inheriting it.

Disclosed honestly: their first 3.11 head run showed one failure in test_proxy.py::test_run_proxy_starts_and_stops — a test that sleeps 0.1 s and asserts a task is not done, against a fixed UDP port, importing nothing from the PTT path. They did not accept it in either direction: it passed 3/3 in isolation, passed on 3.12, passed at base, and a full solo 3.11 re-run with nothing else in flight gave 8537 passed, 0 failed. Contention from their own parallel harness.

Non-blocking

  1. MOR-1193, filed and blocking MOR-1016ManagedTxApi.bind diverges across interpreters for AttributeError specifically. On 3.11 hasattr swallows it, isinstance returns False, and the poller silently takes the unmanaged path — no lease, no owner, no watchdog. On 3.12+ it raises. quick.yml pins 3.11, so the silent-bypass face is the one the per-PR gate exercises. Pre-existing in bind; this change strictly improves the 3.12 side.
  2. When the bind fails, the rig stays keyed — audio torn down, rig still on the air. Strictly better than base by MOR-1013's own worst-outcome reasoning, and the new test's docstring says so plainly.
  3. The pre-existing "can never replace it" comment holds for Exception, not BaseException: a CancelledError from the teardown demotes the original to __context__. Unchanged context, and propagating a cancellation is arguably correct.

Hardware boundary

Software only. No hardware was involved and no hardware claim is made. This does not replace MOR-1033.

@morozsm
morozsm marked this pull request as ready for review July 30, 2026 20:26
@morozsm
morozsm merged commit 5fc18a5 into main Jul 30, 2026
5 of 6 checks passed
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.

1 participant