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

1 participant