Adopt the call-budget guard from #152, and fix the under-chunked read it caught - #166
Merged
Merged
Conversation
… it caught Another account's PR #152 fixed the same Customer 360 N+1 that #160 fixed here, so its code is superseded. Its TEST is not: porting it found a real defect in the merged fix. chunk 50 (what #160 shipped) 300 customers: 51 calls 500: 82 calls FAILS chunk 80 (what #152 used) 300 customers: 34 calls 500: 58 calls passes budget asserted by the test 300 customers: <= 50 500: <= 70 #160 removed the 8-queries-per-customer loop but sized the chunk a third too small, so main was making 82 D1 calls where 58 would do. ID_CHUNK is now 80, still inside D1's ~100 bound-parameter ceiling. Verified by reverting to 50 and watching the test fail with "got 51 D1 calls". The bind-parameter cap this repo already enforced would never have caught either defect. Eight narrow queries per customer are each perfectly legal; only a count of calls catches a fan-out. The two guards catch different things: bind cap -> ONE statement that is too wide call budget -> MANY statements that are individually fine That test also proves something #160 never did: it pins the preserved semantics field by field - default-address-first ordering, newest-first booking timeline, the per-customer LIMIT 50 on coupons, the food-order merge, consent defaults and duplicate detection. The refactor claimed byte-identical output and now has the evidence. tests/helpers/d1-harness.mjs collects both guards so every suite models the same database instead of each writing a more forgiving shim: - D1_MAX_BOUND_PARAMS, previously stranded in one suite - a call counter where batch() costs ONE subrequest as real D1 charges it - otherwise a budget would punish batching, the exact thing we want code to do - assertWithinBudget asserts a FLOOR as well as a ceiling, so a budget cannot pass because the work silently did nothing tests/unit-economics.test.mjs now imports the shared cap rather than defining it. Credit where due: the framing is theirs - assert the property that would actually break, not the shape of the code. Tests 1265 -> 1269. Lint unchanged at 23 errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Hon8zmG1pwVY6zwxtT5th
PawSpaceIND
pushed a commit
that referenced
this pull request
Aug 13, 2026
main fixed two instances of this class while this branch was open - the Customer 360 fan-out (#160, corrected to a chunk of 80 by #166) and the per-booking reads in unit economics (#165, at a chunk of 50). Both declared their own constant in their own module, which is how the same bug came back at a different size in a different place, and neither touched the read that is actually wrong on staging: /team/analytics still counts 331 bookings of GMV next to Rs 0 collected. Both now go through lib/d1-chunked-in, so there is one helper and one size (80, the number #166 measured: a chunk of 50 cost 82 D1 calls for 500 customers where 58 would do). A test fails if any module declares its own chunk constant again. tests/crm-stack-hardening.test.mjs loads lib/customer-360.ts directly without the extensionless-import fallback the other real-execution suites install, so it broke the moment that module imported a sibling. It installs the same hook now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CE4fKzrWRZetcYKh8CaU3b
PawSpaceIND
added a commit
that referenced
this pull request
Aug 13, 2026
…158) /team/analytics reported 331 bookings and GMV of Rs 3,24,472 next to "Collected Rs 0" on every service line, because the payments read for those bookings asked D1 for 331 bound parameters against a cap near 100, and the failure was swallowed into a confident zero. Nine IN-list reads over unbounded sets now go through one shared helper, lib/d1-chunked-in.ts, at the chunk size #166 measured. Also, from review: the chunking is not free. A table-existence guard inside a chunked read is charged once per chunk, which cost 1,012 D1 subrequests over 5,000 bookings against a Worker limit near 1,000 - the fix would have re-broken the same screen one order of magnitude further up. That guard is now memoised per request. An "ORDER BY ... LIMIT 200" inside chunking applies per chunk, so the payroll and incentive approval trails showed the wrong 200 events in block order; both are reapplied to the whole answer. tests/chunked-in-semantics.test.mjs pins all three properties.
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
2 participants
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.
Another account's #152 fixed the same Customer 360 N+1 that #160 fixed here, so its code is superseded. Its test is not — porting it found a real defect in the merged fix.
#160 removed the 8-queries-per-customer loop but sized the chunk a third too small, so main was making 82 D1 calls where 58 would do.
ID_CHUNKis now 80, still inside D1's ~100 bound-parameter ceiling. Verified by reverting to 50 and watching the test fail withgot 51 D1 calls.Why the guard this repo already had would not have caught it
Eight narrow queries per customer are each perfectly legal. Only a count of calls catches a fan-out.
#152's test also proves something #160 never did: it pins the preserved semantics field by field — default-address-first ordering, newest-first booking timeline, the per-customer
LIMIT 50on coupons, the food-order merge, consent defaults, duplicate detection. The refactor claimed byte-identical output; now there's evidence.Shared harness
tests/helpers/d1-harness.mjscollects both guards, so suites stop each writing a more forgiving shim than the real database:D1_MAX_BOUND_PARAMS, previously stranded in one suitebatch()costs one subrequest, as real D1 charges it — otherwise a budget would punish batching, the exact thing we want code to doassertWithinBudgetasserts a floor as well as a ceiling, so a budget can't pass because the work silently did nothingtests/unit-economics.test.mjsnow imports the shared cap rather than defining its own.Coordination
Verification
npm test— 1269/1269 pass (was 1265)npm run lint— 23 errors / 81 warnings, unchanged from baselinenpx tsc --noEmit— clean🤖 Generated with Claude Code
https://claude.ai/code/session_011Hon8zmG1pwVY6zwxtT5th
Generated by Claude Code