Skip to content

Fix goroutine leak in (*UploadCompleter).ensureSessionEndEvent#42061

Merged
espadolini merged 1 commit intomasterfrom
espadolini/ensuresessionendevent-leak
May 28, 2024
Merged

Fix goroutine leak in (*UploadCompleter).ensureSessionEndEvent#42061
espadolini merged 1 commit intomasterfrom
espadolini/ensuresessionendevent-leak

Conversation

@espadolini
Copy link
Copy Markdown
Contributor

This PR fixes a goroutine leak in the upload completer, which wasn't exhausting the event stream from StreamSessionEvent if a session end event was found. The same sort of leak was also fixed in the failure paths of a handful of tests, and in the code used by tsh play and tsh recordings export (even though it's not a problem for the command line tools, since process exit is the ultimate leak collector anyway).

changelog: fixed resource leak in session recording cleanup

@github-actions github-actions Bot requested review from ryanclark and tcsc May 27, 2024 18:45
@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels May 27, 2024
@espadolini espadolini requested a review from zmb3 May 27, 2024 19:44
@espadolini espadolini added this pull request to the merge queue May 28, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 28, 2024
@espadolini espadolini added this pull request to the merge queue May 28, 2024
Merged via the queue into master with commit e49af3a May 28, 2024
@espadolini espadolini deleted the espadolini/ensuresessionendevent-leak branch May 28, 2024 07:31
@public-teleport-github-review-bot
Copy link
Copy Markdown

@espadolini See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Create PR

Comment thread lib/client/player.go
select {
case evts <- evt:
case <-ctx.Done():
errs <- trace.Wrap(err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trace.Wrap(err) is guaranteed to evaluate to nil here, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, this was meant to be errs <-trace.Wrap(ctx.Err()) 😭

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

Labels

audit-log Issues related to Teleports Audit Log size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants