Skip to content

fix(agent): handle None response from retry exhaustion to prevent crashes #787

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

Open
gajeshbhat wants to merge 2 commits into
base: main
Choose a base branch
from
+124 −31
Open

fix(agent): handle None response from retry exhaustion to prevent crashes #787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8c8f7e
fix(agent): handle None response from retry exhaustion to prevent cra…
gajeshbhat Aug 26, 2025
9f6be42
test(agent): add status_code branch tests and use @patch decorators
gajeshbhat Feb 9, 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
66 changes: 47 additions & 19 deletions agent/src/testflinger_agent/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,12 +214,20 @@ def post_result(self, job_id, data):
logger.error(exc)
raise TFServerError("other exception") from exc
if not job_request:
logger.error(
"Unable to post results to: %s (error: %d)",
result_uri,
job_request.status_code,
)
raise TFServerError(job_request.status_code)
if job_request is None:
logger.error(
"Unable to post results to: %s (error: %s)",
result_uri,
"No response received",
)
raise TFServerError("No response received")
else:
logger.error(
Copy link
Contributor

@ajzobro ajzobro Feb 9, 2026

Choose a reason for hiding this comment

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

Thank you for adding tests for the new None branch, but could you please also add tests for these other else branches where the responses have status codes.

Copy link
Contributor Author

@gajeshbhat gajeshbhat Feb 9, 2026

Choose a reason for hiding this comment

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

Updated!

"Unable to post results to: %s (error: %d)",
result_uri,
job_request.status_code,
)
raise TFServerError(job_request.status_code)

def get_result(self, job_id):
"""Get current results data to the testflinger server for this job.
Expand All @@ -238,11 +246,18 @@ def get_result(self, job_id):
logger.error(exc)
return {}
if not job_request:
logger.error(
"Unable to get results from: %s (error: %d)",
result_uri,
job_request.status_code,
)
if job_request is None:
logger.error(
"Unable to get results from: %s (error: %s)",
result_uri,
"No response received",
)
else:
logger.error(
Copy link
Contributor

@ajzobro ajzobro Feb 9, 2026

Choose a reason for hiding this comment

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

And here.

Copy link
Contributor Author

@gajeshbhat gajeshbhat Feb 9, 2026

Choose a reason for hiding this comment

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

Updated!

"Unable to get results from: %s (error: %d)",
result_uri,
job_request.status_code,
)
return {}
if job_request.content:
return job_request.json()
Expand Down Expand Up @@ -332,12 +347,20 @@ def save_artifacts(self, rundir, job_id):
artifact_uri, files=file_upload, timeout=600
)
if not artifact_request:
logger.error(
"Unable to post results to: %s (error: %d)",
artifact_uri,
artifact_request.status_code,
)
raise TFServerError(artifact_request.status_code)
if artifact_request is None:
logger.error(
"Unable to post results to: %s (error: %s)",
artifact_uri,
"No response received",
)
raise TFServerError("No response received")
else:
logger.error(
Copy link
Contributor

@ajzobro ajzobro Feb 9, 2026

Choose a reason for hiding this comment

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

And also here.

Copy link
Contributor Author

@gajeshbhat gajeshbhat Feb 9, 2026

Choose a reason for hiding this comment

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

Updated!

"Unable to post results to: %s (error: %d)",
artifact_uri,
artifact_request.status_code,
)
raise TFServerError(artifact_request.status_code)
else:
shutil.rmtree(artifacts_dir)

Expand Down Expand Up @@ -523,10 +546,15 @@ def post_status_update(
)
# Response code is greater than 399
if not job_request:
error_msg = (
"No response received"
if job_request is None
else f"HTTP {job_request.status_code}"
)
logger.error(
"Unable to post status updates to: %s (error: %d)",
"Unable to post status updates to: %s (error: %s)",
status_update_uri,
job_request.status_code,
error_msg,
)
except requests.exceptions.RequestException as exc:
logger.error(
Expand Down
89 changes: 77 additions & 12 deletions agent/tests/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,9 @@ def test_transmit_job_outcome(self, client, requests_mock, tmp_path):
client.transmit_job_outcome(tmp_path)
assert requests_mock.last_request.json() == {"job_state": "complete"}

@patch.object(_TestflingerClient, "save_artifacts", side_effect=OSError)
def test_transmit_job_outcome_with_error(
self, client, requests_mock, tmp_path, caplog
self, mock_save_artifacts, client, requests_mock, tmp_path, caplog
):
"""
Test that OSError during save_artifacts results in removing the job
Expand All @@ -173,9 +174,7 @@ def test_transmit_job_outcome_with_error(
status_code=HTTPStatus.OK,
)

