Repository navigation
Conversation
Currently, when a testcase's fuzzer no longer exists, preprocessing closes the testcase but then re-raises InvalidFuzzerError. The bot only drops tasks on InvalidTestcaseError, so the task message is never acked and gets redelivered until it expires, roughly a thousand failures per such testcase. With this change, both errors derive from a new NonRetryableError (still an errors.Error, so metric labels are unchanged), and the bot drops tasks on any NonRetryableError, as suggested on b/578100711. This also covers corpus pruning tasks whose engine fuzzer is missing.
94588b6 to
0ed17ac
Compare
IvanBM18
left a comment
IvanBM18
left a comment
There was a problem hiding this comment.
LGTM, but, I would have this running a day in dev first and would use pitter to verify the before/after of this changes to see if everything is 'the same'.
To test this changes in the dev branch:
- Ask internally in the chat with GOSST if anyone is using dev, dev is a shared CF instance so we have to ask if no one else is running anything there before submitting changes to avoid from breaking someone else tests
- If dev is free, sync dev with master, commit your changes in dev & push them
- In around 20-30 min your changes should be reflected there
- You can verify which clusterfuzz version your bots are using by filtering the logs with jsonPayload.extras.clusterfuzz_version : '<first 5 chars of commit ID>`
dylanjew
added
the
ready for GOSST review
g-ortuno
left a comment
g-ortuno
left a comment
There was a problem hiding this comment.
Non-blocking: Curious if you looked into the existing dont_retry mechanism. From a quick check it seems like we have two ways of doing the same thing and it might be worth aligning on a single one eventually. Might be worth opening a bug and leaving a TODO.
| class InvalidTestcaseError(Error): | ||
| class NonRetryableError(Error): | ||
| """Error after which retrying the task cannot succeed, so the bot should | ||
| drop the task rather than leave it to be redelivered.""" |
There was a problem hiding this comment.
Nit: Maybe worth calling out that any tasks that throw this error will be acked and lost:
"only use for conditions that are truly permanent; the task message will be acked and lost."
|
Thanks for working on this! |
Currently, when a testcase's fuzzer no longer exists, preprocessing closes the testcase but then re-raises
InvalidFuzzerError. The bot only drops tasks onInvalidTestcaseError, so the task message is never acked and gets redelivered until it expires, roughly a thousand failures per such testcase.With this change, both errors derive from a new
NonRetryableError(still anerrors.Error, so metric labels are unchanged), and the bot drops tasks on anyNonRetryableError, as suggested on b/578100711. This also covers corpus pruning tasks whose engine fuzzer is missing.This came out of an investigation of b/578100711 (done with AI help; report: https://artifacts.corp.google.com/RXlQ1373Sq). It's a draft for the owners of b/543379729 to weigh: if I'm reading the code right this is the smallest change that stops the redelivery loop, but you know better whether dropping these tasks has side effects I'm missing, or whether you'd rather handle it differently.