Skip to content

Fix/worker startup timeout - #706

Merged
synweap15 merged 6 commits into
Bogdanp:masterfrom
guedesfelipe:fix/worker-startup-timeout
May 15, 2025
Merged

Fix/worker startup timeout#706
synweap15 merged 6 commits into
Bogdanp:masterfrom
guedesfelipe:fix/worker-startup-timeout

Conversation

@guedesfelipe

@guedesfelipe guedesfelipe commented May 8, 2025

Copy link
Copy Markdown
Contributor

@guedesfelipe

guedesfelipe commented May 8, 2025

Copy link
Copy Markdown
Contributor Author

image
I tested it locally and it's working fine.

@synweap15

@guedesfelipe
guedesfelipe force-pushed the fix/worker-startup-timeout branch from e0d057d to fa8f7fe Compare May 8, 2025 15:30
@synweap15

Copy link
Copy Markdown
Collaborator

hey @guedesfelipe, the tests are now timing out. I have confirmed the behavior in my local environment and lack of this behavior on current master.

Github Actor logging outputs only completed lines, but the issue is in test_cli.py, test_cli_fork_functions_have_no_blocked_signals test.

image

@guedesfelipe

Copy link
Copy Markdown
Contributor Author

@guedesfelipe

Copy link
Copy Markdown
Contributor Author

I think the error happens because of the parameter name starting with fork, I changed it to timeout-fork and the test passed:

image

@synweap15 Do you have a better name suggestion for the parameter?

@synweap15

synweap15 commented May 12, 2025

Copy link
Copy Markdown
Collaborator

Indeed it caused the parameter name to be ambigious. Good find.

I suggest that:

  • the parameter gets renamed to --worker-fork-timeout
  • move it a few lines up, so it appears above --worker-shutdown-timeout argument definition - so then it's in logical order of fork function -> worker-fork-timeout -> worker-shutdown timeout
  • I think I would change the value from seconds to miliseconds - the use of miliseconds is consistent throughout the codebase.

@guedesfelipe

Copy link
Copy Markdown
Contributor Author

Make sense! I'll work on it and update the MR ASAP.

@synweap15

Copy link
Copy Markdown
Collaborator

@guedesfelipe could you please address the lint fail?

@guedesfelipe

Copy link
Copy Markdown
Contributor Author

Done @synweap15

@synweap15

Copy link
Copy Markdown
Collaborator

@guedesfelipe looking good, thanks!

@synweap15
synweap15 merged commit a3c1d5f into Bogdanp:master May 15, 2025
@guedesfelipe

Copy link
Copy Markdown
Contributor Author

Great @synweap15! Do you have any idea when the next release will be out?

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.

2 participants