Skip to content

fix(workflows): recover terminal artifact commits - #386

Merged
tt-a1i merged 9 commits into
openpi-dev:mainfrom
testikun:codex/issue-110-artifact-commit
Sep 7, 2026
Merged

fix(workflows): recover terminal artifact commits#386
tt-a1i merged 9 commits into
openpi-dev:mainfrom
testikun:codex/issue-110-artifact-commit

Conversation

@testikun

@testikun testikun commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

A Workflow process crash after writing terminal side artifacts but before the final manifest can orphan valid result/transcript evidence. The initial recovery design also allowed an older completed receipt to overwrite newer failed or cleanup facts. Closes #110.

Value

Verified terminal artifacts become discoverable after a crash without treating arbitrary files as success, and recovery cannot roll back a newer canonical manifest.

Approach

Publish the terminal reference-free manifest, then a bounded receipt containing its exact SHA-256 predecessor identity, the intended final manifest, and fixed side-artifact names/sizes/digests. Promote only when artifacts validate and the canonical predecessor still matches. Already-committed manifests retain later delivery/resource updates. Missing, modified, malformed or superseding canonical state fails closed. Run recovery at the shared persisted-read boundary and before delivery updates.

Validation

Integrated current main and repaired the reproduced failed-to-completed rollback. Focused regressions passed 49/49. bun run check and standard bun run test passed: 1466 Node tests, one platform skip, Vitest 30/30. Two independent final reviews are clean on 3710707286930481c0b95af316abd911d8e6afef, integrated with main b0fc5588cd6ae514cc4a64db87ab1c899d2f034f including #423. Final-head remote CI passed on Node 22.19.0, Node 24 and Windows: https://github.com/openpi-dev/openpi/actions/runs/34078645725. The historical requested change about already-committed identity is addressed, as acknowledged by its reviewer; the final implementation also guards the uncommitted predecessor against newer canonical updates.

Impact

No model tool, permission or configuration change. Persisted terminal writes briefly include a bounded .workflow-commit.json receipt. Legacy runs without receipts remain readable; the unshipped receipt shape now requires predecessorSha256. If an uncommitted manifest changes before recovery, recovery conservatively retains that newer canonical state and the receipt for inspection rather than silently replacing it. The protocol covers process-crash recovery across existing atomic file replacements, not power-loss durability/fsync or a filesystem-wide transaction.

Architecture and evidence.

@github-actions github-actions Bot added documentation Improvements or additions to documentation area:workflows Workflow engine, capability, skills, or tests labels Sep 4, 2026
@testikun

testikun commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

CI status note: Node 22/24 completed this PR's repository checks and failed only at the final fresh-install Smoke-test packed standalone Web CLI step, where the child exits before the test can read its URL. The same failure now occurs on unrelated PRs #385, #386, and #387.

I reproduced the packed install independently: npm now admits Pi 0.85.0 through OpenPI's published peerDependencies: "*"; the CLI then exits with Cannot find module '@earendil-works/pi-server'. The checkout/lock-based bun run check and full test suite remain green. I recorded the shared packaging regression and suggested peer-range follow-up in #328: #328 (comment)

No dependency workaround is mixed into this feature PR.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

需要改:「已提交」不要用整份 workflow.json 字节当身份

文档写 crash 在 unlink 之后是 harmless,因为 recovery 认得 already-committed manifest。实现用 整份 workflow.json 字节相等 判断 already-committed。

这不是无害窗口:persistWorkflowDeliveryState#382 的 delivery owner、#379resourceRefs 都会改同一份文件。下一轮 recoverPendingWorkflowCommit 会把收据里更旧的 manifest 写回去。

请用 run id + 终态 + artifact 引用(名字/digest)判断 already-committed,不要比整文件字节。和 #379/#382 叠在 artifacts.ts 上之前,先把这个身份修好。

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head follow-up on fd2842f: the prior already-committed identity blocker is addressed. The implementation compares runId/status/artifact references after checking artifact size/digests, rather than requiring whole-file manifest equality.

Verification in an isolated exact-head worktree: artifacts tests 18/18, dashboard tests 25/25, plus a direct probe confirming that delivery/resourceRefs survive a leftover receipt, a pre-manifest crash is recovered, and an incomplete receipt does not publish references. All commands exited 0. No new P0/P1 was confirmed in this bounded review.

Please update the architecture text below to match the corrected identity rule. This PR changes recovery writes reached through persisted workflow reads, so this comment is not final integration/runtime acceptance: current-main integration and its full required checks remain outstanding. No branch update or merge was performed in this review.

Comment thread docs/architecture/WORKFLOW_ARTIFACT_COMMIT.md Outdated
@testikun
testikun force-pushed the codex/issue-110-artifact-commit branch from f787df3 to 68774e1 Compare September 6, 2026 06:19
@testikun

testikun commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@tt-a1i 已修复:already-committed 改为校验 runId、terminal state、artifact refs/digest,不再使用整份 workflow.json 字节相等。

@testikun

testikun commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@tt-a1i 已同步 architecture record 与 source boundary,说明已改为 runId + terminal state + artifact refs/digest;当前 head CI 已通过。

@tt-a1i
tt-a1i merged commit 0b28243 into openpi-dev:main Sep 7, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:workflows Workflow engine, capability, skills, or tests documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

workflows: terminal artifact set (result.json + workflow.json) is not committed atomically

2 participants