diff --git a/backend/app/controllers/auth.controller.go b/backend/app/controllers/auth.controller.go index 12a3c310..18f259ff 100644 --- a/backend/app/controllers/auth.controller.go +++ b/backend/app/controllers/auth.controller.go @@ -200,13 +200,7 @@ func (a authController) Register(c *gin.Context) { var projectWithUrl *models.ProjectWithBackendUrl if project != nil { - cache.ProjectCache.AddProject(&models.Project{ - Id: project.Id, - Name: project.Name, - Token: project.Token, - Framework: project.Framework, - OrganizationId: project.OrganizationId, - }) + middleware.OnCommit(c, func() { cache.ProjectCache.AddProject(project) }) projectWithUrl = project.ToProjectWithBackendUrl() } diff --git a/backend/app/controllers/oauth.controller.go b/backend/app/controllers/oauth.controller.go index 60b6dd89..ca0a54a0 100644 --- a/backend/app/controllers/oauth.controller.go +++ b/backend/app/controllers/oauth.controller.go @@ -293,13 +293,7 @@ func (a oauthController) FinishSetup(c *gin.Context) { return } - cache.ProjectCache.AddProject(&models.Project{ - Id: project.Id, - Name: project.Name, - Token: project.Token, - Framework: project.Framework, - OrganizationId: project.OrganizationId, - }) + middleware.OnCommit(c, func() { cache.ProjectCache.AddProject(project) }) projectWithUrl = project.ToProjectWithBackendUrl() } diff --git a/backend/app/controllers/project_cache_registration_test.go b/backend/app/controllers/project_cache_registration_test.go new file mode 100644 index 00000000..49934629 --- /dev/null +++ b/backend/app/controllers/project_cache_registration_test.go @@ -0,0 +1,205 @@ +//go:build !telemetry_ch && !transactional_pg && !telemetry_duckdb + +package controllers + +import ( + "net/http" + "net/http/httptest" + "slices" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/google/uuid" + "github.com/tracewayapp/traceway/backend/app/cache" + "github.com/tracewayapp/traceway/backend/app/db" + "github.com/tracewayapp/traceway/backend/app/middleware" + "github.com/tracewayapp/traceway/backend/app/models" + "github.com/tracewayapp/traceway/backend/app/repositories/transactional" +) + +// ProjectCache is a process-global that outlives the per-test in-memory +// database, so whatever a test puts in it has to come back out even when the +// test fails before it knows the project's id. +func restoreProjectCacheAfterTest(t *testing.T) { + t.Helper() + before := map[uuid.UUID]bool{} + for _, p := range cache.ProjectCache.GetAll() { + before[p.Id] = true + } + t.Cleanup(func() { + for _, p := range cache.ProjectCache.GetAll() { + if !before[p.Id] { + cache.ProjectCache.RemoveProject(p.Id) + } + } + }) +} + +func findProjectByName(t *testing.T, name string) *models.Project { + t.Helper() + tx, err := db.DB.Begin() + if err != nil { + t.Fatalf("begin: %v", err) + } + defer tx.Rollback() + + projects, err := transactional.ProjectRepository.FindAll(tx) + if err != nil { + t.Fatalf("find all projects: %v", err) + } + for _, p := range projects { + if p.Name == name { + return p + } + } + return nil +} + +// Every field the cache serves has to agree with the row, not just the ones a +// past bug happened to drop. +func assertCachedProjectMatchesRow(t *testing.T, cached, stored *models.Project) { + t.Helper() + + if cached.Id != stored.Id { + t.Errorf("cached Id = %v, stored = %v", cached.Id, stored.Id) + } + if cached.Name != stored.Name { + t.Errorf("cached Name = %q, stored = %q", cached.Name, stored.Name) + } + if cached.Token != stored.Token { + t.Errorf("cached Token = %q, stored = %q", cached.Token, stored.Token) + } + if cached.Framework != stored.Framework { + t.Errorf("cached Framework = %q, stored = %q", cached.Framework, stored.Framework) + } + if !equalIntPtr(cached.OrganizationId, stored.OrganizationId) { + t.Errorf("cached OrganizationId = %v, stored = %v", cached.OrganizationId, stored.OrganizationId) + } + // Second granularity: the cached copy carries the value the repository + // built, the row carries whatever survived the round trip through SQLite. + if cached.CreatedAt.Unix() != stored.CreatedAt.Unix() { + t.Errorf("cached CreatedAt = %v, stored = %v", cached.CreatedAt, stored.CreatedAt) + } + if cached.CreatedAt.IsZero() { + t.Error("cached CreatedAt is zero, which reorders GetAll()") + } + if cached.DropHealthyHealthchecks != stored.DropHealthyHealthchecks { + t.Errorf("cached DropHealthyHealthchecks = %v, stored = %v; healthy healthchecks would be ingested", + cached.DropHealthyHealthchecks, stored.DropHealthyHealthchecks) + } + if !equalStringPtr(cached.SourceMapToken, stored.SourceMapToken) { + t.Errorf("cached SourceMapToken = %v, stored = %v", cached.SourceMapToken, stored.SourceMapToken) + } + // nil and empty are the same allowlist; copyProject turns one into the other. + if !slices.Equal(cached.HealthcheckPaths, stored.HealthcheckPaths) { + t.Errorf("cached HealthcheckPaths = %v, stored = %v", cached.HealthcheckPaths, stored.HealthcheckPaths) + } + if !slices.Equal(cached.ProfileLabelAllowlist, stored.ProfileLabelAllowlist) { + t.Errorf("cached ProfileLabelAllowlist = %v, stored = %v", cached.ProfileLabelAllowlist, stored.ProfileLabelAllowlist) + } + if !slices.Equal(cached.AiFlaggedTerms, stored.AiFlaggedTerms) { + t.Errorf("cached AiFlaggedTerms = %v, stored = %v", cached.AiFlaggedTerms, stored.AiFlaggedTerms) + } + if !slices.Equal(cached.AiFlaggedLanguages, stored.AiFlaggedLanguages) { + t.Errorf("cached AiFlaggedLanguages = %v, stored = %v; ingest would scan the wrong term packs", + cached.AiFlaggedLanguages, stored.AiFlaggedLanguages) + } +} + +func equalIntPtr(a, b *int) bool { + if a == nil || b == nil { + return a == b + } + return *a == *b +} + +func equalStringPtr(a, b *string) bool { + if a == nil || b == nil { + return a == b + } + return *a == *b +} + +// The register handler used to cache a five-field copy of the project it had +// just created, so the cached ingest settings disagreed with the row. On the +// SQLite builds nothing ever refreshes the cache, so the wrong copy served +// every /api/report request until the process restarted. +func TestRegisterCachesTheProjectItCreated(t *testing.T) { + setupSetupControllerDB(t) + initRegistrationJWT(t) + restoreProjectCacheAfterTest(t) + + gin.SetMode(gin.TestMode) + router := gin.New() + router.POST("/api/register", middleware.Transactional, AuthController.Register) + + body := `{"email":"cached@example.com","name":"A","password":"password1","organizationName":"CachedOrg","timezone":"UTC","projectName":"Mobile App","framework":"ios"}` + recorder := httptest.NewRecorder() + request := httptest.NewRequest(http.MethodPost, "/api/register", strings.NewReader(body)) + request.Header.Set("Content-Type", "application/json") + router.ServeHTTP(recorder, request) + + if recorder.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", recorder.Code, recorder.Body.String()) + } + + stored := findProjectByName(t, "Mobile App") + if stored == nil { + t.Fatal("project was not created") + } + + cached := cache.ProjectCache.GetByToken(stored.Token) + if cached == nil { + t.Fatal("project token is not in the cache after a committed registration") + } + assertCachedProjectMatchesRow(t, cached, stored) + + // The by-source-map-token index is separate from the by-token one, and it + // is what a symbol upload authenticates against. + if stored.SourceMapToken == nil { + t.Fatal("an ios project should be created with a source map token") + } + if cache.ProjectCache.GetBySourceMapToken(*stored.SourceMapToken) == nil { + t.Error("source map token does not resolve from the cache; symbol upload would 401") + } +} + +// The handler itself must not touch the cache; the write is queued with +// middleware.OnCommit so a registration whose transaction rolls back leaves +// nothing behind. Driving the handler without that middleware runs no commit +// hooks, which is the same observation point as a rollback. +func TestRegisterCachesOnlyViaCommitHook(t *testing.T) { + setupSetupControllerDB(t) + initRegistrationJWT(t) + restoreProjectCacheAfterTest(t) + + tx, err := db.DB.Begin() + if err != nil { + t.Fatalf("begin: %v", err) + } + defer tx.Rollback() + + body := `{"email":"uncommitted@example.com","name":"A","password":"password1","organizationName":"UncommittedOrg","timezone":"UTC","projectName":"Pending App","framework":"react"}` + c, recorder := newControllerTestContext(t, tx, 0, http.MethodPost, "/api/register", body) + AuthController.Register(c) + if recorder.Code != http.StatusCreated { + t.Fatalf("status = %d, body = %s", recorder.Code, recorder.Body.String()) + } + // Commit rather than roll back: the harness caps the main DB at one + // connection, so findProjectByName cannot open its transaction until this + // one closes, and it needs the row to be visible. Commit hooks still never + // run, because middleware.Transactional is what runs them. + if err := tx.Commit(); err != nil { + t.Fatalf("commit: %v", err) + } + + stored := findProjectByName(t, "Pending App") + if stored == nil { + t.Fatal("project was not created") + } + + if cache.ProjectCache.GetByToken(stored.Token) != nil { + t.Error("the handler cached the project inline; a rolled back registration would leave a phantom entry") + } +} diff --git a/backend/app/controllers/setup_test.go b/backend/app/controllers/setup_test.go index 87d3d728..5fb1cc31 100644 --- a/backend/app/controllers/setup_test.go +++ b/backend/app/controllers/setup_test.go @@ -64,6 +64,20 @@ func setupSetupControllerDB(t *testing.T) { } } +func initRegistrationJWT(t *testing.T) { + t.Helper() + prevSecret := config.Config.JWTSecret + config.Config.JWTSecret = strings.Repeat("s", 32) + if err := services.InitJWT(); err != nil { + t.Fatalf("init jwt: %v", err) + } + services.InitTurnstile() + t.Cleanup(func() { + config.Config.JWTSecret = prevSecret + _ = services.InitJWT() + }) +} + func createSetupTestAccount(t *testing.T, tx *sql.Tx, email, role string) (userId int, orgId int) { t.Helper() user, err := transactional.UserRepository.Create(tx, email, "Test User", "hashed-password") @@ -246,16 +260,7 @@ func TestBatchCreateProjectsLimitHookAbortsBatch(t *testing.T) { func TestRegisterWithoutProject(t *testing.T) { setupSetupControllerDB(t) - prevSecret := config.Config.JWTSecret - config.Config.JWTSecret = strings.Repeat("s", 32) - if err := services.InitJWT(); err != nil { - t.Fatalf("init jwt: %v", err) - } - services.InitTurnstile() - t.Cleanup(func() { - config.Config.JWTSecret = prevSecret - _ = services.InitJWT() - }) + initRegistrationJWT(t) register := func(t *testing.T, body string) (*httptest.ResponseRecorder, map[string]any) { t.Helper() diff --git a/backend/app/middleware/commit_hooks_test.go b/backend/app/middleware/commit_hooks_test.go new file mode 100644 index 00000000..045d589e --- /dev/null +++ b/backend/app/middleware/commit_hooks_test.go @@ -0,0 +1,119 @@ +//go:build !transactional_pg && !telemetry_ch && !telemetry_duckdb + +package middleware + +import ( + "database/sql" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/tracewayapp/traceway/backend/app/db" + _ "modernc.org/sqlite" +) + +func newCommitHookTestDB(t *testing.T) *sql.DB { + t.Helper() + conn, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatal(err) + } + conn.SetMaxOpenConns(1) + t.Cleanup(func() { conn.Close() }) + + if _, err := conn.Exec("CREATE TABLE things (id INTEGER PRIMARY KEY)"); err != nil { + t.Fatalf("create table: %v", err) + } + + prev := db.DB + db.DB = conn + t.Cleanup(func() { db.DB = prev }) + return conn +} + +// A queued side effect must fire only when the write it describes is durable, +// and must be dropped otherwise. Callers rely on this to publish state that +// outlives the request — cache.ProjectCache entries, for one, which would +// survive a rollback and then authenticate a project that does not exist. +func TestOnCommitRunsQueuedHooksOnlyAfterCommit(t *testing.T) { + gin.SetMode(gin.TestMode) + + for _, tc := range []struct { + name string + status int + wantHook bool + wantCommit bool + }{ + {name: "committed", status: http.StatusCreated, wantHook: true, wantCommit: true}, + {name: "rolled back", status: http.StatusInternalServerError, wantHook: false, wantCommit: false}, + } { + t.Run(tc.name, func(t *testing.T) { + conn := newCommitHookTestDB(t) + + ran := 0 + r := gin.New() + r.POST("/thing", Transactional, func(c *gin.Context) { + tx := db.GetTx(c) + if _, err := tx.Exec("INSERT INTO things (id) VALUES (1)"); err != nil { + t.Fatalf("insert: %v", err) + } + OnCommit(c, func() { ran++ }) + c.JSON(tc.status, gin.H{}) + }) + + recorder := httptest.NewRecorder() + r.ServeHTTP(recorder, httptest.NewRequest(http.MethodPost, "/thing", nil)) + + if recorder.Code != tc.status { + t.Fatalf("status = %d, want %d", recorder.Code, tc.status) + } + + wantRuns := 0 + if tc.wantHook { + wantRuns = 1 + } + if ran != wantRuns { + t.Errorf("hook ran %d times, want %d", ran, wantRuns) + } + + var rows int + if err := conn.QueryRow("SELECT COUNT(*) FROM things").Scan(&rows); err != nil { + t.Fatalf("count: %v", err) + } + wantRows := 0 + if tc.wantCommit { + wantRows = 1 + } + if rows != wantRows { + t.Errorf("rows = %d, want %d; the hook and the write must agree", rows, wantRows) + } + }) + } +} + +// Hooks run in the order they were queued, so a later hook may depend on what +// an earlier one published. +func TestOnCommitRunsHooksInOrder(t *testing.T) { + gin.SetMode(gin.TestMode) + newCommitHookTestDB(t) + + var order []int + r := gin.New() + r.POST("/thing", Transactional, func(c *gin.Context) { + for i := range 3 { + OnCommit(c, func() { order = append(order, i) }) + } + c.JSON(http.StatusOK, gin.H{}) + }) + + recorder := httptest.NewRecorder() + r.ServeHTTP(recorder, httptest.NewRequest(http.MethodPost, "/thing", nil)) + + if recorder.Code != http.StatusOK { + t.Fatalf("status = %d", recorder.Code) + } + if len(order) != 3 || order[0] != 0 || order[1] != 1 || order[2] != 2 { + t.Errorf("hook order = %v, want [0 1 2]", order) + } +} diff --git a/backend/app/middleware/transactional.middleware.go b/backend/app/middleware/transactional.middleware.go index dd40a849..7f78ff66 100644 --- a/backend/app/middleware/transactional.middleware.go +++ b/backend/app/middleware/transactional.middleware.go @@ -50,9 +50,13 @@ const commitHooksContextKey = "txCommitHooks" // OnCommit queues fn to run after the Transactional middleware successfully // commits the request transaction; queued fns are dropped on rollback. Use it -// for side effects that must only fire once the transaction's writes are -// visible to other connections (e.g. waking the outbox drain worker, which -// would otherwise poll before the enqueued row exists and go back to sleep). +// for side effects that must not observe the write before it lands: waking the +// outbox drain worker, which would otherwise poll before the enqueued row +// exists and go back to sleep, or filling a process-local cache that concurrent +// requests read (cache.ProjectCache, whose entry would outlive a rollback). +// Only routes carrying this middleware run the queued fns; a handler that +// commits its own transaction via db.ExecuteTransaction should call its side +// effect directly instead. func OnCommit(c *gin.Context, fn func()) { hooks, _ := c.Get(commitHooksContextKey) fns, _ := hooks.([]func())