perf: don't disconnect + wipe cache on command rejection

[ChristopherJHart]

Summary

Distinguishes between command rejection (device says "Invalid command") and transport failure (SSH drops) to avoid unnecessary disconnect + cache wipe.

Problem

When a device rejects a command (e.g., SubCommandFailure for "Invalid command at '^' marker"), the SSH session is still healthy. Previously, ANY exception in _run_and_cache triggered:

1. Full SSH disconnect
2. Wipe of the entire device command cache

This turned a 4.4s test into a 20.8s test (reconnect + re-execute all previously cached commands).

Solution

• Import SubCommandFailure from unicon.core.errors
• In _run_and_cache, catch SubCommandFailure before the generic except Exception — log a warning and re-raise without disconnecting
• Only disconnect + wipe cache on actual transport failures

Design note

SubCommandFailure is a fast path, not a health guarantee — unicon wraps transport errors in it too. Those get neither a disconnect in _run_and_cache nor a retry in _execute_command; recovery relies on unicon re-establishing the session on the next execute(), which it does transparently.

Measured Impact

• Failed test cost: 20.8s → 4.4s (4.7x reduction per failure)
• The savings come from eliminating the reconnect-and-replay amplification on each rejection; the per-rejection cost (~4.4s) is still paid because CommandCache is per-test-instance. A follow-up for negative caching could reduce that further.

Files Changed

File	Change
nac_test/pyats_core/broker/connection_broker.py	SubCommandFailure catch in _run_and_cache with design-point comment
tests/integration/test_connection_broker_failure_modes.py	Two contrasting tests: SubCommandFailure preserves connection+cache vs transport failure disconnects+wipes
CHANGELOG.md	Entry under # Unreleased > ## Performance

Test plan

