Add root boundary to untarring #1409
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
cli/internal/cache/cache_http.go
Outdated
| @@ -188,13 +188,14 @@ func (cache *httpCache) retrieve(hash string) (bool, []string, int, error) { | |||
| duration = intVar | |||
| } | |||
| artifactReader := resp.Body | |||
| defer func() { _ = artifactReader.Close() }() | |||
There was a problem hiding this comment.
If I'm understanding correctly, this isn't closing this particular reader unless we return before line 214? I'd prefer to use separate names rather than reusing the name and having to explain what in the world is going on and capturing in the closure. We introduce spooky action at a distance with this one.
There was a problem hiding this comment.
I was trying not to mess with this code too much, but I can restructure it to be a bit clearer.
This calls Close() on whatever artifactReader points to, which is a thing we [maybe] update later on. But we can be more explicit.
| } | ||
| defer gzr.Close() | ||
| defer func() { _ = gzr.Close() }() |
There was a problem hiding this comment.
Check out the lint rule change in the git PR that makes this ceremony unnecessary.
| if isChild, err := root.ContainsPath(filename); err != nil { | ||
| return nil, err | ||
| } else if !isChild { | ||
| return nil, fmt.Errorf("cannot untar file to %v", filename) |
There was a problem hiding this comment.
If we bail here should we clean up? In fact, at any error return from inside of this loop we might want to consider that.
There was a problem hiding this comment.
(We could do that by returning files and then clean up at the restoreTar call site.)
There was a problem hiding this comment.
We can't revert. We can delete files we've written, but we can't restore to previous state because we have potentially truncated existing files. Either way, you're in an inconsistent state and need to start over.
It might be worth considering what to do in this scenario, your best bet is likely engineering a cache miss. If you've gotten into this scenario accidentally, you likely need to make a code change anyways, which will force a miss.
There was a problem hiding this comment.
Re-entrant to the task with no caching is probably the right answer, possibly after deleting any files created by the attempted restore. (Rechecking the original input hashes to make sure we're in an okay state as well?)
| @@ -316,5 +335,6 @@ func newHTTPCache(opts Opts, config *config.Config, recorder analytics.Recorder) | |||
| teamId: config.TeamId, | |||
| enabled: config.TurboJSON.RemoteCacheOptions.Signature, | |||
| }, | |||
| repoRoot: config.Cwd, | |||
There was a problem hiding this comment.
Renaming keeps moving up my list.
There was a problem hiding this comment.
Might be a good commit in the "move main to cobra" PR.
| } | ||
| files = append(files, hdr.Name) | ||
| filename := root.Join(hdr.Name) |
There was a problem hiding this comment.
is hdr.Name always a Unix path? The test has expectedFiles as / separated, if so, this line implicitly Cleans them to system separators.
There was a problem hiding this comment.
Yes, we convert all paths to posix-style on write into the tar file.
There was a problem hiding this comment.
Cool, can you add a comment here so that I remember to make it do the right thing inside of turbopath?
There was a problem hiding this comment.
Traced through this a little more, and I think it gets to our not-great cache abstraction. I don't think I've changed the behavior here, but I also think the return value, which will be posix-style repo-relative paths, is both wrong but also works due to Windows accepting /-delimited paths. This is used when a cache hit from remote cache is used to populate the filesystem cache. The return type here should match the argument type for files to cache.Put. That should be RepoRelativeSystemPath.
I'll add a comment, but I think this is best cleaned up when we have the types to clarify what we actually want.
Also start adding some tests for untarring.