Flaky test: closes_on_malformed_json fails when Ping frame arrives before Close frame

[leynos]

Description

The test backend::inbound::ws::session::tests::closes_on_malformed_json intermittently fails in CI with:

thread 'inbound::ws::session::tests::closes_on_malformed_json' panicked at backend/src/inbound/ws/session_tests.rs:136:18: expected close frame, got Ping(b"")

Root Cause

The test assumes the next frame after sending malformed JSON will be a Close frame. However, the WebSocket session has a heartbeat mechanism that sends Ping frames at regular intervals. When timing aligns unfavorably, a Ping frame can arrive before the Close frame, causing the assertion to fail.

Current Code (lines 131-137)

  let frame = socket.next().await.expect("response frame").expect("frame");
  match frame {
      Frame::Close(reason) => {
          assert_eq!(reason.expect("reason").code, CloseCode::Policy);
      }
      other => panic!("expected close frame, got {other:?}"),
  }

Suggested Fix

Follow the pattern used in closes_after_timeout_without_client_messages which correctly handles intermediate Ping/Pong frames by looping until a Close frame is received:

  let frame = loop {
      let frame = socket.next().await.expect("response frame").expect("frame");
      match frame {
          Frame::Ping(_) | Frame::Pong(_) => continue,
          other => break other,
      }
  };

  match frame {
      Frame::Close(reason) => {
          assert_eq!(reason.expect("reason").code, CloseCode::Policy);
      }
      other => panic!("expected close frame, got {other:?}"),
  }

Reproduction

The failure is timing-dependent and occurs sporadically. It was observed during CI runs for PR #277 but passes on local rerun.

Location

• File: backend/src/inbound/ws/session_tests.rs:122-138
• Test: closes_on_malformed_json

Labels

bug, flaky-test, backend

[traycerai]

Plan

Observations

The test closes_on_malformed_json in file:backend/src/inbound/ws/session_tests.rs fails intermittently because it assumes the next frame after sending malformed JSON will be a Close frame. However, the WebSocket session has a heartbeat mechanism (defined in file:backend/src/inbound/ws/session.rs) that sends Ping frames every 50ms in test mode. When timing aligns unfavorably, a Ping frame arrives before the Close frame, causing the test to panic. The codebase already has the correct pattern implemented in the closes_after_timeout_without_client_messages test and the next_text_frame helper function.

Approach

The fix follows the established pattern already present in the test file: loop through incoming frames and skip Ping/Pong frames until the expected Close frame is received. This approach is consistent with how closes_after_timeout_without_client_messages (lines 142-172) and next_text_frame (lines 59-68) handle intermediate heartbeat frames. The solution is minimal, requires no architectural changes, and aligns with existing test patterns in the codebase.

Implementation Steps

1. Update the closes_on_malformed_json Test

Location: file:backend/src/inbound/ws/session_tests.rs, lines 131-137

Replace the direct frame assertion with a loop that skips Ping/Pong frames:

Current code (lines 131-137):

let frame = socket.next().await.expect("response frame").expect("frame");
match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}

Updated approach:

• Add a loop that continuously reads frames from the socket
• Skip Frame::Ping(_) and Frame::Pong(_) frames by continuing the loop
• Break the loop when any other frame type is encountered
• Assert that the received frame is a Frame::Close with CloseCode::Policy

This pattern matches the implementation in closes_after_timeout_without_client_messages (lines 152-163):

let frame = loop {
    let frame = socket.next().await.expect("response frame").expect("frame");
    match frame {
        Frame::Ping(_) | Frame::Pong(_) => continue,
        other => break other,
    }
};

match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}

2. Verification

After implementing the fix:

• The test should handle Ping frames that arrive before the Close frame without panicking
• The test behavior remains unchanged when no Ping frames arrive (Close frame comes first)
• The fix is consistent with other tests in the file that handle WebSocket frames
• No changes to production code (file:backend/src/inbound/ws/session.rs) are required

Architecture Context

sequenceDiagram
    participant Test as Test Client
    participant WS as WebSocket Session
    participant HB as Heartbeat Timer
    
    Test->>WS: Send malformed JSON
    WS->>WS: Parse fails (line 171-179)
    WS->>WS: Prepare Close frame (CloseCode::Policy)
    
    par Concurrent execution
        HB->>WS: Heartbeat tick (every 50ms)
        WS->>Test: Ping frame (line 109)
    and
        WS->>Test: Close frame
    end
    
    Note over Test: Race condition: Ping may arrive before Close
    
    Test->>Test: Loop through frames
    Test->>Test: Skip Ping/Pong
    Test->>Test: Assert Close frame received

Loading

Files Modified

File	Lines	Change Type
file:backend/src/inbound/ws/session_tests.rs	131-137	Modify test assertion logic

Import In IDE

🤖 Prompt for AI Agents

## Observations

The test `closes_on_malformed_json` in `file:backend/src/inbound/ws/session_tests.rs` fails intermittently because it assumes the next frame after sending malformed JSON will be a Close frame. However, the WebSocket session has a heartbeat mechanism (defined in `file:backend/src/inbound/ws/session.rs`) that sends Ping frames every 50ms in test mode. When timing aligns unfavorably, a Ping frame arrives before the Close frame, causing the test to panic. The codebase already has the correct pattern implemented in the `closes_after_timeout_without_client_messages` test and the `next_text_frame` helper function.

## Approach

The fix follows the established pattern already present in the test file: loop through incoming frames and skip Ping/Pong frames until the expected Close frame is received. This approach is consistent with how `closes_after_timeout_without_client_messages` (lines 142-172) and `next_text_frame` (lines 59-68) handle intermediate heartbeat frames. The solution is minimal, requires no architectural changes, and aligns with existing test patterns in the codebase.

## Implementation Steps

### 1. Update the `closes_on_malformed_json` Test

**Location**: `file:backend/src/inbound/ws/session_tests.rs`, lines 131-137

Replace the direct frame assertion with a loop that skips Ping/Pong frames:

**Current code (lines 131-137)**:
```rust
let frame = socket.next().await.expect("response frame").expect("frame");
match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}
```

**Updated approach**:
- Add a loop that continuously reads frames from the socket
- Skip `Frame::Ping(_)` and `Frame::Pong(_)` frames by continuing the loop
- Break the loop when any other frame type is encountered
- Assert that the received frame is a `Frame::Close` with `CloseCode::Policy`

This pattern matches the implementation in `closes_after_timeout_without_client_messages` (lines 152-163):
```rust
let frame = loop {
    let frame = socket.next().await.expect("response frame").expect("frame");
    match frame {
        Frame::Ping(_) | Frame::Pong(_) => continue,
        other => break other,
    }
};

match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}
```

### 2. Verification

After implementing the fix:

- The test should handle Ping frames that arrive before the Close frame without panicking
- The test behavior remains unchanged when no Ping frames arrive (Close frame comes first)
- The fix is consistent with other tests in the file that handle WebSocket frames
- No changes to production code (`file:backend/src/inbound/ws/session.rs`) are required

### Architecture Context

```mermaid
sequenceDiagram
    participant Test as Test Client
    participant WS as WebSocket Session
    participant HB as Heartbeat Timer
    
    Test->>WS: Send malformed JSON
    WS->>WS: Parse fails (line 171-179)
    WS->>WS: Prepare Close frame (CloseCode::Policy)
    
    par Concurrent execution
        HB->>WS: Heartbeat tick (every 50ms)
        WS->>Test: Ping frame (line 109)
    and
        WS->>Test: Close frame
    end
    
    Note over Test: Race condition: Ping may arrive before Close
    
    Test->>Test: Loop through frames
    Test->>Test: Skip Ping/Pong
    Test->>Test: Assert Close frame received
```

