fix(runtime): persist external tool effects - #449
Conversation
|
这个要driver那边的#89先合 @Yevanchen |
2bd7bc5 to
da90cf5
Compare
a32ddb0 to
7679470
Compare
Yevanchen
left a comment
There was a problem hiding this comment.
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" }), |
There was a problem hiding this comment.
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.
7679470 to
af0bbb7
Compare
|
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; |
|
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. |
|
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 |
Summary
Why
Verification
just checkjust test-file apps/api/tests/driver-instance-record.test.tsjust test-file apps/api/tests/driver-finalization-repair.test.tsImpact
0005_external-tool-effectsplus Drizzle snapshot/journal; no GraphQL or lockfile changes.Review