[rid/store] Fail ISA search instead of silently truncating the result list

[hdimer]

Fixes the RID identification service area case of #1120.

SearchISAs capped its result list to dssmodels.MaxResultLimit and returned it with err == nil, so a client querying a dense area received a non-exhaustive list with no indication that anything had been dropped. Both backends now return dsserr.BadRequest once more than MaxResultLimit areas match, which the v1 and v2 ISA handlers already map to a 400 with the message, so no handler change was needed.

This follows the approach @barroco recorded from the 2024-09-17 contributors sync call in #1120 (comment): fail with a 400 and ask clients to scope their request more tightly, rather than paginate.

What changed

• pkg/rid/store/memstore/identification_service_area.go — the break (and its // This mimics sqlstore behaviour, but it's not very good. TODO) becomes a BadRequest.
• pkg/rid/store/sqlstore/identification_service_area.go — LIMIT MaxResultLimit + 1, then the same BadRequest when that extra row comes back. Selecting one row past the limit is what lets an over-limit result set be distinguished from one that exactly fills it.
• The boundary is unchanged for legitimate queries: exactly MaxResultLimit matches still return the full list with no error.

Incidentally this removes a real divergence between the two backends. memstore appended before checking len(out) > MaxResultLimit, so it returned 10001 items where sqlstore's LIMIT 10000 returned 10000; I measured both before changing anything. The old TODO claimed memstore mimicked sqlstore, and it didn't quite. Both now agree.

ListExpiredISAs is deliberately left alone: it is the operator-run evict sweep, where processing at most MaxResultLimit entries per run and picking up the rest next time is the desired behaviour, not a lie to a client.

One thing I would like your call on

App.SearchISAs has six call sites and only two are the search endpoint. The other four are subscription create/update (pkg/rid/server/v{1,2}/subscription_handler.go), which call it to populate service_areas in the response — and the subscription is already committed by then. So in an area with more than MaxResultLimit ISAs, PUT/PATCH subscription now returns 400 after the write, and a client that retries the same ID gets 409. Previously that branch was unreachable, since SearchISAs could only fail there on empty cells.

That truncated service_areas list is arguably the same bug as #1120 — a client bootstrapping its notification state gets a silently incomplete picture — so failing seems consistent with the recorded decision. But the write-then-400 ordering is not great. Moving the SearchISAs call above the insert/update at those four sites would fix it cleanly (inserting a subscription cannot change which ISAs match; different tables), and I am happy to do that here or in a follow-up. I did not want to touch four handlers uninvited on a PR this size. The release note covers the behaviour either way.

Testing

• TestStoreSearchISAsResultLimit in the memstore pins both halves of the boundary: exactly MaxResultLimit matches succeed with a full list, one more errors with BadRequest. I ran it against the unpatched store first and confirmed it fails on the over-limit assertion for the right reason. It also fails if > is loosened to >=, if the truncated list is returned alongside the error, or if the error is raised without the BadRequest code (which would surface as a 500 rather than a 400).
• go test ./pkg/... ./cmds/... green, gofmt -s -l . clean, golangci-lint run (v2.12.2, per the Makefile) reports 0 issues.
• I could not run make test-go-units-crdb locally, so the sqlstore half is not covered by a test I executed — the existing TestStoreSearchISAs in that package will confirm in CI that the query still works with the new limit, but nothing reaches the new branch. I left it that way on purpose rather than adding a crdb test that inserts 10001 rows into the shared CI suite for logic already pinned in memstore. Say the word and I will add one.

Follow-ups (not in this PR)

The identical pattern is still in pkg/rid/store/{memstore,sqlstore}/subscriptions.go and pkg/scd/store/{memstore,sqlstore}/{subscriptions,operational_intents,constraints}.go — memstore/subscriptions.go even carries the same TODO comment. Worth noting for whoever picks those up: the SCD sites break on >=, so they need the same limit + 1 treatment rather than a copy of the predicate from here. Happy to take them one at a time if this shape is what you want.

Disclosure: this change was prepared with AI assistance (Claude Code). The repro and tests described above were run before it was opened.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: hdimer / name: Haim Dimer (38cdff4)

[mickmis]

Hi @hdimer,
Thanks for your contribution!

In this case I believe it is worth going a step further and pushing this logic down in fetchISAs so that we can deduplicate it. What I mean by that would be e.g.:

• fetchISAs: add a limitRows bool parameter
• implement logic within fetchISAs for identifying that the maximum number of rows have been reached (appending the LIMIT to the SQL query), and return an error there if so
• callsites: remove logic for identifying maximum number of rows reached, use new flag where needed

We will certainly want to do the same for the others similar cases (fetchConstraints, process, fetchOperationalIntents, etc.), but that is outside the scope of this PR. A mention of that in #1120 would be desirable though.

In addition I have two open points:

• Error code to be returned: I think that returning a 400 is technically incorrect: the request sent by the client is totally valid. A more appropriate code would IMO be 422. But we do not have the infra to return that at the moment. We probably don't want to build that now, especially since we are slowly moving away from this store. Would a 500 be more accurate to be returned here? Any opinion @BenjaminPelletier ?
• About having this behavior in the memstore: maybe we want to actually remove that completely from the memstore. The original reason why we has that was because of OOM kills (c.f. #1120). But with the memstore everything is in memory anyway... Maybe we should not touch that in this PR and remove everything in another one? Opinion @the-glu ?

[hdimer]

Done, pushed.

fetchISAs takes a limitRows bool now: when it is set the helper appends LIMIT MaxResultLimit + 1 to the query itself and raises the error if that extra row comes back. SearchISAs is a single call again and its query lost the LIMIT $4; fetchISA, so Get/Insert/Update/Delete, passes false.

ListExpiredISAs also passes false and keeps its own LIMIT. That one is the operator-run evict sweep, where handling MaxResultLimit entries per run and picking the rest up on the next one is what you want, not an error.

Two things worth flagging on the shape:

• The LIMIT is glued to the end of the query, so limitRows is only valid for a plain SELECT; the RETURNING statements that reach the helper through fetchISA would not take it. Nothing can trigger that today, but the point of this is future reuse, so it is in the doc comment.
• If you would rather have one limiting mechanism in the file instead of two, limit int plus a fail-on-overflow flag would let ListExpiredISAs drop its hand-written LIMIT as well, and would carry over to process more directly. I stuck to the bool you specified. Happy to switch.

I also added a datastore-free test for the query construction. The existing sqlstore tests all skip without a live datastore, so nothing was covering the part this commit actually changes; a fake Queryable that records the query and fails the call pins that limitRows appends LIMIT 10001 and that ListExpiredISAs still binds its own. It fails on the pre-refactor code, for the right reason. It is the first test in the package that does not want a datastore, so say if you would rather not have that idiom here and I will drop it. The overflow arithmetic itself is still only covered on the memstore side; that needs a real datastore and 10001 rows, and I left it out on purpose.

The other sites are listed in #1120 (#1120 (comment)), with one gotcha for whoever picks them up: the SCD loops stop on >= and the SQL side selects LIMIT MaxResultLimit, so neither can tell a page that is exactly full from one that overflows. They need the same + 1 rather than a copy of the predicate from here.

On your two open points I have changed nothing and I am happy with either answer:

• Status code. Agreed 400 is a stretch for a well-formed request. Worth knowing that 500 is the smaller change, not the bigger one: the search handlers only special-case dsserr.BadRequest and everything else already falls through to Response500, so it is dropping WithCode from two calls.
• memstore. Equally happy to pull that half out and leave the memstore alone until @the-glu's removal lands separately. That would make this PR sqlstore-only and the release note would need a trim.

Still open from my side, the last section of the description: SearchISAs has six call sites and four of them are subscription create/update, which call it to fill service_areas after the write is committed. So in a dense area those now return an error with the subscription already stored, and a retry gets a 409. Moving the search above the insert/update at those four sites fixes it and cannot change what matches, since a subscription write does not touch the ISA table. I left it out to keep this PR small, but it is the one behavioural wart I would want closed before it merges. Say if you want it here.

[the-glu]

About having this behavior in the memstore: maybe we want to actually remove that completely from the memstore. > The original reason why we has that was because of OOM kills (c.f. #1120). But > with the memstore everything is in memory anyway... Maybe we should not touch that in this PR and remove everything in another one? Opinion @the-glu ?

Yes, the goal was to first make the memstore 'equivalent' and then remove it, when we have the test part done in memstore. I wouldn't touch it for now.

Also for those PR: This is a big behavior change, I'm not sure it's worth it doing it in the 'probably soon old' sql store, since we're going to remove the limits for the raft/memstore. What do you think?