[lecodev-26]

Summary

• reject non-integer and negative limit values in GET /api/v1/tiering/files
• make the slice guard structurally safe with a positive-limit check at the use site
• keep limit=0 as an explicit empty result
• cover invalid limits and 200 happy paths, including a limit larger than the result set
• remove the proposed 1000 cap so this remains a bug fix without an API compatibility change

Validation

• Rebased onto current main.
• gofmt and git diff --check pass locally.
• The focused internal/api test was not run locally because Termux cannot reliably finish the native dependency build; full validation is left to GitHub CI.

Fixes #1135

[jallegri]

Review note: On head 9ee0508, invalid negative, non-integer, and >1000 limit values return 400 before the tiering listing call, with focused regression cases. GitHub CI passed Build & Test and Enterprise chart. I did not run a local storage-backed flow; the change is isolated to query validation. I found no correctness blocker in this diff.

[xe-nvdk]

Thanks for this — the bug is real and I verified it rather than taking it from the description. I have filed #1135 for it, since it had no issue; please add Fixes #1135 to the PR body.

The premise checks out. QueryInt returns the parsed negative value, len(files) > limit can never be false for a negative limit (0 > -1 holds even on an empty slice), and files[:-1] panics. Worth recording the actual severity: Fiber's recover middleware is installed (internal/api/server.go:177-179), so it is a 500 plus a logged stack trace rather than a crash, and the route sits behind RequireAdmin + requireTieringLicense. An admin typo producing a 500.

The fix is also correctly scoped, which I checked because a pattern like this usually repeats: continuous_query.go:1180 and retention.go:796 also read a limit query parameter, but pass it to SQL LIMIT ?, where SQLite treats a negative as unlimited. So no panic there, and this really is the only slice-based one.

I ran the tests you could not. go test -tags=duckdb_arrow ./internal/api/ passes on your branch, after rebasing onto current main (you were 46 commits behind). So nothing here is broken. Four things to address:

1. The test fails pre-fix for the wrong reason. I reverted tiering.go and ran it:

panic: runtime error: invalid memory address or nil pointer dereference

Not slice bounds out of range [:-1]. A zero-value &TieringHandler{} has a nil manager, so the handler panics at h.manager.GetMetadata() before reaching files[:limit]. The test proves the 400 is returned; it does not demonstrate the panic it exists to prevent, and it would keep passing if the validation were ever moved below the manager call — which would silently reintroduce the bug.

2. Please also clamp at the use site, not only at the boundary:

if limit > 0 && len(files) > limit {

That makes the panic structurally impossible wherever the validation lives, and unlike the boundary check it is testable without a manager — which resolves (1) at the same time.

3. The >1000 cap is an API behaviour change and needs a release-notes line. ?limit=5000 returned results before this PR and now returns 400. I found no in-repo caller of the endpoint, but please check arcli and launchpad before assuming nothing passes a larger value. Arc ships docs and release notes with every code PR — add an entry to RELEASE_NOTES_2027.01.1.md describing the new 400s. If the cap is not actually needed, dropping it makes the PR purely a bug fix with no compatibility question.

4. No happy-path case. All three sub-tests assert 400, so a regression that rejected every request would pass. Add a valid limit returning 200, and decide what limit=0 should do — it currently yields an empty list, which an operator could reasonably read as "no files are tiered".

On process, for the next one: open an issue first, or reference an existing one, and let the PR link to it. It is not bureaucracy — #1135 took longer to write than your patch did, because establishing severity (recovered 500 vs crash, admin-only vs public) and scope (which other endpoints share the pattern) is most of the work in deciding whether a fix is worth shipping and what shape it should take. Doing that first also means a reviewer is checking your code rather than re-deriving the problem.

Also noting, since it is the second PR of yours I have looked at today: you flagged on both that you could not run the tests locally. That is the right thing to disclose. If your environment cannot build the native dependencies, say so in the PR and I will run them — what I would rather avoid is a PR whose tests have never executed anywhere but CI, because CI only tells us the new test passes, not that it fails without the fix.

Happy to re-review as soon as you push.

[xe-nvdk]

Good rework — this addressed almost everything, and you picked the better of the two options I offered.

What is now right, verified on 9e29ce9e:

• The 1000 cap is gone, so the PR is purely a bug fix with no compatibility question and no release note needed. That was the cleaner choice.
• The clamp is in (limit > 0 && len(files) > limit), so the panic is structurally impossible regardless of where validation sits.
• limit=0 is now a deliberate, documented decision rather than an accident, with a test.
• The getFiles seam is a good way to make the handler testable without a manager, and I checked it introduces no production risk: NewTieringHandler is the only construction site in non-test code, so a nil getFiles is reachable only from tests.
• Fixes #1135 linked. Tests pass, and the branch is already on top of current main.

One thing left: the test still does not cover the panic

The two halves never meet. I verified this rather than reading it:

• TestTieringGetFilesLimitSemantics is the only test with a working getFiles stub, so it is the only one that reaches files[:limit] — but it tests 0, 2 and larger-than-result. No negative limit. Run alone against the pre-fix logic, it passes.
• TestTieringGetFilesRejectsInvalidLimit does test -1, but it uses &TieringHandler{} with a nil getFiles, so it panics on the nil before the slice is ever reached.

So the bug #1135 is about is still not covered, for the same reason as the first round — just moved. A regression that reintroduced the unguarded slice would keep both tests green.

It is one row. Adding a negative case to the stub-backed table, so the request actually travels the real path:

{name: "negative is refused before the slice", limit: "-1", wantCount: -1},

with wantCount < 0 asserting 400 and returning early. I tried it: it passes on your branch, and against the pre-fix logic it produces exactly the failure the issue describes —

panic: runtime error: slice bounds out of range [:-1]

— rather than the nil-pointer panic the other test produces. That is the difference between a test that proves the fix and a test that happens to pass.

Push that and I will approve. Nothing else is outstanding.

[xe-nvdk]

I pushed the missing test row to your branch myself rather than send you another round for one line — 341bc2ca. Please git pull before your next commit, since your branch now has a commit you do not have locally.

What it adds: a negative-limit case to TestTieringGetFilesLimitSemantics, the stub-backed table, so the request travels the real path and reaches files[:limit].

The reason it matters, and why the existing -1 case did not cover it: TestTieringGetFilesRejectsInvalidLimit builds a handler with no getFiles, so against pre-fix code it panics on the nil before the slice. Through the stub-backed app it instead fails with

panic: runtime error: slice bounds out of range [:-1]

which is the actual failure #1135 describes. I verified both directions: green on your branch, and that panic when I restored the unguarded logic.

Everything else from the review is addressed, and dropping the 1000 cap was the right call — it makes this purely a bug fix with no compatibility question. Approving now; once CI is green this is good to merge.

[xe-nvdk]

Approved. The fix is correct, the clamp makes the panic structurally impossible regardless of where validation sits, and the coverage gap is closed by 341bc2c (pushed to your branch — please pull). Thanks for the quick turnaround, and for dropping the cap rather than defending it.