Skip to content

fix(gtfs): honor context cancellation and fix swallowed errors in Adv…#813

Open
tejasva-vardhan wants to merge 1 commit intoOneBusAway:mainfrom
tejasva-vardhan:fix/advanced-direction-context
Open

fix(gtfs): honor context cancellation and fix swallowed errors in Adv…#813
tejasva-vardhan wants to merge 1 commit intoOneBusAway:mainfrom
tejasva-vardhan:fix/advanced-direction-context

Conversation

@tejasva-vardhan
Copy link
Copy Markdown
Contributor

Description

Fixes #812

The Problem
While auditing context propagation across the GtfsManager, I found a resource leak and a swallowed error bug in AdvancedDirectionCalculator.CalculateStopDirection:

  1. Ignored Cancellation: The code was wrapping the request context with context.WithoutCancel(ctx) before passing it to computeFromShapes. Because computeFromShapes executes multiple SQLite queries (GetStopsWithShapeContext, GetShapePointsWithDistance), closing the HTTP connection or hitting a timeout would not cancel the underlying DB work. Under load, these abandoned requests would continue to burn CPU and hold database connections.
  2. Swallowed Errors in Singleflight: The singleflight.Group.Do callback was implicitly returning a nil error even if computeFromShapes failed. This meant errors (including standard DB timeouts) were discarded, and the singleflight waiters would receive empty strings instead of the actual error.

The Solution

  • Removed context.WithoutCancel(ctx) to ensure SQLite queries correctly honor client cancellation and server shutdowns.
  • Updated the singleflight.Group.Do callback to return ("", err) when the underlying computation fails, allowing the caller to see the actual error.
  • Added an explicit error check after requestGroup.Do to return an empty string gracefully on failure, matching the existing outward behavior but preventing cache poisoning on transient errors.

Behavioral Comparison

  • Before: If a client drops their connection, the SQLite computeFromShapes query keeps running in the background indefinitely. If the database times out or fails, the singleflight group swallows the error and silently caches/returns an empty string "".
  • After: If a client drops their connection, the ctx cancellation propagates down and instantly kills the SQLite query, freeing the DB lock. If an error occurs, it correctly bubbles up from the singleflight group so it can be handled properly without poisoning the cache.

Architectural Note / Tradeoff

By honoring ctx cancellation inside singleflight, there is a known tradeoff: if the "winning" request that initiated the computation is canceled, other concurrent requests waiting on that same stopID in the singleflight group will also receive the context.Canceled error. Given that this is an HTTP server, favoring resource reclamation (canceling the DB queries) over guaranteeing the background work finishes is the preferred Go idiom here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(gtfs): context cancellation ignored and errors swallowed in AdvancedDirectionCalculator

1 participant