Repository navigation
Commit 4e916d8
fix(server): record FAILED when a task's terminal save fails (#1400)
# Description
When the task store rejects the write of a task's terminal status (for
example a dropped database connection during the COMPLETED save),
`DefaultRequestHandler` never records FAILED: the task stays
`TASK_STATE_WORKING` in the store, and clients polling `tasks/get` never
see a terminal state.
`TaskManager.save_task_event` applies a status or artifact event to the
cached `_current_task` in place and only then calls the store. When
`save()` raises, the cache already says COMPLETED. The consumer's
failure handler, `_write_failed_status` in `active_task.py`, reads that
cache through `task_manager.get_task()` and returns early for a terminal
state, so the FAILED write it documents for this case never happens.
Reproducer: an agent emits `Task`, `start_work()`, `complete()`; the
store raises `ConnectionError` on the first COMPLETED write; then
`tasks/get`.
```
# main (43d1eff)
send raised: ConnectionError database connection lost
tasks/get state: TASK_STATE_WORKING
# this branch
send raised: ConnectionError database connection lost
tasks/get state: TASK_STATE_FAILED
```
```mermaid
flowchart TD
E[status or artifact event] --> M{TaskManager.save_task_event}
M -->|main| A[mutate cached _current_task in place]
A --> S1[store.save raises]
S1 --> C1[cache says COMPLETED, store says WORKING]
C1 --> W1[_write_failed_status: cache is terminal, skip]
W1 --> R1[task stays WORKING]
M -->|this PR| B[apply event to a copy]
B --> S2[store.save raises]
S2 --> C2[cache still says WORKING, as persisted]
C2 --> W2[_write_failed_status writes FAILED]
W2 --> R2[task is FAILED]
```
Change, in `src/a2a/server/tasks/task_manager.py`: `save_task_event`
copies the cached task and applies the event to the copy. `_save_task`
already assigns `_current_task` only after `save()` returns, so a failed
save leaves the cache as last persisted. The same change fixes `prev`
for a `VersionedTaskStore`: it was the updated task itself rather than
"the task as previously read" that `VersionedTaskStore.save` documents.
Also:
- The comment in `ActiveTask.cancel` that said ordinary status writes
still update the shared object in place is updated; Task events and
`update_with_message` still do.
- Two existing tests in `test_default_request_handler.py` (legacy
handler) relied on the in-place updates:
- `test_on_message_send_non_blocking` compared `len(result.history)`
with the stored task after polling. `result` was the cached object, so
it kept growing after it was returned. It is now the task as returned,
and the test checks that its history is a prefix of the stored history.
- `test_on_message_send_error_does_not_hang` used an
`AsyncMock(spec=TaskStore)` whose `get` returned an `AsyncMock` in place
of a `Task`; it now returns `None` so the task is created and the first
`save` raises as before.
Behaviour notes:
- `RequestContext.current_task` is no longer updated in place by later
status and artifact events during a request; it is the task as read when
the request started. Those in-place updates raced with the consumer
anyway (`enqueue_event` returns before the event is applied).
- With an unversioned store, a save that raised although it committed
(the acknowledgement was lost) is now followed by a FAILED write over
the committed state. Before this change the same store left the task
WORKING when the write had not committed, which is the usual case. With
`VersionedDatabaseTaskStore` the version check rejects the stale FAILED
write.
- Cost: one `Task.CopyFrom` per status or artifact event. The default
path already copies the whole task per event (`_enqueue_to_subscribers`,
`CopyingTaskStoreAdapter.save`, `DatabaseTaskStore` serialisation).
Not in this PR: `update_with_message` still appends the user message to
the cached task before its save (#1392 rewrites those lines in
`active_task.py`).
Tests:
- `tests/server/tasks/test_task_manager.py`:
`test_save_task_event_failed_save_keeps_cached_task` (status and
artifact events) and
`test_save_task_event_passes_previous_task_as_prev`.
- `tests/server/request_handlers/test_default_request_handler_v2.py`:
`test_failed_terminal_save_records_failed_task`, end to end through
`on_message_send` with an `InMemoryTaskStore` that fails the first
COMPLETED write.
- All four fail on `main` (`assert 3 == 1`, artifact left in the cache,
`assert 2 == 1`, `assert 2 == 4` for WORKING vs FAILED) and pass with
the change.
Checks, run on Windows 11 (CPU, Python 3.10.19); Linux CI will confirm:
- `bash ./scripts/lint.sh`: ruff check, ruff format and `ty` pass (three
existing unused `type: ignore` warnings in files this PR does not
touch).
- `uv run pytest tests/server/tasks/test_task_manager.py
tests/server/request_handlers/test_default_request_handler.py
tests/server/request_handlers/test_default_request_handler_v2.py
tests/server/agent_execution`: 297 passed.
- `uv run pytest --ignore=tests/integration/cross_version
--ignore=tests/integration/test_scenarios.py --cov=src`: 2364 passed,
203 skipped, 4 xfailed; total coverage 93%. The 4 setup errors
(`tests/integration/push_notifications`, `Can't pickle local object`
under Windows spawn) happen on `main` here too.
`tests/integration/cross_version` (uses `os.killpg`) and
`tests/integration/test_scenarios.py` (hangs on Windows on `main`) were
not run locally, so the Linux CI run is the check for those.
- [x] Follow the [`CONTRIBUTING`
Guide](https://github.com/a2aproject/a2a-python/blob/main/CONTRIBUTING.md).
- [x] Make your Pull Request title in the
<https://www.conventionalcommits.org/> specification.
- [x] Ensure the tests and linter pass
- [x] Appropriate docs were updated (if necessary)
Co-authored-by: Mateusz Iwaniuk <45974414+Iwaniukooo11@users.noreply.github.com>1 parent ac48ce1 commit 4e916d8
5 files changed
Lines changed: 152 additions & 10 deletions
File tree
- src/a2a/server
- agent_execution
- tasks
- tests/server
- request_handlers
- tasks
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
964 | 964 | | |
965 | 965 | | |
966 | 966 | | |
967 | | - | |
968 | | - | |
969 | | - | |
970 | | - | |
| 967 | + | |
| 968 | + | |
| 969 | + | |
| 970 | + | |
971 | 971 | | |
972 | 972 | | |
973 | 973 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
267 | 267 | | |
268 | 268 | | |
269 | 269 | | |
270 | | - | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
271 | 276 | | |
272 | 277 | | |
273 | 278 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1098 | 1098 | | |
1099 | 1099 | | |
1100 | 1100 | | |
1101 | | - | |
1102 | | - | |
1103 | | - | |
1104 | | - | |
1105 | | - | |
| 1101 | + | |
| 1102 | + | |
| 1103 | + | |
| 1104 | + | |
1106 | 1105 | | |
1107 | 1106 | | |
1108 | 1107 | | |
| |||
2941 | 2940 | | |
2942 | 2941 | | |
2943 | 2942 | | |
| 2943 | + | |
2944 | 2944 | | |
2945 | 2945 | | |
2946 | 2946 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
190 | 190 | | |
191 | 191 | | |
192 | 192 | | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
193 | 265 | | |
194 | 266 | | |
195 | 267 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
4 | 4 | | |
5 | 5 | | |
6 | 6 | | |
| 7 | + | |
7 | 8 | | |
8 | 9 | | |
9 | 10 | | |
| |||
196 | 197 | | |
197 | 198 | | |
198 | 199 | | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
199 | 264 | | |
200 | 265 | | |
201 | 266 | | |
| |||
0 commit comments