Merge cockroachdb#25439

25439: backport-2.0: engine: actually unlock temp dirs on Windows r=bdarnell a=tschottdorf

Backport 1/1 commits from cockroachdb#25267.

/cc @cockroachdb/release

---

File locks are mandatory on Windows; our cleanup code was previously
assuming file system locks were advisory. Specifically, our cleanup code
was locking the temporary directory, then attempting to remove it before
releasing the lock. This worked on Unix platforms where locks are
advisory-only, but failed on Windows because the process's own lock
would prevent the cleanup! Presumably this ordering was meant to avoid a
race condition where the directory was unlocked before it was cleaned
up. It turns out this race condition doesn't matter (see the comment
within), so just unlock the directory before removing it, which works on
both Unix and Windows.

Release note (bug fix): Restarting a CockroachDB server on Windows no
longer fails due to file system locks in the store directory.

Fix cockroachdb#24144.
Fix cockroachdb#25272.


Co-authored-by: Nikhil Benesch <[email protected]>

pkg/storage/engine/temp_dir.go

@@ -116,11 +116,18 @@ func CleanupTempDirs(recordPath string) error {
 		if err != nil {
 			return errors.Wrapf(err, "could not lock temporary directory %s, may still be in use", path)
 		}
-		defer func() {
-			if err := unlockFile(flock); err != nil {
-				log.Errorf(context.TODO(), "could not unlock file lock when removing temporary directory: %s", err.Error())
-			}
-		}()
+		// On Windows, file locks are mandatory, so we must remove our lock on the
+		// lock file before we can remove the temporary directory. This yields a
+		// race condition: another process could start using the now-unlocked
+		// directory before we can remove it. Luckily, this doesn't matter, because
+		// these temporary directories are never reused. Any other process trying to
+		// lock this temporary directory is just trying to clean it up, too. Only
+		// the original process wants the data in this directory, and we know that
+		// process is dead because we were able to acquire the lock in the first
+		// place.
+		if err := unlockFile(flock); err != nil {
+			log.Errorf(context.TODO(), "could not unlock file lock when removing temporary directory: %s", err.Error())
+		}
 
 		// If path/directory does not exist, error is nil.
 		if err := os.RemoveAll(path); err != nil {