From 38cdff4de670da2dad5599d99124bcc7526de2a9 Mon Sep 17 00:00:00 2001 From: Haim Dimer Date: Mon, 17 Aug 2026 08:31:47 -0700 Subject: [PATCH 1/2] [rid/store] Fail ISA search instead of silently truncating the result list SearchISAs capped its result list to dssmodels.MaxResultLimit and returned it with no error, so a client asking about a dense area got a non-exhaustive answer and no way to know it. Per the decision recorded in #1120, return BadRequest (400) when more than MaxResultLimit areas match, so the client can narrow its query instead. The two backends also disagreed on the boundary: memstore appended before checking and returned MaxResultLimit+1 items, while sqlstore's LIMIT returned MaxResultLimit. Both now return up to MaxResultLimit and error beyond that. The RID and SCD subscription, operational intent and constraint searches truncate the same way; those are left for follow-up PRs. --- NEXT_RELEASE_NOTES.md | 1 + .../memstore/identification_service_area.go | 7 +++-- .../identification_service_area_test.go | 31 +++++++++++++++++++ .../sqlstore/identification_service_area.go | 11 ++++++- 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/NEXT_RELEASE_NOTES.md b/NEXT_RELEASE_NOTES.md index dd3e12514..be7391eee 100644 --- a/NEXT_RELEASE_NOTES.md +++ b/NEXT_RELEASE_NOTES.md @@ -62,6 +62,7 @@ The release notes should contain at least the following sections: ## Important information +* Searching RID identification service areas now fails with a 400 error when more than 10000 areas match, instead of silently returning a truncated list (see issue [#1120](https://github.com/interuss/dss/issues/1120)). Clients that hit this must reduce the size of the requested area or time range. Creating or updating a RID subscription in such an area now fails the same way, because the response lists the identification service areas it covers; note the subscription is still written. * Fixed a bug where the `evict` command ignored entries without a locality. If your DSS instance does not have a locality set, the next `evict` run may be slow while it processes the backlog of old entries. * Fixed a bug where Helm charts and Tanka files didn't actually perform any actions via the `evict` command when run via cron jobs (because no locality was set and no delete flag was specified). If you have a large number of entries, the next run may be slow while it processes the backlog of old entries. * AWS load balancer names are no longer enforced by Helm charts or Tanka files. Existing clusters will retain their current names, while new ones will use names automatically generated by AWS. diff --git a/pkg/rid/store/memstore/identification_service_area.go b/pkg/rid/store/memstore/identification_service_area.go index bf7bb042b..550d0923e 100644 --- a/pkg/rid/store/memstore/identification_service_area.go +++ b/pkg/rid/store/memstore/identification_service_area.go @@ -120,8 +120,11 @@ func (r *repo) SearchISAs(_ context.Context, cells s2.CellUnion, earliest *time. } out = append(out, rec.toModel()) - if len(out) > dssmodels.MaxResultLimit { // This mimics sqlstore behaviour, but it's not very good. - break + // One match beyond the limit is enough to know the response cannot be + // exhaustive, see #1120. + if len(out) > dssmodels.MaxResultLimit { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, + "More than %d identification service areas match; reduce the size of the requested area or time range", dssmodels.MaxResultLimit) } } return out, nil diff --git a/pkg/rid/store/memstore/identification_service_area_test.go b/pkg/rid/store/memstore/identification_service_area_test.go index f6baa8dbf..e9080ce6d 100644 --- a/pkg/rid/store/memstore/identification_service_area_test.go +++ b/pkg/rid/store/memstore/identification_service_area_test.go @@ -7,10 +7,12 @@ import ( "github.com/golang/geo/s2" "github.com/google/uuid" + dsserr "github.com/interuss/dss/pkg/errors" dssmodels "github.com/interuss/dss/pkg/models" ridmodels "github.com/interuss/dss/pkg/rid/models" "github.com/interuss/dss/pkg/rid/repos" "github.com/interuss/dss/pkg/timestamp" + "github.com/interuss/stacktrace" "github.com/stretchr/testify/require" ) @@ -322,3 +324,32 @@ func TestStoreCountISAs(t *testing.T) { require.NoError(t, err) require.Equal(t, int64(0), count) } + +func TestStoreSearchISAsResultLimit(t *testing.T) { + ctx := context.Background() + ctx = timestamp.WithRequestTimestamp(ctx, fakeClock.Now()) + repo := setUpStore(t) + + insertISA := func() { + isa := *serviceArea + isa.ID = dssmodels.ID(uuid.New().String()) + _, err := repo.InsertISA(ctx, &isa) + require.NoError(t, err) + } + + for i := 0; i < dssmodels.MaxResultLimit; i++ { + insertISA() + } + + // Exactly MaxResultLimit matches still fits in an exhaustive response. + serviceAreas, err := repo.SearchISAs(ctx, serviceArea.Cells, &startTime, nil) + require.NoError(t, err) + require.Len(t, serviceAreas, dssmodels.MaxResultLimit) + + insertISA() + + serviceAreas, err = repo.SearchISAs(ctx, serviceArea.Cells, &startTime, nil) + require.Error(t, err) + require.Equal(t, dsserr.BadRequest, stacktrace.GetCode(err)) + require.Nil(t, serviceAreas) +} diff --git a/pkg/rid/store/sqlstore/identification_service_area.go b/pkg/rid/store/sqlstore/identification_service_area.go index c361344eb..61721724c 100644 --- a/pkg/rid/store/sqlstore/identification_service_area.go +++ b/pkg/rid/store/sqlstore/identification_service_area.go @@ -207,7 +207,16 @@ func (r *repo) SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *tim return nil, stacktrace.NewError("Earliest start time is missing") } - return r.fetchISAs(ctx, isasInCellsQuery, earliest, latest, dssql.CellUnionToCellIds(cells), dssmodels.MaxResultLimit) + // Select one row beyond the limit to detect a non-exhaustive result set, see #1120. + isas, err := r.fetchISAs(ctx, isasInCellsQuery, earliest, latest, dssql.CellUnionToCellIds(cells), dssmodels.MaxResultLimit+1) + if err != nil { + return nil, err // No need to Propagate this error as this stack layer does not add useful information + } + if len(isas) > dssmodels.MaxResultLimit { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, + "More than %d identification service areas match; reduce the size of the requested area or time range", dssmodels.MaxResultLimit) + } + return isas, nil } // ListExpiredISAs lists all expired ISAs based on writer. From 4a49e01a155961d1fc25f58761f3fe7bb852a208 Mon Sep 17 00:00:00 2001 From: Haim Dimer Date: Wed, 9 Sep 2026 04:13:20 -0700 Subject: [PATCH 2/2] [rid/store] Move the ISA search row limit down into fetchISAs The cap and the over-limit error now live in fetchISAs behind a limitRows flag, so SearchISAs no longer builds its own LIMIT or checks the row count. ListExpiredISAs opts out and keeps its own limit: truncation is the wanted behaviour for the evict sweep. Adds a datastore-free test pinning the query fetchISAs builds for each flag value, which the existing sqlstore tests cannot cover since they skip without a live datastore. --- .../sqlstore/identification_service_area.go | 37 ++++++++------- .../identification_service_area_test.go | 47 +++++++++++++++++++ 2 files changed, 68 insertions(+), 16 deletions(-) diff --git a/pkg/rid/store/sqlstore/identification_service_area.go b/pkg/rid/store/sqlstore/identification_service_area.go index 61721724c..266940e01 100644 --- a/pkg/rid/store/sqlstore/identification_service_area.go +++ b/pkg/rid/store/sqlstore/identification_service_area.go @@ -21,7 +21,16 @@ const ( updateISAFields = "id, url, cells, starts_at, ends_at, writer, updated_at" ) -func (r *repo) fetchISAs(ctx context.Context, query string, args ...interface{}) ([]*ridmodels.IdentificationServiceArea, error) { +// fetchISAs runs query and returns the ISAs it matches. When limitRows is set, one row beyond +// dssmodels.MaxResultLimit is selected so that a result set overflowing the limit is rejected +// rather than returned silently truncated, see #1120. Only set it for a plain SELECT: the LIMIT +// is appended to the end of the query, which the RETURNING statements reaching this helper +// through fetchISA do not accept. +func (r *repo) fetchISAs(ctx context.Context, limitRows bool, query string, args ...interface{}) ([]*ridmodels.IdentificationServiceArea, error) { + if limitRows { + query = fmt.Sprintf("%s LIMIT %d", query, dssmodels.MaxResultLimit+1) + } + rows, err := r.Query(ctx, query, args...) if err != nil { return nil, stacktrace.Propagate(err, "Error in query: %s", query) @@ -59,11 +68,16 @@ func (r *repo) fetchISAs(ctx context.Context, query string, args ...interface{}) return nil, stacktrace.Propagate(err, "Error in rows query result") } + if limitRows && len(payload) > dssmodels.MaxResultLimit { + return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, + "More than %d identification service areas match; reduce the size of the requested area or time range", dssmodels.MaxResultLimit) + } + return payload, nil } func (r *repo) fetchISA(ctx context.Context, query string, args ...interface{}) (*ridmodels.IdentificationServiceArea, error) { - isas, err := r.fetchISAs(ctx, query, args...) + isas, err := r.fetchISAs(ctx, false, query, args...) if err != nil { return nil, err // No need to Propagate this error as this stack layer does not add useful information } @@ -195,8 +209,7 @@ func (r *repo) SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *tim AND COALESCE(starts_at <= $2, true) AND - cells && $3 - LIMIT $4`, isaFields) + cells && $3`, isaFields) ) if len(cells) == 0 { @@ -207,20 +220,12 @@ func (r *repo) SearchISAs(ctx context.Context, cells s2.CellUnion, earliest *tim return nil, stacktrace.NewError("Earliest start time is missing") } - // Select one row beyond the limit to detect a non-exhaustive result set, see #1120. - isas, err := r.fetchISAs(ctx, isasInCellsQuery, earliest, latest, dssql.CellUnionToCellIds(cells), dssmodels.MaxResultLimit+1) - if err != nil { - return nil, err // No need to Propagate this error as this stack layer does not add useful information - } - if len(isas) > dssmodels.MaxResultLimit { - return nil, stacktrace.NewErrorWithCode(dsserr.BadRequest, - "More than %d identification service areas match; reduce the size of the requested area or time range", dssmodels.MaxResultLimit) - } - return isas, nil + return r.fetchISAs(ctx, true, isasInCellsQuery, earliest, latest, dssql.CellUnionToCellIds(cells)) } // ListExpiredISAs lists all expired ISAs based on writer. // The function queries both empty writer and null writer when passing empty string as a writer. +// Truncation is wanted for this operator-run evict sweep: the remainder is picked up next run. func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold time.Time) ([]*ridmodels.IdentificationServiceArea, error) { if len(writer) == 0 { isasInCellsQuery := fmt.Sprintf(` @@ -233,7 +238,7 @@ func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold tim AND (writer = '' OR writer IS NULL) LIMIT $2`, isaFields) - return r.fetchISAs(ctx, isasInCellsQuery, threshold, dssmodels.MaxResultLimit) + return r.fetchISAs(ctx, false, isasInCellsQuery, threshold, dssmodels.MaxResultLimit) } isasInCellsQuery := fmt.Sprintf(` @@ -246,7 +251,7 @@ func (r *repo) ListExpiredISAs(ctx context.Context, writer string, threshold tim AND writer = $2 LIMIT $3`, isaFields) - return r.fetchISAs(ctx, isasInCellsQuery, threshold, writer, dssmodels.MaxResultLimit) + return r.fetchISAs(ctx, false, isasInCellsQuery, threshold, writer, dssmodels.MaxResultLimit) } func (r *repo) CountISAs(ctx context.Context) (int64, error) { diff --git a/pkg/rid/store/sqlstore/identification_service_area_test.go b/pkg/rid/store/sqlstore/identification_service_area_test.go index 715a30155..7e485f94d 100644 --- a/pkg/rid/store/sqlstore/identification_service_area_test.go +++ b/pkg/rid/store/sqlstore/identification_service_area_test.go @@ -2,6 +2,8 @@ package sqlstore import ( "context" + "errors" + "fmt" "testing" "time" @@ -10,6 +12,9 @@ import ( dssmodels "github.com/interuss/dss/pkg/models" ridmodels "github.com/interuss/dss/pkg/rid/models" "github.com/interuss/dss/pkg/rid/repos" + "github.com/interuss/stacktrace" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jonboulle/clockwork" "github.com/stretchr/testify/require" ) @@ -357,3 +362,45 @@ func TestStoreCountISAs(t *testing.T) { require.NoError(t, err) require.Equal(t, int64(0), count) } + +// recordingQueryable captures the query it is handed and then fails the call, which is enough to +// assert on query construction without a datastore: fetchISAs returns as soon as Query does. +type recordingQueryable struct { + query string +} + +var errRecorded = errors.New("query recorded") + +func (q *recordingQueryable) Query(_ context.Context, query string, _ ...interface{}) (pgx.Rows, error) { + q.query = query + return nil, errRecorded +} + +func (q *recordingQueryable) QueryRow(_ context.Context, _ string, _ ...interface{}) pgx.Row { + panic("not needed by these tests") +} + +func (q *recordingQueryable) Exec(_ context.Context, _ string, _ ...interface{}) (pgconn.CommandTag, error) { + panic("not needed by these tests") +} + +func TestFetchISAsAppliesRowLimit(t *testing.T) { + var ( + ctx = context.Background() + q = &recordingQueryable{} + r = &repo{Queryable: q} + pastLimit = fmt.Sprintf("LIMIT %d", dssmodels.MaxResultLimit+1) + ) + + // A search asks for one row past the limit, so that an over-limit result set can be told + // apart from one that exactly fills it. + _, err := r.SearchISAs(ctx, serviceArea.Cells, &startTime, &endTime) + require.Equal(t, errRecorded, stacktrace.RootCause(err)) + require.Contains(t, q.query, pastLimit) + + // The evict sweep keeps its own truncating limit rather than the search one. + _, err = r.ListExpiredISAs(ctx, writer, endTime) + require.Equal(t, errRecorded, stacktrace.RootCause(err)) + require.NotContains(t, q.query, pastLimit) + require.Contains(t, q.query, "LIMIT $3") +}