fix(crew): key the task registry by task, not by name - #117
CaptainAni187 wants to merge 1 commit into
Conversation
Task names are not unique. CrewSession derives a handler task's name from the session, the event and the handler function, so the same event firing twice while the first handler still runs produces two live tasks with identical names. Keyed by name, the second evicted the first from the registry, and then the first task's done handler deleted the second entry by that shared name, so a running task disappeared from current_tasks() entirely. Key by the task object instead, and have the session log the names it gets from current_tasks() rather than reaching into the registry's keys.
|
crim doesn't review pull requests automatically here. Comment |
|
crim review |
|
crim is reviewing this pull request. Findings will be posted shortly. |
Good to mergeThe registry finally stopped mistaking two tasks for one just because they wore the same name badge — and it brought receipts in the form of five tests. Clean fix, ship it. |
There was a problem hiding this comment.
LGTM
One-sentence assessment: A small, correct bug fix that keys the task registry by the task object rather than its non-unique name, backed by solid regression tests.
What this PR does
Task names in CrewSession are not unique (they are derived from session/event/handler), so two concurrent handlers could share a name. Keying TaskManager._tasks by name let the second task evict the first, and the first's done-callback then deleted the still-running second task's entry — losing it from current_tasks(). This PR changes the registry to Dict[asyncio.Task, TaskData], updates _add_task/_task_done_handler accordingly, and fixes session.cleanup to log names from the live task list. New tests cover the collision, pruning, and cancellation paths.
Findings
No issues found.
TaskManagerkeys its registry by task name, and task names are not unique.CrewSessionbuilds a handler task's name from the session, the event and the handler function:That is fully deterministic, so the same event firing twice while the first handler is still running produces two live tasks with identical names.
_add_taskthen overwrites:and
_task_done_handlerdeletes by that same shared name:So the second task evicts the first from the registry, and when the first finishes it deletes the entry belonging to the second, which is still running. The live task is then invisible to
current_tasks().Reproducing
Before:
After:
The registry reports zero tasks while one is still running.
Scope, honestly
CrewSession._cleanupkeeps its own_event_tasksset holding the task objects, so handler tasks are still cancelled on teardown and I am not claiming they leak. What is wrong is the registry itself:current_tasks()is documented as "the list of currently created/registered tasks" and it under-reports, down to zero while work is in flight. Anything that trusts it as the source of truth, including theWaiting for N tasks to completeline in cleanup, is working from a short count.The change
Key
_tasksby the task object, which is unique and hashable, instead of by its name._add_taskand_task_done_handlerfollow. Nothing else about the manager changes: names are still set on the task and still used for logging.One line in
session.pyreadself.task_manager._tasks.keys()to log task names, reaching into the registry's internals. It now takes the names fromcurrent_tasks(), which is the public accessor and stays correct whatever the keys are.Tests
tests/custom/test_crew_task_registry.py, five cases. Two fail onmain: both tasks sharing a name are registered, and one finishing does not unregister its namesake. The other three pin the behaviour the fix must not break, a finished task still being pruned,cancel_taskstill unregistering, and distinct names being unaffected.src/smallestai/atoms/crew/**is.fernignored, so a regen keeps this.