Return empty metrics if the .db file has no data - #1200
Open
dom111 wants to merge 1 commit into
Open
Conversation
…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
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.
Fixes #1199.
MmapedDict.__init__creates the backing file before sizing it, so it exists at 0 bytes for a moment:read_all_values_from_filedoesn't guard for that, so_unpack_integer(data, 0)on an empty read raisesstruct.errorand 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 ownif capacity == 0branch.Deliberately narrow:
_read_all_values'sRuntimeError('Read beyond file size detected, file is corrupted.')is untouched, so genuine corruption still fails loudly.I looked at catching
struct.errorin_read_metricsinstead (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.renameseems 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_mergeconvention:test_read_all_values_from_empty_fileMmapedDictleveltest_read_all_values_from_file_with_truncated_contentstest_uninitialized_file_during_mergemerge()tolerates it and returns no metricstest_uninitialized_file_does_not_hide_other_metricsThe three empty-file tests fail on
masterwith the exactstruct.errorfrom #1199 and pass with the change; the truncated-file test passes either way by design. Full suite green,flake8andisortclean.@csmarchbanks — flagging you per
CONTRIBUTING.md.LLM use
Please be aware that I used Claude Opus and Sonnet during investigation and resolution.