Refactor job output plumbing - #4500
Conversation
@wasp.sh/spec
@wasp.sh/wasp-cli
@wasp.sh/wasp-cli-darwin-arm64-unknown
@wasp.sh/wasp-cli-darwin-x64-unknown
@wasp.sh/wasp-cli-linux-arm64-glibc
@wasp.sh/wasp-cli-linux-x64-glibc
@wasp.sh/wasp-cli-linux-x64-musl
commit: |
Deploying wasp-docs-on-main with
|
| Latest commit: |
fe3052f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7cdf1934.wasp-docs-on-main.pages.dev |
| Branch Preview URL: | https://miho-job-output-plumbing.wasp-docs-on-main.pages.dev |
e744a06 to
0f434bd
Compare
cprecioso
left a comment
There was a problem hiding this comment.
I'm not sure if we're re-inventing involved code for what is actually somewhat generic subprocess handling.
The JobOutputSink is a bit nebulous. I had a hard time grasping that in the end is just a function that will do some side-effect on output, mainly logging or collecting, and that by virtue of being a function we can "tap" into that stream and do extra side effects on specific output (e.g. triggering an error or a recompile). Fine, a bit underexplained in the current state of the code).
But then, we only build such a sink in one place in the code, the same as where the type is defined, but it feels like the type is leaking everywhere else in the code (also taking about the next PR).
[look at comment below and come back -- storytelling ✨]
So I'm thinking that we could scrap the JobOutputSink and fold it into Job, so it doesn't leak out by default. It feels like we'd like something like WriterT (actually, we want streaming output so we'd have to use conduit, which we already depend on), so we'll need to turn Job into a monad and that will buy us the programmatic behaviors we want.
In the end I think I'd like to, rather, turn Job into
type Job = ExceptT JobExit (ConduitT String (JobOutputType, String) IO ExitCodewhich would allow us to reduce our NIH boilerplate code and just use common facilities for what we want to do. There's also some Subprocess stuff in conduit-extra (which we also already depend on).
If you're not convinced, at least I'd like to see the JobOutputType concept better explained, and folded into Job in some way so that this very specific (IMO) concern doesn't have to leak everywhere.
d39a104 to
4761c1d
Compare
8c28e16 to
16d9bdb
Compare
b1371f6 to
fe3052f
Compare
cprecioso
left a comment
There was a problem hiding this comment.
Comments/proposals/questions on naming, architecture and semantics.
Most of them are nitpicks or proposals that you might have already tried so you know it doesn't work, I trust your judgement so do what you want.
In general, I'm happy that you effectively controlled the spread of JobOutputType. In exchange, it's the API does feel a bit too extensive now. 6 data types for what amounts to a stream of [{stdout, stderr}, String] and a final ExitCode. And the Job/Subprocess differentiation feels a bit arcane. As I commented below, I think there are conceptual simplifications hiding in places.
But also, I trust you've fought against the tide here so you know best what is possible, feasible, and understandable. Feel free to call me out when needed.
| newtype JobFailure = JobFailure Int | ||
|
|
||
| newtype JobOutputSink = JobOutputSink | ||
| { writeJobOutput :: JobOutputStream -> Text -> IO () |
There was a problem hiding this comment.
This API is named as if consumers will use it directly, but not very ergonomic, as it has to getJobOutputSink first always when it just wants to write JobOutput.
Instead I would actually name this one getJobOutputSink or whatever, and then make a writeJobOutput = getJobOutputSink . ask function that deals with getting the sink directly
There was a problem hiding this comment.
Mmm i'm thinking you might not be able to do that because you'd be running inside the monad and blocking other stuff, but you want to do stuff concurrently. So RAW.
There was a problem hiding this comment.
emitJobOutput is now the normal API and gets the sink itself. The raw sink functions stay internal for background IO workers. I tried running those workers directly as concurrent JobActions, but that stack cannot provide the MonadUnliftIO instance the async API needs.
There was a problem hiding this comment.
This file is a bit meh, feels like there's a better API from the Job side hiding in here. But you know best, RAW.
There was a problem hiding this comment.
You were right that there was an unnecessary layer here. ExceptT was being added and immediately removed, so I changed JobExecution to return IO (Either String ()) directly.
There was a problem hiding this comment.
Let me see if I can also get rid of this module.
| printEventsPrefixedUntilExit :: Chan Job.JobEvent -> IO () | ||
| printEventsPrefixedUntilExit chan = runPrefixedWriter go | ||
| where | ||
| go = do | ||
| event <- liftIO $ readChan chan | ||
| case Job._eventData event of | ||
| Job.JobOutput {} -> printEventPrefixed event >> go | ||
| Job.JobExited {} -> return () |
There was a problem hiding this comment.
suggestion:
instead of
printEventsUntilExit foo
printEventsPrefixedUntilExit barwhy not
printEventsUntilExit foo
printEventsUntilExit $ prefixEvents <$> bar(i'm not sure if the <$> is possibe here but a regular $ would probably be)
There was a problem hiding this comment.
Also then you can move it somewhere associated with JobType, because Output.printPrefixed immediately makes me ask "what are we prefixing it with" but Output.print $ JobType.prefix makes it clear what I'm working with.
There was a problem hiding this comment.
The exact <$> version does not work because Chan is not a functor, but the shared-loop idea does. I added one consumeEventsUntilExit helper and pass either printer to it. I kept prefix rendering under Output because it also tracks stderr, interrupted lines, previous events, output handles, and pending newlines; only the label and color come from JobKind.
| -- TODO: | ||
| -- Switch from Data.Conduit.Process to Data.Conduit.Process.Typed. | ||
| -- It is a new module meant to replace Data.Conduit.Process which is about to become deprecated. |
There was a problem hiding this comment.
I guess this TODO is too big to do now? Write an issue for it maybe?
There was a problem hiding this comment.
This is an old TODO and I investigated rewriting to CreateProcess configuration obj from Typed module, but it looked like a big refactor and I wanted to control scope (as much as I can lol). I'll create an issue, good call out!
|
The main distinction is lifecycle: a Job is something we observe once and give one final result, while a subprocess is a child resource the Job can stop, replace, or recover from without ending. The Server Job needs that split because its server process can restart many times. I simplified the smaller API pieces you pointed out, but kept this boundary. |
Description
Split from #4407 so the Job refactor can be reviewed separately from the server lifecycle change.
The new Job model
A
Jobis one complete top-level operation, such as building the SDK or running the server. It owns:JobKind, used to label its output;JobAction, which can emit output, fail with an exit code, and register cleanup work;JobEvents: output while the Job is running, followed by one exit event when it finishes.Only the Job runner emits the exit event. A subprocess can forward stdout and stderr, but its exit does not pretend that the whole Job ended.
Each Job runs in its own
ResourceT, so registered resources are released before the Job reports that it exited. Cancellation releases resources without reporting a normal exit.Subprocesses and output
Wasp.Job.Subprocess.runhandles a finite child process and returns its exit code insideJobAction.Wasp.Job.Nodebuilds on that for Node and npm commands.Output reading carries incomplete UTF-8 bytes between chunks, so a character split across two reads is decoded correctly.
Review map
Mostly code moving or renaming:
Wasp.Job.IO.PrefixedWritermoves toWasp.Job.Output.Prefixed. Most edits there only adaptJobMessage/JobTypenames toJobEvent/JobKind.Wasp.Jobmodule and intoWasp.Job.Node.runJob,JobAction, and the new output helpers.New behavior worth reviewing:
JobAction.Type of change
Checklist
I tested my change in a Wasp app to verify that it works as intended.
🧪 Tests and apps:
examples/kitchen-sink/e2e-tests.waspc/data/Cli/templates, as needed.examples/, as needed.examples/tutorials) I updated the tutorial in the docs (and vice versa).📜 Documentation:
web/docs/.🆕 Changelog: (if change is more than just code/docs improvement)
waspc/ChangeLog.mdwith a user-friendly description of the change.web/docs/migration-guides/.versioninwaspc/waspc.cabalto reflect the changes I introduced.