Peter Sanchez: 1 Adding domain middleware integration test to help test for db transaction leaks. 3 files changed, 219 insertions(+), 31 deletions(-)
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.code.netlandish.com/~netlandish/links-dev/patches/181/mbox | git am -3Learn more about email & git
gobwebs bump --- domain/integration_test.go | 200 +++++++++++++++++++++++++++++++++++++ go.mod | 14 +-- go.sum | 36 +++---- 3 files changed, 219 insertions(+), 31 deletions(-) create mode 100644 domain/integration_test.go diff --git a/domain/integration_test.go b/domain/integration_test.go new file mode 100644 index 0000000..6f671d7 --- /dev/null +++ b/domain/integration_test.go @@ -0,0 +1,200 @@ +package domain + +import ( + "context" + "database/sql" + "links/models" + "net/http" + "net/http/httptest" + "os" + "testing" + "time" + + "github.com/labstack/echo/v4" + "netlandish.com/x/gobwebs/config" + "netlandish.com/x/gobwebs/database" + "netlandish.com/x/gobwebs/server" + "netlandish.com/x/gobwebs/timezone" + + _ "github.com/lib/pq" +) + +// testDB creates a test database connection for domain tests +func testDB(t *testing.T) *sql.DB { + t.Helper() + + // Try to get test database URL from environment + dbURL := os.Getenv("TEST_DATABASE_URL") + if dbURL == "" { + // Default to local test database + dbURL = "postgres://postgres:postgres@localhost/links_test?sslmode=disable" + } + + db, err := sql.Open("postgres", dbURL) + if err != nil { + t.Skip("Test database not available") + } + + // Verify connection + if err := db.Ping(); err != nil { + t.Skip("Test database not available") + } + + // Clean up on test completion + t.Cleanup(func() { + db.Close() + }) + + return db +} + +// TestDomainValidationContextCancellation tests the exact scenario that was causing transaction leaks: +// Domain validation middleware calling ValidDomain() with context cancellation during database lookup +func TestDomainValidationContextCancellation(t *testing.T) { + db := testDB(t) + + // Set up basic test data - create a test domain + // First try to delete any existing test domain + db.Exec("DELETE FROM domains WHERE lookup_name = 'test.example.com'") + + _, err := db.Exec(` + INSERT INTO domains (lookup_name, name, service, status, level, is_active) + VALUES ($1, $2, $3, $4, $5, $6) + `, "test.example.com", "test.example.com", models.DomainServiceLinks, models.DomainStatusApproved, models.DomainLevelSystem, true) + if err != nil { + t.Fatalf("Failed to create test domain: %v", err) + } + + // Clean up after test + defer func() { + db.Exec("DELETE FROM domains WHERE lookup_name = 'test.example.com'") + }() + + // Track connection stats before test + statsBefore := db.Stats() + + // Create Echo server with middleware stack similar to production + e := echo.New() + cfg := &config.Config{Debug: true} + server.New(e, db, cfg).Initialize() + + // Add middleware in same order as production + e.Use( + // Custom context middleware (matches server.DefaultMiddlewareWithConfig) + func(next echo.HandlerFunc) echo.HandlerFunc { + return func(c echo.Context) error { + ctx := &server.Context{Context: c, Server: server.New(e, db, cfg)} + return next(ctx) + } + }, + database.DBIMiddleware(db), // DBI must be first + func(next echo.HandlerFunc) echo.HandlerFunc { // Add timezone context + return func(c echo.Context) error { + ctx := timezone.Context(c.Request().Context(), "UTC") + c.SetRequest(c.Request().WithContext(ctx)) + return next(c) + } + }, + DomainContext(models.DomainServiceLinks), // Domain validation + ) + + // Simple handler that should never be reached due to domain validation failure + e.GET("/test", func(c echo.Context) error { + return c.String(http.StatusOK, "success") + }) + + // Test 1: Context cancellation during domain validation + req := httptest.NewRequest(http.MethodGet, "/test", nil) + req.Host = "test.example.com" // Valid domain that exists in DB + + // Create cancellable context + ctx, cancel := context.WithCancel(req.Context()) + req = req.WithContext(ctx) + rec := httptest.NewRecorder() + + // Start request processing in goroutine + done := make(chan bool) + go func() { + defer func() { done <- true }() + e.ServeHTTP(rec, req) + }() + + // Cancel context immediately to simulate client disconnect + // This should trigger the "context canceled" error in ValidDomain() + cancel() + + // Wait for request to complete + <-done + + // The request should fail (exact status depends on error handling) + if rec.Code == http.StatusOK { + t.Error("Expected request to fail due to context cancellation") + } + + // Wait for cleanup + time.Sleep(300 * time.Millisecond) + + // Verify no connection leak occurred - this is the critical test + statsAfter := db.Stats() + if statsAfter.OpenConnections > statsBefore.OpenConnections { + t.Errorf("Connection leak detected after context cancellation: before=%d, after=%d", + statsBefore.OpenConnections, statsAfter.OpenConnections) + } + + t.Logf("Context cancellation test - Connection stats before: %d, after: %d", + statsBefore.OpenConnections, statsAfter.OpenConnections) + + // Test 2: Multiple rapid cancellations (stress test) + for i := 0; i < 5; i++ { + req := httptest.NewRequest(http.MethodGet, "/test", nil) + req.Host = "test.example.com" + + ctx, cancel := context.WithCancel(req.Context()) + req = req.WithContext(ctx) + rec := httptest.NewRecorder() + + go func() { + e.ServeHTTP(rec, req) + }() + + // Cancel quickly + cancel() + time.Sleep(50 * time.Millisecond) + } + + // Final connection check after stress test + time.Sleep(500 * time.Millisecond) + statsFinal := db.Stats() + + if statsFinal.OpenConnections > statsBefore.OpenConnections+2 { // Allow some tolerance + t.Errorf("Connection leak detected after stress test: before=%d, final=%d", + statsBefore.OpenConnections, statsFinal.OpenConnections) + } + + t.Logf("Stress test complete - Connection stats before: %d, final: %d", + statsBefore.OpenConnections, statsFinal.OpenConnections) +} + +// TestValidDomainWithContextCancellation tests the ValidDomain function directly +func TestValidDomainWithContextCancellation(t *testing.T) { + db := testDB(t) + + // Create context with both DBI and timezone (required by models.GetDomains) + ctx := database.DBIContext(context.Background(), database.NewDBI(db)) + ctx = timezone.Context(ctx, "UTC") // Add required timezone context + ctx, cancel := context.WithCancel(ctx) + + // Cancel context immediately + cancel() + + // Call ValidDomain with cancelled context + _, err := ValidDomain(ctx, "test.example.com", models.DomainServiceLinks, false) + + // Should get context cancelled error + if err == nil { + t.Error("Expected error due to context cancellation") + } + + // The key is that this doesn't leak connections + // The WithTx fix should handle the rollback automatically +} \ No newline at end of file diff --git a/go.mod b/go.mod index f1f6fb8..f00b0af 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/alexedwards/scs/v2 v2.8.0 github.com/jarcoal/httpmock v1.3.0 github.com/labstack/echo/v4 v4.13.3 + github.com/lib/pq v1.10.9 github.com/mattermost/mattermost-plugin-apps v1.1.0 github.com/mattermost/mattermost-server/v6 v6.6.0 github.com/oschwald/geoip2-golang v1.9.0 @@ -30,12 +31,12 @@ require ( golang.org/x/text v0.27.0 golang.org/x/time v0.8.0 hg.code.netlandish.com/~netlandish/sendygo v0.0.0-20230124192435-bbf347776232 - netlandish.com/x/gobwebs v0.1.7 - netlandish.com/x/gobwebs-auditlog v0.2.4 - netlandish.com/x/gobwebs-formguard v0.2.5 - netlandish.com/x/gobwebs-graphql v0.2.5 - netlandish.com/x/gobwebs-oauth2 v0.2.4 - netlandish.com/x/gobwebs-ses-feedback v0.2.5 + netlandish.com/x/gobwebs v0.1.8 + netlandish.com/x/gobwebs-auditlog v0.2.5 + netlandish.com/x/gobwebs-formguard v0.2.6 + netlandish.com/x/gobwebs-graphql v0.2.6 + netlandish.com/x/gobwebs-oauth2 v0.2.5 + netlandish.com/x/gobwebs-ses-feedback v0.2.6 petersanchez.com/x/carrier v0.2.3 petersanchez.com/x/carrier/ses v0.0.0-20250114214955-7f5d9b835a85 petersanchez.com/x/carrier/smtp v0.0.0-20250114214955-7f5d9b835a85 @@ -109,7 +110,6 @@ require ( github.com/lann/builder v0.0.0-20180802200727-47ae307949d0 // indirect github.com/lann/ps v0.0.0-20150810152359-62de8c46ede0 // indirect github.com/leodido/go-urn v1.4.0 // indirect - github.com/lib/pq v1.10.9 // indirect github.com/mattermost/go-i18n v1.11.1-0.20211013152124-5c415071e404 // indirect github.com/mattermost/ldap v0.0.0-20201202150706-ee0e6284187d // indirect github.com/mattermost/logr/v2 v2.0.15 // indirect diff --git a/go.sum b/go.sum index d16509f..d101e72 100644 --- a/go.sum +++ b/go.sum @@ -2578,30 +2578,18 @@ modernc.org/z v1.0.1-0.20210308123920-1f282aa71362/go.mod h1:8/SRk5C/HgiQWCgXdfp modernc.org/z v1.0.1/go.mod h1:8/SRk5C/HgiQWCgXdfpb+1RvhORdkz5sw72d3jjtyqA= modernc.org/z v1.2.20/go.mod h1:zU9FiF4PbHdOTUxw+IF8j7ArBMRPsHgq10uVPt6xTzo= modernc.org/zappy v1.0.0/go.mod h1:hHe+oGahLVII/aTTyWK/b53VDHMAGCBYYeZ9sn83HC4= -netlandish.com/x/gobwebs v0.1.6 h1:/fXVU2hoS6FyIk/Refl1FqxCGJxjcCvCybnrxr2JYlI= -netlandish.com/x/gobwebs v0.1.6/go.mod h1:RAf0VNiujzXen/pu0k6yyhvPWMgCY9tKD7ftXrkT53E= -netlandish.com/x/gobwebs v0.1.7 h1:YPqieX9F8lQpjBRq9mrhG4ZKA6SoWMZ0Inz2xu5ZSOM= -netlandish.com/x/gobwebs v0.1.7/go.mod h1:RAf0VNiujzXen/pu0k6yyhvPWMgCY9tKD7ftXrkT53E= -netlandish.com/x/gobwebs-auditlog v0.2.3 h1:U9Kcbacy+6gUbm2wwXUKg4q7RpbRwHgaLXebQWiYlXk= -netlandish.com/x/gobwebs-auditlog v0.2.3/go.mod h1:E5K6BDkD6BTgwGDz2GWOE/h/PfKuyswLDnhU1gDWNPQ= -netlandish.com/x/gobwebs-auditlog v0.2.4 h1:IloSnIc8yGKeEwX5ylKx0I3VoLoa9Bezf98XUnLFIN0= -netlandish.com/x/gobwebs-auditlog v0.2.4/go.mod h1:l1EJbQ79+vWc6l6yFnzRjE4NSRjGTAiwLPxUPQqzy/w= -netlandish.com/x/gobwebs-formguard v0.2.4 h1:YkLudnPrwr9k/giURyohnjlfbuC+Ws8hnrrU+MY8vUc= -netlandish.com/x/gobwebs-formguard v0.2.4/go.mod h1:0UZ/lWNAqHCwDYotp2WlX0XgfsF8PfV8XBr29kSJmP8= -netlandish.com/x/gobwebs-formguard v0.2.5 h1:+Fa5T2qeP2ldv+sIXCVqsLFzp8NXQyI8ZvxVD5TsOA4= -netlandish.com/x/gobwebs-formguard v0.2.5/go.mod h1:Qv+qymuUOCgKzhNmk31DOxT4PLu5bEwXFe2tMEBaljA= -netlandish.com/x/gobwebs-graphql v0.2.4 h1:BWuWiTkTVJNdBITV36r42w7Uh+6HGgZvxOj3+XICsF4= -netlandish.com/x/gobwebs-graphql v0.2.4/go.mod h1:vegE1Sb69L19qk/Urx7HWFhJ3FCjZZ5e0n5PlXDE1Qc= -netlandish.com/x/gobwebs-graphql v0.2.5 h1:h1tr7PP+PbKABD+gP58ldi6GMPRLjAzp6sjXpcozEKg= -netlandish.com/x/gobwebs-graphql v0.2.5/go.mod h1:nNGgCWC098JgaASCsvzZLl1rePzXgGcUtBEB+eJThhg= -netlandish.com/x/gobwebs-oauth2 v0.2.3 h1:gtxVwa/TFquvPm7sL9CYPN6qF9dayovK09bhv8s/bkE= -netlandish.com/x/gobwebs-oauth2 v0.2.3/go.mod h1:OXmkl1iL3MMy4CzdtAc9oOQbX/2kbR8/2t2GCahHBho= -netlandish.com/x/gobwebs-oauth2 v0.2.4 h1:B5V5HNCeRU3TNXK0/59Vnc5Dn7nOA54snxXDnBjn+0g= -netlandish.com/x/gobwebs-oauth2 v0.2.4/go.mod h1:LqCD8OZJkFr8rcDaNYeX8L6tsv/joAQ1k1iriCUlf+E= -netlandish.com/x/gobwebs-ses-feedback v0.2.4 h1:0jSGszEAKxKOJrhiO/vQuuY6lQv7fcsP4s6Iw5EcOJk= -netlandish.com/x/gobwebs-ses-feedback v0.2.4/go.mod h1:s7BCQ4lYPLdvzDuDvZmQn+7IHKsHNIkecs4jcjoZ0ZM= -netlandish.com/x/gobwebs-ses-feedback v0.2.5 h1:o/9aP9CkijohA61834S2AFPk8mN07SkytLvoSAybOeY= -netlandish.com/x/gobwebs-ses-feedback v0.2.5/go.mod h1:Nj7NSW936hRXq2QL1wa95C+g03GLhBIV5MN2K7zorGk= +netlandish.com/x/gobwebs v0.1.8 h1:hCTpIeNxAccBWq00X3PmwSd9juu/xo2oQjqocfI+nRU= +netlandish.com/x/gobwebs v0.1.8/go.mod h1:RAf0VNiujzXen/pu0k6yyhvPWMgCY9tKD7ftXrkT53E= +netlandish.com/x/gobwebs-auditlog v0.2.5 h1:ABrrl/ghIRVZyTEEqJsLNH6Fi4gtbkvpjUQoFcGU6zs= +netlandish.com/x/gobwebs-auditlog v0.2.5/go.mod h1:s1H4a8JdeXfaaU/q5tHzoXxNka7PyWGOdwochjAD47M= +netlandish.com/x/gobwebs-formguard v0.2.6 h1:MOucfAQP1EK/PKHks5FnELwBFDrxdITaGRNPFGdiaoc= +netlandish.com/x/gobwebs-formguard v0.2.6/go.mod h1:7CiNHjRh7/vh6YKzWcZmBao3dc9uSw9RNg+PhHNtPFY= +netlandish.com/x/gobwebs-graphql v0.2.6 h1:MXlKxFEoTayvw9Ro5nyHSSqgNVdXSqIJk3lADLuv/wQ= +netlandish.com/x/gobwebs-graphql v0.2.6/go.mod h1:rOH7fKL4DTeNy6XzVGyrLiamZmADsU1J1eIpKbdxtBs= +netlandish.com/x/gobwebs-oauth2 v0.2.5 h1:SPGp/HzC6UYfNES+8YcsSO23ElcMbCcT2bVXydOT8yA= +netlandish.com/x/gobwebs-oauth2 v0.2.5/go.mod h1:NO4ewCxCwv5EwACf1zRKdg5tQpk6UHLch1RQLFAGWc8= +netlandish.com/x/gobwebs-ses-feedback v0.2.6 h1:dTx8IXCqu9s7JovUWhQsZ2A+AoJLg1E3/Hy5sKZDSb8= +netlandish.com/x/gobwebs-ses-feedback v0.2.6/go.mod h1:cls3Jkuwj3yaHC85XCf9bpzAgfHZjFj/tEwcjMZyezE= petersanchez.com/x/carrier v0.2.3 h1:6ScWG2HVFqeqafQp2D9kChNdXYkou4rduzppc3SDYMg= petersanchez.com/x/carrier v0.2.3/go.mod h1:GLiDI9OThDmruufk/VHlR6Ihvq/hIJQyA5beU6AFNYk= petersanchez.com/x/carrier/ses v0.0.0-20250114214955-7f5d9b835a85 h1:yy81/rrGdI+YWuTvv7JPvVnY104/g10vnloBnKxVAHk= -- 2.49.1
Applied. To git@git.code.netlandish.com:~netlandish/links 89e69a6..f6d9cda master -> master