Skip to content

events: fix goroutine leak when using --until with dropped contexts - #29499

Open
KHARSHAVARDHAN-eng wants to merge 2 commits into
podman-container-tools:mainfrom
KHARSHAVARDHAN-eng:fix-29491-event-logger-goroutine-leak
Open

events: fix goroutine leak when using --until with dropped contexts#29499
KHARSHAVARDHAN-eng wants to merge 2 commits into
podman-container-tools:mainfrom
KHARSHAVARDHAN-eng:fix-29491-event-logger-goroutine-leak

Conversation

@KHARSHAVARDHAN-eng

@KHARSHAVARDHAN-eng KHARSHAVARDHAN-eng commented Aug 13, 2026

Copy link
Copy Markdown

Fixes #29491

When fetching events with --until on a system using the file events backend, libpod/events/logfile.go spawned an unmanaged goroutine running time.Sleep(time.Until(untilTime)). Because time.Sleep ignores context.Context cancellation, early client disconnects (or Ctrl+C) caused the sleeping goroutine to remain alive for the full until duration.

Changes

  • Replaced time.Sleep with a time.NewTimer and select statement listening on ctx.Done(), ensuring the wait exits immediately when the context is cancelled.
  • Removed the redundant TestReadUntilContextCancelled test as requested during review.

When fetching events with the --until flag on a system using the file events
backend, libpod/events/logfile.go spawned an unmanaged goroutine running
time.Sleep(time.Until(untilTime)). Because time.Sleep is not context-aware,
if a client cancelled the request or dropped the connection, the goroutine
remained sleeping in the background for the full until duration.

Fix this by using time.NewTimer and selecting on ctx.Done() so the background
goroutine exits immediately when the context is cancelled.

Fixes: podman-container-tools#29491
Signed-off-by: Harsha Vardhan <harshahvk2005@gmail.com>
Comment thread libpod/events/logfile_test.go Outdated
require.Equal(t, beforeRename, afterRename)
}

func TestReadUntilContextCancelled(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This unit test doesn't add value, please remove

@mheon mheon added the No New Tests Allow PR to proceed without adding regression tests label Aug 13, 2026

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, once @mheon comment is resolved.

@KHARSHAVARDHAN-eng

Copy link
Copy Markdown
Author

Thanks @mheon. I've removed the redundant TestReadUntilContextCancelled test and its unused imports. The production fix remains unchanged, and I've verified the event package builds and passes linting. Please take another look when you have a chance.

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just last thing put in to commit msg Fixes: https://github.com/podman-container-tools/podman/issues/29491

Remove the redundant context cancellation test as requested during review.

Fixes: podman-container-tools#29491

Signed-off-by: Harsha Vardhan <harshahvk2005@gmail.com>
@KHARSHAVARDHAN-eng
KHARSHAVARDHAN-eng force-pushed the fix-29491-event-logger-goroutine-leak branch from e1e4f01 to f7caec8 Compare August 17, 2026 11:14
@KHARSHAVARDHAN-eng

Copy link
Copy Markdown
Author

Hi @Honny1,

I’ve addressed the review feedback by removing the redundant TestReadUntilContextCancelled test and updated the PR description to reflect the final changes.

The changes are now pushed and the CI checks are running. Once CI completes successfully, this should be ready for merge.

Thanks for the review!

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

PTAL @podman-container-tools/podman-reviewers @podman-container-tools/podman-maintainers

@Honny1

Honny1 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

@KHARSHAVARDHAN-eng PR needs a second review.

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

Labels

No New Tests Allow PR to proceed without adding regression tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Goroutine leak in event logger when using --until with dropped contexts

3 participants