fix: resolve direction calculator caching bugs and prevent stale queries on hot-swap#797
fix: resolve direction calculator caching bugs and prevent stale queries on hot-swap#7973rabiii wants to merge 2 commits intoOneBusAway:mainfrom
Conversation
fletcherw
left a comment
fletcherw
left a comment
There was a problem hiding this comment.
Hi Adel, this is a good improvement to the caching logic. I have some feedback but no large changes are needed.
| adc.queriesMu.Unlock() | ||
|
|
||
| // Evict all cached directions so they are recomputed against the new DB. | ||
| adc.directionResults.Range(func(key, _ any) bool { |
There was a problem hiding this comment.
This can be written more simply as adc.directionResults.Clear().
|
|
||
| // Test with a non-existent stop | ||
| direction := calc.computeFromShapes(ctx, "nonexistent") | ||
| direction, _ := calc.computeFromShapes(ctx, "nonexistent") |
There was a problem hiding this comment.
rather than ignoring errors here and below, you should add assert.NoError(t, err) calls.
| adc.directionResults.Store(stopID, computedDir) | ||
| } | ||
|
|
||
| return computedDir, nil |
There was a problem hiding this comment.
here, even if computeFromShapes returned an err, you're returning computedDir and a nil error. If the intention is to ignore errors, you should leave a comment here explaining why it's safe to do so.
| // a recovered database will be retried on the next request. | ||
| // Lifecycle note: This map grows indefinitely for the lifetime of the application. | ||
| // Unbounded growth is acceptable here because it is strictly bounded by the finite | ||
| // number of valid real-world stops, and computed directions remain stable across GTFS reloads. |
There was a problem hiding this comment.
I think this comment is out-of-date now since the directions can change after reloads. Either don't clear the directions on reload or update the comment.
There was a problem hiding this comment.
is there an easy way you can add a test for the new behavior you've added, verifying that a returned error value isn't cached?
|
|
||
| dbConfig := newGTFSDBConfig(finalDBPath, manager.config) | ||
| if reopenedClient, reopenErr := gtfsdb.NewClient(dbConfig); reopenErr == nil { | ||
| manager.GtfsDB = reopenedClient |
There was a problem hiding this comment.
do you need to call UpdateQueries here if the previous client was closed and then reopened?
2 participants
Description
This PR addresses several reliability and correctness bugs within the
AdvancedDirectionCalculator. Previously, transient database errors could permanently poison the direction cache, insufficient shape points defaulted to an invalid "East" orientation, and the calculator retained a stale database pointer after a GTFS hot-swap.Changes Included
computeFromShapesnow returns(string, error). The loop tracks transient DB errors (lastTransientErr) and propagates them to the caller, ensuring we only cache valid directions or legitimate "no data" states, avoiding permanent cache poisoning during DB hiccups.sql.ErrNoRowswhen there are fewer than 2 shape points. This prevents the caller from incorrectly interpreting anilerror as a valid0radian (East) orientation.slog.Warnfor non-ErrNoRowserrors in the orientation calculation loop to surface silent failures like DB connection drops or timeouts.sync.RWMutexto protect thequeriespointer inAdvancedDirectionCalculator.UpdateQueries()method to atomically swap the DB pointer and safely evict the entire direction result cache.gtfs.Managerduring startup so thatForceUpdatecan automatically trigger the refresh after a successful GTFS reload.Verification
fixes: #747