Skip to content

fix: propagate ctx to gtfstidy subprocess#802

Open
ashnaaseth2325-oss wants to merge 1 commit intoOneBusAway:mainfrom
ashnaaseth2325-oss:fix/tidy-exec-context
Open

fix: propagate ctx to gtfstidy subprocess#802
ashnaaseth2325-oss wants to merge 1 commit intoOneBusAway:mainfrom
ashnaaseth2325-oss:fix/tidy-exec-context

Conversation

@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor

SUMMARY

This PR fixes a context propagation issue in the GTFS tidy pipeline where subprocesses could not be canceled, leading to hangs during shutdown or timeout. It updates tidyGTFSData to accept a context.Context and uses exec.CommandContext instead of exec.Command.

Primary changes are in internal/gtfs/tidy.go, with call-site updates in internal/gtfs/static.go and test adjustments in internal/gtfs/tidy_test.go.


FIX

// Before
func tidyGTFSData(inputZip []byte, logger *slog.Logger) ([]byte, error) {
    cmd := exec.Command(gtfstidyPath, "-OscRCSmeD", "-o", outputZipPath, pristineZipPath)
}

// After
func tidyGTFSData(ctx context.Context, inputZip []byte, logger *slog.Logger) ([]byte, error) {
    cmd := exec.CommandContext(ctx, gtfstidyPath, "-OscRCSmeD", "-o", outputZipPath, pristineZipPath)
}

VERIFICATION

Tested by enabling GTFS tidy, triggering a feed update, and sending a SIGTERM while the tidy process was running. The service now shuts down cleanly without hanging, and the subprocess exits immediately on cancellation. Builds and existing tests pass with no regressions.

Use exec.CommandContext so the gtfstidy process is killed when the
caller's context is cancelled or times out, preventing graceful
shutdown from blocking indefinitely.

Signed-off-by: ashnaaseth2325-oss <ashnaaseth2325@gmail.com>
@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor Author

Hi @aaronbrethorst
Kindly you please review the PR.
Thanks!

@ashnaaseth2325-oss
Copy link
Copy Markdown
Contributor Author

Hello @aaronbrethorst
Just a quick follow-up. Would appreciate your feedback when you have a moment.
Thanks!

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.

1 participant