Watch task outputs to avoid cache restorations #1269

gsoltis · 2022-05-19T23:33:13Z

Start a daemon, notify it of output globs when a task completes, watch those outputs, check if we can skip cache restores on subsequent runs.

Uses grpc and protobuf for communication.

Before shipping:

vercel · 2022-05-19T23:33:16Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
turborepo	❌ Failed (Inspect)			Jun 27, 2022 at 4:37PM (UTC)

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
turbo-site	⬜️ Ignored (Inspect)			Jun 27, 2022 at 4:37PM (UTC)

gsoltis · 2022-05-27T22:58:00Z

On the topic of ensuring that we aren't racing with the filesystem event notifications, watchman uses cookies. We briefly discussed this idea as "writing to a well known file after an operation completes, then waiting for that event". They also have some caveats re: FSEvents on macOS. We are not currently using FSEvents, as that requires CGO. https://facebook.github.io/watchman/docs/cookies.html

More investigation is still required here.

gsoltis · 2022-05-27T23:08:00Z

After the first pass review, and looking over the use of the unix domain socket and pid files, I'm going to make some changes to how we implement the checks for the server being alive. I think the best path forward is to synchronize on the pid file. Whoever owns the pid file is allowed to do whatever they want with the socket file. The server will attempt to lock the pid file, and on success delete any existing socket file and create their own.

The client will try to find a live process matching the pid file. If the pid file does not exist, try starting a server. If it exists but the server is not responding, try killing it. If the pid file exists but has no matching process, delete it and try starting a server, or report an error to have the user delete the pid file. At no point will the client manipulate the socket file.

It is possible for two clients to race trying to delete a pid file and start a new server, so it may be better to just report an error, or allow for this behavior to be controlled via flag (--force-daemon or something).

gsoltis · 2022-05-31T22:32:19Z

Investigated golang internal lockfile. I think it's overkill for what we need, especially given that we'd have to do some work to use it, but if someone publishes a nice library around it, we could potentially drop it in.

nathanhammond

Rather than delaying anything, just going to get comments in when I have them.

cli/internal/server/turbo.proto

cli/internal/server/server.go

nathanhammond · 2022-05-31T14:26:40Z

cli/internal/server/turbo_grpc.pb.go

@@ -0,0 +1,215 @@
+// Code generated by protoc-gen-go-grpc. DO NOT EDIT.


Should we be checking this generated code in?

I'm struggling to come up with a good opinion on this. There is definitely an argument for "don't check in generated code" on the one hand, but on the other hand, it doesn't seem great to have to run a generate step before vscode will be happy. make turbo is properly wired up so that it will work either way.

