Return report id from user reports #709
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the user exception reporting APIs to return report IDs, enabling callers to track and reference specific crash reports. The change affects both the C and Objective-C APIs by modifying return types from void to int64_t.
Key Changes:
- Modified C API
kscrash_reportUserException()to returnint64_treport ID instead ofvoid - Updated Objective-C API
reportUserException:...method to returnint64_treport ID - Added infrastructure to capture report IDs before generating reports
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Sources/KSCrashRecording/include/KSCrashC.h | Updated C API function signature and documentation to return report ID |
| Sources/KSCrashRecording/include/KSCrash.h | Updated Objective-C API method signature and documentation to return report ID |
| Sources/KSCrashRecording/Monitors/KSCrashMonitor_User.h | Updated internal monitor API signature to return report ID |
| Sources/KSCrashRecording/Monitors/KSCrashMonitor_User.c | Implemented report ID capture and return logic in user monitor |
| Sources/KSCrashRecording/KSCrashReportStoreC.c | Added utility function to get next report ID without incrementing |
| Sources/KSCrashRecording/KSCrashReportStoreC+Private.h | Added declaration for new report ID utility function |
| Sources/KSCrashRecording/KSCrashC.c | Updated C API implementation to capture and return report ID |
| Sources/KSCrashRecording/KSCrash.m | Updated Objective-C API implementation to capture and return report ID |
Comments suppressed due to low confidence (1)
Sources/KSCrashRecording/include/KSCrash.h:161
- This is a breaking change to the public Objective-C API. The method signature change from
voidtoint64_treturn type will break all existing call sites and change the method's Swift interface from returningVoidto returningInt64.
- (int64_t)reportUserException:(NSString *)name
reason:(nullable NSString *)reason
language:(nullable NSString *)language
lineOfCode:(nullable NSString *)lineOfCode
stackTrace:(nullable NSArray *)stackTrace
logAllThreads:(BOOL)logAllThreads
terminateProgram:(BOOL)terminateProgram;
| int64_t kscrash_reportUserException(const char *name, const char *reason, const char *language, const char *lineOfCode, | ||
| const char *stackTrace, bool 10BC0 logAllThreads, bool terminateProgram); |
There was a problem hiding this comment.
This is a breaking change to the public C API. The function signature change from void to int64_t return type will break all existing call sites that don't expect a return value.
Copilot generated this review using guidance from repository custom instructions.
There was a problem hiding this comment.
The machine is absolutely right
There was a problem hiding this comment.
I even put it in the summary :)
| - (void)reportUserException:(NSString *)name | ||
| reason:(nullable NSString *)reason | ||
| language:(nullable NSString *)language | ||
| lineOfCode:(nullable NSString *)lineOfCode | ||
| stackTrace:(nullable NSArray *)stackTrace | ||
| logAllThreads:(BOOL)logAllThreads | ||
| terminateProgram:(BOOL)terminateProgram; | ||
| - (int64_t)reportUserException:(NSString *)name | ||
| reason:(nullable NSString *)reason | ||
| language:(nullable NSString *)language | ||
| lineOfCode:(nullable NSString *)lineOfCode | ||
| stackTrace:(nullable NSArray *)stackTrace | ||
| logAllThreads:(BOOL)logAllThreads | ||
| terminateProgram:(BOOL)terminateProgram; |
There was a problem hiding this comment.
This is a breaking change as well
| * | ||
| * @return The report ID of the next report. | ||
| */ | ||
| int64_t kscrs_getNextCrashReportId(void); |
There was a problem hiding this comment.
Because of that, won't we get a TOCTOU race?
| if (!g_isEnabled) { | ||
| KSLOG_WARN("User-reported exception monitor is not installed. Exception has not been recorded."); | ||
| return; | ||
| return -1; |
There was a problem hiding this comment.
-1 is not very Swift-friendly. What else could we come up with that would bridge nicely to Swift?
|
Don't we already get the report ID from KSCrashDidWriteReportCallback? If they need to distinguish from other reports in the callback: The entire report write and callback process is single-threaded (and always will be), so you could do a wrapper function like this:
Then no matter how or when we generate a report ID in future, you'll always get the correct one. |
That's sort of how I'm doing it now, but it's really cumbersome. I Kindof expected this to not pass the smell test, but I find it would be a great improvement. I'll just continue with how I'm already doing it instead of spending time on this. |
Summary
This PR enhances the user report functionality by returning the report ID when creating user reports, allowing
callers to track and reference specific reports.
Changes Made
API Updates
kscrash_reportUserException()to returnint64_treport ID instead ofvoidreportUserException:reason:language:lineOfCode:stackTrace:logAllThreads:terminateProgram:to returnint64_treport ID
kscm_reportUserException()in the user monitor to return report IDImplementation Details
kscrs_getNextCrashReportId()function to get the next report ID without incrementing the counter0if the user monitor is not installed or if immediate exit is requiredPossibly Breaking Changes
This is a breaking change for existing callers of the user exception reporting APIs, as the return type has
changed from
voidtoint64_t.