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

feat: conditionally panic-recover #5553

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

dwisiswant0
Copy link
Member

@dwisiswant0 dwisiswant0 commented Aug 21, 2024

Proposed changes

As discussed with @Mzack9999, we should avoid overusing panic-recover. We need to review the RCA first to determine whether this is an exceptional situation or if it's a higher-level function meant to recover from a panic. This approach will help us establish a robust error-handling strategy.

The implementation of panic-recover should be conditional and NOT applied when running in a CI environment AND IS temporary. Once we've caught all errors and made the necessary corrections, we can remove the deferred recover function.

Things to note:

  • Go version upgraded.

Unresolved questions:

  • Does this have any impact on our internal services (for instance, PDCP) or tools that integrate with Nuclei?

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - Panics in GitHub actions should be visible and be investigated

go.mod Outdated Show resolved Hide resolved
Copy link
Member

@tarunKoyalwar tarunKoyalwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implementation lgtm!

Context on existing panic recovery

earlier we removed all recover mechanism that were related to nuclei code , the existing ones currently are to catch third party library ( in this case goja related panics) , but as suggested i think we can safely throw them when running in CI

We kept 3rd party lib panic recovery, because we observed some panics especially in go-rod in very rare cases which didn't directly affect any application logic and wasn't reproducable


Not sure about RCA but if this is a follow-up of #5547 (review) , Go StdLib provides customization when running race detector , we can set halt_on_error and exit_code in GORACE env variable in github build action job/workflow and it will halt and thus job will fail

Screenshot 2024-08-24 at 8 28 46 PM

ref: https://go.dev/doc/articles/race_detector

cc: @Mzack9999

@tarunKoyalwar
Copy link
Member

also build is failing locally for library code , i think this is due to recent change in template signature pr

As discussed with @Mzack9999, we should avoid
overusing panic-recover. We need to review the RCA
first to determine whether this is an exceptional
situation or if it's a higher-level function meant
to recover from a panic. This approach will help
us establish a robust error-handling strategy.

The implementation of panic-recover should be
conditional and NOT applied when running in a CI
environment AND IS temporary. Once we've caught
all errors and made the necessary corrections, we
can remove the deferred recover function.

Signed-off-by: Dwi Siswanto <[email protected]>
@dwisiswant0 dwisiswant0 force-pushed the dwisiswant0/feat/conditionally-panic-recover branch from 42285c4 to 5ebdfef Compare August 24, 2024 23:58
@dwisiswant0 dwisiswant0 force-pushed the dwisiswant0/feat/conditionally-panic-recover branch from 5ebdfef to b4ac2c8 Compare August 25, 2024 00:00
@dwisiswant0
Copy link
Member Author

[...] we can set halt_on_error and exit_code in GORACE env variable in github build action job/workflow and it will halt and thus job will fail

Note that a data race is UB and by default, it cannot be recovered - alias runtime does not raise a panic. So even without adjusting the GORACE, it'll still remain halted though.

The motive is to analyze and later determine whether this recovery is for an "exceptional situation or if it's a higher-level function", and we're making sure to dodge any kind of so-called shadow errors - which, potentially, trigger another panic and the root cause would be from that. So, this is all about correctness.

also build is failing locally for library code , i think this is due to recent change in template signature pr

Rebased. Please try again.

@ehsandeep ehsandeep merged commit e0b2542 into dev Aug 28, 2024
12 checks passed
@ehsandeep ehsandeep deleted the dwisiswant0/feat/conditionally-panic-recover branch August 28, 2024 12:27
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.

4 participants