Repository navigation
fix(server,scheduler): first slice of the 2026-07-28 review findings (#111) - #1275
Conversation
- scheduler: an unknown provider name in EnabledProviders is now a failed provider, so it lands in FailedProviders and last_collection_error instead of reading as a clean collection of zero recommendations. - config: UpsertRecommendations sets last_collected_at with GREATEST, so an older collection that commits after a newer one can no longer move freshness backwards and trigger another full collection. - server: /health and /ready report a fixed message plus a stable reason code (db_unreachable, auth_store_unreachable, migration_failed) and keep the raw pgx/migrate error in the server log only. - server: runScheduledTask rejects a task name outside the roster before connecting or taking an advisory lock keyed on it (covers --task). - cmd/server: an explicit RUNTIME_MODE now outranks Lambda auto-detection; RUNTIME_MODE=auto is treated as unset rather than unrecognized. Refs LeanerCloud#111 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
effort/l
|
Warning Review limit reached
This review includes 13 billable files and costs up to $3.25. Or wait 58 minutes for your next included review. View limit details
Comment |
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB5. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on |
First slice of the #111 tracking issue. It covers the
internal/server,internal/schedulerandcmd/serverfindings that are still open on current main and don't overlap an in-flight branch. Every finding was re-checked againstorigin/mainata14dc8f2. The table below gives the status of each one, and the ones not fixed here are listed with the reason.Refs #111
Per-finding status
.tfvarsrendered with unescaped self-service values (MEDIUM)addBundleTerraformrenders throughhclEscapeData(data), andhandler_federation_controlchar_test.gocovers itGlobalConfig.validateProviders, called fromhandler_config.go)/healthreturns raw internal error strings (MEDIUM)encryptionKeySourcesibling is no longer in the/healthbodylast_collected_atbackwards (MEDIUM)ListRecommendations/GetRecommendationByID(MEDIUM)ListRecommendationsStrict, which fails closed, for the automated plan-step money path. The dashboard read path still over-shows. Deferred because open PR #1240 rewritesListRecommendationsaround the sameapplySuppressionshandling and addsScanRecommendations, which deliberately copies the over-show behavior. Fixing one path now would leave the two inconsistent once #1240 merges. Choosing between failing the read and zeroingCountis also a UI-visible decision for the ownerhandleScheduledHTTPcallsParseScheduledTaskTypeand returns 400 beforeensureDB. Remaining--taskCLI path fixed here, becauserunScheduledTasknow validates before connecting or lockingRUNTIME_MODEsilently ignored whenAWS_LAMBDA_RUNTIME_APIis set (LOW)IsLambdaused as a proxy for three environment properties (LOW)html/template(LOW), plus A06-005 (executed-notification renderer)internal/email). Still present on main (templates.go:909,:1045)internal/email). Still present (sender.go:250,smtp_sender.go:151/153/182/184)internal/email). Needs a per-scope policy tableinternal/email). This changes delivery on Azure/GCP deploymentswriteAzureTemplateFilespassesshellEscapeData(data)buildSuppressionsdefaults the grace period on aGetGlobalConfigerror (LOW)internal/api,handler_purchases.go:2584and:3503). Still presentThe issue is labeled effort/l, and fixing everything still open would be well over 400 changed lines across
internal/emailandinternal/api, so this PR stops at the server/scheduler slice. Theinternal/emailitems (9 to 12) and A01-016 are a natural second slice.Root causes and fixes
internal/scheduler/scheduler.go). Thedefaultbranch ofcollectProviderRecommendationslogged a warning and returnednilerror, so the merge loop counted the provider as successful andpersistCollectionnever wrotelast_collection_error. It now returnsunknown provider "<name>" (valid: aws, azure, gcp). The provider then shows up inFailedProviders, and the freshness banner shows the error./healthraw errors (internal/server/health.go). The migration, DB-ping and auth-ping checks puterr.Error()into the public body. Each one now returns a fixedmessageand a newreasonfield (migration_failed,db_unreachable,auth_store_unreachable). The DB and auth errors are logged server-side;ensureDBalready logs the migration error. The top-levelstatuscontract (fix(infra): deployment checks and GCP uptime monitor gate on tautological /health 200 #649) and the/readysemantics (fix(server/azure): exclude uninitialized replicas from readiness #488) are unchanged. TheHealthCheckschema inopenapi.yamlgains the optionalreasonproperty.internal/api/health.go(/api/health) already used generic messages and is untouched.last_collected_at(internal/config/store_postgres_recommendations.go). The stateUPDATEinUpsertRecommendationsnow setslast_collected_at = GREATEST(last_collected_at, $1).GREATESTignores the NULL of a never-collected row. Concurrency: the state row is a single row, so two overlapping upserts serialize on its row lock. Under READ COMMITTED the waitingUPDATEre-evaluatesGREATESTagainst the newly committed value, so the newest timestamp survives whichever transaction commits last. The integration test exercises exactly that interleaving on real Postgres.ReplaceRecommendationsis left alone because open PR fix(db): remove ReplaceRecommendations and its unscoped DELETE #1209 deletes it.--taskpath (internal/server/handler.go).runScheduledTasknow callsParseScheduledTaskTypebeforeensureDBand beforeTryAdvisoryLock. The Lambda and HTTP transports already validated at their boundaries; this closes the CLI path, which cast--taskstraight toScheduledTaskType, and covers any future caller. Task names and constant values are identical, so valid callers are unaffected.RUNTIME_MODE(cmd/server/main.go). Precedence is now: the--modeflag, then an explicitRUNTIME_MODE(lambda/http), thenIsLambda()auto-detection.RUNTIME_MODE=auto(the Dockerfile default) or an empty value means auto-detect and no longer logs the "unrecognized" warning. Every deployed module sets either a value that matches its platform (lambdaon Lambda,httpelsewhere) orauto, so deployed behavior doesn't change.scripts/entrypoint.shalready gave the explicit value precedence.Overlap with in-flight branches
I checked each of these with
git diff origin/main...<branch>/gh pr diff: #1099, #1141, fork branchesfix/issue-326,fix/issue-343,fix/issue-337,fix/issue-312, and localfix/issue-339(not on the fork yet). I also checked open PRs #1209, #1239 and #1240, which touchstore_postgres_recommendations.go/scheduler.go. Results:git merge-tree --write-tree HEAD <branch>is clean for all ten.fix/issue-339changescheckConfigStore's signature. The new integration test goes throughcollectHealthso it compiles either way. The merged tree (this branch +fix/issue-339) passesgo vet -tags integration ./internal/server/and the health unit tests.Verification
Unit and mock evidence, plus real-Postgres integration evidence for #3 (DB path) and #4. Everything ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.go build ./...,go vet ./...,go vet -tags integration ./...: cleango test -count=1on each of./internal/scheduler/,./internal/server/,./internal/config/,./cmd/server/,./internal/api/,./internal/mocks/: allokgolangci-lint run --allow-parallel-runners ./cmd/server/ ./internal/scheduler/ ./internal/server/ ./internal/config/with and without--build-tags integration(v2.10.1): 0 issuesgocyclo -over 10 -ignore "_test\.go"on the five changed non-test files: no outputFail-before / pass-after. "Before" means the production file reverted to
origin/mainwith the new tests kept. Forhealth.goit also keeps the newReasonfield and constants so the tests compile; no behavior is added.TestScheduler_CollectRecommendations_UnknownProvider,TestScheduler_CollectProviderRecommendations/unknownSuccessfulProviderswas[AWS azure ],FailedProvidersempty, no error recordedTestHealth_DoesNotExposeRawErrors(/healthand/ready),TestCheckMigrations_Faileduser=cudly_app database=cudly,10.0.3.7:5432and the migration filename / SQLSTATETestCheckConfigStore_DBFailureHidesRawError(integration, real Postgres, closed pgx pool)Message: "Database health check failed: database ping failed: closed pool"TestPostgresStore_UpsertRecommendations_LastCollectedAtNeverMovesBackwards(integration)last_collected_atmoved back 3 minutesTestPostgresStore_UpsertRecommendations_LastCollectedAtConcurrentCommit(integration: newer write holds the row lock, older upsert blocks on it, then the newer commits)TestHandleScheduledTask_UnknownTaskRejectedBeforeLocklockCalls= 1), error came from dispatchTestHandleScheduledTask_KnownTaskStillLocksguards that valid tasks still lock)TestDetermineRuntimeModeRUNTIME_MODE=httpwithAWS_LAMBDA_RUNTIME_APIset returnedlambdaThe integration tests ran narrowly:
Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope / notes
last_collected_atmonotonic. An older collection that commits after a newer one still overwrites the overlapping rows with its own payloads and clearslast_collection_error/last_collection_partial. Fixing that needs an ordering token for the whole collection, not just the timestamp.last_collected_atin the future until real time catches up. Collection timestamps come fromtime.Now()on whichever replica ran the collection, so this was already possible for a single write.HealthCheck.statusenum inopenapi.yaml(healthy, unhealthy) doesn't listpending/failed/disabled/degraded, which the endpoints already return. That's pre-existing and left alone.No frontend change is needed.
Maintainer: please mirror the issue's triage labels onto this PR:
effort/l,triaged,priority/p3,severity/low,urgency/eventually,impact/many,type/bug.🤖 Generated with Claude Code