Skip to content

MOR-1185: give the teardown unkey the session identity it needs to release the lease - #2116

Merged
morozsm merged 1 commit into
mainfrom
codex/mor-1185
Jul 31, 2026
Merged

MOR-1185: give the teardown unkey the session identity it needs to release the lease#2116
morozsm merged 1 commit into
mainfrom
codex/mor-1185

Conversation

@morozsm

@morozsm morozsm commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Blocks MOR-1016. 4 files, 195 net LOC — one file over the guardrail, explained below.

The defect

_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.

The change

Enqueue through the same _CommandMetadataQueue wrapper every other unkey uses, carrying the handler's stable _session_id. Verified against the shipped drain, not a double:

{'cmd': 'PttOn',  'command_id': 'websocket-212559642296208',              'session_id': 'websocket-212559642270375'}
{'cmd': 'PttOff', 'command_id': 'teardown-ptt-off-websocket-212559642270375', 'session_id': 'websocket-212559642270375'}

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_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.

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 + real RadioPoller._run():

base head
A transmitting, B disconnects ['RAW set_ptt(False)'] — blind de-key while A is on the air [] — A keeps the lease and stays keyed
single session, lease intact de-keyed de-keyed via the supervisor's own WRITE_OFF, lease None, phase idle
rig keyed by external CAT, no lease ['RAW set_ptt(False)'] [] — nothing

The third row is a real reduction in defensive coverage and is stated plainly. TxPhase.EXTERNAL_UNOWNED has zero production consumers, and both tick() and emergency_release() require self._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 writes RAW set_ptt(False) on both base and head.

Evidence

  • Slice 2 preserved — with a dead egress socket, run() re-raised and afterwards phase=idle lease=None, de-key via the supervisor with no raw write. The lease, not merely the enqueue, is released through a dead socket.
  • Slice 5 preserved_refuse_key_from_gone_session is called from exactly one site, inside case PttOn(). Executable proof: session_is_live was already False at drain time and the teardown still de-keyed. Mutation extending the gate to PttOff is killed by 5 tests.
  • The synthetic command_id is inert on both paths — on failure _mark_queued_command_failed fires, fail_command returns False for an unknown id, emit_lifecycle is called 0 times, and service state is byte-identical before and after.
  • 8 mutations killed, 2 negative controls survived. A suite where everything fails is as uninformative as one where nothing does.
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 fourth file, and why

The verifier found that this diff falsifies an existing docstring in radio_poller.py: _refuse_key_from_gone_session says 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 the PttOn arm — and anyone who later hoisted that call to somewhere PttOff reaches 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 _drain test helper claimed to execute "exactly as _run's drain does" while omitting command_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_owner only starts the de-key. Undriven, the lease sits in RELEASE_PENDING and a second session is refused with RELEASE_PENDING, not BUSY — 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's request_on = accepted.

Non-blocking

  1. _CommandMetadataQueue.put's bare except TypeError catches TypeErrors raised inside a queue's put body, 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.
  2. TxPhase.EXTERNAL_UNOWNED coverage 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

…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>
@morozsm

morozsm commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

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 stated

The reviewer ran identical scenarios against head and a git archive HEAD base sandbox, using the real ManagedRadioRuntime + real managed_tx_effect_service + real RadioPoller._run().

Multi-session: real, and the change fixes it. A keys and is transmitting, then B connects and disconnects:

BASE: wire log during B's teardown = ['RAW set_ptt(False)']   <- blind de-key while A is on the air
HEAD: wire log = []                                           <- A keeps the lease and stays keyed

External CAT: a real reduction, stated plainly. Driving the supervisor to TxPhase.EXTERNAL_UNOWNED — rig keyed at the wire, no lease — and disconnecting: base gives a defensive de-key, head gives nothing. There is no other path: TxPhase.EXTERNAL_UNOWNED has zero production consumers, and both tick() and emergency_release() require self._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.

The ownership reasoning is sound and the coverage loss is real. It is latent — zero ManagedRadioRuntime( in src/, and the unmanaged teardown still writes on both base and head — so it lands with MOR-1016, not today. Recorded against MOR-1175.

The stable id reaches the real drain

Verified through the shipped _run() drain, not a double:

{'cmd': 'PttOn',  'command_id': 'websocket-212559642296208',              'session_id': 'websocket-212559642270375'}
{'cmd': 'PttOff', 'command_id': 'teardown-ptt-off-websocket-212559642270375', 'session_id': 'websocket-212559642270375'}

_session_id is assigned once and unchanged across the lifecycle. The contrast that makes the ticket: PttOn's command_id is a per-request throwaway while the session_id is stable and identical on both.

The synthetic command_id is inert on both paths

Success: service state {'events': 4, 'overlays': 1, 'readbacks': 1} unchanged. Failure: _mark_queued_command_failed did fire, fail_command returned False for the unknown id, emit_lifecycle called 0 times, service state byte-identical.

Slices 2 and 5 intact

Slice 2 — with a dead egress socket, run() re-raised and afterwards phase=idle lease=None, de-key via the supervisor with no raw write. The lease, not merely the enqueue.

Slice 5 — _refuse_key_from_gone_session is called from exactly one site, inside case PttOn(). Executable proof: session_is_live was already False at drain time (the risky ordering) and the teardown still de-keyed. A mutation extending the gate to PttOff is killed by 5 tests.

except TypeError fallback — the author's claim was wrong, in their own disfavour

Whole-suite instrumentation via a plugin loaded from outside the worktree:

metadata 96 | fallback 128 (57.1%) — entirely the other, pre-existing call site via test doubles
TEARDOWN unkey specifically: metadata 48 across 8 test files | fallback 0

The author claimed tests/test_web_ptt_readonly.py silently falls back. It does not — those handlers are read-only, so teardown returns at the guard before touching the queue. Production command_queue is only ever CommandQueue or _HttpCommandCollector, both keyword-only. Teardown fallbacks across the entire suite: zero.

Vacuity — 8 killed, both negative controls survive

Including two the reviewer wrote: forcing the metadata queue to always fall back, and extending the liveness gate to PttOff. A suite where everything fails is as uninformative as one where nothing does.

The reviewer disclosed a bug in their own detector — it grepped for failed, which also matches xfailed, so it initially misreported both controls — and chased a 1 failed on one control down to a pre-existing sandbox flake by running the pristine unmutated sandbox as a control.

Gates

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

  1. This diff falsified a docstring. _refuse_key_from_gone_session said 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 the PttOn arm. 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.
  2. The _drain test helper claimed to execute "exactly as _run's drain does" while omitting command_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.

@morozsm
morozsm marked this pull request as ready for review July 31, 2026 03:29
@morozsm
morozsm merged commit 2ef4f8d into main Jul 31, 2026
5 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