Repository navigation
fix(buckyd): backfill a metric that appeared mid-copy - #58
Merged
Merged
Conversation
healMetric decides between backfilling and creating from a stat of the destination, then downloads or dumps the body before it writes. On a host running go-carbon the metric can be created in that window, and go-whisper's CreateWithOptions refuses to overwrite an existing file, so CompressTo returns os.ErrExist and the copy fails with a 500 whose body is the bare "file already exists" -- rebalance then retries the same losing race. Treat that as what it is: the destination exists after all. The dump on disk is exactly what the backfill path wants, so fill into the file that appeared instead of failing. Stats report it under Fill rather than Compress, which is also how the fallback shows up in a response. The raw-copy branch below has the same race with a quieter outcome -- os.Create truncates whatever go-carbon just wrote -- and is left alone; it streams the body straight to the destination and has no dump to fill from.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is small and targeted with a regression test; only a minor improvement to error wrapping remains.
Pull request overview
This PR hardens buckyd’s metric-healing copy path against a race where the destination whisper file is created mid-copy (by go-carbon), causing CompressTo to fail with os.ErrExist and the request to 500/retry unnecessarily. It introduces a fallback that backfills into the newly-created destination file instead of treating the race as a hard error, and adds a regression test.
Changes:
- Refactors the “dump then compress” branch in
healMetricto use a newcompressMetrichelper that falls back tofill.All(...)onos.ErrExist. - Records fallback time under
Fill(rather thanCompress) to make the behavior visible in response stats. - Adds
TestCompressMetricFillsFileThatAppearedto validate the race fallback and data arrival.
File summaries
| File | Description |
|---|---|
| cmd/buckyd/metrics.go | Adds compressMetric helper and uses it from healMetric to backfill instead of failing when the destination appears mid-compress. |
| cmd/buckyd/compress_test.go | Adds a regression test that pre-creates a compressed destination and asserts compressMetric performs the backfill fallback and preserves points. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
timtofan
self-requested a review
September 4, 2026 12:24
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
timtofan
approved these changes
Sep 4, 2026
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
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.
A rebalance was failing with:
healMetricpicks between backfilling and creating from a stat of the destination, then downloads or dumps the body before writing. On a host running go-carbon the metric can be created in that window, and go-whisper'sCreateWithOptionsrefuses to overwrite an existing file — soCompressToreturns a bareos.ErrExist, the POST 500s, and rebalance retries the same losing race.The destination does exist after all, and the dump on disk is exactly what the backfill path wants, so fill into the file that appeared instead of failing. The fallback reports its time under
Fillrather thanCompress, so it is visible in the response stats.Not related to the out-of-order sidecar work: that only touched GET and DELETE, and every error it returns is wrapped with a prefix.
Left alone: the raw-copy branch below has the same race with a quieter outcome (
os.Createtruncates what go-carbon just wrote). It streams the body straight to the destination and has no dump to fill from.Test:
TestCompressMetricFillsFileThatAppearedpre-creates the compressed destination, then asserts the fallback ran and both source points arrived.go test ./cmd/buckyd/passes;cmd/bucky-pickle-relayfails on master already (missingtesting/data/gentests.pyfixtures).