Skip to content

Upload crash info to Sentry#418

Open
edolstra wants to merge 27 commits intomainfrom
eelcodolstra/nix-361
Open

Upload crash info to Sentry#418
edolstra wants to merge 27 commits intomainfrom
eelcodolstra/nix-361

Conversation

@edolstra
Copy link
Copy Markdown
Collaborator

@edolstra edolstra commented Apr 7, 2026

Motivation

Context

Summary by CodeRabbit

  • New Features

    • Optional Sentry crash reporting with build-time toggle, conditional initialization, and a new debug output artifact.
    • Tool to collect and upload debug symbols to Sentry.
    • Test-only crash command to trigger crash scenarios.
  • Tests

    • Functional tests added to validate crash reporting and uploaded crash artifacts; test init disables real upload.
  • Documentation

    • Manpage generation now omits undocumented commands.
  • Packaging / CI

    • Packaging adds optional Sentry-native packaging and debug output wiring; CI workflow accepts optional Sentry secrets and conditionally uploads debug info.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 7, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds optional Sentry crash reporting (build-time and runtime), a debug output artifact and conditional CI upload step, a script to fetch and upload ELF debug symbols to Sentry, a test crash command and functional test, SIGSEGV handler preservation, and filtering to hide undocumented commands from completion and manpage generation.

Changes

Cohort / File(s) Summary
CI workflow & upload script
\.github/workflows/build.yml, \.github/workflows/ci.yml, maintainers/upload-debug-info-to-sentry.py
Build workflow accepts optional Sentry secrets, builds a debug output and conditionally runs the new script to collect ELF build-ids, fetch debuginfo from cache.nixos.org, extract .debug files and upload via sentry-cli. CI passes secrets to the reusable workflow.
Runtime Sentry & crash command
src/nix/main.cc, src/nix/crash.cc, src/libcmd/include/nix/cmd/command.hh
Adds conditional Sentry init/shutdown with config and crashpad handler path, defers registering fallback crash handler when Sentry is active, introduces catUndocumented = 103, and registers a new __crash command (CmdCrash) to trigger test crashes.
Build options & Meson
src/nix/meson.options, src/nix/meson.build, src/nix/package.nix
Adds sentry Meson option and HAVE_SENTRY flag, includes crash.cc in sources, conditionally links -lsentry, and accepts sentry-native as a Nix input enabled on Linux non-static builds (crashpad handler passed via Meson).
Packaging outputs & overrides
packaging/everything.nix, packaging/dependencies.nix, packaging/sentry-native.nix
Adds a multi-output debug output aggregated from component debug outputs and required inputs (curl, boehmgc, optional sentry-native); adds sentry-native derivation and an override that inherits curl from scope.
Debug install logic
packaging/everything.nix (highlighted)
Creates $debug/lib/debug, computes debugPaths (including component debug outputs, nix-cli, curl, boehmgc, and conditionally sentry-native) and links/mirrors component debug directories into the debug output.
Signal handling
src/libmain/unix/stack.cc
Preserves and reinstalls the previously installed SIGSEGV handler after handling stack-overflow instead of switching to SIG_DFL.
Completion & manpage filtering
src/libutil/args.cc, doc/manual/generate-manpage.nix
Command completion now omits names starting with __; manpage generator filters out commands in category id 103 (undocumented).
Tests
tests/functional/sentry.sh, tests/functional/meson.build, tests/functional/common/init.sh
Adds functional test sentry.sh exercising nix __crash variants and verifying Sentry envelopes; registers test in Meson; test init exports NIX_SENTRY_ENDPOINT= by default to disable external uploading during general tests, while the functional test sets a local file URL.
Meson assert-fail change
nix-meson-build-support/common/assert-fail/meson.build
Replaced a conditional Meson if block with a preprocessor-style comment block, effectively disabling the prior conditional dependency inclusion.

Sequence Diagram(s)

