Skip to content

Add error logging around FileSink and header serialization failures#911

Merged
godlygeek merged 1 commit into
bloomberg:mainfrom
swaroski:add-error-logging-747
Apr 28, 2026
Merged

Add error logging around FileSink and header serialization failures#911
godlygeek merged 1 commit into
bloomberg:mainfrom
swaroski:add-error-logging-747

Conversation

@swaroski
Copy link
Copy Markdown
Contributor

Refs #747, #731

Summary

  • FileSink::seek and FileSink::grow now log the failing syscall (mmap, munmap, lseek, posix_fallocate),
    the errno string, and a hint to try writing the capture file to a different location such as /tmp.
  • RecordWriter::writeHeaderCommon now logs when header serialization fails, pointing readers at the preceding
    sink-level log entries for the underlying cause.
  • Previously, filesystems that do not support shared writable mmap or posix_fallocate (issue Failed to write output header #731) surfaced only
    as a generic RuntimeError: Failed to write output header with no diagnostic information.

Test plan

  • Extension rebuilds cleanly.
  • tests/unit/test_tracker.py passes against the rebuilt extension.
  • CI

Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/record_writer.cpp Outdated
@swaroski swaroski force-pushed the add-error-logging-747 branch from 8a91a14 to 3b1d7f7 Compare April 28, 2026 21:01
@swaroski
Copy link
Copy Markdown
Contributor Author

@godlygeek addressed: dropped programming-error LOGs
(whence/negative-offset/munmap), reworded mmap+grow logs to use d_filename and bytes wording, gated filesystem-support
hint on first-call only, added macOS F_PREALLOCATE wording, removed redundant header-serialize LOG, added
Signed-off-by.

@godlygeek godlygeek force-pushed the add-error-logging-747 branch from 3b1d7f7 to ce779f6 Compare April 28, 2026 22:20
Comment thread src/memray/_memray/sink.cpp Outdated
Comment thread src/memray/_memray/sink.cpp Outdated
Capture file creation could fail with a generic RuntimeError of
"Failed to write output header" when the destination filesystem did
not support shared writable mmap or posix_fallocate, with no log
indication of the underlying cause. Log the failing syscall, the
errno, and (when failing on the first call) a hint to retry with a
different output location such as /tmp.

Refs bloomberg#747, bloomberg#731

Signed-off-by: Suyog Bhise <bhise.suyog@gmail.com>
@godlygeek godlygeek force-pushed the add-error-logging-747 branch from fe23775 to 9510e67 Compare April 28, 2026 22:23
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.34%. Comparing base (015e6f0) to head (9510e67).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/memray/_memray/sink.cpp 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #911      +/-   ##
==========================================
- Coverage   92.46%   92.34%   -0.13%     
==========================================
  Files          99       99              
  Lines       11748    11758      +10     
  Branches      427      427              
==========================================
- Hits        10863    10858       -5     
- Misses        885      900      +15     
Flag Coverage Δ
cpp 92.34% <0.00%> (-0.13%) ⬇️
python_and_cython 92.34% <0.00%> (-0.13%) ⬇️

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

@godlygeek godlygeek enabled auto-merge (squash) April 28, 2026 22:46
@godlygeek godlygeek merged commit 3640823 into bloomberg:main Apr 28, 2026
17 of 18 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