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 9 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.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.
There was a problem hiding this comment.
This is intentional so we gracefully fallback to an empty direction "" without failing the API. Returning nil also ensures singleflight correctly shares this fallback with concurrent callers. I've added a comment to clarify this. Thanks!
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?
There was a problem hiding this comment.
Sure! I've added TestTransientDBError_NotCached. It simulates a db failure by closing an in-memory database and verifies that the resulting empty string is not cached
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