Fix race condition: handle already completed processes - #470
Closed
LudovicoRighi wants to merge 4 commits into
Closed
Fix race condition: handle already completed processes#470LudovicoRighi wants to merge 4 commits into
LudovicoRighi wants to merge 4 commits into
Conversation
LudovicoRighi
force-pushed
the
fix-race-timeout
branch
2 times, most recently
from
June 24, 2025 13:33
730a81d to
616111c
Compare
LudovicoRighi
changed the title
LudovicoRighi
force-pushed
the
fix-race-timeout
branch
2 times, most recently
from
June 24, 2025 13:39
2d88334 to
e744eec
Compare
LudovicoRighi
force-pushed
the
fix-race-timeout
branch
from
June 24, 2025 14:37
e744eec to
e25b2de
Compare
gusostow
reviewed
Jun 25, 2025
gusostow
left a comment
gusostow
left a comment
Contributor
There was a problem hiding this comment.
Is this issue specific to timeouts, or for any job that terminated while apsis down. If it's specific to timeouts I don't understand why.
| log.debug("expected final FdData") | ||
| except AgentMessageError as exc: | ||
| if "no process" in str(exc): | ||
| log.info(f"{self.run_id}: could not get final output; process already gone") |
Contributor
There was a problem hiding this comment.
Do you want to swallow all agent message errors, or just the no process ones?
| meta["stop"] = {"signals": [ s.name for s in self.stop_signals ]} | ||
|
|
||
| if res.status.exit_code == 0 and not self.timed_out: | ||
| if res.status.exit_code == 0 and res.status.signal is None: |
Contributor
There was a problem hiding this comment.
So you want timed out jobs that exit 0 to be marked as success? Is this related to the bugfix?
Collaborator
Author
There was a problem hiding this comment.
| ) | ||
| def test_timeout_with_delayed_reconnect_process_completed(job_name, expected_state): | ||
| """ | ||
| Tests that when Apsis is down long enough for a process to complete naturally, |
Contributor
There was a problem hiding this comment.
Maybe describe what the race condition is
Collaborator
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While I was doing some final testing, I noticed this race condition. (See
timeout1job runs in Apsis).I've added another test for this:
without the change
with the change: