Skip to content

Handle curl failures while servers are starting #1046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BryceStevenWilley merged 7 commits into from
Apr 17, 2026
Merged

Handle curl failures while servers are starting #1046

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ea36861
Hnadle curl failures while servers are starting
rajeswaripedaballi Apr 7, 2026
4a40f29
Handle curl failures while servers are starting
rajeswaripedaballi Apr 7, 2026
e4ddbfc
Handle curl failures
rajeswaripedaballi Apr 8, 2026
471e9bc
Fixed early exit by handling non zero curl exit codes
rajeswaripedaballi Apr 8, 2026
25f8feb
Retry all curl failures
rajeswaripedaballi Apr 10, 2026
0d4d04e
Update CHANGELOG.md
rajeswaripedaballi Apr 10, 2026
fb438fe
glob match for 200 instead of exact match
rajeswaripedaballi Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ Format:

<!-- ## [Unreleased] -->

## [Unreleased]

### Fixed
- Retry on curl exit code 56 (failure receiving data from peer) while waiting for docassemble server to start up. See [#1044](https://github.com/SuffolkLITLab/ALKiln/issues/1044).


## [5.15.4] - 2025-11-29

### Changed
Expand Down
18 changes: 12 additions & 6 deletions action_for_github_server/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,28 +207,34 @@ runs:
sleep 60

response="start"
while [[ $response != *"200"* ]]; do
while [[ $response != "200" ]]; do
log2="💡 ALK0013 INFO: Time elapsed: $SECONDS seconds"
echo -e "\n$log2" >> "${{ env.DEBUG_LOGS_PATH }}" && echo -e "$log2"

# Check if max seconds till loading has passed
if [[ $SECONDS -ge $MAX_SECONDS_FOR_DOCKER ]]; then
log3="🤕 ALK0014 ERROR: Timed out waiting for docker container to serve the site. If you want to give your docker container more timem to install, set the MAX_SECONDS_FOR_DOCKER input for this action."
log3="🤕 ALK0014 ERROR: Timed out waiting for docker container to serve the site. If you want to give your docker container more time to install, set the MAX_SECONDS_FOR_DOCKER input for this action."
echo -e "\n$log3" >> "${{ env.DEBUG_LOGS_PATH }}" && echo -e "$log3"
exit 1
fi

# Set the variable to a new string value in each iteration
response=$(curl -o /dev/null -s -w "%{http_code}\n" http://localhost:80/health_check?ready=1)
log4="💡 ALK0015 INFO: Response is $response"
response=$(curl -o /dev/null -s -w "%{http_code}" http://localhost:80/health_check?ready=1)
status=$?

if [[ $status -eq 56 ]]; then
response="000"
fi

@BryceStevenWilley BryceStevenWilley Apr 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 plocket Apr 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 plocket Apr 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 BryceStevenWilley Apr 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 plocket Apr 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 plocket Apr 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 plocket Apr 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rajeswaripedaballi Apr 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


log4="💡 ALK0015 INFO: Response is $response (curl exit code: $status)"
echo -e "\n$log4" >> "${{ env.DEBUG_LOGS_PATH }}" && echo -e "$log4"

# Check if the string includes the desired text
if [[ $response == *"200"* ]]; then
if [[ $response == "200" ]]; then
log5="💡 ALK0016 SUCCESS: Success! The docker container is ready!"
echo -e "\n$log5" >> "${{ env.DEBUG_LOGS_PATH }}" && echo -e "$log5"
else
log6="💡 ALK0017 INFO: The docker container is not ready yet. Will check again in 30 seconds."
log6="💡 ALK0017 INFO: The docker container is not ready yet (HTTP=$response, curl exit code=$status). Will check again in 30 seconds."

@plocket plocket Apr 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

echo -e "\n$log6" >> "${{ env.DEBUG_LOGS_PATH }}" && echo -e "$log6"
sleep 30
fi
Expand Down
Loading