fix(sentry): Recover object readers after deserialization errors#5293
Draft
fix(sentry): Recover object readers after deserialization errors#5293
Conversation
Recover from deserializer failures by advancing past the broken value instead of retrying the same token forever. This keeps list and map helpers progressing and preserves later valid entries. Track nesting in JsonObjectReader so recovery also works after a partially consumed object or array. Implement skipValue in MapObjectReader and avoid consuming stack entries before type checks so failed reads do not corrupt the remaining input. Fixes GH-5278 Co-Authored-By: Claude <noreply@anthropic.com>
Abort collection recovery gracefully when the stream is already unrecoverable instead of letting recoverValue fail from inside the original error handler. Log an explicit error and stop iterating so malformed or truncated JSON does not leave the reader in a worse state while still allowing the container to close cleanly. Refs GH-5278 Co-Authored-By: Claude <noreply@anthropic.com>
Move the repeated recovery logging flow behind a dedicated helper so the collection readers stay focused on parsing and keep the guarded recovery behavior aligned across all three paths. This does not change behavior. It only removes duplicated error handling around failed value recovery. Refs GH-5278 Co-Authored-By: Claude <noreply@anthropic.com>
Recover MapObjectReader values back to their stack checkpoint so partially consumed nested objects do not leave child entries or end sentinels behind. This keeps later values readable after a deserializer fails mid-object and adds regression coverage for list, map, and map-of-lists paths that partially enter nested values before throwing. Refs GH-5278 Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
Internal Changes 🔧Deps
🤖 This preview updates automatically when you update the PR. |
📲 Install BuildsAndroid
|
Contributor
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d15471f | 310.66 ms | 368.19 ms | 57.53 ms |
| 9054d65 | 330.94 ms | 403.24 ms | 72.30 ms |
| b77456b | 393.26 ms | 441.10 ms | 47.84 ms |
| 96449e8 | 361.30 ms | 423.39 ms | 62.09 ms |
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| a416a65 | 295.53 ms | 373.74 ms | 78.21 ms |
| ab8a72d | 316.24 ms | 356.38 ms | 40.14 ms |
| 1564554 | 323.06 ms | 336.68 ms | 13.62 ms |
| b3d8889 | 371.69 ms | 432.96 ms | 61.26 ms |
| d15471f | 315.20 ms | 370.22 ms | 55.02 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| 9054d65 | 1.58 MiB | 2.29 MiB | 723.38 KiB |
| b77456b | 1.58 MiB | 2.12 MiB | 548.11 KiB |
| 96449e8 | 1.58 MiB | 2.11 MiB | 539.35 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| ab8a72d | 1.58 MiB | 2.12 MiB | 551.55 KiB |
| 1564554 | 1.58 MiB | 2.20 MiB | 635.33 KiB |
| b3d8889 | 1.58 MiB | 2.10 MiB | 535.06 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
Track recovery state per collection element so post-parse validation failures do not cause the next sibling container to be skipped. This keeps the livelock fix while preserving later valid values after a failed object deserializer. Refs GH-5278 Co-Authored-By: Claude <noreply@anthropic.com>
Treat recovery state as consumed when the current value has already been read, including skipValue()-driven failures. This prevents the fallback recovery step from skipping the next valid sibling after a deserializer consumes a value and then throws. Add regressions for direct list recovery and nested map-of-list recovery so consumed-value failures keep later valid entries. Refs GH-5278 Co-Authored-By: Claude <noreply@anthropic.com>
Log a second error when unknown-field recovery itself fails. This makes nextUnknown consistent with the list and map recovery paths and makes truncated payload failures easier to diagnose. Add regression coverage for unknown-key deserialization failures that also leave the stream unrecoverable. Co-Authored-By: Claude <noreply@anthropic.com>
Clarify that collection recovery emits warning logs for skipped values, while unknown-key failures and unrecoverable recovery paths emit errors. This keeps the release note aligned with the actual logging behavior from the recovery fixes. Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
@sentry review |
Member
Author
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d710cca. Configure here.
|
|
||
| @Override | ||
| public int nextInt() throws IOException { | ||
| markValueConsumed(); |
There was a problem hiding this comment.
Premature markValueConsumed defeats recovery for failed primitive reads
Medium Severity
In all primitive read methods (nextInt, nextLong, nextString, nextBoolean, nextDouble, nextNull, nextFloat), markValueConsumed() is called before the underlying Gson call. If the Gson call then throws (e.g., type mismatch), valueConsumed is already true even though the value remains unconsumed in the stream. This causes recoverValue to skip its fallback skipValue() call, leaving the token stuck in the stream and breaking subsequent reads. Contrast with beginObject/beginArray, which correctly call markValueConsumed() after the Gson call succeeds.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d710cca. Configure here.
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.
📜 Description
Fix
JsonObjectReaderandMapObjectReaderso failed value deserialization no longer hangsor corrupts subsequent reads.
The change makes both readers advance past failed values, keeps later valid entries readable,
and adds coverage for partial-consumption and truncated-input recovery paths.
💡 Motivation and Context
Some collection deserialization helpers could get stuck retrying the same value forever when a
nested deserializer threw. In other cases, partial consumption left the reader in an invalid
state and broke later values.
This fixes GH-5278.
💚 How did you test it?
./gradlew spotlessApply apiDump./gradlew :sentry:test --tests="io.sentry.JsonObjectReaderTest" --tests="io.sentry.util.MapObjectReaderTest"unrecoverable truncated-input recovery
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps