Bound Sentry crash-report buffers and drop dead code - #1708
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens crash-report diagnostics sent via Sentry by bounding in-memory buffers used for crash annotations, removing unused OBS log substreams and dead “breadcrumbs” code, and clarifying naming for server-originated warnings.
Changes:
- Bound the in-memory crash-report annotations: libOBS general log tail (150), server warnings (50), and retained “last actions” (50).
- Removed unused OBS log error/warning buffers and dead breadcrumbs plumbing; simplified log retrieval to the only used path.
- Renamed ambiguous
warningstoserverWarningsand updated the Sentry annotation key to clearly distinguish server-detected anomalies from libOBS warning logs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| obs-studio-server/source/util-crashmanager.h | Removes dead public APIs (breadcrumbs / log type enum) and updates crash-manager interfaces for the new bounded buffers. |
| obs-studio-server/source/util-crashmanager.cpp | Implements bounded server warnings, simplifies OBS log annotation, removes breadcrumbs annotation, and documents crash annotations. |
| obs-studio-server/source/nodeobs_service.cpp | Updates call sites to record server warnings via the renamed API. |
| obs-studio-server/source/nodeobs_api.h | Simplifies LogReport storage to a bounded general-log deque and updates the exposed accessor. |
| obs-studio-server/source/nodeobs_api.cpp | Updates internal logging to the new LogReport::push API and removes unused log buffer accessors. |
Comments suppressed due to low confidence (1)
obs-studio-server/source/util-crashmanager.cpp:1039
- This function uses std::reverse but the translation unit does not include . It currently relies on transitive includes, which is brittle; please include explicitly.
std::reverse(result.begin(), result.end());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1067
to
1069
| std::lock_guard<std::mutex> lock(messageMutex); | ||
| for (auto &msg : serverWarnings) | ||
| result.push_back(msg); |
Comment on lines
1032
to
1036
| auto &general = OBS_API::getOBSLogGeneral(); | ||
| while (!general.empty()) { | ||
| result.push_back(general.front()); | ||
| general.pop_front(); | ||
| } |
|
|
||
| // Annotations attached to the Sentry minidump: | ||
| // "OBS log general" — rolling tail of the libOBS log (capped at LogReport::MaximumMessages lines) | ||
| // "Last actions" — recent IPC calls received by the server (capped at MaximumActionsRegistered) |
| static const std::vector<std::string> &getOBSLogErrors(); | ||
| static const std::vector<std::string> &getOBSLogWarnings(); | ||
| static std::queue<std::string> &getOBSLogGeneral(); | ||
| static std::deque<std::string> &getOBSLogGeneral(); |
The Errors and Warnings branches of RequestOBSLog were never invoked — HandleCrash only requests General. The backing deques were filled on every blog() call but never read, just kept around as dead state. Collapse the OBSLogType enum, drop getOBSLogErrors/getOBSLogWarnings, simplify LogReport::push to a single deque, and reduce RequestOBSLog to its one real path.
The static "warnings" vector and AddWarning/ComputeWarnings shared a name with libOBS LogReport::warnings (now removed) but held a different thing: programmatic notes from the server about file-open failures, encoder failures, and IPC error returns. Rename them to serverWarnings/AddServerWarning/ComputeServerWarnings and the Sentry annotation key from "Warnings" to "Server warnings" so a reader can tell at a glance these are server-detected anomalies, not log lines. Cap the deque at 50. ProcessPostServerCall fires AddServerWarning on every IPC error return, so a noisy session could previously grow it without bound.
AddBreadcrumb, ClearBreadcrumbs, ComputeBreadcrumbs, and the breadcrumbs static vector were public but never called anywhere — the "Breadcrumbs" Sentry annotation was always an empty list. Drop the unused functions, the vector, and the empty annotation.
Spell out what each "log-shaped" annotation represents and where its cap lives, so a future reader doesn't need to chase three files to understand what ends up in a crash report.
|
|
||
| break; | ||
| } | ||
| auto &general = OBS_API::getOBSLogGeneral(); |
Collaborator
There was a problem hiding this comment.
RequestOBSLog() used OBS_API::logReport.general through a mutable reference without synchronizing with node_obs_log(), which appends to the same deque under logMutex. That leaves a data race/UB risk during crash annotation collection. Please move this behind an OBS_API helper that snapshots and optionally clears the log while holding logMutex, preferably with try_lock behavior for crash-time use.
- RegisterAction: pop on size() > Maximum so it truly retains 50 (was >=, capping at 49); matches AddServerWarning and the PR description. - ComputeServerWarnings: try_to_lock messageMutex instead of a blocking lock_guard. The crashing thread may already hold it (crashed inside AddServerWarning/RegisterAction); a blocking acquire would hang crash reporting. Return empty if it can't be acquired. - Replace OBS_API::getOBSLogGeneral() (returned a mutable deque ref that RequestOBSLog drained with no synchronization, racing node_obs_log()) with snapshotOBSLogGeneral(): copies and clears logReport.general under logMutex via try_to_lock. RequestOBSLog now consumes the snapshot, reverse-iterating to keep the newest-first ordering. - Include <algorithm> explicitly (std::transform was relying on a transitive include). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Description
Summary
OBS log general(150),Server warnings(50),Last actions(50). Two were previously unbounded.OBSLogType::Errors/Warningspaths and the never-called breadcrumbs code; rename ambiguouswarnings→serverWarningsso it doesn't collide with the libOBS log warning stream.Types of changes
Checklist: