Conversation
elramen
force-pushed
the
elramen-minidump-outcomes
branch
2 times, most recently
from
bab71e8 to
3ba99b0
Compare
elramen
force-pushed
the
elramen-minidump-outcomes
branch
from
3ba99b0 to
c055532
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
elramen
force-pushed
the
elramen-minidump-outcomes
branch
from
ae96e45 to
d4025f1
Compare
| let item_type = DiscardItemType::Attachment(DiscardAttachmentType::Minidump); | ||
| let _ = items.reject_err(Outcome::Invalid(DiscardReason::TooLarge(item_type))); | ||
| return Err(BadStoreRequest::Overflow(item_type)); |
There was a problem hiding this comment.
Since ownership of the items is not passed here, I would also not reject it, this pattern makes it easy to emit outcomes twice accidentally. In practice Managed has an internal guard against that, but the intention was more to guard against bugs.
How does it look like if we return a better error here, which implements OutcomeError and then the caller can use reject on the result. Like here.
Also fine to explore in a follow-up but in general I think we should stick to that pattern (maybe should force it with making reject_err take &mut self).
There was a problem hiding this comment.
Good point, I'll look into it!
| let prosperodump_idx = items | ||
| .iter() | ||
| .position(|item| item.attachment_type() == Some(AttachmentType::Prosperodump)) | ||
| .ok_or(BadStoreRequest::MissingProsperodump) | ||
| .inspect_err(|_| { | ||
| let _ = items.reject_err(Outcome::Invalid(DiscardReason::MissingProsperodumpUpload)); | ||
| })?; | ||
| let payload = items[prosperodump_idx].payload(); | ||
| validate_prosperodump(&payload, &items)?; | ||
| items.modify(|items, _| { | ||
| items[prosperodump_idx].set_payload(OctetStream, payload); | ||
| }); |
There was a problem hiding this comment.
Can't this be one big try_modify which does the error handling and you don't need to maintain an index?
We just might need better errors that implement OutcomeError or implement it on BadStoreRequest, although I'd prefer more and better scoped errors which then convert to BadStoreRequest.
Relay could use some better error handling in general.
| let envelope = items.map(|items, records| { | ||
| if items.iter().any(|i| i.creates_event()) { | ||
| records.modify_by(DataCategory::Error, 1); | ||
| } | ||
| Box::new(Envelope::from_request(Some(path.event_id), meta).with_items(items)) |
There was a problem hiding this comment.
Steered away from a common envelope-creation function because of the complexity of keeping records correct in all cases. Opened #5949 to address this
| // Doing these operations does not make sense if we already streamed the minidump to objectstore. | ||
| if !minidump_item.is_attachment_ref() { | ||
| let payload = minidump_item.payload(); | ||
| if !items[minidump_idx].is_attachment_ref() { |
There was a problem hiding this comment.
As discussed offline, this should become easier if you do the .find() inside a try_modify.
…men-minidump-outcomes
![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 aa1370c. Configure here.
| }; | ||
| Some(Outcome::Invalid(discard_reason)) | ||
| } | ||
| } |
There was a problem hiding this comment.
MissingProsperodump missing from to_outcome match
High Severity
BadStoreRequest::to_outcome() maps MissingMinidump to DiscardReason::MissingMinidumpUpload but has no arm for MissingProsperodump, causing it to fall through to the _ => DiscardReason::Internal wildcard. The playstation endpoint relies on this mapping via .reject(&items)?, so missing prosperodump requests will emit "internal" instead of "missing_prosperodump_upload" as the discard reason.
Reviewed by Cursor Bugbot for commit aa1370c. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| let discard_reason = match self { | ||
| Self::InvalidCompressionContainer(_) => DiscardReason::InvalidCompression, | ||
| Self::InvalidMinidump => DiscardReason::InvalidMinidump, | ||
| Self::InvalidProsperodump => DiscardReason::InvalidProsperodump, | ||
| Self::MissingMinidump => DiscardReason::MissingMinidumpUpload, | ||
| Self::Overflow(item_type) => DiscardReason::TooLarge(*item_type), | ||
| _ => DiscardReason::Internal, | ||
| }; | ||
| Some(Outcome::Invalid(discard_reason)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: The to_outcome() function doesn't handle BadStoreRequest::MissingProsperodump, causing it to report DiscardReason::Internal instead of the more specific DiscardReason::MissingProsperodumpUpload.
Severity: MEDIUM
Suggested Fix
Add a match arm to the to_outcome() function to map BadStoreRequest::MissingProsperodump to DiscardReason::MissingProsperodumpUpload. This will ensure that missing prosperodump files are categorized correctly.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/endpoints/common.rs#L127-L139
Potential issue: The `to_outcome()` function is missing a match arm for the
`BadStoreRequest::MissingProsperodump` error. When a PlayStation crash upload request is
missing the required `prosperodump` file, this error is generated. However, instead of
being mapped to the specific `DiscardReason::MissingProsperodumpUpload`, it falls
through to the wildcard `_` arm, which incorrectly reports the outcome as
`DiscardReason::Internal`. This miscategorizes a client-side upload error as an internal
server error, hindering correct monitoring of PlayStation crash reports.
Also affects:
relay-server/src/endpoints/playstation.rs:195relay-server/src/services/outcome.rs:381
Did we get this right? 👍 / 👎 to inform future reviews.
3 participants
Use correct discard reasons for outcomes across multipart endpoints. Also refactor from
Vec<Managed<Item>>toManaged<Items>to simplify "all-item"-operations; albeit with a tradeoff of less ergonomic operations on individual items (see the indexing used to access and modify the minidump and proserpodump).