From aeff23a753b58ef831e7a52c8f1874d0a8794aee Mon Sep 17 00:00:00 2001
From: George Barnett <gbarnett@apple.com>
Date: Wed, 1 Nov 2023 14:11:03 +0000
Subject: [PATCH] Fix last peer initiated stream ID when quiescing (#1700)

Motivation:

The idle handler records the ID of the last stream created by the remote
peer and uses it when sending GOAWAY frames. In most paths this was
correctly updated according to the role the handler played in the
connection and the stream ID. However, in the quiescing state it was
unconditionally updated. This can lead to cases where the client
attempts to send a GOAWAY with a client initiated stream ID: this is
invalid and NIO HTTP/2 treats it as a connection level error, closing
all open streams.

Modification:

- Conditionally update the last peer initiated stream ID when quiescing

Result:

Fewer connection errors
---
 .../GRPC/GRPCIdleHandlerStateMachine.swift    | 10 ++++++-
 .../GRPCIdleHandlerStateMachineTests.swift    | 28 +++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/Sources/GRPC/GRPCIdleHandlerStateMachine.swift b/Sources/GRPC/GRPCIdleHandlerStateMachine.swift
index f7e0b7581..9da96068d 100644
--- a/Sources/GRPC/GRPCIdleHandlerStateMachine.swift
+++ b/Sources/GRPC/GRPCIdleHandlerStateMachine.swift
@@ -275,7 +275,15 @@ struct GRPCIdleHandlerStateMachine {
       operations.cancelIdleTask(state.idleTask)
 
     case var .quiescing(state):
-      state.lastPeerInitiatedStreamID = streamID
+      switch state.role {
+      case .client where streamID.isServerInitiated:
+        state.lastPeerInitiatedStreamID = streamID
+      case .server where streamID.isClientInitiated:
+        state.lastPeerInitiatedStreamID = streamID
+      default:
+        ()
+      }
+
       state.openStreams += 1
       self.state = .quiescing(state)
 
diff --git a/Tests/GRPCTests/GRPCIdleHandlerStateMachineTests.swift b/Tests/GRPCTests/GRPCIdleHandlerStateMachineTests.swift
index 7a4e568f3..c17da7f99 100644
--- a/Tests/GRPCTests/GRPCIdleHandlerStateMachineTests.swift
+++ b/Tests/GRPCTests/GRPCIdleHandlerStateMachineTests.swift
@@ -535,6 +535,34 @@ class GRPCIdleHandlerStateMachineTests: GRPCTestCase {
     _ = stateMachine.channelInactive()
     stateMachine.ratchetDownGoAwayStreamID().assertDoNothing()
   }
+
+  func testStreamIDWhenQuiescing() {
+    var stateMachine = self.makeClientStateMachine()
+    let op1 = stateMachine.receiveSettings([])
+    op1.assertConnectionManager(.ready)
+
+    // Open a stream so we enter quiescing when receiving the GOAWAY.
+    let op2 = stateMachine.streamCreated(withID: 1)
+    op2.assertDoNothing()
+
+    let op3 = stateMachine.receiveGoAway()
+    op3.assertConnectionManager(.quiescing)
+
+    // Create a new stream. This can happen if the GOAWAY races with opening the stream; HTTP2 will
+    // open and then close the stream with an error.
+    let op4 = stateMachine.streamCreated(withID: 3)
+    op4.assertDoNothing()
+
+    // Close the newly opened stream.
+    let op5 = stateMachine.streamClosed(withID: 3)
+    op5.assertDoNothing()
+
+    // Close the original stream.
+    let op6 = stateMachine.streamClosed(withID: 1)
+    // Now we can send a GOAWAY with stream ID zero (we're the client and the server didn't open
+    // any streams).
+    XCTAssertEqual(op6.sendGoAwayWithLastPeerInitiatedStreamID, 0)
+  }
 }
 
 extension GRPCIdleHandlerStateMachine.Operations {