Skip to content

Commit a098182

Browse files
committed
test/fix(agentex): address round-2 audit on current_state
- Add the real use-case-level clobber regression test: it forces a stale read (entity fetched RUNNING, task transitioned to COMPLETED before the write) and asserts status stays COMPLETED. This fails on the old whole-row-merge path and passes on the column-scoped update — the prior service-level test could not catch the regression (it exercised the new primitive directly, which is clobber-proof by construction); its docstring is corrected accordingly. - update_mutable_fields_on_task now raises ItemDoesNotExist when the atomic update reports the row vanished, instead of silently returning stale data (consistent with the not-found contract and sibling transition methods; defensive — no live hard-delete path reaches it). - Drop max_length from the response Task.current_state field: input is already bounded by UpdateTaskRequest + the String(255) column, and enforcing it on the read path would turn a future column-widening into a 500 on every read. - Make the repository update_mutable_fields empty-fields branch's contract honest in its docstring (it's an unreachable defensive no-op). - Assert the intermediate set in the clear-to-null test so the clear is a real transition, not a null→null no-op. - Re-await the cancelled collector in the stream test to avoid a dangling-task warning at teardown. - Remove the now-unused `import sqlalchemy as sa` from the migration. - Regenerated openapi.yaml (maxLength now only on the request schema).
1 parent cea9411 commit a098182

8 files changed

Lines changed: 71 additions & 13 deletions

File tree

agentex/database/migrations/alembic/versions/2026_07_22_1200_add_task_current_state_b2c3d4e5f6a7.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
from typing import Sequence, Union
99

1010
from alembic import op
11-
import sqlalchemy as sa
1211

1312

1413
# revision identifiers, used by Alembic.

agentex/openapi.yaml

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6591,7 +6591,6 @@ components:
65916591
current_state:
65926592
anyOf:
65936593
- type: string
6594-
maxLength: 255
65956594
- type: 'null'
65966595
title: Opaque label mirroring the agent's StateMachine current state; null
65976596
when the agent does not emit one. Orthogonal to 'status'.
@@ -6821,7 +6820,6 @@ components:
68216820
current_state:
68226821
anyOf:
68236822
- type: string
6824-
maxLength: 255
68256823
- type: 'null'
68266824
title: Opaque label mirroring the agent's StateMachine current state; null
68276825
when the agent does not emit one. Orthogonal to 'status'.

