Skip to content

fix(api): dynamic handler loses request cancellation and trace context#1019

Merged
acouvreur merged 1 commit into
mainfrom
fix/dynamic-handler-request-context
Jul 11, 2026
Merged

fix(api): dynamic handler loses request cancellation and trace context#1019
acouvreur merged 1 commit into
mainfrom
fix/dynamic-handler-request-context

Conversation

@acouvreur

Copy link
Copy Markdown
Member

Finding

The dynamic handler passed the *gin.Context itself where a context.Context is expected:

sessionState, err = s.Sablier.RequestSession(c, request.Names, request.SessionDuration)        // was: c
sessionState, err = s.Sablier.RequestSessionGroup(c, request.Group, request.SessionDuration)   // was: c

while the blocking handler correctly passed c.Request.Context(). Gin only proxies Done()/Err()/Value() through to the underlying request context when Engine.ContextWithFallback is enabled - and it is enabled nowhere in this codebase (verified: zero occurrences). Without it:

  • c.Done() returns a nil channel and c.Err() returns nil - cancellation never propagates. A client that disconnects (or a server shutting down) does not cancel the session-start work started on its behalf.
  • c.Value(...) stops at gin's own keys and never reaches the request context - so the span installed by otelgin.Middleware is invisible downstream. Every provider call under a dynamic request is recorded as an orphan root span.

The asymmetry is the nasty part: tracing and cancellation work on the blocking path and silently do not on the dynamic path - the highest-traffic, user-facing endpoint. Nobody notices until they need the trace that isn't there.

Discovery

Found during an API-layer design review (comparing the two strategy handlers side by side). Pinned with a regression test that stores a sentinel value on the request context and pre-cancels it, then captures the context.Context the session layer receives:

--- FAIL: TestStartDynamicPropagatesRequestContext/names_path
    assertion failed: sentinel (string) != <nil> (<nil>): request context values must reach the session layer
--- FAIL: TestStartDynamicPropagatesRequestContext/group_path
--- PASS: TestStartBlockingPropagatesRequestContext

The blocking handler passing, and the dynamic handler failing, on the identical assertion is the whole bug in one screenful.

Fix

Pass c.Request.Context() at both dynamic call sites, with a comment explaining why the raw gin context must never be used there (so the next handler copies the right pattern).

Tests

TestStartDynamicPropagatesRequestContext (names + group paths) and TestStartBlockingPropagatesRequestContext pin the contract on both handlers: request-context values must be visible to the session layer, and a cancelled request must be observed as cancelled. API package tests and golangci-lint pass.

Docs

No user-facing docs impact (no config, label, or API surface change).

…equests

The dynamic handler passed the *gin.Context itself where a context.Context
is expected, while the blocking handler correctly passed
c.Request.Context(). Gin only proxies Done/Err/Value to the request context
when Engine.ContextWithFallback is enabled, which it is not: the gin context
has a nil Done() channel and its Value() stops at gin keys.

Two silent consequences on the dynamic path (the main user-facing endpoint):
client disconnects and server shutdown never cancelled session-start work,
and the otelgin span was invisible downstream, so every provider call under
a dynamic request was recorded as an orphan root span while blocking-path
traces looked fine.

Regression tests pin the contract on both handlers: a value stored on the
request context must be visible to the session layer, and a cancelled
request context must be observed as cancelled.
@github-actions

Copy link
Copy Markdown

Test Results

✅ All tests passed! | 759 tests in 144.663s

⚠️ 2 test(s) were flaky (failed then passed on rerun)

  • github.com/sablierapp/sablier/pkg/provider/podman/TestPodmanProvider_GetState
  • github.com/sablierapp/sablier/pkg/provider/podman/TestPodmanProvider_GetState/running_container_with_"starting"_health
    View HTML Test Report

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
3.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@github-actions

Copy link
Copy Markdown
┌─────────────────────────────────────────────────────────────────────────────┐
│ Diff between sablier and sablier                                            │
├─────────┬───────────────────────────────────┬──────────┬──────────┬─────────┤
│ PERCENT │ NAME                              │ OLD SIZE │ NEW SIZE │ DIFF    │
├─────────┼───────────────────────────────────┼──────────┼──────────┼─────────┤
│ +0.17%  │ github.com/sablierapp/sablier     │ 654 kB   │ 655 kB   │ +1.1 kB │
│ +0.02%  │ github.com/quic-go/quic-go        │ 1.3 MB   │ 1.4 MB   │ +278 B  │
│ +0.01%  │ github.com/spf13/viper            │ 73 kB    │ 73 kB    │ +7 B    │
│ +0.00%  │ crypto                            │ 1.9 MB   │ 1.9 MB   │ +5 B    │
│ +0.00%  │ github.com/emicklei/go-restful/v3 │ 134 kB   │ 134 kB   │ +1 B    │
│ -0.01%  │ github.com/pmezard/go-difflib     │ 17 kB    │ 17 kB    │ -1 B    │
│ -0.00%  │ k8s.io/client-go                  │ 14 MB    │ 14 MB    │ -16 B   │
│ -0.02%  │ github.com/gin-gonic/gin          │ 337 kB   │ 337 kB   │ -56 B   │
├─────────┼───────────────────────────────────┼──────────┼──────────┼─────────┤
│ -0.00%  │ .rodata                           │ 2.6 MB   │ 2.6 MB   │ -72 B   │
├─────────┼───────────────────────────────────┼──────────┼──────────┼─────────┤
│ +0.00%  │ sablier                           │ 62 MB    │ 62 MB    │ +0 B    │
│         │ sablier                           │          │          │         │
└─────────┴───────────────────────────────────┴──────────┴──────────┴─────────┘

@acouvreur acouvreur merged commit 0fb0079 into main Jul 11, 2026
7 of 8 checks passed
@acouvreur acouvreur deleted the fix/dynamic-handler-request-context branch July 11, 2026 06:42
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