Failing to delete the stream from the activeStreams map leading to REFUSED_STREAM errors

[arjan-bal]

Original PR with repro: #8504

After upgrading go-grpc, we started see the following error:

stream terminated by RST_STREAM with error code: REFUSED_STREAM

Upon investigation, it appears that in some scenarios, we fail to properly delete streams from the activeStreams map, resulting in this error being wrongly returned.

I added a test case that seems to trigger the problem:

PS: To test the behavior i added a "ActiveStreamTracker" hook -> im not sure this is the best way but i wanted just to show the problem

end2end_test.go:4015: leak streams:  5

The issue seems to have been introduced in the following PR: #8071

Specifically, the check added here:

grpc-go/internal/transport/http2_server.go

Lines 1357 to 1359 in 57b69b4

	if oldState == streamDone {
	return
	}

If this conditional is removed, all streams are correctly cleaned up from the activeStreams map, and the test passes without leaks.

Root cause

1. The server stream exhausts the stream quota, and starts buffering data frames.
2. The server attempts to finish the RPC gracefully by sending trailers by calling finishStream.
3. The write of the header frame gets blocked due to the pending data frames that are awaiting flow control quota.
4. Once the deadline expires, the server's deadline monitoring timer tries to call closeStream, which is a no-op since the stream state is already marked streamDone by finishStream in point 1.
5. The client sends and RST stream, the server calls closeStream which is again a no-op.

The simplest fix is to remove the guard block added in closeStream in #8071. We may end up pushing redundant cleanupStream events on to the controlbuf, but it doesn't cause correctness issues since cleanupStreamHandler returns early in such cases.

[arjan-bal]

@alanprot will be raising a PR with the fix

[arjan-bal]

We are delaying release v1.75 to get this fix in.

[alanprot]

Question: Do we think that is ok to create this hook (or similar) in order to be able to run the unit test?

[arjan-bal]

Avoiding test only hooks in non-test code would be preferable. I can think of the following way to write a test:

1. create a server which allows 1 concurrent stream using the server option.
2. create a client and start a bi-di stream.
3. Have the server send a response message of size > 65535 bytes to exhaust the client's flow control window. Have the server send another message, which should be queued on the server.
4. Ensure the client DOES NOT read any message.
5. Have the server handler return, ending the stream. This should cause the bug to be triggered.
6. Have the client cancel the RPC by cancelling the stream context.
7. Try creating a single new stream for short duration, say 5 millis. Read one message from the stream. Every time we should get an error CONNECTION_REFUSED without the fix.

The test may be flaky due to a race b/w step 5 & 6. Even if it fails 50% of the time, it should be good enough. With the fix, the test should always pass.

[arjan-bal]

@alanprot I suspect the test might be more complicated than I initially estimated. If you'd like, you can open a PR with just the fix, and I can handle the test in a separate PR.

[alanprot]

Idk if I will have time today tbh.. feel free to open the pr with the test and fix.. :)

Thanks again for looking into this :)

[arjan-bal]

Sounds good, I'll raise a PR.