feat(teamharness): persist durable task continuation state - #1183
feat(teamharness): persist durable task continuation state#1183jesseedcp wants to merge 7 commits into
Conversation
a7dc93c to
74a3aa3
Compare
|
I moved this out of draft and rebased it onto the current While checking the merged #1172 path, I found that Controller-side cancellation could leave a submitted TaskMeta as I added the corresponding replan fences as well: a task with a committed cancellation cannot be reopened, dropped, or removed and re-added under the same ID. The focused Go, Python, and Ruby suites are green locally. CI is running again after the rebase and force-push. @shiyiyue1102, when you have time, could you take a look at this state-contract slice? The periodic scanner and Matrix wake-up are still intentionally left for the follow-up to #1177. |
0bd1c13 to
31ede24
Compare
31ede24 to
8d7594c
Compare
|
Rebased onto current @shiyiyue1102, could you take another look when you have time? This PR is still only the state-contract slice; the Controller scanner, Leader wake-up, and Matrix retry will stay in a separate follow-up for #1177. |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
This PR adds durable task continuation state across the Controller (Go), CoPaw (Python), and MCP server (Python), introducing opaque submission identity, submission-fenced accept/cancel decisions, idempotent retry with CAS-style repair, atomic file writes, canonical result digests, and publish-last shared-storage commit ordering. The design is solid and the test coverage is extensive, covering idempotency, legacy adoption, concurrent races, cross-project rejection, and retry-after-failure scenarios. However, three areas warrant attention before merge: (1) _resolve_task_continuation in server.py unconditionally overwrites resolved_at on every call, which could clobber the original decision timestamp when a leader re-accepts the same submission; (2) the _TASK_MUTATION_LOCKS registry in task.py grows without bound as tasks are created, leaking memory over long-running worker processes; (3) the tightened isPlainToken in project_handler.go now rejects tokens starting with non-alphanumeric characters, which is a backward-incompatible validation change that could break existing project IDs or tokens in production. These are not critical correctness bugs but should be addressed or explicitly acknowledged.
Automated review by github-manager-bot
| continuation = task.get("continuation") if isinstance(task.get("continuation"), dict) else {} | ||
| if not continuation: | ||
| return | ||
| continuation["status"] = "resolved" |
There was a problem hiding this comment.
_resolve_task_continuation unconditionally overwrites resolved_at with _utc_timestamp() on every call, unlike CoPaw's _commit_task_decision which uses continuation.get("resolved_at") or _now() to preserve the original timestamp. Today the callers guard against calling this for already-resolved continuations at a higher level, but the function itself is not defensive. If a future code path calls it for an already-resolved continuation (e.g. during a repair that re-runs after resolved_at was already set), it would silently rotate the timestamp, violating the design contract that resolved_at records the first resolution time and retries must not alter it. Consider using continuation.setdefault("resolved_at", _utc_timestamp()) or an early return when continuation.get("status") == "resolved".
| } | ||
| EFFECTIVE_RESULT_STATUSES = {"SUCCESS", "SUCCESS_WITH_NOTES"} | ||
| TERMINAL_TASK_STATUSES = {"completed", "revision", "blocked", "cancelled"} | ||
| _TASK_MUTATION_LOCKS: dict[str, threading.RLock] = {} |
There was a problem hiding this comment.
_TASK_MUTATION_LOCKS is a module-level dict that accumulates one threading.RLock per unique task meta.json path and is never pruned. In a long-running CoPaw process that handles many distinct tasks over its lifetime, this grows without bound. Each entry is small (string key + RLock), so this is unlikely to cause practical memory pressure, but it is a resource leak by design. Consider using a bounded cache (e.g. an OrderedDict capped at a few hundred entries with LRU eviction) or keying by (workspace_dir, task_id) with periodic cleanup.
| return false | ||
| } | ||
| for i, r := range s { | ||
| alphaNumeric := r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' |
There was a problem hiding this comment.
The tightened isPlainToken now requires the first character to be alphanumeric ([A-Za-z0-9]), rejecting identifiers that start with -, _, or .. This is a backward-incompatible change for the replacementTaskId parameter in POST /api/v1/projects/{id}/tasks/{taskId}/cancel. If any existing caller (human CLI scripts, automated tooling, or other runtime integrations) passes replacement IDs starting with _ or -, those requests will start returning 400 after upgrade. The test TestCancelTask_InvalidReplacementID400 explicitly validates that "_task" and "-task" are now rejected. Consider whether this tightening is necessary for the cancellation feature or if the old more-permissive check should be preserved for backward compatibility.
| "submit_task", | ||
| {"project_id": task.get("project_id", "")}, | ||
| task, | ||
| summary=f"submit_task: {task_id} ({status})", |
There was a problem hiding this comment.
In the submit_task retry path, the condition if not task.get("result_digest") or task.get("resultDigest") is misleading. After _load_task runs _ensure_console_task_meta, camelCase keys (including resultDigest) are popped from the task dict, so task.get("resultDigest") is always None/falsy at this point. The condition effectively simplifies to if not task.get("result_digest"). The dead or clause could confuse future maintainers into thinking camelCase migration is still needed here. Consider simplifying to if not task.get("result_digest"):.
| } | ||
| if str(taskData["task_id"]) != taskID || str(taskData["project_id"]) != projectID { | ||
| writeError(w, http.StatusNotFound, "task meta not found") | ||
| return |
There was a problem hiding this comment.
The new ownership check str(taskData["task_id"]) != taskID || str(taskData["project_id"]) != projectID correctly prevents cross-project task cancellation. However, if taskData["project_id"] is nil (absent from the stored JSON), str(nil) returns "", and the check "" != projectID would reject the cancellation with 404 even for a legitimately owned task. All tasks created by TeamHarness runtimes set project_id, so this should not affect normal workflows, but legacy or manually-created TaskMeta files without project_id would become un-cancellable through this API endpoint.
| @@ -3071,28 +3242,159 @@ def _payload_bool_field(payload: dict[str, Any], names: tuple[str, ...], default | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
The _accept_task_result function now requires role == "leader" via _role(arguments), and _role was changed to prioritize the runtime environment role (AGENTTEAMS_AGENT_ROLE) over the payload role argument. This is correct security hardening that prevents Workers from forging Leader operations. Verify that all production Leader deployment configurations set AGENTTEAMS_AGENT_ROLE=leader, since the argument-level role override is now ignored when a runtime role is present. Existing test infrastructure was updated accordingly.
|
Picked up two concrete review notes in Locally, the continuation, project-pull, and CoPaw taskflow suites pass (66 + 15 + 99), along with the Ruby contract/taskflow checks. The new GitHub run is fully green as well: TeamHarness, Controller, all image builds, and all five integration shards passed. |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Re-review of the incremental commit pushed since the last review (8d7594c..6bc6f1f): fix(teamharness): preserve first continuation resolution.
The change is correct and safe: preserving an existing resolved_at keeps repair/re-accept paths from overwriting the original resolution timestamp, and dropping the redundant resultDigest fallback is safe because _first_text() already prefers result_digest and _write_task normalizes camelCase keys before persisting. The new test directly exercises the preservation path on the repair flow.
LGTM. Trivial change, looks good.
Automated review by github-manager-bot
2 participants
Why
submit_taskwrites the Worker result before the Worker sends its separate completion message. If the process stops in between, the result survives but nothing reliably identifies that submission or records that the Leader still needs to review it.This PR adds that durable state contract to the CoPaw-native tools and the standalone TeamHarness MCP server. It also updates the Controller cancel endpoint from #1172 so a human cancellation cannot leave a submitted task with
continuation.status=pending.What changed
On first submit, TaskMeta records an immutable
submission_id,submitted_at, canonical result digest, and deterministic pending continuation ID. An identical retry reuses the first submission; a different result is rejected. Late Worker calls cannot move a terminal task back toin_progressorsubmitted.Leader accept/cancel decisions are fenced by the current
submissionId. Repeating the same decision is idempotent, while stale or conflicting decisions fail before state is changed. CoPaw and the standalone MCP path use the same result-status and continuation-resolution rules.The Controller's project cancel API now follows the same cancellation contract:
submissionId;cancelled;GET .../workflow?includeTasks=truenow returnstasks_detail[].submission_id. The CLI exposes the full path without guessing the current generation:agt get projects demo-project-001 --include-tasks -o json agt project cancel demo-project-001 demo-project-001-01 \ --reason "no longer needed" \ --submission-id submission-123 \ --team biz-teamLocal state files use same-directory atomic replacement. Result files and deliverables are uploaded and verified before remote TaskMeta is committed. Partial project/task writes return retryable state, and a matching retry repairs the missing projection without rotating the submission identity or repeating the terminal transition.
Compatibility
Existing CoPaw
submit_task()callers still receiveTaskMeta, and the legacy positionalevent_idslot is unchanged.Legacy tasks without a submission identity remain cancellable without
submissionId. Once TaskMeta has an identity, both TeamHarness and Controller cancellation require the exact value. The Controller API therefore treatssubmissionIdas conditionally required rather than globally required.The standalone MCP used to accept
FAILEDandPARTIALinsubmit_task, then fail later because neither status had an acceptance mapping. It now rejects both at submission time. This PR also addsINTERRUPTED, matching CoPaw's result-status set. A caller that still sendsFAILEDorPARTIALgetsunsupported result statusbefore task or project state is changed.Tests
Local checks:
go test ./internal/server ./cmd/agtgo vet ./internal/server ./cmd/agtgit diff --checkThe branch is rebased on current
main, including #1169 and #1172.Follow-up
This PR does not add the periodic scanner, wake a sleeping Team Leader, or send a recovery event through Matrix. It also does not automatically accept a Worker result.
The follow-up for #1177 is the Controller side: a leader-elected scanner over the canonical pending submissions, Leader lifecycle wake-up, and structured retryable Matrix delivery. TeamHarness remains responsible for eligibility, fencing, and terminal decisions.
Related to #1177.