Skip to content

fix(runtime): persist external tool effects - #449

Open
AsperforMias wants to merge 2 commits into
langgenius:mainfrom
AsperforMias:fix/runtime-persist-external-tool-effects
Open

fix(runtime): persist external tool effects#449
AsperforMias wants to merge 2 commits into
langgenius:mainfrom
AsperforMias:fix/runtime-persist-external-tool-effects

Conversation

@AsperforMias

@AsperforMias AsperforMias commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Persist MCP external-effect intent, attempt, result, and unknown-fence state in D1.
  • Redeliver a durable MCP result after terminal-receipt loss without invoking the provider again.
  • Keep a terminal Driver record while it has an executing or unknown external effect, so routine expiry cleanup cannot cascade-delete the unresolved fence.
  • Require Driver protocol v2 and the durable effect ledger for MCP execution.

Why

Verification

  • just check
  • just test-file apps/api/tests/driver-instance-record.test.ts
  • just test-file apps/api/tests/driver-finalization-repair.test.ts
  • Local fault injection uses D1 plus the real Driver Dispatcher to simulate provider success followed by lost terminal delivery, restarts the Dispatcher, and asserts exactly one provider call.
  • Not run: live remote MCP or Cloudflare resources (not available locally).

Impact

  • User/API/contract changes: Driver control protocol v2; an unknown effect fails closed with its durable effect ID.
  • Generated files / GraphQL / DB / lockfile: additive 0005_external-tool-effects plus Drizzle snapshot/journal; no GraphQL or lockfile changes.
  • Env or config changes: none.
  • Risk and rollback: the migration is additive; old Driver versions are intentionally rejected to prevent unsafe execution.

Review

@AsperforMias

AsperforMias commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

这个要driver那边的#89先合 @Yevanchen

@AsperforMias
AsperforMias force-pushed the fix/runtime-persist-external-tool-effects branch 2 times, most recently from 2bd7bc5 to da90cf5 Compare July 29, 2026 14:23
@AsperforMias
AsperforMias marked this pull request as ready for review July 29, 2026 14:29
@Yevanchen
Yevanchen force-pushed the fix/runtime-persist-external-tool-effects branch 4 times, most recently from a32ddb0 to 7679470 Compare July 31, 2026 16:09

@Yevanchen Yevanchen 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.

The durable fence is directionally correct, but one persistence invariant is currently lost: unknown effects cascade-delete with their Driver, and maintenance deletes stopped/failed Drivers after 24 hours. That erases the evidence the PR says requires explicit resolution. Please decouple unresolved-effect retention or exclude such Drivers from cleanup and add the retention regression test.

Separately, the PR currently conflicts on apps/driver and points at an obsolete Driver commit; after Driver #89 lands, rebase/update the gitlink. Both this PR and #481 currently allocate migration 0005, so whichever lands second must generate the next append-only migration rather than reusing 0005.

createdAt: integer("created_at").notNull(),
driverInstanceId: platformIdColumn<DriverInstanceId>("driver_instance_id")
.notNull()
.references(() => driverInstancesTable.id, { onDelete: "cascade" }),

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.

cleanupDriverInstances() deletes stopped/failed Driver rows at expiresAt (24 hours), and this cascade then deletes unknown effects plus their attempt/receipt evidence. That contradicts the explicit-resolution fence: after one day Mosoo no longer knows the action was uncertain. Preserve unresolved effects independently, or prevent Driver cleanup while an executing/unknown effect exists, and cover that retention boundary.

@AsperforMias
AsperforMias force-pushed the fix/runtime-persist-external-tool-effects branch from 7679470 to af0bbb7 Compare August 3, 2026 07:51
@AsperforMias

Copy link
Copy Markdown
Collaborator Author

Addressed the requested retention invariant and rebased onto current main. Terminal Driver cleanup now skips records with executing or unknown external effects, preventing the Driver/command cascade from erasing the durable fence. Added direct maintenance and end-to-end persistence regressions; just check and both focused tests pass. The submodule now points at merged Driver #89 (ddbb4305).

@AsperforMias

Copy link
Copy Markdown
Collaborator Author

CI diagnosis: #449 itself does not fail a runtime-persistence test. Its updated Driver #89 gitlink lacks the package-manager manifest that current Mosoo main expects from the earlier #340 gitlink; the contract test fails with ENOENT for apps/driver/environment-package-managers.json. Driver #48 restores that manifest on current Driver main and is now the required merge-order dependency. I have corrected the PR description accordingly; #449 must be refreshed to the post-#48 Driver main commit before merge.

@AsperforMias

Copy link
Copy Markdown
Collaborator Author

#449 的CI 依赖 driver #48,driver #89 的当前 main 缺少 mosoo #340 所依赖的 package-manager manifest。driver #48 合并后需刷新 gitlink,才能合 #449

@Yevanchen

Yevanchen commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Review update: the retention fix and regression now address the inline durable-fence finding, but the PR is still not merge-ready. Driver #48 merged on Aug 6; please rebase onto current Mosoo main, update the Driver gitlink to the post-#48 commit, and generate the next append-only migration with just db-generate instead of retaining stale 0005. Then resolve the remaining review state and rerun just check. Issue #412 stays open until this lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(runtime): persist uncertainty for external tool effects

2 participants