GoFin services consolidation & hardening: shared platform layer, dead-code sweep, correctness fixes#38
Merged
Merged
Conversation
- REST handler uses CreatePeriodWithProRata; the CreatePeriod service method had no callers and duplicated its validation block - delete the method and its ~50 LOC divergent validation copy
- dashboard.go declared a func-local yearMonth identical in shape to the package-level one in prorata.go - drop the local definition; both call sites now share one type
- remove 12 explicit Unimplemented RPC stubs; the embedded UnimplementedFinanceServiceServer already returns codes.Unimplemented - refresh the stale struct doc comment to match
Create the services/pgutil leaf module with three specific helpers that
every Postgres-backed service currently re-implements inline:
- ParseUUID: string -> pgtype.UUID, standardizing the wrap message
fmt.Errorf("parsing UUID: %w", err) across 38 call sites.
- IsNoRows: errors.Is(err, pgx.ErrNoRows) (19 sites, incl. datarights'
non-idiomatic == comparisons).
- IsUniqueViolation: errors.As on *pgconn.PgError, SQLSTATE 23505,
returning the constraint name (3 sites).
Specific over generic (no ScanOne[T]/NilOnNoRows[T]): sqlc row types
differ per query. Pins pgx to v5.9.2 to match the workspace and avoid a
workspace-wide MVS bump. Call-site migration and formatUUID deletion are
handled by the per-service tickets (#7/#8/#10).
Single-source the API error contract that is duplicated across the four
API services: the typed Error, the shared code constants, and the HTTP
wire mapping.
- Error{Code,Message,Status,Fields} with Unauthorized/NotFound/Validation/
Conflict/Internal constructors.
- Respond(c, err) classifies via errors.As so %w-wrapped typed errors keep
their status/code; unknown errors fall through to 500 INTERNAL_SERVER_ERROR.
- Exported wire type APIError adopts the initialism (US-HARDEN-04); JSON
shape {code,message,fields?} is byte-identical to what services emit today.
Kept generic: no RateLimitError special-casing, no retryAfter wire field.
Module + 100%-covered unit tests only; per-service migration is #7-#11.
- remove 7 generic Unimplemented RPC stubs served by the embedded UnimplementedAuthServiceServer - keep explicit Register/Login overrides that redirect to REST endpoints - drop the companion TestGRPCStubs_ReturnUnimplemented and its helper
- only jwt_test.go called RefreshTokenTTL(); rest.go hardcodes 7*24h - keep the refreshTokenTTL field; drop the accessor and its test
Consolidate the two request guards duplicated across HTTP handlers into a shared module that depends on apierr (the only new module depending on another new module). - RequireUserID(c) reads the trusted gateway-injected X-User-ID header; writes a 401 apierr.Error and returns ok=false when missing/empty. - BindJSON[T](c, dst) binds the body, writing a 400 apierr.Error with validator field detail on failure; returns false. Generic so call sites stay one line. Module + 100%-covered unit tests only; handler migration is #7-#11.
- SchemaInitializer interface had no referents; concrete InitSchema stays - SQLResult.Columns was never set or read
Delete DBQueryDuration, ActiveConnections, ObserveQuery, and SetActiveConnections from the shared metrics module. ActiveConnections was the only one that emitted a series, polluting every service's /metrics with a permanent active_connections 0 gauge. The other three were dead-but-invisible. Removes their companion assertions and the two tests that referenced only these symbols.
Invoke the interceptor with a stub handler and assert grpc_requests_total is recorded for both an OK and a NotFound status outcome, verifying the handler runs and its response/error propagate. Exercises the interceptor behaviorally rather than calling .Inc()/.Observe() on collectors directly (US-HARDEN-03).
Remove the dead Run function (only its own tests called it; all 3 consumers use RunWithFS via source/iofs) along with the source/file blank import, which drops that driver from the auth, finance, and datarights binaries. RunWithFS and the rawConnectionURL/ensureSchema helpers remain. Deletes the Run-only test file; RunWithFS coverage lives in dbmigrate_embed_test.go.
- setIdentityHeaders reads only UserID/Role/AssumedBy; Username was set in grpc_validator.go but never read - remove the field, its single write, and companion test references
Delete the test-only Classifies function and its inaccurate "services use it" doc-comment. Its behavioral coverage is preserved by the resolver suite: every-entry-is-classified is subsumed by TestRegistry_EveryEntryResolvesToItsAccess, and the unknown-route deny guardrail folds into TestResolve_FallsBackToDeny (adding the deep unknown-service-path case). Registry and RoutesFor are retained (both live internally via BindRoutes and coverage.go).
- datarights runs no gRPC server; GRPCPort and its GRPC_PORT parsing were never read
- the empty struct was referenced nowhere; export uses the header user ID
- Essentials/Desires/Savings were parsed from brand.json but never copied into templateData, so the template never used them - drop the fields and their test fixture assignments
Remove allocs.go and allocs_test.go: AssertMaxAllocs had zero callers (self-admittedly reserved for a future path). CallCounter (counter.go), live in finance and datarights, is retained. Updates the package doc and README so they no longer document the removed helper; the arch-stable alloc-bound strategy now points at testing.AllocsPerRun directly.
Respond now treats a classified *Error with a non-positive Status the same as an unknown error (500 INTERNAL_SERVER_ERROR) instead of emitting WriteHeader(0). Unreachable via the five constructors, but hardens Respond for the varied *apierr.Error construction coming in the per-service migrations (#7-#11), keeping Respond total. Also assert Message in the constructor table test.
Introduce the in-process shared module services/serverkit that owns the bootstrap spine and serve/shutdown lifecycle the five Go services hand-write today. Wires the module into services/go.work and mirrors the existing shared-module dependency pattern (requires dbmigrate, metrics). Exposes NewLogger, ConnectPostgres, NewRouter, NewGRPCServer, and Serve, all taking primitive inputs so serverkit has no dependency on any service config package. Serve fixes the zombie-on-bind-failure bug in one place for all services: it runs the HTTP and (optional) gRPC servers in goroutines that push their first fatal error onto a buffered channel, selects between ctx cancellation (graceful shutdown, return nil) and a fatal serve error (graceful shutdown, return the error so run() can exit non-zero), and bounds shutdown to 10s. http.ErrServerClosed and the gRPC graceful-stop signal are treated as clean exits via errors.Is. Serve accepts a nil gRPC server and listener for the HTTP-only path. This ticket builds the module only; migrating the services onto it is separate.
Add high-density unit tests for the new module. Densest coverage on Serve: normal ctx cancellation returns nil (HTTP+gRPC and HTTP-only), an HTTP bind failure returns a non-nil error without hanging (port pre-occupied), and http.ErrServerClosed during graceful shutdown is treated as success, all asserted via the Serve return value with a timeout guard against hangs. NewLogger level filtering is tested through the public logger; the JSON output and service attribute via a writer-injectable seam. NewRouter is covered for /health, /metrics, and release mode. NewGRPCServer registers a minimal unary service and asserts the shared metrics interceptor records the call. ConnectPostgres covers the migration-failure boundary (no live DB).
Lock in the pollutant-gauge removal with a NotContains guard so a future edit that reintroduces the gauge fails the endpoint test.
# Conflicts: # services/go.work
- Replace inlined logger/DB-connect/router/gRPC-server/serve/shutdown in run() with serverkit helpers; return serverkit.Serve so a REST bind failure propagates and the process exits non-zero (fixes C5 for finance) - Source the --healthcheck probe port from config.ResolveRESTPort so it always matches the listener's REST_PORT/default (US-PLATFORM-05) - Add serverkit require + replace ../serverkit to finance/go.mod
- NewFinanceService now takes expenseClient and nowFunc directly; delete the mutate-but-say-copy WithExpenseClient/WithNowFunc fluent builders (US-HARDEN-01) - Route GetPeriodSummary's time.Now() through the injected nowFunc so dashboard pacing is deterministic in tests - Drop the expenseClient nil-guards in DeleteTag and pro-rata now that the client is always injected (removes the dashboard nil-deref contradiction by construction) - Update main.go and all service/handler test constructors to the folded signature
Ticket 1 removed perf.AssertMaxAllocs; the validate baseline note still named it. Reword to 'no allocation-bound assertion is made for it' so the doc cites no deleted symbol. Meaning (the guard is behavioral, not an allocation count) is unchanged.
The UserID/Role struct-literal alignment went stale when Ticket 2 removed the Username field from these fixtures. gofmt-clean it (the pinned linter set has no gofmt check, so it was not caught).
Tighten the error-contract test from assert.JSONEq (semantic) to assert.Equal on the raw body, pinning the byte-for-byte claim: field order (code, message) and the absence of a trailing newline are now part of the guarded contract.
- Order the third import group by path so expensepb/pb sit in place instead of appended at the end
- Decode fields:{...} from a multi-field VALIDATION_ERROR response body to pin the C6 contract end-to-end (service Fields -> apierr wire)
Extract the bounded-worker-pool lifecycle shared by the export and deletion engines into a new internal package. Pool owns the semaphore, the running transition, the timeout-derived context, and the background-context fail-on-timeout; engines inject their own Execute strategy and a StatusStore. Sociable tests use a real Pool with a fake StatusStore and stub execute (no mocking of the semaphore or goroutine) and cover: the running transition, success -> CompleteJob, strategy error -> FailJob under a background context, the timeout path, MaxConcurrent bounding, and live ActiveJobs/QueuedJobs.
Rename DeletionEngine -> Engine, DeletionProvider -> Provider, DeletionProviderRegistry -> Registry, and their constructors (NewDeletionEngine -> NewEngine, NewDeletionProviderRegistry -> NewRegistry). The package is internal, so no external module references the old names. Pure mechanical rename; behavior is unchanged (the pool extraction follows).
The deletion Engine now composes a jobrunner.Pool and injects its sequential-retry-with-backoff loop as the Execute strategy. The pool owns the semaphore, running transition, completion, and background-context failure; the deletion job repo satisfies jobrunner.StatusStore directly and is handed to the pool as the store. Removes the engine's now-dead private semaphore, ActiveJobs/MaxConcurrent copies, and failJob helper.
…ool gauges The export Engine now composes a jobrunner.Pool and injects its errgroup fan-out + zip + email closure as the Execute strategy. Because export's CompleteJob persists file_size_bytes and Submit carries userEmail (neither fits the generic pool signatures), the engine adapts its repo behind the StatusStore seam via a small statusStore type plus a per-job state map. The pool now owns the semaphore, so export_pool_active_jobs and export_pool_queued_jobs become GaugeFuncs that read pool.ActiveJobs()/ QueuedJobs() live at scrape time (wired via metrics.SetPoolStats from main), preserving the Grafana dashboard and stuck-pool alert. Job business metrics stay inside the injected strategy.
recoverJobs and recoverDeletionJobs collapsed into one generic helper that owns the query -> empty-check -> per-job submit skeleton; each engine passes a submit closure. Export retains its per-job email-resolve step; recovery on startup still re-submits non-terminal jobs.
…emetry Move the queued-counter increment from the worker goroutine into Submit so a just-submitted job is counted as queued the instant Submit returns, rather than only once its goroutine schedules. The decrement stays after slot acquisition; still atomic and race-clean.
…cket-12' into epic/gofin-consolidation
Replace the three near-identical deletion/providers/{auth,expense,finance}.go
structs (each wrapping one gRPC delete call) with deletion.NewFuncProvider(name,
fn). Registration in main.go now pairs a name with an idempotent delete closure,
preserving execution order (finance, expense, auth). The deletion engine's
retry/backoff execution strategy is unchanged; only provider construction
changes. Engine-level order/retry/fail-fast coverage stays in engine_test.go;
NewFuncProvider gets focused constructor + order/idempotency tests.
…nanceClient The export engine now fetches GetAllUserData once per job in execute and hands the resolved response to the provider factory. The finance-backed providers (tags, budget_periods, default_settings) become pure response -> rows mappers, and the expenses provider takes a pre-derived tag map (new BuildTagMap) instead of self-fetching it. The auth-backed profile and stream-backed expenses providers still self-fetch their own sources. finance_cache.go / MemoizedFinanceClient are removed: the single-fetch guarantee is now structural, asserted by TestExport_DedupesFinanceCalls (GetAllUserData exactly once, ListTags zero), replacing the deleted MemoizedFinanceClient unit tests.
templateData now uses BrandColors/BrandTypography/BrandSpacing directly instead of three parallel template* structs that mirrored them field by field, and buildTemplateData assigns the token groups wholesale. The templates access the same field names, so rendering is unchanged (covered by the existing HTML/text rendering tests). No reference remains to the Essentials/Desires/Savings colors removed in the dead-code sweep.
…etch The dedup-target note named a per-job MemoizedFinanceClient serving GetAllUserData at most once. That wrapper is gone: the export engine now fetches GetAllUserData exactly once in execute and passes the response to pure-mapper providers, so the guarantee is structural (one call site) and the assertion is exactly-once, not at-most-once. Doc-only.
- Add serverkit/apierr/httpx/pgutil (and dbmigrate for gateway/expense) COPY lines to each service Dockerfile so GOWORK=off go mod download can read the epic's new replaced modules' go.mod. - Unblocks the e2e Docker build and CD image build, which failed with "reading ../apierr/go.mod: no such file or directory". - Verified by reproducing each Dockerfile's build context and running GOWORK=off download+build for all 5 services.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates cross-service duplication in
services/into a shared platform layer, deleteslinter-invisible dead code, and fixes three correctness gaps surfaced by a read-only audit. Five
runnable services were originally scaffolded by copy-paste (~600–700 LOC of structural duplication,
no shared platform layer), plus dead exported symbols the
unusedlinter structurally skips. Allmodules passed
go vet+golangci-lintclean before this work: this epic targets what the linterscannot see.
Delivered as 13 reviewed tickets across 4 dependency-ordered waves. Every ticket landed with
per-module
go build/go test/go vet/golangci-lint(pinnedstandardset) clean,-raceon all concurrency code, and an independent reviewer verification.
What changed
New shared modules (
services/)serverkit— bootstrap + serve/shutdown lifecycle.Servefixes a zombie-on-bind-failure bugpresent in all 5 services (a fatal listener error was swallowed in a log-only goroutine): it now
propagates the first fatal serve error, treats
http.ErrServerClosed/gRPC graceful-stop as successvia
errors.Is, and performs a bounded 10s graceful shutdown.apierr+httpx— single-sourced HTTP error contract (Error{Code,Message,Status,Fields},code constants,
Respondviaerrors.As) and request guards (RequireUserID,BindJSON[T]).This closes a latent bug where auth/finance/datarights silently dropped the
Fieldsslot onvalidation errors.
pgutil— the pgx/pgtype boilerplate each Postgres service re-solved:ParseUUID,IsNoRows(
errors.Is),IsUniqueViolation(errors.Ason23505).Per-service migration (finance, auth, datarights, gateway, expense)
run()bootstraps viaserverkit; localApiError/ServiceError/AuthError/handleErrorreplaced by
apierr.Respond;X-User-ID/ JSON-bind guards replaced byhttpx; gRPC handlertype-assertions converted to
errors.As; Postgres call sites (finance/auth) migrated topgutiland both bespoke
formatUUIDcopies deleted.JWT TTLs);
UpdateProfileown-record-missing now returns 401 to match the four-method majority;role strings → constants.
access.Registry/ProxyPrefixesare no longerreassignable exported globals (injected into
router.New);access.Access→access.Level.Correctness fixes (behavior-changing)
Sort/Type/TagID/DateFrom/DateTo) removed(the repo never read them; the frontend filters client-side). Response byte-identical for the same
inputs.
GetCorrectionHistory/GetProRataGroup) that hardcoded an empty ("")user scope removed (no in-workspace or frontend consumer; REST path is correctly scoped). Dev
in-memory client cross-user select-by-id bypass removed; its silent no-op UPDATE now fails loud.
retryAfterJSON body field replaced by the standardRetry-AfterHTTP header (
apierrstays generic; the frontend parses the unchanged message string).Dead-code sweep
metricsDB collectors (incl. theactive_connectionsgauge that polluted every service's
/metrics),dbmigrate.Run+ thesource/filedriver,access.Classifies,perf.AssertMaxAllocs,finance.CreatePeriod, 21 redundant gRPCUnimplementedstubs (auth'sRegister/Login"use REST" overrides preserved), and assorted deadfields/types.
yearMonthdeduped to a single definition.datarights jobrunner (Slice F)
datarights/internal/jobrunner(Pool+StatusStoreseam + injectedExecutestrategy); theexport/deletion engines stay separate and inject their own strategy.
NewFuncProvider; export finance data fetched exactly once (theguarantee is now structural;
MemoizedFinanceClientdeleted); emailtemplateDatano longermirrors
BrandTokensfield by field.Tickets
serverkitmodule + serve/shutdown lifecycle (zombie fix)apierr+httpxmodules (error contract + guards)pgutilmodule (UUID / no-rows / unique-violation)jobrunnerextraction (Pool + StatusStore)jobrunnerprovider consolidationVerification
go build/go vet/go test/golangci-lint run(v2.12.1,standardset) — all 14 modules clean, 0 lint issues.go test -raceclean on all concurrency code (serverkit.Serve,jobrunner.Pool, the datarightsengines).
fetch;
%w-wrapped error classification in gRPC handlers; real bind-failurerun()tests;apierrFieldspropagation.main: 186 files, +5,435 / −4,347.Backwards compatibility
Fieldsis now populated where it was silently dropped(additive), and the datarights rate-limit response moves
retryAfterfrom the JSON body to thestandard
Retry-Afterheader (the only non-additive change; no consumer read the old body field)./healthnow returns{"status":"ok","service":"<name>"}for services adopting the shared router;probes only check HTTP 200, so this is compatible.
Known follow-ups (non-blocking)
go mod tidywould add additive transitive/go.modhash lines toapierr/httpx/pgutilgo.sum(pre-existing; builds green). Optional cleanup.(removal needs a coordinated cross-module change; generated files are out of epic scope). The C2
security goal is fully met regardless.
services/root is not a Go module, so root-levelgo build ./.../golangci-lint run ./...fail by design — per-module gates are the contract. Candidate for a separate fix.
Notes for reviewers
GoFinConsolidation-Spec/.--no-ffper-ticket merges; review per merge commit for aticket-scoped diff.