Make StartMonitor() idempotent — a second call starts a duplicate monitor that races the shared queue and drops events
- 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
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