[rajeswaripedaballi]

In this PR, I have:

• Added tests for any new features or bug fixes (if relevant)
• Added my changes to the CHANGELOG.md at the top, under the "Unreleased" section
• Ensured issues that this PR closes will be automatically closed

Reason for this PR

curl sometimes fails with exit code 56 (failure receiving data from peer) while the docassemble server is still starting up. The script had no handling for curl network-layer failures - it was only checking HTTP response codes, so these transient errors could cause flaky CI failures.

Links to any solved or related issues

Fixes #1044

Any manual testing I have done to ensure my PR is working

Discussed the approach with Bryce before implementing. Reviewed the retry loop logic manually to confirm the curl exit code is captured correctly and the loop continues as expected when curl fails.

[BryceStevenWilley]

Some changes that I think are needed to make this work.

[BryceStevenWilley]

With set +e, the script won't automatically fail anymore, so we'll have to still manually exit on non-56 statuses.

Suggested change

	fi
	else
	exit $status
	fi

[plocket]

[Edit: Look further down where I expressed a different sentiment.]

I think the check here is too permissive. Non-0, non-56 error codes should still exit with the error code. I think the logic we're looking for is something like this:

If the status is 0, continue
If the status 56, set the response to "000"
Otherwise, exit with the status code.

Regarding a response code of "000": I admit I don't have a better suggestion for a response code. I think "000" is actually a decent option. I'm not sure this is a valid response code, so we may want to consider that, but I do like that "000" can tell us we created it ourselves. I'll go with whatever you decide here.

[plocket]

Actually, let me propose something different because I do want to log "ALK0015" before exiting if possible. I propose (in pseudo code):

If status == 56, response = "000" # Make sure response has a value
Log ALK0015 # Make sure values are logged
If status != 0 and status != 56, exit with status # Exit if needed

Note that the log comes in the middle - after we make sure response has a value. That way we can always log before exiting.

[BryceStevenWilley]

For what it's worth when working on this I believe @rajeswari1301 ran into at least one other error code that were also transient and related to the server starting up. There are likely to be quite a few of them if we looked through the docs comprehensively.

[plocket]

It's a good point to bring up, though I'm not sure about that error you mentioned specifically (see below). Maybe the question I should be asking is this: are there status errors it would be bad to skip? I'm not familiar with the status errors that could come up in this context. I can't see what could possibly be that highly concerning, though. Famous last words?

If nothing could go significantly wrong, the only down-side is making authors wait a full 2 minutes while their tests churn for no reason. That's not particularly fun, but re-running the tests costs time too. Also, people usually go do other things while tests are running, which means re-trying is probably the bigger time saver in practical terms.

We don't lose the status error because we already log it. We have a time limit on the operation so it won't loop forever. It's a sandbox, so no harm can be done to an author's servers. I can't see how harm could be done to the repo itself either.

Ok, I'm convinced! If we can't find or come up with a serious ramification, let's skip errors. [There are only about 100 and I took a look. See notes below.]

I still like the idea of changing $response on a non-zero status code. What do folks think of changing "000" to "missing"? That reads well in the log and, after some thought, if it's not going to be a valid status code I'm thinking it's worth making it more expressive.

---

Incidental note:

an into at least one other error code that were also transient and related to the server starting up

Was the error this one? If so, it was because the script exited after a non-56 (which was 0) and the server wasn't ready yet. The test then failed when it tried to dainstall.

[plocket]

Also, how do folks feel about adding a comment or a design doc describing why we're ignoring all status code errors. I suspect future me will be surprised that we're being so permissive.

[plocket]

tldr; Go ahead and skip all status codes!

I've read all the errors. The only problem I see is the potential that it's possible that an error code could be useful if 1. we introduce a bug in the code in a future edit or 2. da changes something and it breaks our code. I'm ok with letting those situations resolve after 2 minutes instead of immediately.

[rajeswaripedaballi]

Thanks for the discussion @plocket ! And thanks @BryceStevenWilley for bringing up the other error code -the loop was exiting before ALK0015 even logged, which pointed to a connection side failure during startup, likely exit code 7 (failed to connect)

My approach of skipping all non zero codes was intentional for this reason, there are likely several transient codes that come up during server startup, and since we already log them and have a timeout, skipping them all is safer than trying to list out each one.

Also agree with @plocket 's suggestion to add a comment explaining why we're being permissive that'll definitely help. And glad we landed on skipping all codes, it keeps it simple and your reasoning makes total sense.

Will update:

• "000" to "missing"
• Add a comment explaining the permissive approach
• Update CHANGELOG to mention the permissive handling of all non-zero curl exit codes

[plocket]

Thanks for the changes! I think we need logic that's a little less permissive for the status code check. Other than that, the rest of the items from our discussion are up to you. Great work!

[plocket]

[Edit: Look further down where I expressed a different sentiment.]

I think the check here is too permissive. Non-0, non-56 error codes should still exit with the error code. I think the logic we're looking for is something like this:

If the status is 0, continue
If the status 56, set the response to "000"
Otherwise, exit with the status code.

Regarding a response code of "000": I admit I don't have a better suggestion for a response code. I think "000" is actually a decent option. I'm not sure this is a valid response code, so we may want to consider that, but I do like that "000" can tell us we created it ourselves. I'll go with whatever you decide here.

[plocket]

I'm not opposed to including this additional information, so we can keep this change. I just want to note that this information is logged above on line 231 with log4="💡 ALK0015 INFO: Response is $response (curl exit code: $status)" in case you didn't notice it. By the time this log happens, we will have seen 0015 already. Thank you for the addition of the status code, btw!

[plocket]

When you update the code above to be less permissive, also update this comment to include the detail about exit code 56. Thank you!

[plocket]

Actually, let me propose something different because I do want to log "ALK0015" before exiting if possible. I propose (in pseudo code):

If status == 56, response = "000" # Make sure response has a value
Log ALK0015 # Make sure values are logged
If status != 0 and status != 56, exit with status # Exit if needed

Note that the log comes in the middle - after we make sure response has a value. That way we can always log before exiting.

[BryceStevenWilley]

I think the test failures here are also due to SuffolkLITLab/docassemble-ALToolbox#339, which is breaking everything. Re-running those now, hopefully they should succeed now that fix is pushed to PyPI.

[plocket]

Looks good to me! Thanks for the thorough review process and for reducing the flakiness of the tests. I recommend a squash and merge in this case.

[plocket]

@BryceStevenWilley, I think you need to approve since you requested changes.