Skip to content

fix: remove body validation from unclean termination#398

Merged
rvagg merged 4 commits intorvagg/trustless-utilsfrom
rvagg/flaky-unclean
Sep 5, 2023
Merged

fix: remove body validation from unclean termination#398
rvagg merged 4 commits intorvagg/trustless-utilsfrom
rvagg/flaky-unclean

Conversation

@rvagg
Copy link
Member

@rvagg rvagg commented Aug 24, 2023

While doing #386 I discovered that Go's HTTP client, once it discovers an unclean end, will immediately decide that the body shouldn't be read. So you have up until the moment of it discovering the unclean end to read the body, after that point you get an EOF.

I wish I saved the Go src where I found this, it might be worth me going back and finding it to confirm that I'm talking sense here.

I believe this explains occasional flakies like this one: https://github.com/filecoin-project/lassie/actions/runs/5960306260/job/16167386428?pr=383

The answer is to just not bother attempting to validate bodies in our integration tests when it's an unclean end. I kind of wish it were different because we do want to validate this, but without the ability to control timing, or change this behaviour, I don't believe we can.

@rvagg rvagg marked this pull request as draft August 29, 2023 02:59
@rvagg
Copy link
Member Author

rvagg commented Aug 29, 2023

backburner for the immediate term, I just need to validate what's actually happening here because of a small amount of uncertainty because the internal body reader shouldn't encounter the problem while you're streaming data out until it reached the bad chunk, so it probably depends on Read size inside the body reader; I just need to check that and confirm I'm not missing something or that there's an alternative thing/way to test here.

@rvagg rvagg force-pushed the rvagg/flaky-unclean branch 3 times, most recently from 12d0598 to 5beee8c Compare September 5, 2023 05:41
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2023

Codecov Report

Merging #398 (e1aaca9) into rvagg/trustless-utils (1129833) will increase coverage by 0.27%.
Report is 1 commits behind head on rvagg/trustless-utils.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                    Coverage Diff                    @@
##           rvagg/trustless-utils     #398      +/-   ##
=========================================================
+ Coverage                  76.28%   76.55%   +0.27%     
=========================================================
  Files                         85       85              
  Lines                       6296     6296              
=========================================================
+ Hits                        4803     4820      +17     
+ Misses                      1244     1231      -13     
+ Partials                     249      245       -4     

see 7 files with indirect coverage changes

@rvagg rvagg force-pushed the rvagg/flaky-unclean branch from 5beee8c to 54240ac Compare September 5, 2023 06:06
@rvagg rvagg force-pushed the rvagg/flaky-unclean branch from 54240ac to 400e836 Compare September 5, 2023 06:55
@rvagg rvagg changed the base branch from main to rvagg/trustless-utils September 5, 2023 07:06
@rvagg rvagg marked this pull request as ready for review September 5, 2023 11:34
@rvagg
Copy link
Member Author

rvagg commented Sep 5, 2023

I think this is as good as it's going to get - instead of removing the body validation, this does more careful body slurping, inching up to the expected error so we get the full body and the error. Seems to be good, after many CI reruns.

@rvagg rvagg merged commit 77c9ced into rvagg/trustless-utils Sep 5, 2023
@rvagg rvagg deleted the rvagg/flaky-unclean branch September 5, 2023 23:38
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.

3 participants