fix: stop recording ErrCompletedByStrategy as a span error (closes #4109) - #4597
fix: stop recording ErrCompletedByStrategy as a span error (closes #4109)#4597pacocartones wants to merge 2 commits into
Conversation
The login handlers deliberately swallow flow.ErrCompletedByStrategy (a control signal, not a failure) from the client response, but left the tracked err set, so the deferred otelx.End(span, &err) recorded the control signal as a span error/exception -- a completed flow shows up in telemetry as an error. Clear the tracked err in the two handlers where it genuinely carries the signal (createNativeLoginFlow, createBrowserLoginFlow). Client behavior and API are unchanged. Adds a test asserting the span is not errored on strategy completion. Closes ory#4109 Signed-off-by: pacocartones <pacocartones@users.noreply.github.com>
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (44)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughSelf-service flow strategies now return explicit completion signals instead of ChangesCompletion signal migration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The completion-control refactor preserves completed-flow handling without treating it as an error, with no active merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for the contribution. The intent is good, but there are two issues:
Thus a better fix is to make this control value not an error, but instead an additional return value. |
…, not an error
flow.ErrCompletedByStrategy was a control signal ("the strategy already
wrote the response, stop processing"), not a failure. Because it was
modelled as an error and returned through functions whose deferred
otelx.End(span, &err) tracked that err, a completed flow was recorded as
a span error/exception -- most visibly on a normal OIDC login, which
produced several false-positive errored spans (ory#4109). Clearing the err
at a single handler only patched one of many sites.
Remove the sentinel entirely and thread completion as an additional
return value across the strategy interfaces and their callers:
- login.Strategy.Login and FastLoginStrategy, registration.Strategy.Register,
recovery.Strategy.Recover, verification.Strategy.Verify and
settings.Strategy.Settings now return a `completed bool`.
- Handler.NewLoginFlow returns `completed bool`; the login/registration/
recovery/verification/settings handlers and the login hook consume it.
- Strategy implementations (code, oidc, webauthn, passkey, idfirst, profile,
password, totp, lookup, link) return the bool from their completion paths.
- Where a strategy's internal call graph is deep (oidc HandleError account
linking; the code and link recovery/verification chains), an unexported
package-local sentinel carries completion internally and is translated to
the bool at the package's exported entry point, so completion never crosses
the interface as an error.
Deleting flow.ErrCompletedByStrategy makes the compiler enforce that every
producer, consumer and comparison site is converted. A completed flow can no
longer be recorded as a span error at any site.
Closes ory#4109
Signed-off-by: pacocartones <pacocartones@users.noreply.github.com>
|
Thanks @gaultier — both points addressed. I removed On "not all sites": the original change only cleared the two login create-handler spans. The errored spans in your screenshot actually came from the internal OIDC strategy spans (
It's a broad change — it touches every self-service flow (44 files) — because the sentinel deletion is the only thing that unambiguously closes "not all sites". If you'd rather review it incrementally, it splits cleanly per flow (login first, then registration / recovery / verification / settings) — just say the word and I'll break it up. |
pacocartones
changed the title
3 participants
Closes #4109.
The bug
ErrCompletedByStrategyis a control-flow signal (selfservice/flow/error.go), not a failure. The login handlers correctly suppress the client error for it (if !errors.Is(err, flow.ErrCompletedByStrategy) { WriteError }), but leave the trackederrset, so the deferredotelx.End(span, &err)records the signal as a span error/exception — a completed flow appears in telemetry as an error.The fix
Clear the tracked
errwhere it genuinely carries the signal, in the two handlers whose deferredotelx.End(span, &err)tracks it:createNativeLoginFlow(the reported one) andcreateBrowserLoginFlow. Client behavior and API are unchanged.Scope note (checked, to avoid a no-op): the other update handlers listed around this issue are not affected —
updateLoginFlow/updateSettingsFlowuse a loop-shadowederrthe deferredotelx.Enddoesn't track, and the registration/recovery/verification update handlers have nootelx.End(span, &err). So the fix is these two sites.Test
New
handler_completed_by_strategy_test.go: installs atracetestspan recorder as the registry tracer, forces a strategy to returnErrCompletedByStrategy, drives the real API endpoint, and asserts thecreateNativeLoginFlowspan is notcodes.Errorand carries no exception event. Passes; reverting only the fix makes it fail with span status"flow response completed by strategy". (Focused test via in-memory mocks; the full login package's DB-backed suites need dockertest.)Disclosure: this contribution was prepared with AI assistance (Claude Code); the fix, scope, and test were verified against HEAD.
Summary by CodeRabbit
Bug Fixes
Tests