Opt-in crash reporting (Sentry) behind two gates that both have to hold - #96
Open
emir-hasanbegovic wants to merge 4 commits into
Open
Opt-in crash reporting (Sentry) behind two gates that both have to hold#96emir-hasanbegovic wants to merge 4 commits into
emir-hasanbegovic wants to merge 4 commits into
Conversation
Satellite has never transmitted anything, and on Linux and macOS it had no crash recorder at all: a segfault died with whatever the distro's core-dump collector happened to catch. Windows wrote a minidump nobody was told about. This wires the Sentry native SDK, gated twice over. The operator's switch (Settings, Diagnostics) defaults to false and stays false through an upgrade. Deliberately not matched to the Dish clients' default-on: an install that never saw the ask must not start transmitting on its owner's behalf. Flipping it off disarms the SDK immediately rather than at the next restart, because withdrawing consent has to stop the next crash and not the one after it. The build is the gate that actually holds. SATELLITE_SENTRY_DSN is empty in CMake and only release.yml fills it in, from a repository secret, so a local build, a PR build and a build from a fork all carry no DSN and cannot report whatever the switch says. The Sentry environment derives from SATELLITE_RELEASE_VERSION, which only the release workflow sets, but that is only a label (build-appimage.sh sets it when run by hand), which is exactly why the DSN and not the label is load-bearing. The release string uses the display version so a -dev build cannot file itself against a real release. On Windows the local dumps\ writer keeps running and dumpFilter now chains to whatever filter was installed before it, so the local artifact and the Sentry report both see the crash instead of whichever recorder armed last winning outright. Session tracking and send_default_pii are off: the crash is the payload, and a server meant to run unattended for weeks should not report every start and stop. The status payload carries the opt-in and whether it actually armed as separate fields, so a build with no DSN says so rather than implying reports are going somewhere they are not. Tests cover the arming policy as a pure function, both directions, plus the config default, the absent-key default and the round-trip.
Two CI failures, both mine.
The core purity gate (scripts/check_core_purity.sh) rejects any non-std
include under src/core, and <sentry.h> is exactly what it exists to keep out:
core has to stay compilable on every platform with no external surface. The
policy and the binding were in one file, so the file was in the wrong layer.
Split along the line the gate draws. core/crash_reporting.{h,cpp} keeps the
part worth testing and the part that must stay portable: the compiled-in
identity, the $SENTRY_DSN read, shouldArm() and databaseDirFor(), all std-only.
adapters/crash_adapter.{h,cpp} owns <sentry.h> and the SDK lifecycle, next to
log_adapter and client_adapter, which is where a third-party binding belongs.
Callers include the adapter; the pure test builds both, with the adapter inert,
so "this binary cannot transmit" is still an assertion and not an assumption.
The hardened MSVC lane builds warnings-as-errors and std::getenv trips C4996,
which is a compile failure there rather than a warning. Read the override
through _dupenv_s under _MSC_VER instead, same contract either way.
Verified locally: core purity gate OK, 36/36 ctest, clang-format OK, and both
translation units compiled with MSVC 14.44 under /W4 /WX, which is the lane
that caught the getenv problem.
Two more, from the same split.
sentry_options_set_send_default_pii does not exist on any platform satellite
ships to. In sentry-native it sits inside #ifdef SENTRY_PLATFORM_NX, so it is
Nintendo Switch only, and the hardened MSVC lane rejected it as an undeclared
identifier. Its own documentation says not sending PII is already the default
("If false (the default), the SDK won't add PII or other sensitive data"), so
the call was buying nothing even where it would compile. Removed, with the
reasoning recorded at the call site so nobody adds it back. Automatic session
tracking is a different case: unguarded, documented as defaulting to on, so
that call stays and is doing real work.
The other one was mine to see coming. Moving the SDK binding into
adapters/ swapped the include in every caller, but linux/main.cpp and
macos/main.mm also call crash::databaseDirFor(), which stayed in core. They
need both headers. Windows does not, because it passes lifecycle::sentryDir()
instead, which is exactly why a Windows-only local build stayed green while
the other two platforms broke.
Verified this round rather than assumed: both translation units compiled under
MSVC /W4 /WX against the real sentry-native 0.16.3 header with the SDK enabled,
which reproduces the CI lane that caught the PII call; a negative control
confirms that compile fails when the call is put back. Plus an audit of every
crash:: symbol each caller uses against the headers it includes, core purity
gate, 36/36 ctest and clang-format.
test_routes_admin and test_routes_client build routes_admin.cpp, which reads crash::active() and flips crash::setEnabled(), but their source list carried only SATELLITE_CORE_SOURCES. That gets the pure policy and not the adapter, so both link steps failed on undefined symbols. They are gated to APPLE OR Linux, because the Windows platform list would drag tray/toast/COM surface into a test binary. That is the whole reason a green local Windows build kept saying nothing about them, twice. Added the adapter to SATELLITE_ROUTE_TEST_COMMON_SOURCES. It compiles inert there: no test target defines SATELLITE_HAS_SENTRY, so this pulls in no Sentry dependency, only the symbols the route table already calls. Checked the other direction too: routes_admin.cpp has exactly two consumers, SATELLITE_NET_SOURCES (the satellite binary, which already takes SATELLITE_ADAPTER_SOURCES on all three platforms) and this helper. Both now resolve.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wires the Sentry native SDK into satellite. Satellite has never transmitted anything, and on Linux and macOS it had no crash recorder at all: a segfault died with whatever the distro's core-dump collector happened to catch. Windows wrote a minidump nobody was ever told about.
Two gates, and only one of them is load-bearing
Consent.
Config::crashReportingdefaults tofalseand stays false through an upgrade. Deliberately not matched to the Dish clients' default-on: an install that never saw the ask must not start transmitting on its owner's behalf. The switch lives in the admin UI under Settings, Diagnostics, in all six locales. Flipping it off disarms the SDK immediately rather than at the next restart, because withdrawing consent has to stop the next crash, not the one after it.The build.
SATELLITE_SENTRY_DSNis empty in CMake and onlyrelease.ymlfills it in, from theSENTRY_DSNrepository secret. Secrets are not exposed to forks, so a local build, a PR build and a fork build all carry no DSN and cannot report no matter what the switch says.The Sentry environment derives from
SATELLITE_RELEASE_VERSION, which only the release workflow sets. But that is only a label, andscripts/build-appimage.shsets it when run by hand, which is precisely why the DSN rather than the label is the gate that holds. The release string usesSATELLITE_VERSION_DISPLAYso a-devbuild cannot file itself against a real release and mix unsymbolicated frames into genuine data.$SENTRY_DSNremains a developer escape hatch, and still respects the opt-in.Windows: two recorders, chained
installCrashHandler()arms in the first lines ofWinMain, before any file I/O; Sentry can only arm once the config is read. Whoever installs last would otherwise win outright and silently replace the other. SodumpFilternow chains to whatever filter preceded it, andrearmCrashFilterChain()puts satellite's back on top aftercrash::init(). The localdumps\*.dmpwriter is unchanged.Privacy posture
auto_session_trackingandsend_default_piiare both off. The crash is the payload; a server meant to run unattended for weeks should not be reporting every start and stop to anyone. The status payload reportscrashReportingandcrashReportingActiveseparately so a build with no DSN says so instead of implying reports are going somewhere they are not.Verification
Built and tested locally (msys2 ucrt64): full build clean, 36/36 ctest pass,
scripts/check-format.shclean. Newtest_crash_reportingcovers the arming policy as a pure function in both directions, the config default, the absent-key default and the round-trip.test_status_json's exact-shape assertion updated for the two new fields.Before merging
SENTRY_DSNrepository secret. Until it exists, releases build with no DSN and report nothing — the code is inert, not broken.ingest.us.sentry.io; region is fixed per organization and can only be changed by creating a new one.sentry-nativeis added tovcpkg.json, so the MSVC lane will build it for the first time. Expect a slow first run before the binary cache warms.sentry-cli) is not in this PR. Without it, native frames arrive as addresses. Worth a follow-up keyed to the same release string.