fix(test): guard YAMCS event subscription teardown against websocket race

[Mikefly123]

Fixes the recurring integration-uart failure tracked in #514.

Root cause

The round-trip test was passing and then failing in teardown. In both failing runs the whole test completed in about a second (1 failed in 0.97s, 1 failed in 1.03s) — far inside the 180 s budget — meaning the NoOpReceived event was observed and the test returned normally. The only exception came from subscription.cancel() in the finally block:

finally:
>   subscription.cancel()
...
websocket/_app.py:211: AttributeError: 'NoneType' object has no attribute 'close_frame'

WebSocketApp.close() has a check-then-use race on self.sock:

if self.sock:
    self.sock.close(**kwargs)              # wakes the reader thread, which sets self.sock = None
    if self.sock.close_frame is not None:  # dereferenced again -> AttributeError

The race is still present in websocket-client 1.9.1, so bumping the dependency does not help.

Corrections to the analysis in #514

• "Subscription never established" — not possible: create_event_subscription blocks on subscription.reply(timeout=60), so a failed handshake raises at subscription time rather than yielding a silently empty queue. No readiness check is needed.
• "Test burned the full timeout" — it did not; see the sub-second durations above.
• "AssertionError was the real failure, masked by teardown" — inverted. The AssertionError was never raised; pytest was only echoing the test's source in the traceback context. Teardown was the sole failure.
• "Bump yamcs-client 1.12.1" — wrong layer, and the bug is unpatched upstream anyway.

Only the "guard the teardown" suggestion from the issue holds, and it is the complete fix.

Change

Wrap subscription.cancel() in try/except and log at warning level, so a teardown error can never replace the test's real result.

Evidence

Attempt-1 logs of runs 33270707473 and 33325337362: identical traceback, both sub-second, both raised through finally: subscription.cancel().

Out of scope

main has been red on integration-uart since the F Prime 4.3.0 upgrade, but that failure occurs at the UART pytest step, before YAMCS starts — a different signature, not addressed here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WBD3NFb2h8TxkuxrfSRyhS

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5ca032d6-0c71-4d24-9b7f-8da9e50b22e4

📥 Commits

Reviewing files that changed from the base of the PR and between a477893 and 9512966.

📒 Files selected for processing (1)

• PROVESFlightControllerReference/test/yamcs/test_yamcs_noop.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Improved test cleanup reliability by safely handling errors during event-subscription teardown.
  • Teardown issues are now logged as warnings instead of masking the test’s actual result.

Walkthrough

The YAMCS noop test now logs exceptions from subscription cancellation during teardown. This prevents cancellation errors from replacing the test result.

Changes

YAMCS teardown handling

Layer / File(s)	Summary
Safe subscription cancellation PROVESFlightControllerReference/test/yamcs/test_yamcs_noop.py	The test imports logging and wraps subscription.cancel() in exception handling. Cancellation failures are logged as warnings with exc_info=True.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 95129

This change prevents a websocket teardown race from masking the completed integration-test result without affecting production behavior. No actionable merge-blocking risk remains after normal checks and review.

Poem

A rabbit watched the socket close,
While warnings tracked the race.
The test result stayed clear and bright,
Through teardown’s troubled space.
Logging kept the carrot safe.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: guarding YAMCS event subscription teardown against a websocket race.
Description check	✅ Passed	The description is detailed and directly explains the issue, root cause, correction, evidence, and scope. It references issue #514 and provides failure evidence. It does not explicitly complete the te…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description is detailed and directly explains the issue, root cause, correction, evidence, and scope. It references issue #514 and provides failure evidence. It does not explicitly complete the testing checklist or provide reproducible test instructions, but the description is mostly complete.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}