mirror of
https://github.com/coder/coder.git
synced 2026-06-03 04:58:23 +00:00
04fca84872
## Background A 5000-chat scaletest (~50k turns, ~2m45s wall time) completed successfully, but the main bottleneck was **DB pool starvation from repeated reads**, not individually expensive SQL. The push/webpush path showed a few especially noisy reads: - `GetLastChatMessageByRole` for push body generation - `GetEnabledChatProviders` + `GetChatModelConfigByID` for push summary model resolution - `GetWebpushSubscriptionsByUserID` for every webpush dispatch This PR keeps the optimizations that remove those duplicate reads while leaving stream behavior unchanged. ## What changes in this PR ### 1. Reuse resolved chat state for push notifications `maybeSendPushNotification` used to re-read the last assistant message and re-resolve the chat model/provider after `runChat` had already done that work. Now `runChat` returns the final assistant text plus the already-resolved model and provider keys, and the push goroutine uses that state directly. That removes the extra push-path reads for: - `GetLastChatMessageByRole` - the second `resolveChatModel` path - the provider/model lookups that came with that second resolution ### 2. Cache webpush subscriptions during dispatch `Dispatch()` previously hit `GetWebpushSubscriptionsByUserID` on every push. A small per-user in-memory cache now avoids those repeated reads. The follow-up fix keeps that optimization correct: `InvalidateUser()` bumps a per-user generation so an older in-flight fetch cannot repopulate the cache with pre-mutation data after subscribe/unsubscribe. That preserves the cache win without letting local subscription changes be silently overwritten by stale fetch results. ## Why this is safe - The push change only reuses data already produced during the same chat run. It does not change notification semantics; if there is no assistant text to summarize, the existing fallback body still applies. - The webpush change keeps the existing TTL and `410 Gone` cleanup behavior. The generation guard only prevents stale in-flight fetches from poisoning the shared cache after invalidation. - The final PR does **not** change stream setup, pubsub/relay behavior, or chat status snapshot timing. ## Deliberately not included - No stream-path optimization in `Subscribe`. - No inline pubsub message payloads. - No distributed cross-replica webpush cache invalidation.
144 lines
5.3 KiB
Go
144 lines
5.3 KiB
Go
package coderd_test
|
|
|
|
import (
|
|
"context"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"sync/atomic"
|
|
"testing"
|
|
|
|
"github.com/google/uuid"
|
|
"github.com/stretchr/testify/require"
|
|
"golang.org/x/xerrors"
|
|
|
|
"github.com/coder/coder/v2/coderd/coderdtest"
|
|
"github.com/coder/coder/v2/coderd/database"
|
|
"github.com/coder/coder/v2/coderd/database/dbtestutil"
|
|
"github.com/coder/coder/v2/codersdk"
|
|
"github.com/coder/coder/v2/testutil"
|
|
)
|
|
|
|
const (
|
|
// These are valid keys for a web push subscription.
|
|
// DO NOT REUSE THESE IN ANY REAL CODE.
|
|
validEndpointAuthKey = "zqbxT6JKstKSY9JKibZLSQ=="
|
|
validEndpointP256dhKey = "BNNL5ZaTfK81qhXOx23+wewhigUeFb632jN6LvRWCFH1ubQr77FE/9qV1FuojuRmHP42zmf34rXgW80OvUVDgTk="
|
|
)
|
|
|
|
func TestWebpushSubscribeUnsubscribe(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitShort)
|
|
|
|
client := coderdtest.New(t, &coderdtest.Options{})
|
|
owner := coderdtest.CreateFirstUser(t, client)
|
|
memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
|
|
_, anotherMember := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
|
|
|
|
var handlerCalls atomic.Int32
|
|
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
|
|
w.WriteHeader(http.StatusCreated)
|
|
handlerCalls.Add(1)
|
|
}))
|
|
defer server.Close()
|
|
|
|
// Seed the dispatcher cache with an empty subscription set. Creating the
|
|
// subscription should invalidate that entry so the next dispatch sees the new
|
|
// subscription immediately.
|
|
err := memberClient.PostTestWebpushMessage(ctx)
|
|
require.NoError(t, err, "test webpush message without a subscription")
|
|
require.Zero(t, handlerCalls.Load(), "a user without subscriptions should not receive a push")
|
|
|
|
err = memberClient.PostWebpushSubscription(ctx, "me", codersdk.WebpushSubscription{
|
|
Endpoint: server.URL,
|
|
AuthKey: validEndpointAuthKey,
|
|
P256DHKey: validEndpointP256dhKey,
|
|
})
|
|
require.NoError(t, err, "create webpush subscription")
|
|
require.Equal(t, int32(1), handlerCalls.Load(), "subscription validation should hit the endpoint once")
|
|
|
|
err = memberClient.PostTestWebpushMessage(ctx)
|
|
require.NoError(t, err, "test webpush message after subscribing")
|
|
require.Equal(t, int32(2), handlerCalls.Load(), "the dispatcher should invalidate empty cache entries after subscribing")
|
|
|
|
err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
|
|
Endpoint: server.URL,
|
|
})
|
|
require.NoError(t, err, "delete webpush subscription")
|
|
|
|
err = memberClient.PostTestWebpushMessage(ctx)
|
|
require.NoError(t, err, "test webpush message after unsubscribing")
|
|
require.Equal(t, int32(2), handlerCalls.Load(), "the dispatcher should invalidate cached subscriptions after unsubscribing")
|
|
|
|
// Deleting the subscription for a non-existent endpoint should return a 404.
|
|
err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
|
|
Endpoint: server.URL,
|
|
})
|
|
var sdkError *codersdk.Error
|
|
require.Error(t, err)
|
|
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
|
|
require.Equal(t, http.StatusNotFound, sdkError.StatusCode())
|
|
|
|
// Creating a subscription for another user should not be allowed.
|
|
err = memberClient.PostWebpushSubscription(ctx, anotherMember.ID.String(), codersdk.WebpushSubscription{
|
|
Endpoint: server.URL,
|
|
AuthKey: validEndpointAuthKey,
|
|
P256DHKey: validEndpointP256dhKey,
|
|
})
|
|
require.Error(t, err, "create webpush subscription for another user")
|
|
|
|
// Deleting a subscription for another user should not be allowed.
|
|
err = memberClient.DeleteWebpushSubscription(ctx, anotherMember.ID.String(), codersdk.DeleteWebpushSubscription{
|
|
Endpoint: server.URL,
|
|
})
|
|
require.Error(t, err, "delete webpush subscription for another user")
|
|
}
|
|
|
|
// testWebpushErrorStore wraps a real database.Store and allows injecting
|
|
// errors into GetWebpushSubscriptionsByUserID.
|
|
type testWebpushErrorStore struct {
|
|
database.Store
|
|
getWebpushSubscriptionsErr atomic.Pointer[error]
|
|
}
|
|
|
|
func (s *testWebpushErrorStore) GetWebpushSubscriptionsByUserID(ctx context.Context, userID uuid.UUID) ([]database.WebpushSubscription, error) {
|
|
if err := s.getWebpushSubscriptionsErr.Load(); err != nil {
|
|
return nil, *err
|
|
}
|
|
return s.Store.GetWebpushSubscriptionsByUserID(ctx, userID)
|
|
}
|
|
|
|
func TestDeleteWebpushSubscription(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
t.Run("database error returns 500", func(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
ctx := testutil.Context(t, testutil.WaitMedium)
|
|
|
|
store, ps := dbtestutil.NewDB(t)
|
|
wrappedStore := &testWebpushErrorStore{Store: store}
|
|
|
|
client := coderdtest.New(t, &coderdtest.Options{
|
|
Database: wrappedStore,
|
|
Pubsub: ps,
|
|
})
|
|
owner := coderdtest.CreateFirstUser(t, client)
|
|
memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
|
|
|
|
// Inject a database error into
|
|
// GetWebpushSubscriptionsByUserID. The handler should
|
|
// return 500, not mask the error as 404.
|
|
dbErr := xerrors.New("database is unavailable")
|
|
wrappedStore.getWebpushSubscriptionsErr.Store(&dbErr)
|
|
|
|
err := memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
|
|
Endpoint: "https://push.example.com/test",
|
|
})
|
|
var sdkError *codersdk.Error
|
|
require.Error(t, err)
|
|
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
|
|
require.Equal(t, http.StatusInternalServerError, sdkError.StatusCode(), "database errors should return 500, not be masked as 404")
|
|
})
|
|
}
|