OpenHands / OpenHands/software-agent-sdk

test_acp_agent.py flakes ~57% of runs: TestACPAgentCleanup counts other agents' _finalize calls

Open Beginner friendly
#4,840 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug priority:medium ready-for-dev testing
Dominant language
Python
Stars
1.1k
Forks
539
Avg merge
1d 19h
Merged PRs (30d)
137

Description

Actual Behavior

tests/sdk/agent/test_acp_agent.py fails intermittently — 4 of 7 full-file runs on main at bf8a121d8:

uv run pytest tests/sdk/agent/test_acp_agent.py -q -p no:randomly
443 passed
443 passed
1 failed 442 passed
1 failed 442 passed
1 failed 442 passed
443 passed
1 failed 442 passed

The failing test moves between runs — observed as both TestACPAgentCleanup::test_atexit_cleanup_is_weak_and_inline and ::test_finalizer_falls_back_when_thread_start_fails — always with:

AssertionError: Expected '_finalize' to be called once. Called 2 times.
Calls: [call(), call()].

All 14 tests in the class pass in isolation (pytest tests/sdk/agent/test_acp_agent.py::TestACPAgentCleanup → 14 passed), which is what makes it look like flakiness rather than a defect.

Root cause. Two tests patch _finalize on the class and then assert a call count that is only meaningful per instance:

with patch.object(ACPAgent, "_finalize") as finalize:
    ...
finalize.assert_called_once_with()
  1. 48 sites in the file attach a MagicMock() executor/connection to an agent and never close it, so _has_runtime_resources() stays true.
  2. Those agents are freed at arbitrary later points in the run, not when their test ends.
  3. ACPAgent.__del__ then calls _finalize on them.
  4. With a class-level patch, that call lands on whichever mock is currently open.

Instrumenting __del__ confirms it fires with live resources during dozens of unrelated tests:

=== ACPAgent.__del__ fired WITH runtime resources, during: ===
    3x  during test: test_claude_provider_supports_runtime_switch
    3x  during test: test_resume_without_stored_cwd_still_works
    3x  during test: test_known_provider_surfaces_applied_override
    2x  during test: test_step_records_latency
    ...

which is why the failing test moves around.

Expected Behavior

The file passes deterministically. TestACPAgentCleanup's assertions should count only the _finalize calls made by their own agent.

Patching the instance is not an option — ACPAgent is a frozen pydantic model, so patch.object(agent, "_finalize") raises frozen_instance. Keeping the class patch but adding autospec=True records self, so each test can filter to its own calls:

OLD  call_count = 2 -> assert_called_once_with() FAILS
NEW  total = 2  own = 1  -> assertion PASSES

Acceptance Criteria

  • pytest tests/sdk/agent/test_acp_agent.py passes on 10 consecutive full-file runs
  • TestACPAgentCleanup still asserts that its own agent's _finalize ran exactly once (the assertion is tightened, not weakened)
  • No change to ACPAgent production code

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in tests/sdk/agent/test_acp_agent.py, especially TestACPAgentCleanup::test_atexit_cleanup_is_weak_and_inline and ::test_finalizer_falls_back_when_thread_start_fails. Review how the class-level _finalize patch records calls, then run the full file with pytest as described. Done means each test counts only its own agent's finalization, production code is unchanged, and ten consecutive full-file runs pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
testing-qa
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.