agentex/src/api/schemas/tasks.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,11 @@ class Task(BaseModel):
6868
None,
6969
title="Task metadata",
7070
)
71+
# No max_length on this response field on purpose: input is already bounded
72+
# by UpdateTaskRequest + the String(255) column, and enforcing the bound on
73+
# the read path would turn a future column-widening into a 500 on every read.
7174
current_state: str | None = Field(
7275
None,
73-
max_length=CURRENT_STATE_MAX_LENGTH,
7476
title=(
7577
"Opaque label mirroring the agent's StateMachine current state; "
7678
"null when the agent does not emit one. Orthogonal to 'status'."

agentex/src/domain/repositories/task_repository.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -264,11 +264,15 @@ async def update_mutable_fields(
264264
changed ``status``/``params``), this issues a single
265265
``UPDATE ... SET <only these columns> WHERE id = :id RETURNING *``.
266266
Touching only the supplied columns means a concurrent status transition
267-
or param merge is never reverted. Returns the updated entity, or
268-
``None`` if no task with ``task_id`` exists.
267+
or param merge is never reverted. With a non-empty ``fields`` this returns
268+
the updated entity, or ``None`` if no task with ``task_id`` exists.
269269
270270
``fields`` values are applied verbatim, so passing ``current_state=None``
271271
clears the column (callers distinguish "clear" from "omit" upstream).
272+
273+
An empty ``fields`` is a defensive no-op that returns the current task
274+
(raising ``ItemDoesNotExist`` if absent); the sole caller never reaches it,
275+
as it gates the call behind a non-empty field set.
272276
"""
273277
if not fields:
274278
return await self.get(id=task_id)

agentex/src/domain/use_cases/tasks_use_case.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,14 @@ async def update_mutable_fields_on_task(
158158
updated = await self.task_service.update_mutable_fields(
159159
task_entity.id, fields
160160
)
161-
if updated is not None:
162-
task_entity = updated
161+
if updated is None:
162+
# The row vanished between the initial read and the write. No live
163+
# hard-delete path reaches here today (delete is a soft status
164+
# update, already guarded above), so this is defensive — but raise
165+
# rather than return stale data, matching the not-found contract
166+
# above and the sibling terminal-transition methods.
167+
raise ItemDoesNotExist(f"Task {id or name} not found")
168+
task_entity = updated
163169

164170
return task_entity
165171

agentex/tests/integration/test_task_stream.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,9 @@ async def collect_stream_events():
275275
await stream_task
276276
except (TimeoutError, asyncio.CancelledError):
277277
stream_task.cancel()
278+
# Re-await so the task's own cancellation cleanup runs and no
279+
# "Task exception was never retrieved" warning leaks at teardown.
280+
await asyncio.gather(stream_task, return_exceptions=True)
278281

279282
task_updated_events = [
280283
e for e in stream_events if e.get("type") == "task_updated"

agentex/tests/unit/services/test_task_service.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -991,12 +991,13 @@ async def test_update_mutable_fields_persists_and_publishes(
991991
assert event_data["type"] == "task_updated"
992992
assert event_data["task"]["current_state"] == "working"
993993

994-
async def test_update_mutable_fields_does_not_clobber_status(
994+
async def test_update_mutable_fields_leaves_status_untouched(
995995
self, task_service, agent_repository, sample_agent, redis_stream_repository
996996
):
997-
"""Regression (blocker): a current_state write must not revert a status
998-
changed by another writer. update_mutable_fields is column-scoped, so a
999-
task that went terminal is not resurrected to its earlier status."""
997+
"""The primitive is column-scoped: writing current_state does not touch
998+
status. (The use-case-level clobber regression — a stale read racing a
999+
status transition — is guarded in test_tasks_use_case.py; this only pins
1000+
that the repository UPDATE sets no columns beyond those supplied.)"""
10001001
await create_or_get_agent(agent_repository, sample_agent)
10011002
created_task = await task_service.create_task(
10021003
agent=sample_agent, task_name="task-for-noclobber"

agentex/tests/unit/use_cases/test_tasks_use_case.py

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -639,9 +639,12 @@ async def test_update_current_state_clears_on_explicit_null(
639639
task = await task_service.create_task(
640640
agent=sample_agent, task_name="current-state-clear-test"
641641
)
642-
await tasks_use_case.update_mutable_fields_on_task(
642+
was_set = await tasks_use_case.update_mutable_fields_on_task(
643643
id=task.id, current_state="working"
644644
)
645+
# Confirm it was actually set, so the clear below is a real transition
646+
# (not a null→null no-op that would pass trivially).
647+
assert was_set.current_state == "working"
645648

646649
cleared = await tasks_use_case.update_mutable_fields_on_task(
647650
id=task.id, current_state=None
@@ -673,6 +676,48 @@ async def test_update_current_state_and_metadata_single_atomic_write(
673676
assert updated.task_metadata == {"stage": "two"}
674677
assert updated.status == TaskStatus.RUNNING
675678

679+
async def test_update_current_state_does_not_clobber_concurrent_status(
680+
self, tasks_use_case, task_service, task_repository, agent_repository, sample_agent
681+
):
682+
"""Regression (round-one blocker): a current_state write must not revert a
683+
status changed concurrently. Reproduces the exact staleness the old
684+
whole-row-merge path needed — the use case reads the entity while RUNNING,
685+
the task goes COMPLETED before the write lands, and the write must keep
686+
COMPLETED. Fails on the old update_task/merge path (reverts to RUNNING),
687+
passes on the column-scoped update.
688+
"""
689+
await create_or_get_agent(agent_repository, sample_agent)
690+
task = await task_service.create_task(
691+
agent=sample_agent, task_name="current-state-clobber-test"
692+
)
693+
694+
# Snapshot the entity as the use case would have read it, BEFORE the
695+
# concurrent transition — this is the stale read the bug wrote back.
696+
stale_entity = await task_service.get_task(id=task.id)
697+
assert stale_entity.status == TaskStatus.RUNNING
698+
699+
# Another writer moves the task to a terminal status after that read.
700+
await task_service.transition_task_status(
701+
task_id=task.id,
702+
expected_status=TaskStatus.RUNNING,
703+
new_status=TaskStatus.COMPLETED,
704+
status_reason="done",
705+
)
706+
707+
# Force the use case to operate on the stale (pre-transition) read.
708+
task_service.get_task = AsyncMock(return_value=stale_entity)
709+
710+
updated = await tasks_use_case.update_mutable_fields_on_task(
711+
id=task.id, current_state="late"
712+
)
713+
714+
# The write set current_state without reverting the terminal status.
715+
assert updated.current_state == "late"
716+
assert updated.status == TaskStatus.COMPLETED
717+
persisted = await task_repository.get(id=task.id)
718+
assert persisted.status == TaskStatus.COMPLETED
719+
assert persisted.current_state == "late"
720+
676721
async def test_update_current_state_on_deleted_task_raises(
677722
self, tasks_use_case, task_service, agent_repository, sample_agent
678723
):

0 commit comments

Comments
 (0)