Conversation
Signed-off-by: Effi-S <effi.szt@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2134
Document severity: MEDIUM · Verified priority: Low · Effort: Low · Type: Task
Old issue reference
[RUN]utils/view/view.goIntent
RunViewintoken/services/utils/view/view.gospawns a goroutine with nowait mechanism, no stop channel, and no way for the caller to know when it
completes. If the goroutine holds open DB transactions, network sessions, or
traces, those resources are leaked until the goroutine eventually finishes
(or never does if the underlying view blocks).
Expected Outcomes
the result).
they are properly closed.
Todo List
<-chan errorfromRunViewso callers can optionally wait.nilor the error) when the goroutine finishes.context.Contextparameter and propagatecancellation into the view execution.
RunViewcan handle the returned channel(they may simply ignore it if fire-and-forget is intentional).
Relevant Context
token/services/utils/view/view.goRunViewfunction.Status in current code
By-design fire-and-forget — the code is as described, but the behaviour is the function's documented contract rather than a defect. One incidental dead-code nit noted below.
Confirmed unchanged at
token/services/utils/view/view.go:46-63. The goroutine isreal and nothing waits on it.
Why this is not a leak to fix here. Fire-and-forget is the stated contract, not an
oversight. The doc comment at
:45reads "RunView runs passed view within the passed Contextand using the passed options in a separate goroutine" — running detached is the entire
purpose of the function, and callers that want a result already have
context.RunViewor theRunViewWithTimeoutwrapper at:30. The goroutine is also not unsupervised in the way thedocument implies: the inner
defer recover()at:53-57catches panics from the view andlogs them, and
:59-61logs any returned error. So it terminates and reports on both thesuccess and failure paths.
The failure mode the document describes — held-open DB transactions, sessions or spans —
requires a view that blocks forever. That is a defect in that view, and the fix belongs
there; making
RunViewreturn a channel does not stop a blocked view from blocking, it onlygives a caller the option to block alongside it. Todo item 4 concedes the point ("they may
simply ignore it if fire-and-forget is intentional").
Changing the signature would also touch every call site to satisfy linters about the discarded
channel, for no behavioural gain in the fire-and-forget case that is the norm.
One genuine nit if this file is edited for another reason. The outer deferred
recover()at
:47-51is dead code. It guards only thegostatement, which cannot panic; the closureruns on a different goroutine, so a panic inside the view can never unwind through this
defer. The inner recovery at:53-57is the one actually doing the work. The outer block isharmless but misleading to a reader who assumes it provides real protection.
Recommended action: close as working-as-intended, or keep at Low solely to carry the
dead-
defercleanup.Severity and priority
The source document rated this MEDIUM. Re-checked against this repository at
origin/mainwithgo 1.26.5on 2026-08-04, the priority is Low for thereasons in Status in current code above.
Effort
Low