client, test/suites: forward session timeout cause over websocket by shardool-patil · Pull Request #1519 · canonical/microcloud

shardool-patil · 2026-08-21T09:51:03Z

Description

When a trust establishment session expires, the client previously received a generic connection error (websocket: close 1006: unexpected EOF) instead of the actual cause (Session timeout exceeded)

Changes

client/websocket.go:
- Updated WriteClose to write ControlClose directly over the WebSocket under writeLock so close frames are sent even when gwCtx is canceled on timeout
- Updated ReceiveWithContext to check channel closure and return context.Cause(w.ctx)
test/suites/basic.sh:
- Added a test case in test_interactive verifying that both the initiator and joiner receive "Session timeout exceeded"

Checklist

I have read the contributing guidelines and attest that all commits in this PR are signed off, cryptographically signed, and follow this project's commit structure.
I have checked and added or updated relevant documentation.

Related issues

Fixes Session expiry isn't forwarded to the client #1245

roosterfish

Thanks for taking a look at this, please see my comments on the test.

Copilot

Pull request overview

This PR improves MicroCloud’s WebSocket session handling so that when a trust establishment session expires, the client receives the real cancellation cause (e.g., “Session timeout exceeded”) rather than a generic abnormal WebSocket EOF error, aligning behavior with issue #1245.

Changes:

Adjusts the WebSocket gateway close-path to reliably send a ControlClose message even when the gateway context is canceled.
Improves receive behavior to surface the gateway context cause when the receive channel closes.
Adds a system test intended to validate that both initiator and joiner surface the timeout error.

Reviewed changes

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

File	Description
client/websocket.go	Ensures close-cause forwarding works reliably and improves receive-side error reporting when the socket closes.
test/suites/basic.sh	Adds an interactive test case intended to validate session-timeout error propagation to initiator/joiner.

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

shardool-patil · 2026-08-21T11:24:35Z

@roosterfish i have addressed all the requested changes:

test/suites/basic.sh: Set EXPECT_PEERS=2, clarified the test comment and echo message to verify both initiator and joiner, and added the missing EOF newline[cite: 2, 3].
test/includes/microcloud.sh: Updated microcloud_interactive to pass ${SESSION_TIMEOUT:-60} dynamically and added SESSION_TIMEOUT to unset_interactive_vars[cite: 2].

take a look whenever possible

roosterfish · 2026-08-21T13:35:57Z

See the test failure https://github.com/canonical/microcloud/actions/runs/32477357735/job/96766897819?pr=1519#step:3:45500.
I suspect what is happening is that the session expires too early now. Maybe let's leave the 60s default (no need to make it configurable in this case).

shardool-patil · 2026-08-21T16:43:09Z

See the test failure https://github.com/canonical/microcloud/actions/runs/32477357735/job/96766897819?pr=1519#step:3:45500. I suspect what is happening is that the session expires too early now. Maybe let's leave the 60s default (no need to make it configurable in this case).

@roosterfish I have worked and handled the requested changes so take a look at the code.

shardool-patil · 2026-08-25T02:27:42Z

@roosterfish I switched the test to reset_systems 3 0 0 because with reset_systems 2 0 0, micro03 is paused by default. Since EXPECT_PEERS=2 triggers table:expect 2, micro01 hangs during mDNS peer discovery waiting for a nonexistent second peer before the session ever starts. Running reset_systems 3 0 0 allows micro01 to discover both peers, select them, connect with micro02, and wait for micro03 until the session timeout triggers. Does reset_systems 3 0 0 work for you here?

shardool-patil · 2026-08-25T16:19:32Z

@roosterfish I've updated test/suites/basic.sh:

Switched to reset_systems 3 0 0 and disabled storage setup (SETUP_ZFS="no", SETUP_CEPH="no").
Kept EXPECT_PEERS=2 with join_session init micro01 micro02 so micro01 connects with micro02 and waits for micro03 until the default 60s session timeout triggers.
With reset_systems 2 0 0, micro03 is paused by default, causing micro01 to hang during mDNS peer discovery (table:expect 2) before the session can start. 3 0 0 keeps micro03 running so discovery succeeds without configuring any unused storage devices.