# Simulate an error during save_artifacts
with patch.object(client, "save_artifacts", side_effect=OSError):
client.transmit_job_outcome(tmp_path)
client.transmit_job_outcome(tmp_path)
assert tmp_path.exists() is False
assert "Unable to save artifacts" in caplog.text

Expand All @@ -191,7 +190,8 @@ def test_result_post_endpoint_error(self, client, requests_mock, caplog):
)
with pytest.raises(TFServerError):
client.post_result(job_id, {})
assert "Unable to post results" in caplog.text
assert "Unable to post results" in caplog.text
assert "error: 404" in caplog.text

def test_result_get_endpoint_error(self, client, requests_mock, caplog):
"""
Expand All @@ -206,6 +206,7 @@ def test_result_get_endpoint_error(self, client, requests_mock, caplog):
response = client.get_result(job_id)
assert response == {}
assert "Unable to get results" in caplog.text
assert "error: 404" in caplog.text

def test_attachment_endpoint_error(self, client, requests_mock, caplog):
"""
Expand Down Expand Up @@ -391,19 +392,20 @@ def test_check_jobs_without_agent_registration_reregisters(
assert len(post_requests) == 1
assert post_requests[0].json() == {"job_id": ""}

def test_check_jobs_request_exception(self, client):
@patch("testflinger_agent.client.time.sleep")
@patch("testflinger_agent.client.logger")
@patch("requests.Session.get")
def test_check_jobs_request_exception(
self, mock_get, mock_logger, mock_sleep, client
):
"""
Test that check_jobs handles RequestException (network failure)
by logging error and sleeping.
"""
network_error = RequestException("Connection refused")
mock_get.side_effect = network_error

with patch.object(client.session, "get", side_effect=network_error):
with patch("testflinger_agent.client.logger") as mock_logger:
with patch(
"testflinger_agent.client.time.sleep"
) as mock_sleep:
result = client.check_jobs()
result = client.check_jobs()

# Verify logger.error was called with the exception
mock_logger.error.assert_called_with(network_error)
Expand Down Expand Up @@ -716,3 +718,66 @@ def test_update_session_auth_skips_cookie_refresh(
and "/v1/agents/data/test_agent" in request.url
]
assert len(agent_data_calls) == 0

@patch("requests.Session.post", return_value=None)
def test_post_status_update_none_response(self, mock_post, client, caplog):
"""
Test that post_status_update handles None response gracefully.
This simulates the case where retry exhaustion returns None.
"""
job_id = str(uuid.uuid1())

client.post_status_update("myjobqueue", "http://foo", [], job_id)
assert "No response received" in caplog.text

@patch("requests.Session.post", return_value=None)
def test_post_result_none_response(self, mock_post, client, caplog):
"""Test that post_result handles None response gracefully."""
job_id = str(uuid.uuid1())

with pytest.raises(TFServerError, match="No response received"):
client.post_result(job_id, {"test": "data"})
assert "No response received" in caplog.text

@patch("requests.Session.get", return_value=None)
def test_get_result_none_response(self, mock_get, client, caplog):
"""Test that get_result handles None response gracefully."""
job_id = str(uuid.uuid1())

result = client.get_result(job_id)
assert result == {}
assert "No response received" in caplog.text

def test_save_artifacts_error_response(
self, client, requests_mock, tmp_path, caplog
):
"""Test that save_artifacts handles HTTP error response correctly."""
job_id = str(uuid.uuid1())

# Create artifacts directory with a file
artifacts_dir = tmp_path / "artifacts"
artifacts_dir.mkdir()
(artifacts_dir / "test.txt").write_text("test content")

artifact_uri = f"http://127.0.0.1:8000/v1/result/{job_id}/artifact"
requests_mock.post(artifact_uri, status_code=500)
with pytest.raises(TFServerError):
client.save_artifacts(tmp_path, job_id)
assert "Unable to post results" in caplog.text
assert "error: 500" in caplog.text

@patch("requests.Session.post", return_value=None)
def test_save_artifacts_none_response(
self, mock_post, client, tmp_path, caplog
):
"""Test that save_artifacts handles None response gracefully."""
job_id = str(uuid.uuid1())

# Create artifacts directory with a file
artifacts_dir = tmp_path / "artifacts"
artifacts_dir.mkdir()
(artifacts_dir / "test.txt").write_text("test content")

with pytest.raises(TFServerError, match="No response received"):
client.save_artifacts(tmp_path, job_id)
assert "No response received" in caplog.text
Loading