Skip to content
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.

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

fix(windows-agent): Delegates agent.WaitReady to the daemon #982

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

CarlosNihelton
Copy link
Contributor

Since #896 the agent might sometimes close the a.ready channel too early, giving the possibility of a call to a.WaitReady() exiting before the daemon really starts serving, thus not really waiting.

The deamon knows best when is the most likely moment to be serving, thus this PR slightly modifies a.WaitReady() to check with the daemon, which had to implement its own daemon.WaitReady().

The a.ready channel is still useful to synchronize the access to a.daemon, preventing nil pointer access when the deamon is not yet initialized. Checking for nil is still needed, as the a.ready channel may be closed without ever serving, such as on a failure case.

This adds a test for daemon.WaitReady(), but the agent tests don't need to change, as their "flakyness" reveal the problem.


UDENG-4900

This reverts commit eb6c321.
It's just one hunk, but I think reverting a commit is more meaningful.
Till the latest moment possible.
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.18%. Comparing base (aeda615) to head (8cb8854).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
+ Coverage   89.16%   89.18%   +0.02%     
==========================================
  Files         110      110              
  Lines        7493     7498       +5     
==========================================
+ Hits         6681     6687       +6     
- Misses        633      634       +1     
+ Partials      179      177       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The deamon knows best when is the most likely moment to
be serving.
The a.ready channel is still useful to synchronize the access to a.daemon,
preventing nil pointer access when the deamon is not yet initialized.
Checking for nil is still needed, as the a.ready channel may be closed without ever serving.
@CarlosNihelton CarlosNihelton marked this pull request as ready for review November 26, 2024 21:35
@CarlosNihelton CarlosNihelton requested a review from a team as a code owner November 26, 2024 21:35
@CarlosNihelton CarlosNihelton self-assigned this Nov 27, 2024
Copy link
Collaborator

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

This looks the right way to fix it to me. Let’s hope that definitively fixes that flackyness.

I do have one request: do you mind double checking on the systemd "ready" notification side that we are still behaving as expected?

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