fix: panic that happens when a target gets deleted when using decompression by kalleep · Pull Request #3475 · grafana/alloy

kalleep · 2025-04-30T10:29:48Z

PR Description

Alternative to #3452. In this pr I am taking a different approach.

Instead of having to call a Stop function on readers we instead accept a context.Context in Run. Run will now block until context is canceled or if it stopped for any other reason, for decompressor this happens when the whole file is read.
Run is also now responsible for cleanups and we don't have to use any locks or check for nil channels. This is all done as an attempt to simplify the logic a bit.

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
Config converters updated

kalleep · 2025-04-30T10:35:15Z

internal/component/loki/source/file/runner.go

-
 	for {
-		r.reader.Run()
+		r.reader.Run(ctx)


This part is a bit problematic when using decompressor and potentially when using tailer. We will retry the until the target disappear. But when using decompressor we don't tail a file but read it fully until its finished. I wonder if we should quit them there then so we don't keep them around because its not necessary.

I can think of a couple of ways to do this, either set MaxRetries so we don't do this forever or return some kind of result from Run that can indicate if we should keep going or stop

I thought that we read the file after initial_delay has elapsed?

Yes it happens here

alloy/internal/component/loki/source/file/decompresser.go

Lines 215 to 218 in 4774ea5

if d.cfg.InitialDelay > 0 {

level.Info(d.logger).Log("msg", "sleeping before starting decompression", "path", d.path, "duration", d.cfg.InitialDelay.String())

time.Sleep(d.cfg.InitialDelay)

}

.

The issue is that when Run is done when using decompressor there is no more to read from the file but we still keep the task around

ptodev

Thank you, this is definitely a step in the right direction!

ptodev · 2025-04-30T11:38:52Z

internal/component/loki/source/file/runner.go

-
 	for {
-		r.reader.Run()
+		r.reader.Run(ctx)


I thought that we read the file after initial_delay has elapsed?

ptodev · 2025-04-30T11:50:24Z

internal/component/loki/source/file/decompresser.go

-	go d.updatePosition()

 	d.metrics.filesActive.Add(1.)
+	go func() {


Do we want to have a waitgroup to make sure all those goroutines stop before Run returns?

Not really necessary, stop function for both components only returns when both of these are done e.g. https://github.com/grafana/alloy/pull/3475/files#diff-fd87f4f66dac7e8ca37dcaa131b3cb5a0c3498347d5c5309fe0bba0b50492576R380-R383

ptodev · 2025-04-30T11:52:11Z

internal/component/loki/source/file/tailer.go

@@ -47,12 +48,10 @@ type tailer struct {

 	componentStopping func() bool


Does this function still work? I thought it'd lock mut and return stopping?

This one is passed by loki.source.file component:

https://github.com/grafana/alloy/blob/main/internal/component/loki/source/file/file.go#L293

https://github.com/grafana/alloy/blob/main/internal/component/loki/source/file/file.go#L315

It is then used by both readers to determine what do do when stopping:
https://github.com/grafana/alloy/blob/main/internal/component/loki/source/file/tailer.go#L392

So yes this till works

ptodev · 2025-04-30T12:05:05Z

internal/component/loki/source/file/tailer.go

+	posquit chan struct{} // used by the readLine method to tell the updatePosition method to stop
+	posdone chan struct{} // used by the updatePosition method to notify when it stopped
+	done    chan struct{} // used by the readLine method to notify when it stopped


I wonder if all these channels have to be struct fields and whether all the goroutines have to be started in Run... it is harder to see what depends on what this way. WDYT about e.g. readLines starting the updatePosition goroutine and closing it before it closes?

I was experimenting a bit with this before, not having them as struct fields but we have to pass them around.

I made it so that both readers works the same.

Run starts both updatePosition and readlines. When it stops either by it self or by context being canceled we stop readlines and wait for it. That one will close posquit that in turn.

I can try to think if there is a better way to structure this

dd1f2e7 is an attempt to do this, wdyt?

decompressor

…ession (#3475) * Fix panic caused that can happen when when file is removed for decompressor * Change to start readLine start and stops updatePositions * Add changelog

* fix: panic that happens when a target gets deleted when using decompression (#3475) * Fix panic caused that can happen when when file is removed for decompressor * Change to start readLine start and stops updatePositions * Add changelog * Fix mimir.rules.kubernetes panic on non-leader debug info retrieval (#3451) * Fix mimir.rules.kubernetes to only return eventProcessor state if it exists * fix: deadlock in loki.source.file when target is removed (#3488) * Fix deadlock that can happen when stopping reader tasks Co-authored-by: William Dumont <william.dumont@grafana.com> * fix: emit valid logfmt key (#3495) * Fix log keys to be valid for logfmt * Add changelog * Fix streams limit error check so that metrics are correctly labeled as `ReasonStreamLimited` (#3466) * fix: replace direct error string compare with isErrMaxStreamsLimitExceeded helper * update CHANGELOG * Make errMaxStreamsLimitExceeded an error type --------- Co-authored-by: Théo Brigitte <theo.brigitte@gmail.com> Co-authored-by: William Dumont <william.dumont@grafana.com> Co-authored-by: Marat Khvostov <marathvostov@gmail.com>

…ession (#3475) * Fix panic caused that can happen when when file is removed for decompressor * Change to start readLine start and stops updatePositions * Add changelog

kalleep requested a review from a team as a code owner April 30, 2025 10:29

kalleep commented Apr 30, 2025

View reviewed changes

ptodev reviewed Apr 30, 2025

View reviewed changes

kalleep mentioned this pull request Apr 30, 2025

fix: panic that happens when a target gets deleted when using decompression #3452

Closed

4 tasks

kalleep added 5 commits April 30, 2025 15:18

Fix panic caused that can happen when when file is removed for

7df5e87

decompressor

fix

dfc3142

Add changelog

7878681

lint

fa65606

Change to start readLine start and stops updatePositions

dd1f2e7

kalleep force-pushed the kalleep/fix-loki-file-panic-2 branch from 8e5e4b4 to dd1f2e7 Compare April 30, 2025 13:18

Update decompresser.go

899bee2

wildum approved these changes May 2, 2025

View reviewed changes

kalleep merged commit 85718e8 into main May 2, 2025
38 checks passed

kalleep deleted the kalleep/fix-loki-file-panic-2 branch May 2, 2025 13:22

kalleep mentioned this pull request May 2, 2025

Alloy 1.8.1 - loki.source.file / local.file_match - decompressor gz - Panic #3415

Closed

kalleep mentioned this pull request May 5, 2025

Prepare patch release v1.8.3 on release branch #3497

Merged

github-actions bot added the frozen-due-to-age label Jun 2, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jun 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: panic that happens when a target gets deleted when using decompression#3475

fix: panic that happens when a target gets deleted when using decompression#3475
kalleep merged 6 commits intomainfrom
kalleep/fix-loki-file-panic-2

kalleep commented Apr 30, 2025 •

edited

Loading

Uh oh!

kalleep Apr 30, 2025

Uh oh!

ptodev Apr 30, 2025

Uh oh!

kalleep Apr 30, 2025

Uh oh!

ptodev left a comment

Uh oh!

ptodev Apr 30, 2025

Uh oh!

ptodev Apr 30, 2025

Uh oh!

kalleep Apr 30, 2025 •

edited

Loading

Uh oh!

ptodev Apr 30, 2025

Uh oh!

kalleep Apr 30, 2025

Uh oh!

ptodev Apr 30, 2025

Uh oh!

kalleep Apr 30, 2025

Uh oh!

kalleep Apr 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if d.cfg.InitialDelay > 0 {
	level.Info(d.logger).Log("msg", "sleeping before starting decompression", "path", d.path, "duration", d.cfg.InitialDelay.String())
	time.Sleep(d.cfg.InitialDelay)
	}

		@@ -47,12 +48,10 @@ type tailer struct {

		componentStopping func() bool

Conversation

kalleep commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptodev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalleep Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalleep Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kalleep commented Apr 30, 2025 •

edited

Loading

kalleep Apr 30, 2025 •

edited

Loading

kalleep Apr 30, 2025 •

edited

Loading