fix: move the Windows C++ redistributable warning so it is only shown if there is an actual access violation#6508
Conversation
🦋 Changeset detectedLatest commit: aef71c3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-wrangler-6508You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6508/npm-package-wrangler-6508Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-wrangler-6508 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-create-cloudflare-6508 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-cloudflare-kv-asset-handler-6508npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-miniflare-6508npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-cloudflare-pages-shared-6508npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-cloudflare-vitest-pool-workers-6508npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-cloudflare-workers-editor-shared-6508npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10419848511/npm-package-cloudflare-workers-shared-6508Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
57d464f to
aef71c3
Compare
| return /CODE_MOVED for unknown code block/.test(chunk); | ||
| }, | ||
| isAccessViolation(chunk: string) { | ||
| return chunk.includes("access violation;"); |
There was a problem hiding this comment.
I think we should be a bit more specific here
| return chunk.includes("access violation;"); | |
| return /Received structured exception .+? access violation;/.test(chunk); |
There was a problem hiding this comment.
I thought about making it more strict but I am more concerned about false negatives than false positives in this case. And making it too strict might mean that we miss this going forward if the messaging changes slightly.
This check is already gated on there being a stack trace which implies a catastrophic workerd error already.
| logger.debug(chunk); | ||
| else { | ||
| logger.debug(chunk); | ||
| } |
There was a problem hiding this comment.
This was intended to make sure we never forget to log to the debug stream.
nit but now we have to remember to call logger.debug(chunk) in every branch added above
There was a problem hiding this comment.
Yeah I realise that was the case. But actually the way the code was laid out (especially with the IGNORABLE comment it wasn't immediately apparent that this logger.debug() would be called for all branches above, which is actually probably why we had the comment to explain that we were falling out of the branch above in the first place.
I think it is reasoanable for each case to decide whether or not it wants to also log the original error.
What this PR solves / how to test
Replaces #6471, which was too verbose.
Fixes #6170
Author has addressed the following