diff --git a/docker-compose.yml b/docker-compose.yml index ea92d724..1889b78d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -70,6 +70,10 @@ services: - EXPENSE_SERVICE_REST=http://expense-service:8082 - FINANCE_SERVICE_REST=http://finance-service:8083 - DATARIGHTS_SERVICE_REST=http://datarights-service:8084 + # Upper bound on the auth ValidateToken gRPC call (parsed as a Go + # duration, default 3s). Caps tail latency if the auth service hangs; + # must comfortably exceed a healthy validation (low milliseconds). + - GATEWAY_VALIDATE_TIMEOUT=${GATEWAY_VALIDATE_TIMEOUT:-3s} - JWT_SECRET=${JWT_SECRET} - LOG_LEVEL=${LOG_LEVEL:-info} - ENVIRONMENT=${ENVIRONMENT:-development} diff --git a/docs/architecture.md b/docs/architecture.md index 6372fdee..b75217b7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -247,7 +247,7 @@ sequenceDiagram ## Data Flow: Data Export -When a user requests a data export, the datarights service collects data from all upstream services asynchronously and delivers the result via email: +When a user requests a data export, the datarights service collects data from all upstream services concurrently in a background goroutine (an `errgroup` fan-out, with ZIP assembly as the fan-in barrier) and delivers the result via email: ```mermaid sequenceDiagram @@ -268,12 +268,17 @@ sequenceDiagram Note over DR: Async goroutine starts DR->>P: UPDATE status=running - DR->>A: gRPC: GetUser (profile + email) - A-->>DR: user profile - DR->>E: gRPC: GetAllUserExpenses (paginated) - E-->>DR: expense data - DR->>F: gRPC: GetAllUserData (tags, periods, defaults) - F-->>DR: finance data + par Providers collect concurrently (errgroup fan-out) + DR->>A: gRPC: GetUser (profile + email) + A-->>DR: user profile + and + DR->>E: gRPC: StreamAllUserExpenses (server-streaming) + E-->>DR: stream expense rows (Recv loop, written incrementally) + and + DR->>F: gRPC: GetAllUserData (tags, periods, defaults) + F-->>DR: finance data + end + Note over DR: Fan-in barrier: all providers complete → assemble ZIP DR->>DR: Generate CSVs + ZIP DR->>R: Send email with ZIP attachment R-->>DR: delivery confirmed diff --git a/docs/data-export.md b/docs/data-export.md index 9a5825e0..7eba3761 100644 --- a/docs/data-export.md +++ b/docs/data-export.md @@ -114,7 +114,7 @@ Single row with current budget defaults (empty if not configured). ## Adding a Data Provider -The export engine uses a provider registry pattern. Each provider is responsible for one CSV file. +The export engine builds a fresh set of providers for each export job. Each provider is responsible for one CSV file. ### 1. Implement the `DataProvider` interface @@ -150,13 +150,20 @@ func (p *MyProvider) Collect(ctx context.Context, userID string) ([][]string, er } ``` -### 3. Register in `cmd/main.go` +### 3. Add it to the per-job provider factory in `cmd/main.go` + +Export providers are built fresh for each job by a `ProviderFactory` (a `func(finance financepb.FinanceServiceClient) []engine.DataProvider`) passed to `engine.NewEngine`. Add your provider to the slice it returns; the slice order is the ZIP order: ```go -registry.Register(providers.NewMyProvider(myClient)) +newExportProviders := func(finance financepb.FinanceServiceClient) []engine.DataProvider { + return []engine.DataProvider{ + // ...existing providers... + providers.NewMyProvider(myClient), + } +} ``` -The engine handles everything else: CSV writing, ZIP assembly, email delivery, error handling, and metrics emission per provider. +A finance-backed provider takes the `finance` parameter: a per-job `MemoizedFinanceClient` that collapses `GetAllUserData` to a single call per export and is never shared across jobs. The engine handles everything else: concurrent collection (`errgroup` fan-out), CSV writing, ZIP assembly, email delivery, error handling, and metrics emission per provider. ### Formatting helpers diff --git a/services/datarights/Dockerfile b/services/datarights/Dockerfile index 7bc7db97..6e808e30 100644 --- a/services/datarights/Dockerfile +++ b/services/datarights/Dockerfile @@ -15,6 +15,7 @@ COPY expense/go.mod expense/go.sum* ./expense/ COPY finance/go.mod finance/go.sum* ./finance/ COPY healthcheck/go.mod ./healthcheck/ COPY metrics/go.mod metrics/go.sum* ./metrics/ +COPY perf/go.mod ./perf/ RUN --mount=type=cache,target=/go/pkg/mod \ cd datarights && GOWORK=off go mod download @@ -27,6 +28,7 @@ COPY expense/ ./expense/ COPY finance/ ./finance/ COPY healthcheck/ ./healthcheck/ COPY metrics/ ./metrics/ +COPY perf/ ./perf/ RUN --mount=type=cache,target=/go/pkg/mod \ --mount=type=cache,target=/root/.cache/go-build \ diff --git a/services/datarights/cmd/main.go b/services/datarights/cmd/main.go index a7744f12..3ec23f41 100644 --- a/services/datarights/cmd/main.go +++ b/services/datarights/cmd/main.go @@ -134,13 +134,21 @@ func run() error { // Build dependency graph repo := repository.NewPostgresJobRepository(pool) - // Set up export engine with provider registry - registry := engine.NewProviderRegistry() - registry.Register(providers.NewProfileProvider(authClient)) - registry.Register(providers.NewExpensesProvider(expenseClient, financeClient)) - registry.Register(providers.NewTagsProvider(financeClient)) - registry.Register(providers.NewBudgetPeriodsProvider(financeClient)) - registry.Register(providers.NewDefaultSettingsProvider(financeClient)) + // Set up export engine with a per-job provider factory. The factory closes + // over the auth and expense clients; the finance-backed providers receive a + // fresh per-job MemoizedFinanceClient (built inside the engine) so a single + // GetAllUserData call is shared across providers without leaking data across + // jobs. Registration/ZIP order: profile, expenses, tags, budget_periods, + // default_settings. + newExportProviders := func(finance financepb.FinanceServiceClient) []engine.DataProvider { + return []engine.DataProvider{ + providers.NewProfileProvider(authClient), + providers.NewExpensesProvider(expenseClient, finance), + providers.NewTagsProvider(finance), + providers.NewBudgetPeriodsProvider(finance), + providers.NewDefaultSettingsProvider(finance), + } + } // Set up email sender emailSender, err := buildEmailSender(cfg, logger) @@ -148,7 +156,7 @@ func run() error { return fmt.Errorf("setting up email sender: %w", err) } - exportEngine := engine.NewEngine(registry, repo, emailSender, cfg.MaxConcurrent, cfg.ExportTimeout, logger) + exportEngine := engine.NewEngine(newExportProviders, financeClient, repo, emailSender, cfg.MaxConcurrent, cfg.ExportTimeout, logger) // Startup recovery: re-submit non-terminal export jobs emailResolver := service.NewAuthUserEmailResolver(authClient) diff --git a/services/datarights/go.mod b/services/datarights/go.mod index 30f300db..165b67ec 100644 --- a/services/datarights/go.mod +++ b/services/datarights/go.mod @@ -10,10 +10,12 @@ require ( github.com/ItsThompson/gofin/services/finance v0.0.0 github.com/ItsThompson/gofin/services/healthcheck v0.0.0 github.com/ItsThompson/gofin/services/metrics v0.0.0 + github.com/ItsThompson/gofin/services/perf v0.0.0 github.com/gin-gonic/gin v1.12.0 github.com/jackc/pgx/v5 v5.9.2 github.com/prometheus/client_golang v1.22.0 github.com/stretchr/testify v1.11.1 + golang.org/x/sync v0.20.0 google.golang.org/grpc v1.80.0 ) @@ -31,6 +33,8 @@ replace github.com/ItsThompson/gofin/services/healthcheck => ../healthcheck replace github.com/ItsThompson/gofin/services/metrics => ../metrics +replace github.com/ItsThompson/gofin/services/perf => ../perf + require ( github.com/beorn7/perks v1.0.1 // indirect github.com/bytedance/gopkg v0.1.3 // indirect @@ -76,7 +80,6 @@ require ( golang.org/x/arch v0.22.0 // indirect golang.org/x/crypto v0.50.0 // indirect golang.org/x/net v0.52.0 // indirect - golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.43.0 // indirect golang.org/x/text v0.36.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260120221211-b8f7ae30c516 // indirect diff --git a/services/datarights/internal/deletion/providers/mock_clients_test.go b/services/datarights/internal/deletion/providers/mock_clients_test.go index 95e1ae8a..aedd480d 100644 --- a/services/datarights/internal/deletion/providers/mock_clients_test.go +++ b/services/datarights/internal/deletion/providers/mock_clients_test.go @@ -116,9 +116,6 @@ func (m *mockExpenseClient) GetExpense(_ context.Context, _ *expensepb.GetExpens func (m *mockExpenseClient) CountExpensesByTag(_ context.Context, _ *expensepb.CountExpensesByTagRequest, _ ...grpc.CallOption) (*expensepb.CountExpensesByTagResponse, error) { return nil, nil } -func (m *mockExpenseClient) GetAllUserExpenses(_ context.Context, _ *expensepb.GetAllUserExpensesRequest, _ ...grpc.CallOption) (*expensepb.ExpenseListResponse, error) { - return nil, nil -} func (m *mockExpenseClient) CorrectExpense(_ context.Context, _ *expensepb.CorrectExpenseRequest, _ ...grpc.CallOption) (*expensepb.ExpenseResponse, error) { return nil, nil } @@ -128,6 +125,9 @@ func (m *mockExpenseClient) GetCorrectionHistory(_ context.Context, _ *expensepb func (m *mockExpenseClient) GetProRataGroup(_ context.Context, _ *expensepb.GetProRataGroupRequest, _ ...grpc.CallOption) (*expensepb.ExpenseListResponse, error) { return nil, nil } +func (m *mockExpenseClient) StreamAllUserExpenses(_ context.Context, _ *expensepb.StreamAllUserExpensesRequest, _ ...grpc.CallOption) (grpc.ServerStreamingClient[expensepb.ExpenseData], error) { + return nil, nil +} // --------------------------------------------------------------------------- // Mock auth client diff --git a/services/datarights/internal/engine/engine.go b/services/datarights/internal/engine/engine.go index d7434938..f4caa12b 100644 --- a/services/datarights/internal/engine/engine.go +++ b/services/datarights/internal/engine/engine.go @@ -2,29 +2,43 @@ package engine import ( "context" + "errors" "fmt" "log/slog" "strings" "time" + "golang.org/x/sync/errgroup" + "github.com/ItsThompson/gofin/services/datarights/internal/email" exportmetrics "github.com/ItsThompson/gofin/services/datarights/internal/metrics" "github.com/ItsThompson/gofin/services/datarights/internal/repository" + "github.com/ItsThompson/gofin/services/finance/proto/financepb" ) +// ProviderFactory builds a fresh set of data providers for a single export job, +// injecting the finance client the finance-backed providers should share. The +// factory closes over the non-finance clients (auth, expense) at startup; the +// finance client is supplied per job so each job gets its own memoized instance. +type ProviderFactory func(finance financepb.FinanceServiceClient) []DataProvider + // Engine manages a bounded pool of export workers. type Engine struct { - registry *ProviderRegistry - repo repository.JobRepository - sender email.Sender - sem chan struct{} - logger *slog.Logger - timeout time.Duration + newProviders ProviderFactory + financeClient financepb.FinanceServiceClient + repo repository.JobRepository + sender email.Sender + sem chan struct{} + logger *slog.Logger + timeout time.Duration } -// NewEngine creates an export engine with bounded concurrency. +// NewEngine creates an export engine with bounded concurrency. newProviders +// builds a fresh provider set per job; financeClient is the raw finance client +// wrapped in a per-job MemoizedFinanceClient before being handed to the factory. func NewEngine( - registry *ProviderRegistry, + newProviders ProviderFactory, + financeClient financepb.FinanceServiceClient, repo repository.JobRepository, sender email.Sender, maxConcurrent int, @@ -32,12 +46,13 @@ func NewEngine( logger *slog.Logger, ) *Engine { return &Engine{ - registry: registry, - repo: repo, - sender: sender, - sem: make(chan struct{}, maxConcurrent), - logger: logger, - timeout: timeout, + newProviders: newProviders, + financeClient: financeClient, + repo: repo, + sender: sender, + sem: make(chan struct{}, maxConcurrent), + logger: logger, + timeout: timeout, } } @@ -99,53 +114,80 @@ func (e *Engine) execute(ctx context.Context, jobID, userID, userEmail string) { return } - // Collect data from all providers - var csvFiles []CSVFile - for _, provider := range e.registry.All() { - // Check context before each provider - if err := ctx.Err(); err != nil { - e.failJob(ctx, jobID, userID, "Export timed out", "collection", jobStart) - return - } - - e.logger.Debug("provider collection started", - slog.String("job_id", jobID), - slog.String("user_id", userID), - slog.String("provider", provider.Name()), - slog.String("method", "engine.execute"), - ) - - providerStart := time.Now() - rows, err := provider.Collect(ctx, userID) - providerDuration := time.Since(providerStart).Seconds() - - exportmetrics.ExportDataCollectionDurationSeconds.WithLabelValues(provider.Name()).Observe(providerDuration) - - if err != nil { - if ctx.Err() != nil { - e.failJob(ctx, jobID, userID, "Export timed out", "collection", jobStart) - return + // Build a fresh provider set for this job. The finance-backed providers + // share one per-job MemoizedFinanceClient, so GetAllUserData is fetched at + // most once; a fresh instance per job prevents cross-user data leakage. + fc := NewMemoizedFinanceClient(e.financeClient) + providerSet := e.newProviders(fc) + + // Collect from every provider concurrently. The providers are independent + // and read-only, so each goroutine writes only its own pre-assigned index in + // csvFiles; this makes collection latency max(providers) instead of sum while + // keeping ZIP order deterministic (the factory's slice order). + csvFiles := make([]CSVFile, len(providerSet)) + g, gctx := errgroup.WithContext(ctx) + for i, provider := range providerSet { + i, provider := i, provider + g.Go(func() error { + e.logger.Debug("provider collection started", + slog.String("job_id", jobID), + slog.String("user_id", userID), + slog.String("provider", provider.Name()), + slog.String("method", "engine.execute"), + ) + + providerStart := time.Now() + rows, err := provider.Collect(gctx, userID) + providerDuration := time.Since(providerStart).Seconds() + + // HistogramVec is goroutine-safe, so the per-provider observation + // stays inside the goroutine. + exportmetrics.ExportDataCollectionDurationSeconds.WithLabelValues(provider.Name()).Observe(providerDuration) + + if err != nil { + // Attach the provider name before errgroup captures the error: + // Wait surfaces only the first error, and the post-Wait switch + // recovers the name via errors.As on *collectError. + return &collectError{provider: provider.Name(), err: err} } - e.failJob(ctx, jobID, userID, fmt.Sprintf("Failed to collect %s data", provider.Name()), "collection", jobStart) - return - } - e.logger.Info("provider collection complete", - slog.String("job_id", jobID), - slog.String("user_id", userID), - slog.String("provider", provider.Name()), - slog.Int("row_count", len(rows)), - slog.Float64("duration_seconds", providerDuration), - slog.String("method", "engine.execute"), - ) - - csvFiles = append(csvFiles, CSVFile{ - Name: provider.Name() + ".csv", - Headers: provider.Headers(), - Rows: rows, + e.logger.Info("provider collection complete", + slog.String("job_id", jobID), + slog.String("user_id", userID), + slog.String("provider", provider.Name()), + slog.Int("row_count", len(rows)), + slog.Float64("duration_seconds", providerDuration), + slog.String("method", "engine.execute"), + ) + + // Write only this goroutine's own slot; never append (data race). + csvFiles[i] = CSVFile{ + Name: provider.Name() + ".csv", + Headers: provider.Headers(), + Rows: rows, + } + return nil }) } + // Fan-in barrier: the first error wins and cancels the siblings via gctx. + // Recheck the job context afterward so a deadline still maps to "Export timed + // out" while a genuine provider failure maps to "Failed to collect X data". + var ce *collectError + switch err := g.Wait(); { + case err == nil: + // all providers succeeded; fall through to ZIP assembly + case ctx.Err() != nil || errors.Is(err, context.DeadlineExceeded): + e.failJob(ctx, jobID, userID, "Export timed out", "collection", jobStart) + return + case errors.As(err, &ce): + e.failJob(ctx, jobID, userID, fmt.Sprintf("Failed to collect %s data", ce.provider), "collection", jobStart) + return + default: + e.failJob(ctx, jobID, userID, "Failed to collect export data", "collection", jobStart) + return + } + // Build ZIP zipBytes, err := BuildZIP(csvFiles) if err != nil { @@ -229,6 +271,19 @@ func (e *Engine) failJob(_ context.Context, jobID, userID, errMsg, stage string, } } +// collectError carries the failing provider's name alongside the underlying +// cause, so the post-Wait switch recovers the name via errors.As instead of +// round-tripping structured data through the error string. It unwraps to the +// cause so errors.Is(err, context.DeadlineExceeded) still classifies timeouts. +type collectError struct { + provider string + err error +} + +func (e *collectError) Error() string { return "collect " + e.provider + ": " + e.err.Error() } + +func (e *collectError) Unwrap() error { return e.err } + // sanitizeError extracts a human-readable reason from an error without exposing internals. func sanitizeError(err error) string { msg := err.Error() diff --git a/services/datarights/internal/engine/engine_bench_test.go b/services/datarights/internal/engine/engine_bench_test.go new file mode 100644 index 00000000..eaef8808 --- /dev/null +++ b/services/datarights/internal/engine/engine_bench_test.go @@ -0,0 +1,41 @@ +package engine + +import ( + "context" + "fmt" + "testing" + "time" +) + +// benchProviderLatency is the base per-provider simulated upstream latency for +// the collection scaling benchmark. The five providers get multiples of it +// (1x..5x), so the serial sum (15x) and the fan-out max (5x) are far enough +// apart that the reported ns/op makes the shape unmistakable. Wall-clock is a +// same-machine reference only, never a CI threshold. +const benchProviderLatency = 10 * time.Millisecond + +// BenchmarkEngineCollectionFanout measures a full export run over five providers +// with differing simulated latency. It drives engine.execute end-to-end; +// collection dominates because the stub repo, ZIP assembly, and stub sender are +// effectively instant, so the reported ns/op tracks total collection latency: +// ≈ max(providers) under the errgroup fan-out versus ≈ sum(providers) under the +// old serial loop. +func BenchmarkEngineCollectionFanout(b *testing.B) { + provs := make([]DataProvider, 5) + for i := range provs { + provs[i] = &stubProvider{ + name: fmt.Sprintf("p%d", i), + headers: []string{"col"}, + rows: [][]string{{"val"}}, + delay: time.Duration(i+1) * benchProviderLatency, + } + } + + eng := NewEngine(staticProviders(provs...), nil, &mockRepo{}, newMockSender(), 5, time.Minute, newTestLogger()) + + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + eng.execute(context.Background(), "job-bench", "user-1", "alex@example.com") + } +} diff --git a/services/datarights/internal/engine/engine_fanout_test.go b/services/datarights/internal/engine/engine_fanout_test.go new file mode 100644 index 00000000..b1db92c8 --- /dev/null +++ b/services/datarights/internal/engine/engine_fanout_test.go @@ -0,0 +1,203 @@ +package engine + +import ( + "archive/zip" + "bytes" + "context" + "fmt" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ItsThompson/gofin/services/datarights/internal/email" + "github.com/ItsThompson/gofin/services/datarights/internal/repository" +) + +// capturingSender records the ZIP bytes handed to the last successful send so +// fan-out tests can inspect the assembled archive (e.g. file order). +type capturingSender struct { + mu sync.Mutex + zip []byte +} + +var _ email.Sender = (*capturingSender)(nil) + +func (s *capturingSender) SendExportEmail(_ context.Context, _ string, zipBytes []byte) error { + s.mu.Lock() + defer s.mu.Unlock() + s.zip = append([]byte(nil), zipBytes...) + return nil +} + +func (s *capturingSender) capturedNames(t *testing.T) []string { + t.Helper() + s.mu.Lock() + defer s.mu.Unlock() + require.NotEmpty(t, s.zip, "no ZIP was captured") + + zr, err := zip.NewReader(bytes.NewReader(s.zip), int64(len(s.zip))) + require.NoError(t, err) + names := make([]string, len(zr.File)) + for i, f := range zr.File { + names[i] = f.Name + } + return names +} + +// TestEngine_FanOut_ZIPOrderDeterministic proves the index-addressed writes keep +// ZIP order equal to registration order even when providers finish in the +// reverse order. Provider 0 is the slowest and provider 4 the fastest, so +// completion order is the reverse of registration order; the archive must still +// be p0..p4. +func TestEngine_FanOut_ZIPOrderDeterministic(t *testing.T) { + const n = 5 + provs := make([]DataProvider, n) + for i := range provs { + provs[i] = &stubProvider{ + name: fmt.Sprintf("p%d", i), + headers: []string{"col"}, + rows: [][]string{{fmt.Sprintf("val-%d", i)}}, + delay: time.Duration(n-i) * 8 * time.Millisecond, // p0 slowest, p4 fastest + } + } + + repo := &mockRepo{} + sender := &capturingSender{} + eng := NewEngine(staticProviders(provs...), nil, repo, sender, 5, time.Minute, newTestLogger()) + eng.Submit("job-order", "user-1", "alex@example.com") + + require.Eventually(t, func() bool { + return len(repo.getCompletedJobs()) == 1 + }, 2*time.Second, 10*time.Millisecond, "job did not complete; failures=%v", repo.getFailedJobs()) + + assert.Equal(t, + []string{"p0.csv", "p1.csv", "p2.csv", "p3.csv", "p4.csv"}, + sender.capturedNames(t), + "ZIP order must match registration order regardless of completion order") +} + +// TestEngine_FanOut_RunsProvidersConcurrently is the deterministic, machine- +// independent form of "max, not sum": it asserts every provider is in flight at +// the same time. A serial loop would peak at one concurrent provider. +func TestEngine_FanOut_RunsProvidersConcurrently(t *testing.T) { + const n = 5 + var running, maxSeen atomic.Int32 + provs := make([]DataProvider, n) + for i := range provs { + provs[i] = &concurrencyTrackingProvider{ + running: &running, + maxSeen: &maxSeen, + delay: 40 * time.Millisecond, + } + } + + repo := &mockRepo{} + eng := NewEngine(staticProviders(provs...), nil, repo, newMockSender(), 5, time.Minute, newTestLogger()) + eng.Submit("job-concurrent", "user-1", "") + + require.Eventually(t, func() bool { + return len(repo.getCompletedJobs()) == 1 + }, 2*time.Second, 10*time.Millisecond, "job did not complete; failures=%v", repo.getFailedJobs()) + + assert.Equal(t, int32(n), maxSeen.Load(), + "all %d providers must run concurrently under the fan-out; saw peak %d", n, maxSeen.Load()) +} + +// TestEngine_FanOut_NamedProviderErrorSurvivesFirstError proves the provider +// name is baked into the error inside the goroutine, so the human-readable +// "Failed to collect X data" message survives errgroup's first-error capture, +// and the underlying cause is not leaked. The failing provider is not the first +// in registration order and returns quickly so its error is the one captured. +func TestEngine_FanOut_NamedProviderErrorSurvivesFirstError(t *testing.T) { + repo := &mockRepo{} + eng := NewEngine(staticProviders( + &stubProvider{name: "profile", headers: []string{"c"}, rows: [][]string{{"v"}}, delay: 50 * time.Millisecond}, + &stubProvider{name: "expenses", err: fmt.Errorf("gRPC unavailable: connection refused")}, + &stubProvider{name: "tags", headers: []string{"c"}, rows: [][]string{{"v"}}, delay: 50 * time.Millisecond}, + ), nil, repo, newMockSender(), 5, time.Minute, newTestLogger()) + eng.Submit("job-named-err", "user-1", "") + + require.Eventually(t, func() bool { + return len(repo.getFailedJobs()) == 1 + }, 2*time.Second, 10*time.Millisecond) + + failed := repo.getFailedJobs() + assert.Equal(t, "Failed to collect expenses data", failed[0].ErrMsg) + assert.NotContains(t, failed[0].ErrMsg, "connection refused", "must not leak the underlying cause") +} + +// TestEngine_FanOut_TimeoutMapsToExportTimedOut proves the post-Wait context +// recheck: when the job context expires, the wrapped context.DeadlineExceeded +// error a provider returns is classified as a timeout ("Export timed out"), +// not a plain collect failure. +func TestEngine_FanOut_TimeoutMapsToExportTimedOut(t *testing.T) { + repo := &mockRepo{} + eng := NewEngine(staticProviders( + &stubProvider{name: "profile", headers: []string{"c"}, rows: [][]string{{"v"}}}, + &stubProvider{name: "expenses", headers: []string{"c"}, rows: [][]string{{"v"}}, delay: 500 * time.Millisecond}, + ), nil, repo, newMockSender(), 5, 50*time.Millisecond, newTestLogger()) + eng.Submit("job-timeout", "user-1", "") + + require.Eventually(t, func() bool { + return len(repo.getFailedJobs()) == 1 + }, 2*time.Second, 10*time.Millisecond) + + failed := repo.getFailedJobs() + assert.Equal(t, "Export timed out", failed[0].ErrMsg) +} + +// ctxCapturingRepo records whether the context passed to FailJob is still live. +// It embeds the JobRepository interface (nil) and overrides only the methods +// execute exercises, mirroring the finance-spy convention in this package. +type ctxCapturingRepo struct { + repository.JobRepository + + mu sync.Mutex + failMsg string + failCtxLive bool + failed bool +} + +func (r *ctxCapturingRepo) UpdateStatus(_ context.Context, _, _ string) error { return nil } + +func (r *ctxCapturingRepo) CompleteJob(_ context.Context, _ string, _ int64) error { return nil } + +func (r *ctxCapturingRepo) FailJob(ctx context.Context, _ string, msg string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.failed = true + r.failMsg = msg + r.failCtxLive = ctx.Err() == nil + return nil +} + +func (r *ctxCapturingRepo) snapshot() (bool, bool, string) { + r.mu.Lock() + defer r.mu.Unlock() + return r.failed, r.failCtxLive, r.failMsg +} + +// TestEngine_FanOut_FailurePersistsViaBackgroundContext proves failJob writes +// the failure with a fresh context.Background(), so the failure DB write still +// succeeds after the job context has expired. +func TestEngine_FanOut_FailurePersistsViaBackgroundContext(t *testing.T) { + repo := &ctxCapturingRepo{} + eng := NewEngine(staticProviders( + &stubProvider{name: "expenses", headers: []string{"c"}, rows: [][]string{{"v"}}, delay: 500 * time.Millisecond}, + ), nil, repo, newMockSender(), 5, 50*time.Millisecond, newTestLogger()) + eng.Submit("job-persist", "user-1", "") + + require.Eventually(t, func() bool { + failed, _, _ := repo.snapshot() + return failed + }, 2*time.Second, 10*time.Millisecond) + + failed, ctxLive, msg := repo.snapshot() + require.True(t, failed) + assert.True(t, ctxLive, "failJob must use a fresh context that survives the expired job context") + assert.Equal(t, "Export timed out", msg) +} diff --git a/services/datarights/internal/engine/engine_test.go b/services/datarights/internal/engine/engine_test.go index 92eb812f..d87e7372 100644 --- a/services/datarights/internal/engine/engine_test.go +++ b/services/datarights/internal/engine/engine_test.go @@ -15,6 +15,7 @@ import ( "github.com/ItsThompson/gofin/services/datarights/internal/email" "github.com/ItsThompson/gofin/services/datarights/internal/model" + "github.com/ItsThompson/gofin/services/finance/proto/financepb" ) // mockRepo implements repository.JobRepository for engine tests. @@ -136,6 +137,15 @@ func (p *stubProvider) Collect(ctx context.Context, _ string) ([][]string, error return p.rows, nil } +// staticProviders returns a ProviderFactory that ignores the injected finance +// client and always yields the given providers, for engine tests using stub +// providers with no finance dependency. +func staticProviders(ps ...DataProvider) ProviderFactory { + return func(financepb.FinanceServiceClient) []DataProvider { + return ps + } +} + func newTestLogger() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } @@ -175,14 +185,11 @@ func newMockSender() *mockSender { func TestEngine_HappyPath_CompletesJob(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() - registry.Register(&stubProvider{ + eng := NewEngine(staticProviders(&stubProvider{ name: "profile", headers: []string{"username", "email"}, rows: [][]string{{"alex", "alex@example.com"}}, - }) - - eng := NewEngine(registry, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) + }), nil, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) eng.Submit("job-1", "user-1", "alex@example.com") // Wait for async completion @@ -204,13 +211,10 @@ func TestEngine_HappyPath_CompletesJob(t *testing.T) { func TestEngine_ProviderError_FailsJob(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() - registry.Register(&stubProvider{ + eng := NewEngine(staticProviders(&stubProvider{ name: "profile", err: fmt.Errorf("gRPC unavailable"), - }) - - eng := NewEngine(registry, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) + }), nil, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) eng.Submit("job-2", "user-1", "") require.Eventually(t, func() bool { @@ -226,16 +230,12 @@ func TestEngine_ProviderError_FailsJob(t *testing.T) { func TestEngine_Timeout_FailsJob(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() - registry.Register(&stubProvider{ + eng := NewEngine(staticProviders(&stubProvider{ name: "slow-provider", headers: []string{"col"}, rows: [][]string{{"val"}}, delay: 500 * time.Millisecond, - }) - - // Very short timeout to trigger - eng := NewEngine(registry, repo, newMockSender(), 5, 50*time.Millisecond, newTestLogger()) + }), nil, repo, newMockSender(), 5, 50*time.Millisecond, newTestLogger()) eng.Submit("job-3", "user-1", "") require.Eventually(t, func() bool { @@ -249,29 +249,16 @@ func TestEngine_Timeout_FailsJob(t *testing.T) { func TestEngine_ConcurrencyBounded(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() var running atomic.Int32 var maxSeen atomic.Int32 - registry.Register(&stubProvider{ - name: "profile", - headers: []string{"col"}, - rows: [][]string{{"val"}}, - delay: 100 * time.Millisecond, - }) - maxConcurrent := 3 - eng := NewEngine(registry, repo, newMockSender(), maxConcurrent, 5*time.Minute, newTestLogger()) - - // Override the execute tracking with a custom provider that monitors concurrency - trackingRegistry := NewProviderRegistry() - trackingRegistry.Register(&concurrencyTrackingProvider{ + eng := NewEngine(staticProviders(&concurrencyTrackingProvider{ running: &running, maxSeen: &maxSeen, delay: 100 * time.Millisecond, - }) - eng.registry = trackingRegistry + }), nil, repo, newMockSender(), maxConcurrent, 5*time.Minute, newTestLogger()) // Submit more jobs than max concurrent totalJobs := 10 @@ -324,22 +311,21 @@ func (p *concurrencyTrackingProvider) Collect(ctx context.Context, _ string) ([] func TestEngine_MultipleProviders_AllCollected(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() - registry.Register(&stubProvider{ - name: "profile", - headers: []string{"username"}, - rows: [][]string{{"alex"}}, - }) - registry.Register(&stubProvider{ - name: "expenses", - headers: []string{"id", "amount"}, - rows: [][]string{ - {"1", "45.99"}, - {"2", "30.00"}, + eng := NewEngine(staticProviders( + &stubProvider{ + name: "profile", + headers: []string{"username"}, + rows: [][]string{{"alex"}}, }, - }) - - eng := NewEngine(registry, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) + &stubProvider{ + name: "expenses", + headers: []string{"id", "amount"}, + rows: [][]string{ + {"1", "45.99"}, + {"2", "30.00"}, + }, + }, + ), nil, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) eng.Submit("job-multi", "user-1", "") require.Eventually(t, func() bool { @@ -353,18 +339,17 @@ func TestEngine_MultipleProviders_AllCollected(t *testing.T) { func TestEngine_SecondProviderError_FailsJob(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() - registry.Register(&stubProvider{ - name: "profile", - headers: []string{"username"}, - rows: [][]string{{"alex"}}, - }) - registry.Register(&stubProvider{ - name: "expenses", - err: fmt.Errorf("upstream timeout"), - }) - - eng := NewEngine(registry, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) + eng := NewEngine(staticProviders( + &stubProvider{ + name: "profile", + headers: []string{"username"}, + rows: [][]string{{"alex"}}, + }, + &stubProvider{ + name: "expenses", + err: fmt.Errorf("upstream timeout"), + }, + ), nil, repo, newMockSender(), 5, 5*time.Minute, newTestLogger()) eng.Submit("job-fail", "user-1", "") require.Eventually(t, func() bool { @@ -377,15 +362,12 @@ func TestEngine_SecondProviderError_FailsJob(t *testing.T) { func TestEngine_EmailSenderCalled_OnSuccess(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() - registry.Register(&stubProvider{ + sender := newMockSender() + eng := NewEngine(staticProviders(&stubProvider{ name: "profile", headers: []string{"username", "email"}, rows: [][]string{{"alex", "alex@example.com"}}, - }) - - sender := newMockSender() - eng := NewEngine(registry, repo, sender, 5, 5*time.Minute, newTestLogger()) + }), nil, repo, sender, 5, 5*time.Minute, newTestLogger()) eng.Submit("job-email", "user-1", "alex@example.com") require.Eventually(t, func() bool { @@ -401,15 +383,12 @@ func TestEngine_EmailSenderCalled_OnSuccess(t *testing.T) { func TestEngine_EmailFailure_FailsJob(t *testing.T) { repo := &mockRepo{} - registry := NewProviderRegistry() - registry.Register(&stubProvider{ + sender := &mockSender{err: fmt.Errorf("Resend API error (status 429): rate limited")} + eng := NewEngine(staticProviders(&stubProvider{ name: "profile", headers: []string{"username"}, rows: [][]string{{"alex"}}, - }) - - sender := &mockSender{err: fmt.Errorf("Resend API error (status 429): rate limited")} - eng := NewEngine(registry, repo, sender, 5, 5*time.Minute, newTestLogger()) + }), nil, repo, sender, 5, 5*time.Minute, newTestLogger()) eng.Submit("job-email-fail", "user-1", "alex@example.com") require.Eventually(t, func() bool { diff --git a/services/datarights/internal/engine/export_dedup_test.go b/services/datarights/internal/engine/export_dedup_test.go new file mode 100644 index 00000000..4fa9c72a --- /dev/null +++ b/services/datarights/internal/engine/export_dedup_test.go @@ -0,0 +1,32 @@ +package engine_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestExport_DedupesFinanceCalls is the export finance-call deduplication regression: a +// full export must hit finance for GetAllUserData at most once and never call +// ListTags. It runs the real provider set through the engine with a finance spy +// as the raw client; the engine wraps it in a per-job MemoizedFinanceClient. +// Reverting the dedup (per-provider raw clients, or expenses.buildTagMap calling +// ListTags) makes this fail. +func TestExport_DedupesFinanceCalls(t *testing.T) { + finance := newFinanceSpy(cannedAllUserData(), cannedTagList()) + repo := newRecordingRepo() + + eng := newExportEngine(finance, repo) + eng.Submit("job-dedup", "user-1", "alex@example.com") + + require.Eventually(t, func() bool { + return repo.completedCount() == 1 + }, 2*time.Second, 10*time.Millisecond, "export job did not complete; failures=%v", repo.failures()) + + assert.LessOrEqual(t, finance.Count("GetAllUserData"), 1, + "export must fetch GetAllUserData at most once (dedup regressed)") + assert.Equal(t, 0, finance.Count("ListTags"), + "export must not call ListTags; the tag map derives from shared GetAllUserData") +} diff --git a/services/datarights/internal/engine/export_helpers_test.go b/services/datarights/internal/engine/export_helpers_test.go new file mode 100644 index 00000000..49945217 --- /dev/null +++ b/services/datarights/internal/engine/export_helpers_test.go @@ -0,0 +1,263 @@ +package engine_test + +import ( + "context" + "io" + "log/slog" + "sync" + "time" + + "google.golang.org/grpc" + + "github.com/ItsThompson/gofin/services/auth/proto/authpb" + "github.com/ItsThompson/gofin/services/datarights/internal/email" + "github.com/ItsThompson/gofin/services/datarights/internal/engine" + "github.com/ItsThompson/gofin/services/datarights/internal/engine/providers" + "github.com/ItsThompson/gofin/services/datarights/internal/model" + "github.com/ItsThompson/gofin/services/datarights/internal/repository" + "github.com/ItsThompson/gofin/services/expense/proto/expensepb" + "github.com/ItsThompson/gofin/services/finance/proto/financepb" + "github.com/ItsThompson/gofin/services/perf" +) + +// financeSpy is a finance client that records how many times each RPC is +// invoked, so efficiency tests can assert the export deduplicates finance +// calls. It embeds the full FinanceServiceClient interface (left nil): any RPC +// other than GetAllUserData / ListTags panics, surfacing unintended finance +// traffic. It embeds *perf.CallCounter for the concurrency-safe Count/Record. +type financeSpy struct { + financepb.FinanceServiceClient + *perf.CallCounter + data *financepb.AllUserDataResponse + dataErr error + tags *financepb.TagListResponse +} + +func newFinanceSpy(data *financepb.AllUserDataResponse, tags *financepb.TagListResponse) *financeSpy { + return &financeSpy{ + CallCounter: perf.NewCallCounter(), + data: data, + tags: tags, + } +} + +func (s *financeSpy) GetAllUserData(_ context.Context, _ *financepb.GetAllUserDataRequest, _ ...grpc.CallOption) (*financepb.AllUserDataResponse, error) { + s.Record("GetAllUserData") + if s.dataErr != nil { + return nil, s.dataErr + } + return s.data, nil +} + +func (s *financeSpy) ListTags(_ context.Context, _ *financepb.ListTagsRequest, _ ...grpc.CallOption) (*financepb.TagListResponse, error) { + s.Record("ListTags") + return s.tags, nil +} + +// stubAuthClient serves a single canned profile for the profile provider. +type stubAuthClient struct { + authpb.AuthServiceClient + user *authpb.UserResponse +} + +func (s *stubAuthClient) GetUser(_ context.Context, _ *authpb.GetUserRequest, _ ...grpc.CallOption) (*authpb.UserResponse, error) { + return s.user, nil +} + +// stubExpenseClient serves canned expenses to the expenses provider. The +// provider consumes StreamAllUserExpenses, so the canned pages are flattened +// into a single ordered server stream (the byte-identical fixture relies on the +// stream preserving the pages' chronological order). +type stubExpenseClient struct { + expensepb.ExpenseServiceClient + pages []*expensepb.ExpenseListResponse +} + +func (s *stubExpenseClient) StreamAllUserExpenses(_ context.Context, _ *expensepb.StreamAllUserExpensesRequest, _ ...grpc.CallOption) (grpc.ServerStreamingClient[expensepb.ExpenseData], error) { + var rows []*expensepb.ExpenseData + for _, page := range s.pages { + rows = append(rows, page.GetData()...) + } + return &fakeExpenseStream{rows: rows}, nil +} + +// fakeExpenseStream is the client side of a StreamAllUserExpenses server stream. +// The embedded nil grpc.ClientStream supplies the methods the consumer never +// calls; only Recv is exercised. +type fakeExpenseStream struct { + grpc.ClientStream + rows []*expensepb.ExpenseData + idx int +} + +func (f *fakeExpenseStream) Recv() (*expensepb.ExpenseData, error) { + if f.idx >= len(f.rows) { + return nil, io.EOF + } + row := f.rows[f.idx] + f.idx++ + return row, nil +} + +// buildRealProviders returns the export provider set in registration/ZIP order: +// profile, expenses, tags, budget_periods, default_settings. The finance-backed +// providers share the injected finance client. +func buildRealProviders( + auth authpb.AuthServiceClient, + expense expensepb.ExpenseServiceClient, + finance financepb.FinanceServiceClient, +) []engine.DataProvider { + return []engine.DataProvider{ + providers.NewProfileProvider(auth), + providers.NewExpensesProvider(expense, finance), + providers.NewTagsProvider(finance), + providers.NewBudgetPeriodsProvider(finance), + providers.NewDefaultSettingsProvider(finance), + } +} + +// cannedUser is the fixed profile served by stubAuthClient. +func cannedUser() *authpb.UserResponse { + return &authpb.UserResponse{ + Username: "alex", + Email: "alex@example.com", + Currency: "USD", + Role: "member", + CreatedAt: "2025-01-01T00:00:00Z", + } +} + +// cannedTags is the shared tag set. The byte-identical export guarantee rests on +// ListTags and GetAllUserData().GetTags() returning this same set, so both +// canned responses are built from it. +func cannedTags() []*financepb.TagData { + return []*financepb.TagData{ + {Id: "tag-1", Name: "Food", IsDefault: true, CreatedAt: "2025-06-01T10:00:00Z"}, + {Id: "tag-2", Name: "Transport", IsDefault: false, CreatedAt: "2025-07-15T14:30:00Z"}, + } +} + +func cannedAllUserData() *financepb.AllUserDataResponse { + return &financepb.AllUserDataResponse{ + Tags: cannedTags(), + Periods: []*financepb.PeriodData{ + { + Id: "period-1", Year: 2026, Month: 5, BudgetAmount: 250000, + EssentialsPercent: 50, DesiresPercent: 30, SavingsPercent: 20, + CreatedAt: "2026-05-01T00:00:00Z", + }, + }, + Defaults: &financepb.DefaultsData{ + BudgetAmount: 300000, EssentialsPercent: 50, DesiresPercent: 30, + SavingsPercent: 20, Currency: "USD", + }, + } +} + +func cannedTagList() *financepb.TagListResponse { + return &financepb.TagListResponse{Tags: cannedTags()} +} + +func cannedExpensePages() []*expensepb.ExpenseListResponse { + return []*expensepb.ExpenseListResponse{ + { + Data: []*expensepb.ExpenseData{ + { + Id: "exp-1", Name: "Groceries", Amount: 4599, Currency: "USD", + ExpenseType: "essentials", TagId: "tag-1", ExpenseDate: "2026-05-01", + PeriodYear: 2026, PeriodMonth: 5, Status: "active", + CreatedAt: "2026-05-01T12:00:00Z", + }, + { + Id: "exp-2", Name: "Bus pass", Amount: 3000, Currency: "USD", + ExpenseType: "essentials", TagId: "tag-2", ExpenseDate: "2026-05-02", + PeriodYear: 2026, PeriodMonth: 5, Status: "active", + CreatedAt: "2026-05-02T09:00:00Z", + }, + }, + HasMore: false, + }, + } +} + +// newExportEngine wires the real export providers through the per-job factory, +// with the finance spy as the raw client the engine wraps in a +// MemoizedFinanceClient per job. Used by the dedup regression test. +func newExportEngine(finance financepb.FinanceServiceClient, repo repository.JobRepository) *engine.Engine { + auth := &stubAuthClient{user: cannedUser()} + expense := &stubExpenseClient{pages: cannedExpensePages()} + return engine.NewEngine( + func(fc financepb.FinanceServiceClient) []engine.DataProvider { + return buildRealProviders(auth, expense, fc) + }, + finance, repo, noopSender{}, 5, 30*time.Second, discardLogger(), + ) +} + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// noopSender is an email.Sender that accepts every send. +type noopSender struct{} + +var _ email.Sender = noopSender{} + +func (noopSender) SendExportEmail(_ context.Context, _ string, _ []byte) error { return nil } + +// recordingRepo is a JobRepository that records terminal transitions so tests +// can wait for job completion. Only the methods engine.execute calls do work. +type recordingRepo struct { + mu sync.Mutex + completed int + failed []string +} + +func newRecordingRepo() *recordingRepo { return &recordingRepo{} } + +func (r *recordingRepo) UpdateStatus(_ context.Context, _, _ string) error { return nil } + +func (r *recordingRepo) CompleteJob(_ context.Context, _ string, _ int64) error { + r.mu.Lock() + defer r.mu.Unlock() + r.completed++ + return nil +} + +func (r *recordingRepo) FailJob(_ context.Context, _ string, errMsg string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.failed = append(r.failed, errMsg) + return nil +} + +func (r *recordingRepo) completedCount() int { + r.mu.Lock() + defer r.mu.Unlock() + return r.completed +} + +func (r *recordingRepo) failures() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.failed...) +} + +func (r *recordingRepo) CreateJob(_ context.Context, _ string) (*model.ExportJob, error) { + return nil, nil +} +func (r *recordingRepo) GetJob(_ context.Context, _ string) (*model.ExportJob, error) { + return nil, nil +} +func (r *recordingRepo) ListJobsByUser(_ context.Context, _ string, _, _ int) ([]*model.ExportJob, int64, error) { + return nil, 0, nil +} +func (r *recordingRepo) GetInProgressJob(_ context.Context, _ string) (*model.ExportJob, error) { + return nil, nil +} +func (r *recordingRepo) GetLatestNonFailedJob(_ context.Context, _ string) (*model.ExportJob, error) { + return nil, nil +} +func (r *recordingRepo) GetNonTerminalJobs(_ context.Context) ([]model.RecoverableJob, error) { + return nil, nil +} diff --git a/services/datarights/internal/engine/export_measurement_test.go b/services/datarights/internal/engine/export_measurement_test.go new file mode 100644 index 00000000..c1421184 --- /dev/null +++ b/services/datarights/internal/engine/export_measurement_test.go @@ -0,0 +1,102 @@ +package engine_test + +import ( + "archive/zip" + "bytes" + "context" + "flag" + "io" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ItsThompson/gofin/services/datarights/internal/engine" +) + +var updateGolden = flag.Bool("update", false, "regenerate golden CSV fixtures") + +// providerCSV runs a provider and returns the exact CSV bytes it contributes to +// the export ZIP, exercising the real BuildZIP + encoding/csv shipping path. +func providerCSV(t testing.TB, p engine.DataProvider) []byte { + t.Helper() + rows, err := p.Collect(context.Background(), "user-1") + require.NoError(t, err) + + zipBytes, err := engine.BuildZIP([]engine.CSVFile{ + {Name: p.Name() + ".csv", Headers: p.Headers(), Rows: rows}, + }) + require.NoError(t, err) + + zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes))) + require.NoError(t, err) + require.Len(t, zr.File, 1) + + f, err := zr.File[0].Open() + require.NoError(t, err) + defer f.Close() //nolint:errcheck + content, err := io.ReadAll(f) + require.NoError(t, err) + return content +} + +// TestExportProviders_CSVByteIdentical guards the byte-identical export guarantee: +// every provider's CSV output (headers + rows) for a fixed input +// must match the committed pre-dedup fixture. Regenerate with `-update`. +func TestExportProviders_CSVByteIdentical(t *testing.T) { + auth := &stubAuthClient{user: cannedUser()} + expense := &stubExpenseClient{pages: cannedExpensePages()} + finance := newFinanceSpy(cannedAllUserData(), cannedTagList()) + + for _, p := range buildRealProviders(auth, expense, finance) { + golden := filepath.Join("testdata", "export", p.Name()+".csv") + got := providerCSV(t, p) + + if *updateGolden { + require.NoError(t, os.MkdirAll(filepath.Dir(golden), 0o755)) + require.NoError(t, os.WriteFile(golden, got, 0o644)) //nolint:gosec + continue + } + + want, err := os.ReadFile(golden) + require.NoError(t, err, "missing golden %s; run: go test -run CSVByteIdentical -update", golden) + assert.Equal(t, string(want), string(got), "provider %s CSV drifted from pre-dedup fixture", p.Name()) + } +} + +// BenchmarkExportCollection measures serial collection across the full provider +// set and logs the finance call shape per export. Collection runs serially here +// to isolate the deduplication shape; BenchmarkEngineCollectionFanout measures +// fan-out latency. The finance client is wrapped +// in a per-job MemoizedFinanceClient exactly as engine.execute does, so this +// records the deduped shape: GetAllUserData=1, ListTags=0 (down from the +// committed pre-dedup baseline of GetAllUserData=3 + ListTags=1). +func BenchmarkExportCollection(b *testing.B) { + auth := &stubAuthClient{user: cannedUser()} + + observed := newFinanceSpy(cannedAllUserData(), cannedTagList()) + observedFC := engine.NewMemoizedFinanceClient(observed) + collectAll(b, buildRealProviders(auth, &stubExpenseClient{pages: cannedExpensePages()}, observedFC)) + b.Logf("finance calls per export: GetAllUserData=%d ListTags=%d total=%d", + observed.Count("GetAllUserData"), observed.Count("ListTags"), observed.Total()) + + b.ReportAllocs() + b.ResetTimer() + for b.Loop() { + finance := newFinanceSpy(cannedAllUserData(), cannedTagList()) + fc := engine.NewMemoizedFinanceClient(finance) + expense := &stubExpenseClient{pages: cannedExpensePages()} + collectAll(b, buildRealProviders(auth, expense, fc)) + } +} + +func collectAll(tb testing.TB, provs []engine.DataProvider) { + tb.Helper() + for _, p := range provs { + if _, err := p.Collect(context.Background(), "user-1"); err != nil { + tb.Fatalf("collect %s: %v", p.Name(), err) + } + } +} diff --git a/services/datarights/internal/engine/finance_cache.go b/services/datarights/internal/engine/finance_cache.go new file mode 100644 index 00000000..e22a3c66 --- /dev/null +++ b/services/datarights/internal/engine/finance_cache.go @@ -0,0 +1,46 @@ +package engine + +import ( + "context" + "sync" + + "google.golang.org/grpc" + + "github.com/ItsThompson/gofin/services/finance/proto/financepb" +) + +// MemoizedFinanceClient is a per-job wrapper over the shared finance gRPC +// client. It embeds financepb.FinanceServiceClient (anonymous field) so it stays +// a drop-in for the full client interface, and overrides GetAllUserData to fetch +// at most once per instance. +// +// A fresh instance MUST be created per export job; it must NEVER be a startup +// singleton, since sync.Once would cache the first user's data and serve it to +// every subsequent user (a cross-user data leak). +type MemoizedFinanceClient struct { + financepb.FinanceServiceClient // embedded: all other methods pass through + + once sync.Once + data *financepb.AllUserDataResponse + err error +} + +// NewMemoizedFinanceClient wraps inner so GetAllUserData is served at most once. +func NewMemoizedFinanceClient(inner financepb.FinanceServiceClient) *MemoizedFinanceClient { + return &MemoizedFinanceClient{FinanceServiceClient: inner} +} + +// GetAllUserData returns the memoized response, fetching from the wrapped client +// exactly once. Safe for concurrent callers: sync.Once serializes the fetch, so +// the later errgroup fan-out over providers still triggers a single RPC. All +// callers of an instance MUST pass the same request; the in and opts of calls +// after the first are ignored (safe today: each instance serves one +// single-user export job). +func (m *MemoizedFinanceClient) GetAllUserData( + ctx context.Context, in *financepb.GetAllUserDataRequest, opts ...grpc.CallOption, +) (*financepb.AllUserDataResponse, error) { + m.once.Do(func() { + m.data, m.err = m.FinanceServiceClient.GetAllUserData(ctx, in, opts...) + }) + return m.data, m.err +} diff --git a/services/datarights/internal/engine/finance_cache_test.go b/services/datarights/internal/engine/finance_cache_test.go new file mode 100644 index 00000000..7a530df0 --- /dev/null +++ b/services/datarights/internal/engine/finance_cache_test.go @@ -0,0 +1,74 @@ +package engine_test + +import ( + "context" + "errors" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ItsThompson/gofin/services/datarights/internal/engine" + "github.com/ItsThompson/gofin/services/finance/proto/financepb" +) + +func TestMemoizedFinanceClient_FetchesAtMostOnce(t *testing.T) { + inner := newFinanceSpy(cannedAllUserData(), cannedTagList()) + mc := engine.NewMemoizedFinanceClient(inner) + + for range 5 { + resp, err := mc.GetAllUserData(context.Background(), &financepb.GetAllUserDataRequest{UserId: "user-1"}) + require.NoError(t, err) + assert.Equal(t, cannedAllUserData().GetTags(), resp.GetTags()) + } + + assert.Equal(t, 1, inner.Count("GetAllUserData"), "inner client should be hit exactly once") +} + +func TestMemoizedFinanceClient_ConcurrentCallsFetchOnce(t *testing.T) { + inner := newFinanceSpy(cannedAllUserData(), cannedTagList()) + mc := engine.NewMemoizedFinanceClient(inner) + + const goroutines = 20 + var wg sync.WaitGroup + wg.Add(goroutines) + for range goroutines { + go func() { + defer wg.Done() + resp, err := mc.GetAllUserData(context.Background(), &financepb.GetAllUserDataRequest{UserId: "user-1"}) + assert.NoError(t, err) + assert.NotNil(t, resp) + }() + } + wg.Wait() + + assert.Equal(t, 1, inner.Count("GetAllUserData"), + "sync.Once must serialize concurrent fan-out callers into one fetch") +} + +func TestMemoizedFinanceClient_MemoizesError(t *testing.T) { + wantErr := errors.New("finance unavailable") + inner := newFinanceSpy(nil, cannedTagList()) + inner.dataErr = wantErr + mc := engine.NewMemoizedFinanceClient(inner) + + for range 3 { + _, err := mc.GetAllUserData(context.Background(), &financepb.GetAllUserDataRequest{UserId: "user-1"}) + require.ErrorIs(t, err, wantErr) + } + + assert.Equal(t, 1, inner.Count("GetAllUserData"), "a failed fetch is memoized too, not retried") +} + +func TestMemoizedFinanceClient_PassesThroughOtherMethods(t *testing.T) { + inner := newFinanceSpy(cannedAllUserData(), cannedTagList()) + mc := engine.NewMemoizedFinanceClient(inner) + + // ListTags is not overridden, so it must delegate to the embedded client. + resp, err := mc.ListTags(context.Background(), &financepb.ListTagsRequest{UserId: "user-1"}) + require.NoError(t, err) + assert.Equal(t, cannedTags(), resp.GetTags()) + assert.Equal(t, 1, inner.Count("ListTags")) + assert.Equal(t, 0, inner.Count("GetAllUserData")) +} diff --git a/services/datarights/internal/engine/providers/expenses.go b/services/datarights/internal/engine/providers/expenses.go index 5cd9e273..705c98d2 100644 --- a/services/datarights/internal/engine/providers/expenses.go +++ b/services/datarights/internal/engine/providers/expenses.go @@ -2,7 +2,9 @@ package providers import ( "context" + "errors" "fmt" + "io" "strconv" "github.com/ItsThompson/gofin/services/datarights/internal/engine" @@ -13,6 +15,10 @@ import ( // Compile-time check that ExpensesProvider implements DataProvider. var _ engine.DataProvider = (*ExpensesProvider)(nil) +// expensesPageSize is the server-side keyset page size requested from the +// StreamAllUserExpenses RPC. It caps how many rows the server materializes per +// page, so consuming the stream incrementally keeps peak memory at +// O(expensesPageSize) instead of O(total rows). const expensesPageSize = 100 // ExpensesProvider fetches all user expenses with pagination and resolves tag names. @@ -47,65 +53,98 @@ func (p *ExpensesProvider) Headers() []string { } } -// Collect fetches all expenses for the user, resolves tag names, and returns formatted rows. +// Collect streams every expense for the user in chronological order, resolves +// tag names, and returns the formatted CSV rows. +// +// It consumes the StreamAllUserExpenses server stream and formats each row as it +// arrives (see streamExpenses) rather than buffering the whole raw-proto history +// first. The returned [][]string is the DataProvider contract the export engine +// collects and hands to BuildZIP; a sink that writes each row onward keeps the +// consumer itself at O(pageSize) (see the bounded-memory benchmark). func (p *ExpensesProvider) Collect(ctx context.Context, userID string) ([][]string, error) { tagMap, err := p.buildTagMap(ctx, userID) if err != nil { return nil, fmt.Errorf("fetching tags for name resolution: %w", err) } - expenses, err := p.fetchAllExpenses(ctx, userID) - if err != nil { + var rows [][]string + if err := p.streamExpenses(ctx, userID, tagMap, func(row []string) error { + rows = append(rows, row) + return nil + }); err != nil { return nil, fmt.Errorf("fetching expenses: %w", err) } - rows := make([][]string, 0, len(expenses)) - for _, exp := range expenses { - rows = append(rows, p.formatRow(exp, tagMap)) - } - return rows, nil } -// buildTagMap fetches all tags for the user and returns a map of tag ID to tag name. +// buildTagMap fetches the user's tags and returns a map of tag ID to tag name. +// It derives the tag map from GetAllUserData().GetTags() (shared with the tags, +// budget_periods, and default_settings providers via the per-job memoized +// finance client) instead of a separate ListTags call, so the export hits +// finance once for this data. func (p *ExpensesProvider) buildTagMap(ctx context.Context, userID string) (map[string]string, error) { - resp, err := p.financeClient.ListTags(ctx, &financepb.ListTagsRequest{UserId: userID}) + resp, err := p.financeClient.GetAllUserData(ctx, &financepb.GetAllUserDataRequest{UserId: userID}) if err != nil { return nil, err } - tagMap := make(map[string]string, len(resp.GetTags())) - for _, tag := range resp.GetTags() { + tags := resp.GetTags() + tagMap := make(map[string]string, len(tags)) + for _, tag := range tags { tagMap[tag.GetId()] = tag.GetName() } return tagMap, nil } -// fetchAllExpenses paginates through all expenses for the user. -func (p *ExpensesProvider) fetchAllExpenses(ctx context.Context, userID string) ([]*expensepb.ExpenseData, error) { - var allExpenses []*expensepb.ExpenseData - page := int32(1) +// streamExpenses consumes the StreamAllUserExpenses server stream and invokes +// emit once per expense, formatted into a CSV row via formatRow, in the order +// the server sends them (chronological: created_at ASC, id ASC). It holds at +// most one row in flight, so a sink that writes each row onward (an incremental +// CSV/ZIP writer) keeps peak memory at O(pageSize) regardless of history size. +// +// The context is checked before each receive so a client disconnect or job +// timeout stops the walk promptly; io.EOF ends the stream cleanly and any other +// receive or emit error is propagated. +func (p *ExpensesProvider) streamExpenses( + ctx context.Context, + userID string, + tagMap map[string]string, + emit func(row []string) error, +) error { + // Derive a cancellable context and cancel on every return path so the gRPC + // client stream is torn down deterministically, including an early + // emit-error return. Without this, a caller whose own context outlives the + // call (e.g. context.Background()) would leak the stream. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + stream, err := p.expenseClient.StreamAllUserExpenses(ctx, &expensepb.StreamAllUserExpensesRequest{ + UserId: userID, + PageSize: expensesPageSize, + }) + if err != nil { + return err + } for { - resp, err := p.expenseClient.GetAllUserExpenses(ctx, &expensepb.GetAllUserExpensesRequest{ - UserId: userID, - Page: page, - PageSize: expensesPageSize, - }) - if err != nil { - return nil, err + if err := ctx.Err(); err != nil { + return err } - allExpenses = append(allExpenses, resp.GetData()...) + exp, err := stream.Recv() + if errors.Is(err, io.EOF) { + return nil + } + if err != nil { + return err + } - if !resp.GetHasMore() { - break + if err := emit(p.formatRow(exp, tagMap)); err != nil { + return err } - page++ } - - return allExpenses, nil } // formatRow converts a single expense into a CSV row with all transformations applied. diff --git a/services/datarights/internal/engine/providers/expenses_stream_test.go b/services/datarights/internal/engine/providers/expenses_stream_test.go new file mode 100644 index 00000000..c82d8d40 --- /dev/null +++ b/services/datarights/internal/engine/providers/expenses_stream_test.go @@ -0,0 +1,203 @@ +package providers + +import ( + "context" + "encoding/csv" + "fmt" + "io" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ItsThompson/gofin/services/expense/proto/expensepb" +) + +// streamRowFixtures builds n expense rows for the stream in chronological order. +// Every row carries a tag id so tag resolution and formatRow run on each one, +// exercising the same per-row work the real consumer does. +func streamRowFixtures(n int) []*expensepb.ExpenseData { + rows := make([]*expensepb.ExpenseData, n) + for i := range rows { + rows[i] = &expensepb.ExpenseData{ + Id: fmt.Sprintf("exp-%08d", i), + Name: "Expense", + Amount: int64(1000 + i), + Currency: "USD", + ExpenseType: "essentials", + TagId: "tag-1", + ExpenseDate: "2026-05-01", + PeriodYear: 2026, + PeriodMonth: 5, + Status: "active", + CreatedAt: fmt.Sprintf("2026-05-01T%02d:%02d:%02dZ", i/3600%24, i/60%60, i%60), + } + } + return rows +} + +func streamTagMap() map[string]string { + return map[string]string{"tag-1": "Food"} +} + +// discardCSVSink returns an emit callback that writes each formatted row into a +// csv.Writer backed by io.Discard, modelling the incremental ZIP write without +// retaining any row. flush surfaces encoding errors. +func discardCSVSink() (emit func([]string) error, flush func() error) { + w := csv.NewWriter(io.Discard) + emit = func(row []string) error { return w.Write(row) } + flush = func() error { + w.Flush() + return w.Error() + } + return emit, flush +} + +// BenchmarkExpensesProvider_StreamIncrementalWrite streams the full history into +// a discarding CSV sink at two very different row counts. Peak *retained* memory +// stays O(pageSize) regardless of total rows (asserted by +// TestExpensesProvider_StreamedConsumptionIsMemoryBounded); total allocations +// still scale with row count because every row is formatted, which is expected. +// The committed baseline records both shapes. +func BenchmarkExpensesProvider_StreamIncrementalWrite(b *testing.B) { + tagMap := streamTagMap() + for _, n := range []int{1000, 50000} { + rows := streamRowFixtures(n) + p := NewExpensesProvider(&mockExpenseServiceClient{streamRows: rows}, nil) + b.Run(fmt.Sprintf("rows=%d", n), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + emit, flush := discardCSVSink() + if err := p.streamExpenses(context.Background(), "user-1", tagMap, emit); err != nil { + b.Fatal(err) + } + if err := flush(); err != nil { + b.Fatal(err) + } + } + }) + } +} + +// retainedHeapBytes reports the live heap bytes that survive build(). A GC before +// and after each reading drops transient per-row garbage, and anything allocated +// before the call (the seeded stream rows) is live in both readings and cancels +// out, so the delta isolates what the consumer itself keeps alive. +func retainedHeapBytes(build func() any) uint64 { + var before, after runtime.MemStats + runtime.GC() + runtime.ReadMemStats(&before) + + kept := build() + + runtime.GC() + runtime.ReadMemStats(&after) + runtime.KeepAlive(kept) + + if after.HeapAlloc <= before.HeapAlloc { + return 0 + } + return after.HeapAlloc - before.HeapAlloc +} + +// TestExpensesProvider_StreamedConsumptionIsMemoryBounded is the bounded-allocation +// growth-ratio regression. It measures the streamExpenses +// primitive paired with a non-buffering sink: given a sink that writes each row +// onward (an incremental ZIP writer), the primitive retains nothing, so its peak +// memory stays O(pageSize) as the row count grows 50x. The buffered contrast +// (append every formatted row, as the old buffered consumer did) retains O(N) and +// blows past the bound, so reverting to buffered collection fails this test. +// +// This bound is a property of the primitive + sink, NOT of production Collect: +// Collect adapts the primitive with an append sink to satisfy the DataProvider +// [][]string contract, so it (and thus the export engine, which buffers each +// provider before BuildZIP) stays O(total) until the export engine is changed +// to stream directly to the ZIP writer. See perf/baseline/stream-consumer.txt. +func TestExpensesProvider_StreamedConsumptionIsMemoryBounded(t *testing.T) { + const ( + smallRows = 1000 + largeRows = 50000 + allowedGrowthFactor = 8 + // noiseFloorBytes keeps the ratio meaningful when the streamed path + // retains ~nothing: heap-measurement jitter must not manufacture a huge + // relative growth from a near-zero baseline. The buffered contrast retains + // at least the outer [][]string backing (largeRows * 24B ~= 1.2MB at 50k), + // a hard structural floor well above this bound. + noiseFloorBytes = 64 * 1024 + ) + tagMap := streamTagMap() + + streamed := func(n int) func() any { + rows := streamRowFixtures(n) + p := NewExpensesProvider(&mockExpenseServiceClient{streamRows: rows}, nil) + return func() any { + emit, flush := discardCSVSink() + require.NoError(t, p.streamExpenses(context.Background(), "user-1", tagMap, emit)) + require.NoError(t, flush()) + return nil // the streamed consumer retains nothing + } + } + + buffered := func(n int) func() any { + rows := streamRowFixtures(n) + p := NewExpensesProvider(&mockExpenseServiceClient{streamRows: rows}, nil) + return func() any { + var out [][]string + require.NoError(t, p.streamExpenses(context.Background(), "user-1", tagMap, func(row []string) error { + out = append(out, row) + return nil + })) + return out // retains O(N): the old buffer-all shape + } + } + + smallStreamed := retainedHeapBytes(streamed(smallRows)) + largeStreamed := retainedHeapBytes(streamed(largeRows)) + + base := smallStreamed + if base < noiseFloorBytes { + base = noiseFloorBytes + } + bound := base * allowedGrowthFactor + + assert.LessOrEqualf(t, largeStreamed, bound, + "streamed consumption must stay O(pageSize): retained %d bytes at %d rows vs %d at %d rows (bound %d)", + largeStreamed, largeRows, smallStreamed, smallRows, bound) + + // Guard: the old buffered shape must exceed the bound, proving the + // assertion above actually distinguishes streaming from buffering. + largeBuffered := retainedHeapBytes(buffered(largeRows)) + + t.Logf("retained heap bytes: streamed[%d]=%d streamed[%d]=%d buffered[%d]=%d (bound %d)", + smallRows, smallStreamed, largeRows, largeStreamed, largeRows, largeBuffered, bound) + + assert.Greaterf(t, largeBuffered, bound, + "buffered consumption should exceed the streamed bound (retained %d bytes, bound %d); "+ + "if this fails the memory-bound assertion is not meaningful", largeBuffered, bound) +} + +// TestExpensesProvider_StreamCancellation_StopsPromptly verifies a mid-stream +// cancellation stops the consumer immediately rather than draining the rest of +// the stream. +func TestExpensesProvider_StreamCancellation_StopsPromptly(t *testing.T) { + const total = 1000 + tagMap := streamTagMap() + p := NewExpensesProvider(&mockExpenseServiceClient{streamRows: streamRowFixtures(total)}, nil) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + consumed := 0 + err := p.streamExpenses(ctx, "user-1", tagMap, func(_ []string) error { + consumed++ + if consumed == 3 { + cancel() + } + return nil + }) + + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 3, consumed, "consumer must stop promptly after cancellation, not drain the stream") + assert.Less(t, consumed, total) +} diff --git a/services/datarights/internal/engine/providers/expenses_test.go b/services/datarights/internal/engine/providers/expenses_test.go index 684bf822..64bba7a3 100644 --- a/services/datarights/internal/engine/providers/expenses_test.go +++ b/services/datarights/internal/engine/providers/expenses_test.go @@ -31,7 +31,7 @@ func TestExpensesProvider_Headers(t *testing.T) { func TestExpensesProvider_Collect_Success(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsResp: &financepb.TagListResponse{ + getAllUserDataResp: &financepb.AllUserDataResponse{ Tags: []*financepb.TagData{ {Id: "tag-1", Name: "Food"}, {Id: "tag-2", Name: "Transport"}, @@ -40,25 +40,20 @@ func TestExpensesProvider_Collect_Success(t *testing.T) { } expenseClient := &mockExpenseServiceClient{ - getAllUserExpensesResponses: []*expensepb.ExpenseListResponse{ + streamRows: []*expensepb.ExpenseData{ { - Data: []*expensepb.ExpenseData{ - { - Id: "exp-1", - Name: "Groceries", - Amount: 4599, - Currency: "USD", - ExpenseType: "essentials", - TagId: "tag-1", - ExpenseDate: "2026-05-01", - PeriodYear: 2026, - PeriodMonth: 5, - Status: "active", - IsProRata: false, - CreatedAt: "2026-05-01T12:00:00Z", - }, - }, - HasMore: false, + Id: "exp-1", + Name: "Groceries", + Amount: 4599, + Currency: "USD", + ExpenseType: "essentials", + TagId: "tag-1", + ExpenseDate: "2026-05-01", + PeriodYear: 2026, + PeriodMonth: 5, + Status: "active", + IsProRata: false, + CreatedAt: "2026-05-01T12:00:00Z", }, }, } @@ -79,7 +74,7 @@ func TestExpensesProvider_Collect_Success(t *testing.T) { func TestExpensesProvider_Collect_ProRataExpense(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsResp: &financepb.TagListResponse{ + getAllUserDataResp: &financepb.AllUserDataResponse{ Tags: []*financepb.TagData{ {Id: "tag-1", Name: "Rent"}, }, @@ -87,28 +82,23 @@ func TestExpensesProvider_Collect_ProRataExpense(t *testing.T) { } expenseClient := &mockExpenseServiceClient{ - getAllUserExpensesResponses: []*expensepb.ExpenseListResponse{ + streamRows: []*expensepb.ExpenseData{ { - Data: []*expensepb.ExpenseData{ - { - Id: "exp-pr-1", - Name: "Rent (1/3)", - Amount: 50000, - Currency: "USD", - ExpenseType: "essentials", - TagId: "tag-1", - ExpenseDate: "2026-05-01", - PeriodYear: 2026, - PeriodMonth: 5, - Status: "active", - IsProRata: true, - ProRataGroup: "group-abc", - ProRataIndex: 1, - ProRataTotal: 3, - CreatedAt: "2026-05-01T10:00:00Z", - }, - }, - HasMore: false, + Id: "exp-pr-1", + Name: "Rent (1/3)", + Amount: 50000, + Currency: "USD", + ExpenseType: "essentials", + TagId: "tag-1", + ExpenseDate: "2026-05-01", + PeriodYear: 2026, + PeriodMonth: 5, + Status: "active", + IsProRata: true, + ProRataGroup: "group-abc", + ProRataIndex: 1, + ProRataTotal: 3, + CreatedAt: "2026-05-01T10:00:00Z", }, }, } @@ -129,30 +119,25 @@ func TestExpensesProvider_Collect_ProRataExpense(t *testing.T) { func TestExpensesProvider_Collect_MissingTagResolvesToUnknown(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsResp: &financepb.TagListResponse{ + getAllUserDataResp: &financepb.AllUserDataResponse{ Tags: []*financepb.TagData{}, // No tags }, } expenseClient := &mockExpenseServiceClient{ - getAllUserExpensesResponses: []*expensepb.ExpenseListResponse{ + streamRows: []*expensepb.ExpenseData{ { - Data: []*expensepb.ExpenseData{ - { - Id: "exp-1", - Name: "Mystery", - Amount: 1000, - Currency: "USD", - ExpenseType: "desires", - TagId: "deleted-tag-id", - ExpenseDate: "2026-01-15", - PeriodYear: 2026, - PeriodMonth: 1, - Status: "active", - CreatedAt: "2026-01-15T08:00:00Z", - }, - }, - HasMore: false, + Id: "exp-1", + Name: "Mystery", + Amount: 1000, + Currency: "USD", + ExpenseType: "desires", + TagId: "deleted-tag-id", + ExpenseDate: "2026-01-15", + PeriodYear: 2026, + PeriodMonth: 1, + Status: "active", + CreatedAt: "2026-01-15T08:00:00Z", }, }, } @@ -165,30 +150,22 @@ func TestExpensesProvider_Collect_MissingTagResolvesToUnknown(t *testing.T) { assert.Equal(t, "Unknown", rows[0][5]) // tag_name column } -func TestExpensesProvider_Collect_Pagination(t *testing.T) { +func TestExpensesProvider_Collect_MultipleRowsInStreamOrder(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsResp: &financepb.TagListResponse{ + getAllUserDataResp: &financepb.AllUserDataResponse{ Tags: []*financepb.TagData{ {Id: "tag-1", Name: "Food"}, }, }, } + // The server streams every expense in one ordered pass (chronological: + // created_at ASC, id ASC); the consumer no longer pages. expenseClient := &mockExpenseServiceClient{ - getAllUserExpensesResponses: []*expensepb.ExpenseListResponse{ - { - Data: []*expensepb.ExpenseData{ - {Id: "exp-1", Name: "Page1", Amount: 100, TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 1, CreatedAt: "2026-01-01T00:00:00Z"}, - {Id: "exp-2", Name: "Page1b", Amount: 200, TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 1, CreatedAt: "2026-01-02T00:00:00Z"}, - }, - HasMore: true, - }, - { - Data: []*expensepb.ExpenseData{ - {Id: "exp-3", Name: "Page2", Amount: 300, TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 2, CreatedAt: "2026-02-01T00:00:00Z"}, - }, - HasMore: false, - }, + streamRows: []*expensepb.ExpenseData{ + {Id: "exp-1", Name: "First", Amount: 100, TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 1, CreatedAt: "2026-01-01T00:00:00Z"}, + {Id: "exp-2", Name: "Second", Amount: 200, TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 1, CreatedAt: "2026-01-02T00:00:00Z"}, + {Id: "exp-3", Name: "Third", Amount: 300, TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 2, CreatedAt: "2026-02-01T00:00:00Z"}, }, } @@ -200,19 +177,17 @@ func TestExpensesProvider_Collect_Pagination(t *testing.T) { assert.Equal(t, "exp-1", rows[0][0]) assert.Equal(t, "exp-2", rows[1][0]) assert.Equal(t, "exp-3", rows[2][0]) - assert.Equal(t, 2, expenseClient.callCount) + // A single StreamAllUserExpenses call replaces the old per-page unary loop. + assert.Equal(t, 1, expenseClient.callCount) + assert.Equal(t, int32(expensesPageSize), expenseClient.lastStreamReq.GetPageSize()) } func TestExpensesProvider_Collect_EmptyData(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsResp: &financepb.TagListResponse{Tags: []*financepb.TagData{}}, + getAllUserDataResp: &financepb.AllUserDataResponse{Tags: []*financepb.TagData{}}, } - expenseClient := &mockExpenseServiceClient{ - getAllUserExpensesResponses: []*expensepb.ExpenseListResponse{ - {Data: []*expensepb.ExpenseData{}, HasMore: false}, - }, - } + expenseClient := &mockExpenseServiceClient{streamRows: nil} p := NewExpensesProvider(expenseClient, financeClient) rows, err := p.Collect(context.Background(), "user-123") @@ -221,13 +196,39 @@ func TestExpensesProvider_Collect_EmptyData(t *testing.T) { assert.Empty(t, rows) } -func TestExpensesProvider_Collect_ExpenseServiceError(t *testing.T) { +func TestExpensesProvider_Collect_StreamOpenError(t *testing.T) { + financeClient := &mockFinanceServiceClient{ + getAllUserDataResp: &financepb.AllUserDataResponse{Tags: []*financepb.TagData{}}, + } + + expenseClient := &mockExpenseServiceClient{ + streamOpenErr: fmt.Errorf("connection refused"), + } + + p := NewExpensesProvider(expenseClient, financeClient) + rows, err := p.Collect(context.Background(), "user-123") + + assert.Nil(t, rows) + require.Error(t, err) + assert.Contains(t, err.Error(), "fetching expenses") +} + +func TestExpensesProvider_Collect_MidStreamRecvError(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsResp: &financepb.TagListResponse{Tags: []*financepb.TagData{}}, + getAllUserDataResp: &financepb.AllUserDataResponse{ + Tags: []*financepb.TagData{{Id: "tag-1", Name: "Food"}}, + }, } + // Two rows arrive, then the third Recv fails: the error must propagate + // (not be swallowed as a clean EOF). expenseClient := &mockExpenseServiceClient{ - getAllUserExpensesErr: fmt.Errorf("connection refused"), + streamRows: []*expensepb.ExpenseData{ + {Id: "exp-1", TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 1, CreatedAt: "2026-01-01T00:00:00Z"}, + {Id: "exp-2", TagId: "tag-1", PeriodYear: 2026, PeriodMonth: 1, CreatedAt: "2026-01-02T00:00:00Z"}, + }, + recvErr: fmt.Errorf("stream reset"), + recvErrAt: 3, } p := NewExpensesProvider(expenseClient, financeClient) @@ -236,11 +237,12 @@ func TestExpensesProvider_Collect_ExpenseServiceError(t *testing.T) { assert.Nil(t, rows) require.Error(t, err) assert.Contains(t, err.Error(), "fetching expenses") + assert.Contains(t, err.Error(), "stream reset") } func TestExpensesProvider_Collect_TagServiceError(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsErr: fmt.Errorf("service unavailable"), + getAllUserDataErr: fmt.Errorf("service unavailable"), } expenseClient := &mockExpenseServiceClient{} @@ -255,31 +257,26 @@ func TestExpensesProvider_Collect_TagServiceError(t *testing.T) { func TestExpensesProvider_Collect_CorrectedExpense(t *testing.T) { financeClient := &mockFinanceServiceClient{ - listTagsResp: &financepb.TagListResponse{ + getAllUserDataResp: &financepb.AllUserDataResponse{ Tags: []*financepb.TagData{{Id: "tag-1", Name: "Food"}}, }, } expenseClient := &mockExpenseServiceClient{ - getAllUserExpensesResponses: []*expensepb.ExpenseListResponse{ + streamRows: []*expensepb.ExpenseData{ { - Data: []*expensepb.ExpenseData{ - { - Id: "exp-correction", - Name: "Groceries (corrected)", - Amount: 5099, - Currency: "USD", - ExpenseType: "essentials", - TagId: "tag-1", - ExpenseDate: "2026-05-01", - PeriodYear: 2026, - PeriodMonth: 5, - Status: "corrected", - CorrectsId: "exp-original", - CreatedAt: "2026-05-02T09:00:00Z", - }, - }, - HasMore: false, + Id: "exp-correction", + Name: "Groceries (corrected)", + Amount: 5099, + Currency: "USD", + ExpenseType: "essentials", + TagId: "tag-1", + ExpenseDate: "2026-05-01", + PeriodYear: 2026, + PeriodMonth: 5, + Status: "corrected", + CorrectsId: "exp-original", + CreatedAt: "2026-05-02T09:00:00Z", }, }, } diff --git a/services/datarights/internal/engine/providers/mock_clients_test.go b/services/datarights/internal/engine/providers/mock_clients_test.go index 0380ed2b..60904b51 100644 --- a/services/datarights/internal/engine/providers/mock_clients_test.go +++ b/services/datarights/internal/engine/providers/mock_clients_test.go @@ -2,6 +2,7 @@ package providers import ( "context" + "io" "google.golang.org/grpc" @@ -13,8 +14,6 @@ import ( type mockFinanceServiceClient struct { getAllUserDataResp *financepb.AllUserDataResponse getAllUserDataErr error - listTagsResp *financepb.TagListResponse - listTagsErr error } func (m *mockFinanceServiceClient) GetAllUserData(_ context.Context, _ *financepb.GetAllUserDataRequest, _ ...grpc.CallOption) (*financepb.AllUserDataResponse, error) { @@ -25,10 +24,7 @@ func (m *mockFinanceServiceClient) GetAllUserData(_ context.Context, _ *financep } func (m *mockFinanceServiceClient) ListTags(_ context.Context, _ *financepb.ListTagsRequest, _ ...grpc.CallOption) (*financepb.TagListResponse, error) { - if m.listTagsErr != nil { - return nil, m.listTagsErr - } - return m.listTagsResp, nil + return nil, nil } // Implement remaining interface methods as no-ops. @@ -87,24 +83,52 @@ func (m *mockFinanceServiceClient) DeleteAllUserData(_ context.Context, _ *finan return nil, nil } -// mockExpenseServiceClient implements ExpenseServiceClient for tests. +// mockExpenseServiceClient implements ExpenseServiceClient for tests. The +// expenses provider consumes StreamAllUserExpenses, so streamRows seeds the +// server stream (one ExpenseData per Recv, in order). streamOpenErr fails the +// RPC open; recvErr fails a Recv (at recvErrAt, 1-based; 0 = after all rows). type mockExpenseServiceClient struct { - // getAllUserExpensesResponses is a list of responses, one per page call. - getAllUserExpensesResponses []*expensepb.ExpenseListResponse - getAllUserExpensesErr error - callCount int + streamRows []*expensepb.ExpenseData + streamOpenErr error + recvErr error + recvErrAt int + lastStreamReq *expensepb.StreamAllUserExpensesRequest + callCount int +} + +// fakeExpenseStream is the client side of a StreamAllUserExpenses server stream. +// The embedded nil grpc.ClientStream supplies the ClientStream methods the +// consumer never calls; only Recv is exercised. +type fakeExpenseStream struct { + grpc.ClientStream + rows []*expensepb.ExpenseData + idx int + recvErr error + recvErrAt int } -func (m *mockExpenseServiceClient) GetAllUserExpenses(_ context.Context, _ *expensepb.GetAllUserExpensesRequest, _ ...grpc.CallOption) (*expensepb.ExpenseListResponse, error) { - if m.getAllUserExpensesErr != nil { - return nil, m.getAllUserExpensesErr +func (f *fakeExpenseStream) Recv() (*expensepb.ExpenseData, error) { + if f.recvErr != nil && f.recvErrAt > 0 && f.idx+1 == f.recvErrAt { + return nil, f.recvErr } - if m.callCount >= len(m.getAllUserExpensesResponses) { - return &expensepb.ExpenseListResponse{}, nil + if f.idx >= len(f.rows) { + if f.recvErr != nil && f.recvErrAt == 0 { + return nil, f.recvErr + } + return nil, io.EOF } - resp := m.getAllUserExpensesResponses[m.callCount] + row := f.rows[f.idx] + f.idx++ + return row, nil +} + +func (m *mockExpenseServiceClient) StreamAllUserExpenses(_ context.Context, req *expensepb.StreamAllUserExpensesRequest, _ ...grpc.CallOption) (grpc.ServerStreamingClient[expensepb.ExpenseData], error) { m.callCount++ - return resp, nil + m.lastStreamReq = req + if m.streamOpenErr != nil { + return nil, m.streamOpenErr + } + return &fakeExpenseStream{rows: m.streamRows, recvErr: m.recvErr, recvErrAt: m.recvErrAt}, nil } // Implement remaining interface methods as no-ops. diff --git a/services/datarights/internal/engine/registry.go b/services/datarights/internal/engine/registry.go deleted file mode 100644 index 547bae63..00000000 --- a/services/datarights/internal/engine/registry.go +++ /dev/null @@ -1,22 +0,0 @@ -package engine - -// ProviderRegistry holds all registered data providers. -// Providers are registered at startup and iterated during export execution. -type ProviderRegistry struct { - providers []DataProvider -} - -// NewProviderRegistry creates an empty provider registry. -func NewProviderRegistry() *ProviderRegistry { - return &ProviderRegistry{} -} - -// Register adds a provider to the registry. -func (r *ProviderRegistry) Register(p DataProvider) { - r.providers = append(r.providers, p) -} - -// All returns all registered providers. -func (r *ProviderRegistry) All() []DataProvider { - return r.providers -} diff --git a/services/datarights/internal/engine/testdata/export/budget_periods.csv b/services/datarights/internal/engine/testdata/export/budget_periods.csv new file mode 100644 index 00000000..2f738223 --- /dev/null +++ b/services/datarights/internal/engine/testdata/export/budget_periods.csv @@ -0,0 +1,2 @@ +id,year,month,budget_amount,essentials_percent,desires_percent,savings_percent,created_at +period-1,2026,5,2500.00,50,30,20,2026-05-01T00:00:00Z diff --git a/services/datarights/internal/engine/testdata/export/default_settings.csv b/services/datarights/internal/engine/testdata/export/default_settings.csv new file mode 100644 index 00000000..63c78982 --- /dev/null +++ b/services/datarights/internal/engine/testdata/export/default_settings.csv @@ -0,0 +1,2 @@ +budget_amount,essentials_percent,desires_percent,savings_percent,currency +3000.00,50,30,20,USD diff --git a/services/datarights/internal/engine/testdata/export/expenses.csv b/services/datarights/internal/engine/testdata/export/expenses.csv new file mode 100644 index 00000000..c64d2bb5 --- /dev/null +++ b/services/datarights/internal/engine/testdata/export/expenses.csv @@ -0,0 +1,3 @@ +id,name,amount,currency,expense_type,tag_name,expense_date,period_year,period_month,status,corrects_id,is_pro_rata,pro_rata_group,pro_rata_index,pro_rata_total,created_at +exp-1,Groceries,45.99,USD,essentials,Food,2026-05-01,2026,5,active,,false,,,,2026-05-01T12:00:00Z +exp-2,Bus pass,30.00,USD,essentials,Transport,2026-05-02,2026,5,active,,false,,,,2026-05-02T09:00:00Z diff --git a/services/datarights/internal/engine/testdata/export/profile.csv b/services/datarights/internal/engine/testdata/export/profile.csv new file mode 100644 index 00000000..50201451 --- /dev/null +++ b/services/datarights/internal/engine/testdata/export/profile.csv @@ -0,0 +1,2 @@ +username,email,currency,role,account_created_at +alex,alex@example.com,USD,member,2025-01-01T00:00:00Z diff --git a/services/datarights/internal/engine/testdata/export/tags.csv b/services/datarights/internal/engine/testdata/export/tags.csv new file mode 100644 index 00000000..a9e0a7fc --- /dev/null +++ b/services/datarights/internal/engine/testdata/export/tags.csv @@ -0,0 +1,3 @@ +id,name,is_default,created_at +tag-1,Food,true,2025-06-01T10:00:00Z +tag-2,Transport,false,2025-07-15T14:30:00Z diff --git a/services/datarights/perf/baseline/export.txt b/services/datarights/perf/baseline/export.txt new file mode 100644 index 00000000..6664a64b --- /dev/null +++ b/services/datarights/perf/baseline/export.txt @@ -0,0 +1,59 @@ +# gofin perf baseline +path: datarights/export-collection (finance-call dedup, P2a) +captured_against: ft/latency-refactor @ 1825b89 (pre-dedup; BenchmarkExportCollection over the full provider set) +machine: Apple M4 Pro, 14 cores, macOS (wall-clock reference only) +date: 2026-07-10 + +# Environment-independent (portable, gated in CI via the call-count assertion) +# Per export, the finance client is hit for data one GetAllUserData call carries: +# tags, budget_periods, default_settings each call GetAllUserData -> 3 +# expenses.buildTagMap calls ListTags -> 1 +get_all_user_data_calls: 3 +list_tags_calls: 1 +finance_calls_total: 4 + +# Wall-clock reference (same-machine only, NOT a threshold) +# go test -bench BenchmarkExportCollection -benchmem -count=1 +ns_op: 1395 +bytes_op: 3883 +allocs_op: 58 + +# Target after dedup (this ticket): a fresh per-job MemoizedFinanceClient serves +# GetAllUserData at most once and the expenses provider derives its tag map from +# GetAllUserData().GetTags(), so ListTags drops to 0: +# get_all_user_data_calls: <= 1 +# list_tags_calls: 0 +# Enforced by TestExport_DedupesFinanceCalls in internal/engine (test-backend CI). + +# --------------------------------------------------------------------------- +# Collection fan-out (P2b, ticket #6) +# --------------------------------------------------------------------------- +# path: datarights/export-collection-scaling +# BenchmarkEngineCollectionFanout drives engine.execute over five providers with +# simulated per-provider latency of 1x..5x a 10ms base (10, 20, 30, 40, 50 ms). +# Serial collection costs the sum (150ms); the errgroup fan-out costs the max +# (50ms). go test -bench BenchmarkEngineCollectionFanout -benchmem -benchtime=15x +# machine: Apple M4 Pro, 14 cores, macOS (wall-clock reference only, NOT a gate) +# +# Structural expectation (portable, the durable signal): +# sum_of_provider_latencies: 150ms (1+2+3+4+5)*10ms +# max_provider_latency: 50ms +# before (serial loop): ns_op ~= sum (~155ms) +# after (errgroup fan): ns_op ~= max (~52ms) +# +# before (serial, captured @ pre-fan-out on ft/latency-refactor): +# ns_op: 154835733 (~155ms ~= sum) +# bytes_op: 143614 +# allocs_op: 181 +# after (errgroup fan-out, this ticket): +# ns_op: 51121856 (~51ms ~= max) +# bytes_op: 416401 +# allocs_op: 202 +# Net: collection dropped from ~155ms (sum) to ~51ms (max), a ~3x reduction that +# matches max/sum = 50/150. Extra bytes/allocs are the five goroutines' stacks +# plus errgroup bookkeeping; expected and bounded by the fixed provider count. +# +# The fan-out is additionally proven structurally (not by wall-clock) in +# test-backend: TestEngine_FanOut_RunsProvidersConcurrently asserts all five +# providers are in flight at once (max-concurrent == provider count), which is +# the deterministic, machine-independent form of "max, not sum". diff --git a/services/datarights/perf/baseline/stream-consumer.txt b/services/datarights/perf/baseline/stream-consumer.txt new file mode 100644 index 00000000..a649b458 --- /dev/null +++ b/services/datarights/perf/baseline/stream-consumer.txt @@ -0,0 +1,46 @@ +# gofin perf baseline +path: datarights/stream-consumer (expenses export stream cutover) +captured_against: ft/latency-refactor (buffered shape vs streamed shape) +machine: Apple M4 Pro, 14 cores, macOS (wall-clock / B/op are same-machine references only) +date: 2026-07-10 + +# What changed +# Before (StreamAllUserExpenses RPC existed but the datarights consumer still buffered): +# ExpensesProvider fetched every page of the unary GetAllUserExpenses RPC into +# one []*ExpenseData slice, then formatted all rows -> peak memory O(total rows). +# After: ExpensesProvider consumes StreamAllUserExpenses via a +# Recv() loop and emits each formatted row to a sink as it arrives. Given a sink +# that writes each row onward (an incremental CSV/ZIP writer) the consumer +# retains nothing -> peak memory O(pageSize). +# +# SCOPE OF THE BOUND: the O(pageSize) result below is a property of the +# streamExpenses primitive + a non-buffering sink. Production Collect adapts +# that primitive with an append sink to satisfy the DataProvider [][]string +# contract, and the export engine buffers each provider's rows before BuildZIP, +# so END-TO-END export memory stays O(total rows) until a follow-up engine-level +# streaming cutover. This baseline bounds and proves the *consumer primitive*; +# the metric must not be read as an end-to-end production figure. + +# Environment-independent structural signal (gated in test-backend CI): +# Peak *retained* heap of the streamExpenses primitive (with a non-buffering +# sink) stays flat as total rows grow 50x; the buffered shape grows linearly. +# Enforced by TestExpensesProvider_StreamedConsumptionIsMemoryBounded in +# internal/engine/providers (bounded-allocation growth-ratio, -race clean). +# +# Retained heap bytes (discarding CSV sink; GC-fenced HeapAlloc delta): +# streamed[rows=1000]: 0 # O(pageSize): retains nothing +# streamed[rows=50000]: 0 # FLAT as rows grow 50x -> the memory bound +# buffered[rows=50000]: ~2.1 MB # O(total rows): the old buffered shape +# assertion bound: 512 KB # max(streamed_small, 64KB) * 8 + +# Wall-clock / total-allocation reference (same-machine only, NOT a threshold). +# Total allocs scale with row count because every row is formatted; this is +# expected. The gated guarantee is retained (peak-live) memory, not total churn. +# go test -bench BenchmarkExpensesProvider_StreamIncrementalWrite -benchmem -run '^$' -count=1 +# rows=1000: ~11.2 ms/op 284664 B/op 4007 allocs/op +# rows=50000: ~702.4 ms/op 14007556 B/op 200011 allocs/op + +# Byte-identical export guarantee: +# The streamed CSV is byte-identical to the buffered-consumer output for the same +# seeded data (same rows, chronological created_at ASC with the id tiebreaker), +# guarded by TestExportProviders_CSVByteIdentical in internal/engine. diff --git a/services/expense/internal/handler/grpc.go b/services/expense/internal/handler/grpc.go index 22707683..79447c56 100644 --- a/services/expense/internal/handler/grpc.go +++ b/services/expense/internal/handler/grpc.go @@ -2,6 +2,7 @@ package handler import ( "context" + "errors" "log/slog" "google.golang.org/grpc/codes" @@ -158,24 +159,24 @@ func (h *GRPCHandler) CountExpensesByTag(ctx context.Context, req *pb.CountExpen }, nil } -func (h *GRPCHandler) GetAllUserExpenses(ctx context.Context, req *pb.GetAllUserExpensesRequest) (*pb.ExpenseListResponse, error) { - result, err := h.expenseService.GetAllUserExpenses(ctx, req.GetUserId(), req.GetPage(), req.GetPageSize()) - if err != nil { - return nil, mapServiceError(err) +func (h *GRPCHandler) StreamAllUserExpenses(req *pb.StreamAllUserExpensesRequest, stream pb.ExpenseService_StreamAllUserExpensesServer) error { + err := h.expenseService.StreamAllUserExpenses(stream.Context(), req.GetUserId(), req.GetPageSize(), func(expense *model.Expense) error { + return stream.Send(expenseToProto(expense)) + }) + if err == nil { + return nil } - - protoExpenses := make([]*pb.ExpenseData, len(result.Data)) - for i, expense := range result.Data { - protoExpenses[i] = expenseToProto(expense) + var svcErr *service.ServiceError + if errors.As(err, &svcErr) { + return mapServiceError(err) } - - return &pb.ExpenseListResponse{ - Data: protoExpenses, - Total: result.Total, - Page: result.Page, - PageSize: result.PageSize, - HasMore: result.HasMore, - }, nil + // Normalize context cancellation / deadline so gRPC reports codes.Canceled / + // codes.DeadlineExceeded rather than codes.Unknown. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return status.FromContextError(err).Err() + } + // Stream-send failures are already gRPC-meaningful; surface them directly. + return err } func (h *GRPCHandler) AnonymizeAllUserExpenses(ctx context.Context, req *pb.AnonymizeRequest) (*pb.AnonymizeResponse, error) { @@ -216,7 +217,8 @@ func expenseToProto(e *model.Expense) *pb.ExpenseData { // mapServiceError converts a service-layer error to a gRPC status error. func mapServiceError(err error) error { - if svcErr, ok := err.(*service.ServiceError); ok { + var svcErr *service.ServiceError + if errors.As(err, &svcErr) { switch svcErr.Code { case model.ErrValidationError: return status.Error(codes.InvalidArgument, svcErr.Message) diff --git a/services/expense/internal/handler/grpc_stream_test.go b/services/expense/internal/handler/grpc_stream_test.go new file mode 100644 index 00000000..447d5364 --- /dev/null +++ b/services/expense/internal/handler/grpc_stream_test.go @@ -0,0 +1,133 @@ +package handler + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/ItsThompson/gofin/services/expense/internal/model" + "github.com/ItsThompson/gofin/services/expense/internal/repository" + pb "github.com/ItsThompson/gofin/services/expense/proto/expensepb" +) + +// fakeStreamServer implements pb.ExpenseService_StreamAllUserExpensesServer +// (grpc.ServerStreamingServer[pb.ExpenseData]) for handler tests. The embedded +// nil grpc.ServerStream supplies the unused stream methods; only Context and +// Send are exercised. +type fakeStreamServer struct { + grpc.ServerStream + ctx context.Context + sent []*pb.ExpenseData + sendErr error + failAt int // 1-based row index whose Send returns sendErr; 0 = never fail +} + +func (f *fakeStreamServer) Context() context.Context { + if f.ctx == nil { + return context.Background() + } + return f.ctx +} + +func (f *fakeStreamServer) Send(e *pb.ExpenseData) error { + if f.failAt > 0 && len(f.sent)+1 == f.failAt { + return f.sendErr + } + f.sent = append(f.sent, e) + return nil +} + +func streamRow(id, createdAt string) *model.Expense { + return &model.Expense{ + ID: id, + UserID: "user-1", + Name: "Expense " + id, + Amount: 2500, + Currency: "USD", + ExpenseType: "essentials", + TagID: "tag-1", + ExpenseDate: "2026-05-01", + PeriodYear: 2026, + PeriodMonth: 5, + Status: "active", + CreatedAt: createdAt, + } +} + +func TestGRPC_StreamAllUserExpenses_SendsAllRowsAsProto(t *testing.T) { + repo := new(mockExpenseRepository) + handler := newTestGRPCHandler(repo) + + page := []*model.Expense{streamRow("exp-1", "2026-05-01T00:00:00Z"), streamRow("exp-2", "2026-05-01T00:00:01Z")} + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", repository.ExpenseCursor{}, int32(2)). + Return(page, repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:01Z", ID: "exp-2"}, false, nil) + + stream := &fakeStreamServer{} + err := handler.StreamAllUserExpenses(&pb.StreamAllUserExpensesRequest{UserId: "user-1", PageSize: 2}, stream) + + require.NoError(t, err) + require.Len(t, stream.sent, 2) + assert.Equal(t, "exp-1", stream.sent[0].GetId()) + assert.Equal(t, "exp-2", stream.sent[1].GetId()) + assert.Equal(t, int64(2500), stream.sent[0].GetAmount()) + assert.Equal(t, "2026-05-01T00:00:01Z", stream.sent[1].GetCreatedAt()) + repo.AssertExpectations(t) +} + +func TestGRPC_StreamAllUserExpenses_EmptyUserIDReturnsInvalidArgument(t *testing.T) { + repo := new(mockExpenseRepository) + handler := newTestGRPCHandler(repo) + + stream := &fakeStreamServer{} + err := handler.StreamAllUserExpenses(&pb.StreamAllUserExpensesRequest{UserId: ""}, stream) + + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.InvalidArgument, st.Code()) + assert.Empty(t, stream.sent) +} + +func TestGRPC_StreamAllUserExpenses_PropagatesSendError(t *testing.T) { + repo := new(mockExpenseRepository) + handler := newTestGRPCHandler(repo) + + page := []*model.Expense{streamRow("exp-1", "2026-05-01T00:00:00Z"), streamRow("exp-2", "2026-05-01T00:00:01Z")} + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", mock.Anything, mock.Anything). + Return(page, repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:01Z", ID: "exp-2"}, false, nil) + + sendErr := errors.New("client disconnected") + stream := &fakeStreamServer{sendErr: sendErr, failAt: 1} + err := handler.StreamAllUserExpenses(&pb.StreamAllUserExpensesRequest{UserId: "user-1", PageSize: 2}, stream) + + require.ErrorIs(t, err, sendErr) + assert.Empty(t, stream.sent) +} + +func TestGRPC_StreamAllUserExpenses_CancelledContextMapsToCanceled(t *testing.T) { + repo := new(mockExpenseRepository) + handler := newTestGRPCHandler(repo) + + // hasMore stays true; the pre-cancelled context stops the walk. The handler + // must normalize context.Canceled to codes.Canceled (not codes.Unknown). + page := []*model.Expense{streamRow("exp-1", "2026-05-01T00:00:00Z")} + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", mock.Anything, mock.Anything). + Return(page, repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:00Z", ID: "exp-1"}, true, nil) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + stream := &fakeStreamServer{ctx: ctx} + err := handler.StreamAllUserExpenses(&pb.StreamAllUserExpensesRequest{UserId: "user-1", PageSize: 2}, stream) + + require.Error(t, err) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.Canceled, st.Code()) +} diff --git a/services/expense/internal/handler/rest_test.go b/services/expense/internal/handler/rest_test.go index 23bd456b..e46e021a 100644 --- a/services/expense/internal/handler/rest_test.go +++ b/services/expense/internal/handler/rest_test.go @@ -17,6 +17,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ItsThompson/gofin/services/expense/internal/model" + "github.com/ItsThompson/gofin/services/expense/internal/repository" "github.com/ItsThompson/gofin/services/expense/internal/service" ) @@ -86,19 +87,20 @@ func (m *mockExpenseRepository) GetActiveExpenseSuggestionInputs(ctx context.Con return args.Get(0).([]*model.ExpenseSuggestionInput), args.Error(1) } -func (m *mockExpenseRepository) GetAllExpensesByUser(ctx context.Context, userID string, page, pageSize int32) ([]*model.Expense, int64, error) { - args := m.Called(ctx, userID, page, pageSize) - if args.Get(0) == nil { - return nil, args.Get(1).(int64), args.Error(2) - } - return args.Get(0).([]*model.Expense), args.Get(1).(int64), args.Error(2) -} - func (m *mockExpenseRepository) AnonymizeAllUserExpenses(ctx context.Context, userID string) error { args := m.Called(ctx, userID) return args.Error(0) } +func (m *mockExpenseRepository) GetExpensesByUserAfter(ctx context.Context, userID string, cursor repository.ExpenseCursor, pageSize int32) ([]*model.Expense, repository.ExpenseCursor, bool, error) { + args := m.Called(ctx, userID, cursor, pageSize) + var rows []*model.Expense + if args.Get(0) != nil { + rows = args.Get(0).([]*model.Expense) + } + return rows, args.Get(1).(repository.ExpenseCursor), args.Bool(2), args.Error(3) +} + func setupTestRouter(repo *mockExpenseRepository) *gin.Engine { gin.SetMode(gin.TestMode) logger := slog.New(slog.NewJSONHandler(io.Discard, nil)) diff --git a/services/expense/internal/repository/immudb.go b/services/expense/internal/repository/immudb.go index f17eaea6..44e457b0 100644 --- a/services/expense/internal/repository/immudb.go +++ b/services/expense/internal/repository/immudb.go @@ -54,7 +54,10 @@ func (r *ImmudbExpenseRepository) InitSchema(ctx context.Context) error { `CREATE INDEX IF NOT EXISTS idx_expenses_user_period ON expenses (user_id, period_year, period_month, status);`, `CREATE INDEX IF NOT EXISTS idx_expenses_corrects ON expenses (corrects_id);`, `CREATE INDEX IF NOT EXISTS idx_expenses_prorata_group ON expenses (pro_rata_group);`, - `CREATE INDEX IF NOT EXISTS idx_expenses_user_created ON expenses (user_id, created_at);`, + // Covers the keyset export seek: the (created_at, id) tiebreaker column is + // included so the index matches the full ORDER BY created_at ASC, id ASC + // tuple and rows tied on created_at seek instead of scanning+sorting. + `CREATE INDEX IF NOT EXISTS idx_expenses_user_created_id ON expenses (user_id, created_at, id);`, } for _, idx := range indexes { @@ -406,47 +409,65 @@ func (r *ImmudbExpenseRepository) GetProRataGroup(ctx context.Context, groupID s return expenses, nil } -// GetAllExpensesByUser returns all expenses (active + corrected) for a user, -// ordered by created_at ASC with LIMIT/OFFSET pagination. No status filter -// is applied: GDPR export requires the full correction chain. -func (r *ImmudbExpenseRepository) GetAllExpensesByUser(ctx context.Context, userID string, page, pageSize int32) ([]*model.Expense, int64, error) { - // Count query for pagination metadata - countQuery := `SELECT COUNT(*) FROM expenses WHERE user_id = @user_id;` - - countResult, err := r.client.SQLQuery(ctx, countQuery, map[string]interface{}{ +// GetExpensesByUserAfter returns one keyset page of expenses (active + +// corrected) for a user past the given cursor, ordered by +// (created_at ASC, id ASC). It seeks with the expanded-OR (created_at, id) +// predicate instead of LIMIT/OFFSET and derives hasMore by fetching pageSize+1 +// rows and inspecting the overflow row, so it issues no OFFSET and no per-page +// COUNT(*). An empty cursor starts from the beginning. +// +// immudb 1.11.0 does not support SQL row-value tuple syntax +// ((created_at, id) > (@c, @cid) raises a syntax error), so the comparison is +// written in expanded-OR form. +func (r *ImmudbExpenseRepository) GetExpensesByUserAfter(ctx context.Context, userID string, cursor ExpenseCursor, pageSize int32) ([]*model.Expense, ExpenseCursor, bool, error) { + if pageSize < 1 { + pageSize = DefaultStreamPageSize + } + + // Fetch one extra row (pageSize+1) so the overflow row reveals whether more + // rows remain, avoiding a per-page COUNT(*). + params := map[string]interface{}{ "user_id": userID, - }) - if err != nil { - return nil, 0, fmt.Errorf("counting all user expenses: %w", err) + "limit": pageSize + 1, } - var total int64 - if len(countResult.Rows) > 0 && len(countResult.Rows[0].Values) > 0 { - total = countResult.Rows[0].Values[0].GetInt() + cursorPredicate := "" + if cursor.CreatedAt != "" { + cursorPredicate = ` AND (created_at > @cursor_created_at + OR (created_at = @cursor_created_at AND id > @cursor_id))` + params["cursor_created_at"] = cursor.CreatedAt + params["cursor_id"] = cursor.ID } - // Data query with pagination, ordered by created_at ASC (chronological for export) - offset := (page - 1) * pageSize dataQuery := fmt.Sprintf(`SELECT %s FROM expenses - WHERE user_id = @user_id - ORDER BY created_at ASC - LIMIT @limit OFFSET @offset;`, expenseSelectColumns) + WHERE user_id = @user_id%s + ORDER BY created_at ASC, id ASC + LIMIT @limit;`, expenseSelectColumns, cursorPredicate) - result, err := r.client.SQLQuery(ctx, dataQuery, map[string]interface{}{ - "user_id": userID, - "limit": pageSize, - "offset": offset, - }) + result, err := r.client.SQLQuery(ctx, dataQuery, params) if err != nil { - return nil, 0, fmt.Errorf("querying all user expenses: %w", err) + return nil, ExpenseCursor{}, false, fmt.Errorf("querying user expenses after cursor: %w", err) } - expenses := make([]*model.Expense, 0, len(result.Rows)) + rows := make([]*model.Expense, 0, len(result.Rows)) for _, row := range result.Rows { - expenses = append(expenses, rowToExpense(row)) + rows = append(rows, rowToExpense(row)) } - return expenses, total, nil + // The overflow row (pageSize+1th) means more rows remain. Drop it from the + // page and report hasMore. + hasMore := int32(len(rows)) > pageSize + if hasMore { + rows = rows[:pageSize] + } + + next := cursor + if len(rows) > 0 { + last := rows[len(rows)-1] + next = ExpenseCursor{CreatedAt: last.CreatedAt, ID: last.ID} + } + + return rows, next, hasMore, nil } // AnonymizeAllUserExpenses redacts PII fields on all expense rows for a user. diff --git a/services/expense/internal/repository/immudb_bench_test.go b/services/expense/internal/repository/immudb_bench_test.go new file mode 100644 index 00000000..7bfa0d8b --- /dev/null +++ b/services/expense/internal/repository/immudb_bench_test.go @@ -0,0 +1,72 @@ +package repository + +import ( + "context" + "fmt" + "io" + "log/slog" + "testing" +) + +const benchUser = "bench-user" + +func newBenchRepo(client ImmudbClient) *ImmudbExpenseRepository { + return NewImmudbExpenseRepository(client, slog.New(slog.NewJSONHandler(io.Discard, nil))) +} + +// exportViaKeyset walks the full export using the NEW keyset cursor path +// (GetExpensesByUserAfter): no OFFSET, no per-page COUNT. +func exportViaKeyset(b *testing.B, repo *ImmudbExpenseRepository, pageSize int32) { + b.Helper() + cursor := ExpenseCursor{} + for { + _, next, hasMore, err := repo.GetExpensesByUserAfter(context.Background(), benchUser, cursor, pageSize) + if err != nil { + b.Fatalf("keyset export failed: %v", err) + } + if !hasMore { + return + } + cursor = next + } +} + +// BenchmarkExportExpenseRead characterizes the keyset export path +// (GetExpensesByUserAfter) at increasing page counts P. Alongside wall-clock +// ns/op (a same-machine reference only), it reports the portable structural +// signals: +// - queries/export: total queries issued for a full export +// - counts/export: COUNT(*) queries issued for a full export +// +// Keyset issues P queries (one data query per page, zero COUNT) and never +// rescans (O(P) rows scanned), versus the removed OFFSET path's 2*P queries and +// O(P^2) rows scanned (recorded in perf/baseline/read.txt). The rows-scanned +// shape is an execution property of the real database; it is analytical (immudb +// 1.11.0 exposes no EXPLAIN to assert it directly). The integration test +// confirms keyset ordering and the absence of OFFSET against real immudb; the +// recording mock reproduces query shape/count, not scan cost. +func BenchmarkExportExpenseRead(b *testing.B) { + const pageSize = int32(50) + for _, pages := range []int{1, 10, 50, 100} { + rows := seedExportRows(benchUser, pages*int(pageSize)) + + b.Run(fmt.Sprintf("Keyset/P=%d", pages), func(b *testing.B) { + probe := newRecordingImmudbClient(rows...) + exportViaKeyset(b, newBenchRepo(probe), pageSize) + queries := float64(len(probe.Queries())) + counts := float64(probe.countQueriesContaining("COUNT(*)")) + + b.ReportAllocs() + for b.Loop() { + // A fresh client per iteration starts each run from an empty query + // log; bracket that setup out so only the keyset walk is timed. + b.StopTimer() + repo := newBenchRepo(newRecordingImmudbClient(rows...)) + b.StartTimer() + exportViaKeyset(b, repo, pageSize) + } + b.ReportMetric(queries, "queries/export") + b.ReportMetric(counts, "counts/export") + }) + } +} diff --git a/services/expense/internal/repository/keyset_integration_test.go b/services/expense/internal/repository/keyset_integration_test.go new file mode 100644 index 00000000..4c889943 --- /dev/null +++ b/services/expense/internal/repository/keyset_integration_test.go @@ -0,0 +1,201 @@ +//go:build integration + +// Integration test for the keyset export path against a real immudb container. +// +// It confirms the unit-level query-shape assertions against the pinned +// production immudb version (codenotary/immudb:1.11.0): that the expanded-OR +// (created_at, id) predicate and multi-column `ORDER BY created_at ASC, id ASC` +// actually work on that version, that keyset ordering is correct across a +// shared-created_at boundary, and that no query uses OFFSET. +// +// Run with a live immudb: +// +// docker run -d --rm -p 3322:3322 codenotary/immudb:1.11.0 +// go test -tags integration -run Integration ./internal/repository/... +// +// Set TEST_IMMUDB_ADDR (default localhost:3322) to point elsewhere. The test +// skips if immudb is unreachable. +package repository + +import ( + "context" + "io" + "log/slog" + "os" + "strconv" + "strings" + "sync" + "testing" + "time" + + immudb "github.com/codenotary/immudb/pkg/client" + + "github.com/ItsThompson/gofin/services/expense/internal/model" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordingRealClient wraps the immudb SDK client as a repository.ImmudbClient +// and records issued SQL, so the integration test can assert query shape (no +// OFFSET, no COUNT) on the statements actually sent to real immudb. +type recordingRealClient struct { + client immudb.ImmuClient + mu sync.Mutex + queries []string +} + +func (c *recordingRealClient) record(sql string) { + c.mu.Lock() + defer c.mu.Unlock() + c.queries = append(c.queries, sql) +} + +func (c *recordingRealClient) recordedQueries() []string { + c.mu.Lock() + defer c.mu.Unlock() + out := make([]string, len(c.queries)) + copy(out, c.queries) + return out +} + +func (c *recordingRealClient) SQLExec(ctx context.Context, sql string, params map[string]interface{}) (*SQLResult, error) { + c.record(sql) + _, err := c.client.SQLExec(ctx, sql, params) + if err != nil { + return nil, err + } + return &SQLResult{}, nil +} + +func (c *recordingRealClient) SQLQuery(ctx context.Context, sql string, params map[string]interface{}) (*SQLResult, error) { + c.record(sql) + res, err := c.client.SQLQuery(ctx, sql, params, true) //nolint:staticcheck // SA1019: mirrors the production client (cmd/immudb_prod.go); SQLQueryReader is a deferred optional refinement, not required for the O(pageSize) bound. + if err != nil { + return nil, err + } + rows := make([]SQLRow, len(res.Rows)) + for i, row := range res.Rows { + values := make([]SQLValue, len(row.Values)) + for j, val := range row.Values { + values[j] = realSQLValue{val: val.GetS(), num: val.GetN(), b: val.GetB()} + } + rows[i] = SQLRow{Values: values} + } + return &SQLResult{Rows: rows}, nil +} + +type realSQLValue struct { + val string + num int64 + b bool +} + +func (v realSQLValue) GetString() string { return v.val } +func (v realSQLValue) GetInt() int64 { return v.num } +func (v realSQLValue) GetBool() bool { return v.b } + +func connectRealImmudb(t *testing.T) *recordingRealClient { + t.Helper() + addr := os.Getenv("TEST_IMMUDB_ADDR") + if addr == "" { + addr = "localhost:3322" + } + host := addr + port := 3322 + if parts := strings.SplitN(addr, ":", 2); len(parts) == 2 { + host = parts[0] + p, err := strconv.Atoi(parts[1]) + require.NoError(t, err) + port = p + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + opts := immudb.DefaultOptions().WithAddress(host).WithPort(port) + client := immudb.NewClient().WithOptions(opts) + if err := client.OpenSession(ctx, []byte("immudb"), []byte("immudb"), "defaultdb"); err != nil { + t.Skipf("immudb not reachable at %s (%v); start it with `docker run -d --rm -p 3322:3322 codenotary/immudb:1.11.0`", addr, err) + } + t.Cleanup(func() { _ = client.CloseSession(context.Background()) }) + return &recordingRealClient{client: client} +} + +func TestGetExpensesByUserAfter_Integration_KeysetOrderingAndNoOffset(t *testing.T) { + client := connectRealImmudb(t) + logger := slog.New(slog.NewJSONHandler(io.Discard, nil)) + repo := NewImmudbExpenseRepository(client, logger) + ctx := context.Background() + + require.NoError(t, repo.InitSchema(ctx)) + + // Unique user + id prefix so repeated runs against the persistent immudb + // volume don't collide (id is the primary key). + runID := strconv.FormatInt(time.Now().UnixNano(), 10) + userID := "itest-" + runID + mkID := func(s string) string { return runID + "-" + s } + + const ( + t1 = "2026-05-01T00:00:01Z" + t2 = "2026-05-01T00:00:02Z" + t3 = "2026-05-01T00:00:03Z" + ) + // Insert out of order, with two shared-created_at groups (t1 and t3). + seed := []*model.Expense{ + buildTestExpense(mkID("id-2"), userID, t1), + buildTestExpense(mkID("id-1"), userID, t1), + buildTestExpense(mkID("id-3"), userID, t2), + buildTestExpense(mkID("id-5"), userID, t3), + buildTestExpense(mkID("id-4"), userID, t3), + } + for _, e := range seed { + _, err := repo.CreateExpense(ctx, e) + require.NoError(t, err) + } + + // Walk the full export with a small page size so pages straddle the shared + // created_at boundaries. + var got []ExpenseCursor + cursor := ExpenseCursor{} + pages := 0 + for { + rows, next, hasMore, err := repo.GetExpensesByUserAfter(ctx, userID, cursor, 2) + require.NoError(t, err) + pages++ + for _, r := range rows { + got = append(got, ExpenseCursor{CreatedAt: r.CreatedAt, ID: r.ID}) + } + if !hasMore { + break + } + cursor = next + require.LessOrEqual(t, pages, 100, "keyset walk did not terminate") + } + + // Chronological (created_at ASC) with id tiebreaker, no duplicates, no skips + // across the shared-created_at boundaries. + want := []ExpenseCursor{ + {CreatedAt: t1, ID: mkID("id-1")}, + {CreatedAt: t1, ID: mkID("id-2")}, + {CreatedAt: t2, ID: mkID("id-3")}, + {CreatedAt: t3, ID: mkID("id-4")}, + {CreatedAt: t3, ID: mkID("id-5")}, + } + assert.Equal(t, want, got) + + // Confirm the unit query-shape assertions hold against real immudb: the + // expanded-OR keyset predicate is used, and no data query uses OFFSET or a + // per-page COUNT. + sawKeysetPredicate := false + for _, q := range client.recordedQueries() { + upper := strings.ToUpper(q) + if strings.Contains(upper, "SELECT") && strings.Contains(upper, "FROM EXPENSES") && + !strings.Contains(upper, "CREATE") { + assert.NotContains(t, upper, "OFFSET", "keyset query must not use OFFSET: %s", q) + } + if strings.Contains(q, "created_at = @cursor_created_at AND id > @cursor_id") { + sawKeysetPredicate = true + } + } + assert.True(t, sawKeysetPredicate, "expected the expanded-OR keyset predicate to be issued") +} diff --git a/services/expense/internal/repository/keyset_test.go b/services/expense/internal/repository/keyset_test.go new file mode 100644 index 00000000..df63c0b3 --- /dev/null +++ b/services/expense/internal/repository/keyset_test.go @@ -0,0 +1,201 @@ +package repository + +import ( + "context" + "fmt" + "io" + "log/slog" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ItsThompson/gofin/services/expense/internal/model" +) + +func newKeysetTestRepo(rows ...*model.Expense) (*ImmudbExpenseRepository, *recordingImmudbClient) { + client := newRecordingImmudbClient(rows...) + logger := slog.New(slog.NewJSONHandler(io.Discard, nil)) + return NewImmudbExpenseRepository(client, logger), client +} + +// seedExportRows builds n canonical-UTC rows for a user, created one second +// apart so (created_at, id) ordering is unambiguous. +func seedExportRows(userID string, n int) []*model.Expense { + rows := make([]*model.Expense, 0, n) + for i := 0; i < n; i++ { + id := fmt.Sprintf("exp-%04d", i) + createdAt := fmt.Sprintf("2026-05-01T00:%02d:%02dZ", i/60, i%60) + rows = append(rows, buildTestExpense(id, userID, createdAt)) + } + return rows +} + +// walkFullExport pages through GetExpensesByUserAfter until hasMore is false and +// returns every row in order plus the number of repo calls (== pages). +func walkFullExport(t *testing.T, repo *ImmudbExpenseRepository, userID string, pageSize int32) ([]*model.Expense, int) { + t.Helper() + var all []*model.Expense + cursor := ExpenseCursor{} + calls := 0 + for { + rows, next, hasMore, err := repo.GetExpensesByUserAfter(context.Background(), userID, cursor, pageSize) + require.NoError(t, err) + calls++ + all = append(all, rows...) + if !hasMore { + break + } + require.NotEqual(t, cursor, next, "cursor must advance between pages") + cursor = next + require.LessOrEqual(t, calls, 10_000, "keyset walk did not terminate") + } + return all, calls +} + +func TestGetExpensesByUserAfter_UsesKeysetPredicateNotOffset(t *testing.T) { + repo, client := newKeysetTestRepo(seedExportRows("user-1", 5)...) + + _, _, _, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", ExpenseCursor{CreatedAt: "2026-05-01T00:00:01Z", ID: "exp-0001"}, 2) + + require.NoError(t, err) + queries := client.Queries() + require.Len(t, queries, 1) + sql := queries[0].SQL + assert.NotContains(t, strings.ToUpper(sql), "OFFSET") + assert.NotContains(t, strings.ToUpper(sql), "COUNT(*)") + assert.Contains(t, sql, "created_at > @cursor_created_at") + assert.Contains(t, sql, "created_at = @cursor_created_at AND id > @cursor_id") + assert.Contains(t, sql, "ORDER BY created_at ASC, id ASC") + assert.Contains(t, sql, "LIMIT @limit") + // pageSize+1 rows are fetched to derive hasMore without a COUNT. + assert.Equal(t, int32(3), queries[0].Params["limit"]) + assert.Equal(t, "2026-05-01T00:00:01Z", queries[0].Params["cursor_created_at"]) + assert.Equal(t, "exp-0001", queries[0].Params["cursor_id"]) +} + +func TestGetExpensesByUserAfter_FirstPageOmitsCursorPredicate(t *testing.T) { + repo, client := newKeysetTestRepo(seedExportRows("user-1", 3)...) + + _, _, _, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", ExpenseCursor{}, 2) + + require.NoError(t, err) + sql := client.Queries()[0].SQL + assert.NotContains(t, sql, "@cursor_created_at") + assert.NotContains(t, client.Queries()[0].Params, "cursor_created_at") + assert.Contains(t, sql, "ORDER BY created_at ASC, id ASC") +} + +func TestGetExpensesByUserAfter_DerivesHasMoreFromOverflowRow(t *testing.T) { + repo, _ := newKeysetTestRepo(seedExportRows("user-1", 3)...) + + // First page: 2 of 3 rows, more remain. + page1, next1, hasMore1, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", ExpenseCursor{}, 2) + require.NoError(t, err) + require.Len(t, page1, 2) + assert.True(t, hasMore1) + assert.Equal(t, "exp-0001", next1.ID) + assert.Equal(t, page1[1].CreatedAt, next1.CreatedAt) + + // Second page: the final row, no overflow, hasMore false. + page2, _, hasMore2, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", next1, 2) + require.NoError(t, err) + require.Len(t, page2, 1) + assert.False(t, hasMore2) + assert.Equal(t, "exp-0002", page2[0].ID) +} + +func TestGetExpensesByUserAfter_ExactPageBoundaryReportsNoMore(t *testing.T) { + // Exactly pageSize rows: the pageSize+1 fetch returns no overflow row. + repo, _ := newKeysetTestRepo(seedExportRows("user-1", 2)...) + + rows, _, hasMore, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", ExpenseCursor{}, 2) + + require.NoError(t, err) + require.Len(t, rows, 2) + assert.False(t, hasMore) +} + +func TestGetExpensesByUserAfter_OrdersBySharedCreatedAtWithIDTiebreaker(t *testing.T) { + // Three rows share a created_at; the id tiebreaker must give a stable, + // total order and a correct keyset boundary across the shared second. + const sharedTime = "2026-05-01T00:00:00Z" + repo, _ := newKeysetTestRepo( + buildTestExpense("exp-c", "user-1", sharedTime), + buildTestExpense("exp-a", "user-1", sharedTime), + buildTestExpense("exp-b", "user-1", sharedTime), + ) + + page1, next1, hasMore1, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", ExpenseCursor{}, 2) + require.NoError(t, err) + require.Len(t, page1, 2) + assert.True(t, hasMore1) + assert.Equal(t, "exp-a", page1[0].ID) + assert.Equal(t, "exp-b", page1[1].ID) + assert.Equal(t, ExpenseCursor{CreatedAt: sharedTime, ID: "exp-b"}, next1) + + // Seeking past (sharedTime, exp-b) must return only exp-c, not re-emit + // exp-a/exp-b that share the same created_at. + page2, _, hasMore2, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", next1, 2) + require.NoError(t, err) + require.Len(t, page2, 1) + assert.False(t, hasMore2) + assert.Equal(t, "exp-c", page2[0].ID) +} + +func TestGetExpensesByUserAfter_EmptyResultCleanTermination(t *testing.T) { + repo, client := newKeysetTestRepo() + + rows, next, hasMore, err := repo.GetExpensesByUserAfter(context.Background(), "user-empty", ExpenseCursor{}, 50) + + require.NoError(t, err) + assert.Empty(t, rows) + assert.False(t, hasMore) + assert.Equal(t, ExpenseCursor{}, next) + assert.Zero(t, client.countQueriesContaining("COUNT(*)")) + assert.Zero(t, client.countQueriesContaining("OFFSET")) +} + +func TestGetExpensesByUserAfter_DefaultsPageSizeWhenNonPositive(t *testing.T) { + repo, client := newKeysetTestRepo(seedExportRows("user-1", 1)...) + + _, _, _, err := repo.GetExpensesByUserAfter(context.Background(), "user-1", ExpenseCursor{}, 0) + + require.NoError(t, err) + // Default page size + 1 overflow row. + assert.Equal(t, DefaultStreamPageSize+1, client.Queries()[0].Params["limit"]) +} + +func TestGetExpensesByUserAfter_FullExportNoOffsetNoPerPageCount(t *testing.T) { + const pageSize = int32(10) + repo, client := newKeysetTestRepo(seedExportRows("user-1", 55)...) + + rows, calls := walkFullExport(t, repo, "user-1", pageSize) + + require.Len(t, rows, 55) + // 55 rows / 10 per page = 6 pages (5 full + 1 partial). + assert.Equal(t, 6, calls) + assert.Zero(t, client.countQueriesContaining("OFFSET"), "keyset export must never use OFFSET") + assert.Zero(t, client.countQueriesContaining("COUNT(*)"), "keyset export must never issue COUNT(*)") + // Exactly one data query per page, no extra scans. + assert.Equal(t, calls, len(client.Queries())) +} + +func TestGetExpensesByUserAfter_QueryCountScalesLinearly(t *testing.T) { + const pageSize = int32(10) + for _, pages := range []int{1, 10, 50, 100} { + t.Run(fmt.Sprintf("P=%d", pages), func(t *testing.T) { + repo, client := newKeysetTestRepo(seedExportRows("user-1", pages*int(pageSize))...) + + rows, calls := walkFullExport(t, repo, "user-1", pageSize) + + require.Len(t, rows, pages*int(pageSize)) + // One data query per page: total queries grow linearly (O(P)), never + // quadratically, and no page re-scans a prior page (no OFFSET). + assert.Equal(t, pages, calls) + assert.Equal(t, pages, len(client.Queries())) + assert.Zero(t, client.countQueriesContaining("OFFSET")) + }) + } +} diff --git a/services/expense/internal/repository/recording_client_test.go b/services/expense/internal/repository/recording_client_test.go new file mode 100644 index 00000000..2a6af60e --- /dev/null +++ b/services/expense/internal/repository/recording_client_test.go @@ -0,0 +1,187 @@ +package repository + +import ( + "context" + "sort" + "strings" + "sync" + + "github.com/ItsThompson/gofin/services/expense/internal/model" +) + +// recordedQuery captures a single SQL statement issued through the client, so +// tests can assert query shape (no OFFSET, at most one COUNT, one data query +// per page) without a real database. +type recordedQuery struct { + SQL string + Params map[string]interface{} +} + +// recordingImmudbClient is a faithful in-memory simulation of the immudb SQL +// surface used by the expense repository. It records every query and answers +// COUNT(*) and keyset (expanded-OR cursor) data queries over seeded rows, so the +// same client backs the keyset unit tests and the export benchmark. It is safe +// for concurrent use. +type recordingImmudbClient struct { + mu sync.Mutex + rows []*model.Expense + queries []recordedQuery +} + +func newRecordingImmudbClient(rows ...*model.Expense) *recordingImmudbClient { + return &recordingImmudbClient{rows: rows} +} + +func (c *recordingImmudbClient) record(sql string, params map[string]interface{}) { + copied := make(map[string]interface{}, len(params)) + for k, v := range params { + copied[k] = v + } + c.mu.Lock() + defer c.mu.Unlock() + c.queries = append(c.queries, recordedQuery{SQL: sql, Params: copied}) +} + +// Queries returns a copy of every query recorded so far. +func (c *recordingImmudbClient) Queries() []recordedQuery { + c.mu.Lock() + defer c.mu.Unlock() + out := make([]recordedQuery, len(c.queries)) + copy(out, c.queries) + return out +} + +// countQueriesContaining returns how many recorded queries contain substr +// (case-insensitive). +func (c *recordingImmudbClient) countQueriesContaining(substr string) int { + upper := strings.ToUpper(substr) + n := 0 + for _, q := range c.Queries() { + if strings.Contains(strings.ToUpper(q.SQL), upper) { + n++ + } + } + return n +} + +func (c *recordingImmudbClient) SQLExec(_ context.Context, sql string, params map[string]interface{}) (*SQLResult, error) { + c.record(sql, params) + return &SQLResult{}, nil +} + +func (c *recordingImmudbClient) SQLQuery(ctx context.Context, sql string, params map[string]interface{}) (*SQLResult, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + c.record(sql, params) + + userID, _ := params["user_id"].(string) + + if strings.Contains(strings.ToUpper(sql), "COUNT(*)") { + var count int64 + for _, row := range c.rows { + if row.UserID == userID { + count++ + } + } + return &SQLResult{Rows: []SQLRow{{Values: []SQLValue{fakeSQLValue{intValue: count}}}}}, nil + } + + // Data query: filter to the user, order by (created_at ASC, id ASC). + matched := make([]*model.Expense, 0, len(c.rows)) + for _, row := range c.rows { + if row.UserID == userID { + matched = append(matched, row) + } + } + sort.SliceStable(matched, func(i, j int) bool { + if matched[i].CreatedAt != matched[j].CreatedAt { + return matched[i].CreatedAt < matched[j].CreatedAt + } + return matched[i].ID < matched[j].ID + }) + + // Keyset seek: expanded-OR (created_at, id) cursor predicate. + if cursorCreatedAt, ok := params["cursor_created_at"].(string); ok { + cursorID, _ := params["cursor_id"].(string) + seeked := matched[:0:0] + for _, row := range matched { + if row.CreatedAt > cursorCreatedAt || + (row.CreatedAt == cursorCreatedAt && row.ID > cursorID) { + seeked = append(seeked, row) + } + } + matched = seeked + } + + page := matched + + // LIMIT. + if limit, ok := params["limit"]; ok { + if l := int(toInt64(limit)); l >= 0 && l < len(page) { + page = page[:l] + } + } + + return expensesToSQLResult(page), nil +} + +func toInt64(v interface{}) int64 { + switch val := v.(type) { + case int64: + return val + case int32: + return int64(val) + case int: + return int64(val) + default: + return 0 + } +} + +// expensesToSQLResult renders expenses as SQL rows in the column order the +// repository's SELECT clauses (expenseSelectColumns) expect. +func expensesToSQLResult(expenses []*model.Expense) *SQLResult { + rows := make([]SQLRow, 0, len(expenses)) + for _, e := range expenses { + rows = append(rows, SQLRow{Values: []SQLValue{ + fakeSQLValue{stringValue: e.ID}, + fakeSQLValue{stringValue: e.UserID}, + fakeSQLValue{stringValue: e.Name}, + fakeSQLValue{intValue: e.Amount}, + fakeSQLValue{stringValue: e.Currency}, + fakeSQLValue{stringValue: e.ExpenseType}, + fakeSQLValue{stringValue: e.TagID}, + fakeSQLValue{stringValue: e.ExpenseDate}, + fakeSQLValue{intValue: int64(e.PeriodYear)}, + fakeSQLValue{intValue: int64(e.PeriodMonth)}, + fakeSQLValue{stringValue: e.Status}, + fakeSQLValue{stringValue: e.CorrectsID}, + fakeSQLValue{boolValue: e.IsProRata}, + fakeSQLValue{stringValue: e.ProRataGroup}, + fakeSQLValue{intValue: int64(e.ProRataIndex)}, + fakeSQLValue{intValue: int64(e.ProRataTotal)}, + fakeSQLValue{stringValue: e.CreatedAt}, + }}) + } + return &SQLResult{Rows: rows} +} + +// buildTestExpense constructs an expense row for repository tests, defaulting +// all required fields and applying overrides. +func buildTestExpense(id, userID, createdAt string) *model.Expense { + return &model.Expense{ + ID: id, + UserID: userID, + Name: "Expense " + id, + Amount: 1000, + Currency: "USD", + ExpenseType: "essentials", + TagID: "tag-1", + ExpenseDate: "2026-05-01", + PeriodYear: 2026, + PeriodMonth: 5, + Status: "active", + CreatedAt: createdAt, + } +} diff --git a/services/expense/internal/repository/repository.go b/services/expense/internal/repository/repository.go index 8efb9009..847d0306 100644 --- a/services/expense/internal/repository/repository.go +++ b/services/expense/internal/repository/repository.go @@ -39,10 +39,15 @@ type ExpenseRepository interface { // GetActiveExpenseSuggestionInputs returns active expense rows for suggestion ranking. GetActiveExpenseSuggestionInputs(ctx context.Context, userID string) ([]*model.ExpenseSuggestionInput, error) - // GetAllExpensesByUser returns all expenses (active + corrected) for a user, - // ordered by created_at ASC, with LIMIT/OFFSET pagination. - // Used by the datarights service for data export (GDPR compliance). - GetAllExpensesByUser(ctx context.Context, userID string, page, pageSize int32) ([]*model.Expense, int64, error) + // GetExpensesByUserAfter returns one keyset page of expenses (active + + // corrected) for a user past the given cursor, ordered by + // (created_at ASC, id ASC). It seeks with a (created_at, id) cursor instead + // of LIMIT/OFFSET and derives hasMore by fetching pageSize+1 rows, so it + // issues no OFFSET and no per-page COUNT(*). An empty cursor starts from the + // beginning. next is the cursor for the following page (the last returned + // row); hasMore reports whether more rows remain. Consumed by the + // StreamAllUserExpenses RPC for O(page_size)-memory export. + GetExpensesByUserAfter(ctx context.Context, userID string, cursor ExpenseCursor, pageSize int32) (rows []*model.Expense, next ExpenseCursor, hasMore bool, err error) // AnonymizeAllUserExpenses redacts PII fields on all expense rows for a user. // immudb is append-only: the UPDATE overwrites the current head while history @@ -56,6 +61,19 @@ type SchemaInitializer interface { InitSchema(ctx context.Context) error } +// ExpenseCursor identifies the last row seen during a keyset walk, used to seek +// the next page without OFFSET. CreatedAt holds an RFC3339 timestamp in +// canonical fixed-precision UTC form (lexicographically sortable); an empty +// CreatedAt means "start from the beginning". +type ExpenseCursor struct { + CreatedAt string + ID string +} + +// DefaultStreamPageSize is the keyset page size applied when a caller passes a +// non-positive page size (e.g. StreamAllUserExpensesRequest.page_size == 0). +const DefaultStreamPageSize int32 = 100 + // SQLResult represents the result of an SQL query row. type SQLResult struct { Columns []string diff --git a/services/expense/internal/service/expense.go b/services/expense/internal/service/expense.go index 2cf67a45..1e2cf59b 100644 --- a/services/expense/internal/service/expense.go +++ b/services/expense/internal/service/expense.go @@ -317,38 +317,86 @@ func (s *ExpenseService) GetProRataGroup(ctx context.Context, userID string, gro return expenses, nil } -// GetAllUserExpenses returns all expenses (active + corrected) for a user, -// with pagination. Used by the datarights service for GDPR data export. -func (s *ExpenseService) GetAllUserExpenses(ctx context.Context, userID string, page, pageSize int32) (*model.ExpenseListResponse, error) { +// StreamAllUserExpenses walks the full expense history (active + corrected) for +// a user with keyset pagination and invokes send for each row in chronological +// order (created_at ASC, id ASC), bounding memory to O(pageSize). It backs the +// StreamAllUserExpenses server-streaming RPC. +// +// A single producer goroutine pages the keyset cursor and feeds a rows channel +// buffered to pageSize; this method (the sole stream owner) consumes it and +// calls send. Buffering to one page lets the producer fetch page N+1 while the +// consumer is still sending page N, while keeping retained memory O(pageSize). +// The producer reports its terminal status on a buffered (cap 1) error channel +// and selects on context cancellation for every hand-off, so a send error, a +// client disconnect, or a job timeout stops the walk promptly without leaking +// the goroutine. +func (s *ExpenseService) StreamAllUserExpenses(ctx context.Context, userID string, pageSize int32, send func(*model.Expense) error) error { if userID == "" { - return nil, &ServiceError{ + return &ServiceError{ Code: model.ErrValidationError, Message: "user_id is required", Status: 400, } } - - if page < 1 { - page = 1 - } - if pageSize < 1 || pageSize > 100 { - pageSize = 50 + if pageSize < 1 { + pageSize = repository.DefaultStreamPageSize } - expenses, total, err := s.repo.GetAllExpensesByUser(ctx, userID, page, pageSize) - if err != nil { - return nil, fmt.Errorf("getting all user expenses: %w", err) - } + // Derive a cancellable context so returning for any reason (send error, + // cancellation, clean EOF) unblocks the producer goroutine. + ctx, cancel := context.WithCancel(ctx) + defer cancel() - hasMore := int64(page)*int64(pageSize) < total + rows := make(chan *model.Expense, pageSize) // buffered to one page: bounds retained memory to O(pageSize) + errc := make(chan error, 1) // cap 1: producer reports terminal status without blocking - return &model.ExpenseListResponse{ - Data: expenses, - Total: total, - Page: page, - PageSize: pageSize, - HasMore: hasMore, - }, nil + go s.produceExpensePages(ctx, userID, pageSize, rows, errc) + + for expense := range rows { + if err := ctx.Err(); err != nil { + return err + } + if err := send(expense); err != nil { + return err + } + } + return <-errc +} + +// produceExpensePages is the single owner and closer of rows. It pages the +// keyset cursor, feeds each row to rows (selecting on ctx.Done() so +// cancellation stops it promptly), and reports its terminal status (nil on a +// clean end-of-history) on errc before returning. +// +// It holds at most one page at a time (the current page slice plus the +// pageSize-buffered rows channel), so producer-side memory stays O(pageSize) +// regardless of total history size. The end-to-end retained-heap bound is +// verified by the datarights streaming consumer growth-ratio test in +// services/datarights/internal/engine/providers/expenses_stream_test.go. +func (s *ExpenseService) produceExpensePages(ctx context.Context, userID string, pageSize int32, rows chan<- *model.Expense, errc chan<- error) { + defer close(rows) + + cursor := repository.ExpenseCursor{} + for { + page, next, hasMore, err := s.repo.GetExpensesByUserAfter(ctx, userID, cursor, pageSize) + if err != nil { + errc <- fmt.Errorf("streaming user expenses: %w", err) + return + } + for _, expense := range page { + select { + case rows <- expense: + case <-ctx.Done(): + errc <- ctx.Err() + return + } + } + if !hasMore { + errc <- nil + return + } + cursor = next + } } // AnonymizeAllUserExpenses redacts PII fields on all expense rows for a user. diff --git a/services/expense/internal/service/expense_test.go b/services/expense/internal/service/expense_test.go index 28d5e9f4..bb7122a0 100644 --- a/services/expense/internal/service/expense_test.go +++ b/services/expense/internal/service/expense_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ItsThompson/gofin/services/expense/internal/model" + "github.com/ItsThompson/gofin/services/expense/internal/repository" ) // mockExpenseRepository implements repository.ExpenseRepository for tests. @@ -81,19 +82,20 @@ func (m *mockExpenseRepository) GetActiveExpenseSuggestionInputs(ctx context.Con return args.Get(0).([]*model.ExpenseSuggestionInput), args.Error(1) } -func (m *mockExpenseRepository) GetAllExpensesByUser(ctx context.Context, userID string, page, pageSize int32) ([]*model.Expense, int64, error) { - args := m.Called(ctx, userID, page, pageSize) - if args.Get(0) == nil { - return nil, args.Get(1).(int64), args.Error(2) - } - return args.Get(0).([]*model.Expense), args.Get(1).(int64), args.Error(2) -} - func (m *mockExpenseRepository) AnonymizeAllUserExpenses(ctx context.Context, userID string) error { args := m.Called(ctx, userID) return args.Error(0) } +func (m *mockExpenseRepository) GetExpensesByUserAfter(ctx context.Context, userID string, cursor repository.ExpenseCursor, pageSize int32) ([]*model.Expense, repository.ExpenseCursor, bool, error) { + args := m.Called(ctx, userID, cursor, pageSize) + var rows []*model.Expense + if args.Get(0) != nil { + rows = args.Get(0).([]*model.Expense) + } + return rows, args.Get(1).(repository.ExpenseCursor), args.Bool(2), args.Error(3) +} + func newTestService(repo *mockExpenseRepository) *ExpenseService { logger := slog.New(slog.NewJSONHandler(io.Discard, nil)) return NewExpenseService(repo, logger) @@ -696,135 +698,6 @@ func TestGetProRataGroup_EmptyGroupID(t *testing.T) { assert.Equal(t, model.ErrValidationError, svcErr.Code) } -// --- GetAllUserExpenses tests --- - -func TestGetAllUserExpenses_Success(t *testing.T) { - repo := new(mockExpenseRepository) - svc := newTestService(repo) - - expenses := []*model.Expense{ - {ID: "exp-1", UserID: "user-1", Name: "Groceries", Amount: 5000, Status: "active", CreatedAt: "2026-01-01T10:00:00Z"}, - {ID: "exp-2", UserID: "user-1", Name: "Coffee", Amount: 500, Status: "corrected", CreatedAt: "2026-01-02T10:00:00Z"}, - {ID: "exp-3", UserID: "user-1", Name: "Coffee (corrected)", Amount: 600, Status: "active", CorrectsID: "exp-2", CreatedAt: "2026-01-02T11:00:00Z"}, - } - - repo.On("GetAllExpensesByUser", mock.Anything, "user-1", int32(1), int32(50)). - Return(expenses, int64(3), nil) - - result, err := svc.GetAllUserExpenses(context.Background(), "user-1", 1, 50) - - require.NoError(t, err) - assert.Equal(t, int64(3), result.Total) - assert.Len(t, result.Data, 3) - assert.Equal(t, int32(1), result.Page) - assert.Equal(t, int32(50), result.PageSize) - assert.False(t, result.HasMore) - // Verify both active and corrected statuses are included - assert.Equal(t, "active", result.Data[0].Status) - assert.Equal(t, "corrected", result.Data[1].Status) - assert.Equal(t, "active", result.Data[2].Status) -} - -func TestGetAllUserExpenses_Pagination(t *testing.T) { - repo := new(mockExpenseRepository) - svc := newTestService(repo) - - // Page 1 of 3 (page_size=2, total=5) - page1 := []*model.Expense{ - {ID: "exp-1", UserID: "user-1", Status: "active"}, - {ID: "exp-2", UserID: "user-1", Status: "corrected"}, - } - - repo.On("GetAllExpensesByUser", mock.Anything, "user-1", int32(1), int32(2)). - Return(page1, int64(5), nil) - - result, err := svc.GetAllUserExpenses(context.Background(), "user-1", 1, 2) - - require.NoError(t, err) - assert.Equal(t, int64(5), result.Total) - assert.Len(t, result.Data, 2) - assert.Equal(t, int32(1), result.Page) - assert.Equal(t, int32(2), result.PageSize) - assert.True(t, result.HasMore) -} - -func TestGetAllUserExpenses_MultiPageTraversal(t *testing.T) { - repo := new(mockExpenseRepository) - svc := newTestService(repo) - - // Last page: page 3 with page_size=2, total=5 - page3 := []*model.Expense{ - {ID: "exp-5", UserID: "user-1", Status: "active"}, - } - - repo.On("GetAllExpensesByUser", mock.Anything, "user-1", int32(3), int32(2)). - Return(page3, int64(5), nil) - - result, err := svc.GetAllUserExpenses(context.Background(), "user-1", 3, 2) - - require.NoError(t, err) - assert.Equal(t, int64(5), result.Total) - assert.Len(t, result.Data, 1) - assert.Equal(t, int32(3), result.Page) - assert.False(t, result.HasMore) // 3*2=6 >= 5 -} - -func TestGetAllUserExpenses_EmptyResults(t *testing.T) { - repo := new(mockExpenseRepository) - svc := newTestService(repo) - - repo.On("GetAllExpensesByUser", mock.Anything, "user-1", int32(1), int32(50)). - Return([]*model.Expense{}, int64(0), nil) - - result, err := svc.GetAllUserExpenses(context.Background(), "user-1", 1, 50) - - require.NoError(t, err) - assert.Equal(t, int64(0), result.Total) - assert.Empty(t, result.Data) - assert.False(t, result.HasMore) -} - -func TestGetAllUserExpenses_EmptyUserID(t *testing.T) { - repo := new(mockExpenseRepository) - svc := newTestService(repo) - - _, err := svc.GetAllUserExpenses(context.Background(), "", 1, 50) - - require.Error(t, err) - svcErr, ok := err.(*ServiceError) - require.True(t, ok) - assert.Equal(t, model.ErrValidationError, svcErr.Code) -} - -func TestGetAllUserExpenses_DefaultsPagination(t *testing.T) { - repo := new(mockExpenseRepository) - svc := newTestService(repo) - - repo.On("GetAllExpensesByUser", mock.Anything, "user-1", int32(1), int32(50)). - Return([]*model.Expense{}, int64(0), nil) - - // page=0 and pageSize=0 should be defaulted to 1 and 50 - result, err := svc.GetAllUserExpenses(context.Background(), "user-1", 0, 0) - - require.NoError(t, err) - assert.Equal(t, int32(1), result.Page) - assert.Equal(t, int32(50), result.PageSize) -} - -func TestGetAllUserExpenses_PageSizeCapped(t *testing.T) { - repo := new(mockExpenseRepository) - svc := newTestService(repo) - - repo.On("GetAllExpensesByUser", mock.Anything, "user-1", int32(1), int32(50)). - Return([]*model.Expense{}, int64(0), nil) - - // pageSize > 100 should be capped to 50 - result, err := svc.GetAllUserExpenses(context.Background(), "user-1", 1, 200) - - require.NoError(t, err) - assert.Equal(t, int32(50), result.PageSize) -} - // --- AnonymizeAllUserExpenses tests --- func TestAnonymizeAllUserExpenses_Success(t *testing.T) { diff --git a/services/expense/internal/service/stream_test.go b/services/expense/internal/service/stream_test.go new file mode 100644 index 00000000..bbf6a896 --- /dev/null +++ b/services/expense/internal/service/stream_test.go @@ -0,0 +1,260 @@ +package service + +import ( + "context" + "errors" + "fmt" + "io" + "log/slog" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/ItsThompson/gofin/services/expense/internal/model" + "github.com/ItsThompson/gofin/services/expense/internal/repository" +) + +func streamExpense(id, createdAt string) *model.Expense { + return &model.Expense{ + ID: id, + UserID: "user-1", + Name: "Expense " + id, + Amount: 1000, + Currency: "USD", + ExpenseType: "essentials", + TagID: "tag-1", + ExpenseDate: "2026-05-01", + PeriodYear: 2026, + PeriodMonth: 5, + Status: "active", + CreatedAt: createdAt, + } +} + +// collectStream runs StreamAllUserExpenses on a fresh goroutine with a guard +// timeout so a producer that fails to stop surfaces as a test failure rather +// than a hang. sink is invoked for every streamed row. +func collectStream(t *testing.T, svc *ExpenseService, userID string, pageSize int32, sink func(*model.Expense) error) error { + t.Helper() + done := make(chan error, 1) + go func() { + done <- svc.StreamAllUserExpenses(context.Background(), userID, pageSize, sink) + }() + select { + case err := <-done: + return err + case <-time.After(2 * time.Second): + t.Fatal("StreamAllUserExpenses did not return: producer failed to stop") + return nil + } +} + +func TestStreamAllUserExpenses_HappyPathStreamsAllRowsInOrder(t *testing.T) { + repo := new(mockExpenseRepository) + svc := newTestService(repo) + + page1 := []*model.Expense{streamExpense("exp-1", "2026-05-01T00:00:00Z"), streamExpense("exp-2", "2026-05-01T00:00:01Z")} + page2 := []*model.Expense{streamExpense("exp-3", "2026-05-01T00:00:02Z")} + cursor1 := repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:01Z", ID: "exp-2"} + cursor2 := repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:02Z", ID: "exp-3"} + + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", repository.ExpenseCursor{}, int32(2)).Return(page1, cursor1, true, nil) + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", cursor1, int32(2)).Return(page2, cursor2, false, nil) + + var got []string + err := collectStream(t, svc, "user-1", 2, func(e *model.Expense) error { + got = append(got, e.ID) + return nil + }) + + require.NoError(t, err) + assert.Equal(t, []string{"exp-1", "exp-2", "exp-3"}, got) + repo.AssertExpectations(t) +} + +func TestStreamAllUserExpenses_EmptyResultCleanEOF(t *testing.T) { + repo := new(mockExpenseRepository) + svc := newTestService(repo) + + repo.On("GetExpensesByUserAfter", mock.Anything, "user-empty", repository.ExpenseCursor{}, int32(50)). + Return([]*model.Expense{}, repository.ExpenseCursor{}, false, nil) + + sendCount := 0 + err := collectStream(t, svc, "user-empty", 50, func(*model.Expense) error { + sendCount++ + return nil + }) + + require.NoError(t, err) + assert.Zero(t, sendCount, "empty history must stream zero rows and terminate cleanly") + repo.AssertExpectations(t) +} + +func TestStreamAllUserExpenses_MidStreamCancellationStopsProducer(t *testing.T) { + repo := new(mockExpenseRepository) + svc := newTestService(repo) + + // hasMore stays true forever: without cancellation the walk never ends, so a + // clean return proves the producer honored cancellation and stopped. + page := []*model.Expense{streamExpense("exp-1", "2026-05-01T00:00:00Z"), streamExpense("exp-2", "2026-05-01T00:00:01Z")} + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", mock.Anything, mock.Anything). + Return(page, repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:01Z", ID: "exp-2"}, true, nil) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + sendCount := 0 + done := make(chan error, 1) + go func() { + done <- svc.StreamAllUserExpenses(ctx, "user-1", 2, func(*model.Expense) error { + sendCount++ + cancel() // cancel mid-stream after the first row + return nil + }) + }() + + select { + case err := <-done: + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 1, sendCount, "consumer must stop sending once the context is cancelled") + case <-time.After(2 * time.Second): + t.Fatal("StreamAllUserExpenses did not return after cancellation: producer failed to stop") + } +} + +func TestStreamAllUserExpenses_SendErrorStopsWalk(t *testing.T) { + repo := new(mockExpenseRepository) + svc := newTestService(repo) + + page := []*model.Expense{streamExpense("exp-1", "2026-05-01T00:00:00Z"), streamExpense("exp-2", "2026-05-01T00:00:01Z")} + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", mock.Anything, mock.Anything). + Return(page, repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:01Z", ID: "exp-2"}, true, nil) + + sendErr := errors.New("stream send failed") + sendCount := 0 + err := collectStream(t, svc, "user-1", 2, func(*model.Expense) error { + sendCount++ + return sendErr + }) + + require.ErrorIs(t, err, sendErr) + assert.Equal(t, 1, sendCount) +} + +func TestStreamAllUserExpenses_RepoErrorPropagates(t *testing.T) { + repo := new(mockExpenseRepository) + svc := newTestService(repo) + + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", repository.ExpenseCursor{}, int32(10)). + Return(nil, repository.ExpenseCursor{}, false, errors.New("immudb unavailable")) + + sendCount := 0 + err := collectStream(t, svc, "user-1", 10, func(*model.Expense) error { + sendCount++ + return nil + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "immudb unavailable") + assert.Zero(t, sendCount) + repo.AssertExpectations(t) +} + +func TestStreamAllUserExpenses_ValidationErrorForEmptyUserID(t *testing.T) { + repo := new(mockExpenseRepository) + svc := newTestService(repo) + + err := svc.StreamAllUserExpenses(context.Background(), "", 10, func(*model.Expense) error { return nil }) + + var svcErr *ServiceError + require.ErrorAs(t, err, &svcErr) + assert.Equal(t, model.ErrValidationError, svcErr.Code) +} + +func TestStreamAllUserExpenses_DefaultsPageSizeWhenNonPositive(t *testing.T) { + repo := new(mockExpenseRepository) + svc := newTestService(repo) + + repo.On("GetExpensesByUserAfter", mock.Anything, "user-1", repository.ExpenseCursor{}, repository.DefaultStreamPageSize). + Return([]*model.Expense{}, repository.ExpenseCursor{}, false, nil) + + err := collectStream(t, svc, "user-1", 0, func(*model.Expense) error { return nil }) + + require.NoError(t, err) + repo.AssertExpectations(t) +} + +// countingStreamRepo answers GetExpensesByUserAfter with a fixed page and an +// always-more cursor (up to a large finite guard), counting how many pages the +// producer fetched. All other repository methods are inherited unused. +type countingStreamRepo struct { + mockExpenseRepository + page []*model.Expense + next repository.ExpenseCursor + pages atomic.Int64 +} + +func (r *countingStreamRepo) GetExpensesByUserAfter(_ context.Context, _ string, _ repository.ExpenseCursor, _ int32) ([]*model.Expense, repository.ExpenseCursor, bool, error) { + n := r.pages.Add(1) + // hasMore stays true until a large finite guard so a correctly back-pressured + // producer blocks on the full buffer, while a regressed unbounded producer + // still terminates (and trips the assertion) instead of looping forever. + return r.page, r.next, n < 1000, nil +} + +func TestStreamAllUserExpenses_ProducerLookAheadBoundedToOnePage(t *testing.T) { + // When the consumer stops draining, the producer must block once the + // pageSize-buffered rows channel fills, retaining O(pageSize) rows rather + // than eagerly fetching the whole history. A regression that unbounds the + // buffer (or drops back-pressure) would fetch far more pages before blocking. + const pageSize = int32(10) + page := make([]*model.Expense, pageSize) + for i := range page { + page[i] = streamExpense(fmt.Sprintf("exp-%d", i), "2026-05-01T00:00:00Z") + } + repo := &countingStreamRepo{ + page: page, + next: repository.ExpenseCursor{CreatedAt: "2026-05-01T00:00:00Z", ID: "exp-9"}, + } + svc := NewExpenseService(repo, slog.New(slog.NewJSONHandler(io.Discard, nil))) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + firstRow := make(chan struct{}) + release := make(chan struct{}) + done := make(chan error, 1) + go func() { + received := 0 + done <- svc.StreamAllUserExpenses(ctx, "user-1", pageSize, func(*model.Expense) error { + received++ + if received == 1 { + close(firstRow) + <-release // block after the first row so the buffer fills up + } + return nil + }) + }() + + <-firstRow + // Let the producer reach its blocked steady state (buffer full). The + // assertion is an upper bound, so an over-short settle only lowers the + // observed count; it never yields a false failure. + time.Sleep(50 * time.Millisecond) + pagesFetched := repo.pages.Load() + + cancel() + close(release) + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("StreamAllUserExpenses did not return after cancellation") + } + + assert.LessOrEqual(t, pagesFetched, int64(3), + "producer must block after buffering ~one page, not drain the full history (fetched %d pages)", pagesFetched) +} diff --git a/services/expense/perf/baseline/read.txt b/services/expense/perf/baseline/read.txt new file mode 100644 index 00000000..9d52dfe2 --- /dev/null +++ b/services/expense/perf/baseline/read.txt @@ -0,0 +1,81 @@ +# gofin perf baseline +path: expense/export-read +new path: GetExpensesByUserAfter (keyset) + StreamAllUserExpenses RPC +baseline captured against: OLD OFFSET path GetAllExpensesByUser +captured_against: ft/latency-refactor OFFSET path (unchanged from main @ 8baca69, pre-optimization) +machine: Apple M4 Pro, 14 cores, macOS (wall-clock reference only) +goos/goarch: darwin/arm64 go: go1.26.2 +date: 2026-07-10 + +# --------------------------------------------------------------------------- +# Environment-independent structural signals (portable, gated in CI) +# +# These reflect the exact SQL the repository issues against any backend, so the +# recording-mock harness reproduces them faithfully. They are the durable +# regression signals (see internal/repository/keyset_test.go and §04). +# --------------------------------------------------------------------------- + +# per full export of P pages (pageSize = 50): +queries_per_export: OFFSET = 2*P | keyset = P +count_queries_per_export: OFFSET = P | keyset = 0 +data_queries_per_export: OFFSET = P | keyset = P (one data query per page) +uses_OFFSET: OFFSET = yes | keyset = no + +# rows scanned across a full export (execution property of the real database; +# analytical -- see the EXPLAIN note below): +rows_scanned_total: OFFSET = O(P^2 * pageSize) (each page rescans+discards prior pages) + keyset = O(P * pageSize) (seek past cursor, no rescan) +# +# The keyset seek is index-backed by idx_expenses_user_created_id +# (user_id, created_at, id), which matches the full ORDER BY created_at ASC, +# id ASC tuple so rows tied on created_at seek rather than scan+sort. +# +# EXPLAIN note: immudb 1.11.0 exposes no EXPLAIN / query-plan statement (its +# 1.11.0 verification helpers are immudb_state / immudb_verify_row / +# immudb_verify_tx / immudb_history -- none surface a plan), so the O(P) +# rows-scanned property cannot be asserted via a range-seek EXPLAIN on the +# pinned version. It is therefore analytical, backed by the covering index +# above. The integration test (keyset_integration_test.go, -tags integration) +# confirms what IS observable against real immudb: keyset ordering across a +# shared-created_at boundary and the absence of OFFSET -- not scan cost. + +# --------------------------------------------------------------------------- +# Structural scaling curve (portable: query/count shape, not wall-clock) +# Source: BenchmarkExportExpenseRead, queries/export + counts/export metrics. +# --------------------------------------------------------------------------- +# OFFSET (baseline) keyset (after) +# P queries counts queries counts + 1 2 1 1 0 + 10 20 10 10 0 + 50 100 50 50 0 + 100 200 100 100 0 +# +# OFFSET queries and COUNT(*) both grow linearly with P, and each OFFSET page +# rescans all prior pages -> O(P^2) rows scanned. keyset drops COUNT entirely +# (hasMore comes from the pageSize+1 overflow row) and never rescans -> O(P). + +# --------------------------------------------------------------------------- +# Wall-clock: intentionally NOT committed +# +# The recording-mock harness (BenchmarkExportExpenseRead) returns canned rows +# and re-sorts/filters the whole dataset per page, so its ns/op showed keyset +# SLOWER than the OFFSET path -- the wrong direction. It cannot reproduce +# OFFSET's real O(P^2) disk-scan cost, so mock wall-clock is not DB-level +# evidence and is deliberately omitted (a wrong-direction number is worse than +# none). The durable regression signals are the query-shape/count assertions +# above; CI asserts those (linear query-count growth, zero COUNT, no OFFSET), +# never absolute ns/op or allocs. +# +# The real O(P^2) -> O(P) timing curve is only observable against the pinned +# codenotary/immudb:1.11.0 container; capture it there if a wall-clock +# reference is needed (the mock cannot stand in for it). +# +# Memory (O(pageSize) retained heap): the portable corroboration is the +# datarights streaming-consumer growth-ratio test +# (services/datarights/internal/engine/providers/expenses_stream_test.go, with +# baseline services/datarights/perf/baseline/stream-consumer.txt), which fences +# peak retained heap across two row counts. An -alloc_space pprof of the mock +# keyset walk is NOT committed: it is dominated (>90%) by the mock's own +# SQLQuery/row-materialization allocations, not the production streaming path, +# so it would mislead the same way the mock wall-clock does. A real pprof +# requires the live immudb transport path. diff --git a/services/expense/proto/expense.proto b/services/expense/proto/expense.proto index ea24c9ca..72b24675 100644 --- a/services/expense/proto/expense.proto +++ b/services/expense/proto/expense.proto @@ -13,8 +13,18 @@ service ExpenseService { // Tag usage check (called by finance service during tag deletion) rpc CountExpensesByTag(CountExpensesByTagRequest) returns (CountExpensesByTagResponse); - // Data export: returns all expenses (active + corrected) for a user, paginated - rpc GetAllUserExpenses(GetAllUserExpensesRequest) returns (ExpenseListResponse); + // Data export streaming: the server streams every expense (active + corrected) + // for a user in chronological order (created_at ASC, id ASC). The server pages + // internally with a keyset cursor, bounding server memory to O(page_size); the + // client writes rows incrementally. + // + // WIRE BREAK: this replaced a unary GetAllUserExpenses RPC (renamed, request + // message swapped, field number 2 reused for page_size). It is an intentional + // atomic internal break: the sole consumer (datarights export) is cut over in + // the same change and no proto bytes are persisted, so there is no + // cross-version wire path and the reserve-field rules for incremental rollout + // do not apply. + rpc StreamAllUserExpenses(StreamAllUserExpensesRequest) returns (stream ExpenseData); // GDPR: anonymize all expenses for a user (field redaction, not deletion) rpc AnonymizeAllUserExpenses(AnonymizeRequest) returns (AnonymizeResponse); @@ -129,13 +139,12 @@ message GetProRataGroupRequest { } // --------------------------------------------------------------------------- -// GetAllUserExpenses (data export) +// StreamAllUserExpenses (data export, server-streaming) // --------------------------------------------------------------------------- -message GetAllUserExpensesRequest { +message StreamAllUserExpensesRequest { string user_id = 1; - int32 page = 2; - int32 page_size = 3; + int32 page_size = 2; // internal server-side keyset page size; 0 = server default } // --------------------------------------------------------------------------- diff --git a/services/expense/proto/expensepb/expense.pb.go b/services/expense/proto/expensepb/expense.pb.go index 4a687781..9f1c2ca4 100644 --- a/services/expense/proto/expensepb/expense.pb.go +++ b/services/expense/proto/expensepb/expense.pb.go @@ -805,29 +805,28 @@ func (x *GetProRataGroupRequest) GetGroupId() string { return "" } -type GetAllUserExpensesRequest struct { +type StreamAllUserExpensesRequest struct { state protoimpl.MessageState `protogen:"open.v1"` UserId string `protobuf:"bytes,1,opt,name=user_id,json=userId,proto3" json:"user_id,omitempty"` - Page int32 `protobuf:"varint,2,opt,name=page,proto3" json:"page,omitempty"` - PageSize int32 `protobuf:"varint,3,opt,name=page_size,json=pageSize,proto3" json:"page_size,omitempty"` + PageSize int32 `protobuf:"varint,2,opt,name=page_size,json=pageSize,proto3" json:"page_size,omitempty"` // internal server-side keyset page size; 0 = server default unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } -func (x *GetAllUserExpensesRequest) Reset() { - *x = GetAllUserExpensesRequest{} +func (x *StreamAllUserExpensesRequest) Reset() { + *x = StreamAllUserExpensesRequest{} mi := &file_proto_expense_proto_msgTypes[10] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } -func (x *GetAllUserExpensesRequest) String() string { +func (x *StreamAllUserExpensesRequest) String() string { return protoimpl.X.MessageStringOf(x) } -func (*GetAllUserExpensesRequest) ProtoMessage() {} +func (*StreamAllUserExpensesRequest) ProtoMessage() {} -func (x *GetAllUserExpensesRequest) ProtoReflect() protoreflect.Message { +func (x *StreamAllUserExpensesRequest) ProtoReflect() protoreflect.Message { mi := &file_proto_expense_proto_msgTypes[10] if x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) @@ -839,26 +838,19 @@ func (x *GetAllUserExpensesRequest) ProtoReflect() protoreflect.Message { return mi.MessageOf(x) } -// Deprecated: Use GetAllUserExpensesRequest.ProtoReflect.Descriptor instead. -func (*GetAllUserExpensesRequest) Descriptor() ([]byte, []int) { +// Deprecated: Use StreamAllUserExpensesRequest.ProtoReflect.Descriptor instead. +func (*StreamAllUserExpensesRequest) Descriptor() ([]byte, []int) { return file_proto_expense_proto_rawDescGZIP(), []int{10} } -func (x *GetAllUserExpensesRequest) GetUserId() string { +func (x *StreamAllUserExpensesRequest) GetUserId() string { if x != nil { return x.UserId } return "" } -func (x *GetAllUserExpensesRequest) GetPage() int32 { - if x != nil { - return x.Page - } - return 0 -} - -func (x *GetAllUserExpensesRequest) GetPageSize() int32 { +func (x *StreamAllUserExpensesRequest) GetPageSize() int32 { if x != nil { return x.PageSize } @@ -1116,11 +1108,10 @@ const file_proto_expense_proto_rawDesc = "" + "\x19CorrectionHistoryResponse\x12.\n" + "\aentries\x18\x01 \x03(\v2\x14.expense.ExpenseDataR\aentries\"3\n" + "\x16GetProRataGroupRequest\x12\x19\n" + - "\bgroup_id\x18\x01 \x01(\tR\agroupId\"e\n" + - "\x19GetAllUserExpensesRequest\x12\x17\n" + - "\auser_id\x18\x01 \x01(\tR\x06userId\x12\x12\n" + - "\x04page\x18\x02 \x01(\x05R\x04page\x12\x1b\n" + - "\tpage_size\x18\x03 \x01(\x05R\bpageSize\"K\n" + + "\bgroup_id\x18\x01 \x01(\tR\agroupId\"T\n" + + "\x1cStreamAllUserExpensesRequest\x12\x17\n" + + "\auser_id\x18\x01 \x01(\tR\x06userId\x12\x1b\n" + + "\tpage_size\x18\x02 \x01(\x05R\bpageSize\"K\n" + "\x19CountExpensesByTagRequest\x12\x15\n" + "\x06tag_id\x18\x01 \x01(\tR\x05tagId\x12\x17\n" + "\auser_id\x18\x02 \x01(\tR\x06userId\"2\n" + @@ -1135,7 +1126,7 @@ const file_proto_expense_proto_rawDesc = "" + "\n" + "GetExpense\x12\x1a.expense.GetExpenseRequest\x1a\x18.expense.ExpenseResponse\x12]\n" + "\x12CountExpensesByTag\x12\".expense.CountExpensesByTagRequest\x1a#.expense.CountExpensesByTagResponse\x12V\n" + - "\x12GetAllUserExpenses\x12\".expense.GetAllUserExpensesRequest\x1a\x1c.expense.ExpenseListResponse\x12Q\n" + + "\x15StreamAllUserExpenses\x12%.expense.StreamAllUserExpensesRequest\x1a\x14.expense.ExpenseData0\x01\x12Q\n" + "\x18AnonymizeAllUserExpenses\x12\x19.expense.AnonymizeRequest\x1a\x1a.expense.AnonymizeResponse\x12J\n" + "\x0eCorrectExpense\x12\x1e.expense.CorrectExpenseRequest\x1a\x18.expense.ExpenseResponse\x12`\n" + "\x14GetCorrectionHistory\x12$.expense.GetCorrectionHistoryRequest\x1a\".expense.CorrectionHistoryResponse\x12P\n" + @@ -1155,21 +1146,21 @@ func file_proto_expense_proto_rawDescGZIP() []byte { var file_proto_expense_proto_msgTypes = make([]protoimpl.MessageInfo, 15) var file_proto_expense_proto_goTypes = []any{ - (*ExpenseData)(nil), // 0: expense.ExpenseData - (*CreateExpenseRequest)(nil), // 1: expense.CreateExpenseRequest - (*ExpenseResponse)(nil), // 2: expense.ExpenseResponse - (*GetExpensesForPeriodRequest)(nil), // 3: expense.GetExpensesForPeriodRequest - (*ExpenseListResponse)(nil), // 4: expense.ExpenseListResponse - (*GetExpenseRequest)(nil), // 5: expense.GetExpenseRequest - (*CorrectExpenseRequest)(nil), // 6: expense.CorrectExpenseRequest - (*GetCorrectionHistoryRequest)(nil), // 7: expense.GetCorrectionHistoryRequest - (*CorrectionHistoryResponse)(nil), // 8: expense.CorrectionHistoryResponse - (*GetProRataGroupRequest)(nil), // 9: expense.GetProRataGroupRequest - (*GetAllUserExpensesRequest)(nil), // 10: expense.GetAllUserExpensesRequest - (*CountExpensesByTagRequest)(nil), // 11: expense.CountExpensesByTagRequest - (*CountExpensesByTagResponse)(nil), // 12: expense.CountExpensesByTagResponse - (*AnonymizeRequest)(nil), // 13: expense.AnonymizeRequest - (*AnonymizeResponse)(nil), // 14: expense.AnonymizeResponse + (*ExpenseData)(nil), // 0: expense.ExpenseData + (*CreateExpenseRequest)(nil), // 1: expense.CreateExpenseRequest + (*ExpenseResponse)(nil), // 2: expense.ExpenseResponse + (*GetExpensesForPeriodRequest)(nil), // 3: expense.GetExpensesForPeriodRequest + (*ExpenseListResponse)(nil), // 4: expense.ExpenseListResponse + (*GetExpenseRequest)(nil), // 5: expense.GetExpenseRequest + (*CorrectExpenseRequest)(nil), // 6: expense.CorrectExpenseRequest + (*GetCorrectionHistoryRequest)(nil), // 7: expense.GetCorrectionHistoryRequest + (*CorrectionHistoryResponse)(nil), // 8: expense.CorrectionHistoryResponse + (*GetProRataGroupRequest)(nil), // 9: expense.GetProRataGroupRequest + (*StreamAllUserExpensesRequest)(nil), // 10: expense.StreamAllUserExpensesRequest + (*CountExpensesByTagRequest)(nil), // 11: expense.CountExpensesByTagRequest + (*CountExpensesByTagResponse)(nil), // 12: expense.CountExpensesByTagResponse + (*AnonymizeRequest)(nil), // 13: expense.AnonymizeRequest + (*AnonymizeResponse)(nil), // 14: expense.AnonymizeResponse } var file_proto_expense_proto_depIdxs = []int32{ 0, // 0: expense.ExpenseResponse.expense:type_name -> expense.ExpenseData @@ -1179,7 +1170,7 @@ var file_proto_expense_proto_depIdxs = []int32{ 3, // 4: expense.ExpenseService.GetExpensesForPeriod:input_type -> expense.GetExpensesForPeriodRequest 5, // 5: expense.ExpenseService.GetExpense:input_type -> expense.GetExpenseRequest 11, // 6: expense.ExpenseService.CountExpensesByTag:input_type -> expense.CountExpensesByTagRequest - 10, // 7: expense.ExpenseService.GetAllUserExpenses:input_type -> expense.GetAllUserExpensesRequest + 10, // 7: expense.ExpenseService.StreamAllUserExpenses:input_type -> expense.StreamAllUserExpensesRequest 13, // 8: expense.ExpenseService.AnonymizeAllUserExpenses:input_type -> expense.AnonymizeRequest 6, // 9: expense.ExpenseService.CorrectExpense:input_type -> expense.CorrectExpenseRequest 7, // 10: expense.ExpenseService.GetCorrectionHistory:input_type -> expense.GetCorrectionHistoryRequest @@ -1188,7 +1179,7 @@ var file_proto_expense_proto_depIdxs = []int32{ 4, // 13: expense.ExpenseService.GetExpensesForPeriod:output_type -> expense.ExpenseListResponse 2, // 14: expense.ExpenseService.GetExpense:output_type -> expense.ExpenseResponse 12, // 15: expense.ExpenseService.CountExpensesByTag:output_type -> expense.CountExpensesByTagResponse - 4, // 16: expense.ExpenseService.GetAllUserExpenses:output_type -> expense.ExpenseListResponse + 0, // 16: expense.ExpenseService.StreamAllUserExpenses:output_type -> expense.ExpenseData 14, // 17: expense.ExpenseService.AnonymizeAllUserExpenses:output_type -> expense.AnonymizeResponse 2, // 18: expense.ExpenseService.CorrectExpense:output_type -> expense.ExpenseResponse 8, // 19: expense.ExpenseService.GetCorrectionHistory:output_type -> expense.CorrectionHistoryResponse diff --git a/services/expense/proto/expensepb/expense_grpc.pb.go b/services/expense/proto/expensepb/expense_grpc.pb.go index 1e87d858..698a74e5 100644 --- a/services/expense/proto/expensepb/expense_grpc.pb.go +++ b/services/expense/proto/expensepb/expense_grpc.pb.go @@ -23,7 +23,7 @@ const ( ExpenseService_GetExpensesForPeriod_FullMethodName = "/expense.ExpenseService/GetExpensesForPeriod" ExpenseService_GetExpense_FullMethodName = "/expense.ExpenseService/GetExpense" ExpenseService_CountExpensesByTag_FullMethodName = "/expense.ExpenseService/CountExpensesByTag" - ExpenseService_GetAllUserExpenses_FullMethodName = "/expense.ExpenseService/GetAllUserExpenses" + ExpenseService_StreamAllUserExpenses_FullMethodName = "/expense.ExpenseService/StreamAllUserExpenses" ExpenseService_AnonymizeAllUserExpenses_FullMethodName = "/expense.ExpenseService/AnonymizeAllUserExpenses" ExpenseService_CorrectExpense_FullMethodName = "/expense.ExpenseService/CorrectExpense" ExpenseService_GetCorrectionHistory_FullMethodName = "/expense.ExpenseService/GetCorrectionHistory" @@ -40,8 +40,11 @@ type ExpenseServiceClient interface { GetExpense(ctx context.Context, in *GetExpenseRequest, opts ...grpc.CallOption) (*ExpenseResponse, error) // Tag usage check (called by finance service during tag deletion) CountExpensesByTag(ctx context.Context, in *CountExpensesByTagRequest, opts ...grpc.CallOption) (*CountExpensesByTagResponse, error) - // Data export: returns all expenses (active + corrected) for a user, paginated - GetAllUserExpenses(ctx context.Context, in *GetAllUserExpensesRequest, opts ...grpc.CallOption) (*ExpenseListResponse, error) + // Data export streaming: the server streams every expense (active + corrected) + // for a user in chronological order (created_at ASC, id ASC). The server pages + // internally with a keyset cursor, bounding server memory to O(page_size); the + // client writes rows incrementally. + StreamAllUserExpenses(ctx context.Context, in *StreamAllUserExpensesRequest, opts ...grpc.CallOption) (grpc.ServerStreamingClient[ExpenseData], error) // GDPR: anonymize all expenses for a user (field redaction, not deletion) AnonymizeAllUserExpenses(ctx context.Context, in *AnonymizeRequest, opts ...grpc.CallOption) (*AnonymizeResponse, error) // Stubs: implemented in later tickets @@ -98,16 +101,25 @@ func (c *expenseServiceClient) CountExpensesByTag(ctx context.Context, in *Count return out, nil } -func (c *expenseServiceClient) GetAllUserExpenses(ctx context.Context, in *GetAllUserExpensesRequest, opts ...grpc.CallOption) (*ExpenseListResponse, error) { +func (c *expenseServiceClient) StreamAllUserExpenses(ctx context.Context, in *StreamAllUserExpensesRequest, opts ...grpc.CallOption) (grpc.ServerStreamingClient[ExpenseData], error) { cOpts := append([]grpc.CallOption{grpc.StaticMethod()}, opts...) - out := new(ExpenseListResponse) - err := c.cc.Invoke(ctx, ExpenseService_GetAllUserExpenses_FullMethodName, in, out, cOpts...) + stream, err := c.cc.NewStream(ctx, &ExpenseService_ServiceDesc.Streams[0], ExpenseService_StreamAllUserExpenses_FullMethodName, cOpts...) if err != nil { return nil, err } - return out, nil + x := &grpc.GenericClientStream[StreamAllUserExpensesRequest, ExpenseData]{ClientStream: stream} + if err := x.ClientStream.SendMsg(in); err != nil { + return nil, err + } + if err := x.ClientStream.CloseSend(); err != nil { + return nil, err + } + return x, nil } +// This type alias is provided for backwards compatibility with existing code that references the prior non-generic stream type by name. +type ExpenseService_StreamAllUserExpensesClient = grpc.ServerStreamingClient[ExpenseData] + func (c *expenseServiceClient) AnonymizeAllUserExpenses(ctx context.Context, in *AnonymizeRequest, opts ...grpc.CallOption) (*AnonymizeResponse, error) { cOpts := append([]grpc.CallOption{grpc.StaticMethod()}, opts...) out := new(AnonymizeResponse) @@ -158,8 +170,11 @@ type ExpenseServiceServer interface { GetExpense(context.Context, *GetExpenseRequest) (*ExpenseResponse, error) // Tag usage check (called by finance service during tag deletion) CountExpensesByTag(context.Context, *CountExpensesByTagRequest) (*CountExpensesByTagResponse, error) - // Data export: returns all expenses (active + corrected) for a user, paginated - GetAllUserExpenses(context.Context, *GetAllUserExpensesRequest) (*ExpenseListResponse, error) + // Data export streaming: the server streams every expense (active + corrected) + // for a user in chronological order (created_at ASC, id ASC). The server pages + // internally with a keyset cursor, bounding server memory to O(page_size); the + // client writes rows incrementally. + StreamAllUserExpenses(*StreamAllUserExpensesRequest, grpc.ServerStreamingServer[ExpenseData]) error // GDPR: anonymize all expenses for a user (field redaction, not deletion) AnonymizeAllUserExpenses(context.Context, *AnonymizeRequest) (*AnonymizeResponse, error) // Stubs: implemented in later tickets @@ -188,8 +203,8 @@ func (UnimplementedExpenseServiceServer) GetExpense(context.Context, *GetExpense func (UnimplementedExpenseServiceServer) CountExpensesByTag(context.Context, *CountExpensesByTagRequest) (*CountExpensesByTagResponse, error) { return nil, status.Error(codes.Unimplemented, "method CountExpensesByTag not implemented") } -func (UnimplementedExpenseServiceServer) GetAllUserExpenses(context.Context, *GetAllUserExpensesRequest) (*ExpenseListResponse, error) { - return nil, status.Error(codes.Unimplemented, "method GetAllUserExpenses not implemented") +func (UnimplementedExpenseServiceServer) StreamAllUserExpenses(*StreamAllUserExpensesRequest, grpc.ServerStreamingServer[ExpenseData]) error { + return status.Error(codes.Unimplemented, "method StreamAllUserExpenses not implemented") } func (UnimplementedExpenseServiceServer) AnonymizeAllUserExpenses(context.Context, *AnonymizeRequest) (*AnonymizeResponse, error) { return nil, status.Error(codes.Unimplemented, "method AnonymizeAllUserExpenses not implemented") @@ -296,24 +311,17 @@ func _ExpenseService_CountExpensesByTag_Handler(srv interface{}, ctx context.Con return interceptor(ctx, in, info, handler) } -func _ExpenseService_GetAllUserExpenses_Handler(srv interface{}, ctx context.Context, dec func(interface{}) error, interceptor grpc.UnaryServerInterceptor) (interface{}, error) { - in := new(GetAllUserExpensesRequest) - if err := dec(in); err != nil { - return nil, err +func _ExpenseService_StreamAllUserExpenses_Handler(srv interface{}, stream grpc.ServerStream) error { + m := new(StreamAllUserExpensesRequest) + if err := stream.RecvMsg(m); err != nil { + return err } - if interceptor == nil { - return srv.(ExpenseServiceServer).GetAllUserExpenses(ctx, in) - } - info := &grpc.UnaryServerInfo{ - Server: srv, - FullMethod: ExpenseService_GetAllUserExpenses_FullMethodName, - } - handler := func(ctx context.Context, req interface{}) (interface{}, error) { - return srv.(ExpenseServiceServer).GetAllUserExpenses(ctx, req.(*GetAllUserExpensesRequest)) - } - return interceptor(ctx, in, info, handler) + return srv.(ExpenseServiceServer).StreamAllUserExpenses(m, &grpc.GenericServerStream[StreamAllUserExpensesRequest, ExpenseData]{ServerStream: stream}) } +// This type alias is provided for backwards compatibility with existing code that references the prior non-generic stream type by name. +type ExpenseService_StreamAllUserExpensesServer = grpc.ServerStreamingServer[ExpenseData] + func _ExpenseService_AnonymizeAllUserExpenses_Handler(srv interface{}, ctx context.Context, dec func(interface{}) error, interceptor grpc.UnaryServerInterceptor) (interface{}, error) { in := new(AnonymizeRequest) if err := dec(in); err != nil { @@ -409,10 +417,6 @@ var ExpenseService_ServiceDesc = grpc.ServiceDesc{ MethodName: "CountExpensesByTag", Handler: _ExpenseService_CountExpensesByTag_Handler, }, - { - MethodName: "GetAllUserExpenses", - Handler: _ExpenseService_GetAllUserExpenses_Handler, - }, { MethodName: "AnonymizeAllUserExpenses", Handler: _ExpenseService_AnonymizeAllUserExpenses_Handler, @@ -430,6 +434,12 @@ var ExpenseService_ServiceDesc = grpc.ServiceDesc{ Handler: _ExpenseService_GetProRataGroup_Handler, }, }, - Streams: []grpc.StreamDesc{}, + Streams: []grpc.StreamDesc{ + { + StreamName: "StreamAllUserExpenses", + Handler: _ExpenseService_StreamAllUserExpenses_Handler, + ServerStreams: true, + }, + }, Metadata: "proto/expense.proto", } diff --git a/services/finance/Dockerfile b/services/finance/Dockerfile index 60cf3663..e19477cc 100644 --- a/services/finance/Dockerfile +++ b/services/finance/Dockerfile @@ -13,6 +13,7 @@ COPY dbmigrate/go.mod dbmigrate/go.sum* ./dbmigrate/ COPY healthcheck/go.mod ./healthcheck/ COPY metrics/go.mod metrics/go.sum* ./metrics/ COPY expense/go.mod expense/go.sum* ./expense/ +COPY perf/go.mod ./perf/ RUN --mount=type=cache,target=/go/pkg/mod \ cd finance && GOWORK=off go mod download @@ -23,6 +24,7 @@ COPY dbmigrate/ ./dbmigrate/ COPY healthcheck/ ./healthcheck/ COPY metrics/ ./metrics/ COPY expense/ ./expense/ +COPY perf/ ./perf/ RUN --mount=type=cache,target=/go/pkg/mod \ --mount=type=cache,target=/root/.cache/go-build \ diff --git a/services/finance/go.mod b/services/finance/go.mod index 1f804a62..242589d3 100644 --- a/services/finance/go.mod +++ b/services/finance/go.mod @@ -8,10 +8,12 @@ require ( github.com/ItsThompson/gofin/services/expense v0.0.0-00010101000000-000000000000 github.com/ItsThompson/gofin/services/healthcheck v0.0.0 github.com/ItsThompson/gofin/services/metrics v0.0.0 + github.com/ItsThompson/gofin/services/perf v0.0.0 github.com/gin-gonic/gin v1.12.0 github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.9.2 github.com/stretchr/testify v1.11.1 + golang.org/x/sync v0.20.0 google.golang.org/grpc v1.80.0 google.golang.org/protobuf v1.36.11 ) @@ -22,6 +24,8 @@ replace github.com/ItsThompson/gofin/services/healthcheck => ../healthcheck replace github.com/ItsThompson/gofin/services/metrics => ../metrics +replace github.com/ItsThompson/gofin/services/perf => ../perf + replace github.com/ItsThompson/gofin/services/expense => ../expense replace github.com/ItsThompson/gofin/services/dbmigrate => ../dbmigrate @@ -71,7 +75,6 @@ require ( golang.org/x/arch v0.22.0 // indirect golang.org/x/crypto v0.50.0 // indirect golang.org/x/net v0.52.0 // indirect - golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.43.0 // indirect golang.org/x/text v0.36.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260120221211-b8f7ae30c516 // indirect diff --git a/services/finance/internal/handler/grpc_test.go b/services/finance/internal/handler/grpc_test.go index 2f16d231..4a48d993 100644 --- a/services/finance/internal/handler/grpc_test.go +++ b/services/finance/internal/handler/grpc_test.go @@ -115,7 +115,12 @@ func TestGetAllUserData_InternalError(t *testing.T) { repo := new(mockFinanceRepository) handler := setupGRPCHandler(repo) + // GetAllUserData fans out the three reads concurrently (no serial + // short-circuit), so the sibling reads must be stubbed even though ListTags + // is the one failing. The errgroup surfaces the ListTags error. repo.On("ListTags", mock.Anything, "user-1").Return(nil, fmt.Errorf("connection refused")) + repo.On("ListPeriods", mock.Anything, "user-1").Return([]*model.BudgetPeriod{}, nil) + repo.On("GetDefaults", mock.Anything, "user-1").Return((*model.DefaultSettings)(nil), nil) resp, err := handler.GetAllUserData(context.Background(), &pb.GetAllUserDataRequest{UserId: "user-1"}) assert.Nil(t, resp) diff --git a/services/finance/internal/service/alluserdata.go b/services/finance/internal/service/alluserdata.go index 559cce22..52c92864 100644 --- a/services/finance/internal/service/alluserdata.go +++ b/services/finance/internal/service/alluserdata.go @@ -4,26 +4,56 @@ import ( "context" "fmt" + "golang.org/x/sync/errgroup" + "github.com/ItsThompson/gofin/services/finance/internal/model" ) // GetAllUserData retrieves all tags, budget periods, and default settings for a user. // Returns empty slices (not errors) for users with no data beyond defaults. // Used by the datarights service for GDPR data export. +// +// The three repository reads are independent, so they run concurrently under a +// bounded errgroup; each goroutine writes its own distinct variable (the writes +// are disjoint by construction, not shared slice slots). Nil-slice normalization +// and result assembly run after the g.Wait() barrier, so the output is identical +// to the serial version. The bound is dashboardFanoutLimit, shared with the +// dashboard fan-out for a uniform cap; this fixed 3-read fan-out never reaches it. func (s *FinanceService) GetAllUserData(ctx context.Context, userID string) (*model.AllUserData, error) { - tags, err := s.repo.ListTags(ctx, userID) - if err != nil { - return nil, fmt.Errorf("listing tags for export: %w", err) - } - - periods, err := s.repo.ListPeriods(ctx, userID) - if err != nil { - return nil, fmt.Errorf("listing periods for export: %w", err) - } + var ( + tags []*model.Tag + periods []*model.BudgetPeriod + defaults *model.DefaultSettings + ) - defaults, err := s.repo.GetDefaults(ctx, userID) - if err != nil { - return nil, fmt.Errorf("getting defaults for export: %w", err) + g, gctx := errgroup.WithContext(ctx) + g.SetLimit(dashboardFanoutLimit) + g.Go(func() error { + v, err := s.repo.ListTags(gctx, userID) + if err != nil { + return fmt.Errorf("listing tags for export: %w", err) + } + tags = v + return nil + }) + g.Go(func() error { + v, err := s.repo.ListPeriods(gctx, userID) + if err != nil { + return fmt.Errorf("listing periods for export: %w", err) + } + periods = v + return nil + }) + g.Go(func() error { + v, err := s.repo.GetDefaults(gctx, userID) + if err != nil { + return fmt.Errorf("getting defaults for export: %w", err) + } + defaults = v + return nil + }) + if err := g.Wait(); err != nil { + return nil, err } // Normalize nil slices to empty slices diff --git a/services/finance/internal/service/alluserdata_bench_test.go b/services/finance/internal/service/alluserdata_bench_test.go new file mode 100644 index 00000000..6630c545 --- /dev/null +++ b/services/finance/internal/service/alluserdata_bench_test.go @@ -0,0 +1,68 @@ +package service + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/ItsThompson/gofin/services/finance/internal/model" +) + +// allUserDataReadLatency is a synthetic per-read upstream latency baked into the +// GetAllUserData benchmark so wall-clock reflects the fan-out shape (max of the +// three reads) rather than the serial sum. It is a benchmark knob only; +// correctness tests run the fake with zero delay. +const allUserDataReadLatency = 200 * time.Microsecond + +// seedAllUserData returns a fake repo populated with a representative export +// payload: ten tags, a full year of budget periods, and default settings. +func seedAllUserData(delay time.Duration) *countingAllUserDataRepo { + repo := newCountingAllUserDataRepo() + repo.delay = delay + for i := 0; i < 10; i++ { + repo.tags = append(repo.tags, &model.Tag{ + ID: fmt.Sprintf("tag-%02d", i), + UserID: "user-1", + Name: fmt.Sprintf("Tag %02d", i), + IsDefault: i < 8, + }) + } + for m := int32(12); m >= 1; m-- { + repo.periods = append(repo.periods, &model.BudgetPeriod{ + ID: fmt.Sprintf("p-2026-%02d", m), + UserID: "user-1", + Year: 2026, + Month: m, + BudgetAmount: 300000, + EssentialsPercent: 50, + DesiresPercent: 30, + SavingsPercent: 20, + }) + } + repo.defaults = &model.DefaultSettings{ + UserID: "user-1", + BudgetAmount: 300000, + EssentialsPercent: 50, + DesiresPercent: 30, + SavingsPercent: 20, + Currency: "GBP", + } + return repo +} + +// BenchmarkGetAllUserData measures the 3-read export path (tags, periods, +// defaults). Primary signal: repo call count (exactly one each) and wall-clock +// (fan-out max vs serial sum). GetAllUserData never touches the expense client, +// so none is injected. +func BenchmarkGetAllUserData(b *testing.B) { + repo := seedAllUserData(allUserDataReadLatency) + svc := newFanoutService(repo, nil) + + b.ReportAllocs() + for b.Loop() { + if _, err := svc.GetAllUserData(context.Background(), "user-1"); err != nil { + b.Fatal(err) + } + } +} diff --git a/services/finance/internal/service/alluserdata_test.go b/services/finance/internal/service/alluserdata_test.go index 151e8b7a..68222c66 100644 --- a/services/finance/internal/service/alluserdata_test.go +++ b/services/finance/internal/service/alluserdata_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "log/slog" + "sync" "testing" "time" @@ -13,6 +14,8 @@ import ( "github.com/stretchr/testify/require" "github.com/ItsThompson/gofin/services/finance/internal/model" + "github.com/ItsThompson/gofin/services/finance/internal/repository" + "github.com/ItsThompson/gofin/services/perf" ) func newAllUserDataTestService(repo *mockRepo) *FinanceService { @@ -133,7 +136,11 @@ func TestGetAllUserData_ListTagsError(t *testing.T) { repo := new(mockRepo) svc := newAllUserDataTestService(repo) + // The fan-out issues all three reads concurrently (no serial short-circuit), + // so the sibling reads must be stubbed even though ListTags is the one failing. repo.On("ListTags", mock.Anything, "user-1").Return(nil, fmt.Errorf("db connection failed")) + repo.On("ListPeriods", mock.Anything, "user-1").Return([]*model.BudgetPeriod{}, nil) + repo.On("GetDefaults", mock.Anything, "user-1").Return((*model.DefaultSettings)(nil), nil) result, err := svc.GetAllUserData(context.Background(), "user-1") assert.Nil(t, result) @@ -145,8 +152,10 @@ func TestGetAllUserData_ListPeriodsError(t *testing.T) { repo := new(mockRepo) svc := newAllUserDataTestService(repo) + // All three reads fire concurrently; ListPeriods is the failing one. repo.On("ListTags", mock.Anything, "user-1").Return([]*model.Tag{}, nil) repo.On("ListPeriods", mock.Anything, "user-1").Return(nil, fmt.Errorf("db connection failed")) + repo.On("GetDefaults", mock.Anything, "user-1").Return((*model.DefaultSettings)(nil), nil) result, err := svc.GetAllUserData(context.Background(), "user-1") assert.Nil(t, result) @@ -167,3 +176,134 @@ func TestGetAllUserData_GetDefaultsError(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "getting defaults for export") } + +// --- Fan-out regression tests --- + +// TestGetAllUserData_FanOutByteIdentical asserts the fan-out assembles exactly +// the serial result over seeded fixtures while reading each source exactly once +// (tags, periods, defaults). It fails if the fan-out drops, duplicates, or +// reorders a read, or diverges from the serial assembly. +func TestGetAllUserData_FanOutByteIdentical(t *testing.T) { + repo := seedAllUserData(0) + svc := newFanoutService(repo, nil) + + result, err := svc.GetAllUserData(context.Background(), "user-1") + require.NoError(t, err) + + expected := &model.AllUserData{ + Tags: repo.tags, + Periods: repo.periods, + Defaults: repo.defaults, + } + assert.Equal(t, expected, result, "fan-out output must be identical to the serial assembly") + + assert.Equal(t, 1, repo.counter.Count("ListTags"), "tags read exactly once") + assert.Equal(t, 1, repo.counter.Count("ListPeriods"), "periods read exactly once") + assert.Equal(t, 1, repo.counter.Count("GetDefaults"), "defaults read exactly once") + assert.Equal(t, 3, repo.counter.Total(), "exactly three reads total") +} + +// TestGetAllUserData_FanOutNormalizesNilSlicesAfterBarrier confirms the nil -> +// empty-slice normalization (and nil defaults passthrough) runs after g.Wait(), +// matching the serial version, when every read returns nil. Each source is still +// read exactly once. +func TestGetAllUserData_FanOutNormalizesNilSlicesAfterBarrier(t *testing.T) { + repo := newCountingAllUserDataRepo() // tags/periods nil, defaults nil + svc := newFanoutService(repo, nil) + + result, err := svc.GetAllUserData(context.Background(), "user-1") + require.NoError(t, err) + assert.NotNil(t, result.Tags) + assert.Empty(t, result.Tags) + assert.NotNil(t, result.Periods) + assert.Empty(t, result.Periods) + assert.Nil(t, result.Defaults) + + assert.Equal(t, 1, repo.counter.Count("ListTags")) + assert.Equal(t, 1, repo.counter.Count("ListPeriods")) + assert.Equal(t, 1, repo.counter.Count("GetDefaults")) +} + +// TestGetAllUserData_FanOutRunsConcurrently confirms the three reads overlap +// (fan-out, not serial) while staying within SetLimit(dashboardFanoutLimit). +func TestGetAllUserData_FanOutRunsConcurrently(t *testing.T) { + repo := seedAllUserData(5 * time.Millisecond) + svc := newFanoutService(repo, nil) + + _, err := svc.GetAllUserData(context.Background(), "user-1") + require.NoError(t, err) + assert.LessOrEqual(t, repo.maxConcurrent(), dashboardFanoutLimit, + "in-flight reads must not exceed SetLimit(dashboardFanoutLimit)") + assert.Equal(t, 3, repo.maxConcurrent(), + "all three reads must overlap under SetLimit(dashboardFanoutLimit>=3)") +} + +// --- Fan-out test infrastructure --- + +// countingAllUserDataRepo is a concurrency-aware fake FinanceRepository for the +// GetAllUserData fan-out regression tests and benchmark. It records one call per +// read through an embedded *perf.CallCounter, tracks the maximum number of reads +// in flight simultaneously (so both the SetLimit bound and real overlap can be +// asserted), and can simulate per-read latency so the benchmark shows fan-out +// (max) rather than serial (sum) wall-clock. Only the three methods +// GetAllUserData reads are implemented; the embedded interface is nil, so any +// other repo call panics and surfaces an accidental extra read. All methods are +// safe for concurrent use. +type countingAllUserDataRepo struct { + repository.FinanceRepository + counter *perf.CallCounter + delay time.Duration + tags []*model.Tag + periods []*model.BudgetPeriod + defaults *model.DefaultSettings + + mu sync.Mutex + inFlight int + maxInFlight int +} + +func newCountingAllUserDataRepo() *countingAllUserDataRepo { + return &countingAllUserDataRepo{counter: perf.NewCallCounter()} +} + +// enter records the call, bumps the in-flight gauge (tracking the peak), and +// simulates per-read latency. It returns the teardown func the caller defers. +func (r *countingAllUserDataRepo) enter(op string) func() { + r.counter.Record(op) + r.mu.Lock() + r.inFlight++ + if r.inFlight > r.maxInFlight { + r.maxInFlight = r.inFlight + } + r.mu.Unlock() + if r.delay > 0 { + time.Sleep(r.delay) + } + return func() { + r.mu.Lock() + r.inFlight-- + r.mu.Unlock() + } +} + +func (r *countingAllUserDataRepo) ListTags(context.Context, string) ([]*model.Tag, error) { + defer r.enter("ListTags")() + return r.tags, nil +} + +func (r *countingAllUserDataRepo) ListPeriods(context.Context, string) ([]*model.BudgetPeriod, error) { + defer r.enter("ListPeriods")() + return r.periods, nil +} + +func (r *countingAllUserDataRepo) GetDefaults(context.Context, string) (*model.DefaultSettings, error) { + defer r.enter("GetDefaults")() + return r.defaults, nil +} + +// maxConcurrent reports the peak number of reads observed in flight at once. +func (r *countingAllUserDataRepo) maxConcurrent() int { + r.mu.Lock() + defer r.mu.Unlock() + return r.maxInFlight +} diff --git a/services/finance/internal/service/dashboard.go b/services/finance/internal/service/dashboard.go index fb292051..edbe048d 100644 --- a/services/finance/internal/service/dashboard.go +++ b/services/finance/internal/service/dashboard.go @@ -7,6 +7,8 @@ import ( "sort" "time" + "golang.org/x/sync/errgroup" + "github.com/ItsThompson/gofin/services/finance/internal/model" ) @@ -112,24 +114,37 @@ func (s *FinanceService) GetSpendingTrends(ctx context.Context, userID string, y window[i], window[j] = window[j], window[i] } - // Fetch expenses for each month and build input arrays for the pure function + // Fetch expenses for each month and build input arrays for the pure function. + // The per-month reads are independent and write only their own index slot, so + // they fan out under a bounded errgroup; the pure + // ComputeSpendingTrends aggregation runs after the g.Wait() barrier. periodSlice := make([]*model.BudgetPeriod, len(window)) expenseSlice := make([][]ExpenseData, len(window)) yearSlice := make([]int32, len(window)) monthSlice := make([]int32, len(window)) + g, gctx := errgroup.WithContext(ctx) + g.SetLimit(dashboardFanoutLimit) for i, ym := range window { + i, ym := i, ym yearSlice[i] = ym.year monthSlice[i] = ym.month p := periodMap[[2]int32{ym.year, ym.month}] periodSlice[i] = p - if p != nil { - var err error - expenseSlice[i], err = s.expenseClient.GetExpensesForPeriod(ctx, userID, ym.year, ym.month) + if p == nil { + continue // no read for missing periods; slot stays empty + } + g.Go(func() error { + exps, err := s.expenseClient.GetExpensesForPeriod(gctx, userID, ym.year, ym.month) if err != nil { - return nil, fmt.Errorf("fetching expenses for %d-%02d: %w", ym.year, ym.month, err) + return fmt.Errorf("fetching expenses for %d-%02d: %w", ym.year, ym.month, err) } - } + expenseSlice[i] = exps // own slot only + return nil + }) + } + if err := g.Wait(); err != nil { + return nil, err } return ComputeSpendingTrends(periodSlice, expenseSlice, yearSlice, monthSlice), nil @@ -213,49 +228,67 @@ func (s *FinanceService) computeHistoricalComparison( return nil, fmt.Errorf("requested period not found in list") } - // Get current period's total spent - currentSpent, err := s.getTotalSpentForPeriod(ctx, userID, periods[requestedIdx]) - if err != nil { - return nil, err - } - // Collect up to 3 prior periods (the ones after requestedIdx in DESC order) var priorPeriods []*model.BudgetPeriod for i := requestedIdx + 1; i < len(periods) && len(priorPeriods) < 3; i++ { priorPeriods = append(priorPeriods, periods[i]) } + // Build the distinct set of periods whose spend the result actually needs, + // mirroring the serial reads: the current period always; priorPeriods[0] when + // any prior exists (it feeds both "previous" and the first rolling-average + // term); priorPeriods[1..2] only when a full 3-period rolling average is + // possible. Each period is therefore read exactly once (the serial code read + // priorPeriods[0] twice). These reads are independent, so they fan out under a + // bounded errgroup with index-addressed result slots; the change-percent and + // rolling-average math runs after the g.Wait() barrier, unchanged. + hasRollingAverage := len(priorPeriods) >= 3 + targets := []*model.BudgetPeriod{periods[requestedIdx]} + if len(priorPeriods) > 0 { + targets = append(targets, priorPeriods[0]) + } + if hasRollingAverage { + targets = append(targets, priorPeriods[1], priorPeriods[2]) + } + + spent := make([]int64, len(targets)) + g, gctx := errgroup.WithContext(ctx) + g.SetLimit(dashboardFanoutLimit) + for i, p := range targets { + i, p := i, p + g.Go(func() error { + v, err := s.getTotalSpentForPeriod(gctx, userID, p) + if err != nil { + return err // already wrapped with the period by getTotalSpentForPeriod + } + spent[i] = v // own slot only + return nil + }) + } + if err := g.Wait(); err != nil { + return nil, err + } + result := &model.HistoricalComparison{ - CurrentSpent: currentSpent, + CurrentSpent: spent[0], } // Previous period spent if len(priorPeriods) > 0 { - prevSpent, err := s.getTotalSpentForPeriod(ctx, userID, priorPeriods[0]) - if err != nil { - return nil, err - } + prevSpent := spent[1] result.PreviousSpent = prevSpent // Change percent if prevSpent > 0 { - result.ChangePercent = math.Round(float64(currentSpent-prevSpent)/float64(prevSpent)*10000) / 100 - } else if currentSpent > 0 { + result.ChangePercent = math.Round(float64(spent[0]-prevSpent)/float64(prevSpent)*10000) / 100 + } else if spent[0] > 0 { result.ChangePercent = 100.0 } } // Rolling average: need at least 3 prior periods - if len(priorPeriods) >= 3 { - var totalForAvg int64 - for _, p := range priorPeriods[:3] { - spent, err := s.getTotalSpentForPeriod(ctx, userID, p) - if err != nil { - return nil, err - } - totalForAvg += spent - } - avg := totalForAvg / 3 + if hasRollingAverage { + avg := (spent[1] + spent[2] + spent[3]) / 3 result.RollingAverage = &avg } diff --git a/services/finance/internal/service/dashboard_bench_test.go b/services/finance/internal/service/dashboard_bench_test.go new file mode 100644 index 00000000..72b7a1d9 --- /dev/null +++ b/services/finance/internal/service/dashboard_bench_test.go @@ -0,0 +1,81 @@ +package service + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/ItsThompson/gofin/services/finance/internal/model" +) + +// benchReadLatency is a synthetic per-read upstream latency baked into the +// dashboard fan-out benchmarks so wall-clock reflects the fan-out shape +// (total ≈ max·ceil(n/limit)) rather than the serial sum. It is a benchmark knob +// only; correctness tests run the fake with zero delay. +const benchReadLatency = 200 * time.Microsecond + +// seedYearPeriods seeds twelve 2026 budget periods (DESC order, mirroring the +// repository's ListPeriods contract) and per-period expenses on exp, returning +// the periods for the fake repo. +func seedYearPeriods(exp *countingExpenseClient) []*model.BudgetPeriod { + periods := make([]*model.BudgetPeriod, 0, 12) + for m := int32(12); m >= 1; m-- { + periods = append(periods, &model.BudgetPeriod{ + ID: fmt.Sprintf("p-2026-%02d", m), + UserID: "user-1", + Year: 2026, + Month: m, + BudgetAmount: 300000, + EssentialsPercent: 50, + DesiresPercent: 30, + SavingsPercent: 20, + }) + exp.set(2026, m, []ExpenseData{ + {Amount: int64(m) * 1000, ExpenseType: "essentials"}, + {Amount: int64(m) * 500, ExpenseType: "desires"}, + }) + } + return periods +} + +// BenchmarkGetSpendingTrends measures the up-to-12-read trends path at window +// sizes 1, 6, and 12. Primary signal: expense call count (one read per non-nil +// period) and wall-clock (fan-out max vs serial sum). +func BenchmarkGetSpendingTrends(b *testing.B) { + for _, months := range []int32{1, 6, 12} { + b.Run(fmt.Sprintf("months=%d", months), func(b *testing.B) { + exp := newCountingExpenseClient() + exp.delay = benchReadLatency + repo := &fakeFanoutRepo{periods: seedYearPeriods(exp)} + svc := newFanoutService(repo, exp) + + b.ReportAllocs() + for b.Loop() { + if _, err := svc.GetSpendingTrends(context.Background(), "user-1", 2026, 12, months); err != nil { + b.Fatal(err) + } + } + }) + } +} + +// BenchmarkGetHistoricalComparison measures the current + up-to-3-prior read +// path (≤4 reads). Primary signal: expense call count and wall-clock. +func BenchmarkGetHistoricalComparison(b *testing.B) { + exp := newCountingExpenseClient() + exp.delay = benchReadLatency + periods := seedYearPeriods(exp) + repo := &fakeFanoutRepo{ + periods: periods, + current: map[[2]int32]*model.BudgetPeriod{periodKey(2026, 12): periods[0]}, + } + svc := newFanoutService(repo, exp) + + b.ReportAllocs() + for b.Loop() { + if _, err := svc.GetHistoricalComparison(context.Background(), "user-1", 2026, 12); err != nil { + b.Fatal(err) + } + } +} diff --git a/services/finance/internal/service/dashboard_test.go b/services/finance/internal/service/dashboard_test.go index 7c39e963..7b96d239 100644 --- a/services/finance/internal/service/dashboard_test.go +++ b/services/finance/internal/service/dashboard_test.go @@ -1,13 +1,21 @@ package service import ( + "context" + "errors" "fmt" + "io" + "log/slog" + "sync" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/ItsThompson/gofin/services/finance/internal/model" + "github.com/ItsThompson/gofin/services/finance/internal/repository" + "github.com/ItsThompson/gofin/services/perf" ) // historicalNow is a fixed time far in the future, ensuring any test month @@ -568,3 +576,319 @@ func TestComputeSpendingTrends_NilPeriodReturnsZeros(t *testing.T) { assert.Equal(t, float64(0), result[0].DesiresPercent) assert.Equal(t, float64(0), result[0].SavingsPercent) } + +// --- Fan-out test infrastructure (shared by regression tests and benchmarks) --- + +// periodKey is the {year, month} lookup key used across the fan-out fakes. +func periodKey(year, month int32) [2]int32 { return [2]int32{year, month} } + +// monthOp is the CallCounter operation name for a period read. +func monthOp(year, month int32) string { return fmt.Sprintf("%d-%02d", year, month) } + +// countingExpenseClient is a concurrency-aware fake ExpenseClient shared by the +// dashboard fan-out regression tests and benchmarks. It records one call per +// (year, month) through an embedded *perf.CallCounter, tracks the maximum number +// of reads in flight simultaneously (so the SetLimit bound can be asserted), +// checks its context on entry so a read launched after a sibling failure returns +// without recording (exercising first-error cancellation alongside the goroutine +// error-wrapping path), and can simulate per-read latency so benchmarks show +// fan-out (max) rather than serial (sum) wall-clock. The latency uses a plain +// time.Sleep so it does not allocate, keeping the benchmark's allocs/op a clean +// measure of the production fan-out cost. All methods are safe for concurrent use. +type countingExpenseClient struct { + counter *perf.CallCounter + delay time.Duration + byMonth map[[2]int32][]ExpenseData + errs map[[2]int32]error + + mu sync.Mutex + inFlight int + maxInFlight int +} + +func newCountingExpenseClient() *countingExpenseClient { + return &countingExpenseClient{ + counter: perf.NewCallCounter(), + byMonth: make(map[[2]int32][]ExpenseData), + errs: make(map[[2]int32]error), + } +} + +// set seeds the expenses returned for a period. Call before the fan-out runs; +// the maps are read-only during concurrent reads. +func (c *countingExpenseClient) set(year, month int32, expenses []ExpenseData) { + c.byMonth[periodKey(year, month)] = expenses +} + +// failOn makes the read for a period return err, exercising the goroutine +// error-wrapping path. Call before the fan-out runs. +func (c *countingExpenseClient) failOn(year, month int32, err error) { + c.errs[periodKey(year, month)] = err +} + +func (c *countingExpenseClient) GetExpensesForPeriod(ctx context.Context, _ string, year, month int32) ([]ExpenseData, error) { + // Check cancellation before recording: a read launched after a sibling has + // already failed (and cancelled the errgroup's derived context) returns + // without recording, so tests can assert siblings were cancelled via the + // call count. A goroutine handed the parent ctx instead of gctx would not + // see this cancellation. + if err := ctx.Err(); err != nil { + return nil, err + } + c.counter.Record(monthOp(year, month)) + + c.mu.Lock() + c.inFlight++ + if c.inFlight > c.maxInFlight { + c.maxInFlight = c.inFlight + } + c.mu.Unlock() + defer func() { + c.mu.Lock() + c.inFlight-- + c.mu.Unlock() + }() + + // Fail fast, before the synthetic delay, so an injected error cancels the + // group while sibling reads are still in flight. + if err := c.errs[periodKey(year, month)]; err != nil { + return nil, err + } + + if c.delay > 0 { + time.Sleep(c.delay) + } + return c.byMonth[periodKey(year, month)], nil +} + +// maxConcurrent reports the peak number of reads observed in flight at once. +func (c *countingExpenseClient) maxConcurrent() int { + c.mu.Lock() + defer c.mu.Unlock() + return c.maxInFlight +} + +// CountExpensesByTag and CreateExpense are not used by the dashboard read paths; +// they fail loudly if a path unexpectedly calls them. +func (c *countingExpenseClient) CountExpensesByTag(context.Context, string, string) (int64, error) { + return 0, fmt.Errorf("CountExpensesByTag not expected in dashboard fan-out tests") +} + +func (c *countingExpenseClient) CreateExpense(context.Context, CreateExpenseInput) (*CreatedExpenseData, error) { + return nil, fmt.Errorf("CreateExpense not expected in dashboard fan-out tests") +} + +// fakeFanoutRepo returns canned periods for the dashboard read paths. Only the +// two methods those paths call (ListPeriods, GetCurrentPeriod) are implemented; +// the embedded interface is nil, so any other repo call panics and surfaces an +// accidental extra read. +type fakeFanoutRepo struct { + repository.FinanceRepository + periods []*model.BudgetPeriod + current map[[2]int32]*model.BudgetPeriod +} + +func (r *fakeFanoutRepo) ListPeriods(context.Context, string) ([]*model.BudgetPeriod, error) { + return r.periods, nil +} + +func (r *fakeFanoutRepo) GetCurrentPeriod(_ context.Context, _ string, year, month int32) (*model.BudgetPeriod, error) { + return r.current[periodKey(year, month)], nil +} + +// newFanoutService wires a FinanceService for fan-out tests/benchmarks with a +// discarded logger and no transaction beginner (the read paths never open a tx). +func newFanoutService(repo repository.FinanceRepository, exp ExpenseClient) *FinanceService { + logger := slog.New(slog.NewJSONHandler(io.Discard, nil)) + return NewFinanceService(repo, nil, logger).WithExpenseClient(exp) +} + +// --- GetSpendingTrends fan-out regression tests --- + +// TestGetSpendingTrends_FanOutByteIdentical seeds a full 2026 window with two +// gaps (no period for April/September) and asserts the fan-out produces the +// serial result: chronological order preserved, per-month aggregation correct, +// gaps left as zero slots, exactly one read per non-nil period, and no read for +// the gaps. +func TestGetSpendingTrends_FanOutByteIdentical(t *testing.T) { + exp := newCountingExpenseClient() + var periods []*model.BudgetPeriod + nonNil := 0 + for m := int32(12); m >= 1; m-- { + if m == 4 || m == 9 { + continue // gap: no BudgetPeriod for this month + } + nonNil++ + periods = append(periods, &model.BudgetPeriod{ + ID: fmt.Sprintf("p-2026-%02d", m), + UserID: "user-1", + Year: 2026, + Month: m, + BudgetAmount: 300000, + EssentialsPercent: 50, + DesiresPercent: 30, + SavingsPercent: 20, + }) + exp.set(2026, m, []ExpenseData{ + {Amount: int64(m) * 1000, ExpenseType: "essentials"}, + {Amount: int64(m) * 500, ExpenseType: "desires"}, + }) + } + svc := newFanoutService(&fakeFanoutRepo{periods: periods}, exp) + + result, err := svc.GetSpendingTrends(context.Background(), "user-1", 2026, 12, 12) + require.NoError(t, err) + require.Len(t, result, 12) + + for i := 0; i < 12; i++ { + month := int32(i + 1) + assert.Equal(t, int32(2026), result[i].Year) + assert.Equal(t, month, result[i].Month, "results must stay in chronological (index) order") + if month == 4 || month == 9 { + assert.Equal(t, int64(0), result[i].TotalSpent) + assert.Equal(t, int64(0), result[i].BudgetAmount) + assert.Equal(t, 0, exp.counter.Count(monthOp(2026, month)), "gap month must issue no read") + continue + } + assert.Equal(t, int64(month)*1500, result[i].TotalSpent) + assert.Equal(t, int64(month)*1000, result[i].EssentialsSpent) + assert.Equal(t, int64(month)*500, result[i].DesiresSpent) + assert.Equal(t, int64(300000), result[i].BudgetAmount) + assert.Equal(t, 1, exp.counter.Count(monthOp(2026, month)), "one read per non-nil period") + } + assert.Equal(t, nonNil, exp.counter.Total(), "total reads must equal the non-nil period count") +} + +// TestGetSpendingTrends_FanOutRespectsLimit confirms the 12-wide window overlaps +// reads (not serial) while never exceeding SetLimit(dashboardFanoutLimit). +func TestGetSpendingTrends_FanOutRespectsLimit(t *testing.T) { + exp := newCountingExpenseClient() + exp.delay = 5 * time.Millisecond + svc := newFanoutService(&fakeFanoutRepo{periods: seedYearPeriods(exp)}, exp) + + _, err := svc.GetSpendingTrends(context.Background(), "user-1", 2026, 12, 12) + require.NoError(t, err) + + assert.LessOrEqual(t, exp.maxConcurrent(), dashboardFanoutLimit, + "in-flight reads must not exceed SetLimit(dashboardFanoutLimit)") + assert.Greater(t, exp.maxConcurrent(), 1, "reads should overlap (fan-out), not run serially") +} + +// TestGetSpendingTrends_FanOutWrapsError confirms a per-month read failure is +// surfaced with the period baked into the error (wrapped inside the goroutine). +func TestGetSpendingTrends_FanOutWrapsError(t *testing.T) { + exp := newCountingExpenseClient() + exp.failOn(2026, 7, errors.New("boom")) + svc := newFanoutService(&fakeFanoutRepo{periods: seedYearPeriods(exp)}, exp) + + _, err := svc.GetSpendingTrends(context.Background(), "user-1", 2026, 12, 12) + require.Error(t, err) + assert.Contains(t, err.Error(), "2026-07", "error must name the failing period") + assert.Contains(t, err.Error(), "boom") +} + +// TestGetSpendingTrends_FanOutCancelsSiblings proves the errgroup's first-error +// cancellation reaches sibling reads: when one period read fails fast, the +// derived gctx is cancelled, so reads not yet started return early (via the +// entry-time context check) instead of every period being read to completion. +// If the production goroutines passed the parent ctx (not gctx) to +// GetExpensesForPeriod, the siblings would not observe the cancellation and all +// twelve reads would complete, failing this assertion. This is the spec-09 +// sibling-cancellation row the other fan-out tests miss. +func TestGetSpendingTrends_FanOutCancelsSiblings(t *testing.T) { + const window = 12 + exp := newCountingExpenseClient() + exp.delay = 20 * time.Millisecond // keep the first wave in flight past the failure + exp.failOn(2026, 1, errors.New("boom")) // window[0]: fails fast, before its delay + svc := newFanoutService(&fakeFanoutRepo{periods: seedYearPeriods(exp)}, exp) + + _, err := svc.GetSpendingTrends(context.Background(), "user-1", 2026, 12, window) + require.Error(t, err) + assert.Less(t, exp.counter.Total(), window, + "first-error must cancel sibling reads; not every period should be read to completion") +} + +// --- GetHistoricalComparison fan-out regression tests --- + +func historicalRepo(periods []*model.BudgetPeriod) *fakeFanoutRepo { + return &fakeFanoutRepo{ + periods: periods, + current: map[[2]int32]*model.BudgetPeriod{periodKey(periods[0].Year, periods[0].Month): periods[0]}, + } +} + +// TestGetHistoricalComparison_FanOutByteIdentical asserts the fan-out yields the +// serial values (current/previous/change/rolling) while reading each distinct +// needed period exactly once. The serial path read priorPeriods[0] twice; the +// fan-out reads it once, and the unused 4th prior is never read. +func TestGetHistoricalComparison_FanOutByteIdentical(t *testing.T) { + exp := newCountingExpenseClient() + periods := []*model.BudgetPeriod{ + makePeriod("p12", 2026, 12), + makePeriod("p11", 2026, 11), + makePeriod("p10", 2026, 10), + makePeriod("p09", 2026, 9), + makePeriod("p08", 2026, 8), + } + exp.set(2026, 12, []ExpenseData{{Amount: 80000}}) + exp.set(2026, 11, []ExpenseData{{Amount: 70000}}) + exp.set(2026, 10, []ExpenseData{{Amount: 60000}}) + exp.set(2026, 9, []ExpenseData{{Amount: 50000}}) + exp.set(2026, 8, []ExpenseData{{Amount: 40000}}) + svc := newFanoutService(historicalRepo(periods), exp) + + result, err := svc.GetHistoricalComparison(context.Background(), "user-1", 2026, 12) + require.NoError(t, err) + assert.Equal(t, int64(80000), result.CurrentSpent) + assert.Equal(t, int64(70000), result.PreviousSpent) + require.NotNil(t, result.RollingAverage) + assert.Equal(t, int64(60000), *result.RollingAverage) // (70000+60000+50000)/3 + assert.InDelta(t, 14.29, result.ChangePercent, 0.01) // (80000-70000)/70000 + + assert.Equal(t, 1, exp.counter.Count(monthOp(2026, 12))) + assert.Equal(t, 1, exp.counter.Count(monthOp(2026, 11)), "priorPeriods[0] read once, not twice") + assert.Equal(t, 1, exp.counter.Count(monthOp(2026, 10))) + assert.Equal(t, 1, exp.counter.Count(monthOp(2026, 9))) + assert.Equal(t, 0, exp.counter.Count(monthOp(2026, 8)), "4th prior is never needed, never read") + assert.Equal(t, 4, exp.counter.Total(), "at most 4 distinct period reads") +} + +// TestGetHistoricalComparison_FanOutRunsConcurrently confirms the current and +// prior reads overlap while staying within the SetLimit bound. +func TestGetHistoricalComparison_FanOutRunsConcurrently(t *testing.T) { + exp := newCountingExpenseClient() + exp.delay = 5 * time.Millisecond + periods := []*model.BudgetPeriod{ + makePeriod("p12", 2026, 12), makePeriod("p11", 2026, 11), + makePeriod("p10", 2026, 10), makePeriod("p09", 2026, 9), + } + for m := int32(9); m <= 12; m++ { + exp.set(2026, m, []ExpenseData{{Amount: int64(m) * 1000}}) + } + svc := newFanoutService(historicalRepo(periods), exp) + + _, err := svc.GetHistoricalComparison(context.Background(), "user-1", 2026, 12) + require.NoError(t, err) + assert.LessOrEqual(t, exp.maxConcurrent(), dashboardFanoutLimit) + assert.Greater(t, exp.maxConcurrent(), 1, "prior reads should overlap the current read") +} + +// TestGetHistoricalComparison_FanOutWrapsError confirms a prior-period read +// failure is surfaced with the period named (wrapped inside the goroutine). +func TestGetHistoricalComparison_FanOutWrapsError(t *testing.T) { + exp := newCountingExpenseClient() + periods := []*model.BudgetPeriod{ + makePeriod("p12", 2026, 12), makePeriod("p11", 2026, 11), + makePeriod("p10", 2026, 10), makePeriod("p09", 2026, 9), + } + for m := int32(9); m <= 12; m++ { + exp.set(2026, m, []ExpenseData{{Amount: int64(m) * 1000}}) + } + exp.failOn(2026, 10, errors.New("upstream down")) + svc := newFanoutService(historicalRepo(periods), exp) + + _, err := svc.GetHistoricalComparison(context.Background(), "user-1", 2026, 12) + require.Error(t, err) + assert.Contains(t, err.Error(), "2026-10", "error must name the failing period") + assert.Contains(t, err.Error(), "upstream down") +} diff --git a/services/finance/internal/service/fanout.go b/services/finance/internal/service/fanout.go new file mode 100644 index 00000000..217fb310 --- /dev/null +++ b/services/finance/internal/service/fanout.go @@ -0,0 +1,15 @@ +package service + +// dashboardFanoutLimit is the uniform cap on concurrent upstream reads shared by +// every fan-out path in this service (GetSpendingTrends, GetHistoricalComparison, +// and GetAllUserData). It is a domain truth, not a per-call literal, so the paths +// share one tunable bound rather than each choosing its own. +// +// The cap is sized for the pgxpool that GetAllUserData's repository reads draw +// from: its default max connections is max(4, NumCPU) and each in-flight pg read +// checks out one connection, so a wide pg fan-out could starve the pool. The +// dashboard paths fan out gRPC GetExpensesForPeriod reads (which consume no pg +// connections); they adopt the same bound for uniformity and to keep the widest +// window (the 12-month trends fan-out) from bursting the expense service. The +// value sits in the 4-6 band. +const dashboardFanoutLimit = 5 diff --git a/services/finance/perf/baseline/alluserdata.txt b/services/finance/perf/baseline/alluserdata.txt new file mode 100644 index 00000000..de60f226 --- /dev/null +++ b/services/finance/perf/baseline/alluserdata.txt @@ -0,0 +1,33 @@ +# gofin perf baseline +path: finance/alluserdata (GetAllUserData, 3-read fan-out) +captured_against: ft/latency-refactor (pre-change: serial ListTags -> ListPeriods -> GetDefaults reads) +machine: Apple M4 Pro, 14 cores, macOS 26.3.1 (wall-clock reference only, arm64) +go: go1.26.2 +date: 2026-07-10 +benchmark_note: reads use a synthetic 200µs per-read latency (allUserDataReadLatency + in alluserdata_bench_test.go) so wall-clock reflects serial sum (3 + reads) vs fan-out max (~1 read); absolute ns/op is a same-machine + reference, never a CI threshold. + +# Environment-independent (portable; the durable signal, gated by regression tests) +# GetAllUserData issues exactly one read each of tags, periods, and defaults, +# regardless of serial vs fan-out (the export contract is unchanged): +repo_calls@GetAllUserData(ListTags): 1 +repo_calls@GetAllUserData(ListPeriods): 1 +repo_calls@GetAllUserData(GetDefaults): 1 +total_reads@GetAllUserData: 3 +max_reads_in_flight(serial): 1 (fully sequential) + +# Wall-clock reference (same-machine only, NOT a threshold; count=5 mean) +# serial ~= sum of the three reads (~3 * 200µs + assembly overhead): +ns_op@GetAllUserData(serial): ~700,000 (4 allocs/op, 112 B/op) + +# Expected post-change shape (bounded errgroup, dashboardFanoutLimit=5): +# - call counts unchanged (1 each of tags, periods, defaults; total 3) +# - assembled AllUserData byte-identical to the serial version +# - wall-clock ~ max of the three reads (~200µs) since limit=5 >= 3 reads all +# overlap: ~3x faster on this synthetic profile +# - max_reads_in_flight rises to 3 (all three reads overlap; asserted <= 5 and +# > 1 in the fan-out regression tests) +# - allocs rise modestly (errgroup + goroutine bookkeeping); not gated +# Re-capture on the same machine after the change; diff via benchstat old.txt new.txt. diff --git a/services/finance/perf/baseline/dashboard.txt b/services/finance/perf/baseline/dashboard.txt new file mode 100644 index 00000000..7c3d0b85 --- /dev/null +++ b/services/finance/perf/baseline/dashboard.txt @@ -0,0 +1,53 @@ +# gofin perf baseline +path: finance/dashboard (GetSpendingTrends + GetHistoricalComparison, fan-out) +captured_against: main @ 8baca69 (pre-optimization: serial GetExpensesForPeriod reads) +machine: Apple M4 Pro, 14 cores, macOS 26.3.1 (wall-clock reference only, arm64) +go: go1.26.2 +date: 2026-07-11 +benchmark_note: reads use a synthetic 200µs per-read latency (benchReadLatency in + dashboard_bench_test.go) so wall-clock reflects serial sum vs + fan-out max·ceil(n/limit); absolute ns/op is a same-machine + reference, never a CI threshold. + +# Environment-independent (portable; the durable signal, gated by regression tests) +# GetSpendingTrends issues exactly one GetExpensesForPeriod per non-nil period in +# the window (nil/missing periods issue no read): +expense_calls@GetSpendingTrends(months=1): 1 +expense_calls@GetSpendingTrends(months=6): 6 +expense_calls@GetSpendingTrends(months=12): 12 +# GetHistoricalComparison (serial) reads current + previous + up-to-3 rolling. +# The serial path reads priorPeriods[0] TWICE (once for "previous", once as the +# first rolling-average term), so with >=3 priors it issues 5 reads for 4 +# distinct periods: +expense_calls@GetHistoricalComparison(>=3 priors, serial): 5 (4 distinct periods) +expense_calls@GetHistoricalComparison(2 priors, serial): 2 (current + previous) +expense_calls@GetHistoricalComparison(0 priors, serial): 1 (current only) +max_reads_in_flight(serial): 1 (fully sequential) + +# Wall-clock reference (same-machine only, NOT a threshold; count=10 mean per +# perf-methodology §3, re-captured 2026-07-11 on the machine above). +# The serial (pre-fan-out) side is reproducible by checking out commit 6036abb, +# where dashboard.go still issues serial reads; re-run HEAD for the fan-out side. +# +# Serial (checkout 6036abb): +ns_op@GetSpendingTrends(months=1): ~237,000 (6 allocs/op, 424 B/op) +ns_op@GetSpendingTrends(months=6): ~1,418,000 (19 allocs/op, 1112 B/op) +ns_op@GetSpendingTrends(months=12): ~2,834,000 (33 allocs/op, 1994 B/op) +ns_op@GetHistoricalComparison: ~1,182,000 (12 allocs/op, 112 B/op) +# +# Fan-out (HEAD, dashboardFanoutLimit=5): +ns_op@GetSpendingTrends(months=1): ~243,000 (14 allocs/op, 936 B/op) # 1 read: errgroup overhead, no parallelism gain +ns_op@GetSpendingTrends(months=6): ~482,000 (41 allocs/op, 2685 B/op) # ~2.9x faster +ns_op@GetSpendingTrends(months=12): ~722,000 (73 allocs/op, 4860 B/op) # ~3.9x faster +ns_op@GetHistoricalComparison: ~247,000 (29 allocs/op, 1313 B/op) # ~4.8x faster + +# Expected post-change shape (bounded errgroup, dashboardFanoutLimit=5): +# - call counts unchanged for trends (1 per non-nil period) +# - historical drops to <=4 reads (distinct periods; priorPeriods[0] read once) +# - wall-clock ~ max·ceil(n/limit): months=12 drops ~3.9x, historical ~4.8x; +# months=1 is flat (one read, so errgroup setup makes it marginally slower) +# - max_reads_in_flight rises to <= dashboardFanoutLimit (asserted in tests) +# - allocs/op RISES (errgroup + up to N goroutine closures + per-read wrapped +# error funcs): months=12 33->73, historical 12->29. This is the documented +# cost of the latency win; a same-machine reference, not gated (spec 04). +# Re-capture on the same machine after the change; diff via benchstat old.txt new.txt. diff --git a/services/gateway/cmd/grpc_validator.go b/services/gateway/cmd/grpc_validator.go index c371570c..ff772b4d 100644 --- a/services/gateway/cmd/grpc_validator.go +++ b/services/gateway/cmd/grpc_validator.go @@ -3,6 +3,7 @@ package main import ( "context" "fmt" + "time" "google.golang.org/grpc" @@ -10,22 +11,43 @@ import ( "github.com/ItsThompson/gofin/services/gateway/internal/access" ) +// validateTokenClient is the narrow slice of authpb.AuthServiceClient that +// GRPCTokenValidator depends on. Depending on the one RPC it calls (rather than +// the full client) keeps the validator testable with a fake that implements +// only ValidateToken; authpb.AuthServiceClient satisfies it structurally. +type validateTokenClient interface { + ValidateToken(ctx context.Context, in *authpb.ValidateTokenRequest, opts ...grpc.CallOption) (*authpb.ValidateTokenResponse, error) +} + // GRPCTokenValidator implements access.TokenValidator by calling the // auth service's ValidateToken gRPC endpoint. type GRPCTokenValidator struct { - client authpb.AuthServiceClient + client validateTokenClient + timeout time.Duration } // NewGRPCTokenValidator creates a validator backed by the given gRPC connection. -func NewGRPCTokenValidator(conn *grpc.ClientConn) *GRPCTokenValidator { +// timeout bounds every ValidateToken call so a hung auth service returns a fast +// deadline error instead of blocking the gateway worker indefinitely. +func NewGRPCTokenValidator(conn *grpc.ClientConn, timeout time.Duration) *GRPCTokenValidator { return &GRPCTokenValidator{ - client: authpb.NewAuthServiceClient(conn), + client: authpb.NewAuthServiceClient(conn), + timeout: timeout, } } // ValidateToken calls the auth service to validate an access token. // Returns the user identity on success or an error on failure. +// +// The call is bounded by a per-call timeout derived from the incoming request +// context, so a client disconnect still cancels it immediately while a hung +// auth service is capped at v.timeout. A timeout surfaces as an error that +// unwraps to context.DeadlineExceeded (and carries gRPC codes.DeadlineExceeded), +// which AccessControl maps to 503 rather than 401. func (v *GRPCTokenValidator) ValidateToken(ctx context.Context, accessToken string) (*access.TokenValidationResult, error) { + ctx, cancel := context.WithTimeout(ctx, v.timeout) + defer cancel() + resp, err := v.client.ValidateToken(ctx, &authpb.ValidateTokenRequest{ AccessToken: accessToken, }) diff --git a/services/gateway/cmd/grpc_validator_test.go b/services/gateway/cmd/grpc_validator_test.go new file mode 100644 index 00000000..ca80801c --- /dev/null +++ b/services/gateway/cmd/grpc_validator_test.go @@ -0,0 +1,232 @@ +package main + +import ( + "context" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/ItsThompson/gofin/services/auth/proto/authpb" + "github.com/ItsThompson/gofin/services/gateway/internal/access" +) + +// hangingAuthClient simulates a hung auth service: ValidateToken blocks until +// its context is cancelled (by the bounded-timeout wrap), then returns the gRPC +// DeadlineExceeded status the real client surfaces on a deadline. hardStop lets +// the test drain the goroutine during cleanup if the wrap is ever missing, so a +// reverted wrap fails the test instead of leaking a goroutine forever. +type hangingAuthClient struct { + hardStop <-chan struct{} +} + +func (h *hangingAuthClient) ValidateToken(ctx context.Context, _ *authpb.ValidateTokenRequest, _ ...grpc.CallOption) (*authpb.ValidateTokenResponse, error) { + select { + case <-ctx.Done(): + return nil, status.FromContextError(ctx.Err()).Err() + case <-h.hardStop: + return nil, errors.New("test cleanup") + } +} + +// TestGRPCTokenValidator_HangingBackend_ReturnsWithinTimeout is the wrap +// regression guard: with the per-call context.WithTimeout in place, a hung auth +// backend returns within the configured timeout (plus margin) with the gRPC +// DeadlineExceeded status the real client surfaces on a deadline. Reverting the +// wrap passes the unbounded background context to the client, which blocks +// forever, so this test hangs until it hits the deadline below and fails. +func TestGRPCTokenValidator_HangingBackend_ReturnsWithinTimeout(t *testing.T) { + const timeout = 50 * time.Millisecond + + hardStop := make(chan struct{}) + t.Cleanup(func() { close(hardStop) }) + + validator := &GRPCTokenValidator{ + client: &hangingAuthClient{hardStop: hardStop}, + timeout: timeout, + } + + done := make(chan error, 1) + go func() { + _, err := validator.ValidateToken(context.Background(), "token") + done <- err + }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected a deadline error from the hung backend, got nil") + } + // The real client surfaces a context deadline as a gRPC status error, + // which does not unwrap to context.DeadlineExceeded; the gRPC-code prong + // (mirrored in AccessControl) is what classifies it as a timeout. Assert + // it survives the fmt.Errorf %w wrap in ValidateToken. + if st, ok := status.FromError(err); !ok || st.Code() != codes.DeadlineExceeded { + t.Errorf("error does not carry gRPC DeadlineExceeded through the %%w wrap: ok=%v err=%v", ok, err) + } + case <-time.After(timeout + 500*time.Millisecond): + t.Fatal("ValidateToken did not return within the timeout window; the context.WithTimeout bound is missing") + } +} + +// TestGatewayValidateTimeout_EndToEnd_Returns503 wires the production path: +// AccessControl in front of a real GRPCTokenValidator backed by a hung auth +// client. It is the behavioral regression guard for the whole feature: with the +// bound in place, a hung backend makes AccessControl return 503 +// SERVICE_UNAVAILABLE within the timeout window (not after client disconnect); +// reverting the context.WithTimeout wrap leaves the client blocked on an +// unbounded context, so ServeHTTP never returns and this test fails at the +// deadline below. +func TestGatewayValidateTimeout_EndToEnd_Returns503(t *testing.T) { + gin.SetMode(gin.TestMode) + const timeout = 50 * time.Millisecond + + hardStop := make(chan struct{}) + t.Cleanup(func() { close(hardStop) }) + + validator := &GRPCTokenValidator{ + client: &hangingAuthClient{hardStop: hardStop}, + timeout: timeout, + } + + engine := gin.New() + engine.Use(access.AccessControl(validator, access.GatewayResolve, slog.New(slog.NewTextHandler(io.Discard, nil)))) + engine.GET("/api/finance/periods", func(c *gin.Context) { c.Status(http.StatusOK) }) + + req := httptest.NewRequest(http.MethodGet, "/api/finance/periods", nil) + req.AddCookie(&http.Cookie{Name: "gofin_access", Value: "token"}) + rec := httptest.NewRecorder() + + done := make(chan struct{}) + go func() { + engine.ServeHTTP(rec, req) + close(done) + }() + + select { + case <-done: + if rec.Code != http.StatusServiceUnavailable { + t.Errorf("status = %d, want 503 Service Unavailable", rec.Code) + } + if !strings.Contains(rec.Body.String(), "SERVICE_UNAVAILABLE") { + t.Errorf("body = %q, want it to contain SERVICE_UNAVAILABLE", rec.Body.String()) + } + case <-time.After(timeout + 500*time.Millisecond): + t.Fatal("AccessControl did not return within the timeout window; the ValidateToken bound is missing") + } +} + +// stubAuthClient is a validateTokenClient returning a canned response/error +// without touching gRPC, so the concrete GRPCTokenValidator's field mapping and +// error wrapping are exercised directly. The access package's tests use a +// fakeValidator and never run the real validator, so this closes that gap. +type stubAuthClient struct { + resp *authpb.ValidateTokenResponse + err error +} + +func (s *stubAuthClient) ValidateToken(_ context.Context, _ *authpb.ValidateTokenRequest, _ ...grpc.CallOption) (*authpb.ValidateTokenResponse, error) { + return s.resp, s.err +} + +// TestGRPCTokenValidator_MapsResponseFields covers the success-path mapping from +// authpb.ValidateTokenResponse to access.TokenValidationResult, including the +// assumed-session case where AssumedBy is populated. +func TestGRPCTokenValidator_MapsResponseFields(t *testing.T) { + cases := []struct { + name string + resp *authpb.ValidateTokenResponse + want access.TokenValidationResult + }{ + { + name: "plain user session", + resp: &authpb.ValidateTokenResponse{UserId: "user-1", Role: "user", Username: "alex"}, + want: access.TokenValidationResult{UserID: "user-1", Role: "user", Username: "alex"}, + }, + { + name: "admin session", + resp: &authpb.ValidateTokenResponse{UserId: "admin-1", Role: "admin", Username: "root"}, + want: access.TokenValidationResult{UserID: "admin-1", Role: "admin", Username: "root"}, + }, + { + name: "assumed session carries AssumedBy", + resp: &authpb.ValidateTokenResponse{UserId: "target-1", Role: "user", Username: "target", AssumedBy: "admin-1"}, + want: access.TokenValidationResult{UserID: "target-1", Role: "user", Username: "target", AssumedBy: "admin-1"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + validator := &GRPCTokenValidator{client: &stubAuthClient{resp: tc.resp}, timeout: time.Second} + + got, err := validator.ValidateToken(context.Background(), "token") + if err != nil { + t.Fatalf("ValidateToken() error = %v, want nil", err) + } + if *got != tc.want { + t.Errorf("ValidateToken() = %+v, want %+v", *got, tc.want) + } + }) + } +} + +// TestGRPCTokenValidator_NonDeadlineError_Wraps asserts a non-deadline client +// error is returned wrapped (the fmt.Errorf %w prefix) with its underlying gRPC +// status preserved, so it does not read as a validation timeout on either +// isValidationTimeout prong. +func TestGRPCTokenValidator_NonDeadlineError_Wraps(t *testing.T) { + clientErr := status.Error(codes.Unauthenticated, "invalid token") + validator := &GRPCTokenValidator{client: &stubAuthClient{err: clientErr}, timeout: time.Second} + + _, err := validator.ValidateToken(context.Background(), "token") + if err == nil { + t.Fatal("ValidateToken() error = nil, want a wrapped client error") + } + if !strings.Contains(err.Error(), "auth service validation failed") { + t.Errorf("error = %q, want it to carry the wrap prefix", err.Error()) + } + if !errors.Is(err, clientErr) { + t.Errorf("error = %v, want it to unwrap to the underlying client error", err) + } + if st, ok := status.FromError(err); !ok || st.Code() != codes.Unauthenticated { + t.Errorf("wrapped error must preserve the gRPC Unauthenticated code: ok=%v st=%v", ok, st) + } +} + +// TestGatewayValidateNonDeadline_EndToEnd_Returns401 wires AccessControl in +// front of the real GRPCTokenValidator whose client returns a non-deadline gRPC +// error, proving the concrete validator's %w wrap preserves the code so +// isValidationTimeout classifies it as a genuine rejection (401), not a 503. +func TestGatewayValidateNonDeadline_EndToEnd_Returns401(t *testing.T) { + gin.SetMode(gin.TestMode) + + validator := &GRPCTokenValidator{ + client: &stubAuthClient{err: status.Error(codes.Unauthenticated, "invalid token")}, + timeout: time.Second, + } + + engine := gin.New() + engine.Use(access.AccessControl(validator, access.GatewayResolve, slog.New(slog.NewTextHandler(io.Discard, nil)))) + engine.GET("/api/finance/periods", func(c *gin.Context) { c.Status(http.StatusOK) }) + + req := httptest.NewRequest(http.MethodGet, "/api/finance/periods", nil) + req.AddCookie(&http.Cookie{Name: "gofin_access", Value: "token"}) + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Errorf("status = %d, want 401 Unauthorized", rec.Code) + } + if !strings.Contains(rec.Body.String(), "UNAUTHORIZED") { + t.Errorf("body = %q, want it to contain UNAUTHORIZED", rec.Body.String()) + } +} diff --git a/services/gateway/cmd/main.go b/services/gateway/cmd/main.go index 98d051f3..b2b129d2 100644 --- a/services/gateway/cmd/main.go +++ b/services/gateway/cmd/main.go @@ -66,10 +66,12 @@ func run() error { logger.Info("gRPC client configured", slog.String("auth_service_addr", cfg.AuthServiceAddr), + slog.Duration("validate_timeout", cfg.ValidateTimeout), ) - // Create the token validator wrapping the gRPC client. - validator := NewGRPCTokenValidator(grpcConn) + // Create the token validator wrapping the gRPC client. Its per-call timeout + // bounds ValidateToken so a hung auth service cannot block a worker. + validator := NewGRPCTokenValidator(grpcConn, cfg.ValidateTimeout) // Parse downstream service URLs. authURL, err := url.Parse(cfg.AuthServiceREST) diff --git a/services/gateway/internal/access/bench_test.go b/services/gateway/internal/access/bench_test.go new file mode 100644 index 00000000..5e009ff8 --- /dev/null +++ b/services/gateway/internal/access/bench_test.go @@ -0,0 +1,36 @@ +package access_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/ItsThompson/gofin/services/gateway/internal/access" +) + +// BenchmarkValidateHappyPath measures the allocations on the validation happy +// path: an Authenticated route with a valid cookie whose token the validator +// accepts. It is the committed reference for services/gateway/perf/baseline. +// +// The absolute allocs/op is a same-machine reference, not a CI gate (see the +// services/perf README): this path's regression guard is behavioral (the +// bounded-timeout test), not an allocation count. The benchmark exists so the +// baseline file records a concrete "before" number alongside the note that the +// pre-change ValidateToken call had no upper latency bound. +func BenchmarkValidateHappyPath(b *testing.B) { + validator := &fakeValidator{ + result: &access.TokenValidationResult{UserID: "user-1", Role: "user"}, + } + engine := buildEngine(validator, silentLogger(), http.MethodPost, "/api/auth/restore", okHandler) + + b.ReportAllocs() + for b.Loop() { + req := httptest.NewRequest(http.MethodPost, "/api/auth/restore", nil) + req.AddCookie(&http.Cookie{Name: "gofin_access", Value: "token"}) + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + b.Fatalf("unexpected status %d", rec.Code) + } + } +} diff --git a/services/gateway/internal/access/control.go b/services/gateway/internal/access/control.go index c438b977..9c3aecd9 100644 --- a/services/gateway/internal/access/control.go +++ b/services/gateway/internal/access/control.go @@ -1,10 +1,14 @@ package access import ( + "context" + "errors" "log/slog" "net/http" "github.com/gin-gonic/gin" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/ItsThompson/gofin/services/access" ) @@ -76,6 +80,19 @@ func AccessControl(validator TokenValidator, resolve func(method, path string) a result, err := validator.ValidateToken(c.Request.Context(), cookie.Value) if err != nil { + // A bounded-timeout deadline means the auth dependency is unhealthy, + // not that the client's token is invalid: fail fast with 503 so the + // worker is freed, distinct from the 401 for a genuine rejection. + if isValidationTimeout(err) { + logger.Warn("auth validation timed out", + slog.String("method", c.Request.Method), + slog.String("path", c.Request.URL.Path), + slog.String("dependency", "auth"), + slog.String("error", err.Error()), + ) + abortUnavailable(c) + return + } logger.Warn("token validation failed", slog.String("method", c.Request.Method), slog.String("path", c.Request.URL.Path), @@ -154,6 +171,33 @@ func abortUnauthorized(c *gin.Context, message string) { }) } +// isValidationTimeout reports whether a ValidateToken error is the bounded +// timeout tripping (a hung auth dependency) rather than a genuine auth +// rejection. The gateway bounds the RPC with context.WithTimeout in the +// concrete validator; a deadline surfaces either as a wrapped +// context.DeadlineExceeded or as a gRPC DeadlineExceeded status, so both prongs +// are checked. status.FromError unwraps the validator's fmt.Errorf %w wrap. +// Every other error (including all other gRPC codes) stays a 401. +func isValidationTimeout(err error) bool { + if errors.Is(err, context.DeadlineExceeded) { + return true + } + if st, ok := status.FromError(err); ok && st.Code() == codes.DeadlineExceeded { + return true + } + return false +} + +// abortUnavailable ends the request with 503 SERVICE_UNAVAILABLE, used when the +// auth dependency is unhealthy (validation timed out) rather than the client's +// token being invalid. +func abortUnavailable(c *gin.Context) { + c.AbortWithStatusJSON(http.StatusServiceUnavailable, gin.H{ + "code": "SERVICE_UNAVAILABLE", + "message": "Authentication service unavailable", + }) +} + // rejectForbidden ends the request with the unchanged 403 code contract (the // machine-readable "FORBIDDEN" code is preserved; the human-facing message text // was consolidated to a generic "Access denied"), preserving the role-denied diff --git a/services/gateway/internal/access/control_test.go b/services/gateway/internal/access/control_test.go index 83e561e0..a0f037a0 100644 --- a/services/gateway/internal/access/control_test.go +++ b/services/gateway/internal/access/control_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "log/slog" "net/http" @@ -13,6 +14,8 @@ import ( "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" sharedaccess "github.com/ItsThompson/gofin/services/access" "github.com/ItsThompson/gofin/services/gateway/internal/access" @@ -308,6 +311,79 @@ func TestAccessControl_ValidationError_Returns401(t *testing.T) { assert.Contains(t, rec.Body.String(), "UNAUTHORIZED") } +// --- 503 timeout mapping: a bounded-validation deadline is not a bad token --- + +// TestAccessControl_ValidationTimeout_Returns503 pins the key semantic split: +// a ValidateToken error that is (or wraps) context.DeadlineExceeded, or that +// carries a gRPC DeadlineExceeded status, maps to 503 SERVICE_UNAVAILABLE (the +// auth dependency is unhealthy), distinct from the 401 for a genuine rejection. +// Both detection prongs and the fmt.Errorf %w wrap the concrete validator adds +// are covered. +func TestAccessControl_ValidationTimeout_Returns503(t *testing.T) { + cases := []struct { + name string + err error + }{ + {"raw context deadline", context.DeadlineExceeded}, + {"wrapped context deadline", fmt.Errorf("auth service validation failed: %w", context.DeadlineExceeded)}, + {"grpc deadline status", status.Error(codes.DeadlineExceeded, "context deadline exceeded")}, + {"wrapped grpc deadline status", fmt.Errorf("auth service validation failed: %w", status.Error(codes.DeadlineExceeded, "context deadline exceeded"))}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + validator := &fakeValidator{err: tc.err} + engine := buildEngine(validator, silentLogger(), http.MethodGet, "/api/finance/periods", okHandler) + + req := httptest.NewRequest(http.MethodGet, "/api/finance/periods", nil) + req.AddCookie(&http.Cookie{Name: "gofin_access", Value: "token"}) + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusServiceUnavailable, rec.Code, "a validation deadline must map to 503") + assert.Contains(t, rec.Body.String(), "SERVICE_UNAVAILABLE") + assert.NotContains(t, rec.Body.String(), "UNAUTHORIZED", "a timeout must not be reported as an invalid token") + }) + } +} + +// TestAccessControl_NonDeadlineGRPCError_Returns401 proves only the deadline is +// special-cased: every other gRPC status (here Unauthenticated) still returns +// the unchanged 401 contract. +func TestAccessControl_NonDeadlineGRPCError_Returns401(t *testing.T) { + validator := &fakeValidator{err: status.Error(codes.Unauthenticated, "invalid token")} + engine := buildEngine(validator, silentLogger(), http.MethodGet, "/api/finance/periods", okHandler) + + req := httptest.NewRequest(http.MethodGet, "/api/finance/periods", nil) + req.AddCookie(&http.Cookie{Name: "gofin_access", Value: "token"}) + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusUnauthorized, rec.Code) + assert.Contains(t, rec.Body.String(), "UNAUTHORIZED") +} + +// TestAccessControl_ValidationTimeout_LogsWarningNamingAuth asserts the 503 path +// emits a distinct warn log that names the auth dependency as the cause, so the +// timeout is diagnosable separately from a 401. +func TestAccessControl_ValidationTimeout_LogsWarningNamingAuth(t *testing.T) { + logger, buf := captureLogger() + validator := &fakeValidator{err: status.Error(codes.DeadlineExceeded, "context deadline exceeded")} + engine := buildEngine(validator, logger, http.MethodGet, "/api/finance/periods", okHandler) + + req := httptest.NewRequest(http.MethodGet, "/api/finance/periods", nil) + req.AddCookie(&http.Cookie{Name: "gofin_access", Value: "token"}) + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + + require.Equal(t, http.StatusServiceUnavailable, rec.Code) + entry := buf.lastEntry(t) + assert.Equal(t, "WARN", entry["level"]) + assert.Equal(t, "auth validation timed out", entry["msg"]) + assert.Equal(t, "auth", entry["dependency"]) + assert.Equal(t, "/api/finance/periods", entry["path"]) +} + // --- Warn logging on 401 and 403 --- func TestAccessControl_Unauthorized_LogsWarning(t *testing.T) { diff --git a/services/gateway/internal/config/config.go b/services/gateway/internal/config/config.go index 2d6db844..f6e0ed00 100644 --- a/services/gateway/internal/config/config.go +++ b/services/gateway/internal/config/config.go @@ -2,9 +2,33 @@ package config import ( "fmt" + "log/slog" "os" + "time" ) +// defaultValidateTimeout is a hung-dependency / availability BACKSTOP: it bounds +// the catastrophic-hang tail of the gateway's ValidateToken gRPC call so a hung +// auth service returns a fast error instead of blocking the worker +// indefinitely. It is deliberately generous: it must comfortably exceed a +// healthy ValidateToken (a JWT check plus a DB lookup, low milliseconds) so it +// never trips in normal operation, and only fires when the dependency is +// effectively hung. +// +// This is NOT p99/tail-latency control in the "Tail at Scale" sense. Trimming +// the p99 tail toward the auth service's measured P99 (a 100-300 ms starting +// point near the auth P99) is explicitly out of scope for P1 and left as +// possible future work once that percentile is measured. The earlier "2-5s +// band" framing is superseded: 3s is a hang backstop, not a percentile-derived +// bound. Override with GATEWAY_VALIDATE_TIMEOUT. +const defaultValidateTimeout = 3 * time.Second + +// maxRecommendedValidateTimeout is the largest GATEWAY_VALIDATE_TIMEOUT that +// still serves as a hang backstop. Larger values are accepted (there is no hard +// clamp) but warned about at load: a "1h" typo would silently re-introduce the +// near-unbounded tail this bound exists to prevent. +const maxRecommendedValidateTimeout = 30 * time.Second + // Config holds all configuration for the API gateway, loaded from environment variables. type Config struct { AuthServiceAddr string // gRPC address for auth service (e.g., "auth-service:9081") @@ -15,6 +39,7 @@ type Config struct { LogLevel string Environment string Port string + ValidateTimeout time.Duration // upper bound on the ValidateToken gRPC call (GATEWAY_VALIDATE_TIMEOUT, default 3s) } // Load reads configuration from environment variables and returns a Config. @@ -60,6 +85,24 @@ func Load() (*Config, error) { port = "8080" } + validateTimeout := defaultValidateTimeout + if raw := os.Getenv("GATEWAY_VALIDATE_TIMEOUT"); raw != "" { + parsed, err := time.ParseDuration(raw) + if err != nil { + return nil, fmt.Errorf("parsing GATEWAY_VALIDATE_TIMEOUT %q: %w", raw, err) + } + if parsed <= 0 { + return nil, fmt.Errorf("GATEWAY_VALIDATE_TIMEOUT must be positive, got %q", raw) + } + if parsed > maxRecommendedValidateTimeout { + slog.Warn("GATEWAY_VALIDATE_TIMEOUT is unusually large; it is a hung-dependency backstop, not a normal latency bound", + slog.Duration("configured", parsed), + slog.Duration("recommended_ceiling", maxRecommendedValidateTimeout), + ) + } + validateTimeout = parsed + } + return &Config{ AuthServiceAddr: authAddr, AuthServiceREST: authREST, @@ -69,6 +112,7 @@ func Load() (*Config, error) { LogLevel: logLevel, Environment: environment, Port: port, + ValidateTimeout: validateTimeout, }, nil } diff --git a/services/gateway/internal/config/config_test.go b/services/gateway/internal/config/config_test.go new file mode 100644 index 00000000..90ba792b --- /dev/null +++ b/services/gateway/internal/config/config_test.go @@ -0,0 +1,109 @@ +package config + +import ( + "log/slog" + "strings" + "testing" + "time" +) + +// setGatewayEnv sets the minimum required env vars so Load reaches the +// GATEWAY_VALIDATE_TIMEOUT parsing under test. t.Setenv restores them after. +func setGatewayEnv(t *testing.T) { + t.Helper() + t.Setenv("AUTH_SERVICE_ADDR", "auth-service:9081") + t.Setenv("AUTH_SERVICE_REST", "http://auth-service:8081") + t.Setenv("EXPENSE_SERVICE_REST", "http://expense-service:8082") + t.Setenv("FINANCE_SERVICE_REST", "http://finance-service:8083") + t.Setenv("DATARIGHTS_SERVICE_REST", "http://datarights-service:8084") +} + +func TestLoad_ValidateTimeout_DefaultsWhenUnset(t *testing.T) { + setGatewayEnv(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if cfg.ValidateTimeout != 3*time.Second { + t.Errorf("ValidateTimeout = %v, want default 3s", cfg.ValidateTimeout) + } +} + +func TestLoad_ValidateTimeout_ParsesOverride(t *testing.T) { + setGatewayEnv(t) + t.Setenv("GATEWAY_VALIDATE_TIMEOUT", "5s") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if cfg.ValidateTimeout != 5*time.Second { + t.Errorf("ValidateTimeout = %v, want 5s", cfg.ValidateTimeout) + } +} + +func TestLoad_ValidateTimeout_RejectsUnparseable(t *testing.T) { + setGatewayEnv(t) + t.Setenv("GATEWAY_VALIDATE_TIMEOUT", "not-a-duration") + + if _, err := Load(); err == nil { + t.Fatal("Load() error = nil, want error for unparseable GATEWAY_VALIDATE_TIMEOUT") + } +} + +func TestLoad_ValidateTimeout_RejectsNonPositive(t *testing.T) { + setGatewayEnv(t) + t.Setenv("GATEWAY_VALIDATE_TIMEOUT", "0s") + + if _, err := Load(); err == nil { + t.Fatal("Load() error = nil, want error for non-positive GATEWAY_VALIDATE_TIMEOUT") + } +} + +// captureDefaultLogger swaps slog's default logger for one writing to the +// returned buffer (Load emits its oversized-value warning through slog.Warn, +// i.e. the default logger) and restores it on cleanup. +func captureDefaultLogger(t *testing.T) *strings.Builder { + t.Helper() + buf := &strings.Builder{} + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelWarn}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + return buf +} + +// TestLoad_ValidateTimeout_WarnsAboveCeiling proves the GW-4 soft bound: a value +// above the recommended ceiling is accepted unchanged (no hard clamp) but logs +// a visible warning so a "1h"-style typo that defeats the backstop is caught. +func TestLoad_ValidateTimeout_WarnsAboveCeiling(t *testing.T) { + setGatewayEnv(t) + t.Setenv("GATEWAY_VALIDATE_TIMEOUT", "45s") + buf := captureDefaultLogger(t) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if cfg.ValidateTimeout != 45*time.Second { + t.Errorf("ValidateTimeout = %v, want 45s (accepted, no hard clamp)", cfg.ValidateTimeout) + } + if !strings.Contains(buf.String(), "unusually large") { + t.Errorf("expected a warning for a value above the ceiling, got %q", buf.String()) + } +} + +// TestLoad_ValidateTimeout_NoWarnAtCeiling confirms the warning does not fire at +// or below the recommended ceiling. +func TestLoad_ValidateTimeout_NoWarnAtCeiling(t *testing.T) { + setGatewayEnv(t) + t.Setenv("GATEWAY_VALIDATE_TIMEOUT", "30s") + buf := captureDefaultLogger(t) + + if _, err := Load(); err != nil { + t.Fatalf("Load() error = %v", err) + } + if strings.Contains(buf.String(), "unusually large") { + t.Errorf("did not expect a warning at the recommended ceiling, got %q", buf.String()) + } +} diff --git a/services/gateway/perf/baseline/validate.txt b/services/gateway/perf/baseline/validate.txt new file mode 100644 index 00000000..ce476f4f --- /dev/null +++ b/services/gateway/perf/baseline/validate.txt @@ -0,0 +1,83 @@ +# Baseline: Gateway ValidateToken validation path (P1) + +Captured on `main` **before** the bounded-timeout change (spec §05, ticket #2). + +## Happy-path allocations + +Benchmark: `BenchmarkValidateHappyPath` +(`services/gateway/internal/access/bench_test.go`) exercises the full +`AccessControl` validation path (strip headers → resolve → read cookie → +`ValidateToken` → set identity headers → role check) on an Authenticated route +with a valid token, via `gin` `ServeHTTP`. + +``` +$ cd services/gateway && go test -run '^$' -bench '^BenchmarkValidateHappyPath$' -benchmem -count=3 ./internal/access/ +goos: darwin +goarch: arm64 +cpu: Apple M4 Pro +BenchmarkValidateHappyPath-14 447820 2652 ns/op 8467 B/op 59 allocs/op +BenchmarkValidateHappyPath-14 455296 2680 ns/op 8466 B/op 59 allocs/op +BenchmarkValidateHappyPath-14 447896 2678 ns/op 8467 B/op 59 allocs/op +``` + +**Reference figure: ~59 allocs/op, ~8467 B/op.** + +Per the `services/perf` README, wall-clock ns/op and absolute allocs/op are +same-machine references (arm64 dev here vs amd64 CI), not a CI gate. This path's +regression guard is **behavioral** (see below), not an allocation count, so no +`AssertMaxAllocs` bound is asserted for it. + +## The meaningful "before": no upper latency bound + +Before this change, `GRPCTokenValidator.ValidateToken` called +`client.ValidateToken(ctx, …)` with **no `context.WithTimeout`**. The incoming +request context carries no deadline of its own, so: + +- If the auth service **hangs** (as opposed to being down, which returns a fast + connection error), the gateway worker blocks on the validation RPC + **indefinitely** — it would never return until the client disconnects. +- There was therefore **no upper bound** on validation latency on the hottest + path (every authenticated request). This is unbounded tail latency and a real + availability risk under the VPS's modest worker limits. + +There is no pre-change wall-clock number for a hang because a hang never +returns. The "before" is the **absence of a bound**; the regression guard added +with this change is the behavioral test asserting a hung/slow validator now +returns `503` within `GATEWAY_VALIDATE_TIMEOUT` (default `3s`) plus a small +margin, and that reverting the timeout wrap makes that test hang/fail. + +## Scope of the bound: a hang backstop, not p99 tail control + +`GATEWAY_VALIDATE_TIMEOUT` (default `3s`, `internal/config/config.go`) is a +hung-dependency / availability **backstop**. It bounds the catastrophic-hang +tail: a healthy `ValidateToken` runs in low single-digit milliseconds, so at +`3s` the bound never trips in normal operation and only fires when the auth +dependency is effectively hung. + +This is deliberately NOT p99/tail-latency control in the "Tail at Scale" sense. +Trimming the p99 tail toward the auth service's measured P99 (research §D.1 +suggests a 100-300 ms starting point, near the auth P99) is explicitly OUT OF +SCOPE for P1 and left as possible future work once that percentile is measured +against `04-measurement-and-baselining.md`. The earlier "audit's 2-5s band" +framing is superseded: `3s` is a hang backstop, not a percentile-derived bound. +The default value is left unchanged by this review: tightening an +auth-validation timeout is a production behavioral change that risks spurious +fail-closed 503s and needs measured auth P99 data first. + +The knob is validated to be positive (non-positive is rejected). There is no +hard upper clamp, but a value above `maxRecommendedValidateTimeout` (`30s`) +defeats the backstop's purpose (it re-introduces the near-unbounded tail) and is +logged as a warning at load, so a `1h`-style typo is visible. + +## Metrics on the 503 timeout path: deliberately deferred + +The fail-closed timeout path (`internal/access/control.go`, the `503` warn) is +log-only. It emits a distinct `dependency=auth` warn ("auth validation timed +out") and no counter/metric, so the timeout is diagnosable via that log event, +which names auth as the cause and is separable from the `401` warn. + +Adding a metric for the `DeadlineExceeded` failure class (research §D.3 calls for +distinguishing `DeadlineExceeded` (auth slow) from `Unauthenticated` (auth said +no) in metrics) is a deliberate OUT-OF-SCOPE deferral per spec §04/§12: this epic +adds no new metrics, and the `access` middleware emits none today, so this is a +pre-existing convention rather than a P1 regression. No metric code is added. diff --git a/services/go.work b/services/go.work index e0304c1a..0dbf95e8 100644 --- a/services/go.work +++ b/services/go.work @@ -10,4 +10,5 @@ use ( ./gateway ./healthcheck ./metrics + ./perf ) diff --git a/services/perf/README.md b/services/perf/README.md new file mode 100644 index 00000000..b1cdbacd --- /dev/null +++ b/services/perf/README.md @@ -0,0 +1,170 @@ +# perf + +Shared **test-only** helpers for the gofin latency & efficiency epic. This module +carries no runtime dependencies and is imported only from `_test.go` files in the +gateway, datarights, expense, and finance services. It provides the measurement +primitives every optimization slice (P1-P4) uses to capture baselines and write +regression assertions. + +It sits alongside `metrics`, `access`, and `healthcheck` as a shared module and is +registered in `services/go.work`, so `go test ./...` picks it up in the existing +`test-backend` CI job. No new CI job is added. + +## Why measure this way + +gofin runs on a single VPS with ~5 concurrent users and only 3 days of Prometheus +retention, so neither production traffic nor Prometheus is a durable "did this +help?" signal. The strategy inverts the usual instinct: **the signals we commit +and gate on are the ones that reproduce on any machine.** + +| Signal | Portable? | Role | +|--------|-----------|------| +| Upstream call counts (gRPC/repo) | Yes, structural | Committed baseline + CI assertion (`CallCounter`) | +| Query counts / rows scanned | Yes, structural | Committed baseline + CI assertion | +| Complexity-scaling ratio (cost across input sizes) | Yes, the *ratio* is portable | Committed baseline + growth-ratio assertion | +| `allocs/op`, `B/op` | Per arch/compiler only | Baseline is documentation; CI asserts **bounds/ratios**, never committed absolutes (`AssertMaxAllocs`) | +| Wall-clock `ns/op` | No | Same-machine before/after reference only, never a gate | + +**Durable, portable signals are allocs bounds, call counts, and scaling ratios. +Committed wall-clock numbers are same-machine references** labeled with machine +spec + date; read them only as the direction and magnitude of a same-session +delta. + +### Architecture caveat for allocation counts + +`allocs/op` and `B/op` are deterministic for a given compiler, GC, and **CPU +architecture**, but differ across architectures and Go versions. Baselines are +captured on the maintainer's machine (Apple M-series, **arm64**); CI runs on +`ubuntu-latest` (**amd64**). So committed **absolute** alloc numbers are +documentation for the same-machine before/after story: they are **not** valid CI +thresholds. CI alloc assertions use only arch-stable forms: + +- `AssertMaxAllocs(t, max, fn)` max-bounds ("≤ N allocations") with headroom. +- Growth-ratios ("allocs must not scale with input size"). + +Never assert a committed absolute alloc number in CI. + +## Helpers + +### `CallCounter` + +A concurrency-safe counter that spy implementations of gRPC clients / repo +interfaces embed (as a `*perf.CallCounter`) so a test can assert bounds like +"`GetAllUserData` called at most once". Because P2/P4 call spies from inside +`errgroup` fan-out goroutines, all methods are safe for concurrent use. + +```go +type financeSpy struct { + financepb.FinanceServiceClient + *perf.CallCounter +} + +func (s *financeSpy) GetAllUserData(ctx context.Context, in *financepb.GetAllUserDataRequest, _ ...grpc.CallOption) (*financepb.AllUserDataResponse, error) { + s.Record("GetAllUserData") + return cannedAllUserData, nil +} + +// in the test: +spy := &financeSpy{CallCounter: perf.NewCallCounter(), /* ... */} +// ... exercise the code under test ... +if got := spy.Count("GetAllUserData"); got > 1 { + t.Fatalf("GetAllUserData called %d times; want <=1 (dedup regressed)", got) +} +``` + +### `AssertMaxAllocs` + +Wraps `testing.AllocsPerRun` with a clear observed-vs-allowed failure message. +Pass an arch-stable upper bound with headroom, not the exact number captured on +one machine. + +```go +perf.AssertMaxAllocs(t, 8, func() { + _ = encodeRow(row) +}) +// fails with: "allocations exceeded bound: observed 12 allocs/op, allowed <= 8" +``` + +> **Intentionally not provided:** a `baseline.Load`/`baseline.Compare` helper. +> Nothing would consume it: the before/after comparison is `benchstat old.txt +> new.txt` (local, reviewed in the PR diff) and the CI gate is hand-written +> bound/ratio assertions. Committed baselines are documentation reviewed in the +> PR diff, not a programmatically-read gate. + +## Local benchmark / pprof / benchstat workflow + +Benchmarks are standard `Benchmark*` functions in `_test.go` files inside the +package under test; where scaling matters, parameterize by input size with +`b.Run` sub-benchmarks. + +### Run benchmarks + +```sh +# From the service module under test: +go test -bench=. -benchmem -run '^$' ./... +``` + +`-benchmem` reports `allocs/op` and `B/op` (the portable signals); `-run '^$'` +skips the normal `Test*` functions so only benchmarks run. + +### Capture pprof (local diagnostic only) + +pprof locates hotspots and confirms the *mechanism* of a win (e.g. OFFSET scan +cost dominates, or streaming removed a large allocation). Absolute CPU numbers are +not portable; alloc profiles corroborate `allocs/op`. + +```sh +go test -bench=BenchmarkExportExpenseRead -benchmem \ + -cpuprofile cpu.out -memprofile mem.out -run '^$' +go tool pprof -top cpu.out +go tool pprof -alloc_space -top mem.out +``` + +### Diff with benchstat + +This is the before/after comparison mechanism (same machine, same session): + +```sh +go install golang.org/x/perf/cmd/benchstat@latest + +# 1. On current main, capture the baseline: +go test -bench=. -benchmem -count=10 -run '^$' ./... > old.txt + +# 2. Implement the optimization, then re-run on the SAME machine: +go test -bench=. -benchmem -count=10 -run '^$' ./... > new.txt + +# 3. Compare and paste the result into the PR description: +benchstat old.txt new.txt +``` + +Use `-count=N` so `benchstat` can apply its significance test; rely on +allocs/counts for the hard signal since wall-clock reruns are noisy. + +### The `-race` gate + +Run the concurrency-safety tests under the race detector locally before pushing +concurrent changes (P2/P4): + +```sh +go test -race ./... +``` + +`-race` is a **local/manual gate**, not part of CI. + +## Baseline artifacts + +Optimization slices commit baselines under +`services//perf/baseline/.txt` (plus optional `.pprof` +snapshots). Each file records the portable metrics as primary content, with +wall-clock clearly labeled as a same-machine reference. The committed baseline is +documentation reviewed in the PR diff; the non-flaky regression gate is the +efficiency assertion (`CallCounter` bounds, query-shape checks, `AssertMaxAllocs` +bounds, growth-ratios) that rides the existing `test-backend` job. + +## Testing this module + +```sh +cd services/perf +go test ./... +go test -race ./... # exercises CallCounter concurrency +``` diff --git a/services/perf/allocs.go b/services/perf/allocs.go new file mode 100644 index 00000000..2b205d21 --- /dev/null +++ b/services/perf/allocs.go @@ -0,0 +1,41 @@ +package perf + +import "testing" + +// allocRunsPerAssertion is the number of measured runs passed to +// testing.AllocsPerRun; its integer-floored average smooths sporadic background +// allocations. +const allocRunsPerAssertion = 100 + +// allocReporter is the subset of testing.TB that AssertMaxAllocs needs to report +// a failure. testing.TB satisfies it; tests supply a fake to exercise the +// failure path without failing the enclosing test (testing.TB is sealed and +// cannot be implemented outside the testing package). +type allocReporter interface { + Helper() + Errorf(format string, args ...any) +} + +// AssertMaxAllocs fails t if fn allocates more than maxAllocs times per run, +// reporting observed vs allowed allocations. +// +// maxAllocs is an arch-stable upper bound with headroom, NOT a committed absolute +// alloc count: allocs/op differ between architectures (arm64 dev vs amd64 CI) +// and Go versions, so pick a bound the path should never exceed rather than the +// exact number captured on one machine. See README.md. +// +// This helper is a shipped part of the measurement foundation but currently has +// no caller; it is reserved for a non-scaling allocation-bound path that +// warrants a fixed bound (growth-ratio tests cover the scaling paths instead). +func AssertMaxAllocs(t testing.TB, maxAllocs float64, fn func()) { + t.Helper() + assertMaxAllocs(t, maxAllocs, fn) +} + +func assertMaxAllocs(r allocReporter, maxAllocs float64, fn func()) { + r.Helper() + observed := testing.AllocsPerRun(allocRunsPerAssertion, fn) + if observed > maxAllocs { + r.Errorf("allocations exceeded bound: observed %.0f allocs/op, allowed <= %.0f", observed, maxAllocs) + } +} diff --git a/services/perf/allocs_test.go b/services/perf/allocs_test.go new file mode 100644 index 00000000..f340cc75 --- /dev/null +++ b/services/perf/allocs_test.go @@ -0,0 +1,77 @@ +package perf + +import ( + "fmt" + "strings" + "testing" +) + +// allocSink gives allocations in the test closures a heap escape so +// testing.AllocsPerRun counts them; without an escape the compiler may +// stack-allocate and report zero. It is read back to confirm the allocation +// actually happened (and to keep it from being a write-only variable). +var allocSink []byte + +// fakeReporter stands in for testing.TB so the failure path can be exercised +// without failing the enclosing test. testing.TB is sealed and cannot be +// implemented outside the testing package, so AssertMaxAllocs delegates to +// assertMaxAllocs, which accepts the narrower allocReporter. +type fakeReporter struct { + helperCalls int + messages []string +} + +func (f *fakeReporter) Helper() { f.helperCalls++ } + +func (f *fakeReporter) Errorf(format string, args ...any) { + f.messages = append(f.messages, fmt.Sprintf(format, args...)) +} + +func TestAssertMaxAllocs_FailsWhenOverBound(t *testing.T) { + r := &fakeReporter{} + + assertMaxAllocs(r, 0, func() { allocSink = make([]byte, 64) }) + + if len(allocSink) != 64 { + t.Fatalf("sink not populated; allocation may have been optimized away (len=%d)", len(allocSink)) + } + if r.helperCalls == 0 { + t.Error("expected Helper() to be called") + } + if len(r.messages) != 1 { + t.Fatalf("expected exactly 1 failure message, got %d: %v", len(r.messages), r.messages) + } + msg := r.messages[0] + if !strings.Contains(msg, "observed") || !strings.Contains(msg, "allowed") { + t.Errorf("failure message should report observed vs allowed, got: %q", msg) + } + if !strings.Contains(msg, "<= 0") { + t.Errorf("failure message should report the allowed bound (0), got: %q", msg) + } +} + +func TestAssertMaxAllocs_PassesWhenWithinBound(t *testing.T) { + r := &fakeReporter{} + + assertMaxAllocs(r, 2, func() { allocSink = make([]byte, 32) }) + + if len(r.messages) != 0 { + t.Errorf("expected no failure within bound, got: %v", r.messages) + } +} + +func TestAssertMaxAllocs_PassesForZeroAllocFunc(t *testing.T) { + r := &fakeReporter{} + + assertMaxAllocs(r, 0, func() {}) + + if len(r.messages) != 0 { + t.Errorf("expected no failure for zero-allocation func, got: %v", r.messages) + } +} + +// TestAssertMaxAllocs_PublicPassPath drives the exported wrapper with a real +// *testing.T to confirm it does not fail the test when fn stays within bound. +func TestAssertMaxAllocs_PublicPassPath(t *testing.T) { + AssertMaxAllocs(t, 5, func() { allocSink = make([]byte, 16) }) +} diff --git a/services/perf/counter.go b/services/perf/counter.go new file mode 100644 index 00000000..a90b166b --- /dev/null +++ b/services/perf/counter.go @@ -0,0 +1,50 @@ +// Package perf provides shared test helpers for the gofin latency & efficiency +// epic: a concurrency-safe call counter that spy client/repo implementations +// embed, and an allocation-bound assertion over testing.AllocsPerRun. It carries +// no runtime dependencies and is imported only from test files. See README.md +// for the benchmark / pprof / benchstat workflow. +package perf + +import "sync" + +// CallCounter records how many times named operations were invoked. Spy +// implementations of gRPC clients and repository interfaces embed a +// *CallCounter so efficiency tests can assert bounds such as "GetAllUserData +// called at most once". All methods are safe for concurrent use, so spies may +// be called from concurrent errgroup fan-out goroutines in the datarights and +// finance services. +type CallCounter struct { + mu sync.Mutex + counts map[string]int +} + +// NewCallCounter returns an empty CallCounter ready for concurrent use. +func NewCallCounter() *CallCounter { + return &CallCounter{counts: make(map[string]int)} +} + +// Record increments the invocation count for op. Call it at the top of each +// spy method. +func (c *CallCounter) Record(op string) { + c.mu.Lock() + defer c.mu.Unlock() + c.counts[op]++ +} + +// Count returns how many times op was recorded. +func (c *CallCounter) Count(op string) int { + c.mu.Lock() + defer c.mu.Unlock() + return c.counts[op] +} + +// Total returns the total number of recorded invocations across all ops. +func (c *CallCounter) Total() int { + c.mu.Lock() + defer c.mu.Unlock() + total := 0 + for _, n := range c.counts { + total += n + } + return total +} diff --git a/services/perf/counter_test.go b/services/perf/counter_test.go new file mode 100644 index 00000000..f25da77e --- /dev/null +++ b/services/perf/counter_test.go @@ -0,0 +1,97 @@ +package perf_test + +import ( + "sync" + "testing" + + "github.com/ItsThompson/gofin/services/perf" +) + +func TestCallCounter_CountsPerOperation(t *testing.T) { + c := perf.NewCallCounter() + + c.Record("GetAllUserData") + c.Record("ListTags") + c.Record("ListTags") + c.Record("ListTags") + + if got := c.Count("GetAllUserData"); got != 1 { + t.Errorf("Count(GetAllUserData) = %d, want 1", got) + } + if got := c.Count("ListTags"); got != 3 { + t.Errorf("Count(ListTags) = %d, want 3", got) + } +} + +func TestCallCounter_UnrecordedOperationIsZero(t *testing.T) { + c := perf.NewCallCounter() + + if got := c.Count("NeverCalled"); got != 0 { + t.Errorf("Count(NeverCalled) = %d, want 0", got) + } +} + +func TestCallCounter_TotalSumsAllOperations(t *testing.T) { + c := perf.NewCallCounter() + + if got := c.Total(); got != 0 { + t.Errorf("Total() on empty counter = %d, want 0", got) + } + + c.Record("A") + c.Record("B") + c.Record("B") + + if got := c.Total(); got != 3 { + t.Errorf("Total() = %d, want 3", got) + } +} + +// TestCallCounter_ConcurrentRecord exercises the mutex under the race detector: +// run with `go test -race`. Spies embed CallCounter and are invoked from +// errgroup fan-out goroutines, so concurrent Record must not race. A reader +// goroutine hits Count/Total during the writes so -race also covers reads +// concurrent with writes, backing the "all methods are safe" claim. +func TestCallCounter_ConcurrentRecord(t *testing.T) { + c := perf.NewCallCounter() + + const goroutines = 50 + const recordsPerGoroutine = 100 + + stop := make(chan struct{}) + readerDone := make(chan struct{}) + go func() { + defer close(readerDone) + for { + select { + case <-stop: + return + default: + _ = c.Count("fanout") + _ = c.Total() + } + } + }() + + var wg sync.WaitGroup + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + for j := 0; j < recordsPerGoroutine; j++ { + c.Record("fanout") + } + }() + } + wg.Wait() + close(stop) + <-readerDone + + want := goroutines * recordsPerGoroutine + if got := c.Count("fanout"); got != want { + t.Errorf("Count(fanout) = %d, want %d", got, want) + } + if got := c.Total(); got != want { + t.Errorf("Total() = %d, want %d", got, want) + } +} diff --git a/services/perf/go.mod b/services/perf/go.mod new file mode 100644 index 00000000..4be2eb03 --- /dev/null +++ b/services/perf/go.mod @@ -0,0 +1,3 @@ +module github.com/ItsThompson/gofin/services/perf + +go 1.26