From 21fc7848a71c45458f3c75258decfe93648f2caf Mon Sep 17 00:00:00 2001 From: Jefferson Rodrigues Date: Sat, 21 Mar 2026 17:51:12 -0300 Subject: [PATCH] fix: revalidatePoolSettings bypasses client cache with WithSkipCache Revalidation was reading from the 1h in-memory cache instead of making a fresh HTTP request. Added WithSkipCache() to detect 403 (tenant suspended/purged) promptly. X-Lerian-Ref: 0x1 --- commons/tenant-manager/mongo/manager.go | 2 +- commons/tenant-manager/mongo/manager_test.go | 88 +++++++++++++++++++ commons/tenant-manager/postgres/manager.go | 2 +- .../tenant-manager/postgres/manager_test.go | 85 ++++++++++++++++++ 4 files changed, 175 insertions(+), 2 deletions(-) diff --git a/commons/tenant-manager/mongo/manager.go b/commons/tenant-manager/mongo/manager.go index f2c7751..cc1aea8 100644 --- a/commons/tenant-manager/mongo/manager.go +++ b/commons/tenant-manager/mongo/manager.go @@ -346,7 +346,7 @@ func (p *Manager) revalidatePoolSettings(tenantID string) { revalidateCtx, cancel := context.WithTimeout(context.Background(), settingsRevalidationTimeout) defer cancel() - _, err := p.client.GetTenantConfig(revalidateCtx, tenantID, p.service) + _, err := p.client.GetTenantConfig(revalidateCtx, tenantID, p.service, client.WithSkipCache()) if err != nil { // If tenant service was suspended/purged, evict the cached connection immediately. // The next request for this tenant will call createConnection, which fetches fresh diff --git a/commons/tenant-manager/mongo/manager_test.go b/commons/tenant-manager/mongo/manager_test.go index b768c96..52b7520 100644 --- a/commons/tenant-manager/mongo/manager_test.go +++ b/commons/tenant-manager/mongo/manager_test.go @@ -1524,6 +1524,94 @@ func TestManager_RevalidateSettings_EvictsSuspendedTenant(t *testing.T) { } } +func TestManager_RevalidateSettings_BypassesClientCache(t *testing.T) { + t.Parallel() + + // This test verifies that revalidatePoolSettings uses WithSkipCache() + // to bypass the client's in-memory cache. Without it, a cached "active" + // response would hide a subsequent 403 (suspended/purged) from tenant-manager. + // + // Setup: The httptest server returns 200 (active) on the first request + // and 403 (suspended) on all subsequent requests. We first call + // GetTenantConfig directly to populate the client cache, then trigger + // revalidatePoolSettings. If WithSkipCache is working, the revalidation + // hits the server (gets 403) and evicts the connection. If the cache + // were used, it would return the stale 200 and the connection would + // remain. + var requestCount atomic.Int32 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + count := requestCount.Add(1) + w.Header().Set("Content-Type", "application/json") + + if count == 1 { + // First request: return active config (populates client cache) + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{ + "id": "tenant-cache-test", + "tenantSlug": "cached-tenant", + "service": "ledger", + "status": "active", + "databases": { + "onboarding": { + "mongodb": {"host": "localhost", "port": 27017, "database": "testdb", "username": "user", "password": "pass"} + } + } + }`)) + + return + } + + // Subsequent requests: return 403 (tenant suspended) + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"code":"TS-SUSPENDED","error":"service suspended","status":"suspended"}`)) + })) + defer server.Close() + + capLogger := testutil.NewCapturingLogger() + tmClient, err := client.NewClient(server.URL, capLogger, client.WithAllowInsecureHTTP(), client.WithServiceAPIKey("test-key")) + require.NoError(t, err) + + // Populate the client cache by calling GetTenantConfig directly + cfg, err := tmClient.GetTenantConfig(context.Background(), "tenant-cache-test", "ledger") + require.NoError(t, err) + assert.Equal(t, "tenant-cache-test", cfg.ID) + assert.Equal(t, int32(1), requestCount.Load(), "should have made exactly 1 HTTP request") + + // Create a manager with a cached connection for this tenant + manager := NewManager(tmClient, "ledger", + WithLogger(capLogger), + WithModule("onboarding"), + WithSettingsCheckInterval(1*time.Millisecond), + ) + + // Pre-populate a cached connection (nil DB to avoid real MongoDB) + manager.connections["tenant-cache-test"] = &MongoConnection{DB: nil} + manager.lastAccessed["tenant-cache-test"] = time.Now() + manager.lastSettingsCheck["tenant-cache-test"] = time.Now() + + // Trigger revalidatePoolSettings -- should bypass cache and hit the server + manager.revalidatePoolSettings("tenant-cache-test") + + // Verify a second HTTP request was made (cache was bypassed) + assert.Equal(t, int32(2), requestCount.Load(), + "revalidatePoolSettings should bypass client cache and make a fresh HTTP request") + + // Verify the connection was evicted (server returned 403) + statsAfter := manager.Stats() + assert.Equal(t, 0, statsAfter.TotalConnections, + "connection should be evicted after revalidation detected suspended tenant via cache bypass") + + // Verify lastAccessed and lastSettingsCheck were cleaned up + manager.mu.RLock() + _, accessExists := manager.lastAccessed["tenant-cache-test"] + _, settingsExists := manager.lastSettingsCheck["tenant-cache-test"] + manager.mu.RUnlock() + + assert.False(t, accessExists, "lastAccessed should be removed for evicted tenant") + assert.False(t, settingsExists, "lastSettingsCheck should be removed for evicted tenant") +} + func TestManager_RevalidateSettings_FailedDoesNotBreakConnection(t *testing.T) { t.Parallel() diff --git a/commons/tenant-manager/postgres/manager.go b/commons/tenant-manager/postgres/manager.go index cabffb6..6c3aca8 100644 --- a/commons/tenant-manager/postgres/manager.go +++ b/commons/tenant-manager/postgres/manager.go @@ -378,7 +378,7 @@ func (p *Manager) revalidatePoolSettings(tenantID string) { revalidateCtx, cancel := context.WithTimeout(context.Background(), settingsRevalidationTimeout) defer cancel() - config, err := p.client.GetTenantConfig(revalidateCtx, tenantID, p.service) + config, err := p.client.GetTenantConfig(revalidateCtx, tenantID, p.service, client.WithSkipCache()) if err != nil { // If tenant service was suspended/purged, evict the cached connection immediately. // The next request for this tenant will call createConnection, which fetches fresh diff --git a/commons/tenant-manager/postgres/manager_test.go b/commons/tenant-manager/postgres/manager_test.go index 46e0fa0..07badc5 100644 --- a/commons/tenant-manager/postgres/manager_test.go +++ b/commons/tenant-manager/postgres/manager_test.go @@ -1736,3 +1736,88 @@ func TestManager_RevalidateSettings_EvictsSuspendedTenant(t *testing.T) { }) } } + +func TestManager_RevalidateSettings_BypassesClientCache(t *testing.T) { + t.Parallel() + + // This test verifies that revalidatePoolSettings uses WithSkipCache() + // to bypass the client's in-memory cache. Without it, a cached "active" + // response would hide a subsequent 403 (suspended/purged) from tenant-manager. + // + // Setup: The httptest server returns 200 (active) on the first request + // and 403 (suspended) on all subsequent requests. We first call + // GetTenantConfig directly to populate the client cache, then trigger + // revalidatePoolSettings. If WithSkipCache is working, the revalidation + // hits the server (gets 403) and evicts the connection. If the cache + // were used, it would return the stale 200 and the connection would + // remain. + var requestCount atomic.Int32 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + count := requestCount.Add(1) + w.Header().Set("Content-Type", "application/json") + + if count == 1 { + // First request: return active config (populates client cache) + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{ + "id": "tenant-cache-test", + "tenantSlug": "cached-tenant", + "service": "ledger", + "status": "active", + "databases": { + "onboarding": { + "postgresql": {"host": "localhost", "port": 5432, "database": "testdb", "username": "user", "password": "pass"} + } + } + }`)) + + return + } + + // Subsequent requests: return 403 (tenant suspended) + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"code":"TS-SUSPENDED","error":"service suspended","status":"suspended"}`)) + })) + defer server.Close() + + capLogger := testutil.NewCapturingLogger() + tmClient, err := client.NewClient(server.URL, capLogger, client.WithAllowInsecureHTTP(), client.WithServiceAPIKey("test-key")) + require.NoError(t, err) + + // Populate the client cache by calling GetTenantConfig directly + cfg, err := tmClient.GetTenantConfig(context.Background(), "tenant-cache-test", "ledger") + require.NoError(t, err) + assert.Equal(t, "tenant-cache-test", cfg.ID) + assert.Equal(t, int32(1), requestCount.Load(), "should have made exactly 1 HTTP request") + + // Create a manager with a cached connection for this tenant + manager := NewManager(tmClient, "ledger", + WithLogger(capLogger), + WithModule("onboarding"), + WithSettingsCheckInterval(1*time.Millisecond), + ) + + mockDB := &pingableDB{} + var dbIface dbresolver.DB = mockDB + + manager.connections["tenant-cache-test"] = &PostgresConnection{ConnectionDB: &dbIface} + manager.lastAccessed["tenant-cache-test"] = time.Now() + manager.lastSettingsCheck["tenant-cache-test"] = time.Now() + + // Trigger revalidatePoolSettings -- should bypass cache and hit the server + manager.revalidatePoolSettings("tenant-cache-test") + + // Verify a second HTTP request was made (cache was bypassed) + assert.Equal(t, int32(2), requestCount.Load(), + "revalidatePoolSettings should bypass client cache and make a fresh HTTP request") + + // Verify the connection was evicted (server returned 403) + statsAfter := manager.Stats() + assert.Equal(t, 0, statsAfter.TotalConnections, + "connection should be evicted after revalidation detected suspended tenant via cache bypass") + + // Verify the DB was closed + assert.True(t, mockDB.closed, + "cached connection's DB should have been closed on eviction") +}