fix(cli): cache with correct file perms #1429

tknickman · 2022-06-22T16:51:42Z

Fixes #156

Issue Summary

There were two main issues causing this:

We're using generic filesystem function for the cache operations. These functions require a file mode for the source file, and a file mode for the destination file. We were setting the incorrect bit here causing all files to be restored from cache with one file mode.
When recursively walking files, the walk function we use seems to be calling the callback with the incorrect file mode (it's always 0)

The Fix

To fix this, I've made the file copying functions a bit less generalized (they no longer require a source mode, or a destination mode). Instead, they always use the mode of the source file for the destination file. I also pushed the logic of sourcing the source files mode into the Copy function, below where the incorrect mode was being returned from godirwalk.

This makes these functions slightly less reusable, but for our purposes, this is what we need. We can always add back generalized versions in the future.

vercel · 2022-06-22T16:51:45Z

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

Name	Status	Preview	Updated
turborepo	❌ Failed (Inspect)		Jun 27, 2022 at 2:21PM (UTC)

1 Ignored Deployment

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

tknickman · 2022-06-22T16:57:07Z

cli/internal/fs/copy_file.go

@@ -61,10 +65,10 @@ func RecursiveCopyOrLinkFile(from string, to string, mode os.FileMode, link, fal
 			} else if isSame {
 				return nil
 			}
-			return CopyOrLinkFile(name, dest, fileMode, mode, link, fallback)


This fileMode, passed to the WalkMode callback is what I found to be incorrect (always 0)

So, this is "correct", but it's not what anybody might expect:

https://github.com/karrick/godirwalk/blob/9a7752c108e7ea76255201b9f47bd4d4d2df868e/modeType.go#L19

It's explicitly not a complete FileMode, it's masked to only include only os.ModeType.

Anyway, I didn't go looking for why that is until you made this note, this is definitely an API footgun and I believe it's likely why we had this bug to begin with.

Ah interesting, yea this wasn't what I was expecting at all. Thanks for digging into this.

nathanhammond

The tests need a change, and I think it's worth spending a bit more time on trying to reduce the number of times we stat.

cli/internal/cache/cache_fs.go

cli/internal/fs/copy_file.go

cli/internal/fs/copy_file_test.go

jaredpalmer

Lots of stat calls. Is this something we can avoid or cache?

tknickman · 2022-06-22T22:34:26Z

cli/internal/fs/fs.go

@@ -60,12 +68,33 @@ func FileExists(filename string) bool {
 	return err == nil && !info.IsDir()
 }

+// StatFile adds FileInfo to the given StatedFile if it doesn't already exist
+func StatFile(file StatedFile) (StatedFile, error) {


This ensures we only call os.Lstat one time if the fs.FileInfo doesn't exist.

I verified that we only stat once per file during a cache write / restore cycle.

nathanhammond

Blocking:
Path string => Path AbsolutePath
Stat and Lstat method name matching.

cli/internal/fs/copy_file.go

nathanhammond · 2022-06-23T10:06:38Z

cli/internal/fs/copy_file.go

@@ -61,10 +65,10 @@ func RecursiveCopyOrLinkFile(from string, to string, mode os.FileMode, link, fal
 			} else if isSame {
 				return nil
 			}
-			return CopyOrLinkFile(name, dest, fileMode, mode, link, fallback)


So, this is "correct", but it's not what anybody might expect:

https://github.com/karrick/godirwalk/blob/9a7752c108e7ea76255201b9f47bd4d4d2df868e/modeType.go#L19

It's explicitly not a complete FileMode, it's masked to only include only os.ModeType.

Anyway, I didn't go looking for why that is until you made this note, this is definitely an API footgun and I believe it's likely why we had this bug to begin with.

cli/internal/fs/fs.go

cli/internal/fs/copy_file.go

cli/internal/fs/fs.go

cli/internal/fs/lstat.go

cli/internal/run/run_state.go

cli/internal/fs/copy_file.go

gsoltis · 2022-06-23T17:50:54Z

cli/internal/cache/cache_fs.go

@@ -89,7 +89,7 @@ func (f *fsCache) Put(target, hash string, duration int, files []string) error {
 						return fmt.Errorf("error ensuring directory file from cache: %w", err)
 					}

-					if err := fs.CopyOrLinkFile(file, filepath.Join(f.cacheDirectory, hash, file), fromInfo.Mode(), fs.DirPermissions, false, false); err != nil {
+					if err := fs.CopyOrLinkFile(&fs.LstatCachedFile{Path: fs.AbsolutePath(file), Info: fromInfo}, filepath.Join(f.cacheDirectory, hash, file), false, false); err != nil {


Re: fs.AbsolutePath, please don't cast to it. Use one of CheckedToAbsolutePath if you don't know if it's an absolute path, UnsafeToAbsolutePath if you are certain that it's an absolute path but we haven't migrated the calling code to use AbsolutePath yet, and AbsolutePathFromUpstream if the value is coming from some library function that you are certain is providing absolute paths.

This helps us find all the places that need to be migrated, as well as communicates what APIs have been investigated already. I expect this applies to anything from turbopath as well.

Indeed, the turbopath stuff is following the same rules. (Good catch on this cast, I noticed a different one.)

Some notes on turbopath:

AbsoluteSystemPath models the same thing as turbofs.AbsolutePath. There will be a migration PR which should hopefully "just work." I just tried to do it in an automated fashion and everything broke terribly so I need to figure out what I did wrong.

At some point there may be a bonus migration where we make a AnchoredXxxxPath type that is io/fs-friendly, but that's not today.

Ordering for things:

Land the daemon code which extensively uses AbsolutePath so that we don't have to do it twice. (Friday US?)

Migrate turbofs.AbsolutePath to turbopath.AbsoluteSystemPath. (Monday HK/US?)

Delete anything unused that remains. (Tuesday HK/US?)

@gsoltis I want to make sure we're on the same page with terminology.

I interpret absolute path as a path from system root. For our purposes, is that valid here? Or is our notion of an absolute path (fs.AbsolutePath) a path from the monorepo root.

I'm asking because If the former, most (if not all) of the paths in this PR are relative and will need to be converted before using one of CheckedToAbsolutePath, UnsafeToAbsolutePath, or AbsolutePathFromUpstream

fs.AbsolutePath is semantically equivalent to turbopath.AbsoluteSystemPath and represents an absolute path in the native format of the particular OS. So <Volume>:\some\path or /some/path as appropriate.

The motivation for all this work separating out different kinds of paths is that they show up in our user input as strings, and all of the APIs for interacting with the filesystem and for interacting with potentially-shared cache artifacts also operate on strings, and it is tempting to naively assume one can be passed to the other. This has been a source of bugs, as well as my suspicion that the --cwd flag doesn't really work as intended.

The solution we've landed on is attempting to identify the specific kinds of paths we care about internally and being strict about what we're working with at any given point. The two important ones are fs.AbsolutePath/turbopath.AbsoluteSystemPath for anything that touches the filesystem, and turbopath.AnchoredUnixPath for anything that is potentially shared.

We've been migrating gradually, so some parts of the code are already using fs.AbsolutePath for filesystem access. The turbopath stuff is new, and should help us further clarify paths especially around the cache.

There's a judgement call in how much to migrate at any one point. You will likely at some point end up doing UnsafeTo<SomePathType> with a comment explaining that you've verified that the string is of the correct type. Likewise, if you need to interact with some part of the system that hasn't migrated yet, you can do ToStringDuringMigration (you may need to add this, depending what kind of type you already have). This lets us easily find the boundaries of what has and hasn't been migrated.

If it's not there already, you should feel free to thread Cofing.Cwd into whatever code you're working with. It is the fs.AbsolutePath to the root of the monorepo. You can then use your relative paths to get absolute paths for filesystem interactions.

Since it sounds like the transition from fs.AbsolutePath to turbopath.AbsoluteSystemPath is going to be an automated one, I would go with the fs.AbsolutePath for now, since it has the filesystem operations mostly implemented already. But I would except the migration to happen in short order.

windows file system lacks executable bit

comments

nathanhammond

I'm moving out of the way on this; but still a few more idiosyncrasies left. I suspect @gsoltis can get you another review on this today.

cli/internal/fs/copy_file.go

cli/internal/fs/fs.go

cli/internal/cache/cache_fs.go

gsoltis

Just a heads up, I referenced fs.AbsolutePath, but I mean whichever of fs.AbsolutePath and turbopath.AbsoluteSystemPath has the filesystem integrations implemented in main at the time of merge.

cli/internal/fs/copy_file.go

gsoltis

Just a couple minor things I noticed on another readthrough, then ship it!

Thanks for tackling this!

cli/internal/fs/fs.go

cli/internal/fs/path.go

Changes addressed, feel free to review if you have time!

gsoltis · 2022-06-27T16:32:43Z

cli/internal/fs/path.go

@@ -145,7 +145,7 @@ func (ap AbsolutePath) RelativePathString(path string) (string, error) {
 	return filepath.Rel(ap.asString(), path)
 }

-// Symlink implements os.Symlink(target, ap) for absolute path
+// Symlink implements os.Symlink(target) for absolute path


I think this was correct before. The underlying call is in fact os.Symlink(target, ap.asString()), so os.Symlink(target, ap) where ap is the AbsolutePath matches.

nathanhammond

Two things:

There is definitely one bug that landed with this which we need to sort out.
We apparently don't have any tests that would catch it. Can you get us a red/green pair of commits for this?

nathanhammond · 2022-06-27T17:58:54Z

cli/internal/cache/cache_fs.go

 				if err != nil {
 					return fmt.Errorf("error stat'ing cache source %v: %v", file, err)
 				}
-				if !fromInfo.IsDir() {
+				if fromType != os.ModeDir {


This is an extremely subtle bug. os.ModeDir is part of a bitmask. To test if fromType is not a directory you need to do something like:

fromType & os.ModeDir != os.ModeDir

https://go.dev/play/p/2ZNJ_-cvDx-

nathanhammond · 2022-06-27T18:00:34Z

cli/internal/fs/copy_file.go

 	if err != nil {
 		return err
 	}

-	if fromInfo.IsDir() {
+	if fromType == os.ModeDir {


fromType & os.ModeDir == os.ModeDir

nathanhammond · 2022-06-27T18:02:13Z

cli/internal/fs/fs.go

-		fromMode, err := from.GetMode()
-		isSymlink := err == nil && fromMode&os.ModeSymlink == os.ModeSymlink
+		fromType, err := from.GetType()
+		isSymlink := err == nil && fromType == os.ModeSymlink


This change was the thing that tipped me off. Thank you for being so consistent! If you weren't I wouldn't have noted this.

fromType & os.ModeSymlink == os.ModeSymlink

nathanhammond · 2022-06-27T18:08:01Z

cli/internal/run/run_state.go

+	if err != nil {
+		return err
+	}
+	// chrometracing.Path() is absolute by default, but can still be relative if overriden via $CHROMETRACING_DIR


The change seems fine. That's some wild spooky action at a distance from pushing types through the system.

Follow up to #1429

tknickman requested review from jaredpalmer, gsoltis, gaspar09 and nathanhammond as code owners June 22, 2022 16:51

tknickman commented Jun 22, 2022

View reviewed changes

nathanhammond suggested changes Jun 22, 2022

View reviewed changes

jaredpalmer reviewed Jun 22, 2022

View reviewed changes

tknickman force-pushed the file_perms branch from 8bd439d to 5d0138a Compare June 22, 2022 17:59

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

tknickman force-pushed the file_perms branch from 5d0138a to 8025384 Compare June 22, 2022 22:27

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

tknickman force-pushed the file_perms branch from 8025384 to 8639735 Compare June 22, 2022 22:28

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

tknickman commented Jun 22, 2022

View reviewed changes

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

nathanhammond suggested changes Jun 23, 2022

View reviewed changes

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

nathanhammond previously requested changes Jun 23, 2022

View reviewed changes

tknickman force-pushed the file_perms branch from 5e28f76 to ad83385 Compare June 23, 2022 15:46

gsoltis suggested changes Jun 23, 2022

View reviewed changes

tknickman added 4 commits June 24, 2022 11:09

fix(cli): cache with correct file perms

8453611

fix(test): use readonly perm

66b04d7

windows file system lacks executable bit

Move to LstatCachedFile & AbsolutePath

66438e1

comments

Updates from review comments

06fce8b

tknickman force-pushed the file_perms branch from ad83385 to 06fce8b Compare June 24, 2022 15:09

nathanhammond reviewed Jun 24, 2022

View reviewed changes

cli/internal/fs/copy_file.go Outdated Show resolved Hide resolved

cli/internal/fs/copy_file.go Outdated Show resolved Hide resolved

cli/internal/fs/fs.go Outdated Show resolved Hide resolved

cli/internal/cache/cache_fs.go Outdated Show resolved Hide resolved

tknickman added 2 commits June 24, 2022 14:26

Use LstatCachedFile wherever possible

244ef21

Update LstatCachedFile interface

fd7e0b2

gsoltis suggested changes Jun 24, 2022

View reviewed changes

tknickman added 2 commits June 24, 2022 16:22

Remote direct cast to AbsolutePath

0bc68ba

Add Link / ReadLink path methods

f9e4db1

tknickman requested review from gsoltis and nathanhammond June 24, 2022 20:46

Move from UnsafeToAbsolutePath to Join

33e3c10

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

gsoltis approved these changes Jun 24, 2022

View reviewed changes

cli/internal/fs/fs.go Outdated Show resolved Hide resolved

cli/internal/fs/path.go Show resolved Hide resolved

review updates

9aeac3c

tknickman 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 e655e44 into main Jun 27, 2022

kodiakhq bot deleted the file_perms branch June 27, 2022 15:38

gsoltis reviewed Jun 27, 2022

View reviewed changes

nathanhammond reviewed Jun 27, 2022

View reviewed changes

tknickman mentioned this pull request Jun 28, 2022

fix(cli): prefer IsDir() over explicit type check #1445

Merged

kodiakhq bot pushed a commit that referenced this pull request Jun 28, 2022

fix(cli): prefer IsDir() over explicit type check (#1445)

7edc56a

Follow up to #1429

tknickman mentioned this pull request Jun 30, 2022

Move some fields out of Config #1460

Merged

natew mentioned this pull request Aug 13, 2022

Permissions change when using turbo vs direct build #1691

Closed

fix(cli): cache with correct file perms #1429

fix(cli): cache with correct file perms #1429

Uh oh!

Conversation

tknickman commented Jun 22, 2022

Issue Summary

The Fix

Uh oh!

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

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!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaredpalmer left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

gsoltis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsoltis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

vercel bot commented Jun 22, 2022 •

edited

Loading

gsoltis left a comment •

edited

Loading

nathanhammond Jun 27, 2022 •

edited

Loading