From 133e99bc7e191a4a1e58495d65e2dfc95744f4e7 Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Mon, 19 Oct 2026 23:27:36 -0700 Subject: [PATCH 1/8] Free the parsed feed before precomputing ReloadStatic held the parsed feed in a local variable that it read again after the import, so the whole feed stayed reachable while stop directions were precomputed. For NJ Transit bus that kept several hundred MB alive through a stage that needs about a hundred. storeConfiguredFeed now loads, parses and stores the feed, and the parsed copy is reachable only from there. ReloadStatic precomputes directions after it returns, first calling debug.FreeOSMemory so the feed's memory is collected and returned to the OS instead of waiting for the GC to get around to it. importStaticIntoDB, which did the store and the precompute together, is replaced by precomputeStopDirections. On NJ Transit, the precompute starts at 113 MiB of heap instead of about 630 MiB, and the process's peak RSS falls from 1.0 GiB to about 950 MiB. The peak is now the parse itself (about 555 MiB of heap), which only a streaming parser (#1657, step 3) can lower. --- internal/gtfs/static.go | 73 +++++++++++++++++++----------------- internal/gtfs/static_test.go | 62 ++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 34 deletions(-) diff --git a/internal/gtfs/static.go b/internal/gtfs/static.go index 4371cf031..e836b54d6 100644 --- a/internal/gtfs/static.go +++ b/internal/gtfs/static.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "os" + "runtime/debug" "strings" "time" @@ -82,7 +83,7 @@ func rawGtfsData(ctx context.Context, source string, config Config) ([]byte, err } // openGtfsDB opens (or creates) the single SQLite database used by the manager. -// No import work happens here — use importStaticIntoDB against the returned client. +// No import work happens here — ReloadStatic imports into the returned client. func openGtfsDB(config Config) (*gtfsdb.Client, error) { dbConfig := newGTFSDBConfig(config.GTFSDataPath, config) client, err := gtfsdb.NewClient(dbConfig) @@ -92,28 +93,15 @@ func openGtfsDB(config Config) (*gtfsdb.Client, error) { return client, nil } -// importStaticIntoDB imports the already-parsed GTFS data into the provided client. -// Returns (changed, err): changed is true when the DB was actually updated. When changed, -// it also precomputes stop directions. Trip time bounds are now computed inside the -// import transaction by ImportParsedGTFS itself. -func importStaticIntoDB(ctx context.Context, client *gtfsdb.Client, data *gtfsdb.GtfsData) (bool, error) { - changed, err := client.StoreGtfsData(ctx, data) - if err != nil { - return false, err - } - - if !changed { - return false, nil - } - - logger := slog.Default().With(slog.String("component", "gtfs_db_builder")) +// precomputeStopDirections stores each stop's direction for the feed now in +// the database. A failure is logged, not returned: the API computes missing +// directions on demand. +func precomputeStopDirections(ctx context.Context, client *gtfsdb.Client) { precomputer := NewDirectionPrecomputer(client.Queries, client.DB) if err := precomputer.PrecomputeAllDirections(ctx); err != nil { - // Log error but don't fail the entire import + logger := slog.Default().With(slog.String("component", "gtfs_db_builder")) logging.LogError(logger, "Failed to precompute stop directions - API will fallback to on-demand calculation", err) } - - return true, nil } func newGTFSDBConfig(dbPath string, config Config) gtfsdb.Config { @@ -199,26 +187,20 @@ func (manager *Manager) ReloadStatic(ctx context.Context) (bool, error) { defer manager.staticUpdateMutex.Unlock() logger := slog.Default().With(slog.String("component", "gtfs_updater")) - newData, err := loadGTFSData(ctx, manager.config) - if err != nil { - logging.LogError(logger, "Error loading GTFS data", err, - slog.String("source", manager.config.GtfsURL)) - return false, err - } - - if err := ctx.Err(); err != nil { - return false, err - } - - changed, err := importStaticIntoDB(ctx, manager.GtfsDB, newData) + changed, err := manager.storeConfiguredFeed(ctx, logger) if err != nil { - logging.LogError(logger, "Error importing GTFS data", err) return false, err } - if !changed { + if changed { + // The parsed feed became garbage when storeConfiguredFeed returned. + // Collecting it now, and returning its pages to the OS, keeps it out + // of the precompute's memory and out of the process's footprint. + debug.FreeOSMemory() + precomputeStopDirections(ctx, manager.GtfsDB) + } else { logging.LogOperation(logger, "gtfs_static_data_unchanged", - slog.String("source", newData.Source)) + slog.String("source", manager.config.GtfsURL)) } newRegionBounds := computeRegionBounds(ctx, manager.GtfsDB) @@ -249,6 +231,29 @@ func (manager *Manager) ReloadStatic(ctx context.Context) (bool, error) { return changed, nil } +// storeConfiguredFeed loads the configured feed and stores it in the +// database. The parsed feed is reachable only from here, so it can be freed +// as soon as this returns. It reports whether the database changed. +func (manager *Manager) storeConfiguredFeed(ctx context.Context, logger *slog.Logger) (bool, error) { + newData, err := loadGTFSData(ctx, manager.config) + if err != nil { + logging.LogError(logger, "Error loading GTFS data", err, + slog.String("source", manager.config.GtfsURL)) + return false, err + } + + if err := ctx.Err(); err != nil { + return false, err + } + + changed, err := manager.GtfsDB.StoreGtfsData(ctx, newData) + if err != nil { + logging.LogError(logger, "Error importing GTFS data", err) + return false, err + } + return changed, nil +} + // logFeedExpiry reads the feed_expires_at value persisted by StoreGtfsData // and updates the metrics gauge / emits warning logs about how soon the feed // will expire. The DB write itself happens atomically inside the import diff --git a/internal/gtfs/static_test.go b/internal/gtfs/static_test.go index 2b1507ddf..168626875 100644 --- a/internal/gtfs/static_test.go +++ b/internal/gtfs/static_test.go @@ -1,11 +1,17 @@ package gtfs import ( + "context" + "log/slog" + "path/filepath" + "sync" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "maglev.onebusaway.org/internal/appconf" "maglev.onebusaway.org/internal/metrics" + "maglev.onebusaway.org/internal/models" ) func TestNewGTFSDBConfig_QueryMetricsRecorder(t *testing.T) { @@ -27,3 +33,59 @@ func TestNewGTFSDBConfig_QueryMetricsRecorder(t *testing.T) { assert.Same(t, m, dbConfig.QueryMetricsRecorder) }) } + +// logMessageCounter is a slog.Handler that counts records by message. +type logMessageCounter struct { + mu sync.Mutex + counts map[string]int +} + +func (c *logMessageCounter) Enabled(context.Context, slog.Level) bool { return true } +func (c *logMessageCounter) WithAttrs([]slog.Attr) slog.Handler { return c } +func (c *logMessageCounter) WithGroup(string) slog.Handler { return c } +func (c *logMessageCounter) Handle(_ context.Context, record slog.Record) error { + c.mu.Lock() + defer c.mu.Unlock() + c.counts[record.Message]++ + return nil +} + +func (c *logMessageCounter) count(message string) int { + c.mu.Lock() + defer c.mu.Unlock() + return c.counts[message] +} + +// countLogMessages routes the default logger to a counter for the rest of the +// test. +func countLogMessages(t *testing.T) *logMessageCounter { + counter := &logMessageCounter{counts: map[string]int{}} + previous := slog.Default() + slog.SetDefault(slog.New(counter)) + t.Cleanup(func() { slog.SetDefault(previous) }) + return counter +} + +// TestReloadStaticPrecomputesOnlyWhenChanged checks that stop directions are +// precomputed after an import that changed the data, and not after a reload +// that found the same feed. +func TestReloadStaticPrecomputesOnlyWhenChanged(t *testing.T) { + counter := countLogMessages(t) + manager, err := InitGTFSManager(context.Background(), Config{ + GtfsURL: models.GetFixturePath(t, "raba.zip"), + GTFSDataPath: filepath.Join(t.TempDir(), "gtfs.db"), + Env: appconf.Development, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = manager.Shutdown(context.Background()) }) + require.Equal(t, 1, counter.count("precomputing_stop_directions_started")) + + var withDirection int + require.NoError(t, manager.GtfsDB.DB.QueryRow(`SELECT COUNT(*) FROM stops WHERE direction IS NOT NULL`).Scan(&withDirection)) + assert.Positive(t, withDirection) + + changed, err := manager.ReloadStatic(context.Background()) + require.NoError(t, err) + assert.False(t, changed) + assert.Equal(t, 1, counter.count("precomputing_stop_directions_started")) +} From e4d32df0eae9414028e2cd021ffa31fef43c6421 Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Mon, 19 Oct 2026 23:31:23 -0700 Subject: [PATCH 2/8] Check the feed hash before parsing Every startup and every daily reload read the whole static feed into memory and parsed it, and only then did StoreGtfsData compare its hash with import_metadata. So a restart with an unchanged feed and an existing database reached nearly the same memory peak as a first import. fetchFeedFile now puts the feed in a local file while hashing it: a local feed is read where it is, and a remote one is downloaded next to the database rather than to a temporary directory that may be held in memory. Downloads left behind by a crash are removed first. storeConfiguredFeed asks the new Client.HasImported whether the database already holds that hash from that source, and parses the feed only when it doesn't. HasImported and StoreGtfsData share one isSameFeed rule. The hash is still the SHA-256 of the feed bytes, so existing databases match and don't re-import. parse hashes the bytes it actually reads, so the stored hash always describes the stored data. With gtfstidy on, the hash is marked "+gtfstidy" only when tidying succeeded: turning tidy on or off causes a re-import, and a failed tidy is retried on the next reload instead of being recorded as done. A database imported with tidy on re-imports once. The static download now clones http.DefaultTransport, like the realtime client, so it honors proxy settings from the environment. For NJ Transit bus with an existing database, a restart is healthy in 1 s with a peak RSS of 194 MB. On main it takes 4 s and 825 MB. --- gtfsdb/conditional_import_test.go | 30 +++++ gtfsdb/helpers.go | 57 +++++++-- internal/gtfs/feed_file.go | 185 +++++++++++++++++++++++++++ internal/gtfs/feed_file_test.go | 204 ++++++++++++++++++++++++++++++ internal/gtfs/static.go | 120 ++++-------------- internal/gtfs/static_test.go | 71 +++++++++++ 6 files changed, 563 insertions(+), 104 deletions(-) create mode 100644 internal/gtfs/feed_file.go create mode 100644 internal/gtfs/feed_file_test.go diff --git a/gtfsdb/conditional_import_test.go b/gtfsdb/conditional_import_test.go index 4406a5087..1cc1a6707 100644 --- a/gtfsdb/conditional_import_test.go +++ b/gtfsdb/conditional_import_test.go @@ -325,3 +325,33 @@ func TestClearAllGTFSData(t *testing.T) { require.NoError(t, err, "Import metadata should still exist after clear") assert.NotEmpty(t, metadata.FileHash, "Import metadata should not be cleared") } + +func TestHasImported(t *testing.T) { + client, err := NewClient(Config{DBPath: ":memory:", Env: appconf.Test}) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + + imported, err := client.HasImported(t.Context(), "hash", "source") + require.NoError(t, err) + assert.False(t, imported, "an empty database holds no feed") + + raba, err := os.ReadFile("../testdata/raba.zip") + require.NoError(t, err) + parsed, err := ParseGtfsData(raba, "source") + require.NoError(t, err) + _, err = client.StoreGtfsData(t.Context(), parsed) + require.NoError(t, err) + + for _, tt := range []struct { + hash, source string + want bool + }{ + {parsed.Hash, "source", true}, + {parsed.Hash, "other-source", false}, + {"other-hash", "source", false}, + } { + imported, err := client.HasImported(t.Context(), tt.hash, tt.source) + require.NoError(t, err) + assert.Equal(t, tt.want, imported, "%s from %s", tt.hash, tt.source) + } +} diff --git a/gtfsdb/helpers.go b/gtfsdb/helpers.go index a8d068d5a..95190191e 100644 --- a/gtfsdb/helpers.go +++ b/gtfsdb/helpers.go @@ -7,6 +7,7 @@ import ( "database/sql" _ "embed" "encoding/hex" + "errors" "fmt" "iter" "log/slog" @@ -23,9 +24,9 @@ import ( //go:embed schema.sql var ddl string -// GtfsData bundles parsed static GTFS data with the sha256 hash of the -// bytes it was parsed from. The hash is used to skip reimport when the -// source data is unchanged. +// GtfsData bundles parsed static GTFS data with the hash that identifies the +// feed in import_metadata. The hash is used to skip reimport when the feed is +// unchanged. type GtfsData struct { Static *gtfs.Static Hash string @@ -36,8 +37,12 @@ type GtfsData struct { // The given source is stored on the returned struct for later use by StoreGtfsData. func ParseGtfsData(b []byte, source string) (*GtfsData, error) { hash := sha256.Sum256(b) - hashStr := hex.EncodeToString(hash[:]) + return ParseGtfsDataWithHash(b, hex.EncodeToString(hash[:]), source) +} +// ParseGtfsDataWithHash parses and structurally validates GTFS zip bytes whose +// identifying hash the caller has already computed. +func ParseGtfsDataWithHash(b []byte, hash, source string) (*GtfsData, error) { logger := slog.Default().With(slog.String("component", "gtfs_parser")) logging.LogOperation(logger, "parsing_gtfs_data", slog.Int("bytes", len(b))) staticData, err := gtfs.ParseStatic(b, gtfs.ParseStaticOptions{}) @@ -49,7 +54,7 @@ func ParseGtfsData(b []byte, source string) (*GtfsData, error) { return nil, fmt.Errorf("GTFS validation failed: %w", err) } - return &GtfsData{Static: staticData, Hash: hashStr, Source: source}, nil + return &GtfsData{Static: staticData, Hash: hash, Source: source}, nil } // metricsWrapper wraps *sql.DB for metric reporting purposes @@ -203,6 +208,38 @@ func (c *Client) withTransaction(ctx context.Context, tx *sql.Tx, label string, return nil } +// HasImported reports whether the database already holds the feed with this +// hash from this source, so a caller can skip parsing it. +func (c *Client) HasImported(ctx context.Context, hash, source string) (bool, error) { + metadata, err := c.Queries.GetImportMetadata(ctx) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("error checking import metadata: %w", err) + } + return isSameFeed(metadata, hash, source), nil +} + +// isSameFeed reports whether import_metadata describes the feed with this hash +// from this source. +func isSameFeed(metadata ImportMetadatum, hash, source string) bool { + return metadata.FileHash == hash && metadata.FileSource == source +} + +// RefreshFeedExpiry recomputes feed_expires_at from the stored calendar. It +// runs when a feed is found unchanged, so existing databases pick up +// corrections to the expiry calculation on upgrade without a re-import. +func (c *Client) RefreshFeedExpiry(ctx context.Context) error { + err := c.withTransaction(ctx, nil, "refresh_feed_expiry", func(tx *sql.Tx) error { + return updateFeedExpiresAtFromCalendar(ctx, c.Queries.WithTx(tx)) + }) + if err != nil { + return fmt.Errorf("failed to refresh feed expiry: %w", err) + } + return nil +} + func (c *Client) StoreGtfsData(ctx context.Context, data *GtfsData) (bool, error) { logger := slog.Default().With(slog.String("component", "gtfs_importer")) @@ -223,13 +260,9 @@ func (c *Client) StoreGtfsData(ctx context.Context, data *GtfsData) (bool, error if err == nil { hasExisting = true // We have existing metadata, check if hash matches - if existingMetadata.FileHash == data.Hash && existingMetadata.FileSource == data.Source { - // Refresh derived metadata even when the feed is unchanged, so existing - // databases pick up corrections to the expiry calculation on upgrade. - if err := c.withTransaction(ctx, nil, "refresh_feed_expiry", func(tx *sql.Tx) error { - return updateFeedExpiresAtFromCalendar(ctx, c.Queries.WithTx(tx)) - }); err != nil { - return false, fmt.Errorf("failed to refresh feed expiry: %w", err) + if isSameFeed(existingMetadata, data.Hash, data.Source) { + if err := c.RefreshFeedExpiry(ctx); err != nil { + return false, err } logging.LogOperation(logger, "gtfs_data_unchanged_skipping_import", slog.String("hash", data.Hash[:8])) diff --git a/internal/gtfs/feed_file.go b/internal/gtfs/feed_file.go new file mode 100644 index 000000000..b795fb111 --- /dev/null +++ b/internal/gtfs/feed_file.go @@ -0,0 +1,185 @@ +package gtfs + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "os" + "path/filepath" + "strings" + "time" + + "maglev.onebusaway.org/gtfsdb" + "maglev.onebusaway.org/internal/logging" +) + +// maxStaticFeedBytes is the largest static feed Maglev downloads. +const maxStaticFeedBytes = 200 * 1024 * 1024 + +// feedFile is a static GTFS zip on local disk. hash identifies it in +// import_metadata when it is imported as configured. +type feedFile struct { + path string + hash string + cleanup func() +} + +// downloadPattern names feed downloads, so ones left behind by a crash can be +// found and removed. +const downloadPattern = "gtfs-static-*.zip" + +// fetchFeedFile puts the configured feed in a local file and hashes it, +// without holding the feed in memory. A local feed is read where it is; a +// remote feed is downloaded next to the database. Call cleanup when done. +func fetchFeedFile(ctx context.Context, config Config) (*feedFile, error) { + logger := slog.Default().With(slog.String("component", "gtfs_loader")) + logging.LogOperation(logger, "reading_gtfs_data", slog.String("source", config.GtfsURL)) + + if config.isLocalFile() { + contentHash, err := hashFile(config.GtfsURL) + if err != nil { + return nil, fmt.Errorf("error reading local GTFS file: %w", err) + } + return &feedFile{path: config.GtfsURL, hash: feedHash(contentHash, config), cleanup: func() {}}, nil + } + return downloadFeedFile(ctx, config) +} + +func hashFile(path string) (string, error) { + file, err := os.Open(path) + if err != nil { + return "", err + } + defer func() { _ = file.Close() }() + + hasher := sha256.New() + if _, err := io.Copy(hasher, file); err != nil { + return "", err + } + return hex.EncodeToString(hasher.Sum(nil)), nil +} + +func downloadFeedFile(ctx context.Context, config Config) (*feedFile, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, config.GtfsURL, nil) + if err != nil { + return nil, fmt.Errorf("error creating GTFS request: %w", err) + } + if config.StaticAuthHeaderKey != "" && config.StaticAuthHeaderValue != "" { + req.Header.Set(config.StaticAuthHeaderKey, config.StaticAuthHeaderValue) + } + + resp, err := newStaticHTTPClient().Do(req) + if err != nil { + return nil, fmt.Errorf("error downloading GTFS data: %w", err) + } + defer logging.SafeCloseWithLogging(resp.Body, + slog.Default().With(slog.String("component", "gtfs_downloader")), + "http_response_body") + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("failed to download GTFS data: received HTTP status %s", resp.Status) + } + + removeStaleDownloads(downloadDir(config)) + file, err := os.CreateTemp(downloadDir(config), downloadPattern) + if err != nil { + return nil, fmt.Errorf("error creating file for GTFS download: %w", err) + } + remove := func() { _ = os.Remove(file.Name()) } + + hasher := sha256.New() + written, copyErr := io.Copy(io.MultiWriter(file, hasher), io.LimitReader(resp.Body, maxStaticFeedBytes+1)) + if err := errors.Join(copyErr, file.Close()); err != nil { + remove() + return nil, fmt.Errorf("error reading GTFS data: %w", err) + } + if written > maxStaticFeedBytes { + remove() + return nil, fmt.Errorf("static GTFS response exceeds size limit of %d bytes", maxStaticFeedBytes) + } + return &feedFile{path: file.Name(), hash: feedHash(hex.EncodeToString(hasher.Sum(nil)), config), cleanup: remove}, nil +} + +// newStaticHTTPClient returns a client for downloading a static feed. Like the +// realtime client, its transport is cloned from http.DefaultTransport to keep +// its defaults (proxy from environment, dialer, HTTP/2). +func newStaticHTTPClient() *http.Client { + transport := &http.Transport{} + if t, ok := http.DefaultTransport.(*http.Transport); ok { + transport = t.Clone() + } + transport.TLSHandshakeTimeout = 10 * time.Second + transport.ResponseHeaderTimeout = 30 * time.Second + transport.IdleConnTimeout = 90 * time.Second + return &http.Client{Timeout: 5 * time.Minute, Transport: transport} +} + +// removeStaleDownloads deletes feed downloads that a crashed process never +// cleaned up. Reloads are serialized, so none of them is still in use. +func removeStaleDownloads(dir string) { + if dir == "" { + dir = os.TempDir() + } + stale, _ := filepath.Glob(filepath.Join(dir, downloadPattern)) + for _, path := range stale { + _ = os.Remove(path) + } +} + +// downloadDir keeps a download on the same disk as the database, rather than +// in a temporary directory that may be held in memory (tmpfs). +func downloadDir(config Config) string { + if config.GTFSDataPath == "" || strings.HasPrefix(config.GTFSDataPath, ":memory:") { + return "" + } + return filepath.Dir(config.GTFSDataPath) +} + +// feedHash identifies a feed in import_metadata. With gtfstidy on, the stored +// data comes from the tidied feed, so the setting is part of the identity: +// turning it on or off causes a re-import. +func feedHash(contentHash string, config Config) string { + if config.EnableGTFSTidy { + return contentHash + "+gtfstidy" + } + return contentHash +} + +// parse reads the feed, runs gtfstidy when enabled, then parses and validates +// it. The returned hash is that of the bytes it read, so it matches the stored +// data even if the file changed since it was fetched, and it is marked as +// tidied only if gtfstidy succeeded, so a failed tidy is retried next time. +func (f *feedFile) parse(ctx context.Context, config Config) (*gtfsdb.GtfsData, error) { + b, err := os.ReadFile(f.path) + if err != nil { + return nil, fmt.Errorf("error reading GTFS data: %w", err) + } + sum := sha256.Sum256(b) + hash := hex.EncodeToString(sum[:]) + + if config.EnableGTFSTidy { + logger := slog.Default().With(slog.String("component", "gtfs_loader")) + logging.LogOperation(logger, "gtfstidy_enabled_processing_gtfs_data") + tidiedData, err := tidyGTFSData(ctx, b, logger) + if err != nil { + logging.LogError(logger, "Failed to tidy GTFS data, using original data", err) + } else { + b = tidiedData + hash = feedHash(hash, config) + } + } + + data, err := gtfsdb.ParseGtfsDataWithHash(b, hash, config.GtfsURL) + if err != nil { + return nil, err + } + + if err := validateStaticAgencyTimezones(data.Static); err != nil { + return nil, fmt.Errorf("invalid GTFS agency timezone: %w", err) + } + return data, nil +} diff --git a/internal/gtfs/feed_file_test.go b/internal/gtfs/feed_file_test.go new file mode 100644 index 000000000..94ca9b8c8 --- /dev/null +++ b/internal/gtfs/feed_file_test.go @@ -0,0 +1,204 @@ +package gtfs + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "maglev.onebusaway.org/internal/appconf" + "maglev.onebusaway.org/internal/models" +) + +func rabaContentHash(t *testing.T) string { + t.Helper() + b, err := os.ReadFile(models.GetFixturePath(t, "raba.zip")) + require.NoError(t, err) + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:]) +} + +func TestFetchFeedFileLocal(t *testing.T) { + path := models.GetFixturePath(t, "raba.zip") + feed, err := fetchFeedFile(context.Background(), Config{GtfsURL: path}) + require.NoError(t, err) + defer feed.cleanup() + + assert.Equal(t, path, feed.path, "a local feed is read where it is") + assert.Equal(t, rabaContentHash(t), feed.hash) +} + +func TestFetchFeedFileDownload(t *testing.T) { + raba, err := os.ReadFile(models.GetFixturePath(t, "raba.zip")) + require.NoError(t, err) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("X-Api-Key") != "secret" { + w.WriteHeader(http.StatusForbidden) + return + } + _, _ = w.Write(raba) + })) + t.Cleanup(server.Close) + + dbDir := t.TempDir() + config := Config{ + GtfsURL: server.URL + "/gtfs.zip", + GTFSDataPath: filepath.Join(dbDir, "gtfs.db"), + StaticAuthHeaderKey: "X-Api-Key", + StaticAuthHeaderValue: "secret", + } + feed, err := fetchFeedFile(context.Background(), config) + require.NoError(t, err) + + assert.Equal(t, dbDir, filepath.Dir(feed.path), "the download goes next to the database") + assert.Equal(t, rabaContentHash(t), feed.hash) + feed.cleanup() + _, err = os.Stat(feed.path) + assert.True(t, os.IsNotExist(err), "cleanup removes the download") + + config.StaticAuthHeaderValue = "wrong" + _, err = fetchFeedFile(context.Background(), config) + assert.ErrorContains(t, err, "403") + entries, err := os.ReadDir(dbDir) + require.NoError(t, err) + assert.Empty(t, entries, "a failed download leaves no file behind") +} + +func TestFeedHashIncludesGTFSTidy(t *testing.T) { + assert.Equal(t, "abc", feedHash("abc", Config{})) + assert.Equal(t, "abc+gtfstidy", feedHash("abc", Config{EnableGTFSTidy: true})) +} + +func TestFetchFeedFileTruncatedDownloadLeavesNoFile(t *testing.T) { + const promisedBytes, sentBytes = 1024, 10 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Length", strconv.Itoa(promisedBytes)) + _, _ = w.Write(make([]byte, sentBytes)) + })) + t.Cleanup(server.Close) + + dbDir := t.TempDir() + _, err := fetchFeedFile(context.Background(), Config{GtfsURL: server.URL, GTFSDataPath: filepath.Join(dbDir, "gtfs.db")}) + assert.ErrorContains(t, err, "error reading GTFS data") + + entries, err := os.ReadDir(dbDir) + require.NoError(t, err) + assert.Empty(t, entries, "a truncated download leaves no file behind") +} + +func TestFetchFeedFileServerUnreachable(t *testing.T) { + server := httptest.NewServer(http.NotFoundHandler()) + server.Close() + + _, err := fetchFeedFile(context.Background(), Config{GtfsURL: server.URL, GTFSDataPath: filepath.Join(t.TempDir(), "gtfs.db")}) + assert.ErrorContains(t, err, "error downloading GTFS data") +} + +func TestDownloadDir(t *testing.T) { + tests := []struct { + name, dataPath, want string + }{ + {"no database path", "", ""}, + {"in-memory database", ":memory:", ""}, + {"database file", "/data/gtfs.db", "/data"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, downloadDir(Config{GTFSDataPath: tt.dataPath})) + }) + } +} + +// installFakeGTFSTidy puts a gtfstidy on PATH that copies its input to its +// output, so tidying succeeds without the real binary. +func installFakeGTFSTidy(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the fake gtfstidy is a shell script") + } + dir := t.TempDir() + // tidyGTFSData runs: gtfstidy -o + script := "#!/bin/sh\n/bin/cp \"$4\" \"$3\"\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "gtfstidy"), []byte(script), 0o755)) + t.Setenv("PATH", dir) +} + +// TestParseHashMarksTidyWhenItRan checks that a feed whose tidy succeeded is +// stored under the tidied hash that the next reload looks for. +func TestParseHashMarksTidyWhenItRan(t *testing.T) { + installFakeGTFSTidy(t) + config := Config{GtfsURL: models.GetFixturePath(t, "raba.zip"), EnableGTFSTidy: true} + + feed, err := fetchFeedFile(context.Background(), config) + require.NoError(t, err) + defer feed.cleanup() + + data, err := feed.parse(context.Background(), config) + require.NoError(t, err) + assert.Equal(t, rabaContentHash(t)+"+gtfstidy", data.Hash) + assert.Equal(t, feed.hash, data.Hash, "the stored hash matches the one the next reload checks") +} + +// TestReloadStaticSkipsParseForImportedFeed checks that reloading a feed the +// database already holds doesn't parse it again. +func TestReloadStaticSkipsParseForImportedFeed(t *testing.T) { + counter := countLogMessages(t) + + manager, err := InitGTFSManager(context.Background(), Config{ + GtfsURL: models.GetFixturePath(t, "raba.zip"), + GTFSDataPath: filepath.Join(t.TempDir(), "gtfs.db"), + Env: appconf.Development, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = manager.Shutdown(context.Background()) }) + require.Equal(t, 1, counter.count("parsing_gtfs_data")) + + changed, err := manager.ReloadStatic(context.Background()) + require.NoError(t, err) + assert.False(t, changed) + assert.Equal(t, 1, counter.count("parsing_gtfs_data"), "an unchanged feed must not be parsed again") +} + +func TestFetchFeedFileRemovesStaleDownloads(t *testing.T) { + raba, err := os.ReadFile(models.GetFixturePath(t, "raba.zip")) + require.NoError(t, err) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(raba) + })) + t.Cleanup(server.Close) + + dbDir := t.TempDir() + stale := filepath.Join(dbDir, "gtfs-static-123.zip") + require.NoError(t, os.WriteFile(stale, []byte("left by a crash"), 0o600)) + + feed, err := fetchFeedFile(context.Background(), Config{GtfsURL: server.URL, GTFSDataPath: filepath.Join(dbDir, "gtfs.db")}) + require.NoError(t, err) + defer feed.cleanup() + _, err = os.Stat(stale) + assert.True(t, os.IsNotExist(err)) +} + +// TestParseHashMarksTidyOnlyWhenItRan checks that a feed whose tidy failed is +// stored under its plain hash, so the next reload doesn't match the tidied +// hash and tries again. +func TestParseHashMarksTidyOnlyWhenItRan(t *testing.T) { + t.Setenv("PATH", t.TempDir()) // no gtfstidy binary, so tidying fails + path := models.GetFixturePath(t, "raba.zip") + config := Config{GtfsURL: path, EnableGTFSTidy: true} + + feed, err := fetchFeedFile(context.Background(), config) + require.NoError(t, err) + assert.Equal(t, rabaContentHash(t)+"+gtfstidy", feed.hash, "the check looks for a tidied import") + + data, err := feed.parse(context.Background(), config) + require.NoError(t, err) + assert.Equal(t, rabaContentHash(t), data.Hash, "an untidied import is stored under the plain hash") +} diff --git a/internal/gtfs/static.go b/internal/gtfs/static.go index e836b54d6..8157b5ef1 100644 --- a/internal/gtfs/static.go +++ b/internal/gtfs/static.go @@ -3,10 +3,7 @@ package gtfs import ( "context" "fmt" - "io" "log/slog" - "net/http" - "os" "runtime/debug" "strings" "time" @@ -16,72 +13,6 @@ import ( "maglev.onebusaway.org/internal/logging" ) -func rawGtfsData(ctx context.Context, source string, config Config) ([]byte, error) { - var b []byte - var err error - - logger := slog.Default().With(slog.String("component", "gtfs_loader")) - logging.LogOperation(logger, "reading_gtfs_data", slog.String("source", source)) - - if config.isLocalFile() { - b, err = os.ReadFile(source) - if err != nil { - return nil, fmt.Errorf("error reading local GTFS file: %w", err) - } - } else { - req, err := http.NewRequestWithContext(ctx, "GET", source, nil) - if err != nil { - return nil, fmt.Errorf("error creating GTFS request: %w", err) - } - - // Add auth header if provided - if config.StaticAuthHeaderKey != "" && config.StaticAuthHeaderValue != "" { - req.Header.Set(config.StaticAuthHeaderKey, config.StaticAuthHeaderValue) - } - - client := &http.Client{ - Timeout: 5 * time.Minute, - Transport: &http.Transport{ - TLSHandshakeTimeout: 10 * time.Second, - ResponseHeaderTimeout: 30 * time.Second, - IdleConnTimeout: 90 * time.Second, - }} - - resp, err := client.Do(req) - if err != nil { - return nil, fmt.Errorf("error downloading GTFS data: %w", err) - } - defer logging.SafeCloseWithLogging(resp.Body, - slog.Default().With(slog.String("component", "gtfs_downloader")), - "http_response_body") - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to download GTFS data: received HTTP status %s", resp.Status) - } - const maxStaticSize = 200 * 1024 * 1024 - b, err = io.ReadAll(io.LimitReader(resp.Body, maxStaticSize+1)) - if err != nil { - return nil, fmt.Errorf("error reading GTFS data: %w", err) - } - if int64(len(b)) > maxStaticSize { - return nil, fmt.Errorf("static GTFS response exceeds size limit of %d bytes", maxStaticSize) - } - } - - // Process through gtfstidy if enabled - if config.EnableGTFSTidy { - logging.LogOperation(logger, "gtfstidy_enabled_processing_gtfs_data") - tidiedData, err := tidyGTFSData(ctx, b, logger) - if err != nil { - logging.LogError(logger, "Failed to tidy GTFS data, using original data", err) - } else { - b = tidiedData - } - } - - return b, nil -} - // openGtfsDB opens (or creates) the single SQLite database used by the manager. // No import work happens here — ReloadStatic imports into the returned client. func openGtfsDB(config Config) (*gtfsdb.Client, error) { @@ -112,25 +43,6 @@ func newGTFSDBConfig(dbPath string, config Config) gtfsdb.Config { return dbConfig } -// loadGTFSData loads, parses, hashes, and validates GTFS data from either a URL or a local file. -func loadGTFSData(ctx context.Context, config Config) (*gtfsdb.GtfsData, error) { - b, err := rawGtfsData(ctx, config.GtfsURL, config) - if err != nil { - return nil, fmt.Errorf("error reading GTFS data: %w", err) - } - - data, err := gtfsdb.ParseGtfsData(b, config.GtfsURL) - if err != nil { - return nil, err - } - - if err := validateStaticAgencyTimezones(data.Static); err != nil { - return nil, fmt.Errorf("invalid GTFS agency timezone: %w", err) - } - - return data, nil -} - func validateStaticAgencyTimezones(staticData *gtfs.Static) error { for i, agency := range staticData.Agencies { tz := strings.TrimSpace(agency.Timezone) @@ -231,11 +143,35 @@ func (manager *Manager) ReloadStatic(ctx context.Context) (bool, error) { return changed, nil } -// storeConfiguredFeed loads the configured feed and stores it in the -// database. The parsed feed is reachable only from here, so it can be freed -// as soon as this returns. It reports whether the database changed. +// storeConfiguredFeed fetches the configured feed and, unless the database +// already holds it, parses and stores it. The parsed feed is reachable only +// from here, so it can be freed as soon as this returns. It reports whether +// the database changed. func (manager *Manager) storeConfiguredFeed(ctx context.Context, logger *slog.Logger) (bool, error) { - newData, err := loadGTFSData(ctx, manager.config) + feed, err := fetchFeedFile(ctx, manager.config) + if err != nil { + logging.LogError(logger, "Error loading GTFS data", err, + slog.String("source", manager.config.GtfsURL)) + return false, err + } + defer feed.cleanup() + + imported, err := manager.GtfsDB.HasImported(ctx, feed.hash, manager.config.GtfsURL) + if err != nil { + logging.LogError(logger, "Error checking whether GTFS data is already imported", err, + slog.String("source", manager.config.GtfsURL)) + return false, err + } + if imported { + if err := manager.GtfsDB.RefreshFeedExpiry(ctx); err != nil { + logging.LogError(logger, "Error refreshing feed expiry", err) + return false, err + } + logging.LogOperation(logger, "gtfs_data_unchanged_skipping_parse", slog.String("hash", feed.hash)) + return false, nil + } + + newData, err := feed.parse(ctx, manager.config) if err != nil { logging.LogError(logger, "Error loading GTFS data", err, slog.String("source", manager.config.GtfsURL)) diff --git a/internal/gtfs/static_test.go b/internal/gtfs/static_test.go index 168626875..15b4a5224 100644 --- a/internal/gtfs/static_test.go +++ b/internal/gtfs/static_test.go @@ -2,10 +2,12 @@ package gtfs import ( "context" + "database/sql" "log/slog" "path/filepath" "sync" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -89,3 +91,72 @@ func TestReloadStaticPrecomputesOnlyWhenChanged(t *testing.T) { assert.False(t, changed) assert.Equal(t, 1, counter.count("precomputing_stop_directions_started")) } + +// TestReloadStaticUnchangedFeedRefreshesExpiry checks that a reload that finds +// the same feed skips the parse but still recomputes feed_expires_at, so a +// database written by an older build picks up a corrected expiry. +func TestReloadStaticUnchangedFeedRefreshesExpiry(t *testing.T) { + counter := countLogMessages(t) + manager, err := InitGTFSManager(context.Background(), Config{ + GtfsURL: models.GetFixturePath(t, "raba.zip"), + GTFSDataPath: filepath.Join(t.TempDir(), "gtfs.db"), + Env: appconf.Development, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = manager.Shutdown(context.Background()) }) + + want := manager.FeedExpiresAt(context.Background()) + require.False(t, want.IsZero()) + stale := sql.NullInt64{Int64: want.Add(-24 * time.Hour).Unix(), Valid: true} + require.NoError(t, manager.GtfsDB.Queries.UpdateFeedExpiresAt(context.Background(), stale)) + + changed, err := manager.ReloadStatic(context.Background()) + require.NoError(t, err) + assert.False(t, changed) + assert.Equal(t, 1, counter.count("gtfs_data_unchanged_skipping_parse")) + assert.Equal(t, want, manager.FeedExpiresAt(context.Background())) +} + +// initManagerWithFileDB imports RABA into a database file and returns the +// manager, so a test can then change the database under a reload. +func initManagerWithFileDB(t *testing.T) *Manager { + t.Helper() + manager, err := InitGTFSManager(context.Background(), Config{ + GtfsURL: models.GetFixturePath(t, "raba.zip"), + GTFSDataPath: filepath.Join(t.TempDir(), "gtfs.db"), + Env: appconf.Development, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = manager.Shutdown(context.Background()) }) + return manager +} + +// TestReloadStaticUnchangedFeedExpiryFailure checks that a reload of an +// unchanged feed reports a failure to refresh feed_expires_at, and doesn't +// fall back to parsing the feed. +func TestReloadStaticUnchangedFeedExpiryFailure(t *testing.T) { + counter := countLogMessages(t) + manager := initManagerWithFileDB(t) + require.Equal(t, 1, counter.count("parsing_gtfs_data")) + + _, err := manager.GtfsDB.DB.Exec(`UPDATE agencies SET timezone = 'Invalid/Timezone'`) + require.NoError(t, err) + + changed, err := manager.ReloadStatic(context.Background()) + assert.ErrorContains(t, err, "failed to refresh feed expiry") + assert.False(t, changed) + assert.Equal(t, 1, counter.count("parsing_gtfs_data"), "a failed refresh must not parse the feed") +} + +// TestReloadStaticHasImportedError checks that a reload reports a failure to +// read import_metadata instead of treating the feed as new. +func TestReloadStaticHasImportedError(t *testing.T) { + manager := initManagerWithFileDB(t) + + _, err := manager.GtfsDB.DB.Exec(`DROP TABLE import_metadata`) + require.NoError(t, err) + + changed, err := manager.ReloadStatic(context.Background()) + assert.ErrorContains(t, err, "error checking import metadata") + assert.False(t, changed) +} From 7d8045d9c7410cc72cbc94b738ac395db0ab7eeb Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 7 Nov 2026 10:28:56 -0800 Subject: [PATCH 3/8] Store a failed tidy under the tidied hash With gtfstidy on, a feed was stored under its plain hash when tidying failed, but every reload looked for the hash marked "+gtfstidy". The check never matched, so a feed whose tidy always fails was read, tidied again and parsed on every restart and reload, and only then skipped by StoreGtfsData. Hash-first never helped such a feed. The import hash now depends only on the feed's content and whether tidy is on. A failed tidy is stored under the marked hash, so the next reload skips the feed, and tidy is retried when the feed changes. --- internal/gtfs/feed_file.go | 11 ++++++----- internal/gtfs/feed_file_test.go | 25 +++++++++++++++---------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/internal/gtfs/feed_file.go b/internal/gtfs/feed_file.go index b795fb111..f183da2dc 100644 --- a/internal/gtfs/feed_file.go +++ b/internal/gtfs/feed_file.go @@ -141,7 +141,10 @@ func downloadDir(config Config) string { // feedHash identifies a feed in import_metadata. With gtfstidy on, the stored // data comes from the tidied feed, so the setting is part of the identity: -// turning it on or off causes a re-import. +// turning it on or off causes a re-import. The mark records that tidy was on, +// not that it succeeded: a feed whose tidy fails is stored under the same +// hash, so the next reload skips it instead of parsing it again. Tidy is +// retried when the feed changes. func feedHash(contentHash string, config Config) string { if config.EnableGTFSTidy { return contentHash + "+gtfstidy" @@ -151,15 +154,14 @@ func feedHash(contentHash string, config Config) string { // parse reads the feed, runs gtfstidy when enabled, then parses and validates // it. The returned hash is that of the bytes it read, so it matches the stored -// data even if the file changed since it was fetched, and it is marked as -// tidied only if gtfstidy succeeded, so a failed tidy is retried next time. +// data even if the file changed since it was fetched. func (f *feedFile) parse(ctx context.Context, config Config) (*gtfsdb.GtfsData, error) { b, err := os.ReadFile(f.path) if err != nil { return nil, fmt.Errorf("error reading GTFS data: %w", err) } sum := sha256.Sum256(b) - hash := hex.EncodeToString(sum[:]) + hash := feedHash(hex.EncodeToString(sum[:]), config) if config.EnableGTFSTidy { logger := slog.Default().With(slog.String("component", "gtfs_loader")) @@ -169,7 +171,6 @@ func (f *feedFile) parse(ctx context.Context, config Config) (*gtfsdb.GtfsData, logging.LogError(logger, "Failed to tidy GTFS data, using original data", err) } else { b = tidiedData - hash = feedHash(hash, config) } } diff --git a/internal/gtfs/feed_file_test.go b/internal/gtfs/feed_file_test.go index 94ca9b8c8..a8d691a23 100644 --- a/internal/gtfs/feed_file_test.go +++ b/internal/gtfs/feed_file_test.go @@ -186,19 +186,24 @@ func TestFetchFeedFileRemovesStaleDownloads(t *testing.T) { assert.True(t, os.IsNotExist(err)) } -// TestParseHashMarksTidyOnlyWhenItRan checks that a feed whose tidy failed is -// stored under its plain hash, so the next reload doesn't match the tidied -// hash and tries again. -func TestParseHashMarksTidyOnlyWhenItRan(t *testing.T) { +// TestReloadStaticSkipsParseWhenTidyFails checks that a feed whose tidy fails +// is stored under the hash the next reload looks for, so it isn't parsed again. +func TestReloadStaticSkipsParseWhenTidyFails(t *testing.T) { t.Setenv("PATH", t.TempDir()) // no gtfstidy binary, so tidying fails - path := models.GetFixturePath(t, "raba.zip") - config := Config{GtfsURL: path, EnableGTFSTidy: true} + counter := countLogMessages(t) - feed, err := fetchFeedFile(context.Background(), config) + manager, err := InitGTFSManager(context.Background(), Config{ + GtfsURL: models.GetFixturePath(t, "raba.zip"), + GTFSDataPath: filepath.Join(t.TempDir(), "gtfs.db"), + Env: appconf.Development, + EnableGTFSTidy: true, + }) require.NoError(t, err) - assert.Equal(t, rabaContentHash(t)+"+gtfstidy", feed.hash, "the check looks for a tidied import") + t.Cleanup(func() { _ = manager.Shutdown(context.Background()) }) + require.Equal(t, 1, counter.count("parsing_gtfs_data")) - data, err := feed.parse(context.Background(), config) + changed, err := manager.ReloadStatic(context.Background()) require.NoError(t, err) - assert.Equal(t, rabaContentHash(t), data.Hash, "an untidied import is stored under the plain hash") + assert.False(t, changed) + assert.Equal(t, 1, counter.count("parsing_gtfs_data"), "a feed whose tidy failed must not be parsed again") } From 1d4a092b99a7be518221b6650238458abae1656c Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 7 Nov 2026 10:29:48 -0800 Subject: [PATCH 4/8] Share one skip rule for unchanged feeds The hash-first check in storeConfiguredFeed and the check inside StoreGtfsData each compared import_metadata and refreshed the feed expiry on their own, and logged the skip differently: one with the full hash, the other with its first 8 characters. A later change to one path would make an unchanged feed behave differently depending on which check caught it. Client.SkipIfImported replaces HasImported and, like StoreGtfsData, goes through skipUnchangedFeed, which compares the feed, refreshes the expiry and logs gtfs_data_unchanged_skipping_import with the short hash. RefreshFeedExpiry is no longer needed outside gtfsdb, so it is unexported. --- gtfsdb/conditional_import_test.go | 6 ++--- gtfsdb/helpers.go | 43 +++++++++++++++++++------------ internal/gtfs/static.go | 9 ++----- internal/gtfs/static_test.go | 6 ++--- 4 files changed, 34 insertions(+), 30 deletions(-) diff --git a/gtfsdb/conditional_import_test.go b/gtfsdb/conditional_import_test.go index 1cc1a6707..ec995e9fd 100644 --- a/gtfsdb/conditional_import_test.go +++ b/gtfsdb/conditional_import_test.go @@ -326,12 +326,12 @@ func TestClearAllGTFSData(t *testing.T) { assert.NotEmpty(t, metadata.FileHash, "Import metadata should not be cleared") } -func TestHasImported(t *testing.T) { +func TestSkipIfImported(t *testing.T) { client, err := NewClient(Config{DBPath: ":memory:", Env: appconf.Test}) require.NoError(t, err) t.Cleanup(func() { _ = client.Close() }) - imported, err := client.HasImported(t.Context(), "hash", "source") + imported, err := client.SkipIfImported(t.Context(), "hash", "source") require.NoError(t, err) assert.False(t, imported, "an empty database holds no feed") @@ -350,7 +350,7 @@ func TestHasImported(t *testing.T) { {parsed.Hash, "other-source", false}, {"other-hash", "source", false}, } { - imported, err := client.HasImported(t.Context(), tt.hash, tt.source) + imported, err := client.SkipIfImported(t.Context(), tt.hash, tt.source) require.NoError(t, err) assert.Equal(t, tt.want, imported, "%s from %s", tt.hash, tt.source) } diff --git a/gtfsdb/helpers.go b/gtfsdb/helpers.go index 95190191e..3c1c059bb 100644 --- a/gtfsdb/helpers.go +++ b/gtfsdb/helpers.go @@ -208,9 +208,11 @@ func (c *Client) withTransaction(ctx context.Context, tx *sql.Tx, label string, return nil } -// HasImported reports whether the database already holds the feed with this -// hash from this source, so a caller can skip parsing it. -func (c *Client) HasImported(ctx context.Context, hash, source string) (bool, error) { +// SkipIfImported reports whether the database already holds the feed with +// this hash from this source, so a caller can skip parsing and storing it. +// When it does, it refreshes the feed's derived metadata, as StoreGtfsData +// does for an unchanged feed. +func (c *Client) SkipIfImported(ctx context.Context, hash, source string) (bool, error) { metadata, err := c.Queries.GetImportMetadata(ctx) if errors.Is(err, sql.ErrNoRows) { return false, nil @@ -218,19 +220,28 @@ func (c *Client) HasImported(ctx context.Context, hash, source string) (bool, er if err != nil { return false, fmt.Errorf("error checking import metadata: %w", err) } - return isSameFeed(metadata, hash, source), nil + return c.skipUnchangedFeed(ctx, metadata, hash, source) } -// isSameFeed reports whether import_metadata describes the feed with this hash -// from this source. -func isSameFeed(metadata ImportMetadatum, hash, source string) bool { - return metadata.FileHash == hash && metadata.FileSource == source +// skipUnchangedFeed is the one rule for a feed the database already holds. +// When import_metadata describes the feed with this hash from this source, it +// refreshes the derived metadata and reports true; otherwise it does nothing. +func (c *Client) skipUnchangedFeed(ctx context.Context, metadata ImportMetadatum, hash, source string) (bool, error) { + if metadata.FileHash != hash || metadata.FileSource != source { + return false, nil + } + if err := c.refreshFeedExpiry(ctx); err != nil { + return false, err + } + logger := slog.Default().With(slog.String("component", "gtfs_importer")) + logging.LogOperation(logger, "gtfs_data_unchanged_skipping_import", slog.String("hash", hash[:8])) + return true, nil } -// RefreshFeedExpiry recomputes feed_expires_at from the stored calendar. It +// refreshFeedExpiry recomputes feed_expires_at from the stored calendar. It // runs when a feed is found unchanged, so existing databases pick up // corrections to the expiry calculation on upgrade without a re-import. -func (c *Client) RefreshFeedExpiry(ctx context.Context) error { +func (c *Client) refreshFeedExpiry(ctx context.Context) error { err := c.withTransaction(ctx, nil, "refresh_feed_expiry", func(tx *sql.Tx) error { return updateFeedExpiresAtFromCalendar(ctx, c.Queries.WithTx(tx)) }) @@ -259,13 +270,11 @@ func (c *Client) StoreGtfsData(ctx context.Context, data *GtfsData) (bool, error existingMetadata, err := c.Queries.GetImportMetadata(ctx) if err == nil { hasExisting = true - // We have existing metadata, check if hash matches - if isSameFeed(existingMetadata, data.Hash, data.Source) { - if err := c.RefreshFeedExpiry(ctx); err != nil { - return false, err - } - logging.LogOperation(logger, "gtfs_data_unchanged_skipping_import", - slog.String("hash", data.Hash[:8])) + skipped, err := c.skipUnchangedFeed(ctx, existingMetadata, data.Hash, data.Source) + if err != nil { + return false, err + } + if skipped { return false, nil } } else if err != sql.ErrNoRows { diff --git a/internal/gtfs/static.go b/internal/gtfs/static.go index 8157b5ef1..f6c51ee4a 100644 --- a/internal/gtfs/static.go +++ b/internal/gtfs/static.go @@ -156,18 +156,13 @@ func (manager *Manager) storeConfiguredFeed(ctx context.Context, logger *slog.Lo } defer feed.cleanup() - imported, err := manager.GtfsDB.HasImported(ctx, feed.hash, manager.config.GtfsURL) + skipped, err := manager.GtfsDB.SkipIfImported(ctx, feed.hash, manager.config.GtfsURL) if err != nil { logging.LogError(logger, "Error checking whether GTFS data is already imported", err, slog.String("source", manager.config.GtfsURL)) return false, err } - if imported { - if err := manager.GtfsDB.RefreshFeedExpiry(ctx); err != nil { - logging.LogError(logger, "Error refreshing feed expiry", err) - return false, err - } - logging.LogOperation(logger, "gtfs_data_unchanged_skipping_parse", slog.String("hash", feed.hash)) + if skipped { return false, nil } diff --git a/internal/gtfs/static_test.go b/internal/gtfs/static_test.go index 15b4a5224..51d6c1c2d 100644 --- a/internal/gtfs/static_test.go +++ b/internal/gtfs/static_test.go @@ -113,7 +113,7 @@ func TestReloadStaticUnchangedFeedRefreshesExpiry(t *testing.T) { changed, err := manager.ReloadStatic(context.Background()) require.NoError(t, err) assert.False(t, changed) - assert.Equal(t, 1, counter.count("gtfs_data_unchanged_skipping_parse")) + assert.Equal(t, 1, counter.count("gtfs_data_unchanged_skipping_import")) assert.Equal(t, want, manager.FeedExpiresAt(context.Background())) } @@ -148,9 +148,9 @@ func TestReloadStaticUnchangedFeedExpiryFailure(t *testing.T) { assert.Equal(t, 1, counter.count("parsing_gtfs_data"), "a failed refresh must not parse the feed") } -// TestReloadStaticHasImportedError checks that a reload reports a failure to +// TestReloadStaticSkipIfImportedError checks that a reload reports a failure to // read import_metadata instead of treating the feed as new. -func TestReloadStaticHasImportedError(t *testing.T) { +func TestReloadStaticSkipIfImportedError(t *testing.T) { manager := initManagerWithFileDB(t) _, err := manager.GtfsDB.DB.Exec(`DROP TABLE import_metadata`) From 1df548cd96dd23f28b6daa7d0a350ac0ef024e8c Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 7 Nov 2026 10:30:52 -0800 Subject: [PATCH 5/8] Keep each process's feed downloads apart Before each download, every gtfs-static-*.zip in the database's directory, or in the temporary directory for an in-memory database, was removed. Reloads are serialized only within one Manager, so a second process whose database shares that directory, or a second in-memory process, could remove a download between its fetch and its parse and fail the reload. Any operator file that matched the pattern was removed too. A download's name now carries its owner: the database file's name, or the process ID for an in-memory database. A process removes only its own leftovers, matched by the exact name os.CreateTemp gives them. The database path is also read as SQLite reads it. A "file:" URI, such as file:/data/gtfs.db?cache=shared, was taken as a directory named "file:", so every remote reload failed, and file::memory:?cache=shared sent downloads to the working directory. --- internal/gtfs/feed_file.go | 98 ++++++++++++++++++++++++++------- internal/gtfs/feed_file_test.go | 44 ++++++++++++--- 2 files changed, 114 insertions(+), 28 deletions(-) diff --git a/internal/gtfs/feed_file.go b/internal/gtfs/feed_file.go index f183da2dc..93cb2efd1 100644 --- a/internal/gtfs/feed_file.go +++ b/internal/gtfs/feed_file.go @@ -9,8 +9,10 @@ import ( "io" "log/slog" "net/http" + "net/url" "os" "path/filepath" + "strconv" "strings" "time" @@ -29,10 +31,6 @@ type feedFile struct { cleanup func() } -// downloadPattern names feed downloads, so ones left behind by a crash can be -// found and removed. -const downloadPattern = "gtfs-static-*.zip" - // fetchFeedFile puts the configured feed in a local file and hashes it, // without holding the feed in memory. A local feed is read where it is; a // remote feed is downloaded next to the database. Call cleanup when done. @@ -84,8 +82,7 @@ func downloadFeedFile(ctx context.Context, config Config) (*feedFile, error) { return nil, fmt.Errorf("failed to download GTFS data: received HTTP status %s", resp.Status) } - removeStaleDownloads(downloadDir(config)) - file, err := os.CreateTemp(downloadDir(config), downloadPattern) + file, err := newDownloadLocation(config).create() if err != nil { return nil, fmt.Errorf("error creating file for GTFS download: %w", err) } @@ -118,25 +115,86 @@ func newStaticHTTPClient() *http.Client { return &http.Client{Timeout: 5 * time.Minute, Transport: transport} } -// removeStaleDownloads deletes feed downloads that a crashed process never -// cleaned up. Reloads are serialized, so none of them is still in use. -func removeStaleDownloads(dir string) { - if dir == "" { - dir = os.TempDir() +// Feed downloads are named downloadPrefix, then their owner, then "-" and the +// random digits os.CreateTemp adds, then downloadSuffix. The owner part lets a +// process find the downloads it left behind in a crash without touching +// anyone else's. +const ( + downloadPrefix = "gtfs-static-" + downloadSuffix = ".zip" +) + +// sqliteMemoryDatabase is the name SQLite gives an in-memory database. +const sqliteMemoryDatabase = ":memory:" + +// downloadLocation is where a process keeps its feed downloads, and the name +// prefix that marks them as its own. +type downloadLocation struct { + dir string + prefix string +} + +// newDownloadLocation keeps downloads on the same disk as the database, rather +// than in a temporary directory that may be held in memory (tmpfs), and names +// them after the database file, so processes whose databases share a +// directory leave each other's downloads alone. With an in-memory database, +// downloads go to the temporary directory and are named after the process. +func newDownloadLocation(config Config) downloadLocation { + dbFile, onDisk := databaseFile(config.GTFSDataPath) + if !onDisk { + owner := "pid" + strconv.Itoa(os.Getpid()) + return downloadLocation{dir: os.TempDir(), prefix: downloadPrefix + owner + "-"} + } + return downloadLocation{dir: filepath.Dir(dbFile), prefix: downloadPrefix + filepath.Base(dbFile) + "-"} +} + +// databaseFile returns the file that holds the database at dataPath, a path +// or a SQLite "file:" URI, and reports whether the database is on disk at +// all rather than in memory. +func databaseFile(dataPath string) (string, bool) { + if !strings.HasPrefix(dataPath, "file:") { + inMemory := dataPath == "" || strings.HasPrefix(dataPath, sqliteMemoryDatabase) + return dataPath, !inMemory } - stale, _ := filepath.Glob(filepath.Join(dir, downloadPattern)) - for _, path := range stale { - _ = os.Remove(path) + uri, err := url.Parse(dataPath) + if err != nil { + return "", false + } + path := uri.Path + if path == "" { + path = uri.Opaque + } + inMemory := path == "" || path == sqliteMemoryDatabase || uri.Query().Get("mode") == "memory" + return path, !inMemory +} + +// create removes the downloads this owner left behind, then creates a new one. +func (l downloadLocation) create() (*os.File, error) { + l.removeStale() + return os.CreateTemp(l.dir, l.prefix+"*"+downloadSuffix) +} + +// removeStale deletes downloads this owner left behind in a crash. An owner's +// reloads are serialized, so none of them is still in use. +func (l downloadLocation) removeStale() { + entries, _ := os.ReadDir(l.dir) + for _, entry := range entries { + if entry.Type().IsRegular() && l.owns(entry.Name()) { + _ = os.Remove(filepath.Join(l.dir, entry.Name())) + } } } -// downloadDir keeps a download on the same disk as the database, rather than -// in a temporary directory that may be held in memory (tmpfs). -func downloadDir(config Config) string { - if config.GTFSDataPath == "" || strings.HasPrefix(config.GTFSDataPath, ":memory:") { - return "" +// owns reports whether name is one of this owner's downloads. The random part +// must be all digits, so another owner whose name merely starts with this one +// (gtfs.db-old next to gtfs.db) doesn't match. +func (l downloadLocation) owns(name string) bool { + random, found := strings.CutPrefix(name, l.prefix) + if !found { + return false } - return filepath.Dir(config.GTFSDataPath) + random, found = strings.CutSuffix(random, downloadSuffix) + return found && random != "" && strings.Trim(random, "0123456789") == "" } // feedHash identifies a feed in import_metadata. With gtfstidy on, the stored diff --git a/internal/gtfs/feed_file_test.go b/internal/gtfs/feed_file_test.go index a8d691a23..f7c515d0a 100644 --- a/internal/gtfs/feed_file_test.go +++ b/internal/gtfs/feed_file_test.go @@ -102,21 +102,46 @@ func TestFetchFeedFileServerUnreachable(t *testing.T) { assert.ErrorContains(t, err, "error downloading GTFS data") } -func TestDownloadDir(t *testing.T) { +func TestNewDownloadLocation(t *testing.T) { + inMemory := downloadLocation{dir: os.TempDir(), prefix: "gtfs-static-pid" + strconv.Itoa(os.Getpid()) + "-"} tests := []struct { - name, dataPath, want string + name, dataPath string + want downloadLocation }{ - {"no database path", "", ""}, - {"in-memory database", ":memory:", ""}, - {"database file", "/data/gtfs.db", "/data"}, + {"no database path", "", inMemory}, + {"in-memory database", ":memory:", inMemory}, + {"shared in-memory URI", "file::memory:?cache=shared", inMemory}, + {"in-memory mode URI", "file:gtfs.db?mode=memory", inMemory}, + {"database file", "/data/gtfs.db", downloadLocation{"/data", "gtfs-static-gtfs.db-"}}, + {"relative database file", "gtfs.db", downloadLocation{".", "gtfs-static-gtfs.db-"}}, + {"database URI", "file:/data/gtfs.db?cache=shared", downloadLocation{"/data", "gtfs-static-gtfs.db-"}}, + {"relative database URI", "file:data/gtfs.db", downloadLocation{"data", "gtfs-static-gtfs.db-"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, downloadDir(Config{GTFSDataPath: tt.dataPath})) + assert.Equal(t, tt.want, newDownloadLocation(Config{GTFSDataPath: tt.dataPath})) }) } } +func TestDownloadLocationOwns(t *testing.T) { + location := downloadLocation{dir: ".", prefix: "gtfs-static-gtfs.db-"} + tests := []struct { + name string + want bool + }{ + {"gtfs-static-gtfs.db-123.zip", true}, + {"gtfs-static-gtfs.db-.zip", false}, + {"gtfs-static-gtfs.db-old-123.zip", false}, + {"gtfs-static-other.db-123.zip", false}, + {"gtfs-static-gtfs.db-123.zip.part", false}, + {"gtfs-static-123.zip", false}, + } + for _, tt := range tests { + assert.Equal(t, tt.want, location.owns(tt.name), tt.name) + } +} + // installFakeGTFSTidy puts a gtfstidy on PATH that copies its input to its // output, so tidying succeeds without the real binary. func installFakeGTFSTidy(t *testing.T) { @@ -176,14 +201,17 @@ func TestFetchFeedFileRemovesStaleDownloads(t *testing.T) { t.Cleanup(server.Close) dbDir := t.TempDir() - stale := filepath.Join(dbDir, "gtfs-static-123.zip") + stale := filepath.Join(dbDir, "gtfs-static-gtfs.db-123.zip") require.NoError(t, os.WriteFile(stale, []byte("left by a crash"), 0o600)) + othersDownload := filepath.Join(dbDir, "gtfs-static-other.db-456.zip") + require.NoError(t, os.WriteFile(othersDownload, []byte("in use by another process"), 0o600)) feed, err := fetchFeedFile(context.Background(), Config{GtfsURL: server.URL, GTFSDataPath: filepath.Join(dbDir, "gtfs.db")}) require.NoError(t, err) defer feed.cleanup() _, err = os.Stat(stale) - assert.True(t, os.IsNotExist(err)) + assert.True(t, os.IsNotExist(err), "a download left by this database's process is removed") + assert.FileExists(t, othersDownload, "another database's download is left alone") } // TestReloadStaticSkipsParseWhenTidyFails checks that a feed whose tidy fails From 87edf798b42d30b3a57dc2b39aa71b3d4ac486fd Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 7 Nov 2026 10:31:21 -0800 Subject: [PATCH 6/8] Hash a downloaded feed only once downloadFeedFile hashes the feed while it writes it to disk, and parse then hashed the same bytes again, about a second of extra work on a 200 MB feed. Only a local feed can change between the fetch and the parse; a download is a file private to this import. feedFile now records whether it is private, and parse hashes again only a local feed. --- internal/gtfs/feed_file.go | 25 ++++++++++++++++++------- internal/gtfs/feed_file_test.go | 23 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/internal/gtfs/feed_file.go b/internal/gtfs/feed_file.go index 93cb2efd1..455dfc4a1 100644 --- a/internal/gtfs/feed_file.go +++ b/internal/gtfs/feed_file.go @@ -26,8 +26,11 @@ const maxStaticFeedBytes = 200 * 1024 * 1024 // feedFile is a static GTFS zip on local disk. hash identifies it in // import_metadata when it is imported as configured. type feedFile struct { - path string - hash string + path string + hash string + // private is set for a download, which nothing but this import can + // change, so hash still describes it when it is parsed. + private bool cleanup func() } @@ -98,7 +101,12 @@ func downloadFeedFile(ctx context.Context, config Config) (*feedFile, error) { remove() return nil, fmt.Errorf("static GTFS response exceeds size limit of %d bytes", maxStaticFeedBytes) } - return &feedFile{path: file.Name(), hash: feedHash(hex.EncodeToString(hasher.Sum(nil)), config), cleanup: remove}, nil + return &feedFile{ + path: file.Name(), + hash: feedHash(hex.EncodeToString(hasher.Sum(nil)), config), + private: true, + cleanup: remove, + }, nil } // newStaticHTTPClient returns a client for downloading a static feed. Like the @@ -211,15 +219,18 @@ func feedHash(contentHash string, config Config) string { } // parse reads the feed, runs gtfstidy when enabled, then parses and validates -// it. The returned hash is that of the bytes it read, so it matches the stored -// data even if the file changed since it was fetched. +// it. A local feed is hashed again from the bytes read, so the stored hash +// matches the stored data even if the file changed since it was fetched. func (f *feedFile) parse(ctx context.Context, config Config) (*gtfsdb.GtfsData, error) { b, err := os.ReadFile(f.path) if err != nil { return nil, fmt.Errorf("error reading GTFS data: %w", err) } - sum := sha256.Sum256(b) - hash := feedHash(hex.EncodeToString(sum[:]), config) + hash := f.hash + if !f.private { + sum := sha256.Sum256(b) + hash = feedHash(hex.EncodeToString(sum[:]), config) + } if config.EnableGTFSTidy { logger := slog.Default().With(slog.String("component", "gtfs_loader")) diff --git a/internal/gtfs/feed_file_test.go b/internal/gtfs/feed_file_test.go index f7c515d0a..f133c6195 100644 --- a/internal/gtfs/feed_file_test.go +++ b/internal/gtfs/feed_file_test.go @@ -235,3 +235,26 @@ func TestReloadStaticSkipsParseWhenTidyFails(t *testing.T) { assert.False(t, changed) assert.Equal(t, 1, counter.count("parsing_gtfs_data"), "a feed whose tidy failed must not be parsed again") } + +// TestParseHashesOnlyLocalFeedsAgain checks that parse trusts the hash of a +// download, which nothing else can change, and hashes a local feed again. +func TestParseHashesOnlyLocalFeedsAgain(t *testing.T) { + const fetchedHash = "hash-when-fetched" + path := models.GetFixturePath(t, "raba.zip") + tests := []struct { + name string + private bool + want string + }{ + {"download", true, fetchedHash}, + {"local feed", false, rabaContentHash(t)}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + feed := &feedFile{path: path, hash: fetchedHash, private: tt.private, cleanup: func() {}} + data, err := feed.parse(context.Background(), Config{GtfsURL: path}) + require.NoError(t, err) + assert.Equal(t, tt.want, data.Hash) + }) + } +} From 3d6de3d5f88196c768a30e86db4936250d048dfb Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 7 Nov 2026 10:32:28 -0800 Subject: [PATCH 7/8] Pass the feed file to gtfstidy With gtfstidy on, parse read the whole feed from disk into memory only so tidyGTFSData could write it back out to a temporary file for gtfstidy. That added feed-sized copies at the memory peak this change set out to lower. tidyGTFSFile now gives gtfstidy the feed's own path, made absolute because gtfstidy runs in its temporary directory. The feed is read into memory untidied only when tidying fails. --- internal/gtfs/feed_file.go | 51 ++++++++++++++++++++++++-------------- internal/gtfs/tidy.go | 29 +++++++++++++--------- internal/gtfs/tidy_test.go | 6 ++--- 3 files changed, 52 insertions(+), 34 deletions(-) diff --git a/internal/gtfs/feed_file.go b/internal/gtfs/feed_file.go index 455dfc4a1..7a7f6224b 100644 --- a/internal/gtfs/feed_file.go +++ b/internal/gtfs/feed_file.go @@ -219,37 +219,50 @@ func feedHash(contentHash string, config Config) string { } // parse reads the feed, runs gtfstidy when enabled, then parses and validates -// it. A local feed is hashed again from the bytes read, so the stored hash -// matches the stored data even if the file changed since it was fetched. +// it. func (f *feedFile) parse(ctx context.Context, config Config) (*gtfsdb.GtfsData, error) { - b, err := os.ReadFile(f.path) + b, hash, err := f.read(ctx, config) if err != nil { - return nil, fmt.Errorf("error reading GTFS data: %w", err) + return nil, err } - hash := f.hash - if !f.private { - sum := sha256.Sum256(b) - hash = feedHash(hex.EncodeToString(sum[:]), config) + + data, err := gtfsdb.ParseGtfsDataWithHash(b, hash, config.GtfsURL) + if err != nil { + return nil, err } + if err := validateStaticAgencyTimezones(data.Static); err != nil { + return nil, fmt.Errorf("invalid GTFS agency timezone: %w", err) + } + return data, nil +} + +// read returns the feed's bytes to import, tidied when gtfstidy is on and +// succeeds, and the hash to store them under. gtfstidy reads the file itself, +// so a tidied feed is never held in memory untidied. +// +// An untidied local feed is hashed again from the bytes read, so the stored +// hash matches the stored data even if the file changed since it was fetched. +// A tidied local feed keeps the fetched hash; if the file changed in between, +// the next reload sees a new hash and imports it again. +func (f *feedFile) read(ctx context.Context, config Config) ([]byte, string, error) { if config.EnableGTFSTidy { logger := slog.Default().With(slog.String("component", "gtfs_loader")) logging.LogOperation(logger, "gtfstidy_enabled_processing_gtfs_data") - tidiedData, err := tidyGTFSData(ctx, b, logger) - if err != nil { - logging.LogError(logger, "Failed to tidy GTFS data, using original data", err) - } else { - b = tidiedData + tidiedData, err := tidyGTFSFile(ctx, f.path, logger) + if err == nil { + return tidiedData, f.hash, nil } + logging.LogError(logger, "Failed to tidy GTFS data, using original data", err) } - data, err := gtfsdb.ParseGtfsDataWithHash(b, hash, config.GtfsURL) + b, err := os.ReadFile(f.path) if err != nil { - return nil, err + return nil, "", fmt.Errorf("error reading GTFS data: %w", err) } - - if err := validateStaticAgencyTimezones(data.Static); err != nil { - return nil, fmt.Errorf("invalid GTFS agency timezone: %w", err) + if f.private { + return b, f.hash, nil } - return data, nil + sum := sha256.Sum256(b) + return b, feedHash(hex.EncodeToString(sum[:]), config), nil } diff --git a/internal/gtfs/tidy.go b/internal/gtfs/tidy.go index b61970beb..a9c0bbdff 100644 --- a/internal/gtfs/tidy.go +++ b/internal/gtfs/tidy.go @@ -50,13 +50,23 @@ func checkGTFSTidyAvailable() (string, error) { return path, nil } -// tidyGTFSData processes GTFS data through gtfstidy with flags -OscRCSmeD -// Returns the tidied GTFS zip data or an error -func tidyGTFSData(ctx context.Context, inputZip []byte, logger *slog.Logger) ([]byte, error) { +// tidyGTFSFile processes the GTFS zip at inputPath through gtfstidy with +// flags -OscRCSmeD. gtfstidy reads the file itself, so the untidied feed is +// never loaded into memory. Returns the tidied GTFS zip data or an error. +func tidyGTFSFile(ctx context.Context, inputPath string, logger *slog.Logger) ([]byte, error) { gtfstidyPath, err := checkGTFSTidyAvailable() if err != nil { return nil, err } + // gtfstidy runs in its own directory, so a relative path would miss. + inputPath, err = filepath.Abs(inputPath) + if err != nil { + return nil, fmt.Errorf("failed to resolve pristine GTFS zip path: %w", err) + } + input, err := os.Stat(inputPath) + if err != nil { + return nil, fmt.Errorf("failed to read pristine GTFS zip: %w", err) + } // Create temp directory for processing // Note: We should clean up the temp dir on error || after processing @@ -71,19 +81,14 @@ func tidyGTFSData(ctx context.Context, inputZip []byte, logger *slog.Logger) ([] } }() - pristineZipPath := filepath.Join(tempDir, "gtfs_pristine.zip") - if err := os.WriteFile(pristineZipPath, inputZip, 0600); err != nil { - return nil, fmt.Errorf("failed to write pristine GTFS zip: %w", err) - } - outputZipPath := filepath.Join(tempDir, "gtfs_tidied.zip") logging.LogOperation(logger, "running_gtfstidy", - slog.String("input_file", pristineZipPath), - slog.Int("input_size_bytes", len(inputZip))) + slog.String("input_file", inputPath), + slog.Int64("input_size_bytes", input.Size())) // -o: output file - cmd := exec.CommandContext(ctx, gtfstidyPath, "-OscRCSmeD", "-o", outputZipPath, pristineZipPath) + cmd := exec.CommandContext(ctx, gtfstidyPath, "-OscRCSmeD", "-o", outputZipPath, inputPath) cmd.Dir = tempDir var stdout, stderr bytes.Buffer @@ -107,7 +112,7 @@ func tidyGTFSData(ctx context.Context, inputZip []byte, logger *slog.Logger) ([] logging.LogOperation(logger, "gtfs_tidied_successfully", slog.Int("output_size_bytes", len(tidiedZip)), - slog.Int("size_reduction_bytes", len(inputZip)-len(tidiedZip))) + slog.Int64("size_reduction_bytes", input.Size()-int64(len(tidiedZip)))) return tidiedZip, nil } diff --git a/internal/gtfs/tidy_test.go b/internal/gtfs/tidy_test.go index 308ff61fd..53aa051c9 100644 --- a/internal/gtfs/tidy_test.go +++ b/internal/gtfs/tidy_test.go @@ -10,7 +10,7 @@ import ( "testing" ) -func TestTidyGTFSData(t *testing.T) { +func TestTidyGTFSFile(t *testing.T) { projectRoot, err := findProjectRoot() if err != nil { @@ -38,9 +38,9 @@ func TestTidyGTFSData(t *testing.T) { logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) // Run tidy - outputData, err := tidyGTFSData(context.Background(), inputData, logger) + outputData, err := tidyGTFSFile(context.Background(), testDataPath, logger) if err != nil { - t.Fatalf("tidyGTFSData failed: %v", err) + t.Fatalf("tidyGTFSFile failed: %v", err) } // Verify output is not empty From bebd526491add872faf52bda79a35b3609725ec4 Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Sat, 7 Nov 2026 10:32:47 -0800 Subject: [PATCH 8/8] Reuse one HTTP client for static downloads Each reload built a new client with a new transport and never closed it, so the keep-alive connection and its goroutines stayed idle for 90 seconds on a transport nothing would use again. A package-level client, like the realtime one, is shared by every reload instead. The download timeouts are now named constants, and the static and realtime clients clone http.DefaultTransport through one cloneDefaultTransport helper. --- internal/gtfs/feed_file.go | 37 +++++++++++++++++++++++++++---------- internal/gtfs/realtime.go | 7 +------ 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/internal/gtfs/feed_file.go b/internal/gtfs/feed_file.go index 7a7f6224b..a81e99721 100644 --- a/internal/gtfs/feed_file.go +++ b/internal/gtfs/feed_file.go @@ -74,7 +74,7 @@ func downloadFeedFile(ctx context.Context, config Config) (*feedFile, error) { req.Header.Set(config.StaticAuthHeaderKey, config.StaticAuthHeaderValue) } - resp, err := newStaticHTTPClient().Do(req) + resp, err := staticHTTPClient.Do(req) if err != nil { return nil, fmt.Errorf("error downloading GTFS data: %w", err) } @@ -109,18 +109,35 @@ func downloadFeedFile(ctx context.Context, config Config) (*feedFile, error) { }, nil } -// newStaticHTTPClient returns a client for downloading a static feed. Like the -// realtime client, its transport is cloned from http.DefaultTransport to keep -// its defaults (proxy from environment, dialer, HTTP/2). +// Timeouts for downloading a static feed. The overall timeout allows for a +// feed of maxStaticFeedBytes on a slow link. +const ( + staticTLSHandshakeTimeout = 10 * time.Second + staticResponseHeaderTimeout = 30 * time.Second + staticIdleConnTimeout = 90 * time.Second + staticDownloadTimeout = 5 * time.Minute +) + +// staticHTTPClient downloads static feeds. It is shared by every reload, so +// an idle connection is reused or closed by its transport rather than left +// behind on one that nothing uses again. +var staticHTTPClient = newStaticHTTPClient() + func newStaticHTTPClient() *http.Client { - transport := &http.Transport{} + transport := cloneDefaultTransport() + transport.TLSHandshakeTimeout = staticTLSHandshakeTimeout + transport.ResponseHeaderTimeout = staticResponseHeaderTimeout + transport.IdleConnTimeout = staticIdleConnTimeout + return &http.Client{Timeout: staticDownloadTimeout, Transport: transport} +} + +// cloneDefaultTransport returns a copy of http.DefaultTransport, to keep its +// defaults (proxy from environment, dialer, HTTP/2, keepalives). +func cloneDefaultTransport() *http.Transport { if t, ok := http.DefaultTransport.(*http.Transport); ok { - transport = t.Clone() + return t.Clone() } - transport.TLSHandshakeTimeout = 10 * time.Second - transport.ResponseHeaderTimeout = 30 * time.Second - transport.IdleConnTimeout = 90 * time.Second - return &http.Client{Timeout: 5 * time.Minute, Transport: transport} + return &http.Transport{} } // Feed downloads are named downloadPrefix, then their owner, then "-" and the diff --git a/internal/gtfs/realtime.go b/internal/gtfs/realtime.go index 5f4c64da5..c51163c49 100644 --- a/internal/gtfs/realtime.go +++ b/internal/gtfs/realtime.go @@ -159,12 +159,7 @@ const maxVehicleFeedFutureSkew = 15 * time.Minute var realtimeHTTPClient = newRealtimeHTTPClient() func newRealtimeHTTPClient() *http.Client { - var transport *http.Transport - if t, ok := http.DefaultTransport.(*http.Transport); ok { - transport = t.Clone() - } else { - transport = &http.Transport{} - } + transport := cloneDefaultTransport() transport.MaxIdleConns = 50 transport.MaxIdleConnsPerHost = 10 transport.IdleConnTimeout = 90 * time.Second