Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multiple workflows and add proper engine persistance #7286

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

LuisDuarte1
Copy link
Contributor

@LuisDuarte1 LuisDuarte1 commented Nov 18, 2024

Fixes #7127.
Closes WOR-322, WOR-395

Previously, engine did store everything in-memory - this is not expected behaviour since R2/KV/DOs all have persistenece, by default.

Also, multiple workflow definitions were broken since the DO unique key was a constant (therefore it would fail when workerd launched)


  • Tests
    • TODO (before merge)
    • Tests included
    • Tests not necessary because:
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required
    • Not required because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Documentation not necessary because: there isn't anything to document (except changelog, which will come later) - this is expected behavior

@LuisDuarte1 LuisDuarte1 requested a review from a team as a code owner November 18, 2024 18:42
Copy link

changeset-bot bot commented Nov 18, 2024

🦋 Changeset detected

Latest commit: 4a17f40

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@cloudflare/workflows-shared Minor
miniflare Minor
@cloudflare/pages-shared Patch
@cloudflare/vitest-pool-workers Patch
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@LuisDuarte1 LuisDuarte1 force-pushed the lduarte/fix-multiple-workflows-miniflare branch from f858f6c to 9371733 Compare November 18, 2024 18:57
Copy link
Contributor

github-actions bot commented Nov 18, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-wrangler-7286

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7286/npm-package-wrangler-7286

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-wrangler-7286 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-create-cloudflare-7286 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-cloudflare-kv-asset-handler-7286
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-miniflare-7286
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-cloudflare-pages-shared-7286
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-cloudflare-vitest-pool-workers-7286
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-cloudflare-workers-editor-shared-7286
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-cloudflare-workers-shared-7286
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11940185264/npm-package-cloudflare-workflows-shared-7286

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20241106.0
workerd 1.20241106.1 1.20241106.1
workerd --version 1.20241106.1 2024-11-06

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

@LuisDuarte1 LuisDuarte1 added the e2e Run e2e tests on a PR label Nov 19, 2024
fixtures/workflow-multiple/worker-configuration.d.ts Outdated Show resolved Hide resolved
@@ -62,15 +62,20 @@ export const WORKFLOWS_PLUGIN: Plugin<
sharedOptions.workflowsPersist
);
await fs.mkdir(persistPath, { recursive: true });
const storageService: Service = {
name: WORKFLOWS_STORAGE_SERVICE_NAME,
// each workflow should get its own storage service
Copy link
Contributor

Choose a reason for hiding this comment

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

sanity check for me: why does each workflow need its own storage service? as far as i can tell all other plugins use one storage service for all instances.

Copy link
Contributor Author

@LuisDuarte1 LuisDuarte1 Nov 19, 2024

Choose a reason for hiding this comment

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

It was personal preference mostly - this way each workflow gets its own folder in .wrangler/workflow and makes it way easier to debug the engine state

@masylum
Copy link

masylum commented Nov 19, 2024

Will adding persistence enable workflows to return serializable values like steps do? This would be great to compose workflows.

@LuisDuarte1
Copy link
Contributor Author

Will adding persistence enable workflows to return serializable values like steps do? This would be great to compose workflows.

I'm sorry, not sure what you mean by that - afaik you shouldn't have any problem composing workflows atm

@masylum
Copy link

masylum commented Nov 20, 2024

Will adding persistence enable workflows to return serializable values like steps do? This would be great to compose workflows.

I'm sorry, not sure what you mean by that - afaik you shouldn't have any problem composing workflows atm

I meant being able to call a workflow and get the return value. Right now you can only compose by having them triggering each other, like queues. But it would be great to be able to have them act like steps, with a return value. This is how it's implemented at inngest: https://www.inngest.com/docs/reference/functions/step-invoke

@LuisDuarte1
Copy link
Contributor Author

I meant being able to call a workflow and get the return value. Right now you can only compose by having them triggering each other, like queues. But it would be great to be able to have them act like steps, with a return value. This is how it's implemented at inngest: https://www.inngest.com/docs/reference/functions/step-invoke

That is unrelated to the PR - but you can already do this - you can pass the workflow output as an event payload to the next. What you are describing is syntactic sugar for that, which we might (or might not) consider in the future 😄 thanks ^^

@LuisDuarte1 LuisDuarte1 force-pushed the lduarte/fix-multiple-workflows-miniflare branch from 520685f to ec0404b Compare November 20, 2024 14:02
Copy link
Contributor

@emily-shen emily-shen left a comment

Choose a reason for hiding this comment

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

looks good from the wrangler side pending a tiny fixup

fixtures/workflow-multiple/tests/index.test.ts Outdated Show resolved Hide resolved
packages/miniflare/test/plugins/workflows/index.spec.ts Outdated Show resolved Hide resolved
@LuisDuarte1 LuisDuarte1 force-pushed the lduarte/fix-multiple-workflows-miniflare branch from 764a168 to 4a17f40 Compare November 20, 2024 19:14
@LuisDuarte1 LuisDuarte1 merged commit 563439b into main Nov 20, 2024
30 of 32 checks passed
@LuisDuarte1 LuisDuarte1 deleted the lduarte/fix-multiple-workflows-miniflare branch November 20, 2024 19:33
@davidhollenbeckx
Copy link

@LuisDuarte1 you're my hero man. multiple workflows on local working great now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
Projects
Status: Done
4 participants