fix(generate): don't kill the model worker when cancelling a job that already ended

[kevin9327]

What

Cancelling a generation job that has already ended still kills the model worker:

• If another generation is running, it fails with Subprocess died during generation.
• If nothing is running, the warm model is thrown away and the next generation has to load it from scratch.

This is easy to hit from the headless surface: python tools/modly-cli/agent.py legacy cancel <job_id> or workflow-run cancel <run_id> on a job that finished, failed, or was already cancelled. It also happens in the app when Cancel is pressed just as a generation completes, because the poll loop posts the cancel on its next tick, after the backend has marked the job done.

Why it triggers

cancel_job (/generate/cancel/{id}) and its sibling cancel_run (/workflow-runs/{id}/cancel) only update the status while the job is pending or running. The subprocess kill after that runs unconditionally:

if job.status in ("pending", "running"):
    job.status = "cancelled"
    _completed_at[job_id] = time.monotonic()
# Kill the active generator subprocess immediately so inference stops now.
try:
    gen = generator_registry._generators.get(generator_registry._active_id)
    if gen is not None and hasattr(gen, "_proc") and gen._proc and gen._proc.poll() is None:
        gen._proc.kill()

ExtensionProcess keeps its subprocess alive between generations to hold the loaded model, so gen._proc.poll() is None is true for an idle worker too. For an ended job, that subprocess isn't running the job being cancelled.

Fix

In both endpoints, return before the kill when the job has already ended. Cancelling a pending or running job is unchanged. The response stays {"cancelled": true} in every case, so clients see no difference.

Verification

New CancelEndedJobTests in api/tests/test_workflow_runs_lifecycle.py, next to the existing cancel_run test. The registry's active worker is a fake with a live _proc.

• test_cancelling_a_finished_job_leaves_the_active_worker_alone (cancel_job, status done): fails before, passes after.
• test_cancelling_a_failed_run_leaves_the_active_worker_alone (cancel_run, status error): fails before, passes after.
• test_cancelling_a_running_job_still_stops_the_worker and test_cancelling_a_running_run_still_stops_the_worker: pass before and after. A running job is still marked cancelled, and its worker is still killed, detached and marked unloaded. That proves the change doesn't widen into leaving a real cancellation running.

The ended-job tests also assert the job's status is unchanged (done / error), which holds before and after.

Fail-before output, with the two routers reverted and the test kept. It is verbatim except that the local checkout path prefix is replaced with <repo>, and the registry's import-time path banner that follows it is omitted:

test_cancelling_a_failed_run_leaves_the_active_worker_alone (tests.test_workflow_runs_lifecycle.CancelEndedJobTests.test_cancelling_a_failed_run_leaves_the_active_worker_alone) ... FAIL
test_cancelling_a_finished_job_leaves_the_active_worker_alone (tests.test_workflow_runs_lifecycle.CancelEndedJobTests.test_cancelling_a_finished_job_leaves_the_active_worker_alone) ... FAIL
test_cancelling_a_running_job_still_stops_the_worker (tests.test_workflow_runs_lifecycle.CancelEndedJobTests.test_cancelling_a_running_job_still_stops_the_worker) ... ok
test_cancelling_a_running_run_still_stops_the_worker (tests.test_workflow_runs_lifecycle.CancelEndedJobTests.test_cancelling_a_running_run_still_stops_the_worker) ... ok
test_cancel_run_records_completion_so_it_can_be_purged (tests.test_workflow_runs_lifecycle.WorkflowRunJobLifecycleTests.test_cancel_run_records_completion_so_it_can_be_purged) ... ok
test_create_run_purges_terminal_jobs_past_ttl (tests.test_workflow_runs_lifecycle.WorkflowRunJobLifecycleTests.test_create_run_purges_terminal_jobs_past_ttl) ... ok

======================================================================
FAIL: test_cancelling_a_failed_run_leaves_the_active_worker_alone (tests.test_workflow_runs_lifecycle.CancelEndedJobTests.test_cancelling_a_failed_run_leaves_the_active_worker_alone)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "<repo>\api\tests\test_workflow_runs_lifecycle.py", line 163, in test_cancelling_a_failed_run_leaves_the_active_worker_alone
    self.assertFalse(proc.killed)
AssertionError: True is not false

======================================================================
FAIL: test_cancelling_a_finished_job_leaves_the_active_worker_alone (tests.test_workflow_runs_lifecycle.CancelEndedJobTests.test_cancelling_a_finished_job_leaves_the_active_worker_alone)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "<repo>\api\tests\test_workflow_runs_lifecycle.py", line 152, in test_cancelling_a_finished_job_leaves_the_active_worker_alone
    self.assertFalse(proc.killed)
AssertionError: True is not false

----------------------------------------------------------------------
Ran 6 tests in 0.004s

FAILED (failures=2)

After the fix, python -m unittest tests.test_workflow_runs_lifecycle passes all 6 tests (Ran 6 tests in 0.021s / OK).

Whole suite, python -m unittest discover -s tests in api/ (venv with fastapi + python-multipart + httpx):

	dev	this branch
Whole suite	Ran 93 tests / OK (skipped=3)	Ran 97 tests / OK (skipped=3)
test_workflow_runs_lifecycle.py	2 pass	6 pass

The 3 skips were already there on dev.

Lint: the repo configures no Python linter, and ESLint ignores api/**. As a spot check, ruff check --isolated reports the same counts on dev and this branch: generation.py 10 → 10, workflow_runs.py 8 → 8, test_workflow_runs_lifecycle.py 4 → 4, so nothing new. No TypeScript touched.

🤖 Generated with Claude Code

[iammojogo-sudo]

Question: Im curious as if this also has an effect of the ability to free memory after a cancel/kill. Has anyone experience python separation from Modly processes when doing this? If so, that could be tied into this kind of fix.

[kevin9327]

This change does not touch what happens on a real cancel. When the job is still pending or running, the worker is killed exactly as before, and the process exit is what returns its memory to the OS, so freeing memory after a cancel is unchanged.

What changes is the case where the job has already ended: the cancel used to kill the worker anyway, which either broke a generation that was running for someone else or threw away the warm model. Now a cancel on a finished job only reports the job's state and leaves the worker, with the loaded model, in place. That is the same state the worker is in after any job completes normally, so the memory it holds is the warm-model memory that was already there, not something this PR keeps alive longer.

If you have seen a worker's memory not being released after a kill, that would be a separate problem in the kill path itself, and worth its own issue with the platform and what ps showed.

[lightningpixel]

Thanks for this, the diagnosis was spot on. Since then, 10cdc15 on dev reworked cancellation: cancel_job now only kills the generator bound to that job (_job_generators[job_id]), and that binding is dropped as soon as the job ends, so cancelling an ended job no longer touches any worker (cancel_run now delegates to cancel_job). This PR is superseded, but a small follow-up porting your "ended job leaves the worker alone" tests to _job_generators would be welcome as a regression guard.

[kevin9327]

Thanks for the review, and for pointing me at 10cdc15. Binding the kill to _job_generators[job_id] and releasing that binding when the job ends covers this more cleanly than my status guard, so I'm closing this one as superseded. I've opened #366 with the tests ported onto _job_generators as a regression guard.