Skip to content

Return empty metrics if the .db file has no data - #1200

Open
dom111 wants to merge 1 commit into
prometheus:masterfrom
dom111:1199-zero-byte-file-corruption
Open

Return empty metrics if the .db file has no data#1200
dom111 wants to merge 1 commit into
prometheus:masterfrom
dom111:1199-zero-byte-file-corruption

Conversation

@dom111

@dom111 dom111 commented Aug 12, 2026

Copy link
Copy Markdown

Fixes #1199.

MmapedDict.__init__ creates the backing file before sizing it, so it exists at 0 bytes for a moment:

self._f = open(filename, 'rb' if read_mode else 'a+b')   # creates at 0 bytes
capacity = os.fstat(self._f.fileno()).st_size
if capacity == 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:

Test Covers
test_read_all_values_from_empty_file empty file reads as empty at the MmapedDict level
test_read_all_values_from_file_with_truncated_contents a truncated non-empty file still raises — pins the guard's scope
test_uninitialized_file_during_merge merge() tolerates it and returns no metrics
test_uninitialized_file_does_not_hide_other_metrics 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.

@csmarchbanks — flagging you per CONTRIBUTING.md.

LLM use

Please be aware that I used Claude Opus and Sonnet during investigation and resolution.

…1199.

Signed-off-by: Dom Hastings <dominik.hastings@fundingcircle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MultiProcessCollector fails permanently on an empty metrics file left by a killed worker

1 participant