Caching, but to tar files. #1991

nathanhammond · 2022-09-17T04:45:58Z

So it's a bit more visible, here's the portion of the underlying cache work focused on tar file creation.

vercel · 2022-09-17T04:46:03Z

@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

cli/internal/cacheitem/cacheitem.go

gsoltis · 2022-09-20T21:03:55Z

cli/internal/cacheitem/restore_directory.go

+	calculatedAnchor := anchor
+	var checkPathErr error
+	for _, segment := range pathSegments {
+		calculatedAnchor, checkPathErr = checkPath(anchor, calculatedAnchor, turbopath.RelativeSystemPath(segment))


Is this going to lstat every path segment on every file we restore?

Not once I throw LstatCachedFile in here.

Hmm. Maybe, but it looks like you'd need to cache the lstat output for every intermediate path we're restoring.

What's the consequence of not doing this check?

The default version of "restore outside of tar root" is to change the header name field to ../../foo. That's easy to check for and prevent (checkName). Much harder is when you create one symlink and one file: external -> ../ and external/foo.

There is a fast path I can create here that would enable smarter behavior here (beyond just clever caching):

Confirm that the output directory is empty. (1 dirent)

Store all symlinks in their DAG and look through the resolved value there instead of hitting the OS. (rounds to zero)

The clever caching approach will take advantage of knowing that a (turbo-created) tar.gz is going to be breadth-first. I should only need to maintain a single cached resolution at any time. The number of lstat calls will be equivalent to the number of folders in the archive.

The other option is to let people arbitrarily restore outside of their root if they use symlinks to do it.

gsoltis · 2022-09-20T21:04:43Z

General comment, and not a blocker on this, but using AbsoluteSystemPath is causing us to lose a lot of the type safety we might otherwise get. Calls to things like os.Open are going to be hard to track down in the future. We can't just search for .ToString(), since that also covers places where we do in fact want the string value, rather just as a means to get to the filesystem. Maybe in the meantime duplicate some of the fs wrappers from AbsolutePath?

nathanhammond · 2022-09-21T08:49:06Z

General comment, and not a blocker on this, but using AbsoluteSystemPath is causing us to lose a lot of the type safety we might otherwise get. Calls to things like os.Open are going to be hard to track down in the future. We can't just search for .ToString(), since that also covers places where we do in fact want the string value, rather just as a means to get to the filesystem. Maybe in the meantime duplicate some of the fs wrappers from AbsolutePath?

#2026 sets me up to address this. I'll note that there is a large chunk of this that is intentionally working with strings because we don't actually know anything about the input.

Here are some examples of UnsafeJoins in the existing tar behavior:

turborepo/cli/internal/cache/cache_http.go

Line 254 in 8928b11

filename := root.Join(hdr.Name)
turborepo/cli/internal/cache/cache_http.go

Line 295 in 8928b11

linkFilename := root.Join(hdr.Name)
turborepo/cli/internal/cache/cache_http.go

Line 301 in 8928b11

linkTarget := linkFilename.Dir().Join(relativeLinkTarget)

vercel · 2022-09-28T17:26:15Z

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

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Oct 6, 2022 at 5:53PM (UTC)

cli/internal/cache/cache_fs.go

gsoltis · 2022-09-28T23:10:31Z

cli/internal/cacheitem/cacheitem.go

+
+	for _, closer := range closers {
+		// Skip the things which may not exist in this particular instance.
+		if reflect.ValueOf(closer).IsZero() {


In what scenarios would this happen? And is this different than a nil check?

The nil check panics, so I switched to reflect in order to make the check. Not sure if there is a better solution?

I did a little digging, and there are a couple things going on here.

As currently written, the reflect dance is required because these are typed nils, not untyped nils. Honestly, I think the way to handle this is to be less clever and just unroll the loop.

Our Close discipline is a little sloppy. The underlying writers error on double-close, whereas we have code here to no-op double close. I think we should error on double-close, and be certain about when we are closing things. To that end, I think we can replace ci.err with ci.isClosed. Either way, we ought to be consistent about checking whatever mechanism we use. Currently it appears that only AddFile checks. Alternatively, we could skip this entirely and defer to the underlying readers / writers to error-after-close.

Follow-on to the point above, some of the tests double-close, and make assertions in defer statements. For tests in particular, I think we ought to be explicit about testing the close behavior, and not do it in a deferred statement.

I added my suggestion for a Close implementation. After cleaning up the double-close from the create-test, the tests all pass.

Of the readers and writers, only the file implementation of io.Closer throws an error on multiple calls to Close(); I think it's reasonable to insulate ourselves from that error.

https://go.dev/play/p/e8n7IcLYHf6
https://github.com/golang/go/blob/master/src/archive/tar/writer.go#L463-L483
https://github.com/golang/go/blob/master/src/compress/flate/deflate.go#L630-L651

The error pattern I used is explicitly borrowed from the implementations of gzip/tar in stdlib, isClosed is not a state we can confirm, but having an error is a state we can confirm. AddFile is the only method which needs to interact with Close(), though Open and Restore are actually redundant (right now).

cli/internal/cacheitem/cacheitem.go

cli/internal/cacheitem/create.go

cli/internal/cacheitem/restore.go

gsoltis · 2022-09-28T23:32:39Z

cli/internal/cacheitem/restore_directory.go

+	calculatedAnchor := anchor
+	var checkPathErr error
+	for _, segment := range pathSegments {
+		calculatedAnchor, checkPathErr = checkPath(anchor, calculatedAnchor, turbopath.RelativeSystemPath(segment))


Hmm. Maybe, but it looks like you'd need to cache the lstat output for every intermediate path we're restoring.

What's the consequence of not doing this check?

cli/internal/cacheitem/restore_symlink.go

cli/internal/cacheitem/tar.go

cli/internal/cacheitem/create.go

nathanhammond · 2022-09-29T15:05:10Z

Checkpoint, the only open items here:

gsoltis · 2022-10-06T17:39:34Z

Lint error is a formatting issue with a comment. Going to force-merge if everything else passes and then submit a separate PR to fix it (or ignore it?)

This reverts commit 4ec2013.

Starting from where #1991 finished, this adopts Zstandard for compression, and supports restoration from both compressed and uncompressed caches. The adopted zstd library requires CGO. This also edits our Makefile to reflect that requirement.

nathanhammond requested review from gaspar09 and gsoltis September 20, 2022 16:39

nathanhammond force-pushed the cache-busters branch 2 times, most recently from fca2be8 to e25922c Compare September 20, 2022 17:15

gsoltis reviewed Sep 20, 2022

View reviewed changes

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

gsoltis reviewed Sep 20, 2022

View reviewed changes

nathanhammond force-pushed the cache-busters branch from e25922c to 9edd006 Compare September 21, 2022 07:47

nathanhammond force-pushed the cache-busters branch 10 times, most recently from 36a6aaf to 56838d0 Compare September 28, 2022 12:33

nathanhammond marked this pull request as ready for review September 28, 2022 17:18

nathanhammond requested a review from a team as a code owner September 28, 2022 17:18

nathanhammond force-pushed the cache-busters branch from 272ed53 to 4cb0b21 Compare September 28, 2022 17:24

vercel bot deployed to Preview September 28, 2022 17:27 View deployment

gsoltis reviewed Sep 28, 2022

View reviewed changes

nathanhammond force-pushed the cache-busters branch from 4cb0b21 to 920d949 Compare September 29, 2022 10:47

nathanhammond force-pushed the cache-busters branch from b9c84a4 to 0ddefae Compare October 3, 2022 13:13

vercel bot deployed to Preview October 3, 2022 13:16 View deployment

nathanhammond added 12 commits October 6, 2022 22:01

Consistent Close and return behavior.

aaec371

Handle close error for restoration.

4aada3b

Create GlobAll for restoration fast-path enablement.

b762aeb

Remove already-landed code.

b6b8807

Update to use dir cache.

8535893

Protect against traversal, any number of recursions.

f108245

Move sort location.

ca095db

Correct turbopath usage.

bdcfafd

Change close behavior.

48b45ec

Clean up symlink restoration.

be7a8ed

Windows. Always Windows.

f3f07a4

Lint.

4553549

nathanhammond force-pushed the cache-busters branch from e4e381b to 4553549 Compare October 6, 2022 14:02

Merge branch 'main' into cache-busters

08bf1a2

gsoltis approved these changes Oct 6, 2022

View reviewed changes

gsoltis added the pr: automerge Kodiak will merge these automatically after checks pass label Oct 6, 2022

Merge branch 'main' into cache-busters

5c00f72

vercel bot deployed to Preview October 6, 2022 17:53 View deployment

gsoltis merged commit 4ec2013 into vercel:main Oct 6, 2022

gsoltis added a commit that referenced this pull request Oct 7, 2022

Revert "Caching, but to tar files. (#1991)"

123f135

This reverts commit 4ec2013.

gsoltis mentioned this pull request Oct 7, 2022

Revert "Caching, but to tar files." #2178

Merged

nathanhammond mentioned this pull request Oct 11, 2022

cache-to-.tar.zst #2195

Merged

Caching, but to tar files. #1991

Caching, but to tar files. #1991

Uh oh!

Conversation

nathanhammond commented Sep 17, 2022

Uh oh!

vercel bot commented Sep 17, 2022

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 Sep 29, 2022 • 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!

gsoltis commented Sep 20, 2022

Uh oh!

nathanhammond commented Sep 21, 2022

Uh oh!

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

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!

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!

nathanhammond commented Sep 29, 2022

Uh oh!

gsoltis commented Oct 6, 2022

Uh oh!

Uh oh!

nathanhammond Sep 29, 2022 •

edited

Loading

vercel bot commented Sep 28, 2022 •

edited

Loading