Please take a look at this setup before re-running the tests.

Copilot

Pull request overview

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

Suppressed comments (1)

test/suites/basic.sh:447

The new test claims a short session timeout, but join_session runs microcloud init/add with a hard-coded --session-timeout=60 (see test/includes/microcloud.sh:200-204). As written, this test will likely wait ~60s for the session to expire, which slows the suite and doesn’t match the PR description (SESSION_TIMEOUT=5). Consider making the timeout configurable (e.g., env var) and setting it to 5 for this case.

  # Initiate a MicroCloud cluster with a session timeout and verify both initiator and joiner receive "Session timeout exceeded".
  reset_systems 3 0 0
  unset_interactive_vars

  echo "Initiate a MicroCloud cluster with a short session timeout and verify both initiator and joiner receive the timeout error"

roosterfish · 2026-10-21T09:04:20Z

 func (w *WebsocketGateway) Write(v any) error {
 	w.writeLock.Lock()
 	defer w.writeLock.Unlock()

-	if w.ctx.Err() != nil {
-		return context.Cause(w.ctx)
-	}
-
 	return w.conn.WriteJSON(v)


@shardool-patil have you seen this one?

I've restored the cancellation guard in Write()

roosterfish · 2026-09-02T12:07:12Z

 		return fmt.Errorf("Failed to write control message: %w", writeErr)
 	}

-	// Wait on the other end to hang up.


Why don't we have to wait anymore for the other site to hang up in case we signal close of the websocket?

While debugging earlier test failures, I removed that trying to simplify WriteClose, but realized in hindsight that was a mistake. We definitely still need to wait for the other end to hang up so the connection isn't severed before the close frame is processed. I've restored the wait logic, so take a look at this when you get time @roosterfish

shardool-patil · 2026-09-21T09:42:27Z

@roosterfish just a ping, to check the status

roosterfish · 2026-10-21T09:03:25Z