sequenceDiagram
    participant CI as CI Job
    participant Build as build.yml workflow
    participant Script as upload-debug-info-to-sentry.py
    participant Cache as cache.nixos.org
    participant NAR as nix nar
    participant Sentry as sentry-cli

    CI->>Build: trigger build with optional Sentry secrets
    Build->>Build: build `debug` output
    Build->>Script: run upload step (executable + debug dirs) [conditional: linux && secret present]
    Script->>Script: run ldd / readelf -> collect build IDs
    Script->>Cache: GET /debuginfo/<build_id>
    alt 200 OK
        Cache-->>Script: JSON with archive member
        Script->>NAR: download NAR & extract member
        NAR-->>Script: writes /tmp/debug-info/<build_id>.debug
    else 404
        Cache-->>Script: 404 (skip)
    end
    Script->>Sentry: sentry-cli debug-files upload --project? <.debug files>
    Sentry-->>Script: upload result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • cole-h

Poem

🐰 Hoppity-hop, I found the trace,

build-ids hiding in a quiet place.
Handlers kept and crashes tried,
Debugs collected, neatly tied.
A rabbit cheers: "Upload—we're ready!"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Upload crash info to Sentry' directly and accurately summarizes the main objective of the changeset: integrating Sentry crash reporting throughout the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch eelcodolstra/nix-361

Comment @coderabbitai help to get the list of available commands and usage tips.

@edolstra edolstra force-pushed the eelcodolstra/nix-361 branch from e306227 to 088b068 Compare April 7, 2026 20:08
@edolstra edolstra changed the title Eelcodolstra/nix 361 Upload crash info to Sentry Apr 7, 2026
@edolstra edolstra force-pushed the eelcodolstra/nix-361 branch 2 times, most recently from 338ef39 to 1d0df75 Compare April 7, 2026 20:35
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

@github-actions github-actions bot temporarily deployed to pull request April 7, 2026 20:38 Inactive
@edolstra edolstra force-pushed the eelcodolstra/nix-361 branch from 1d0df75 to 963a0d5 Compare April 8, 2026 12:35
@github-actions github-actions bot temporarily deployed to pull request April 8, 2026 12:40 Inactive
@edolstra edolstra marked this pull request as ready for review April 8, 2026 13:51
@github-actions github-actions bot temporarily deployed to pull request April 8, 2026 13:59 Inactive
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/nix/meson.build (1)

84-85: ⚠️ Potential issue | 🟡 Minor

Duplicate source file entry.

flake-prefetch-inputs.cc is listed twice on lines 84 and 85. This should be deduplicated.

🧹 Proposed fix
   'flake-prefetch-inputs.cc',
-  'flake-prefetch-inputs.cc',
   'flake.cc',
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nix/meson.build` around lines 84 - 85, The source list in meson.build
contains a duplicate entry 'flake-prefetch-inputs.cc'; remove the redundant
occurrence so the file appears only once in the sources array (update the
sources array where 'flake-prefetch-inputs.cc' is listed twice to a single
entry), and run a quick build or lint to confirm no other duplicates remain.
🧹 Nitpick comments (2)
src/nix/meson.build (1)

197-197: Consider using Meson's dependency() for sentry.

Using -lsentry directly in link_args works but is less idiomatic than using Meson's dependency() function which provides better cross-platform handling and pkg-config integration. However, if sentry-native doesn't provide a reliable pkg-config file, this approach is acceptable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nix/meson.build` at line 197, Replace the hard-coded '-lsentry' in the
link_args conditional with Meson's dependency() so Meson can resolve sentry
cross-platform; specifically, create a dependency object (e.g., sentry_dep =
dependency('sentry' or 'sentry-native', required: sentry_required.enabled()))
and when enabled append sentry_dep's link arguments or library to the targets
(use sentry_dep.get_link_args() or add the dependency object to
link_with/link_args) instead of the literal '-lsentry'; keep the current literal
fallback only if sentry-native does not provide a pkg-config file.
maintainers/upload-debug-info-to-sentry.py (1)

