remarkjs / remark-lint-no-dead-urls

lint rule to warn when URLs are dead
https://unifiedjs.com
MIT License
78 stars 13 forks source link

Intermittent false negatives (silently exiting without any output) #53

Closed karlhorky closed 2 months ago

karlhorky commented 2 months ago

Initial checklist

Affected packages and versions

remark-cli@12.0.1, remark-lint@10.0.0, remark-lint-no-dead-urls@2.0.0

Link to runnable example

https://github.com/karlhorky/repro-remark-lint-no-dead-urls-intermittent-silent-failures

Steps to reproduce

  1. Run the linting script multiple times in a row with the .mdx content in https://github.com/karlhorky/repro-remark-lint-no-dead-urls-intermittent-silent-failures/blob/main/src/pages/a/c/y.mdx
  2. 💥 Observe that the run results are different, with some of the runs exiting with code 0 silently without any output, indicating some kind of silent crash, see test run (the 1st run exits without any output, the 2nd run fails as it should): Screenshot 2024-09-29 at 14 26 12

Originally investigated in https://github.com/remarkjs/remark-lint-no-dead-urls/issues/52

Runtime: Latest Node.js LTS (v20.17.0)

Expected behavior

remark-lint-no-dead-urls checks all URLs in all specified files and shows the results

Actual behavior

remark-lint-no-dead-urls exits with code 0 and no output in some runs

Runtime

Other (please specify in steps to reproduce)

Package manager

pnpm

OS

Linux

Build and bundle tools

No response

wooorm commented 2 months ago

Thanks, I can reproduce!

wooorm commented 2 months ago

I checked which URLs failed, out of the 50. The 4 ones that never came back were to https://www.youtube.com/. There were in total 6 connections to https://www.youtube.com/. That seems to indicate that youtube does some DDoS prevention

wooorm commented 2 months ago

It seems to be that response.text() just hangs:

https://github.com/wooorm/dead-or-alive/blob/42d293a850870b507862c6ae318501a6fd2858be/lib/index.js#L329

Seems to be related to the signal. No hang happens without an abort signal:

https://github.com/wooorm/dead-or-alive/blob/42d293a850870b507862c6ae318501a6fd2858be/lib/index.js#L210

wooorm commented 2 months ago

I’m thinking there’s some race condition in play as well. Perhaps YouTube is, when it detects many requests, slow in sending the HTML body. The request went fine but the body is slow and then aborted. This may be an undici bug though, that it just aborts silently, instead of throwing. No clue.

github-actions[bot] commented 2 months ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

wooorm commented 2 months ago

released in dead-or-alive@1.0.3; npm update should do the trick.

karlhorky commented 2 months ago

Hmm, tried dead-or-alive@1.0.3 now in my reproduction repo in a PR, and it still silently exited in the first step (the rest of the steps reported each a consistent 15 warnings):

This PR also adds || true to enable running the same command multiple times without the workflow run failing.

Maybe still intermittently failing for another reason?

I can reduce the reproduction to simplify if it would help.

Screenshot 2024-09-30 at 18 41 09

ChristianMurphy commented 2 months ago

I can reduce the reproduction to simplify if it would help.

Please do, thanks!

karlhorky commented 2 months ago

I simplified the repro now:

Screenshot 2024-10-01 at 10 59 45

Failed the first time, but then after merging to the dead-or-alive@1.0.3 branch, it's harder to reproduce (a few test runs now without the silently exiting behavior).

It's not dead-or-alive@1.0.3 that fixed this though, because I reproduced the behavior with dead-or-alive@1.0.3 above with the more complex repro.

So maybe:

  1. The extra complexity in the reproduction also caused another, unrelated situation that causes remark-lint-no-dead-urls to fail
  2. It's a behavior of another of the services (eg. maybe archive.is - I see a 429 response there in the screenshot above) that only happens the first time after some period of inactivity.

I'll try re-running after a few minutes and few hours. And also try adding back parts of the original, more complex reproduction, in case those were indeed relevant.

karlhorky commented 2 months ago

Yeah, it seems like the 2nd file x.mdx was indeed a relevant part of it (or at least, it causes the problem to appear right away):

Interestingly, it is now very consistent in its failures:

Screenshot 2024-10-01 at 11 27 24

karlhorky commented 2 months ago

Hm... one other change that I did while copying the original reproduction back, was this change:

/** @type {import('unified-engine').Preset} */
const config = {
-  plugins: ['remark-mdx', 'remark-lint', 'remark-lint-no-dead-urls'],
+  plugins: [
+    'remark-mdx',
+    [
+      'remark-lint-no-dead-urls',
+      {
+        skipUrlPatterns: [
+          'chrome://',
+          'codesandbox-link://',
+          'embedded-html-codesandbox://',
+          'embedded-css-codesandbox://',

Maybe this omission of 'remark-lint' is the problem? I thought I tested dead-or-alive@1.0.3 with 'remark-lint' in the plugins, but maybe not...

Edit: No, I guess not, still intermittently exiting silently with 'remark-lint' in plugins:

Screenshot 2024-10-01 at 11 37 06

wooorm commented 2 months ago

I did not reproduce anything else. If you want me to spend time on this, can you please please make a tiny reproduction that I can work with? Removing everything unrelated?

karlhorky commented 2 months ago

The dead-or-alive-1.0.3 branch (PR 1 in my reproduction repo) is the smallest reproduction which I have been able to consistently reproduce the problem with.

To me, this seems pretty minimal - but maybe you have another opinion on what should still be simplified here.

It's:

  1. 1 remark config file
  2. 1 npm script for anything I cannot specify in the remark config
  3. 2 MDX files (as mentioned above - 2 files are necessary, it seems)
  4. CI setup to run the npm script

Screenshot 2024-10-01 at 12 10 34

karlhorky commented 2 months ago

@wooorm I'm really not sure what I should change to make this more minimal, but happy to learn if you have some ideas.

Should this issue be reopened, since it's still failing in the same way? Or I could open this as a new issue if that's preferred.

wooorm commented 1 month ago

Here’s how I work through your code:

karlhorky commented 1 month ago

Wow nice, that's a great simplification, thanks for the deep dive!

I think I'm not versed enough on how to use remark-cli flags, and was not aware that remark-lint could be dropped, I thought it was a dependency of remark-lint-no-dead-urls 👀

Next time that I do a repro in any of the remark / unified things, I'll be sure to try out simpler combinations of packages using these steps.

karlhorky commented 1 month ago

I can confirm that the silent exiting behavior is no longer reproducible in my first 5 test runs, after I upgraded to remark-lint-no-dead-urls@2.0.1 in my reproduction repo:

Screenshot 2024-10-08 at 18 06 38

karlhorky commented 1 month ago

Upgrading to remark-lint-no-dead-urls@2.0.1 in the monorepo with 147 MDX files, I noticed timeouts with v2:

I'll see if this continues, and if it does, I'll try to find time to create a minimal repro.