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

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.

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

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)
+}

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,11 +220,12 @@ 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)
+	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(`
@@ -224,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(`
@@ -237,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) {

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")
+}