123-130: Consider validating JSON response keys for clearer error messages.

If the debuginfo JSON response is missing member or archive keys, a raw KeyError will be raised with no context about which library or build ID caused the failure.

💡 Suggested improvement for error clarity
         debuginfo = fetch_debuginfo(build_id)
         if debuginfo is None:
             print(f"  {lib} ({build_id}, no debug info in cache)", file=sys.stderr)
             continue
+        if "member" not in debuginfo or "archive" not in debuginfo:
+            print(f"  {lib} ({build_id}): malformed debuginfo response", file=sys.stderr)
+            continue
         print(f"  {lib} ({build_id}): member={debuginfo['member']}", file=sys.stderr)
         nar_path = download_nar(build_id, debuginfo["archive"])
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@maintainers/upload-debug-info-to-sentry.py` around lines 123 - 130, The code
assumes debuginfo dict contains "member" and "archive" but will raise an
unhelpful KeyError; update the block after fetch_debuginfo(build_id) to
explicitly check for required keys ("member" and "archive") on debuginfo and, if
missing, log or print a descriptive error including lib and build_id and then
continue (do not call download_nar or extract_debug_symbols). Use the debuginfo
variable and functions download_nar and extract_debug_symbols in the message so
it's clear which library/build failed and why before appending to debug_files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/build.yml:
- Around line 62-67: The Sentry upload step (the run invoking
./maintainers/upload-debug-info-to-sentry.py with env vars SENTRY_AUTH_TOKEN,
SENTRY_ORG, SENTRY_PROJECT) should be guarded by a GitHub Actions conditional so
it only runs when those secrets are populated; add an if: expression to that
job/step such as checking each secret is non-empty (e.g. if: ${{
secrets.sentry_auth_token != '' && secrets.sentry_org != '' &&
secrets.sentry_project != '' }} ) so the upload is skipped for forks/PRs without
secrets.

In `@doc/manual/generate-manpage.nix`:
- Around line 79-83: The code currently filters out category id 103 only inside
the showCommand() mapping, but processCommand(), tableOfContents, and other
consumers still iterate over details.commands causing undocumented commands
(e.g., nix __crash) to be rendered; fix this by creating a single filtered
attrset (e.g., filteredCommands = builtins.filter (cmd: cmd.category.id != 103)
(attrValues details.commands) or an attrset variant) and use filteredCommands
everywhere—replace uses of details.commands in processCommand(),
tableOfContents, and showCommand() with this new filteredCommands so the same
set is reused for both subcommand rendering and page generation.

In `@maintainers/upload-debug-info-to-sentry.py`:
- Around line 63-73: The code opens nar_path and passes it to subprocess.Popen
for xz without closing the file and doesn't check xz's exit status; update the
block around xz = subprocess.Popen(...) and xz.wait() to use a context manager:
open nar_path with "with open(nar_path, 'rb') as nar_f:" and pass nar_f as stdin
to subprocess.Popen (xz), then run subprocess.run for ["nix", "nar", "cat",
"/dev/stdin", member] with stdin=xz.stdout and check=True, ensure you close
xz.stdout (e.g., call xz.stdout.close()) and then call xz.wait() and verify
xz.returncode (or use check to raise on non-zero) before writing nar_cat.stdout
to out_path using a with open(out_path, "wb") as f: block; reference symbols:
xz, subprocess.Popen, open(nar_path, "rb"), nar_cat, xz.wait, out_path.

In `@nix-meson-build-support/common/assert-fail/meson.build`:
- Around line 27-32: The conditional block currently uses Meson comment syntax
"#if"/"#endif" so it never runs; change it to a real Meson conditional using if
can_wrap_assert_fail ... endif and restore the declare_dependency call so that
deps_other is appended with sources: 'wrap-assert-fail.cc' and link_args:
wrap_assert_fail_args when can_wrap_assert_fail is true (refer to the
can_wrap_assert_fail variable, deps_other list, and
wrap_assert_fail_args/wrap-assert-fail.cc symbols).

In `@src/libmain/unix/stack.cc`:
- Around line 13-14: The code currently overwrites savedSigsegvAction each time
detectStackOverflow() installs sigsegvHandler; modify the install logic in
detectStackOverflow() so it preserves the first external SIGSEGV action instead
of overwriting it on subsequent calls: only populate savedSigsegvAction when it
has not been initialized (or when its handler/sa_sigaction is not already
sigsegvHandler), or add a boolean flag like savedSigsegvActionInitialized to
guard the save; leave the existing restore logic that uses savedSigsegvAction
unchanged so restore still chains to the original handler.

In `@src/nix/crash.cc`:
- Line 1: The file uses std::bitset<4> (referenced as std::bitset<4>) but does
not explicitly include <bitset>; add an explicit `#include` <bitset> near the
other includes (e.g., alongside the existing `#include` "nix/cmd/command.hh") so
the use of std::bitset<4> is correct and not dependent on transitive includes.

