Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rm fallback from submit v2.1 mutation #6778

Closed
wants to merge 1 commit into from

Conversation

LinHuiqing
Copy link
Contributor

@LinHuiqing LinHuiqing commented Oct 5, 2023

Problem

Fallback for virus can mutation catches all errors. This is an issue as when malicious files are scanned, the fallback is triggered as well. This bug was identified by @KenLSM in #6777.

Solution

There are many points of failure for the mutation to fail. The original intention of the fallback mechanism is to try the submission again if there are any unexpected errors related to the virus scan itself (e.g. if lambda is down, if s3 uploads can't be done, etc). However, these block submissions anyway - the response to this should be to rollback using the growthbook flag.

As such, we want to remove the onError block.

Breaking Changes

  • No - this PR is backwards compatible

Bug Fixes:

  • Malicious file scan triggering retry on submit fetch on storage mode form.

Tests

@LinHuiqing LinHuiqing requested review from KenLSM and tshuli October 5, 2023 07:44
@LinHuiqing LinHuiqing marked this pull request as ready for review October 5, 2023 07:44
@LinHuiqing LinHuiqing marked this pull request as draft October 5, 2023 08:02
@LinHuiqing LinHuiqing assigned LinHuiqing and unassigned LinHuiqing Oct 5, 2023
@tshuli tshuli self-assigned this Oct 5, 2023
@LinHuiqing
Copy link
Contributor Author

LinHuiqing commented Oct 5, 2023

Decided to not remove the fallback for #6777 as soft validation is useful (though not originally intended). Will be revisited!

@tshuli tshuli closed this Feb 2, 2024
@KenLSM KenLSM deleted the fix/rm-virus-scan-fallback branch November 13, 2024 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants