prometheus / prometheus/client_python

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

未关闭
#1,199 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看

还没有人认领这个 Issue。

主要语言
Python
星标
4.4k
派生
876
平均合并
8 天 4 小时
30 天内合并 PR
1

描述

What did you do?

Ran a FastAPI app under gunicorn with PROMETHEUS_MULTIPROC_DIR set and several workers, and scraped /metrics.

What did you expect to see?

A successful scrape. Failing that, a failure confined to the worker whose file could not be read.

What did you see instead?

MultiProcessCollector.collect() raising, and taking every metric in the registry with it:

struct.error: unpack_from requires a buffer of at least 4 bytes for unpacking 4 bytes at offset 0 (actual buffer size is 0)

  File ".../prometheus_client/mmap_dict.py", line 90, in read_all_values_from_file
    used = _unpack_integer(data, 0)[0]
  File ".../prometheus_client/multiprocess.py", line 63, in _read_metrics
    file_values = MmapedDict.read_all_values_from_file(f)
  File ".../prometheus_client/multiprocess.py", line 43, in merge
    metrics = MultiProcessCollector._read_metrics(files)
  File ".../prometheus_client/multiprocess.py", line 158, in collect
    return self.merge(files, accumulate=True)

(Line numbers are from 0.20.0, where this was hit in production; collect is at 171 in 0.26.0. The relevant code is byte-identical between the two.)

Mechanism

MmapedDict.__init__ creates the backing file and only sizes it afterwards, so there is a window in which it exists at 0 bytes:

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 has no guard for that, so _unpack_integer(data, 0) on an empty read raises. This is at least one verified mechanism; a truncate failing on a full volume (ENOSPC/EDQUOT), or external cleanup that truncates rather than unlinks, would leave the same state.

This has two quite different consequences, and the second is the reason I'm filing:

1. Transient. A scrape lands inside the creation window. Verified with 6 concurrent writer processes and a reader looping glob+read using a verbatim copy of 0.26.0's header logic: 410 observations of 0-byte .db files and 180 struct.errors in 20 seconds. This one does resolve itself on the next scrape.

2. Permanent, and total. A worker killed inside that window (OOM, SIGKILL during a rolling restart) leaves the empty file behind for good — it is named after a pid that never comes back, so nothing ever cleans it up. Every subsequent scrape then fails, and because the exception aborts the whole merge, all other workers' metrics are lost too. Repro:

import os, tempfile
d = tempfile.mkdtemp()
os.environ['PROMETHEUS_MULTIPROC_DIR'] = d
from prometheus_client import CollectorRegistry, Counter, values
from prometheus_client.multiprocess import MultiProcessCollector
values.ValueClass = values.MultiProcessValue()
reg = CollectorRegistry()
c = MultiProcessCollector(reg)
Counter('good', 'help', registry=None).inc()
# a 0-byte file left behind by a process killed between open('a+b') and truncate()
open(os.path.join(d, 'counter_31337.db'), 'wb').close()
for i in range(3):
    try:
        print(f"  scrape {i+1}: OK, metrics={sorted(m.name for m in c.collect())}")
    except Exception as e:
        print(f"  scrape {i+1}: {type(e).__name__}: {e}")

On 0.26.0 all three scrapes raise, and the unrelated good counter is never exported.

_read_metrics already tolerates the analogous race of a gauge_live* file vanishing between the glob and the read (FileNotFoundError, via mark_process_dead), but there is no equivalent tolerance for a file that exists and has not been initialised — for any metric type.

Suggested fix

Treat an empty read as a file with nothing recorded in it yet, which is exactly what __init__'s own if capacity == 0 branch already does on the write side:

data = infp.read(mmap.PAGESIZE)
if not data:
    return iter(())
used = _unpack_integer(data, 0)[0]

Deliberately narrow, for two reasons:

  • It changes nothing for files that are non-empty. 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 — genuine corruption keeps failing loudly.
  • It is a smaller change 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 introducing new leniency.

Two alternatives I looked at and would argue against:

  • Catching struct.error in _read_metrics — far too broad, and would swallow real corruption. An os.path.getsize() pre-check instead just adds a syscall per file per scrape and is still TOCTOU.
  • Deleting 0-byte files on read (suggested in #604) — risky: the file may belong to a live worker that holds the fd and is about to truncate it, and unlinking would orphan that worker's metrics for its whole lifetime.

Closing the window at source in __init__ (write to a temp file, then atomic os.rename) is worth considering as a follow-up, but it is a bigger change and, crucially, would not heal empty files already on disk from earlier versions.

Relation to #604

#604 reports this identical error and was closed in 2020, with the suggestion that it was misuse or, in the maintainer's words, "data corruption on disk". I think that conclusion was reached without the permanent-file case being visible; the repro above shows a 0-byte file is enough on its own, with no corruption involved. A 2025 comment on that thread independently pins it on 0-byte .db files, with a directory listing. Happy for this to be folded back into #604 if you'd prefer.

Versions
  • prometheus_client: hit in production on 0.20.0; verified unchanged on 0.26.0 (mmap_dict.py is byte-identical between the two).
  • Python 3.13, Linux (Kubernetes), gunicorn.

I have the fix plus four regression tests ready as a PR against master (DCO signed) — the tests cover the empty file at both the MmapedDict and merge() levels, that other metrics survive a stale empty file, and that a truncated non-empty file still raises. Full suite passes on the tox matrix, flake8 and isort clean. Glad to open it if this looks like the right direction.

LLM use

Please note that I used Claude Opus and Sonnet whilst investigating this.

贡献指南

打开贡献指南

从这里开始

  1. 先读完整个 Issue,再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库,在一个分支上完成修改。
  4. 提交 Pull Request,并在描述里引用这个 Issue 编号。

调研方向

从 prometheus_client/mmap_dict.py 中的 read_all_values_from_file 开始,然后检查 prometheus_client/multiprocess.py 中的 MultiProcessCollector.merge。运行覆盖 MmapedDict 和 merge 的回归测试:空文件应被忽略,其他指标应保留,并且截断的非空文件仍应引发错误。

由索引模型根据 Issue 内容生成。

评估

技术栈
python
领域
observability-sre
Issue 类型
缺陷
难度
2/5
预计耗时
1-3 小时
活跃度
冷清
描述清晰度
描述清楚
新手友好度
58/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。