MmapedDict.__init__ creates the backing file before sizing it, so it exists at 0 bytes for a moment:
self._f=open(filename, 'rb'ifread_modeelse'a+b') # creates at 0 bytescapacity=os.fstat(self._f.fileno()).st_sizeifcapacity==0:
self._f.truncate(_INITIAL_MMAP_SIZE) # sized only here
read_all_values_from_file doesn't guard for that, so _unpack_integer(data, 0) on an empty read raises struct.error and aborts the whole merge — losing every other worker's metrics with it. A worker killed inside that window leaves the empty file behind for good, since it's named after a pid that never returns, so this is not always self-correcting. #1199 has the repro and the numbers.
This guards the empty read and treats it as a file with nothing recorded yet — the read-side mirror of __init__'s own if capacity == 0 branch.
Deliberately narrow:
Non-empty files are unaffected. A file claiming more than it holds still raises, and _read_all_values's RuntimeError('Read beyond file size detected, file is corrupted.') is untouched, so genuine corruption still fails loudly.
Smaller in effect than it looks: a 4-to-8-byte all-zero file already reads as empty today, so this extends existing behaviour to the 0-byte case rather than adding new leniency.
I looked at catching struct.error in _read_metrics instead (too broad — swallows real corruption) and at deleting 0-byte files on read (risky — the file may belong to a live worker holding the fd, and unlinking would orphan its metrics for the rest of its lifetime). Closing the window at source in __init__ via temp-file-then-os.rename seems worth a follow-up, but it's a bigger change and wouldn't heal files already on disk.
Tests
Four cases, mirroring the existing test_missing_gauge_file_during_merge convention:
a stale empty file doesn't suppress other workers' metrics
The three empty-file tests fail on master with the exact struct.error from #1199 and pass with the change; the truncated-file test passes either way by design. Full suite green, flake8 and isort clean.
…1199.
Signed-off-by: Dom Hastings <dominik.hastings@fundingcircle.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
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.
Fixes #1199.
MmapedDict.__init__ creates the backing file before sizing it, so it exists at 0 bytes for a moment:
read_all_values_from_file doesn't guard for that, so _unpack_integer(data, 0) on an empty read raises struct.error and aborts the whole merge — losing every other worker's metrics with it. A worker killed inside that window leaves the empty file behind for good, since it's named after a pid that never returns, so this is not always self-correcting. #1199 has the repro and the numbers.
This guards the empty read and treats it as a file with nothing recorded yet — the read-side mirror of __init__'s own if capacity == 0 branch.
Deliberately narrow:
I looked at catching struct.error in _read_metrics instead (too broad — swallows real corruption) and at deleting 0-byte files on read (risky — the file may belong to a live worker holding the fd, and unlinking would orphan its metrics for the rest of its lifetime). Closing the window at source in __init__ via temp-file-then-os.rename seems worth a follow-up, but it's a bigger change and wouldn't heal files already on disk.
Tests
Four cases, mirroring the existing test_missing_gauge_file_during_merge convention:
The three empty-file tests fail on master with the exact struct.error from #1199 and pass with the change; the truncated-file test passes either way by design. Full suite green, flake8 and isort clean.
@csmarchbanks — flagging you per CONTRIBUTING.md.
LLM use
Please be aware that I used Claude Opus and Sonnet during investigation and resolution.