Bound Sentry crash-report buffers and drop dead code#1708
Open
summeroff wants to merge 5 commits into
Open
Conversation
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.
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(); |
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: