Add runtime support for running same-worker Workflows - #7361
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
I'm Bonk, and I've done a quick review of your PR. Adds runtime wiring for same-worker Workflow bindings through
|
kentonv
left a comment
kentonv
left a comment
There was a problem hiding this comment.
This PR seems to contain no comments.
Comments are important. We should not be writing code without comments. It is difficult for a code reviewer to know what they are looking at without comments. As nice as it would be for code to be "self-documenting", in reality, it usually isn't.
It's especially important that workerd.capnp contain detailed comments as that file serves as the public-facing documentation of our config format. Every declaration in that file needs comments.
In general, we also expect that every declaration in a header file and most declarations in source files have doc comments, unless their meaning is obvious. And big or complicated chunks of implementation code should also have narrative comments explaining at a higher level what is going on.
bd806b7 to
b68f3d4
Compare
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Approved from the point of view of the typings.
I didn't look deeply into server.c++ as I don't have much knowledge there.
7aec43d to
d74bf56
Compare
d74bf56 to
f2e6600
Compare
7720781 to
9783c2c
Compare
Add explanatory doc and narrative comments describing the workerd-side support for exposing configured Workflows on ctx.exports: the synthetic loopback-workflows-<name> namespaces, the two-phase service linking that sources Workflow storage from the bindingService Worker, the shared engine actor class driven by per-Workflow props, and the wrapped ctx.exports bindings. No behavior change.
serviceActorConfigs.insert() copied namespaceKey for the map key while moving it into Durable::uniqueKey in the same call. On the MSVC ABI arguments are evaluated right to left, so the key was built from the already-moved-from string and the Workflow namespace was registered under an empty name, failing the ctx.exports Workflow tests on Windows.
Miniflare's Workflow engine namespaces use `miniflare-workflows-<name>` as their unique key, which determines actor IDs and the on-disk directory. Using the same prefix for the synthetic namespaces behind ctx.exports Workflows lets Miniflare serve existing local instances through ctx.exports, so env bindings and ctx.exports share one set of instances. The prefix is hardcoded for now; a TODO tracks making it configurable.
Move Durable::isWorkflow next to isEvictable and enableSql, and reorder the synthetic Workflow namespace's designated initializer to match the new declaration order. No behavior change.
workflowActorStorageSources recorded each Workflow namespace's bindingService Worker as a raw WorkerService pointer. Hold a kj::Own<WorkerService> instead, as EntrypointService and ActorClassImpl already do, and clear the map in unlink() so the reference is released before the server verifies that unlinking removed every refcount cycle.
9783c2c to
0c8dbcf
Compare
5 participants
Adds support for declaring, per worker, a "workflow engine" that will run workflows declared on that same worker. Useful for dealing with same-worker Workflows, which should appear in ctx.exports. and are built by the runtime (as opposed to the current local-dev Workflow bindings which are injected externally, usually by Miniflare).
On any given worker, a "workflows engine" can be configured. This engine: 1) points to an actor class that implements the engine's logic and 2) a white list of workflows that can be ran by that same engine.
For the actor class, the idea is that we need to let the runtime know what runs workflows or how to run them, since it cannot do this natively. For normal bindings, in local-dev, it is usually Miniflare that configures the necessary services needed for running a workflow as well as linking everything up. In the case of
ctx.exports, since it is the runtime itself that builds this object from the worker's own exported entrypoints, it needs to know what runs workflows: that is the service designated byactorClass. Furthermore, similar to what happens today, each workflow has its own actorNamespace associated with the workflow's own instances, but the actor class implementing the engine is the same and is pointed to byactorClass.For the white list, since binding presence in
ctx.exportsis config driven, we need some external source to tell the runtime which workflows it should include in that same object: if a Workflow is exported in code but is not declared as exported, it is effectively not exported as a Workflow at all.