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)[cite: 5].

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[cite: 5].
- Updated ReceiveWithContext to check channel closure and return context.Cause(w.ctx)[cite: 5].
test/suites/basic.sh:
- Added a test case in test_interactive with SESSION_TIMEOUT="5" verifying that both the initiator and joiner receive "Session timeout exceeded"[cite: 2].

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 i have taken into consideration the insights you provided and resolved the issue, so take a look at it when you get time.

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.

roosterfish · 2026-08-26T07:47:09Z

 		// Send close control message.
 		// Try to send the cause from the outer context if present.
-		_ = gw.WriteClose(context.Cause(gwCtx))
+		cause := context.Cause(gwCtx)


Don't we always have to write the close message? From what I remember it's important that it's of type ControlClose, but we don't have to populate the ControlMessage if there isn't any error.

Here we will only ever send the control message if context.Cause returns a non-nil error.

roosterfish · 2026-08-26T07:49:44Z

-// It waits for the other side to hang up or the gateway's context being cancelled.
 func (w *WebsocketGateway) WriteClose(err error) error {
-	writeErr := w.Write(ControlClose{
+	w.writeLock.Lock()


Why the extra lock? By invoking w.Write, it already obtains the lock for us. I don't see why we have to use w.conn.WriteJSON directly here.

thanks for pointing that out, i have removed the manual lock and reverted to write close

roosterfish · 2026-08-26T07:49:55Z

    set -eu
    if [ "${SKIP_SETUP_LOG}" = 1 ]; then
-      exec > /dev/null 2>&1
+      exec > /dev/null


Why is this necessary?

this was an accidental edit left from local debugging, reverted microcloud.sh back to main.

roosterfish · 2026-08-26T07:51:05Z

            pkill -f \"ceph-osd .* --id \${osd}\"
-            microceph.ceph osd purge \${osd} --yes-i-really-mean-it --force
-            microceph.ceph osd destroy \${osd} --yes-i-really-mean-it --force
+            microceph.ceph osd purge \${osd} --yes-i-really-really-mean-it --force


Why is this necessary? According to ceph osd purge -h it's --yes-i-really-mean-it.

roosterfish · 2026-08-26T16:00:11Z

  lxc exec micro01 -- tail -1 out | grep "User aborted" -q
  lxc exec micro04 -- tail -1 out | grep "Failed waiting during join: Initiator aborted the setup" -q

+  # Initiate a MicroCloud cluster with a session timeout and verify both initiator and joiner receive "Session timeout exceeded".


Please make sure the changes are separated into multiple commits (as you did before).

i have separated the commits so take a look @roosterfish

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"

 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)


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

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

Copilot AI reviewed Aug 26, 2026

View reviewed changes

Comment thread client/websocket.go

Comment on lines 143 to 147

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 added 2 commits August 26, 2026 17:03

client: forward session timeout cause over websocket

1a3cb43

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

test/suites: add session timeout test to basic suite

b1cfb10

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

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

Uh oh!

Conversation

shardool-patil commented Aug 21, 2026

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!

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!

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

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants