test: Fix a randomly failed test

[minhtule]

Related Tickets & Documents

• Issue: fix(ssh): sequence channel-request events so the session recorder is in place before they are applied #460

Changes

Skip test and comment out window-change channel request in flaky tests due to race condition (see #460)

[Copilot]

Pull request overview

This PR targets a flaky SSH handler unit test by disabling the window-change path in TestChannelPair_SessionRecorderWriteErrors, due to a known race where the window-change can be processed before serve() finishes creating the session recorder.

Changes:

• Comments out injecting resizeErr and sending window-change in TestChannelPair_SessionRecorderWriteErrors.
• Comments out the assertion that a resize event was recorded, replacing it with a TODO(#460) note.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.45%. Comparing base (1d408f8) to head (d8af112).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #461      +/-   ##
==========================================
- Coverage   90.63%   90.45%   -0.18%     
==========================================
  Files          41       41              
  Lines        2903     2903              
==========================================
- Hits         2631     2626       -5     
- Misses        180      182       +2     
- Partials       92       95       +3     

Flag	Coverage Δ
integration	57.62% <ø> (ø)
unit	85.53% <ø> (-0.18%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sghiocel]

I think this is flaky too, because if rec == nil, it won't panic and EOF checks here will timeout

[sghiocel]

Hm, we could use t.Skip to make it cleaner vs commenting out these lines, but it would require splitting these into two tests ... your call

[sghiocel]

2 comments, decide + fix + merge

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

internal/sshhandler/channel_pair_test.go:682

• The test name suggests multiple recorder write errors are being exercised, but the resize error path is currently disabled (resizeErr/window-change/assertion are commented out). Renaming the test to reflect the remaining behavior will reduce confusion when triaging failures.

func TestChannelPair_SessionRecorderWriteErrors(t *testing.T) {
	channels := newProxyChannels(t, "session")
	channels.recorder.headerErr = errors.New("header write failed")
	//nolint:godox
	// TODO(#460): flaky, the window-change can be handled before serve() creates the recorder.

internal/sshhandler/channel_pair_test.go:691

• This comment says “Both writes failed”, but the resize write failure path is currently commented out. Updating the comment will keep the test intent accurate.

	// channels.sendWindowChange(t, 120, 40)

	// Both writes failed, yet the session keeps going: output still flows and is recorded.
	_, err := channels.target.ch.Write([]byte("survives"))

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/sshhandler/channel_pair_test.go:682

• This test no longer exercises resize write failures (resizeErr/window-change and the resize assertion are commented out), but the test name and later comment still imply both header+resize writes fail. This can mislead future readers and makes it harder to understand what coverage is currently active vs parked behind #460.

func TestChannelPair_SessionRecorderWriteErrors(t *testing.T) {
	channels := newProxyChannels(t, "session")
	channels.recorder.headerErr = errors.New("header write failed")
	//nolint:godox
	// TODO(#460): flaky, the window-change can be handled before serve() creates the recorder.

internal/sshhandler/channel_pair_test.go:726

• Skipping this test permanently drops coverage of the “request handler panic closes channels” behavior. This race can be avoided in the test by waiting until serve() has created/written the recorder header before sending the window-change, making the resize panic deterministic without skipping.

func TestChannelPair_RequestPanicClosesChannels(t *testing.T) {
	t.Skip("TODO(#460): flaky; the window-change can be handled before serve() creates the recorder, so no resize panic fires and the channel stays open")