MOR-1185: give the teardown unkey the session identity it needs to release the lease - #2116
Conversation
…release _release_ptt_on_teardown enqueued PttOff() on the raw server queue, so the entry reached the drain with no source and no session id. Because _managed_tx requires a truthy session id, the defensive unkey took the legacy write. After MOR-1016 that means a disconnecting session de-keys the rig but never releases its lease, and the next session is refused until MOR-1191's watchdog expires. Enqueue through the same metadata wrapper every other unkey uses, carrying the handler's stable _session_id. Verified against the shipped drain, not a double: the entry arrives as session_id='websocket-…' — the same value the key carried, while the command_id is a per-request throwaway. That contrast is the ticket. Shape 2 — releasing the lease directly from teardown — is not reachable here: release_owner is async and _release_ptt_on_teardown is a sync method called from run()'s finally, so it would mean spawning a task during teardown and splitting lease bookkeeping from the physical write across two layers. This replaces today's unconditional defensive write on the managed path, and that is the point rather than a cost. A session holding no lease answers STALE and nothing reaches the rig — because a blind set_ptt(False) from one session's teardown would de-key another session's live transmission. Confirmed both ways against a real runtime: at base, B's disconnect issues a raw de-key while A is transmitting; at head it issues nothing and A keeps the lease. The unmanaged path is unchanged and still writes unconditionally. Also correct the _refuse_key_from_gone_session docstring, which this change falsifies: the teardown unkey now carries a session id, so it no longer passes the gate for want of one. It passes because the gate is reached only from the PttOn arm — the weaker reason — and hoisting that call anywhere PttOff can reach would strand a keyed rig on every disconnect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Agent Review: PASS Independent non-author verification at max effort. All three files sha256-pinned before and after; unchanged. The accepted behaviour change — argument holds, one real cost statedThe reviewer ran identical scenarios against head and a Multi-session: real, and the change fixes it. A keys and is transmitting, then B connects and disconnects: External CAT: a real reduction, stated plainly. Driving the supervisor to The ownership reasoning is sound and the coverage loss is real. It is latent — zero The stable id reaches the real drainVerified through the shipped
The synthetic command_id is inert on both pathsSuccess: service state Slices 2 and 5 intactSlice 2 — with a dead egress socket, Slice 5 —
|
| gate | 3.11 | 3.13 |
|---|---|---|
pytest tests/ --ignore=tests/integration — base |
8541 | 8545 |
| same — head | 8545 | 8549 |
ruff / lint-imports / mypy |
clean, diff vs base empty |
same |
The author's "8545 on both" is right for 3.11 only; the delta of +4 is right on both.
Two findings I applied before merging
- This diff falsified a docstring.
_refuse_key_from_gone_sessionsaid the teardown unkey passes the gate because it carries no session id. It now carries one, and passes for a weaker reason — the gate is reached only from thePttOnarm. Anyone hoisting that call would strand a keyed rig on every disconnect. Corrected here; it is the fourth file, and net LOC stays under 200. - The
_draintest helper claimed to execute "exactly as_run's drain does" while omittingcommand_service=entry.command_service, so the new tests never reached_mark_queued_command_failed. Now it passes it.
Scope caveat, verified
release_owner only starts the de-key; undriven, a second session is refused with RELEASE_PENDING, not BUSY. The reviewer proved acceptance with no in-test driver at all on the real runtime: A disconnects → lease=None → B's request_on = accepted.
Non-blocking
_CommandMetadataQueue.put's bare except TypeError catches TypeErrors raised inside a queue's put body, which would silently restore the old behaviour. Pre-existing, now inherited by a safety path.
Hardware boundary
Software only. No hardware was involved and no hardware claim is made. This does not replace MOR-1033.
Blocks MOR-1016. 4 files, 195 net LOC — one file over the guardrail, explained below.
The defect
_release_ptt_on_teardownenqueuedPttOff()on the raw server queue, so the entry reached the drain with no source and no session id. Because_managed_txrequires a truthy session id, the defensive unkey took the legacy write. After MOR-1016 that means a disconnecting session de-keys the rig but never releases its lease, and the next session is refused until MOR-1191's watchdog expires.The change
Enqueue through the same
_CommandMetadataQueuewrapper every other unkey uses, carrying the handler's stable_session_id. Verified against the shipped drain, not a double:The contrast is the ticket: the command_id is a per-request throwaway, the session_id is stable and identical on both.
Shape 2 — releasing the lease directly from teardown — is not reachable here:
release_ownerisasyncand_release_ptt_on_teardownis a sync method called fromrun()'sfinally, so it would mean spawning a task during teardown and splitting lease bookkeeping from the physical write across two layers.The behaviour change, accepted deliberately
This replaces today's unconditional defensive write on the managed path, and that is the point rather than a cost: a blind
set_ptt(False)from one session's teardown would de-key another session's live transmission. Under management the lease is the authority on who is on the air.Confirmed both ways by the verifier against a real
ManagedRadioRuntime+ real effect service + realRadioPoller._run():['RAW set_ptt(False)']— blind de-key while A is on the air[]— A keeps the lease and stays keyedWRITE_OFF, leaseNone, phase idle['RAW set_ptt(False)'][]— nothingThe third row is a real reduction in defensive coverage and is stated plainly.
TxPhase.EXTERNAL_UNOWNEDhas zero production consumers, and bothtick()andemergency_release()requireself._lease— so neither MOR-1191's watchdog nor shutdown covers a leaseless keyed rig. After this change the Web layer has no defensive de-key for a rig keyed outside the supervisor's knowledge. That is MOR-1175's to close, and it stops being latent when MOR-1016 lands. Recorded there.Latent today:
grep -rn "ManagedRadioRuntime(" src/= 0 hits, and the unmanaged teardown still writesRAW set_ptt(False)on both base and head.Evidence
run()re-raised and afterwardsphase=idle lease=None, de-key via the supervisor with no raw write. The lease, not merely the enqueue, is released through a dead socket._refuse_key_from_gone_sessionis called from exactly one site, insidecase PttOn(). Executable proof:session_is_livewas alreadyFalseat drain time and the teardown still de-keyed. Mutation extending the gate toPttOffis killed by 5 tests.command_idis inert on both paths — on failure_mark_queued_command_failedfires,fail_commandreturnsFalsefor an unknown id,emit_lifecycleis called 0 times, and service state is byte-identical before and after.pytest tests/ --ignore=tests/integration— baseruff/lint-imports/mypydiffvs base emptyThe fourth file, and why
The verifier found that this diff falsifies an existing docstring in
radio_poller.py:_refuse_key_from_gone_sessionsays the teardown unkey passes the gate because it carries no session id. It now carries one. It passes for a different and weaker reason — the gate is reached only from thePttOnarm — and anyone who later hoisted that call to somewherePttOffreaches would strand a keyed rig on every disconnect.I corrected it here rather than filing it, because this change is what made it false. Three lines, in a file the diff otherwise does not touch. Net LOC stays under 200.
Also corrected from the verifier's findings
The
_draintest helper claimed to execute "exactly as_run's drain does" while omittingcommand_service=entry.command_service, which the real drain passes — so the new tests never reached_mark_queued_command_failed. Now it does pass it, and the helper's claim is true.Scope caveat, stated by the author and confirmed
release_owneronly starts the de-key. Undriven, the lease sits inRELEASE_PENDINGand a second session is refused withRELEASE_PENDING, notBUSY— a different refusal, not an acceptance. The verifier proved acceptance with no in-test driver at all on the real runtime: A disconnects →lease=None→ B'srequest_on=accepted.Non-blocking
_CommandMetadataQueue.put's bareexcept TypeErrorcatches TypeErrors raised inside a queue'sputbody, silently retrying without metadata — which would silently restore the old behaviour. Pre-existing, now inherited by a safety path. Verifier instrumented the whole suite: teardown fallbacks = 0, production queues all take the metadata path.TxPhase.EXTERNAL_UNOWNEDcoverage loss — record against MOR-1175 before MOR-1016.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-1185