Skip to content

fix(runtime): heartbeat while harness child is active - #144

Merged
AbirAbbas merged 3 commits into
Agent-Field:mainfrom
ddbaron:fm/sweaf-heartbeat-fix-r1
Sep 24, 2026
Merged

AbirAbbas merged 3 commits into
Agent-Field:mainfrom
ddbaron:fm/sweaf-heartbeat-fix-r1

Conversation

@ddbaron

@ddbaron ddbaron commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Proposal

Keep AgentField's existing inactivity timeout, and keep the execution activity timestamp current while a harness child is running. That gives the stale cleanup enough information to leave a long tool call alone while it is still alive.

Implementation

This PR adds a small activity monitor around harness child waits:

  • It observes the subprocesses created by the AgentField harness.
  • Every 90 seconds, it checks whether an observed child still has no return code.
  • While a child is running, it sends Agent.note("Harness child tool is still running") with the harness and heartbeat tags.
  • The existing stream drain records output as an additional liveness signal for diagnostics.
  • The heartbeat task is canceled when the wait succeeds, raises, or is canceled.

The patch leaves the timeout values, control-plane code, and SDK code unchanged. The child process return code is the liveness signal. A child that stays alive while stuck internally will continue to receive heartbeats, so detecting that case would need a separate watchdog policy.

Why this is needed

A tool call that runs quietly for ten minutes was being reaped as stale. We found five coder runs that died after 11 to 30 silent minutes. File commits continued after the kill times. When the workers reported completion, their records were already terminal, so the updates returned HTTP 409 and the results were lost.

Validation

  • AGENTFIELD_SERVER=http://localhost:9999 uv run --extra dev python -m pytest tests -q: 1218 passed, 1 skipped, 54 warnings.
  • uv run --extra dev ruff check swe_af/runtime/activity_heartbeat.py tests/test_activity_heartbeat.py: passed.
  • uv run --extra dev ruff format --check swe_af/runtime/activity_heartbeat.py tests/test_activity_heartbeat.py: passed.
  • uv run python -m compileall -q swe_af: passed.

On run_20260908_061901_qk9lfxfq, three coders ran for 10m41s, 13m35s, and 12m06s. Each produced 7 to 9 heartbeat notes and completed successfully:

  • exec_20260908_061902_ssv9k381: 10m41.099s, succeeded, 7 heartbeats.
  • exec_20260908_061902_rvueq36b: 13m35.355s, succeeded, 9 heartbeats.
  • exec_20260908_061902_11d9d2y3: 12m06.546s, succeeded, 8 heartbeats.

No coder in that snapshot timed out. One code reviewer (exec_20260908_063109_gnix0790) timed out independently and is outside this change.

This PR is the node-side proposal. The workflow cleanup path also needs to use this activity clock; that separate issue is tracked in AgentField #1047.

The implementation is ready for maintainer review. I am open to a different way of observing child activity or delivering the note if that better fits the runtime.

@ddbaron
ddbaron requested a review from AbirAbbas as a code owner September 8, 2026 11:45
@CLAassistant

CLAassistant commented Sep 8, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment thread swe_af/runtime/activity_heartbeat.py
Comment thread swe_af/runtime/activity_heartbeat.py Outdated
Comment thread tests/test_activity_heartbeat.py

@AbirAbbas AbirAbbas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the writeup and for splitting the control-plane half out into agentfield#1047 — that made this much easier to verify. I traced the control-plane side and the mechanism does hold: Agent.note POSTs to /executions/note, AddExecutionNoteHandler sets execution.UpdatedAt, UpdateExecutionRecord persists updated_at, and that is exactly the column MarkStaleExecutions reads. Worth noting for anyone deploying this: the workflow-side reaper only started consulting the execution clock in agentfield#1046, which landed today in v0.1.138, so on an older control plane the heartbeat still won't save a run from markStaleWorkflowExecutions.

The blocker is coverage. The wrapper is on Agent.harness for every provider, but a note only fires once is_alive() sees an attached process, and processes are only attached through the asyncio.create_subprocess_exec hook. Our default runtime is claude_code, which spawns via anyio.open_process and never touches that function, so the default path gets zero heartbeats — I've left the probe numbers inline. Your validation run passed because codex/opencode both go through run_cli.

Local gates on 77f552b match yours (1218 passed, 1 skipped) and the lifecycle handling reads correctly to me — no leaked tasks, note can't raise or block the child, nothing hangs without a control plane. Two smaller notes inline on the _drain patch and on what the tests actually pin down.

@ddbaron

ddbaron commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor Author

@AbirAbbas Thanks for the detailed review, and for catching the anyio gap. Here is what I took from it and what changed on the branch.

What I understood you wanted:

  1. The heartbeat has to fire on the default claude_code runtime. My observation seam only wrapped asyncio.create_subprocess_exec, and claude_agent_sdk.query() goes through anyio.open_process, so the default path attached nothing and never recorded a note. My earlier validation looked healthy only because codex and opencode take the CLI path.
  2. Drop the _drain patch. It copied a private SDK internal, only produced last_output_at, and nothing outside the tests read that value.
  3. Make the tests exercise the real seam. The four existing tests hand-fed a fake child, so deleting the hook install, or replacing the cancellation with a pass, still passed the suite. The 10-70ms sleeps were also tight enough to go flaky on a loaded runner.

What changed:

  • anyio.open_process is now wrapped the same way as the create_subprocess_exec path, under the same best-effort try/ImportError guard. Its process object exposes .returncode, so the existing liveness check works unchanged and I did not special-case any provider.
  • Removed the _drain override and everything that existed only to produce last_output_at.
  • Added a test that spawns a real short-lived child through the anyio path and expects notes to be recorded, instead of injecting a fake child. Added a cancellation test that tracks the tasks created inside the wrapper and asserts the heartbeat task is really cancelled, so removing that finally block fails. Added a test that the production patch installs the hooks, so deleting that call fails.
  • Raised the sleeps by an order of magnitude.

State on the branch: 19 focused tests pass, and the full suite is 1221 passed with 1 skipped. I re-ran the two mutations from your review. Deleting the hook install from codex_harness_patch.py now fails the suite, and turning the cancellation into a pass fails it too.

Could you take another look when you get a chance, and let me know whether anything else needs changing? I am happy to observe child processes differently if you would prefer another approach.

@ddbaron

ddbaron commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

@AbirAbbas the round-2 push is up. It wraps the anyio spawn path so the default runtime's children get attached, drops the _drain patch, and the tests now fail if the attach hook or the task cancellation is removed.

CI is green. Would you mind taking another look when you get a chance?

@ddbaron
ddbaron requested a review from AbirAbbas September 18, 2026 11:01

@AbirAbbas AbirAbbas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Round 2 verified. The anyio gap is closed on the real seam, the _drain copy is gone, and the mutations from round 1 now fail the way you describe.

What I ran on your head merged onto latest main: full suite 1238 passed / 1 skipped, plus a probe through claude_agent_sdk.query() itself (fake cli_path, so the actual SubprocessCLITransport.connect() spawn rather than a direct anyio.open_process call) — attached_procs=2, heartbeats=16, against 0/0 before this round. CI on the new head was sitting in action_required; approved, and it's green.

Three notes inline. One is a real cancellation race; the other two are about what the tests still don't pin. None of them change the mechanism.

Comment on lines +177 to +187
finally:
heartbeat_task.cancel()
with suppress(asyncio.CancelledError):
await heartbeat_task

# If the caller cancels this wrapper, do not leave the harness process
# running detached from the reasoner that owns it.
if not child_task.done():
child_task.cancel()
with suppress(asyncio.CancelledError):
await child_task

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The suppress(CancelledError) here catches more than the heartbeat task's own cancellation — it also swallows a cancellation aimed at this task that lands during teardown. Repro on this head (cancel delivered from the heartbeat task's done-callback, i.e. just after the child returned):

cancel() -> True | wrapper outcome: ('returned', 'child-result') | task.cancelled(): False | cancelling(): 1

So a cancel that races harness completion is dropped: the reasoner keeps running and the call reports success. Narrow window, but every coder call here is wrapped in asyncio.wait_for (execution/dag_executor.py:44, execution/coding_loop.py:37, fast/executor.py:74), so it is reachable. Snapshotting asyncio.current_task().cancelling() before the await and re-raising if it grew closes it — or just don't await the heartbeat task, the loop has no cleanup to run.

Separately, the comment on 182-184 promises more than the code delivers. By the time await child_task raises, the child task is already done, so that branch doesn't fire; and cancelling the task doesn't kill the spawned process — I checked, the codex child survives cancellation identically with and without this wrapper, because _run_codex_cli_with_stdin never kills it. The orphan is pre-existing and not this PR's job, but the comment reads like it is handled.

# AgentField's run_cli resolves create_subprocess_exec through asyncio at
# call time. Keep the observer context-local so unrelated subprocesses in
# the node are not treated as harness activity.
asyncio.create_subprocess_exec = create_subprocess_exec_with_activity # type: ignore[assignment]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The anyio twin below is now pinned by a test; this line still isn't. Deleting it leaves pytest tests/test_activity_heartbeat.py tests/test_codex_harness_patch.py at 19 passed.

That's the half the #145 coders actually ran on — codex/open_code/gemini/aforge all reach agentfield.harness._cli.run_cli, which resolves asyncio.create_subprocess_exec at call time. A mirror of test_anyio_spawned_child_drives_activity_heartbeat that spawns through asyncio.create_subprocess_exec would guard it.

return await _orig_agent_harness(self, prompt, *args, **kwargs)
# Agent.note is the existing execution activity/status channel. The
# heartbeat reuses it rather than posting a second kind of update.
return await run_with_activity_heartbeat(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nothing pins this wiring. Replacing it with return await _orig_agent_harness(self, prompt, *args, **kwargs) leaves the full suite green — 1237 passed, and the single failure is tests/fast/test_integration.py::test_ac_14_no_existing_swe_af_files_modified, which trips on any edit under swe_af/ rather than on behaviour. The new hook-install test covers the seam, but not that Agent.harness actually runs the heartbeat.

It matters because the production path is one import away from losing it: swe_af/app.py:84 captures _original_harness = app.harness and then overrides the instance attribute, so the heartbeat only survives because from swe_af.reasoners import router (app.py:21) applies the patch first. It does hold today — swe_af.app._original_harness.__func__.__qualname__ is apply_codex_harness_patch.<locals>._harness_with_provider_context — but a test asserting app.harness routes through run_with_activity_heartbeat is what keeps it holding.

- Re-raise an outer cancellation (e.g. asyncio.wait_for) that lands while
  the heartbeat task is awaited down, instead of swallowing it with the
  heartbeat's own CancelledError.
- Add a real-subprocess test for the asyncio.create_subprocess_exec seam,
  which the codex/open_code/gemini/aforge CLI path resolves at call time.
- Add a fresh-interpreter test that app.harness reaches
  run_with_activity_heartbeat, pinning the app import-order wiring.
- Correct the cleanup comment: cancelling the harness wait does not kill
  an OS child the provider already spawned (pre-existing behavior).

@AbirAbbas AbirAbbas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Round 3 verified on 6426fd6 merged onto current main. The late-cancellation race is closed (a cancel delivered from the heartbeat task's done-callback now ends the wrapper cancelled instead of returning the child's result), the comment no longer promises to stop the spawned process, and both seams are pinned: deleting the asyncio.create_subprocess_exec hook fails test_asyncio_spawned_child_drives_activity_heartbeat, and replacing the Agent.harness wrapper with a passthrough fails test_app_harness_routes_through_activity_heartbeat. The round-1 mutations still fail too. Through the real seams: claude_agent_sdk.query() gives attached_procs=2, heartbeats=16, and run_cli gives 1 and 16. make check is 1293 passed / 1 skipped; Go build/vet/race clean.

One small non-blocking suggestion: if current.cancelling(): raise fires on any nonzero count, so a caller that caught an earlier cancellation without calling uncancel() would see a successful harness result turned into CancelledError. Nothing in swe_af does that today, so it isn't reachable here. Snapshotting cancelling() before awaiting the heartbeat task and re-raising only if it grew is the tighter check, and it keeps the race fix.

Thanks for the careful follow-through on all three rounds.

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.

3 participants