http: Fix downstream permission bug

[V-FEXrt]

If a downstream user (directly or indirectly) removes the write permissions of a Path created by makeBinaryRequest then the request is triggered again it would fail as the call to curl wouldn't be able to overwrite the underlying file.

This changes the function to remove the file right before recreation to avoid that edge case.

[ag-eitilt]

Is there anything specific to curl or the local runner in that failure case, or is it pointing to a broader issue/feature of permissions across the board? Either way, this fixing the RSC failure is a good thing, but if this makes the HTTP library act differently than the rest of Wake, I do wonder if there's a more precision fix possible. (Mainly, is there a reason the hardlink has to be included in the repro instructions, or can the user change the permissions of the output in-place, so then the rm winds up potentially throwing out their work rather than safely aborting with "File has been edited since it was created" or whatever the message is.)

[V-FEXrt]

Is there anything specific to curl or the local runner in that failure case, or is it pointing to a broader issue/feature of permissions across the board?

Curl will refuse to overwrite the file if it has "weird" permissions but since the weird permission only happens if the user specifically changes them somewhere else its not safe for us to change them back as it would edit an unrelated Path owned by some other part of the system.

It's kind of hard to describe the problem in text but I'll give it a try

File on Disk <---- hard link --- http Path
   ^-------------- hard link --- User's Path

chmod applies to File on Disk so when the User's Path is chmod'd to match the users needs/expectations it is reflected as a breaking change to http Path. If we later reverse that on http Path we break the expected perms for User's Path which has downstream build consequences.

Using rm is a bit nicer because File on Disk only gets deleted when all the hardlinks are removed. So after rm http Path we have the following

File on Disk <---- hard link --- User's Path (unchanged and exactly as the expect it)

 <nothing> <------- http Path (No longer a path in the filesystem, free for curl to write to)

The weirdest part here is that a User could have a reference to http Path outside of the function call which means that file could temporarily disappear but then it would be quickly recreated by curl

aside: immediately after the call to this function it would be something like

File on Disk <---- hard link --- User's Path 
File on Disk 2 <------- http Path

The original File on Disk only ever gets changed/updated if the downstream user requests it

[ag-eitilt]

Yeah, makes sense. I think I was a bit unclear about my questions, though: 1, is this the resolution method we want to support if the user didn't make a link,¹ since it risks losing work if they also edited it in the intervening time?, and 2, what do we want to do if the file was created by a non-curl job instead before the same process of hardlink-and-chmod,² since it should presumably have the same behaviour?

I guess one answer for question 1 is that it's irrelevant because the check is only run when the job is exactly the same rather than every time the output is known,³ and if we assume that that's not a bug we can say curl is closer to virtual jobs like write which will never trigger the check, but I'd be happier making everything fail across the board -- though that admittedly wouldn't solve the problem we're having here.

After having gone through the cases explicitly and exhaustively, and now having a better idea of the rules around this, I think I would be fine with merging this as-is if we don't think we'll be tightening up that changed-output check in the future. The discrepancies/bugs example 2 reveals should be fixed either way, but that doesn't have to be part of this same PR.

Footnotes

1. $ wake -x 'downloadTo "test.txt"' ; echo edit >> test.txt ; chmod -w test.txt ; wake -x 'downloadTo "test.txt"'
   $ ls -l test.txt
   -rw-r--r-- 1 sammay sammay 4 Jul  8 12:48 test.txt
   $ grep edit test.txt
   $ echo $?
   1
   

   ↩

2. $ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob' ; ln test.txt test2.txt ; chmod -w test2.txt ; wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob'
   Job 9652
   Job 9652
   $ ls -l test.txt test2.txt
   -r--r--r-- 2 sammay sammay 4 Jul  8 12:48 test2.txt
   -r--r--r-- 2 sammay sammay 4 Jul  8 12:48 test.txt
   $ # Note that they're still both hardlinks to the same inode, and so have the same permissions
   $ rm -f test.txt test2.txt 
   $ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob' ; ln test.txt test2.txt ; chmod -w test2.txt ; wake -x 'makePlan "test" Nil "echo -n test with different contents > test.txt" | runJob'
   Job 9666
   Job 9679
   $ ls -l test.txt test2.txt
   -rwxrwxrwx 1 sammay sammay  4 Jul  8 12:48 test2.txt
   -rw-r--r-- 1 sammay sammay 28 Jul  8 12:48 test.txt
   $ # The permissions on test2.txt seem like a bug in their own right...
   

   ↩