-				// Cancel the inner context with the respective error.
-				defer gwCancel(errors.New(controlClose.ControlMessage))
+				var closeErr error
+				if controlClose.ControlMessage != "" {


Can you please add a comment why we are overriding the received control message here if it is set?

Added an inline comment

roosterfish · 2026-10-21T09:04:20Z

 func (w *WebsocketGateway) Write(v any) error {
 	w.writeLock.Lock()
 	defer w.writeLock.Unlock()

-	if w.ctx.Err() != nil {
-		return context.Cause(w.ctx)
-	}
-
 	return w.conn.WriteJSON(v)


@shardool-patil have you seen this one?

roosterfish · 2026-10-21T09:05:06Z

-// Unlike the actual websocket control close message this supports message longer than 125 bytes
+// Unlike the actual websocket control close message this supports messages longer than 125 bytes
 // as well as special characters.
-// It waits for the other side to hang up or the gateway's context being cancelled.


Why dropping this comment?

roosterfish · 2026-10-21T09:05:49Z

-		err = json.Unmarshal(bytes, v)
+	case data, ok := <-w.Receive():
+		if !ok {
+			if w.ctx.Err() != nil {


Could you please add a comment when this is happening?

Added an explanatory comment

Copilot

🟡 Changes recommended

Preserve the write cancellation guard while bypassing it only for close frames, and ensure the test actually applies the five-second timeout.

3 open issues

_{Review effort: Lite}

shardool-patil · 2026-10-26T08:14:38Z

 	writeErr := w.Write(ControlClose{
-		ControlMessage: err.Error(),
+		ControlMessage: msg,
 	})


Addressed in the latest push. Restored the cancellation guard in Write()

shardool-patil · 2026-10-26T08:12:53Z

+  export SETUP_CEPH="no"
+  export OVN_WARNING="no"
+
+  ! join_session init micro01 micro02 || false


The test runs against the default 60-second timeout rather than requiring changes to shared testbed helpers in test/includes/microcloud.sh. I have updated the PR description to remove the reference to SESSION_TIMEOUT="5".

@roosterfish PTAL

Signed-off-by: Shardool Patil <shardoolpatil999@gmail.com>

roosterfish requested changes Aug 21, 2026

View reviewed changes

Comment thread test/suites/basic.sh Outdated

Comment thread test/suites/basic.sh Outdated

Comment thread test/suites/basic.sh Outdated

roosterfish requested a lite review from Copilot August 21, 2026 10:11

Copilot started reviewing on behalf of roosterfish August 21, 2026 10:11

View session

Copilot AI reviewed Aug 21, 2026

View reviewed changes

Comment thread test/suites/basic.sh Outdated

Comment thread test/suites/basic.sh Outdated

Comment thread client/websocket.go Outdated

shardool-patil force-pushed the fix/session-timeout-forwarding branch from b287951 to 4ffe082 Compare August 21, 2026 11:28

roosterfish marked this pull request as draft August 21, 2026 15:41

shardool-patil force-pushed the fix/session-timeout-forwarding branch from 4ffe082 to 14a4c0b Compare August 21, 2026 16:41

shardool-patil requested a review from roosterfish August 21, 2026 17:35

roosterfish marked this pull request as ready for review August 24, 2026 11:42

shardool-patil force-pushed the fix/session-timeout-forwarding branch from 14a4c0b to c3f95c6 Compare August 25, 2026 02:27

shardool-patil force-pushed the fix/session-timeout-forwarding branch from c3f95c6 to e98fe8e Compare August 25, 2026 16:16

roosterfish requested changes Aug 26, 2026

View reviewed changes

Comment thread client/websocket.go Outdated

Comment thread client/websocket.go Outdated

Comment thread test/includes/microcloud.sh Outdated

Comment thread test/includes/microcloud.sh Outdated

shardool-patil force-pushed the fix/session-timeout-forwarding branch 2 times, most recently from e0eafdf to e5601b6 Compare August 26, 2026 15:48

roosterfish requested a lite review from Copilot August 26, 2026 15:59

Copilot started reviewing on behalf of roosterfish August 26, 2026 15:59

View session

roosterfish reviewed Aug 26, 2026

View reviewed changes

Comment thread test/suites/basic.sh

Copilot AI reviewed Aug 26, 2026

View reviewed changes

shardool-patil force-pushed the fix/session-timeout-forwarding branch from e5601b6 to b1cfb10 Compare August 26, 2026 17:03

shardool-patil requested a review from roosterfish August 30, 2026 13:08

roosterfish reviewed Sep 2, 2026

View reviewed changes

shardool-patil force-pushed the fix/session-timeout-forwarding branch from b1cfb10 to c6fb5c0 Compare September 3, 2026 02:30

shardool-patil requested a review from roosterfish September 9, 2026 18:03

roosterfish requested changes Oct 21, 2026

View reviewed changes

roosterfish requested a lite review from Copilot October 21, 2026 09:55

Copilot started reviewing on behalf of roosterfish October 21, 2026 09:56

View session

Copilot AI reviewed Oct 21, 2026

View reviewed changes

shardool-patil added 2 commits October 26, 2026 08:08

client: forward session timeout cause over websocket

d0a33db

Signed-off-by: Shardool Patil <shardoolpatil999@gmail.com>

test/suites: add session timeout test to basic suite

b0b4e85

Signed-off-by: Shardool Patil <shardoolpatil999@gmail.com>

shardool-patil force-pushed the fix/session-timeout-forwarding branch from 23e0335 to b0b4e85 Compare October 26, 2026 08:08

Conversation

shardool-patil commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Checklist

Related issues

Uh oh!

roosterfish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shardool-patil commented Aug 21, 2026

Uh oh!

roosterfish commented Aug 21, 2026

Uh oh!

shardool-patil commented Aug 21, 2026

Uh oh!

shardool-patil commented Aug 25, 2026

Uh oh!

shardool-patil commented Aug 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shardool-patil commented Sep 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

shardool-patil commented Aug 21, 2026 •

edited

Loading