In `@src/nix/main.cc`:
- Around line 384-403: The Finally cleanup currently always calls
sentry_shutdown() even if sentry_init() was skipped; modify the logic so the
cleanup callback is only registered when sentryEnabled is true (or make the
callback check sentryEnabled before calling sentry_shutdown()). Specifically,
update the block around sentry_init/sentryEnabled and the Finally
cleanupSentry([]() { sentry_shutdown(); }); usage so sentry_shutdown() is not
invoked unless sentry_init() actually ran (refer to sentryEnabled, sentry_init,
sentry_shutdown, and cleanupSentry).

In `@tests/functional/meson.build`:
- Line 179: The test enabling production Sentry is being added to the always-on
suite via inclusion of sentry.sh; either remove sentry.sh from the default suite
or change the test to use a test-only DSN/offline transport so it cannot
initialize the production Sentry from main. Fix options: (1) in the meson.build
test list, remove sentry.sh from the always-on suite so it runs only in an
explicit Sentry test suite; or (2) modify sentry.sh to preserve the
NIX_DISABLE_SENTRY guard or set a test SENTRY_DSN/offline transport before
unsetting NIX_DISABLE_SENTRY (e.g., export SENTRY_DSN=a_dummy_value or enable
offline transport) so src/nix/main.cc will not send real events; reference
sentry.sh, NIX_DISABLE_SENTRY, and src/nix/main.cc to locate and apply the
change.

---

Outside diff comments:
In `@src/nix/meson.build`:
- Around line 84-85: The source list in meson.build contains a duplicate entry
'flake-prefetch-inputs.cc'; remove the redundant occurrence so the file appears
only once in the sources array (update the sources array where
'flake-prefetch-inputs.cc' is listed twice to a single entry), and run a quick
build or lint to confirm no other duplicates remain.

---

Nitpick comments:
In `@maintainers/upload-debug-info-to-sentry.py`:
- Around line 123-130: The code assumes debuginfo dict contains "member" and
"archive" but will raise an unhelpful KeyError; update the block after
fetch_debuginfo(build_id) to explicitly check for required keys ("member" and
"archive") on debuginfo and, if missing, log or print a descriptive error
including lib and build_id and then continue (do not call download_nar or
extract_debug_symbols). Use the debuginfo variable and functions download_nar
and extract_debug_symbols in the message so it's clear which library/build
failed and why before appending to debug_files.

In `@src/nix/meson.build`:
- Line 197: Replace the hard-coded '-lsentry' in the link_args conditional with
Meson's dependency() so Meson can resolve sentry cross-platform; specifically,
create a dependency object (e.g., sentry_dep = dependency('sentry' or
'sentry-native', required: sentry_required.enabled())) and when enabled append
sentry_dep's link arguments or library to the targets (use
sentry_dep.get_link_args() or add the dependency object to link_with/link_args)
instead of the literal '-lsentry'; keep the current literal fallback only if
sentry-native does not provide a pkg-config file.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0a84855e-60b7-4ec6-8345-b834ac644486

📥 Commits

