aws / aws/aws-logging-dotnet

Make StartMonitor() idempotent — a second call starts a duplicate monitor that races the shared queue and drops events

Open Beginner friendly
#376 2 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C#
Stars
307
Forks
135
Avg merge
1d 10h
Merged PRs (30d)
4

Description

### Summary

`AWSLoggerCore.StartMonitor()` is `public`, but the constructor **already** calls it. Calling `StartMonitor()` again after construction starts a **second** background monitor task, and both monitors then drain the **same** shared `_pendingMessageQueue`/`_repo` concurrently. Because that state isn't guarded for concurrent drainers, the race can silently drop log events — typically the tail of a burst.

This is an easy footgun: the method is public and looks like required setup, so consumers reasonably call it after `new AWSLoggerCore(...)`. (It's exactly what bit the consumer that led to #372 — with a single, constructor-started monitor there is no loss; the double-start was the actual cause. Closing that issue surfaced this one.)

### Why it's easy to trip into

`StartMonitor()` also re-assigns `_flushTriggerEvent`, `_flushCompletedEvent`, and `_cancelStartSource`, then `Task.Run`s another `Monitor(...)`. So a second call:
- leaves the first monitor running on an orphaned `_cancelStartSource` (the field now points at the new one, so `Close()`/`Cancel()` only stops the second monitor — the first never stops), and
- has two monitors concurrently `TryDequeue`-ing the shared queue and mutating/`Reset()`-ing the shared `_repo` batch.

### Repro

```csharp
var config = new AWSLoggerConfig("/repro/double-start") { BatchPushInterval = TimeSpan.FromSeconds(1) };
var logger = new AWSLoggerCore(config, "repro");
logger.StartMonitor(); // <-- constructor already started one; this starts a SECOND
for (int i = 0; i < 11; i++)
logger.AddMessage($"{i:D4}:" + new string('D', 251_000)); // ~251 KB each
logger.Close();
// Read the group back: ~10 of 11 events land, a near-tail event is missing (varies run to run).
```

Removing the second `StartMonitor()` call → all 11 delivered, every run.

### Suggested fixes (any of these)

1. **Make `StartMonitor()` idempotent** — no-op if the monitor is already running (guard with an `Interlocked` flag), so a redundant call can't spawn a second monitor. Lowest-risk, fully backward compatible.
2. If keeping it non-idempotent, at least guard the shared queue/batch so concurrent drainers can't lose events, and ensure a restart cancels the prior monitor.
3. **Documentation** — clearly state that the constructor starts the monitor and consumers must not call `StartMonitor()` (and/or consider making it non-public). Weakest option since the public method remains a trap.

Option 1 seems best: it's a one-line guard that eliminates the whole class of consumer bug without changing correct-usage behavior.

Happy to send a PR if that's welcome.

Contributor guide

Open the contributing guide

Research direction

Start at AWSLoggerCore.StartMonitor() and inspect how the constructor initializes it, how Monitor(...) uses the shared queue, and how Close()/Cancel() stop it. Run the provided double-start reproduction, then make repeated StartMonitor() calls safe and verify that all 11 events are delivered without duplicate monitors or dropped events.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
observability-sre
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.