fix(server): route OIDC in HTTP transport, graceful shutdown, validate env defaults - #1040
Merged
cristim merged 11 commits intoJun 7, 2026
Merged
Conversation
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reached
More reviews will be available in 35 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR hardens server startup robustness by validating payment and ramp-schedule configuration at application initialization, fixes feature parity between Lambda and HTTP transports for OIDC federation endpoints, and implements graceful shutdown on process termination to prevent connection and lock leaks. ChangesServer startup robustness and HTTP transport alignment
Sequence DiagramsequenceDiagram
participant Server as HTTP Server
participant Signal as OS Signal
participant Handler as signal.NotifyContext
participant ListenServe as ListenAndServe
participant Shutdown as server.Shutdown
participant AppClose as app.Close
Server->>Handler: signal.NotifyContext(SIGINT, SIGTERM)
Server->>ListenServe: ListenAndServe (goroutine)
Signal-->>Handler: SIGINT/SIGTERM
Handler->>Shutdown: ctx canceled
Shutdown->>ListenServe: graceful drain (30s timeout)
ListenServe-->>Shutdown: drain complete
Shutdown->>AppClose: deferred cleanup runs
AppClose-->>Server: connections closed, locks released
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/server/http_test.go (1)
419-420: 💤 Low valueRedundant assertion—already covered by line 414.
The assertion on line 414-415 already guarantees
Content-Typestarts withapplication/json. If we reach line 419,ctis already known to be JSON, so this condition will always pass.Consider removing or simplifying:
♻️ Suggested simplification
- // With no signer configured HandleOIDC returns 404 JSON (not 200); - // the important invariant is that it is *not* an SPA-served HTML 200. - testutil.AssertTrue(t, resp.StatusCode != http.StatusOK || strings.HasPrefix(ct, "application/json"), - "SPA fallback must not serve OIDC paths") + // With no signer configured HandleOIDC returns 404 JSON. + testutil.AssertEqual(t, http.StatusNotFound, resp.StatusCode)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/http_test.go` around lines 419 - 420, The assertion using testutil.AssertTrue that checks "resp.StatusCode != http.StatusOK || strings.HasPrefix(ct, \"application/json\")" is redundant because the earlier assertion already guarantees ct starts with "application/json"; remove this assertion (the testutil.AssertTrue call referencing resp.StatusCode, http.StatusOK, ct and strings.HasPrefix) or simplify it to only assert resp.StatusCode != http.StatusOK so the SPA fallback test only enforces that OIDC paths are not served with HTTP 200.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/server/http_test.go`:
- Around line 419-420: The assertion using testutil.AssertTrue that checks
"resp.StatusCode != http.StatusOK || strings.HasPrefix(ct,
\"application/json\")" is redundant because the earlier assertion already
guarantees ct starts with "application/json"; remove this assertion (the
testutil.AssertTrue call referencing resp.StatusCode, http.StatusOK, ct and
strings.HasPrefix) or simplify it to only assert resp.StatusCode !=
http.StatusOK so the SPA fallback test only enforces that OIDC paths are not
served with HTTP 200.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 369ba0d9-cd29-455a-91ff-35f8ff76db20
📒 Files selected for processing (6)
cmd/server/main.gointernal/config/validation.gointernal/server/app.gointernal/server/app_test.gointernal/server/http.gointernal/server/http_test.go
11 tasks
Member
Author
|
Addressed CR review findings:
All 294 tests in internal/server pass; gofmt/vet/build clean. @coderabbitai review |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
This was referenced Jun 7, 2026
Member
Author
FOLD-1040 additions (issue #1067)Seven additional commits implementing the remaining findings from code-review report 04 (server/config) and report 06 (analytics).
Verification:
@coderabbitai review |
Contributor
|
Kicking off a fresh review pass now. 🧠 Learnings used✅ Action performedReview finished.
|
…shutdown Closes #1024: /oidc/.well-known/* was only wired in the Lambda transport (handleLambdaHTTPEvent); the HTTP/container transport (Cloud Run, Container Apps) had no OIDC registration so requests fell to the SPA file server and returned HTML or 404 -- breaking federated-credential trust chains. Fix: register handleOIDCHTTP at api.OIDCBasePath+"/" in CreateHTTPServer, placed before the static/SPA handler so the ordering matches handleLambdaHTTPEvent (D1 anti-drift). Also replace the double staticDirFromEnv() call with app.staticDir (single source of truth, M2). Closes #1025: StartHTTPServer blocked on ListenAndServe with no signal handling, so SIGTERM (normal stop in Cloud Run/Container Apps/Fargate) killed the process before deferred app.Close() could drain the DB pool and release session-pinned advisory locks. Fix: wire signal.NotifyContext(SIGINT,SIGTERM), run ListenAndServe in a goroutine, on signal call srv.Shutdown with a 30s bounded timeout. Regression tests added: TestHTTPTransportServesOIDCEndpoints confirms the HTTP transport returns application/json (not HTML) for OIDC paths; TestHandleOIDCHTTP verifies the bridge handler itself. Graceful-shutdown unit test is not feasible without a live server -- wiring is exercised through code review of StartHTTPServer.
… startup boundary Closes #1026: DEFAULT_PAYMENT_OPTION and DEFAULT_RAMP_SCHEDULE env values were taken raw by LoadApplicationConfig and passed to purchase.NewManager without validation, despite config.ValidPaymentOptions / config.ValidRampScheduleTypes existing. A typo ("AllUpfront", "Immediate") silently became the system-wide purchase default -- a money-moving misconfiguration caught only at AWS purchase time (noisy, late) or never. Fix: add config.ValidatePaymentOptionEnv / config.ValidateRampScheduleEnv exported at the config package boundary, called from a new validateAppConfigEnvDefaults helper in NewApplicationFromDeps. Invalid non-empty values cause a startup error (consistent with the scheduledauth / ADMIN_PASSWORD_SECRET fail-fast posture in the same file). Empty value remains valid. Also fix M1: getEnvInt / getEnvFloat in app.go and getTaskTimeout in cmd/server/main.go silently swallowed strconv parse errors, leaving operators who fat-fingered a tuning env var believing their value was in effect. All three now log WARNING with the key name and the default value being used. Regression tests added: TestNewApplicationFromDepsValidatesEnvDefaults (invalid payment option and ramp schedule each cause a startup error with the env var name in the message); TestGetEnvIntLogsOnBadValue and TestGetEnvFloatLogsOnBadValue confirm WARNING is emitted on parse failure. Existing TestNewApplicationFromDeps updated to use canonical "all-upfront" (was "AllUpfront", now correctly rejected).
The previous assertion (resp.StatusCode != http.StatusOK || strings.HasPrefix(ct, "application/json")) was a tautology: line 414 already guarantees ct starts with application/json, so the OR could never fail. Replace it with an explicit AssertEqual to http.StatusNotFound so the test fails loudly if an OIDC path ever falls through to the SPA fallback. Addresses CodeRabbit nitpick on http_test.go:419-420.
…daRuntime() wrapper The isLambdaRuntime() wrapper existed to keep call sites unchanged during an earlier refactor but its own comment said new code should call runtime.IsLambda() directly (04-N3). Remove the wrapper and inline runtime.IsLambda() at all three app.go call sites. cmd/server/main.go read AWS_LAMBDA_RUNTIME_API directly despite the runtime package existing to encapsulate exactly this detection rule (04-M5). Replace with runtime.IsLambda() to keep the detection logic consistent if the rule changes in future. Part of #1067.
…rror on unknown event 04-L4: handleHealthCheck silently discarded the json.Encode return value while handleScheduledHTTP already logged it. Log the error for parity; the headers are already sent so the only recovery is a log entry. 04-L5: handleScheduledHTTP re-derived the task-type segment by splitting the path and indexing parts[2], which requires a length guard and breaks if the route prefix changes. Replace with strings.TrimPrefix against the known prefix; reject empty or slash-containing results. 04-N4: unrecognised Lambda payloads were silently routed to the scheduled handler, which then failed with "unknown scheduled task action" masking the real root cause. Return a distinct "unrecognised Lambda event shape" error so operators see the actual problem in the logs. Regression test: TestHandleLambdaEvent_UnknownEventReturnsError. Part of #1067.
…Content 04-M6: spaHandler.ServeHTTP (HTTP transport) and resolveStaticFilePath (Lambda transport) implemented two different directory-containment checks. spaHandler delegated containment to http.ServeFile's built-in protection while resolveStaticFilePath used strings.HasPrefix(absFile, absDir) without a trailing separator -- allowing a sibling directory sharing a name prefix to pass the check. Unify by delegating spaHandler to resolveStaticFilePath and fix the prefix check to include os.PathSeparator. 04-L2: hasFileContent was dead code. The doc comment claimed it was used at startup to validate STATIC_DIR, but staticDirFromEnv does that work and never called hasFileContent. Remove it and its three tests. Regression test: TestResolveStaticFilePath_SiblingDirBlocked. Part of #1067.
…env round-trip
Both cmd/server and cmd/lambda pushed the build-time Version into
os.Setenv("VERSION",...) so LoadApplicationConfig could read it back.
This round-trip was awkward (cmd/lambda even had a thread-safety caveat)
and unnecessary once the constructor accepts the value directly (04-N1).
Add a version parameter to NewApplication: when non-empty it overrides
the env-sourced value; "" falls back to os.Getenv("VERSION") for
callers that do not have a build-time constant. Both cmd entrypoints
now pass their ldflags-stamped Version directly.
Update TestInitApp_SetsVersion to reflect the new behaviour (VERSION
env var is intentionally not set by initApp).
Part of #1067.
04-M4: when SCHEDULED_TASK_SECRET_NAME is configured but the SecretResolver (Key Vault, Secrets Manager) fails, resolveScheduledTaskSecret logged the error and fell back to the empty SCHEDULED_TASK_SECRET env var. In bearer mode buildScheduledAuth then raised "bearer mode requires SCHEDULED_TASK_SECRET" -- pointing the operator at the wrong env var and hiding the real cause. Fix: - resolveScheduledTaskSecret now returns (string, error); the error is non-nil when a SecretName is configured and resolution fails. - NewApplicationFromDeps loads the scheduledauth config before resolving the secret so it knows the mode. In bearer mode, a non-nil resolver error is propagated immediately with the original cause and the secret name in the message. - buildScheduledAuth is replaced by buildScheduledAuthFromConfig which accepts the pre-loaded Config, avoiding a duplicate LoadConfig call. Regression test: TestNewApplicationFromDeps_BearerModeSecretResolutionFails. Part of #1067.
…tion struct fields 04-M3: migrationsTimeout and runMigrations were package-level mutable vars used as test-injection points, with a "MUST NOT call t.Parallel()" comment the compiler cannot enforce. A future contributor adding t.Parallel() to a migration test would trigger a data race. Fix: - Add migrationsTimeout time.Duration and runMigrationsFunc fields to Application; set them in NewApplicationFromDeps. - Add app.runMigrationsBounded() method that delegates to runMigrationsBoundedWith(), a pure function accepting a runner argument. - Remove package-level migrationsTimeout var and runMigrations var. - Update tests to pass a fake runner directly to runMigrationsBoundedWith and add t.Parallel() calls (now safe). Regression test: TestEnsureDB_UsesInstanceMigrationsTimeout -- two Application instances with different timeouts run in parallel. Part of #1067.
…urface error in result 06-M4 (migration half): REFRESH MATERIALIZED VIEW CONCURRENTLY cannot run inside a transaction block. Migration 000003 called refresh_savings_materialized_views() which used CONCURRENTLY for all three views -- this would fail at every fresh deploy. Replace with plain (non- concurrent) REFRESH statements directly in the migration body. Reserve CONCURRENTLY for the runtime scheduled-task path (refresh_savings_ materialized_views()) which runs outside any transaction. 06-M4 (server half): handleRefreshAnalytics only logged a view-refresh failure as "Warning" with no way for the API caller or the operator reading the scheduled-task response body to see the cause. Add "views_error" to the result map so the error is visible in the response alongside the existing server-side log. Part of #1067.
CI pre-commit checks flagged two issues after the FOLD-1040 commits: gofmt: trailing newline in static.go and misaligned struct fields in app.go (migrationsTimeout/runMigrationsFunc). Fixed by gofmt -w. Cyclomatic complexity > 10: - resolveStaticFilePath (11): extract isPathContainedIn helper that encapsulates the separator-aware prefix check (04-M6). Reduces the compound &&-condition branch inside resolveStaticFilePath. - NewApplicationFromDeps (12): extract initScheduledAuth helper that consolidates the scheduledauth.LoadConfig / resolveScheduledTaskSecret / buildScheduledAuthFromConfig / Warmup sequence added for 04-M4. The function reads as one call at the call site instead of four nested if/err branches. Part of #1067.
cristim
force-pushed
the
fix/server-transport-config
branch
from
June 7, 2026 18:24
3407fd0 to
e50df95
Compare
Member
Author
Rebase onto feat/multicloud-web-frontend (post #1049)Rebased Conflict resolved: single conflict in Verification: @coderabbitai review |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
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
/oidc/.well-known/openid-configuration,/oidc/.well-known/jwks.json) were only wired in the Lambda transport; the HTTP/container transport (Cloud Run, Container Apps) had no registration so requests fell to the SPA file server and returned HTML or 404, breaking federated-credential trust chains. Fix: registerhandleOIDCHTTPatapi.OIDCBasePath+"/"inCreateHTTPServer, ordered before the SPA handler (mirrorshandleLambdaHTTPEvent, closes D1 transport-drift gap). Also fixes M2: readsapp.staticDirinstead of re-invokingstaticDirFromEnv().StartHTTPServerblocked onListenAndServewith no signal handling; SIGTERM (normal container stop) killed the process before deferredapp.Close()could drain the DB pool and release session-pinned advisory locks. Fix:signal.NotifyContext(SIGINT, SIGTERM), goroutine forListenAndServe,srv.Shutdownwith 30s bounded timeout on signal.DEFAULT_PAYMENT_OPTIONandDEFAULT_RAMP_SCHEDULEenv values were taken raw and passed topurchase.NewManagerwithout validation despiteValidPaymentOptions/ValidRampScheduleTypesexisting. A typo propagated silently as a system-wide purchase default. Fix:config.ValidatePaymentOptionEnv/config.ValidateRampScheduleEnvexported at config boundary; called fromvalidateAppConfigEnvDefaultsinNewApplicationFromDeps(fail-fast, consistent with scheduledauth/ADMIN_PASSWORD_SECRET). Also fixes M1:getEnvInt/getEnvFloat(app.go) andgetTaskTimeout(cmd/server/main.go) now log WARNING on parse failure instead of silently swallowing it.Changed files
internal/server/http.gohandleOIDCHTTP, register at/oidc/, useapp.staticDir, add graceful shutdown toStartHTTPServerinternal/server/app.govalidateAppConfigEnvDefaults, call inNewApplicationFromDeps, fixgetEnvInt/getEnvFloatto warn on bad parseinternal/config/validation.goValidatePaymentOptionEnv/ValidateRampScheduleEnvexported boundary validatorscmd/server/main.gogetTaskTimeoutto warn on bad parseinternal/server/http_test.goTestHTTPTransportServesOIDCEndpoints,TestHandleOIDCHTTPinternal/server/app_test.goTestNewApplicationFromDepsValidatesEnvDefaults,TestGetEnvIntLogsOnBadValue,TestGetEnvFloatLogsOnBadValue; fix existing test using canonical payment option spellingTest plan
go build ./...-- cleango vet ./internal/server/... ./internal/config/... ./cmd/server/...-- cleango test ./internal/server/... ./internal/config/... ./cmd/server/...-- 912 passed, 0 failedTestHTTPTransportServesOIDCEndpoints-- verifies OIDC paths returnapplication/json, not HTML, in HTTP transport (pre-fix: would have served SPA fallback)TestHandleOIDCHTTP-- verifies bridge handler returns JSONTestNewApplicationFromDepsValidatesEnvDefaults-- verifies"AllUpfront"/"Immediate"(wrong-case typos) are rejected at startup with the env var name in the errorTestGetEnvIntLogsOnBadValue/TestGetEnvFloatLogsOnBadValue-- verify WARNING is logged on malformed env varsignal.NotifyContext, goroutine,srv.Shutdownwith bounded timeout) has been code-reviewed against the patternSummary by CodeRabbit
Bug Fixes
New Features
Improvements
Scope expanded
Added 7 commits implementing the remaining FOLD-1040 findings from report 04 (server/config) and report 06 (analytics). Tracked by issue #1067.
04-M5 + 04-N3 — runtime.IsLambda() used directly; isLambdaRuntime() wrapper removed
cmd/server/main.gowas readingAWS_LAMBDA_RUNTIME_APIdirectly, bypassing theruntime.IsLambda()helper that exists to keep the detection rule consistent. TheisLambdaRuntime()wrapper inapp.gohad a "do not use" comment but was still called at 3 sites. Removed the wrapper and usedruntime.IsLambda()everywhere.04-L4 + 04-L5 + 04-N4 — health encode log; scheduled task path; unknown event error
handleHealthChecknow logs thejson.Encodeerror for parity withhandleScheduledHTTP.handleScheduledHTTPreplaced manualparts[2]indexing withstrings.TrimPrefix.04-L2 + 04-M6 — dead hasFileContent removed; unified static-file containment check
hasFileContentwas dead code with a misleading comment. Removed.spaHandler.ServeHTTP(HTTP path) andresolveStaticFilePath(Lambda path) used divergent containment checks. Unified by delegatingspaHandlertoresolveStaticFilePath. Fixed the prefix check to includeos.PathSeparator(prevents sibling-dir confusion).04-N1 — Version threaded directly via NewApplication; env round-trip removed
Both
cmd/serverandcmd/lambdapushed the build-timeVersionintoos.Setenv("VERSION",...)and read it back. ChangedNewApplicationto accept a version parameter;""falls back to the env for callers without a build-time constant.04-M4 — Bearer-mode secret resolution error propagated at startup
When
SCHEDULED_TASK_SECRET_NAMEresolved but the Key Vault / Secrets Manager lookup failed, startup fell through to the misleading "bearer mode requires SCHEDULED_TASK_SECRET" error.resolveScheduledTaskSecretnow returns(string, error);NewApplicationFromDepsloads the auth mode config first and propagates resolver errors in bearer mode with the actual cause.04-M3 — migrationsTimeout/runMigrationsFunc moved to struct fields (parallel-safe)
Package-level
migrationsTimeoutandrunMigrationsvars required tests to avoidt.Parallel(). Moved toApplicationstruct fields set by the constructor. Tests now pass a fake runner torunMigrationsBoundedWith()directly and can callt.Parallel().06-M4 — Non-concurrent MV refresh in migration; error surfaced in API response
refresh_savings_materialized_views()usesCONCURRENTLYwhich is illegal inside a transaction. Migration 000003 called this function at schema-apply time. Replaced with plainREFRESH MATERIALIZED VIEWstatements in the migration. Also added"views_error"to thehandleRefreshAnalyticsresult map so refresh failures are visible in the API response, not only in server logs.Also closes #1067