Reviewing files that changed from the base of the PR and between e9b4735 and b32fe09.

📒 Files selected for processing (18)
  • .github/workflows/build.yml
  • .github/workflows/ci.yml
  • doc/manual/generate-manpage.nix
  • maintainers/upload-debug-info-to-sentry.py
  • nix-meson-build-support/common/assert-fail/meson.build
  • packaging/dependencies.nix
  • packaging/everything.nix
  • src/libcmd/include/nix/cmd/command.hh
  • src/libmain/unix/stack.cc
  • src/libutil/args.cc
  • src/nix/crash.cc
  • src/nix/main.cc
  • src/nix/meson.build
  • src/nix/meson.options
  • src/nix/package.nix
  • tests/functional/common/init.sh
  • tests/functional/meson.build
  • tests/functional/sentry.sh

Comment on lines +79 to +83
unique (
map (cmd: { inherit (cmd.category) id description; }) (
builtins.filter (cmd: cmd.category.id != 103) (attrValues details.commands)
)
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Filter undocumented commands out of the command set, not just the category list.

Line 81 only removes category 103 from the grouping logic inside showCommand(). processCommand() later still recurses over every entry in details.commands, and tableOfContents still renders every generated page, so nix __crash will still get its own manpage and SUMMARY.md entry. Please compute a filtered command attrset once and reuse it for both subcommand rendering and page generation.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@doc/manual/generate-manpage.nix` around lines 79 - 83, The code currently
filters out category id 103 only inside the showCommand() mapping, but
processCommand(), tableOfContents, and other consumers still iterate over
details.commands causing undocumented commands (e.g., nix __crash) to be
rendered; fix this by creating a single filtered attrset (e.g., filteredCommands
= builtins.filter (cmd: cmd.category.id != 103) (attrValues details.commands) or
an attrset variant) and use filteredCommands everywhere—replace uses of
details.commands in processCommand(), tableOfContents, and showCommand() with
this new filteredCommands so the same set is reused for both subcommand
rendering and page generation.

Comment on lines +27 to +32
#if can_wrap_assert_fail
# deps_other += declare_dependency(
# sources : 'wrap-assert-fail.cc',
# link_args : wrap_assert_fail_args,
# )
#endif
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== assert-fail block ==\n'
sed -n '21,32p' nix-meson-build-support/common/assert-fail/meson.build

printf '\n== active dependency patterns in parallel Meson files ==\n'
sed -n '76,86p' nix-meson-build-support/common/cxa-throw/meson.build
sed -n '1,20p' nix-meson-build-support/common/asan-options/meson.build

printf '\n== active declare_dependency lines in assert-fail ==\n'
rg -n --fixed-strings "deps_other += declare_dependency" nix-meson-build-support/common/assert-fail/meson.build || true

printf '\n== wrap implementation source ==\n'
fd -a wrap-assert-fail.cc

Repository: DeterminateSystems/nix-src

Length of output: 1106


Restore a real Meson if block here.

Lines 27–32 use #if and #endif, which are comments in Meson—not conditionals. This block never executes, so wrap-assert-fail.cc is never added to deps_other even when can_wrap_assert_fail is true. This silently disables the assert-fail interposer.

Meson uses if/endif syntax (without the #). Parallel files like nix-meson-build-support/common/asan-options/meson.build correctly use active conditionals: if 'address' in get_option('b_sanitize') ... deps_other += declare_dependency(...) ... endif.

Suggested fix
-#if can_wrap_assert_fail
-#  deps_other += declare_dependency(
-#    sources : 'wrap-assert-fail.cc',
-#    link_args : wrap_assert_fail_args,
-#  )
-#endif
+if can_wrap_assert_fail
+  deps_other += declare_dependency(
+    sources : 'wrap-assert-fail.cc',
+    link_args : wrap_assert_fail_args,
+  )
+endif
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#if can_wrap_assert_fail
# deps_other += declare_dependency(
# sources : 'wrap-assert-fail.cc',
# link_args : wrap_assert_fail_args,
# )
#endif
if can_wrap_assert_fail
deps_other += declare_dependency(
sources : 'wrap-assert-fail.cc',
link_args : wrap_assert_fail_args,
)
endif
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nix-meson-build-support/common/assert-fail/meson.build` around lines 27 - 32,
The conditional block currently uses Meson comment syntax "#if"/"#endif" so it
never runs; change it to a real Meson conditional using if can_wrap_assert_fail
... endif and restore the declare_dependency call so that deps_other is appended
with sources: 'wrap-assert-fail.cc' and link_args: wrap_assert_fail_args when
can_wrap_assert_fail is true (refer to the can_wrap_assert_fail variable,
deps_other list, and wrap_assert_fail_args/wrap-assert-fail.cc symbols).

@github-actions github-actions bot temporarily deployed to pull request April 8, 2026 14:32 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 8, 2026 16:00 Inactive
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/build.yml:
- Around line 62-69: The step-level if currently uses the secrets context (if:
contains(inputs.system, 'linux') && secrets.sentry_auth_token != ''), which is
invalid; update the "Upload debug info to Sentry" step so the step-level
condition only uses allowed contexts (e.g., contains(inputs.system, 'linux')),
and move the secrets check into an environment or shell check: either set
SENTRY_AUTH_TOKEN in env (or at job-level) and use if: contains(inputs.system,
'linux') && env.SENTRY_AUTH_TOKEN != '' or keep the step conditional only on the
platform and inside the run command for
./maintainers/upload-debug-info-to-sentry.py add a shell guard that exits early
if $SENTRY_AUTH_TOKEN is empty; ensure SENTRY_AUTH_TOKEN, SENTRY_ORG and
SENTRY_PROJECT are supplied via env rather than referenced in the step-level if.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8603603a-d2b0-46c9-88e0-3b1a8299df84

📥 Commits

Reviewing files that changed from the base of the PR and between 66c57b0 and 17e5632.

📒 Files selected for processing (1)
  • .github/workflows/build.yml

@edolstra edolstra force-pushed the eelcodolstra/nix-361 branch from f6a2e2f to 9e2138b Compare April 8, 2026 20:46
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
src/nix/main.cc (1)

391-403: ⚠️ Potential issue | 🟠 Major

Gate Sentry cleanup on successful initialization.

Line 400 sets sentryEnabled = true unconditionally, but sentry_init() returns 0 on success. If init fails, Line 406 suppresses registerCrashHandler(), and Line 403 still invokes deprecated sentry_shutdown() during teardown. Only mark Sentry active after a successful init, and use sentry_close() for cleanup instead. (raw.githubusercontent.com)

Suggested fix
-        sentry_init(options);
-        sentryEnabled = true;
+        sentryEnabled = sentry_init(options) == 0;
     }

