ING-1399: Fix flakiness in graceful shutdown tests

[Westwooo]

This PR changes how we test graceful shutdown, we now ensure that:

1. Requests received by the HTTP and gRPC server before shutdown begins are allowed to complete
2. Requests received before shutdown that take longer than the shutdown timeout are forcible cancelled

To support configurable server side delays we use the barrier hooks for gRPC. This meant that we had to expose the hooks manager through the startup call back, as we can't use an RPC to unblock the hook as the server is shutting down so this must be done in process. Such middleware was not implemented for Data API so this has been added and put behind the Debug flag.

The original plan was to use slow running queries. However the time taken for a query to execute is non-deterministic so this approach would just introduce a different type of flakiness.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

gateway/gateway.go:444

• ctx, cancel := context.WithCancel(ctx) is created before system.NewSystem(...). If NewSystem returns an error, cancel is never called, which can leak any goroutines tied to that derived context. Consider calling cancel() on the error path (or deferring it until after NewSystem succeeds).

		ctx, cancel := context.WithCancel(ctx)

		config.Logger.Info("initializing protostellar system")
		gatewaySys, err := system.NewSystem(&system.SystemOptions{
			Logger:      config.Logger.Named("gateway-system"),
			DataImpl:    dataImpl,
			DapiImpl:    dapiImpl,
			Metrics:     metrics.GetSnMetrics(),
			RateLimiter: rateLimiter,
			GrpcTlsConfig: &tls.Config{
				ClientCAs:  config.ClientCaCert,
				ClientAuth: tls.VerifyClientCertIfGiven,
				GetCertificate: func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) {
					return g.atomicGrpcCert.Load(), nil
				},
			},
			DapiTlsConfig: &tls.Config{
				GetCertificate: func(chi *tls.ClientHelloInfo) (*tls.Certificate, error) {
					return g.atomicDapiCert.Load(), nil
				},
				ClientCAs:  config.ClientCaCert,
				ClientAuth: tls.VerifyClientCertIfGiven,
			},
			ShutdownTimeout: config.ShutdownTimeout,
			AlphaEndpoints:  config.AlphaEndpoints,
			Debug:           config.Debug,
			Cancel:          cancel,
		})
		if err != nil {
			config.Logger.Error("error creating legacy proxy")
			return err
		}

gateway/test/dapi_graceful_shutdown_test.go:129

• The response body is never closed in the request loop. Not closing resp.Body can leak connections/resources and can also affect keep-alive behavior, which is relevant for a shutdown test. Consider reading/discarding the body (if needed) and calling resp.Body.Close() each iteration.

				return
			}

			respCloseChan <- resp.Close
			time.Sleep(time.Millisecond * 10)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[brett19]

Why did this need to be added? I think the intention was that hooks were required to have 'execute' as part of them?

[Westwooo]

It was originally added because we call ServeHTTP in runACtion_Execute and don't want to call it again in the Run() function. Since ServeHTTP does.t return anything we couldn't do things the same way as grpc where we set a fields for the resp and err and uses that to decide if we ned to execute the request later.

In the latest commit I have added an interceptor writer that allows us to access the http response inside the actions like we do for grpc and we can check if the request has been executed based on that.

[brett19]

Why does the HTTPWriter need to be included here? Shouldn't it flow through the result the same way we do in GRPC, with the exception that we write it out differently at the end?

[Westwooo]

It was included because we actually server the request and wrote the response inside of run state. I;ve changed it so that we use an interceptor to get the http response/error which we now propagate back up to HandleHTTPRequest and then we write the response there.

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.