Conversation
With this caller can cancel any enqueue retries
blocked enqueues are canceled.
|
💻 Deploy preview available (fix(loki.write): Potential deadlock when queue is full and component shuts down): |
kgeckhart
left a comment
There was a problem hiding this comment.
Looks like both PRs still hang on a config update when the queue is full. Mine was supposed to fix it but it did not and goroutine dump on this one shows,
| Goroutine | Parked at | State |
|---|---|---|
fanout run |
client/endpoint.go:67 (bo.Wait()) via client/consumer_fanout.go:68 |
retrying a full queue |
| mutator forwarder | common/loki/entry_handler.go:86 |
blocked on nextChan <- f(e) |
Component.Run |
loki/write/write.go:139 |
blocked on c.sink.Chan() <- entry, holding c.mut.RLock() |
Component.Update |
loki/write/write.go:159 |
blocked on c.mut.Lock() |
Run's inner select only escapes on ctx.Done(), and a reload does not cancel the component context, so the read lock is never released.
| } | ||
|
|
||
| return nil | ||
| return bo.Err() |
There was a problem hiding this comment.
If we got here we dropped data due to a cancellation on the context breaking the bo, right?
There was a problem hiding this comment.
Yeah so if WAL is enabled we dont since we dont update data as sent. But for non WAL we do drop data
31f98e4 to
b374571
Compare
There was a problem hiding this comment.
🟡 Changes recommended
WAL failures become silent and repeated writer shutdown can panic.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Prevents loki.write shutdown and updates from deadlocking when send queues are full.
Changes:
- Replaces channel-based consumers with context-aware
ConsumeEntry. - Adds cancellation-aware WAL draining and synchronous WAL writes.
- Adds shutdown tests and corrects metrics/documented defaults.
File summaries
| File | Description |
|---|---|
internal/component/loki/write/write.go |
Routes entries through the new consumer API. |
internal/component/loki/write/write_test.go |
Tests blocked update and shutdown behavior. |
internal/component/loki/source/api/api_test.go |
Migrates tests to ConsumeEntry. |
internal/component/common/loki/wal/writer.go |
Makes WAL writes synchronous. |
internal/component/common/loki/wal/writer_test.go |
Updates writer tests. |
internal/component/common/loki/wal/watcher.go |
Propagates shutdown cancellation. |
internal/component/common/loki/wal/watcher_test.go |
Updates watcher test implementations. |
internal/component/common/loki/wal/internal/watcher_state.go |
Exposes a stopping context. |
internal/component/common/loki/client/util_test.go |
Adds blocked-consumer test helpers. |
internal/component/common/loki/client/shards.go |
Distinguishes full and stopped queues. |
internal/component/common/loki/client/metrics.go |
Corrects initialized drop reasons. |
internal/component/common/loki/client/endpoint.go |
Makes enqueue cancellation-aware. |
internal/component/common/loki/client/endpoint_test.go |
Tests enqueue cancellation. |
internal/component/common/loki/client/consumer.go |
Defines the function-based consumer API. |
internal/component/common/loki/client/consumer_wal.go |
Adapts WAL consumers to the new API. |
internal/component/common/loki/client/consumer_wal_test.go |
Tests WAL shutdown behavior. |
internal/component/common/loki/client/consumer_fanout.go |
Removes the fanout receive channel. |
internal/component/common/loki/client/consumer_fanout_test.go |
Tests fanout shutdown behavior. |
docs/sources/reference/components/loki/loki.write.md |
Corrects retry and drain-timeout documentation. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Brief description of Pull Request
Be able to shutdown in respect to drain timeouts when
loki.writeis blocked on full queue.Pull Request Details
Before this pr loki.write would not be able to perform updates or shutdown if any entry was blocked being enqueued. So the drain timeouts would not be respected.
To be able to handle both of these cases I had to remove channel usage from both of our consumer clients (WAL and non-WAL). IMO channel usage where the consumer of that channel is also responsible to close that channel is a bad pattern.
So instead I updated to just a plain function call
ConsumeEntry. This one will propagate context from component so we can detect shutdown. We also "snapshot" consumer under read lock before callingConsumeEntryso that we can unblock a potential update call. If update happensloki.ErrConsumerStoppedwill be returned and we have to abandon the entry.Alternative implementation to #7087
Issue(s) fixed by this Pull Request
Related to: #7094
Notes to the Reviewer
First commit in this pr adds 2 test that failed without the fix.
PR Checklist