Fix Windows permission error with self remove

[vapopov]

After regression testing for v14 and additional cases recognized in issue with aliases, identified another issue related specifically for windows platform and double re-execution after update.

teleport/lib/utils/packaging/unarchive.go

Lines 132 to 134 in e3ff2c0

	if err := os.Remove(appPath); err != nil && !os.IsNotExist(err) {
	return trace.Wrap(err)
	}

Windows prevents the self-removal of an executed binary but allows it to be replaced, making this additional removal unnecessary, otherwise we get the access denied error.

teleport/lib/utils/fs.go

Lines 425 to 439 in e3ff2c0

	func CopyFile(src, dest string, perm os.FileMode) error {
	srcFile, err := os.Open(src)
	if err != nil {
	return trace.ConvertSystemError(err)
	}
	destFile, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_TRUNC, perm)
	if err != nil {
	return trace.ConvertSystemError(err)
	}
	_, err = destFile.ReadFrom(srcFile)
	if err != nil {
	return trace.ConvertSystemError(err)
	}
	return nil
	}

The helper function does not close file descriptors, which blocks execution on Windows. In integration tests, we launch the update from the test function body and then attempt to execute it by exec.CommandContext which leads to the error with execution.

The process cannot access the file because it is being used by another process.

Related: https://github.com/gravitational/cloud/issues/11856

[vapopov]

Would appreciate your review

[hugoShaka]

The deferred function will overwrite the error from return trace.ConvertSystemError(err)

You can use trace.NewAggregate() to keep the original error if it's already non-nil.

[hugoShaka]

Is this change required for the PR to work? Why?

[vapopov]

This one required for windows, to be able run tests.

In integration/autoupdate/tools/updater_tsh_test.go we use testserver.MakeTestServer which register test modules with BuildType = CLI

teleport/tool/teleport/testenv/test_server.go

Lines 79 to 88 in 217d014

	func init() {
	// If the test is re-executing itself, execute the command that comes over
	// the pipe. Used to test tsh ssh and tsh scp commands.
	if srv.IsReexec() {
	common.Run(common.Options{Args: os.Args[1:]})
	return
	}
	modules.SetModules(&cliModules{})
	}

Since it is disabled for windows (we don't support to build server). By default BuildType is oss which leads to failing test where updater was disabled for OSS versions.

[hugoShaka]

Thanks for the explanation :)

[hugoShaka]

If I understand correctly, utils.CopyFile already overwrote the destination file? This is why removing it before is not needed?

[vapopov]

yes, it was not necessary, and windows return error if we try to self-remove

[ravicious]

It might be good to leave a comment about it, someone touching this code in 6 months is likely not going to remember about this Windows-specific behavior.

[vapopov]

@ravicious @doggydogworld could you please also take a look, wanted to include this fix in next release

[ravicious]

Should the comment for this method be updated? Same could be probably said about IsEnterpriseBuild.

[vapopov]

Updated, I've restored previous check, reconsidered that it might be useful in further testing.

[ravicious]

It might be good to leave a comment about it, someone touching this code in 6 months is likely not going to remember about this Windows-specific behavior.

[ravicious]

Why do we only propagate the error from destFile.Close() and not srcFile.Close()? I don't know what's the correct behavior here, it's just something that caught my eye.

[vapopov]

destFile.Close() errors can happen if there’s a problem flushing the final buffered data to disk, like running out of space, this error is crucial because it directly impacts whether the data was successfully written and saved. srcFile is opened for reading and without error propagation because failure there doesn’t affect data integrity. Should I add the logging for srcFile what do you think?

[public-teleport-github-review-bot]

@vapopov See the table below for backport results.

Branch	Result
branch/v15	Failed
branch/v16	Failed
branch/v17	Create PR