Repository navigation
feat(workflows): retry agent nodes and register the resume contract as unavailable - #151
Conversation
38c4f0e to
68732cf
Compare
Kyber5323
left a comment
There was a problem hiding this comment.
Reviewed at 5607ad0 (base ed3badd). The retry core follows D1–D6 and I found no behavioral defect in the implementation:
- D1: the publish gate and the total runtime parser agree on the shape and bounds; absent/null/invalid reads as the default policy; the wait is
initialDelaySeconds × 2^(n-1)capped at 600s; the budget ismaxRetries + 1. - D2: the decision reads
result.reasonbefore the status mapping, onlyagent_failedretries, and theagent_plugin_unavailabledetail is lifted only at terminal settlement. - D3/D5: cancellation runs first, then the
retryingbranch (which always returns), then running-session settlement;retryAtcomes fromclock_timestamp(); cancellation clearsretryAtand starts no attempt even when the due instant has passed. - D4: migration 0062 is forward-only, adds
NOT NULL DEFAULT 1columns and re-keys the two partial unique indexes; the attempt is copied from the claimed work item, never from the wire; the claim join and delivery ownership check carry it; every evidence/end/cancel lookup is attempt-scoped; the Controller projection still excludes the internal column. - D7 and the contract: route shape, authorization-before-availability, 400 before the 501, no idempotency record, the
modeenum and the 501 description all match; the generated OpenAPI/frontend client and the zh/en docs are consistent.
Two evidence gaps to close before this can carry the "Only The Current Attempt Can Settle Its Node" core case:
-
workflow_retry_db_test.godoes not exercise cross-attempt command scoping. Every command path in the tests is attempt 1. The core case requires a direct PostgreSQL test where:- attempt 1 has an undelivered
end_sessionand attempt 2's session is live:claimThreadCommandsmust not return attempt 1's command for attempt 2, andrecordThreadCommandDeliveredwith attempt 1's command and attempt 2's execution must returndispatch_conflict; - a repeated/late attempt 1 terminal receipt is posted after attempt 2 starts, and the pass must leave
node_statesatrunning/attempt 2. The current replay assertion runs while the node is stillretrying, so it does not cover the attempt-2-in-flight projection.
The code for both is present and looks right; the tests are what is missing. This also lets the landing run record those obligations as
Coveredinstead ofMissing(specstest-cases/cloud/workflow/retry-and-resume.md). - attempt 1 has an undelivered
-
internal/core/migrations/README.md:9andREADME.en.md:9say 63 SQL files; the tree at5607ad0has 64. (The base was already one behind: 63 files, README 62.) Please set both to 64.
Non-blocking for the owner: I agree with the literal D2 reading of the #147 interaction recorded in the PR body — the decision keys on the session's result.reason, so a session-reported agent_failed with detail=agent_plugin_unavailable stays retryable, and narrowing the closed set is an ADR change, not an implementation detail. That question can be settled in the owner's review or a separate decision issue.
5607ad0 to
9f72d15
Compare
zhangshilong123
left a comment
There was a problem hiding this comment.
Reviewed at 9f72d15 (base d085bb9). The retry implementation matches D1–D6 and the resume registration matches D7, and the two evidence gaps from the earlier review are closed at this head: TestWorkflowRetryKeepsAttemptOneRequestAndResultOutOfAttemptTwo covers cross-attempt claim/delivery plus a late attempt-1 receipt while attempt 2 is in flight, and both migration READMEs now say 64. CI is green and the file list is all deliverables.
Two inaccuracies in the new nodeStates wording (internal/contract/openapi.go:137, regenerated into api/openapi.json and frontend/src/api/generated.schemas.ts):
- "
startedAtis kept from the first attempt" — a node that retries twice has aretryingstate whosestartedAtis attempt 2's start, not the first attempt's.startWorkflowAgentSessionwrites a freshStartedAtfor every attempt andcommitWorkflowAgentTerminalkeeps the failed attempt's value, so the description should say "the failed attempt's start". - "
finishedAtis absent" —workflowNodeState.object()always emitsfinishedAt; a retrying state carries"". Please say it is empty. (Omitting the key instead would change the wire shape for every status, so the wording is the smaller fix.)
Please fix both in openapi.go, regenerate the contract and the frontend client, and re-request review.
Non-blocking: resumeWorkflowRunAs (integration/workflow_run_resume_test.go:42) takes a uid parameter it never uses — the request identity comes from user — so it can be dropped.
|
@zhangshilong123 both wording issues are fixed on the new head
Local gates on |
zhangshilong123
left a comment
There was a problem hiding this comment.
Reviewed at 6cb8f7e (base d085bb9). The two requested fixes are correct: the nodeStates description now names the failed attempt's start and an empty finishedAt, api/openapi.json and frontend/src/api/generated.schemas.ts carry exactly the regenerated wording, and the unused uid parameter is gone from resumeWorkflowRunAs.
Two blocking items before this head can be reviewed for merge:
-
The branch is
CONFLICTINGwithmain(mergeStateStatus: DIRTY). The conflicts are 7 files underfrontend/src/features/workflows/, caused by #131 (f65172b) removingawaiting_inputwhile this branch still carries it from itsd085bb9base. Please rebase onto currentupstream/main(49a427eor newer) and resolve by droppingawaiting_inputand keepingretrying; then re-check that noawaiting_inputreference is left in the touched frontend files (run-node-state.ts,run-status-style.ts,run-status-mark.test.tsx,run-status-style.test.ts,translations.ts,types.ts). -
No CI has run on
6cb8f7e: the commit has 0 check-runs and there is no workflow run for it, so thetask check/task test:race/task frontend:checkresult claimed in ora-space/circus#469 is not evidenced on GitHub. After the rebase, push and let the Backend and Frontend workflows finish green; if they do not start, say so on the run issue rather than waiting.
Also, before merge the landing run has to open the specs PR that turns decisions/cloud/workflow/20261021-node-retry-and-resume-contract.md from approved into implemented. Per the protocol that PR is opened once this review passes and merged after this one, so the description's "follows after this PR merges" has the order reversed; please correct it when you update the PR.
6cb8f7e to
fa60a7e
Compare
zhangshilong123
left a comment
There was a problem hiding this comment.
Review 结论:需要修改(fa60a7e,base 6e302ef)
上一轮的两项修正、以及这次 rebase 对 cloud#131 冲突的处理都已核对:
nodeStates描述(失败尝试的开始时间、finishedAt为空)与resumeWorkflowRunAs的未使用参数清理都正确。frontend/src与api/openapi.json已无awaiting_input,retrying保留;internal/contract/openapi.go只保留“存储保留值、没有写入者”的注释。生成的契约与前端 client 无手改。fa60a7e上 7 个检查全部通过(check、frontend、integration-0–integration-3、packages)。- PR 描述里 ADR 保持
approved、implemented的 specs PR 在 review 通过后开、产品 PR 合并后合并,顺序正确。
但当前 head 还不能合并,原因都是 main 在本分支上次 rebase 之后又前进了:
-
与当前 main
5ba0d25冲突(mergeStateStatus: DIRTY)。冲突文件:internal/core/migrations/README.md、internal/core/migrations/README.en.md、internal/core/node_execution.go。node_execution.go里 main(#154/#158)把分发并发栅栏改成允许deliver_revision与deliver_scheduled_artifacts并存,本分支在同一个INSERT上加了workflow_node_attempt;rebase 时两者都要保留:main 的新栅栏加回去,INSERT带上 attempt 列并从领取的工单复制。请 rebase 到当前upstream/main。 -
迁移编号与 main 同号:main 现在已有
0063_scheduled_run_results.sql到0068_scheduled_manifest_completeness.sql,本分支的0063_workflow_node_attempt.sql需要顺延为0069_workflow_node_attempt.sql(memory/rules/migration-numbering.md),并同步:- 迁移 README 两种语言的行与计数。注意 main 的计数行目前写 62,但索引里已有 70 个文件(这是 main 已有的过时值);加上本迁移后正确是 71。
TestMigration0063WorkflowNodeAttemptAppliesFreshAndUpgrades测试名、migrationsBefore(t, "0063_workflow_node_attempt.sql")、before 0063等引用。internal/core/control_projection_db_test.go与internal/core/workflow_run_session.go里提到 0063 的注释。- PR 描述里的 base(
6e302ef→ 最新 main)和迁移编号。
rebase 后重新 push,等 CI 绿,再重新打 status:in-review,我复审。
Implement the agent-node retry contract from specs decisions/cloud/workflow/ 20261021-node-retry-and-resume-contract.md (D1–D6, D8): - publishing validates agentConfig.retry with four named gate codes; absent or null keeps the default policy, and frozen snapshots are never re-gated - a retryable agent_failed session commits `retrying` with a database-clock retryAt, keeps its evidence and the run's serial session slot, while the run stays running - the scheduler starts the next attempt only when retryAt is due, through the ordinary session path and under the run's version compare-and-swap - migration 0062 gives work items, executions and commands a per-attempt identity, re-keying the session and end-request uniqueness so attempt 2 can register without reusing attempt 1 evidence or end request - cancellation wins over a waiting retry; budget exhaustion, a disabled policy and every non-agent_failed reason fail the node and release the run - the run UI shows retrying, the attempt number and the next retry instant, and offers no resume entry Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
Implement the resume half of decisions/cloud/workflow/ 20261021-node-retry-and-resume-contract.md (D7–D8): - POST /api/v1/tenants/:tid/workflows/:wfid/runs/:rid/resume is authorized like cancel and validated strictly: an unknown field and a non-string, null or unknown mode are 400, while a valid keep/node_files/checkpoint request is always 501 workflow_run_resume_unavailable - a refused request writes no run state and records no idempotency key, so a replay receives the same refusal; the UI has no resume entry - the OpenAPI document and both workflow guides describe the retrying node shape, the attempt and retryAt fields, and the unavailable endpoint Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
Record the new migration in both migration indexes, including its upgrade path and coverage, so the schema history stays complete after upstream took 0061 for scheduled tasks. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
… resume The retrying node shape with its attempt and retryAt, the resume request and its 501 response are part of the published contract. The artifacts are produced by task frontend:generate on the current base and contain no hand edits. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
A Controller resending an already recorded agent_session result must be a replay, not a second settlement: the retrying state, its retryAt and the run version stay exactly as committed, and the replay releases no work. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
The per-attempt identity is migration 0062; upstream took 0061 for scheduled tasks. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
Upstream ora-space#147 derives the public reason agent_plugin_unavailable from a session's agent_failed plus detail. D2's retry set is keyed on the session's result.reason, and widening that set is an ADR change rather than an implementation detail, so the retry decision still reads agent_failed; the lifted reason is applied only at terminal settlement. The public-row test keeps its subject by seeding the no-retry graph, and a new retry test pins the reason/detail split through budget exhaustion. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
…t passes The wake runs under the run's exclusive transaction, so the pass that loses reads the winning attempt as running and releases nothing. Two replicas advance the same due retry under a synchronized start, and the test asserts the complete permitted outcome: attempt 2 running, exactly one released session work item, and no retryAt left behind. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
Attempt identity covers the whole session handshake, not just the session row: while attempt 2 runs, attempt 1's undelivered end request is neither claimable by the controller nor accepted as delivered to attempt 2, and delivering it there is refused with dispatch_conflict. Attempt 2's own turn end releases a fresh request, and replaying attempt 1's terminal result is a receipt replay that leaves the node and run on attempt 2. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
A node that retries twice has a retrying state whose startedAt is the failed attempt's start, not the first attempt's, because every attempt begins a fresh start instant and the failed attempt's value is kept. finishedAt is always emitted; a retrying state carries an empty string, not an absent key. Regenerate the OpenAPI document and the frontend client from the corrected description. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
resumeWorkflowRunAs takes the request identity from the verified claims, so its uid parameter only repeated a value the helper never read. Refs ora-space/circus#469 Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
main gained 0062–0068 while this branch was in review; the node attempt migration takes the next free number and its test, comments and migration README follow it. Co-Authored-By: Claude Code <noreply@anthropic.com>
The retried base gives enqueueExecutionWork a node attempt between the graph node and the availability instant. Scheduled artifact delivery has no graph node and is IssueRun work, so it passes an empty node and attempt zero like the other non-graph callers. Co-Authored-By: Claude Code <noreply@anthropic.com>
fa60a7e to
4a1efb2
Compare
|
Addressed both blockers in
Local @zhangshilong123 please take another look when you have a moment. |
zhangshilong123
left a comment
There was a problem hiding this comment.
已审 head 4a1efb2(base 5ba0d25)。上一轮提出的两项阻塞都已解决,本轮未发现新的阻塞项,Approve。
已核对通过
- rebase 冲突:
mergeStateStatus: CLEAN。node_execution.go同时保留 main 的双交付并发栅栏和本分支的workflow_node_attempt列;scheduled_artifact_delivery.go按非图节点给enqueueExecutionWork传空节点和 attempt 0,与其它 IssueRun 调用一致;attempt 只从领取的工作项复制到执行行,Controller 线上形状不变。 - 迁移编号:
0069_workflow_node_attempt.sql接在 main 最新的0068之后(main 70 个 SQL + 本迁移 = 71,README 两份的计数行、索引行、说明段、测试名TestMigration0069WorkflowNodeAttemptAppliesFreshAndUpgrades、升级起点0068与 schema 前缀都已同步),符合memory/rules/migration-numbering.md。 - 实现与 ADR D1–D7:发布门禁四个命名错误码与完整对象校验;
agent_failed按会话原因闭集判定后才映射状态;retrying的attempt/retryAt用数据库时钟计算、保留失败尝试的startedAt/error/output.reason并清空finishedAt,运行保持running且不释放 Workspace;到期只按已提交的retryAt唤醒且并发只放出一个 attempt;证据、end_session、取消路径全部按 attempt 限定,跨 attempt 投递被拒绝;取消优先于等待;预算耗尽按最后一次证据失败并释放;resume 按最终请求形状校验后一律 501、不写状态与幂等记录,没有 UI 入口。 - 测试:等待→到期→第 2 次尝试结算、并发 pass 只放出一个 attempt、跨 attempt 命令/结果隔离、预算耗尽、不可重试原因、取消优先、迁移升级,均有真实 PostgreSQL 证据。
- CI:
4a1efb2上check、packages、frontend、integration-0–integration-3七个检查全部通过。 - 范围与描述:改动文件只有代码、测试、迁移、生成的契约与前端客户端、正式文档;PR 描述写明 ADR 保持
approved及落地运行顺序。
后续
review 已通过,接下来由落地运行在 specs 开 PR 把 decisions/cloud/workflow/20261021-node-retry-and-resume-contract.md 改为 implemented,产品 PR 合并后合并该 specs PR。
Implements specs ADR
decisions/cloud/workflow/20261021-node-retry-and-resume-contract.mdon top of main5ba0d25. The ADR is approved and stays approved in this change. A separate specs landing run opens theapproved→implementedPR once this PR's review passes; that specs PR merges after this PR.What changes
agentConfig.retryis validated with the four named gate codes (agent_retry_not_an_object,agent_retry_enabled_invalid,agent_retry_max_retries_invalid,agent_retry_initial_delay_invalid). Absent or null selects the defaultenabled=true, maxRetries=2, initialDelaySeconds=10; frozen snapshots are never re-gated.agent_failedsession within budget commitsretryingwithretryAtfrom the database clock. The failed attempt's evidence andstartedAtare kept, the run staysrunning, and its serial session slot stays held.retryAtis due, through the ordinary session path, under the run's exclusive transaction and version CAS. The wait isinitialDelaySeconds × 2^(n-1), capped at 600s, and the budget ismaxRetries + 1attempts.0069_workflow_node_attempt.sqladdsworkflow_node_attempttoexecution_work,node_executionsandthread_commandsand re-keys the session and end-request partial unique indexes on it. Main took 0063–0068 while this branch was in review (the last,0068_scheduled_manifest_completeness.sql, from cloud#154), so this migration follows them; the migration README count and the test name/comments follow the new number. The attempt travels from the work item to the execution and command; the Controller wire shape is unchanged.agent_failedreason fail the node and release the Workspace.POST /api/v1/tenants/:tid/workflows/:wfid/runs/:rid/resumeuses the cancel authorization, accepts only optionalmode(keep,node_files,checkpoint), and answers every valid request with 501workflow_run_resume_unavailable. It writes no run state and no idempotency record, so a replay is the same refusal; there is no UI entry.retrying, the attempt number and the next retry instant; both workflow guides document the behavior and link the ADR. The frontend keepsretryingand carries noawaiting_input, which cloud#131 removed from the public status surface.Tests
task check— formatting, lint, proto drift, full PostgreSQL-backed suitetask test:race— race-enabled full suitetask frontend:check— generated-client drift, lint, types, tests, module docs, buildRebased onto main
5ba0d25.node_execution.gokeeps both main's dual-delivery concurrency fence and this branch's attempt column, and the scheduled-artifact enqueue passes the new attempt argument as IssueRun work. Regeneratingapi/openapi.jsonand the frontend client produces no diff.Refs ora-space/circus#469
🤖 Generated with Claude Code