3. $ wake -x 'write "test.txt" "test"' ; echo edit >> test.txt ; wake -x 'write "test.txt" "test"'
   Pass (Path "test.txt" "928b20366943e2afd11ebc0eae2e53a93bf177a4fcf35bcc64d503704e65e202")
   Pass (Path "test.txt" "928b20366943e2afd11ebc0eae2e53a93bf177a4fcf35bcc64d503704e65e202")
   $ grep edit test.txt
   $ echo $?
   1
   $ rm -f test.txt
   $ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob' ; echo edit >> test.txt ; wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob'
   Job 9428
   PANIC: The hashcode of output file 'test.txt' has changed from 928b20366943e2afd11ebc0eae2e53a93bf177a4fcf35bcc64d503704e65e202 (when wake last ran) to d319334a830d32f8ab3b4c9d641360c834f712a36bfa6909277217569ea4ca33 (when inspected this time). Presumably it was hand edited. Please move this file out of the way. Aborting the build to prevent loss of your data.
   $ grep edit test.txt
   testedit
   $ rm -f test.txt
   $ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob' ; echo edit >> test.txt ; wake -x 'makePlan "test" Nil "echo -n test with different contents > test.txt" | runJob'
   Job 9515
   Job 9528
   $ grep edit test.txt
   $ echo $?
   1
   

   ↩

[V-FEXrt]

1, is this the resolution method we want to support if the user didn't make a link,1 since it risks losing work if they also edited it in the intervening time?

Folks really shouldn't be editing anything inside of the private/internal .build directory. If they need to edit they should make a copy or a link. External users don't really get any guarentees about what goes on in .build (aside: I really wish we would have named it .wake for this reason)

and 2, what do we want to do if the file was created by a non-curl job instead before the same process of hardlink-and-chmod

Since the file path is a hash of the http request, the only way a user would ever reasonably create the file with a non-curl job would be to hand write out the exact path, hash included. If they do that then they are opting in to any internal implementation weirdness of the http library

$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob' ; ln test.txt test2.txt ; chmod -w test2.txt ; wake -x 'makePlan "test" Nil "echo -n test with different contents > test.txt" | runJob'
Job 9666
Job 9679
$ ls -l test.txt test2.txt
-rwxrwxrwx 1 sammay sammay  4 Jul  8 12:48 test2.txt
-rw-r--r-- 1 sammay sammay 28 Jul  8 12:48 test.txt
$ # The permissions on test2.txt seem like a bug in their own right...

I believe echo -n test with different contents > test.txt in there is creating a new file named test.txt not editing the old one which is why you see the permissions change. Try echo -n test with different contents >> test.txt to append to the existing file

[ag-eitilt]

1, is this the resolution method we want to support if the user didn't make a link, since it risks losing work if they also edited it in the intervening time?

Folks really shouldn't be editing anything inside of the private/internal .build directory. If they need to edit they should make a copy or a link. External users don't really get any guarentees about what goes on in .build (aside: I really wish we would have named it .wake for this reason)

Missed (or forgot) the fact that this was only ever going to be happening in .build, that eases several of my concerns.

and 2, what do we want to do if the file was created by a non-curl job instead before the same process of hardlink-and-chmod

Since the file path is a hash of the http request, the only way a user would ever reasonably create the file with a non-curl job would be to hand write out the exact path, hash included. If they do that then they are opting in to any internal implementation weirdness of the http library

This is specifically asking about things which don't have any connection to curl. If a user writes a Job which creates a file (echo ... > ... in my examples), then modifies the permissions of the output, Wake should handle it in a predictable manner. Since the curl Job happens in .build the non-curl Job doesn't have to match its behaviour, and I'll file a separate ticket for that discussion if it's not resolved in today's meeting, but we should still be able to describe what happens.

$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob' ; ln test.txt test2.txt ; chmod -w test2.txt ; wake -x 'makePlan "test" Nil "echo -n test with different contents > test.txt" | runJob'
Job 9666
Job 9679
$ ls -l test.txt test2.txt
-rwxrwxrwx 1 sammay sammay  4 Jul  8 12:48 test2.txt
-rw-r--r-- 1 sammay sammay 28 Jul  8 12:48 test.txt
$ # The permissions on test2.txt seem like a bug in their own right...

I believe echo -n test with different contents > test.txt in there is creating a new file named test.txt not editing the old one which is why you see the permissions change. Try echo -n test with different contents >> test.txt to append to the existing file

Good point, the only way to edit the file would be to use localRunner, which is it's own opting in to weirdness. We should still probably make the hash check apply more broadly.

However, the main issue I was trying to point out with that comment is that test2.txt has somehow magically gotten 777 permissions, when I would have expected 444.

[ag-eitilt]

Since this is happening in .build, I'm happy with any potential weirdness compared to other jobs. I still have a number of remaining questions, but they don't apply specifically to this PR.

[colinschmidt]

I agree with Sam. I think we should take a closer look at the "file changed underneath wake" behaviors but I think this specific fix for the http library is quite targeted, so should not be a problem to merge.