My inclination is to bias towards ease-of-contribution, since I don't think we're particularly susceptible to the problems with checked-in generated code (it's stale -- auto run by our build process, people modify it -- we can watch for it in reviews). But I don't feel super strongly about it.

We could at least partially alleviate the issue using VS Code's tasks.json (which we should do a better job leveraging at some point anyway).

They have a "runOptions": { "runOn": "folderOpen" }" that would make things happy. The other thing is I'm not sure if it will ever generate platform-specific code?

nathanhammond · 2022-06-07T17:12:46Z

cli/internal/daemon/daemon.go

+	base := repoRoot.Base()
+	logFilename := fmt.Sprintf("%v-%v.log", hexHash, base)
+
+	logsDirRaw, err := xdg.DataFile("logs")


This is ~/Library/Application Support/logs ... we should probably create our own directory to namespace things. Turborepo/logs, but lol who knows what path structure it needs.

Also, this API from XDG has side effects, so we should consider carefully when this gets called.

What do you think about a policy of only ever using the "constants" (not actually constant) from xdg? I put a wrapper in path.go to get the DataHome field as an AbsolutePath. We can use our existing code to do any work we need to do off of that. That gives us better control of the side effects.

I'm 100% on board with getting the base path out of xdg and then moving it into our path handling infrastructure as soon as possible. That will make our output far more predictable.

…ests

…sary .gitignore entry for fake Makefile target

nathanhammond

This is a full checkpoint; if it isn't referenced here it's done.

Old comments:

#1269 (comment) - I'm not intending to push too hard on this, just want to make sure you saw my follow-on comment.
#1269 (comment) - I want to change this but since it so easy to change later there's zero reason to block landing on it. I think the number one reason to maybe go ahead and change it is that tmpfs for tmp on Linux may not play nicely with it.

New comments:

pid extraction is the sole fixship.
questions on backoff at 2ms
The pid unlock + signal handling thing has a question about behavior difference, but not intending to block on it.

cli/Makefile

cli/internal/server/server.go

cli/internal/daemon/connector/connector.go

nathanhammond · 2022-06-24T13:32:50Z

cli/internal/daemon/connector/connector.go

+func backoffWithTimeout(timeout time.Duration) *backoff.ExponentialBackOff {
+	return &backoff.ExponentialBackOff{
+		Multiplier:      2,
+		InitialInterval: 2 * time.Millisecond,


Did you check to see if we could ever catch it in 1ms?

On my M1 macbook, it looks like either the initial attempt succeeds (in microseconds), or we wait ~15-25ms to start a daemon and connect.

cli/internal/daemon/daemon.go

nathanhammond · 2022-06-24T13:38:49Z

cli/internal/daemon/daemon.go

+	// If we catch a signal, unlock the pid file. This also wires up
+	// the pidfile for unlock on program exit.
+	signalWatcher.AddOnClose(func() {
+		p.Unlock()


This is the one that's weird to me; did you get a chance to check what the behavior difference is if we drop it?

In case I don't get to implementing this, leaving some notes here.

I did some experimentation with sending signals at different points in the execution of this function. It's also important to note that we are not listening to SIGKILL. Since registering a signal handler alters the default behavior of the runtime, we can block indefinitely on getting a Ctrl+C / SIGINT. This means that rather than needing to register watchers for signals from runTurboServer, we can instead add the signal watcher's Done() channel to the select at the bottom.

This definitely simplifies things, as our defer lock.Unlock() will always run. I also think it can be the only wait runs: no matter which method of triggering occurs (signal, request handler panic, server goroutine panic), the end result is breaking out the select at the bottom of runTurboServer. With that in mind, I think we don't noeed the pidLock abstraction. Future work that might apply would be wiring up a context.NotifyContext and dropping the signals.Watcher abstraction.

cli/internal/daemon/daemon.go

gsoltis · 2022-06-25T03:36:13Z

@nathanhammond I think I've addressed everything, but rather than extract pidLock, I nuked it and [hopefully] simplified the code there after reading a bit more about signaling, as well as running some experiments. I'll let you hit the button if you're still on board, otherwise we can go over it.

Quick summary is that the defer from runTurboServer should always run, as long as we don't get SIGKILL, which we don't catch anyways.

vercel bot deployed to Preview May 20, 2022 20:50 View deployment

gsoltis force-pushed the gsoltis/daemon branch from 1e6f90c to e9fc0b7 Compare May 23, 2022 21:01

gsoltis changed the base branch from gsoltis/cache_config to main May 23, 2022 21:01

vercel bot deployed to Preview May 23, 2022 21:02 View deployment

gsoltis force-pushed the gsoltis/daemon branch from e9fc0b7 to c2fdbd3 Compare May 23, 2022 21:12

vercel bot deployed to Preview May 23, 2022 21:13 View deployment

gsoltis force-pushed the gsoltis/daemon branch from c2fdbd3 to 27d2ade Compare May 23, 2022 21:22

vercel bot deployed to Preview May 23, 2022 21:23 View deployment

vercel bot deployed to Preview May 24, 2022 18:20 View deployment

gsoltis force-pushed the gsoltis/daemon branch from e754d6f to e0e4181 Compare May 25, 2022 20:38

gsoltis marked this pull request as ready for review May 25, 2022 22:21

gsoltis requested review from jaredpalmer, gaspar09 and nathanhammond as code owners May 25, 2022 22:21

gsoltis added the pr: work in progress label May 25, 2022

vercel bot deployed to Preview May 31, 2022 21:53 View deployment

gsoltis force-pushed the gsoltis/daemon branch from 76eb5e1 to e98f24f Compare May 31, 2022 22:26

vercel bot deployed to Preview June 1, 2022 16:59 View deployment

gsoltis force-pushed the gsoltis/daemon branch 2 times, most recently from beb6ecf to 640d037 Compare June 2, 2022 21:48

vercel bot deployed to Preview June 6, 2022 23:13 View deployment

nathanhammond reviewed Jun 7, 2022

View reviewed changes

gsoltis force-pushed the gsoltis/daemon branch from 80985ae to 1e18d42 Compare June 7, 2022 19:41

Greg Soltis added 3 commits June 7, 2022 13:38

First pass daemon implementation

496b1e3

Notes from review

4591f48

Move to pid file locking

c10685e

vercel bot had a problem deploying to Preview – turborepo June 22, 2022 18:15 Failure

Add exponential backoff to the client

939eabd

vercel bot had a problem deploying to Preview – turborepo June 22, 2022 18:59 Failure

Add comment re: node usage, remove unnecessary EnsureDir calls from t…

fe3f2d4

…ests

vercel bot had a problem deploying to Preview – turborepo June 22, 2022 19:15 Failure

vercel bot had a problem deploying to Preview – turborepo June 22, 2022 19:35 Failure

vercel bot had a problem deploying to Preview – turborepo June 22, 2022 20:22 Failure

gsoltis force-pushed the gsoltis/daemon branch from 6d44dd8 to 25b29a0 Compare June 22, 2022 23:54

vercel bot had a problem deploying to Preview – turborepo June 22, 2022 23:55 Failure

Greg Soltis added 3 commits June 22, 2022 17:08

Upgrade Makefile to properly generate Go code, remove no-longer-neces…

3c9b94d

…sary .gitignore entry for fake Makefile target

Comments and move to panic-on-error for lockfile.New in daemon.go

9b77cca

Extract pidfile management in daemon

1a5e13a

gsoltis force-pushed the gsoltis/daemon branch from 25b29a0 to 1a5e13a Compare June 23, 2022 00:08

vercel bot had a problem deploying to Preview – turborepo June 23, 2022 00:09 Failure

Greg Soltis added 2 commits June 23, 2022 10:25

Add comments to daemon_test

61b5a27

Merge branch 'main' into gsoltis/daemon

62d758a

vercel bot deployed to Preview June 23, 2022 17:27 View deployment

nathanhammond added the pr: fixship A PR which is approved with notes and does not require re-review. label Jun 24, 2022

nathanhammond approved these changes Jun 24, 2022

View reviewed changes

Greg Soltis added 2 commits June 24, 2022 16:10

Comments and removing pidLock abstraction

8d043de

Merge branch 'main' into gsoltis/daemon

aec9b1f

vercel bot deployed to Preview June 25, 2022 03:33 View deployment

Merge branch 'main' into gsoltis/daemon

fbeb201

nathanhammond added pr: automerge Kodiak will merge these automatically after checks pass and removed pr: fixship A PR which is approved with notes and does not require re-review. labels Jun 27, 2022

kodiakhq bot merged commit 5815666 into main Jun 27, 2022

kodiakhq bot deleted the gsoltis/daemon branch June 27, 2022 19:18

AuthorityFX mentioned this pull request Jun 22, 2023

[turborepo] Build step output files not restored with message "Skipping cache check for %v, outputs have not changed since previous run" #4137

Open

		@@ -0,0 +1,215 @@
		// Code generated by protoc-gen-go-grpc. DO NOT EDIT.

Watch task outputs to avoid cache restorations #1269

Watch task outputs to avoid cache restorations #1269

Uh oh!

Conversation

gsoltis commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsoltis commented May 27, 2022

Uh oh!

gsoltis commented May 27, 2022

Uh oh!

gsoltis commented May 31, 2022

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gsoltis commented Jun 25, 2022

Uh oh!

Uh oh!

gsoltis commented May 19, 2022 •

edited

Loading

vercel bot commented May 19, 2022 •

edited

Loading