Order stop search results by combined stop ID

gtfsdb/client.go

@@ -43,6 +43,7 @@ func NewClient(config Config) (*Client, error) {
 	// fallback, so a missing or stale index yields an empty combined ID rather than a
 	// merely degraded one - this must succeed before the client is usable.
 	if err := client.backfillStopAgencyIndex(context.Background()); err != nil {
+		_ = client.Close()
 		return nil, fmt.Errorf("unable to backfill stop agency index: %w", err)
 	}
 
@@ -54,7 +55,7 @@ func NewClient(config Config) (*Client, error) {
 // the feed hash is unchanged, so such a database would otherwise keep stale data for as
 // long as its feed stays the same.
 //
-// The legacy check and the indexed/served counts are read-only and run outside any
+// The legacy check and the indexed/joinable checks are read-only and run outside any
 // transaction; the rebuild and the rebuild alone runs inside one, so a failure partway
 // through never leaves stop_agencies dropped or half-populated.
 func (c *Client) backfillStopAgencyIndex(ctx context.Context) error {
@@ -64,16 +65,25 @@ func (c *Client) backfillStopAgencyIndex(ctx context.Context) error {
 	}
 
 	if !legacy {
-		var indexed, served int
+		var indexed, joinable int
+		// joinable checks stop times that resolve to a route, not merely that stop_times
+		// has rows: a stop_times row with a dangling trip_id or route_id would otherwise
+		// make BuildStopAgencies insert nothing every time, leaving indexed permanently 0
+		// and triggering a full rebuild on every NewClient forever.
 		err := c.DB.QueryRowContext(ctx, `
 			SELECT
-				(SELECT COUNT(*) FROM stop_agencies),
-				(SELECT EXISTS (SELECT 1 FROM stop_times))
-		`).Scan(&indexed, &served)
+				(SELECT EXISTS (SELECT 1 FROM stop_agencies)),
+				(SELECT EXISTS (
+					SELECT 1
+					FROM stop_times
+					JOIN trips ON stop_times.trip_id = trips.id
+					JOIN routes ON trips.route_id = routes.id
+				))
+		`).Scan(&indexed, &joinable)
 		if err != nil {
 			return fmt.Errorf("failed to check stop agency index: %w", err)
 		}
-		if indexed > 0 || served == 0 {
+		if indexed > 0 || joinable == 0 {
 			return nil
 		}
 	}

gtfsdb/driver_cgo.go

@@ -5,3 +5,8 @@ package gtfsdb
 import _ "github.com/mattn/go-sqlite3" // CGo-based SQLite driver
 
 const DriverName = "sqlite3"
+
+// DSN returns the connection string for path with foreign key enforcement enabled.
+// foreign_keys is connection-scoped and not persistent, so setting it via PRAGMA on a
+// single connection leaves the rest of the pool unenforced; it has to ride the DSN.
+func DSN(path string) string { return path + "?_foreign_keys=on" }

gtfsdb/driver_pure.go

@@ -5,3 +5,8 @@ package gtfsdb
 import _ "modernc.org/sqlite" // Pure Go SQLite driver
 
 const DriverName = "sqlite"
+
+// DSN returns the connection string for path with foreign key enforcement enabled.
+// foreign_keys is connection-scoped and not persistent, so setting it via PRAGMA on a
+// single connection leaves the rest of the pool unenforced; it has to ride the DSN.
+func DSN(path string) string { return path + "?_pragma=foreign_keys(1)" }

gtfsdb/foreign_keys_test.go

@@ -0,0 +1,46 @@
+package gtfsdb
+
+import (
+	"context"
+	"database/sql"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"maglev.onebusaway.org/internal/appconf"
+)
+
+// TestForeignKeysEnabledOnEveryPooledConnection guards against foreign_keys being set on
+// only the connection that runs migrations. foreign_keys is connection-scoped and
+// non-persistent, so it must ride the DSN to reach every connection in the pool; holding
+// several connections open at once (rather than reusing one via sequential Conn calls) is
+// what actually exercises that.
+func TestForeignKeysEnabledOnEveryPooledConnection(t *testing.T) {
+	dbPath := filepath.Join(t.TempDir(), "foreign_keys.db")
+	client, err := NewClient(Config{DBPath: dbPath, Env: appconf.Development})
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = client.Close() })
+
+	ctx := context.Background()
+	const connCount = 5
+
+	conns := make([]*sql.Conn, connCount)
+	for i := range conns {
+		conn, err := client.DB.Conn(ctx)
+		require.NoError(t, err)
+		conns[i] = conn
+	}
+	t.Cleanup(func() {
+		for _, conn := range conns {
+			_ = conn.Close()
+		}
+	})
+
+	for i, conn := range conns {
+		var enabled int
+		err := conn.QueryRowContext(ctx, "PRAGMA foreign_keys").Scan(&enabled)
+		require.NoError(t, err)
+		assert.Equal(t, 1, enabled, "connection %d should have foreign key enforcement on", i)
+	}
+}

gtfsdb/fts_queries.go

@@ -4,7 +4,7 @@ package gtfsdb
 // sqlc cannot handle queries that use FTS5-specific syntax (MATCH operator,
 // bm25() function), so these are maintained manually instead of in query.sql.
 //
-// IMPORTANT: If the 'routes', 'stops', 'routes_fts', or 'stops_fts' table schemas change,
+// IMPORTANT: If the 'routes', 'stops', 'stop_agencies', 'routes_fts', or 'stops_fts' table schemas change,
 // the SQL and Go types in this file must be updated manually to match.
 // Running 'make models' will NOT update this file.
 
@@ -80,6 +80,14 @@ func (q *Queries) SearchRoutesByFullText(ctx context.Context, arg SearchRoutesBy
 	return items, nil
 }
 
+// searchStopsByName orders results by the combined {agency_id}_{stop_id} ID callers see
+// rather than by the raw stop ID. The sort key has to be built here, before LIMIT, or the
+// wrong set of stops would be truncated away.
+//
+// stop_agencies holds one row per (stop, agency) pair, so a stop served by more than one
+// agency is resolved with MIN() to the single agency whose prefix its combined ID carries -
+// otherwise the join would fan out into one result row per agency. A stop no route serves
+// has no row at all and therefore no combined ID; those sort last, by raw stop ID.
 const searchStopsByName = `
 SELECT
     s.id,
@@ -90,12 +98,17 @@ SELECT
     s.location_type,
     s.wheelchair_boarding,
     s.direction,
-    s.parent_station
+    s.parent_station,
+    (
+        SELECT MIN(sa.agency_id)
+        FROM stop_agencies sa
+        WHERE sa.stop_id = s.id
+    ) AS agency_id
 FROM stops s
 JOIN stops_fts fts
   ON s.rowid = fts.rowid
 WHERE fts.stop_name MATCH ?
-ORDER BY s.id
+ORDER BY agency_id IS NULL, agency_id || '_' || s.id, s.id
 LIMIT ?
 `
 
@@ -114,6 +127,9 @@ type SearchStopsByNameRow struct {
 	WheelchairBoarding sql.NullInt64
 	Direction          sql.NullString
 	ParentStation      sql.NullString
+	// AgencyID is the lowest agency ID among the routes serving the stop, and is null
+	// when no route serves it.
+	AgencyID sql.NullString
 }
 
 func (q *Queries) SearchStopsByName(ctx context.Context, arg SearchStopsByNameParams) ([]SearchStopsByNameRow, error) {
@@ -136,6 +152,7 @@ func (q *Queries) SearchStopsByName(ctx context.Context, arg SearchStopsByNamePa
 			&i.WheelchairBoarding,
 			&i.Direction,
 			&i.ParentStation,
+			&i.AgencyID,
 		); err != nil {
 			return nil, err
 		}

gtfsdb/fts_queries_test.go

@@ -254,7 +254,8 @@ func TestSearchStopsByName(t *testing.T) {
 		require.NoError(t, err)
 		require.Len(t, results, 3)
 
-		// Assert that the result set is ordered ascending by s.id (hub1 < s1 < s3).
+		// No route serves these stops, so none resolves an agency to build a combined ID
+		// from, and they fall through to the raw stop ID tiebreak (hub1 < s1 < s3).
 		assert.Equal(t, "Main Street Hub", results[0].Name.String)
 		assert.Equal(t, "Main Street Station", results[1].Name.String)
 		assert.Equal(t, "Main Street Mall", results[2].Name.String)
@@ -327,6 +328,92 @@ func TestSearchStopsByName(t *testing.T) {
 	})
 }
 
+// stopServedByAgency creates a stop plus the route, trip and stop time that make the given
+// agency serve it, so the search query has an agency to resolve for that stop.
+func stopServedByAgency(t *testing.T, client *Client, agencyID, stopID, stopName string) {
+	t.Helper()
+	ctx := context.Background()
+
+	_, err := client.Queries.CreateAgency(ctx, CreateAgencyParams{
+		ID:       agencyID,
+		Name:     "Agency " + agencyID,
+		Url:      "http://" + agencyID + ".example.test",
+		Timezone: "America/New_York",
+	})
+	require.NoError(t, err)
+
+	_, err = client.Queries.CreateStop(ctx, CreateStopParams{
+		ID: stopID, Name: nulls.String(stopName), Lat: 40.0, Lon: -74.0,
+	})
+	require.NoError(t, err)
+
+	routeID := "route_" + stopID
+	_, err = client.Queries.CreateRoute(ctx, CreateRouteParams{
+		ID: routeID, AgencyID: agencyID, Type: 3,
+	})
+	require.NoError(t, err)
+
+	tripID := "trip_" + stopID
+	_, err = client.Queries.CreateTrip(ctx, CreateTripParams{
+		ID: tripID, RouteID: routeID, ServiceID: "service_1",
+	})
+	require.NoError(t, err)
+
+	_, err = client.Queries.CreateStopTime(ctx, CreateStopTimeParams{
+		TripID: tripID, StopID: stopID, StopSequence: 1, ArrivalTime: 28800, DepartureTime: 28800,
+	})
+	require.NoError(t, err)
+}
+
+func TestSearchStopsByNameOrdersByCombinedID(t *testing.T) {
+	client := createFTSTestClient(t)
+	defer func() { _ = client.Close() }()
+
+	ctx := context.Background()
+
+	_, err := client.DB.ExecContext(ctx, `
+		INSERT INTO calendar (id, monday, tuesday, wednesday, thursday, friday, saturday, sunday, start_date, end_date)
+		VALUES ('service_1', 1, 1, 1, 1, 1, 1, 1, '20240101', '20251231')
+	`)
+	require.NoError(t, err)
+
+	// Raw stop ID order (aaa, zzz) is the reverse of combined ID order (111_zzz, 999_aaa).
+	stopServedByAgency(t, client, "999", "aaa", "Ordertest Alpha")
+	stopServedByAgency(t, client, "111", "zzz", "Ordertest Zulu")
+
+	// No route serves this stop, so it has no combined ID to sort by.
+	_, err = client.Queries.CreateStop(ctx, CreateStopParams{
+		ID: "bbb", Name: nulls.String("Ordertest Bravo"), Lat: 40.0, Lon: -74.0,
+	})
+	require.NoError(t, err)
+
+	require.NoError(t, buildStopAgencyIndex(ctx, client.Queries))
+
+	results, err := client.Queries.SearchStopsByName(ctx, SearchStopsByNameParams{
+		SearchQuery: "Ordertest",
+		Limit:       10,
+	})
+	require.NoError(t, err)
+	require.Len(t, results, 3)
+
+	assert.Equal(t, []string{"zzz", "aaa", "bbb"}, []string{results[0].ID, results[1].ID, results[2].ID},
+		"stops must be ordered by combined ID, with agency-less stops last")
+
+	assert.Equal(t, "111", results[0].AgencyID.String)
+	assert.Equal(t, "999", results[1].AgencyID.String)
+	assert.False(t, results[2].AgencyID.Valid, "a stop no route serves resolves no agency")
+
+	t.Run("limit keeps the lowest combined IDs", func(t *testing.T) {
+		capped, err := client.Queries.SearchStopsByName(ctx, SearchStopsByNameParams{
+			SearchQuery: "Ordertest",
+			Limit:       1,
+		})
+		require.NoError(t, err)
+		require.Len(t, capped, 1)
+		assert.Equal(t, "zzz", capped[0].ID)
+	})
+}
+
 func TestSearchStopsByNameEmptyDB(t *testing.T) {
 	client := createFTSTestClient(t)
 	defer func() { _ = client.Close() }()

gtfsdb/helpers.go

@@ -137,7 +137,7 @@ func createDB(config Config) (*sql.DB, error) {
 		return nil, fmt.Errorf("test database must use in-memory storage, got path: %s", config.DBPath)
 	}
 
-	db, err := sql.Open(DriverName, config.DBPath)
+	db, err := sql.Open(DriverName, DSN(config.DBPath))
 	if err != nil {
 		return nil, err
 	}
@@ -146,11 +146,13 @@ func createDB(config Config) (*sql.DB, error) {
 	ctx := context.Background()
 	err = configureSQLitePerformance(ctx, db)
 	if err != nil {
+		_ = db.Close()
 		return nil, fmt.Errorf("error configuring SQLite performance: %w", err)
 	}
 
 	err = performDatabaseMigration(ctx, db)
 	if err != nil {
+		_ = db.Close()
 		return nil, fmt.Errorf("error performing database migration: %w", err)
 	}
 

gtfsdb/helpers_test.go

@@ -197,6 +197,13 @@ func TestNewClient_RecordsQueryMetricsWhenOnlyMetricsEnabled(t *testing.T) {
         agency_id TEXT NOT NULL,
         PRIMARY KEY (stop_id, agency_id)
     );
+    CREATE TABLE IF NOT EXISTS routes (
+        id TEXT PRIMARY KEY
+    );
+    CREATE TABLE IF NOT EXISTS trips (
+        id TEXT PRIMARY KEY,
+        route_id TEXT NOT NULL
+    );
     CREATE TABLE IF NOT EXISTS stop_times (
         trip_id TEXT NOT NULL
     );

gtfsdb/schema.sql

@@ -1,3 +1,7 @@
+-- foreign_keys is connection-scoped, so this PRAGMA only enforces on the one connection
+-- that runs migrations. Pool-wide enforcement actually comes from the DSN built by
+-- gtfsdb.DSN (see driver_cgo.go / driver_pure.go); this line is kept because it's free
+-- and documents the intent at the schema level.
 PRAGMA foreign_keys = ON;
 
 -- migrate
@@ -170,7 +174,8 @@ CREATE TABLE
 -- per query, which is too slow to do while serving a request. Rebuilt from scratch on every
 -- import. A caller wanting a single agency per stop takes MIN(agency_id) GROUP BY stop_id.
 -- IF NOT EXISTS cannot reshape a table created under an earlier, single-column-PK version
--- of this schema; Client.rebuildLegacyStopAgenciesTable handles that case at startup.
+-- of this schema; Client.hasLegacyStopAgenciesTable detects that case at startup and
+-- Client.recreateStopAgenciesTable rebuilds it.
 CREATE TABLE
     IF NOT EXISTS stop_agencies (
         stop_id TEXT NOT NULL,

gtfsdb/stop_agency_test.go

@@ -1,7 +1,9 @@
 package gtfsdb
 
 import (
+	"bytes"
 	"context"
+	"log/slog"
 	"os"
 	"path/filepath"
 	"testing"
@@ -270,3 +272,46 @@ func TestBuildStopAgencyIndex_RebuildsFromScratch(t *testing.T) {
 	require.NoError(t, err)
 	assert.Zero(t, stray, "rebuild should drop rows for stops no route serves")
 }
+
+func TestBackfillStopAgencyIndex_SettlesWhenNoStopTimeJoinsARoute(t *testing.T) {
+	client := newTestClientWithRABA(t)
+	ctx := context.Background()
+
+	_, err := client.DB.ExecContext(ctx, "DELETE FROM stop_agencies")
+	require.NoError(t, err)
+
+	// Replace every stop_times row with one whose trip_id resolves to nothing, so the
+	// index has stop_times but none of them join through to a route. FK checks are off
+	// just long enough to write the dangling reference.
+	_, err = client.DB.ExecContext(ctx, "PRAGMA foreign_keys = OFF")
+	require.NoError(t, err)
+	_, err = client.DB.ExecContext(ctx, "DELETE FROM stop_times")
+	require.NoError(t, err)
+	_, err = client.DB.ExecContext(ctx, `
+		INSERT INTO stop_times (trip_id, arrival_time, departure_time, stop_id, stop_sequence)
+		SELECT 'no-such-trip', 0, 0, id, 0 FROM stops LIMIT 1
+	`)
+	require.NoError(t, err)
+	_, err = client.DB.ExecContext(ctx, "PRAGMA foreign_keys = ON")
+	require.NoError(t, err)
+
+	require.NoError(t, client.backfillStopAgencyIndex(ctx),
+		"a stop_times row that can't join to a route should settle, not error")
+
+	var indexed int
+	require.NoError(t, client.DB.QueryRowContext(ctx, "SELECT COUNT(*) FROM stop_agencies").Scan(&indexed))
+	assert.Zero(t, indexed, "nothing joins to a route, so the index should stay empty")
+
+	// A second call must settle without rebuilding: capture the default logger, since
+	// backfillStopAgencyIndex logs "rebuilding stop agency index" only when it actually
+	// runs the rebuild transaction. Before the fix, indexed staying at 0 forever meant
+	// every call rebuilt regardless of whether anything was joinable.
+	previousLogger := slog.Default()
+	var logBuf bytes.Buffer
+	slog.SetDefault(slog.New(slog.NewTextHandler(&logBuf, nil)))
+	err = client.backfillStopAgencyIndex(ctx)
+	slog.SetDefault(previousLogger)
+	require.NoError(t, err)
+	assert.NotContains(t, logBuf.String(), "rebuilding stop agency index",
+		"a settled index with nothing joinable should not trigger another rebuild")
+}