Skip to content

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

Open
testikun wants to merge 7 commits into
openpi-dev:mainfrom
testikun:codex/issue-110-artifact-commit
Open

fix(workflows): recover terminal artifact commits#386
testikun wants to merge 7 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

Workflow terminal persistence writes result.json, transcripts.json, optional journal.json, and the final referencing workflow.json as separate atomic replacements. A process crash after the side artifacts but before the final manifest can leave the earlier manifest without references and the terminal evidence orphaned.

Current main already publishes a reference-free terminal manifest first, which prevents a known terminal run from remaining running, but it cannot prove that an otherwise orphaned artifact belongs to a complete terminal commit.

Closes #110.

Value

Restart recovery can now distinguish a fully prepared terminal artifact set from partial, stale, substituted, or malformed files. Exact terminal results remain discoverable after the crash window without treating an arbitrary result.json as completion evidence.

Approach

  • Write one bounded, versioned .workflow-commit.json receipt before the terminal side-artifact sequence. It records the exact final manifest plus the fixed artifact names, byte counts, and SHA-256 digests.
  • Keep workflow.json canonical: publish the existing reference-free terminal projection first, prepare every side artifact, atomically commit the exact recorded manifest last, then remove the receipt.
  • Recover at the persisted Workflow read boundary and before delivery-state mutation. Promotion requires an exact run-directory identity, known terminal status, fixed path whitelist, matching manifest references, regular non-symlink files, byte budgets, lengths, and digests.
  • Leave incomplete or invalid receipts fail-closed; a normal retry replaces the single fixed receipt. Legacy runs without receipts retain their old behavior, and orphan result.json alone never proves completion.
  • Document the ownership, sequence, recovery outcomes, compatibility, and fsync limitation in docs/architecture/WORKFLOW_ARTIFACT_COMMIT.md, linked to workflows: terminal artifact set (result.json + workflow.json) is not committed atomically #110.

Validation

  • bun run check — passed.
  • Focused artifact and dashboard tests — 42 passed, 0 failed.
  • bun run test — Node tests: 1247 passed, 0 failed, 1 skipped; Vitest: 30 passed.
  • Regression cases include complete crash recovery, missing artifacts, same-size digest substitution, already-committed idempotence, delivery update after recovery, dashboard/startup hydration, normal cleanup, and dependent-write failure.

Impact

  • User-visible behavior: no UI changes.
  • Model-visible context/tools: none.
  • Runtime/lifecycle: terminal Workflow artifact persistence gains a recoverable commit receipt at the existing filesystem boundary.
  • Persisted config/data: no configuration changes. New terminal writes may briefly contain .workflow-commit.json; successful writes remove it, and legacy artifacts remain readable.
  • Compatibility/risk: filenames and canonical manifest references remain unchanged. Recovery fails closed on unknown versions, identities, paths, states, sizes, symlinks, or hashes. The guarantee covers process-crash recovery at the existing atomic-rename boundary, not power-loss durability beyond writeFileAtomic.

@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 已通过。

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