fix: add per-device execute lock to prevent I/O interleaving

[ChristopherJHart]

Summary

Adds a per-hostname asyncio.Lock around command execution to prevent interleaved PTY I/O on shared Unicon spawns.

Problem

The broker dispatches commands via run_in_executor(None, connection.execute, cmd) with no per-device serialization. Unicon's spawn is not thread-safe — concurrent execute() calls on the same device interleave I/O on the PTY, corrupting command output.

Today this is safe only by accident (tests are sequential per device). Any future intra-device parallelism would immediately hit this bug.

Solution

• Add _execute_locks: dict[str, asyncio.Lock] to the broker
• Acquire the per-hostname lock before dispatching to the executor
• Cross-device parallelism is unaffected (each device has its own lock)

Impact

• No performance change for sequential execution (no-op lock acquisition)
• Correctness prerequisite for any future test parallelism within a device

Files Changed

File	Change
nac_test/pyats_core/broker/connection_broker.py	Per-device asyncio.Lock in _execute_command

Test plan

• Existing tests pass (lock is no-op for sequential execution)
• Validated end-to-end on 7-device fleet and 2-device FTD pair

---

🤖 Generated with Claude Code

[oboehmer]

@ChristopherJHart : good catch, and I reckon worth a defensive fix. Executing the tests sequentially on each device has been a design decision to not overload any device, so the risk is rather low.

[oboehmer]

Heads-up from reviewing #899, since the two overlap.

Merge conflict incoming either way. #899 rewrites _execute_command and moves the executor dispatch out into a new _run_and_cache() helper. Whichever of the two lands second will conflict, and this PR's "acquire the per-hostname lock before dispatching to the executor" will need re-siting — the dispatch it currently wraps won't be in _execute_command any more. Probably worth deciding the stacking order now rather than at rebase time.

Lock scoping — worth a thought while you're in here. The interleaved-PTY problem this PR fixes is real and I'd like it merged. But there's a second, adjacent hazard that the current placement doesn't cover, and it gets more likely once #899's reconnect-and-retry lands:

A: execute → fails → releases
B: execute on the same dead handle → fails → releases
A: _disconnect_device → _create_connection → C1 → executing on C1
B: _disconnect_device  →  tears down C1 mid-execute

_disconnect_device guards on connection_locks[hostname], while this PR's lock is _execute_locks[hostname] — two different locks over two halves of one critical section, so B's teardown never contends with the execute it destroys. This is already possible on main (disconnect-on-failure predates both PRs); #899's retry just widens the window.

Two options if you want to close it here rather than in a separate issue:

• Widen _execute_locks to span the whole critical section (get connection → execute → failure handling → retry), which subsumes both locks. Costs intra-device parallelism the broker doesn't currently have.
• Cheaper and orthogonal: _disconnect_device(hostname, expect=connection) that only tears down when connected_devices.get(hostname) is connection, so a stale caller can't destroy a successor's connection. ~2 lines.

No objection to keeping this PR narrowly scoped to the I/O interleaving and tracking the teardown race separately — mainly flagging that a per-device lock landing now is the natural moment to decide which lock owns the teardown, so we don't end up with three.

[ChristopherJHart]

Thanks for the detailed review — both the merge-order flag and the lock-scoping analysis were spot-on.

Rebased onto main (post-#899 merge). The old commit is replaced by a single new one (5b47140).

Lock consolidation (your Option A). connection_locks and the proposed _execute_locks are replaced by a single _device_locks dict. The lock in _execute_command now spans the full critical section: get-connection → execute → disconnect-on-failure → reconnect → retry. This closes both hazards you identified:

1. Interleaved PTY I/O — concurrent execute() calls on the same device serialize at the top of _execute_command.
2. Teardown race — _disconnect_device_internal is called directly from _run_and_cache (already under the lock), so a stale caller can never tear down a successor's connection mid-execute.

_get_connection and _run_and_cache are now lock-free — callers hold the lock. External entry points (_ensure_connection, _disconnect_device) acquire _device_locks themselves.

I also added a double-check of the command cache under the lock to avoid redundant execution when two callers race on the same command.

All 62 broker tests (unit + integration) pass.