Repository navigation
Conversation
The ladder timeline endpoints returned 100-row pages, and every page
re-aggregated the whole account scope, so a client load of n rows cost
about n/100 statements each scanning n rows. The client also compares
total_count and the totals across pages, so an amendment, tranche or run
committed between two page requests failed the whole timeline load
("The ladder timeline changed while loading").
Both GET /api/ladder/tranches and /api/ladder/runs now return every scoped
row, with the counts and totals, from one statement, so a load is one
snapshot and one scan. No next_cursor is issued, so the web client's loop
finishes after its first request; a cursor from the earlier API is still
honored as a lower bound for clients mid-load across a deploy.
Closes LeanerCloud#668
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
type/chore
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.
Root cause
GET /api/ladder/tranchesandGET /api/ladder/runsreturned 100-row pages, and the web client'scompleteTimelinefollowsnext_cursoruntil the last page. This caused two problems that are still open on #668:total_count,total_usd_hrand the on-demand total. A tranches page also shipped every scoped row's amount triple to Go. Loading n rows therefore took ceil(n/100) statements, each scanning all n rows: O(n²/100).completeTimelinechecks thattotal_count,total_usd_hrandtotal_od_usd_hrmatch on every page, that no ID repeats, and that the rows add up to the totals. So any amendment, new tranche or new run committed between two page requests failed the whole load with "The ladder timeline changed while loading. Reload to try again."Fix
The fix is backend only (
internal/config/store_postgres_ladder_timeline.go,internal/config/ladder_timeline.go):LIMITand no second CTE scan. One statement means one snapshot: a concurrent write lands wholly before or wholly after the read.next_cursoris issued (NextCursorandLadderTimelinePageSizeare removed). The unchanged web client therefore finishes after its first request, and its cross-page checks have nothing to disagree about.after_created_at/after_idparameters are still accepted, as a lower bound on the rows only (never on the totals). An old browser bundle that is part-way through a paginated load during the deploy still gets a consistent tail. The handler, the store interface and the mocks are unchanged.fillLadderTimelineOnDemandis untouched), so the results match the old stitched-together pages. The integration test checks this against an independent SQL oracle.Response size: the whole history now travels in one response instead of ceil(n/100) responses. The client already held all of it in memory. Large JSON responses are gzipped by
internal/server/lambda_compress.go(#723), which keeps the Lambda 6 MB limit far above realistic ladder histories (daily cadence, at mostmax_actions_per_runtranches per run).Closes #668
Verification
All of this is local evidence: real Postgres 16 through the shared testcontainers DB (
testhelpers.RequireMigratedDatabase) for the integration tests, and pgxmock for the unit tests. Nothing ran against a deployed environment or a real cloud account.New integration tests, in
internal/config/store_postgres_ladder_timeline_snapshot_integration_test.go. They drive the store exactly likecompleteTimeline: they marshal each response to JSON, follownext_cursor, and apply the same cross-page checks.TestLadderTimelineLoadsWholeScopeInOneRead: 250 tranches and 250 runs. It asserts the client load takes 1 request per endpoint, and that the rows (in(created_at, id)order) andtotal_usd_hrequal a direct SQL oracle.TestLadderTimelineLoadToleratesMidLoadChanges: commits an amendment (throughAmendLadderTranche), a new tranche or a new run between two page requests, and asserts the load succeeds and the next load sees the change.TestLadderTimelineLoadIsSnapshotConsistentUnderConcurrentWrites: one goroutine keeps amending the timeline and adding tranches and runs, while 40 rounds of client loads (tranches and runs, 450 rows each) run concurrently. Every load must be internally consistent, and the loads must have observed more than four distinct states, so the writes really did interleave.Fail before (origin/main
a14dc8f2plus only the new test file, in a temporary worktree):The concurrent test failed on main in 4 of 4 runs. With the one-read assertion turned into a log line, main's 3-page stitched load matched the same SQL oracle (same rows, order and total). The new single read returns identical results.
Pass after (this branch):
Other checks:
The existing
TestLadderTimelineScopedPagesAndSnapshotswas updated: one read now returns all 101 rows, and a legacy cursor narrows the rows but not the totals.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 (per the maintainer's comment on #668)
ErrLadderAmendInvalidmapping: already done in fix(config): map invalid ladder amendments to ErrLadderAmendInvalid #694.FOR NO KEY UPDATEand an explicitON DELETE RESTRICT/ editing migration 000107: deliberately not done, as decided on the issue. This PR touches neither the amend path nor any migration.completeTimelinedoes not cross-check them, so this cannot fail a load. A run and its tranches committed between the two requests would just show up on the next reload.Frontend follow-up for the UI owner
No UI change is needed: the current
completeTimelineworks unchanged and now makes one request per endpoint. Optionally, once this ships, its pagination loop and cross-page "changed while loading" checks could be simplified, because the server no longer sendsnext_cursor.Maintainer: please mirror the issue's triage labels onto this PR:
type/chore,triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/m.🤖 Generated with Claude Code