• Existing 25 integration tests pass
• test_sub_command_failure_preserves_connection_and_cache — successful command populates cache, SubCommandFailure on second command propagates error but connection and cache remain intact
• test_transport_failure_disconnects_and_wipes_cache — same setup with OSError triggers disconnect and cache wipe (regression guard)
• Rebased onto main after perf(broker): replace SSH liveness probe with reconnect-and-retry (#909) #899 merge; SubCommandFailure handler placed in _run_and_cache (not _execute_command)

---

🤖 Generated with Claude Code

---

🤖 AI Generation Metadata

• AI Generated: Yes
• AI Tool: claude-code
• AI Model: opus-4.6
• AI Contribution: ~80% (rebase conflict resolution, PR description update, test implementation)
• AI Reason: rebase onto post-perf(broker): replace SSH liveness probe with reconnect-and-retry (#909) #899 main, address reviewer feedback, update PR description
• Human Oversight: Code reviewed and approved by repository maintainer

[oboehmer]

thanks again for fixing this, @ChristopherJHart ! I raised #908 to track it. could you please complete this PR with adding test coverage, and also adding a changelog entry.

Suggested test strategy:

Extend tests/integration/test_connection_broker_failure_modes.py using the existing make_broker + _patch_executor + _run_broker infrastructure with two contrasting tests:

1. SubCommandFailure path — Execute a successful command to populate cache, then trigger SubCommandFailure on a second command. Assert: error propagated to client, but connection and command cache remain intact, device never disconnected.
2. Transport failure path — Same setup, but trigger a transport-level exception (e.g., OSError). Assert: connection removed, cache wiped. This serves as a regression guard for existing behavior.

This validates the fix at the real socket-protocol level and explicitly contrasts the two exception paths against each other.

[ChristopherJHart]

Thanks for the review guidance! I've added both items:

Tests (tests/integration/test_connection_broker_failure_modes.py):

• test_sub_command_failure_preserves_connection_and_cache — executes a successful command to populate cache, triggers SubCommandFailure on a second command, then asserts: error propagated to client, connection and command cache remain intact, device never disconnected.
• test_transport_failure_disconnects_and_wipes_cache — same setup but triggers OSError. Asserts: connection removed, cache wiped. Regression guard for existing behavior.

Both use the existing make_broker / _patch_executor / _run_broker infrastructure.

Changelog — added entry under # unreleased > Performance.

[oboehmer]

This looks right to me in substance, but I'll hold the formal approval until I can see the rebased code — as noted at the bottom, it will look meaningfully different after #899 goes in, and I'd rather approve what actually merges.

Upfront caveat: I've leaned heavily on an agent-assisted analysis of the unicon call path here, and I don't claim to hold all of its subtleties in my head myself 😉 — so if something below is wrong, say so plainly and I'll take your word over the analysis.

On the premise

The PR says "If the exception is SubCommandFailure ... the SSH session is still healthy." That's true for the case you're targeting, but it isn't true in general. unicon.plugins.generic.service_implementation.Execute.call_service ends with:

except StateMachineError:
    raise
except UniconBackendDecodeError:
    pass
except Exception as err:
    raise SubCommandFailure("Command execution failed", err) from err

so an EOF or socket error inside dialog.process() also arrives as SubCommandFailure. I checked whether the platform plugins avoid this and they don't: nxos has no Execute override (at runtime it's NxosExecute with call_service resolving from generic), iosxe subclasses generic without overriding call_service, and iosxr overrides it only to set detect_state=False before delegating to super().

This does not change my view on the PR, because the recovery you'd be skipping already exists one layer down. I tested it against a mock device — SIGKILL the spawn and its child, then execute() again, and pyATS/unicon transparently re-establishes: new pid, new fd, spawn object replaced, no exception reaching the caller. So the broker's disconnect was largely redundant with unicon's own healing, which is consistent with this having run clean in your live environment.

I looked at discriminating on e.__cause__ and decided against suggesting it: it relies on unicon keeping the from err, the inner exception type isn't a stable taxonomy (a plain NX-OS rejection has a SubCommandFailure as its own __cause__), and the payoff would only be an earlier disconnect that unicon makes unnecessary anyway. Arguably negative, in fact — _disconnect_device_internal() wipes the command cache, and a show output doesn't become stale just because a session dropped.

What I would like is a two-line comment in the code capturing the resulting design point, so it's deliberate rather than rediscovered during an incident:

# SubCommandFailure is a fast path, not a health guarantee - unicon wraps
# transport errors in it too. Those get neither a disconnect here nor a retry
# in _execute_command; recovery relies on unicon re-establishing the session
# on the next execute(), which it does transparently.

Question on the measured impact

Here I think one of us is missing something, and it's quite possibly me. Reading the code, a rejection isn't written to either cache — the broker only calls cache.set() on the success path, and ssh_base_test.py:201 builds a CommandCache per test instance rather than sharing one. That reads as though each of the 30+ BGP tests still pays the full ~4.4s rejection, and what this PR removes is the reconnect-and-replay amplification rather than the repeated cost itself.

That doesn't square with the 45% outlier figure, and you measured it on real hardware while I'm reasoning from source, so I'd rather ask than assert: where does the repeat get absorbed? If it genuinely doesn't, that's not a problem with this PR — the amplification removal stands on its own — but it might be worth a follow-up for negative caching, and the PR description would be worth softening slightly.

Merge order

I'd like #899 to go in first, since it's lab-validated as it stands and I'd rather not disturb that branch. This one then needs a rebase, and note the disconnect will have moved: the except SubCommandFailure belongs in _run_and_cache, not _execute_command. Putting it in the old location would leave it as dead code — though your new integration test would catch that, which is a nice property.

Nit

# unreleased → # Unreleased, to match the existing convention in the file.

---

So: no objections to the approach, and nothing here that I'd expect to change the shape of the fix. Ping me once #899 is merged and this is rebased, and I'll do the final pass and approve on the post-rebase diff.

[ChristopherJHart]

Thanks for the thorough analysis — rebased onto main now that #899 is in. Here's what changed and responses to your points:

On the premise / design-point comment — Added verbatim. It's in _run_and_cache right above the except SubCommandFailure catch:

# SubCommandFailure is a fast path, not a health guarantee - unicon wraps
# transport errors in it too. Those get neither a disconnect here nor a retry
# in _execute_command; recovery relies on unicon re-establishing the session
# on the next execute(), which it does transparently.

On the measured impact — You're right. CommandCache is per-test-instance, so the per-rejection cost (~4.4s) is still paid on every test that hits an unsupported command. What this PR eliminates is the reconnect-and-replay amplification — previously each rejection also tore down SSH and wiped the cache, turning 4.4s into 20.8s. I've softened the PR description accordingly and noted that negative caching could be a follow-up to reduce the repeated base cost.

Merge order / rebase — Done. The except SubCommandFailure now lives in _run_and_cache where the disconnect happens, not in _execute_command. _is_transport_failure already excludes SubCommandFailure from the retry path, so both layers are consistent.

Changelog nit — Fixed: # Unreleased.

All 27 integration tests pass, including the two new ones.

[ChristopherJHart]

@oboehmer reached out via Webex and asked to defer the SubCommandFailure import inside _run_and_cache rather than keeping it at module level — consistent with how _is_transport_failure handles its unicon imports. Updated and force-pushed.

[oboehmer]

Thanks — rebase looks clean, and this addressed everything from the last round. The design-point comment is in _run_and_cache verbatim, the except SubCommandFailure moved to the right layer, # Unreleased is fixed, and I appreciate you re-checking the perf claim and reframing it around the amplification rather than the base cost. That was the one substantive question and it's settled.

And yes — the deferred import inside _run_and_cache was my request over Webex. For anyone reading later: pyATS isn't supported on win32 at all, so that function never executes there and the module-level Windows guard was protecting a path that can't be reached. Deferring it also matches how _is_transport_failure handles its unicon imports.

Three things I'd like fixed before this merges. None are architectural and none should take long, but I'd rather they go in with the change than trail it.

1. Lint is red

ruff flags a violation in each of the two changed files — one import ordering, one formatting. Both auto-fixable.

2. _run_and_cache docstring now contradicts the code

Raises:
    Exception: Whatever the device layer raised, after tearing the
        connection down so it is not handed to the next caller.

That's precisely the invariant this PR breaks, and it sits directly above the new comment explaining why. The comment you added is good, but a reader hits the docstring first and it now states the opposite. Suggest:

Raises:
    SubCommandFailure: Re-raised as-is. The device answered and rejected
        the command, so the session and its cache are left intact.
    Exception: Any other failure, after tearing the connection down so it
        is not handed to the next caller.

3. test_transport_failure_disconnects_and_wipes_cache covers more than it says

Since OSError is in the _is_transport_failure tuple and the side_effect persists, that test drives the full #899 retry path. I instrumented it to confirm:

execute()    called 3x   (1 success + 2 failures)
disconnect() called 2x

The assertions still hold and the test should stay — but the name and docstring ("regression guard for existing behavior") describe single-shot behaviour, so the retry-exhaustion coverage is currently accidental. That's the kind of thing that gets "simplified" away later without anyone realising what was lost. Two lines make it deliberate:

# OSError is a transport failure, so this exercises the full #899 path:
# fail -> disconnect -> reconnect -> fail -> disconnect -> raise
assert good_device.execute.call_count == 3
assert good_device.disconnect.call_count == 2

One note for the record

test_sub_command_failure_preserves_connection_and_cache is the only test guarding this PR's actual behaviour change. The unit test at test_connection_broker.py:424 mocks _disconnect_device and never asserts it wasn't called — as its own docstring says, it deferred that question to #900. I verified by deleting the except SubCommandFailure block: that integration test is the single one that fails. Worth knowing before anyone prunes it as overlapping with the unit suite.

Ping me once those three are in and CI is green, and I'll approve and merge.