Merged
Conversation
Otherwise, it can sometimes remain unclear in the diagnostics, whether it was InstallationV2 or InstallationV2CacheBust that timed out.
The current production logs show two types of verification timeouts: * service_error: "Unhandled Browserless response status: 408" (vast majority of cases) * service_error: :timeout (only a few cases) The latter happens when we hit the Req receive_timeout (endpoint_timeout + 2s). I've seen Browserless not respect the timeout param from time to time, so it's better to keep the timeout logic "in-house" only.
...but still consider them "unhandled" for telemetry, also notifying Sentry and logging the warning.
apata
reviewed
Oct 23, 2025
apata
reviewed
Oct 23, 2025
apata
reviewed
Oct 23, 2025
apata
reviewed
Oct 23, 2025
apata
reviewed
Oct 23, 2025
apata
reviewed
Oct 23, 2025
Contributor
apata
left a comment
apata
left a comment
There was a problem hiding this comment.
All good with the new functionality. The app-side refactors are good, though I think there's two potential spots to discuss (whether we still want a Browserless-side timeout and whether we need two different Sentry messages).
Regarding refactored tests, I left a few non-blocking nitpicks which are probably a matter of personal preference. Ideally, though, we'd come to some sort of agreement what's good enough so we don't end up doing too many stylistic test refactors.
apata
approved these changes
Oct 23, 2025
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
2 participants
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.
Changes
NOTE: Reviewing commit by commit recommended
The only customer facing functional change
When the Browserless request responds with an unexpected status code (400 | 429), or the request times out, we'll show this to the customer:
Observability changes
Add the check module name into the
service_errorwhen our own Elixir check timeout kicks in. This makes it clearer in the logged diagnostics, whether it was cache bust or the initial installation_v2 check that timed out.Set the Req
receive_timeoutoption to the currentendpoint_timeoutwhich gets passed to the /function API request. With this we should stop getting 408s from Browserless. See this commit (description) for more info: 557f18fIn case we arrive to the case displayed in the above screenshot, a warning log + telemetry
:unhandledevent are emitted (current behaviour). With this PR, we'll start notifying Sentry with a new error message that will be separate from the current"Unhandled case for site verification (v2)". These new sentry errors, called"Browserless failure in verification (v2)"will be turned into escalating issues in Sentry, because a certain volume of those events is expected.Refactorings
verification/checks_test.exsmore DRY, also organise them into describe blockscodeandextrakeys, wherecodeshould always be an atom.Tests
Changelog
Documentation
Dark mode