-    Finally cleanupSentry([]() { sentry_shutdown(); });
+    Finally cleanupSentry([&]() {
+        if (sentryEnabled)
+            sentry_close();
+    });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nix/main.cc` around lines 391 - 403, sentryEnabled is being set
unconditionally even if sentry_init fails and cleanupSentry always calls
sentry_shutdown; change the logic so you call sentry_init(options) and check its
return value (sentry_init returns 0 on success) before setting sentryEnabled and
registering crash handling; also modify the cleanup passed to cleanupSentry to
call sentry_close() (the current sentry_shutdown() is deprecated) only when
sentryEnabled was set after a successful sentry_init. Ensure the checks
reference sentry_init, sentryEnabled, cleanupSentry, sentry_shutdown, and
sentry_close in the updated flow.
🧹 Nitpick comments (1)
src/nix/meson.build (1)

53-61: Model Sentry as a Meson dependency instead of raw -lsentry.

Right now the sentry feature is reduced to enabled() plus a linker flag, so auto never attempts discovery and the target never gets a dependency object to carry the include/link metadata needed for <sentry.h>. A proper dependency()/find_library() flow would preserve the feature semantics and propagate the compiler/link flags automatically. (mesonbuild.com)

Also applies to: 205-205

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/nix/meson.build` around lines 53 - 61, The current code treats `sentry`
as a plain option and injects `-lsentry` instead of using Meson's dependency
resolution; change the flow to call dependency('sentry', required:
sentry_required == 'enabled' or use find_library('sentry', required:
...)/dependency() so you get a dependency object (e.g., `sentry_dep`) and use
`sentry_dep.found()` to set `HAVE_SENTRY` via `configdata.set('HAVE_SENTRY',
sentry_dep.found().to_int())` and propagate include/link metadata from that
object instead of raw flags; keep the crashpad-handler assertion and
`configdata.set_quoted('CRASHPAD_HANDLER_PATH', crashpad_handler_path)` as
before but ensure any target uses `sentry_dep` in its dependencies to carry
compiler/link flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/nix/main.cc`:
- Around line 391-403: sentryEnabled is being set unconditionally even if
sentry_init fails and cleanupSentry always calls sentry_shutdown; change the
logic so you call sentry_init(options) and check its return value (sentry_init
returns 0 on success) before setting sentryEnabled and registering crash
handling; also modify the cleanup passed to cleanupSentry to call sentry_close()
(the current sentry_shutdown() is deprecated) only when sentryEnabled was set
after a successful sentry_init. Ensure the checks reference sentry_init,
sentryEnabled, cleanupSentry, sentry_shutdown, and sentry_close in the updated
flow.

---

Nitpick comments:
In `@src/nix/meson.build`:
- Around line 53-61: The current code treats `sentry` as a plain option and
injects `-lsentry` instead of using Meson's dependency resolution; change the
flow to call dependency('sentry', required: sentry_required == 'enabled' or use
find_library('sentry', required: ...)/dependency() so you get a dependency
object (e.g., `sentry_dep`) and use `sentry_dep.found()` to set `HAVE_SENTRY`
via `configdata.set('HAVE_SENTRY', sentry_dep.found().to_int())` and propagate
include/link metadata from that object instead of raw flags; keep the
crashpad-handler assertion and `configdata.set_quoted('CRASHPAD_HANDLER_PATH',
crashpad_handler_path)` as before but ensure any target uses `sentry_dep` in its
dependencies to carry compiler/link flags.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4551d378-2750-4ee1-af58-40d9e28eefbc

📥 Commits

Reviewing files that changed from the base of the PR and between 17e5632 and f6a2e2f.

📒 Files selected for processing (10)
  • packaging/dependencies.nix
  • packaging/dev-shell.nix
  • packaging/sentry-native.nix
  • src/nix/crash.cc
  • src/nix/main.cc
  • src/nix/meson.build
  • src/nix/meson.options
  • src/nix/package.nix
  • tests/functional/common/init.sh
  • tests/functional/sentry.sh
✅ Files skipped from review due to trivial changes (2)
  • src/nix/meson.options
  • packaging/sentry-native.nix
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/functional/common/init.sh
  • tests/functional/sentry.sh
  • src/nix/package.nix
  • packaging/dependencies.nix

@github-actions github-actions bot temporarily deployed to pull request April 9, 2026 08:50 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 9, 2026 09:10 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 9, 2026 10:17 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 9, 2026 10:39 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 9, 2026 12:03 Inactive
Also respect the DETSYS_IDS_TELEMETRY environment variable.
@github-actions github-actions bot temporarily deployed to pull request April 10, 2026 10:28 Inactive
mandir = fs.is_absolute(mandir) ? mandir : prefix / mandir
configdata.set_quoted('NIX_MAN_DIR', mandir)

sentry_required = get_option('sentry')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: why sentry_required vs sentry?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In Meson, sentry would be the result of calling dependency('sentry').

link_args : wrap_assert_fail_args,
)
endif
#if can_wrap_assert_fail
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this not play well with sentry, or are we not wrapping it so sentry can handle it?

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.

2 participants