Skip to content

Append strerror to listen, /proc/self/status, and header-write errors#914

Merged
godlygeek merged 2 commits into
bloomberg:mainfrom
swaroski:feat/error-logging
May 6, 2026
Merged

Append strerror to listen, /proc/self/status, and header-write errors#914
godlygeek merged 2 commits into
bloomberg:mainfrom
swaroski:feat/error-logging

Conversation

@swaroski
Copy link
Copy Markdown
Contributor

@swaroski swaroski commented May 2, 2026

Summary

Follow-up to #911. Append strerror(errno) to three remaining bare error sites so the cause is visible in tracebacks without raising MEMRAY_LOG_LEVEL:

  • SocketSink::openlisten() failure (e.g. port already bound).
  • Tracker::BackgroundThread/proc/self/status open failure (mount namespace without /proc, post-privilege-drop, EMFILE).
  • Tracker ctor — Failed to write output header RuntimeError (e.g. ENOSPC, EFBIG, EPIPE).

Per maintainer guidance, the FileSink::seek munmap path is left unchanged.

Test plan

  • Builds clean with pip install -e . --no-build-isolation
  • pytest tests/ passes
  • Manual: bind port, run SocketSink against same port — IoError contains Address already in use
  • Manual: write capture to tiny tmpfs — RuntimeError traceback contains No space left on device

Follow-up to bloomberg#911. Adds the underlying errno via strerror to three
remaining bare error sites so the cause is visible in user-facing
messages without raising MEMRAY_LOG_LEVEL:

- SocketSink listen() failure (e.g. port already in use)
- BackgroundThread /proc/self/status open failure (mount ns, EMFILE,
  permission drop)
- Tracker ctor "Failed to write output header" RuntimeError, which
  surfaces in Python tracebacks (bloomberg#731)

errno is captured into a local immediately after each failure check,
before any close()/string-allocation that could clobber it.

Skips the FileSink::seek munmap path: only reachable via a bug or
memory corruption, so not worth the maintenance cost.

Signed-off-by: Suyog Bhise <bhise.suyog@gmail.com>
Comment thread news/747.feature.rst Outdated
Comment thread src/memray/_memray/tracking_api.cpp Outdated
Comment thread src/memray/_memray/tracking_api.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/tracking_api.cpp Outdated
- Shorten news/747.feature.rst per reviewer wording.
- Drop saved_errno locals; strerror(errno) is sequenced before the
  std::string operations on the same line.
- Compute strerror once in SocketSink::open and reuse for both log and
  exception message.
- Replace <cstring> with <stdio.h> for strerror in tracking_api.cpp.

Signed-off-by: Suyog Bhise <bhise.suyog@gmail.com>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.38%. Comparing base (faf7d12) to head (487b5fe).

Files with missing lines Patch % Lines
src/memray/_memray/sink.cpp 0.00% 3 Missing ⚠️
src/memray/_memray/tracking_api.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #914      +/-   ##
==========================================
- Coverage   92.45%   92.38%   -0.08%     
==========================================
  Files          99       99              
  Lines       11784    11786       +2     
  Branches      428      428              
==========================================
- Hits        10895    10888       -7     
- Misses        889      898       +9     
Flag Coverage Δ
cpp 92.38% <0.00%> (-0.08%) ⬇️
python_and_cython 92.38% <0.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@godlygeek godlygeek merged commit 796647f into bloomberg:main May 6, 2026
18 of 20 checks passed
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.

3 participants