Conversation
Send() issued two separate Write() calls (data, then separator). With more than one decode worker calling Send() concurrently (collector's `workers` listen option), another goroutine's write can land between them, dropping the separator for that record and merging it with the next one on disk. Observed in production: a file transport running with workers=16 produced records where a fully valid new FlowMessage started at the exact byte offset a separator should have been, corrupting length-prefixed framing for any binary-format consumer built on top of it. Reducing to workers=1 eliminated the corruption immediately (confirmed clean across 1.3M+ decoded records), which pointed at a write race rather than a decode/encode bug. Fix: build the record+separator into one buffer and issue a single Write() call, which is atomic with respect to other writers on a regular file opened O_APPEND. No behavior change when a separator isn't configured.
… the read Send() took RLock() only long enough to snapshot d.w, then released it before calling Write(). Init()'s SIGHUP-triggered reload goroutine takes the full Lock() to Close() the current file and open a new one (used for log rotation, e.g. `kill -1` after mv'ing the file aside). If a reopen happened between Send()'s snapshot and its Write() call, the write would fail with "file already closed" - losing whichever record was in flight at that exact moment. Observed in production: 6,498 occurrences over 8 days, clustered exactly at every 15-minute rotation boundary. Fix: hold the RLock for the duration of the write (defer RUnlock instead of releasing right after the read). Concurrent Send() calls still proceed in parallel since RLock is shared; the reopen's exclusive Lock() now blocks until in-flight writes finish, and any Send() starting after a completed reopen correctly sees the new d.w. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
So I've been tracking an issue when I point production traffic to a goflow2 server writing protobuf directly to disk (double-writes with Kafka will speed up the death of my SSDs). To give an idea of my traffic levels, the protobuf file for 15 minutes is about 2.1GB (at peak to be fair). I've been tracking an issue where the files get corrupted when traffic levels climb. I was performing the analysis / reading in rust which I was learning at the time (https://github.com/dmcken/goflow2_reader_rs) so I honestly for quite a while thought the issue was my code. With a hex editor I could find end of lines in the protobuf file in the wrong locations which gave the initial hints as to what was going on (the records all fall within a certain range of lengths).
Weirdly enough JSON was fine, so as a temporary fix I was processing JSON from /dev/shm (to avoid massacring my disks) and then turning that into a binary format, hence the gaps before this PR. As Go isn't a language I'm that familiar with I've been letting Claude analyze a running server for the past week and it found the below issue after confirming I wasn't going crazy. Another fix that worked was dropping to workers=1 which removed the concurrency problem (but creates a max throughput problem, with this fix in place I can scale to multiple workers).
At this point I'm not sure what the AI policy of this project is, this PR was written with the help of Claude. I can't easily post corrupted files here as I can't anonymize them without being able to read them consistently. Let me know of any issues / tests you want to see and I'll try to accommodate to my best ability, the fix looks pretty simple so if its just simpler for a maintainer to re-write it I'm perfectly fine with that (probably better written by someone who knows Go better than I do), I just want a goflow2 that can handle the load I'm throwing at it.
Claude summary of issue and fix:
Send() issued two separate Write() calls (data, then separator). With more than one decode worker calling Send() concurrently (collector's
workerslisten option), another goroutine's write can land between them, dropping the separator for that record and merging it with the next one on disk.Observed in production: a file transport running with workers=16 produced records where a fully valid new FlowMessage started at the exact byte offset a separator should have been, corrupting length-prefixed framing for any binary-format consumer built on top of it. Reducing to workers=1 eliminated the corruption immediately (confirmed clean across 1.3M+ decoded records), which pointed at a write race rather than a decode/encode bug.
Fix: build the record+separator into one buffer and issue a single Write() call, which is atomic with respect to other writers on a regular file opened O_APPEND. No behavior change when a separator isn't configured.