Repository navigation
Conversation
- analytics_refresh reports status "skipped" when the analytics store is absent and no longer returns partition counters it never writes; cleanup_expired_records drops the always-0 sessions_deleted counter. - The HTTP transport reads request bodies through http.MaxBytesReader at the API router's own 1 MB cap: an oversize body is a 413 and a body read error (client disconnect mid-upload) is a 400, instead of a truncated or empty body being routed. - resolveAccountID returns an error instead of "unknown", and the RI exchange reshape run aborts before touching AWS or the store when no account ID is resolved, so no audit row carries a fabricated account. The fourth finding (empty-name guard in force-delete-owned-ecr-repo.sh) was already fixed on main by LeanerCloud#489. Closes LeanerCloud#326 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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
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
Fixes the three
internal/serversilent-fallback gaps from #326. The fourth finding, in the ECR cleanup script, was already fixed on main. Each finding was checked again against current main (a14dc8f2) before any change was made:a14dc8f2analytics_refreshreportssuccesswith zero counters when the store is absent;sessions_deletedis never assignedhandler.gostill starts withstatus: successandpartitions_created/partitions_dropped= 0, and the nil-store branch only logs.sessions_deletedis still set to 0, never assigned, and logged as a count.api.MaxRequestBodySize) with a JSON 413,/api/*and/versionalready returned 413 for a truncated 10 MB body. Only/oidc/*routed it (404 instead of 413). The transport still buffered up to 10 MB that the router then threw away."unknown"account ID that is written onto exchange audit rowsresolveAccountIDstill returns"unknown"on an STS error or a nilAccount.9b50029c, "test(ci): tighten destroy-script guards and fixtures"). It added thecase "$OWNED_REPO" in '' | *[![:graph:]]*)guard before the first AWS call, plus stubbed tests inscripts/test-ecr-delete-selection.sh.Root cause and fix
internal/server/handler.go):handleRefreshAnalyticsnow reportsstatus: "skipped"whenapp.Analyticsis nil, the same statushandleCollectAnalyticsuses for that case. It no longer returnspartitions_created/partitions_dropped, because this task never creates or drops partitions (collect_analyticsdoes).views_refreshedstays, since it is populated.handleCleanupExpiredRecordsno longer returns or logssessions_deleted, becauseCleanupExpiredSessionsreports no count.internal/server/http.go, newinternal/server/http_body.go):httpToLambdaRequest(w, r)now returns an error.http.MaxBytesReader, capped atapi.MaxRequestBodySize(the router's own 1 MB limit, so the transport and the router can't disagree).handleHTTPRequest,handleVersion,handleOIDCHTTP) answer a failed read withwriteBodyReadError:{"error":"Request body too large"}, so the status and shape seen by/api/*clients are unchanged;{"error":"Failed to read request body"}.http.go(already over the 500-line guideline) grows only by its call sites.internal/server/handler_ri_exchange.go):resolveAccountIDtakes a narrowcallerIdentityGetterinterface (satisfied by*sts.Client). It returns an error on an STS failure, a nil output, or a nil or emptyAccount, asresolveLadderIdentityalready does.handleRIExchangeReshaperesolves the account before it builds any EC2 client and aborts the run if that fails.executeRIExchangeReshaperefuses an emptyaccountIDbefore it lists RIs or touches the store. A caller that drops the error still can't write a blank account onto audit rows."unknown"behavior, is updated.Closes #326
Verification
All of the evidence below is unit / mock / fixture evidence. No real AWS, STS or Postgres calls were made. Environment:
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.Fail before. The new and changed tests were copied onto a
git archiveof maina14dc8f2. Tests that call the new signatures (httpToLambdaRequest(w, r)andresolveAccountID(ctx, client)) don't compile against main, so they were left out of this run.Pass after, on this branch:
All of the tests above pass, plus:
TestHttpToLambdaRequest_BodyAtLimitIsKeptWhole: a body of exactly 1 MB is accepted whole.TestHttpToLambdaRequest_OversizeBodyIsAnError: 1 MB + 1 gives*http.MaxBytesError. This test also catches a mutation that raises the cap back to 10 MB, which the handler-level test alone would miss because the router's own 413 hides it.TestHttpToLambdaRequest_ReadErrorIsReturnedTestResolveAccountID_RefusesToFabricateAnAccount(STS error, nil output, nil account, empty account)TestResolveAccountID_ReturnsTheCallerAccountA14-042 (already fixed), checked offline only. The script was never run against real AWS.
scripts/test-ecr-delete-selection.shruns it end to end against stubbedterraformandawsbinaries onPATH:Mutation probe on a throwaway copy, with the
case "$OWNED_REPO"guard deleted: both cases above FAIL (57 passed, 2 failed). The existing test therefore pins the guard.shellcheck0.11.0 onscripts/force-delete-owned-ecr-repo.shis clean.scripts/test-ecr-delete-selection.sh(unchanged) shows only pre-existing info-level SC1091 and SC2016 notes.No DB or migration changes and no new integration tests: none of the fixes touches a query. The RI-exchange path change happens before any store call.
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.Behavior changes worth knowing
analytics_refreshwith no analytics store:"status":"skipped". The outer HTTP envelope fromhandleScheduledHTTPstill says"status":"success"(the task ran without error), exactly as forcollect_analytics's existing"skipped".partitions_created,partitions_droppedandsessions_deletedare gone from the result maps. I found no consumer of them in the repo, includingfrontend/, Terraform and docs (other than the audit report)./api/*and/version, because the router already returned 413 above 1 MB./oidc/*(GET-only endpoints) now answers 413 for a body over 1 MB and 400 for a read error instead of routing it.Content-Type: application/jsonand the security headers from thesecurityHeadersmiddleware, but not the router's CORS headers. The SPA is same-origin, so this shouldn't matter.account_id = "unknown". The stale-exchange alarm sweep still runs first, because it comes before the AWS config and STS step.Out of scope
handleScheduledHTTP's outer"status":"success"envelope wraps any inner status (skipped,partial,disabled). That is the shared contract for every scheduled task, so I left it alone.httpToLambdaRequest, so it is unaffected.Maintainer: please mirror the issue's triage labels onto this PR:
impact/internal,triaged,priority/p3,severity/low,urgency/eventually,effort/s,type/bug.🤖 Generated with Claude Code