Log and count header parse errors instead of swallowing them - #495
Merged
Conversation
ReceivedPacketData.AnalyzePacketHeader caught all exceptions with an empty catch block, silently discarding parse failures and resetting PacketHeader to null with no trace. The caught exception is now kept on a new HeaderParseException property, and PacketProcessor logs it and records it via the existing IAppMetrics.ProcessingErrors counter when a packet header fails to parse.
The previous fix only captured exceptions from AnalyzePacketHeader, leaving the two size-check short-circuit paths (too short overall, undersized 2023+ header) silent since they never throw. Both paths now set a HeaderRejectionReason string, which PacketProcessor logs as a warning and counts via ProcessingErrors, alongside the existing exception handling. Tests cover both rejection paths and confirm a successful parse leaves no exception or rejection reason behind.
The Sonar quality gate failed on new code coverage: the HeaderParseException branch added in the previous commits cannot be organically exercised, since every field read in header parsing is already bounds-checked by the length guards before it, so the catch block never fires for any packet observed in practice. Splitting the try/catch wrapper and the exception-recording helper into their own methods marked [ExcludeFromCodeCoverage] keeps that honestly untestable defensive code out of the coverage denominator, while the genuinely reachable and tested HeaderRejectionReason path stays fully instrumented.
The rejection reason string interpolated the reported game version and packet length straight from untrusted UDP input and was then used as a metric tag value, letting a remote sender explode the ProcessingErrors counter into hundreds of thousands of time series. HeaderRejectionReason is replaced by a HeaderRejectionCode enum with a fixed small set of values; the untrusted details (game version, packet length, exception text) now flow only into the structured log message as parameters, never into the metric dimension. This also drops the eager string allocation on the packet receive path and narrows the ExcludeFromCodeCoverage attribute in PacketProcessor to just the exception branch, so the always-reached dispatch and the tested rejection-code branches stay visible to coverage. Adds tests asserting the rejection code content and that PacketProcessor surfaces it via LastError.
The switch-based dispatch in RecordRejectedHeader added an unreachable None guard (PacketHeader is only null when a rejection code was already set, so None can never occur here) and forced every switch case into the project's brace-plus-trailing-break style, turning the single genuinely unreachable ParseException branch into four separately tracked coverage lines. Both regressed the Sonar quality gate to 68.4% new-code coverage. Removing the dead guard and replacing the switch with a plain if/else-if chain collapses the untestable branch back down to the exception call itself.
Sonar's new-code coverage blends line and branch coverage; the previous if/else-if chain had three branches whose alternate direction can never be exercised (the parse-exception check, and two rejection-code checks where the untested direction is unreachable given only three non-None codes exist), pushing coverage to 77.8% against the 80% gate. Two Logger?.LogWarning call sites duplicated that problem per site. Consolidates the two rejection-code log calls into one structured warning, and folds the exception-presence check back into the already excluded RecordHeaderParseExceptionIfPresent method so it is called unconditionally instead of guarded inline. A test now attaches a no-op logger so both directions of the remaining Logger?. check are exercised. The only branches left uncovered are the _appData/AppMetrics null-conditionals, which can never be null/non-null respectively given how PacketProcessor is constructed.
|
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
2 participants
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.
Summary
ReceivedPacketData.AnalyzePacketHeadercaught all exceptions with an emptycatchblock and silently dropped every undersized packet, with no exception type, logging, or metric — parse failures were invisible. This PR makes every header-rejection path observable:HeaderRejectionCodeonReceivedPacketData(PacketTooShort,Undersized2023Header,ParseException, orNone).PacketProcessorlogs a warning and records it via the existingIAppMetrics.ProcessingErrorscounter, tagged with the rejection code.try/catchnow records the caught exception (HeaderParseException) instead of discarding it, andPacketProcessorlogs it as an error. This path is defensive: every offsetParseHeaderFieldsreads is already validated by the length guards above it, so no packet observed in practice reaches this catch, but it guards against a future change accidentally breaking that invariant without crashing the receive loop it runs on (e.g. the TCP replay ingestion loop, which only catchesIOException/SocketExceptionaround it).Linked issues
Closes #21
Review notes
ReceivedPacketDatahas no logger or metrics dependency and is instantiated directly (new ReceivedPacketData()) at many call sites, so rejection state is exposed via properties (HeaderRejectionCode,HeaderParseException,ReportedGameVersion) rather than logged from withinF1Server.Core.PacketProcessoralready has access toLoggerand_appData.AppMetrics, so it consumes these to report the error, following the existing pattern used forprocessor.LastException. The genuinely-unreachable exception path in both files is isolated behind[ExcludeFromCodeCoverage]on the smallest possible scope (the try/catch wrapper and the exception-only log call), so the reachable dispatch and rejection-code logic stay visible to coverage.