### Files Modified

| File | Lines | Change Type |
|------|-------|-------------|
| `file:backend/src/inbound/ws/session_tests.rs` | 131-137 | Modify test assertion logic |

---

Execution Information

Branch: main
Commit: c491524

---

💡 Tips

Supported Commands (Inside Comments)

• Use @traycerai generate to iterate on the previous version of the implementation plan.

Supported Commands (Inside Description)

• Add @traycerai ignore anywhere in the ticket description to prevent this ticket from being processed.
• Add @traycerai branch:<branch-name> anywhere in the ticket description to specify the target branch for the implementation plan.

Community

• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

📝 CodeRabbit Plan Mode

Generate an implementation plan and prompts that you can use with your favorite coding agent.

• Create Plan

Examples

• Example 1
• Example 2

---

🔗 Similar Issues

Related Issues

• Remove wall-clock sleeps from exponential backoff unit test wireframe#310
• Add async test for accept_loop backoff behaviour wireframe#294
• Flaky test: hnsw_pipeline_matches_exact_baseline::case_1 intermittently fails in CI chutoro#70
• Enhance connection_panic_is_caught test to assert on panic log output wireframe#216
• handle_connection method is a stub that immediately closes connections wireframe#33

👤 Suggested Assignees

• coderabbitai

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[traycerai]

I have updated the Implementation Plan to reflect the recent changes in the ticket.

New Plan

Observations

The test closes_on_malformed_json in file:backend/src/inbound/ws/session_tests.rs fails intermittently because it assumes the next frame after sending malformed JSON will be a Close frame. However, the WebSocket session has a heartbeat mechanism (defined in file:backend/src/inbound/ws/session.rs) that sends Ping frames every 50ms in test mode. When timing aligns unfavorably, a Ping frame arrives before the Close frame, causing the test to panic. The codebase already has the correct pattern implemented in the closes_after_timeout_without_client_messages test and the next_text_frame helper function.

Approach

The fix follows the established pattern already present in the test file: loop through incoming frames and skip Ping/Pong frames until the expected Close frame is received. This approach is consistent with how closes_after_timeout_without_client_messages (lines 142-172) and next_text_frame (lines 59-68) handle intermediate heartbeat frames. The solution is minimal, requires no architectural changes, and aligns with existing test patterns in the codebase.

Implementation Steps

1. Update the closes_on_malformed_json Test

Location: file:backend/src/inbound/ws/session_tests.rs, lines 131-137

Replace the direct frame assertion with a loop that skips Ping/Pong frames:

Current code (lines 131-137):

let frame = socket.next().await.expect("response frame").expect("frame");
match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}

Updated approach:

• Add a loop that continuously reads frames from the socket
• Skip Frame::Ping(_) and Frame::Pong(_) frames by continuing the loop
• Break the loop when any other frame type is encountered
• Assert that the received frame is a Frame::Close with CloseCode::Policy

This pattern matches the implementation in closes_after_timeout_without_client_messages (lines 152-163):

let frame = loop {
    let frame = socket.next().await.expect("response frame").expect("frame");
    match frame {
        Frame::Ping(_) | Frame::Pong(_) => continue,
        other => break other,
    }
};

match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}

2. Verification

After implementing the fix:

• The test should handle Ping frames that arrive before the Close frame without panicking
• The test behavior remains unchanged when no Ping frames arrive (Close frame comes first)
• The fix is consistent with other tests in the file that handle WebSocket frames
• No changes to production code (file:backend/src/inbound/ws/session.rs) are required

Architecture Context

sequenceDiagram
    participant Test as Test Client
    participant WS as WebSocket Session
    participant HB as Heartbeat Timer
    
    Test->>WS: Send malformed JSON
    WS->>WS: Parse fails (line 171-179)
    WS->>WS: Prepare Close frame (CloseCode::Policy)
    
    par Concurrent execution
        HB->>WS: Heartbeat tick (every 50ms)
        WS->>Test: Ping frame (line 109)
    and
        WS->>Test: Close frame
    end
    
    Note over Test: Race condition: Ping may arrive before Close
    
    Test->>Test: Loop through frames
    Test->>Test: Skip Ping/Pong
    Test->>Test: Assert Close frame received

Loading

Files Modified

File	Lines	Change Type
file:backend/src/inbound/ws/session_tests.rs	131-137	Modify test assertion logic

Import In IDE

🤖 Prompt for AI Agents

## Observations

The test `closes_on_malformed_json` in `file:backend/src/inbound/ws/session_tests.rs` fails intermittently because it assumes the next frame after sending malformed JSON will be a Close frame. However, the WebSocket session has a heartbeat mechanism (defined in `file:backend/src/inbound/ws/session.rs`) that sends Ping frames every 50ms in test mode. When timing aligns unfavorably, a Ping frame arrives before the Close frame, causing the test to panic. The codebase already has the correct pattern implemented in the `closes_after_timeout_without_client_messages` test and the `next_text_frame` helper function.

## Approach

The fix follows the established pattern already present in the test file: loop through incoming frames and skip Ping/Pong frames until the expected Close frame is received. This approach is consistent with how `closes_after_timeout_without_client_messages` (lines 142-172) and `next_text_frame` (lines 59-68) handle intermediate heartbeat frames. The solution is minimal, requires no architectural changes, and aligns with existing test patterns in the codebase.

## Implementation Steps

### 1. Update the `closes_on_malformed_json` Test

**Location**: `file:backend/src/inbound/ws/session_tests.rs`, lines 131-137

Replace the direct frame assertion with a loop that skips Ping/Pong frames:

**Current code (lines 131-137)**:
```rust
let frame = socket.next().await.expect("response frame").expect("frame");
match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}
```

**Updated approach**:
- Add a loop that continuously reads frames from the socket
- Skip `Frame::Ping(_)` and `Frame::Pong(_)` frames by continuing the loop
- Break the loop when any other frame type is encountered
- Assert that the received frame is a `Frame::Close` with `CloseCode::Policy`

This pattern matches the implementation in `closes_after_timeout_without_client_messages` (lines 152-163):
```rust
let frame = loop {
    let frame = socket.next().await.expect("response frame").expect("frame");
    match frame {
        Frame::Ping(_) | Frame::Pong(_) => continue,
        other => break other,
    }
};

match frame {
    Frame::Close(reason) => {
        assert_eq!(reason.expect("reason").code, CloseCode::Policy);
    }
    other => panic!("expected close frame, got {other:?}"),
}
```

### 2. Verification

After implementing the fix:

- The test should handle Ping frames that arrive before the Close frame without panicking
- The test behavior remains unchanged when no Ping frames arrive (Close frame comes first)
- The fix is consistent with other tests in the file that handle WebSocket frames
- No changes to production code (`file:backend/src/inbound/ws/session.rs`) are required

### Architecture Context

```mermaid
sequenceDiagram
    participant Test as Test Client
    participant WS as WebSocket Session
    participant HB as Heartbeat Timer
    
    Test->>WS: Send malformed JSON
    WS->>WS: Parse fails (line 171-179)
    WS->>WS: Prepare Close frame (CloseCode::Policy)
    
    par Concurrent execution
        HB->>WS: Heartbeat tick (every 50ms)
        WS->>Test: Ping frame (line 109)
    and
        WS->>Test: Close frame
    end
    
    Note over Test: Race condition: Ping may arrive before Close
    
    Test->>Test: Loop through frames
    Test->>Test: Skip Ping/Pong
    Test->>Test: Assert Close frame received
```

### Files Modified

| File | Lines | Change Type |
|------|-------|-------------|
| `file:backend/src/inbound/ws/session_tests.rs` | 131-137 | Modify test assertion logic |

---

Execution Information

Branch: main
Commit: c491524