Repository navigation
Cut sequential D1 round trips on the hottest endpoints - #77
Merged
Merged
Conversation
D1 runs these queries in well under a millisecond, but each one is a network round trip from the edge to the database, which costs tens of milliseconds. Several endpoints made them one after another in loops. - GetMailList: 2 + 3 queries per conversation -> 1 query - GetBBSMentionList: 1 + 2 per mention -> 1 query - GetPosts: count and page in one batch, which also keeps both on one snapshot; last reply found through idx_bbs_reply_post_time instead of a window over every reply - NewReply/NewPost: captcha check overlaps the read-only lookups - NewReply/EditReply: @-mention existence checks and mention writes run in parallel; recipients are de-duplicated first so two writes for the same person can't both insert - DeletePost: one DELETE for the replies instead of one per reply - CheckSessionID: an upsert instead of count + insert, and a session xmoj has already rejected is no longer checked a second time; only a fetch that fails or errors is retried Also records the hand-made production indexes as a migration, so local and fresh databases match production. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017dqWu3YgeVRGwBt5pXDq4A
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer's GuideThe PR reduces latency on the hottest backend paths by replacing sequential D1 and upstream calls with consolidated SQL, D1 batches, and parallel promises, while preserving snapshot consistency and error ordering. It also narrows legacy session retries to transient xmoj failures, records production indexes in migrations, and adds targeted regression coverage. Sequence diagram for batched post listingsequenceDiagram
participant Client
participant Process
participant D1
Client->>Process: GetPosts
Process->>D1: batch([COUNT query, page query])
D1-->>Process: CountResult and PageResult
Process-->>Client: Posts with pagination metadata
Sequence diagram for parallel reply validationsequenceDiagram
actor User
participant Process
participant XMOJ
participant D1
User->>Process: NewReply
par Validate captcha
Process->>XMOJ: VerifyCaptcha
XMOJ-->>Process: CaptchaResult
and Load post
Process->>D1: Select
D1-->>Process: PostResult
and Check lock
Process->>D1: GetTableSize
D1-->>Process: LockResult
end
Process->>Process: FindMentionedUsers
par Check distinct mentioned users
Process->>XMOJ: IfUserExistChecker
XMOJ-->>Process: Exist
and Check another mentioned user
Process->>XMOJ: IfUserExistChecker
XMOJ-->>Process: Exist
end
Process->>D1: Insert reply
par Write de-duplicated mentions
Process->>D1: AddBBSMention
Process->>D1: AddBBSMention
end
Process-->>User: Result
Sequence diagram for legacy session retry handlingsequenceDiagram
participant Process
participant XMOJ
participant D1
Process->>XMOJ: CheckSessionID
alt Session verified
XMOJ-->>Process: HTTP 2xx with username
Process->>D1: Insert with OrReplace
D1-->>Process: Session recorded
else Fetch failure or non-2xx response
XMOJ-->>Process: Failed fetch or HTTP error
Process->>XMOJ: CheckSessionID
XMOJ-->>Process: Result
else Session rejected in valid response
XMOJ-->>Process: HTTP 2xx without matching username
Process-->>Process: No retry
end
Flow diagram for consolidated read endpointsflowchart LR
A[Read endpoint] --> B{Data access pattern}
B -->|GetMailList| C[One SQL query with window functions]
B -->|GetBBSMentionList| D[One SQL query with joins and subqueries]
B -->|GetPosts| E[D1 batch]
E --> F[Count query]
E --> G[Page query using reply index]
C --> H[Lower network round trips]
D --> H
F --> H
G --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b3cc20e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…licate index - @-mention existence checks and mention writes run at most four at a time. Content can name any number of users, so firing every check at once let one request start a burst as large as its input. - NewReply/NewPost settle the captcha check and the lookups beside it with allSettled, so a failed database read can no longer hide a failed captcha. - 0007 no longer adds idx_phpsessid: token is the primary key, which SQLite already indexes. - CLAUDE.md: the round-trip rule targets per-item queries, not loops where each query needs the previous one's result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017dqWu3YgeVRGwBt5pXDq4A
Contributor
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Requires human review: Performance refactor to cut sequential D1 round trips via batching, window functions and bounded concurrency; also changes session retry/upsert and adds migration 0007. Needs human review due to database migration and auth/session behavior changes.
View guided diff | Turn on auto-fix | Re-trigger cubic
# Conflicts: # test/process.test.js
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.
What does this PR aim to accomplish?:
Makes the backend faster. Production D1 stats for the last 7 days show SQL time isn't the problem: the hottest query (the
phpsessidlookup, about 38k runs) averages 0.2 ms. The cost is the network round trip for each query, from the edge to the database. Probe requests from mainland China landed in SEA/SJC, and one D1 read added roughly 50–80 ms there. Several endpoints made those round trips one after another, often in loops.How does this PR accomplish the above?:
Round trips per request, before → after:
GetMailListGetBBSMentionListGetPostsbbs_reply)batch(), last reply looked up throughidx_bbs_reply_post_timeNewReply/NewPostallSettled); a failed read can't hide a failed captcha@userchecks and mention writes (NewReply,EditReply)DeletePostINSERT OR REPLACE(Database.Insertgains anOrReplaceflag)GetPostsstill reads its count and page from one snapshot. A D1 batch runs as one transaction, which replaces the shared-session trick the old comment described.Small behavior changes:
PageinGetPostsnow returns a page instead of an SQL error.migrations/0007_add_existing_indexes.sqlrecords three indexes that were created by hand in production but never added tomigrations/. It uses the same names withIF NOT EXISTS, so it does nothing in production. A fourth production index,idx_phpsessid, is left out because it duplicates the primary key's own index. It makes local and fresh databases match.Testing:
npm test: 155 pass, 0 fail. 7 new tests cover the retry rule (in both directions), the single upsert, mention de-duplication, the one-batchGetPosts, captcha-first error reporting and the 4-at-a-time mention bound. The 4 that test new behavior fail on the old code; the one that checks retries still happen when xmoj doesn't answer passes on both, as intended.GetPosts(three filters × three pages),GetBBSMentionListandGetMailListwere run side by side on a real SQLite, built from the migrations plus the production indexes and seeded with random data. That includes ties onreply_time/send_time, mentions of deleted posts and replies, and self-conversations. All 168 comparisons matched across 8 seeds.EXPLAIN QUERY PLANconfirms the newGetPostsuses the index for all three subqueries.wrangler deploy --dry-runbuilds.Not included: Smart Placement and moving the D1 primary out of APAC. Both depend on where real users' requests land, which should be checked in the Workers dashboard first.
Does this PR need a documentation update?:
No.
CLAUDE.mdgains a note on avoiding sequential round trips.By submitting this pull request, I confirm the following:
devfor XMOJ-Script,masterfor everything else).git rebase)🤖 Generated with Claude Code
https://claude.ai/code/session_017dqWu3YgeVRGwBt5pXDq4A
Summary by Sourcery
Reduce sequential D1 round trips on high-traffic endpoints while preserving response behavior and consistency.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by cubic
Cuts sequential D1 round trips on the hottest endpoints, folding per-item N+1 lookups into single queries and running independent reads and fetches in parallel — network round trips cost tens of milliseconds each while the SQL itself runs in well under one. Slightly different behavior: a fractional
Pagenow returns a page instead of an SQL error, and a user @-mentioned twice gets one existence check. All 153 tests pass; the rewritten queries were compared against the old SQL on a real SQLite across eight random seeds with matching results.Round trips per request (before → after)
GetMailList: 2 + 3 per conversation → 1 query using window functions.GetBBSMentionList: 1 + 2 per mention → 1 query, still dropping mentions whose post is gone.GetPosts: count and page in onebatch()(one transaction, one snapshot); the last reply comes fromidx_bbs_reply_post_timeinstead of a window over every reply.NewPost/NewReply: captcha check overlaps the read-only lookups and its error is still reported first when a lookup beside it fails; @-mention checks and writes run in parallel at most four at a time, with recipients de-duplicated first so two writes for the same person can't both insert.DeletePost: one DELETE for all replies.Insertgains anOrReplaceflag) instead of count then insert; a session xmoj already rejected is checked once, but a failed fetch still gets one retry.Migration
migrations/0007_add_existing_indexes.sqlrecords three production indexes created by hand, using the same names withIF NOT EXISTSso it's a no-op in production and brings local and fresh databases in line.Written for commit 43cd19f. Summary will update on new commits.