feat(amber): reject invalid dashboard pagination - #8146
Conversation
Automated Reviewer SuggestionsBased on the
|
Backport auto-label reportThis
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 512 | 0.313 | 19,114/24,588/24,588 us | 🔴 -9.7% / 🔴 +54.6% |
| ⚪ | bs=100 sw=10 sl=64 | 1,212 | 0.74 | 82,808/88,918/88,918 us | ⚪ within ±5% / 🟢 -26.4% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,404 | 0.857 | 709,738/788,183/788,183 us | ⚪ within ±5% / 🟢 +41.4% |
Baseline details
Latest main b28faee from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 512 tuples/sec | 567 tuples/sec | 750.65 tuples/sec | -9.7% | -31.8% |
| bs=10 sw=10 sl=64 | MB/s | 0.313 MB/s | 0.346 MB/s | 0.458 MB/s | -9.5% | -31.7% |
| bs=10 sw=10 sl=64 | p50 | 19,114 us | 17,679 us | 13,049 us | +8.1% | +46.5% |
| bs=10 sw=10 sl=64 | p95 | 24,588 us | 24,152 us | 15,902 us | +1.8% | +54.6% |
| bs=10 sw=10 sl=64 | p99 | 24,588 us | 24,152 us | 19,184 us | +1.8% | +28.2% |
| bs=100 sw=10 sl=64 | throughput | 1,212 tuples/sec | 1,252 tuples/sec | 960.3 tuples/sec | -3.2% | +26.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.74 MB/s | 0.764 MB/s | 0.586 MB/s | -3.1% | +26.3% |
| bs=100 sw=10 sl=64 | p50 | 82,808 us | 80,565 us | 104,199 us | +2.8% | -20.5% |
| bs=100 sw=10 sl=64 | p95 | 88,918 us | 91,531 us | 110,645 us | -2.9% | -19.6% |
| bs=100 sw=10 sl=64 | p99 | 88,918 us | 91,531 us | 120,888 us | -2.9% | -26.4% |
| bs=1000 sw=10 sl=64 | throughput | 1,404 tuples/sec | 1,395 tuples/sec | 993.27 tuples/sec | +0.6% | +41.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.857 MB/s | 0.851 MB/s | 0.606 MB/s | +0.7% | +41.4% |
| bs=1000 sw=10 sl=64 | p50 | 709,738 us | 711,606 us | 1,012,191 us | -0.3% | -29.9% |
| bs=1000 sw=10 sl=64 | p95 | 788,183 us | 821,195 us | 1,054,057 us | -4.0% | -25.2% |
| bs=1000 sw=10 sl=64 | p99 | 788,183 us | 821,195 us | 1,081,313 us | -4.0% | -27.1% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,390.55,200,128000,512,0.313,19114.21,24587.67,24587.67
1,100,10,64,20,1649.78,2000,1280000,1212,0.740,82807.75,88918.48,88918.48
2,1000,10,64,20,14245.71,20000,12800000,1404,0.857,709737.81,788183.44,788183.44
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8146 +/- ##
============================================
- Coverage 94.07% 94.07% -0.01%
+ Complexity 4811 4810 -1
============================================
Files 1193 1193
Lines 48573 48581 +8
Branches 5854 5855 +1
============================================
+ Hits 45696 45703 +7
Misses 1428 1428
- Partials 1449 1450 +1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟡 0 must-fix · 2 advisory · 1 polish — the guard is clean and covers every reader of offset/count; nothing here blocks it.
Correctness (1)
DashboardResource.scala:95—count = Int.MaxValueoverflows in the+ 1one line below the guard, so the arithmetic path may still 500 where this establishes a 400 (advisory, question, see inline)
Simplifications (1)
DashboardResourceSpec.scala:135— nothing pins the "zero remains valid" boundary the description promises;< 0→<= 0would keep CI green (advisory, see inline)
Polish: 1 quick touch-up (see inline comment).
Verification trace
Checked that the guard actually dominates the values it protects rather than assuming placement: all four readers of offset/count sit below it, it covers both HTTP entry points plus the internal ProjectResource caller, and it keeps zero valid. The exception type matches the 11-site amber BadRequestException pattern rather than introducing a new one.
On the overflow: I verified the wrap itself — Scala Int wraps, and the + 1 is at :113 — but not the end-to-end response, which needs a build and a live database. That is why it is posted as a question and marked advisory rather than must-fix, and it is pre-existing on main: this diff does not touch that line.
|
Feedback is addressed and CI is green. Ready for another review. |
carloea2
changed the title
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 3 resolved · 0 open · 6 new (6 new = 6 newly introduced · 0 late catches)
All three round-2 findings are genuinely fixed — I checked each against the tree rather than the replies. The only blocker is the title.
Conventions (2)
- Retitle →
fix(amber): reject invalid dashboard pagination—featcontradicts thefixlabel, both commit subjects, and #8145's HTTP 500 reproduction; "negative" also no longer covers theInt.MaxValuerejection (must-fix) - Description: the body and the testing list still promise only that negatives return 400, but
?count=2147483647now does too (advisory)
Design & architecture (1)
DashboardResource.scala:94— the max-countbound lives in three unlinked places and is documented in none of them (advisory, see inline)
Simplifications (1)
DashboardResourceSpec.scala:145— the accepted end of the new range has no case, unlike the zero end (advisory, see inline)
Polish: 2 quick touch-ups (see inline comments).
Verification trace
Two claims were load-bearing here. First, that count == Int.MaxValue closes the overflow completely: params.count feeds exactly one arithmetic expression in amber/src/main, the params.count + 1 at :115, and for a Scala Int that wraps on exactly one input — so the new clause covers it, and params.offset needs no upper bound because it feeds no arithmetic at all.
Second, that the new zero-boundary case actually discriminates rather than passing by luck. I extracted javax.ws.rs-api and parsed the class files instead of trusting recall: BadRequestException → ClientErrorException → WebApplicationException → RuntimeException. It is not an IllegalArgumentException, so tightening < 0 to <= 0 really would fail that test.
I also traced every consumer of the newly rejected value: both HTTP entry points through search.service.ts, and ProjectResource.scala:173. None sends a large-count sentinel, so the new upper bound breaks no caller.
fe0e232 to
2ccf517
Compare
carloea2
changed the title
|
I kept this as feat after checking the history against the definition you shared. Invalid pagination has been accepted since this endpoint was introduced, so this adds validation rather than restoring previously working behavior. I also broadened the title to cover both bounds. |
|
@Yicong-Huang, all review comments are addressed and CI is green. Could you take another look? |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 5 resolved · 1 open · 0 new (0 new = 0 newly introduced · 0 late catches)
Every code finding from the last three rounds is closed, and I checked each against the tree rather than the replies. Only the title is left.
Conventions (1)
- Retitle →
fix(amber): reject invalid dashboard pagination—featclaims a new capability, but no client can do anything it could not before; three broken responses became correct 400s (must-fix)
Thanks for engaging the definition rather than just retitling. Squashing the branch retired my commit-subject evidence, so here is what replaces it:
- The head branch is
fix/dashboard-negative-pagination, and AGENTS.md:134 asks for the same shape on branch and commit subject — the two now disagree. - #8083
fix(amber): reject duplicate worker initializationmerged 2026-08-31: a guard that never existed, rejecting previously-accepted invalid input, same module, shipped asfix. backport-auto-label.yml:110gates release backports on/^fix(...)/iagainst the title, sofeatquietly removes this from every release branch.
"Support never existed" reads on user-facing capability, and no endpoint, parameter or option is added here. A 500 on client input and a 200 carrying more=true are what §5 of the review standards calls defects.
Verification trace
Two claims were load-bearing. First, that this round changes no behavior: params.count is an Int (:86), so count > MaxSearchCount and the previous count == Int.MaxValue have identical truth sets — the accept/reject partition is byte-for-byte the one already traced to all three callers and to search.service.ts, so nothing needed re-tracing downstream.
Second, that the extraction closes the overflow rather than renaming it. params.count feeds exactly one arithmetic expression in amber/src/main — params.count + SearchResultLookahead at :121 — and the guard admits at most Int.MaxValue - SearchResultLookahead, so the sum cannot wrap for any lookahead, not just 1. That is the part worth having: the guard now derives from the probe, so widening the probe moves the bound and the message with it. I also checked the val initialization order, since MaxSearchCount reads SearchResultLookahead — it is declared first at :53, and the spec's literal at :71 would render 2147483647 otherwise.
5 participants
What changes were proposed in this PR?
Reject negative dashboard search start and count values, and reject counts above the largest value that can safely include the one-row lookahead. Define the lookahead and maximum count once, and cover both accepted boundaries.
Before: negative start returned 500, negative count returned an inconsistent 200 response, and Int.MaxValue overflowed the query limit.
After: invalid pagination returns 400, while zero and the maximum safe count remain valid.
Any related issues, documentation, discussions?
Closes #8145
How was this PR tested?
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex