Skip to content

test(agent): cancel the ask once its kimi is up, not on a 0.3s clock - #943

Merged
LivXue merged 1 commit into
mainfrom
test/kimi_ask_cancel_race
Oct 14, 2026
Merged

LivXue merged 1 commit into
mainfrom
test/kimi_ask_cancel_race

Conversation

@LivXue

@LivXue LivXue commented Oct 14, 2026

Copy link
Copy Markdown
Member

Summary

A follow-up to #927, which merged before this was pushed. Its test test_an_ask_that_runs_out_takes_what_its_kimi_started cut the Kimi Code ask's timeout to 0.3s and then read the worker.pid its stand-in kimi writes. The timeout runs from the spawn, so wherever the stand-in starts slower than that, the ask's finally ended the process group before the stand-in had noted its worker, and reading worker.pid raised FileNotFoundError. The review on #927 reproduced it deterministically on macOS; on Linux the stand-in's start crossed 0.3s here only at low priority beside busy loops.

The test now cancels the ask once the stand-in is up rather than waiting on the clock: it waits for ask.pid, which the stand-in writes only after noting its worker, checks the worker is alive, cancels, and asserts the worker went with the group. Cancel and timeout end the ask through the same finally, and the timeout path keeps its own tests in the file, the 1.0s POSIX one and the Windows-branch one. Test-only: no production file changes.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

Linux, Python 3.12, on main d106a5c:

  • uv run --frozen --python 3.12 --all-extras pytest tests/test_subagent_kimi_code.py tests/test_subagent_third_party.py -q: 348 passed.

  • The race, forced: with the stand-in made to wait 0.5s before starting its worker (a scratch edit to _through_kimi, not part of this change), main's test fails with FileNotFoundError on worker.pid, the failure reported on fix(*): stop cli sub-agents on windows and hand new ones a rotated web key #927, and the new test passes.

  • Against the fix: making the POSIX branch of kimi_code._end_tree kill the leader alone (proc.kill() in place of os.killpg) turns the new test red, and only it (1 failed, 347 passed over the two files).

  • ruff check and ruff format --check on the changed file: clean. make lint-types leaves tests/ out, so no type check covers this change.

  • Relevant tests pass locally

  • Relevant lint / type checks pass locally

  • User-facing docs or screenshots are updated when needed

Risk

Test-only. One test is rewritten under a new name; no production file changes, so reverting the commit is the whole rollback.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

#927

The 0.3s cap ran from the spawn, so wherever the stand-in `kimi` starts
slower than that, as on macOS, the ask was killed while it was still
starting and had noted no worker to take. Readiness now comes from
`ask.pid`, which the stand-in writes only after the worker is noted, and
the cancel path proves the group goes rather than the leader alone.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

The readiness marker is written only after the worker PID, so cancelling after ask.pid appears reliably reaches the behavior under test without weakening the assertion that the child process dies with the ask's process group. The 5-second readiness bound and 2-second reap allowance are consistent with the neighboring subprocess tests.

I covered the repository rules, the complete diff, the production ask cancellation/timeout implementation and its callers' expectations, the test's introducing history, backward compatibility, test-strength/regression concerns, and architecture constraints. This is test-only and changes no runtime behavior or public contract.

Verification:

  • uv run --extra dev pytest tests/test_subagent_kimi_code.py -q: 59 passed.
  • Changed test repeated 10 times with -n0: 10 passed.
  • The initial default-extra invocation did not reach the test because raven_everos was absent; an all-extras retry also did not reach it because this host lacks gcc for datrie.

@ZuyiZhou ZuyiZhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test-only. The stand-in writes worker.pid before ask.pid, so waiting on ask.pid removes the 0.3s race, and the group-kill assertion is kept (reaping is polled). CI green at 9533b27.

@LivXue
LivXue merged commit a97f92c into main Oct 14, 2026
24 checks passed
@LivXue
LivXue deleted the test/kimi_ask_cancel_race branch October 14, 2026 08:55
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