mk-oracle, mk-sql: stop leaving temp directories behind on Windows

Three test helpers each left one directory in %TEMP% per run on the
Windows CI nodes.

permissions-check-run.ps1 restricts the ACL of the unpacked Oracle client
runtime to Administrators and SYSTEM, and never restored it. The cleanup
in the finally block runs unelevated and could therefore no longer delete
oci.dll. The elevated script now hands the rights back once its checks
are done. The finally block grants them back as well, for the case where
the elevated script ended early, and reports a directory that still
cannot be removed instead of discarding the error.

make_endpoint_tns_admin_dir created its TNS_ADMIN directory with
create_dir_all and returned a plain path, so nothing ever deleted it. It
now returns a TempDir, which the calling test holds until it ends.

LogMe wrote its log file into a TempDir. The process-global logger keeps
that file open until the test process exits, Windows refuses to delete an
open file, and TempDir discards the resulting error. The log now goes to
one fixed directory that each run clears when it starts.

Change-Id: Ic985f0dd0457f6dc0eb1c6afa498899ccdd1decc

Author: andiumbreit

packages/mk-oracle/permissions-check-run.ps1

@@ -44,6 +44,8 @@ while (!(Test-Path "$root_dir/.werks" -ErrorAction SilentlyContinue)) {
 Write-Host "Building $package_name..." -ForegroundColor White
 Push-Location $PSScriptRoot
 $temp_dir = Join-Path $env:TEMP "mk-oracle-perms-check-$([System.IO.Path]::GetRandomFileName())"
+$runtime_path = "$temp_dir/runtimes/plugins/packages/mk-oracle/runtime"
+$current_user_sid = [System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value
 New-Item -ItemType Directory -Path $temp_dir -Force | Out-Null
 try {
     & cargo build --release --package $package_name --target $cargo_target
@@ -61,7 +63,6 @@ try {
     if ($LASTEXITCODE -ne 0) { Write-Error "OCI download failed" }
 
     $oci_zip = & bazel cquery $target --output=starlark --starlark:expr='target.files.to_list()[0].path'
-    $runtime_path = "$temp_dir/runtimes/plugins/packages/mk-oracle/runtime"
     New-Item -ItemType Directory -Path $runtime_path -Force | Out-Null
     Expand-Archive -Path "$root_dir/$oci_zip" -DestinationPath $runtime_path -Force
 
@@ -111,9 +112,9 @@ try {
     # restrict it to Administrators-only (elevated, so the ACL reset itself
     # isn't fighting a UAC-filtered token), then verify it's trusted again.
     Write-Host "Step 4: running elevated checks..." -ForegroundColor White
-    $admin_out_before = "$env:TEMP\perms-check-stdout-before.txt"
-    $admin_out_after = "$env:TEMP\perms-check-stdout-after.txt"
-    $admin_sql_out_before = "$env:TEMP\perms-check-sql-stdout-before.txt"
+    $admin_out_before = Join-Path $temp_dir "perms-check-stdout-before.txt"
+    $admin_out_after = Join-Path $temp_dir "perms-check-stdout-after.txt"
+    $admin_sql_out_before = Join-Path $temp_dir "perms-check-sql-stdout-before.txt"
     $admin_script = Join-Path $temp_dir "admin-check.ps1"
     @"
 `$ErrorActionPreference = 'Stop'
@@ -124,6 +125,8 @@ Set-Location '$PSScriptRoot'
 icacls '$runtime_path' /inheritance:r /remove:g '*S-1-5-32-545' /grant:r '*S-1-5-32-544:(OI)(CI)F' '*S-1-5-18:(OI)(CI)F' /T /C | Out-Null
 if (`$LASTEXITCODE -ne 0) { exit 2 }
 & '$binary' -c tests/files/test-mini-one-section.yml *> '$admin_out_after'
+# Hand the directory back to the account that created it
+icacls '$runtime_path' /grant '*${current_user_sid}:(OI)(CI)F' /T /C | Out-Null
 "@ | Set-Content -Path $admin_script -Encoding utf8
     $proc = Start-Process pwsh -ArgumentList "-NoProfile -File `"$admin_script`"" -Verb RunAs -Wait -PassThru -WindowStyle Hidden
     if ($proc.ExitCode -ne 0) {
@@ -143,23 +146,20 @@ if (`$LASTEXITCODE -ne 0) { exit 2 }
     $refusal = "No Oracle client runtime found"
 
     $output_before = Get-Content $admin_out_before -Raw
-    Remove-Item $admin_out_before -ErrorAction SilentlyContinue
     if ($output_before -match '<<<' -or $output_before -notmatch $refusal) {
         Write-Host $output_before -ForegroundColor Red
         Write-Error "FAIL: expected '$refusal' and no section output from admin run before restricting permissions"
     }
     Write-Host "OK: root can't exec non-root code" -ForegroundColor Green
 
     $sql_output_before = Get-Content $admin_sql_out_before -Raw
-    Remove-Item $admin_sql_out_before -ErrorAction SilentlyContinue
     if ($sql_output_before -match '<<<' -or $sql_output_before -notmatch $refusal) {
         Write-Host $sql_output_before -ForegroundColor Red
         Write-Error "FAIL: expected '$refusal' and no section output from admin run with custom SQL file"
     }
     Write-Host "OK: root can't read non-root custom SQL file" -ForegroundColor Green
 
     $output_after = Get-Content $admin_out_after -Raw
-    Remove-Item $admin_out_after -ErrorAction SilentlyContinue
     # Same shape as the non-elevated run (step 3): real section output, not
     # just any bytes — error text on stderr must not count as success.
     $after_lines = ($output_after -split "`n" | Where-Object { $_.Trim() -ne "" })
@@ -176,6 +176,14 @@ if (`$LASTEXITCODE -ne 0) { exit 2 }
 
 }
 finally {
-    Remove-Item $temp_dir -Recurse -Force -ErrorAction SilentlyContinue
+    if (Test-Path $runtime_path) {
+        & icacls $runtime_path /grant "*${current_user_sid}:(OI)(CI)F" /T /C 2>&1 | Out-Null
+    }
+    try {
+        Remove-Item $temp_dir -Recurse -Force
+    }
+    catch {
+        Write-Warning "Leftover directory $temp_dir could not be removed: $_"
+    }
     Pop-Location
 }

packages/mk-oracle/tests/common/tools.rs

@@ -20,6 +20,7 @@
 use mk_oracle::config::authentication::{AuthType, Role, SqlDbEndpoint};
 use mk_oracle::config::ora_sql::Config;
 use mk_oracle::types::{Credentials, InstanceAlias};
+use tempfile::TempDir;
 
 /// Mandatory reference endpoint for all DB-dependent tests.
 pub const ORA_ENDPOINT_ENV_VAR: &str = "CI_ORA2_DB_TEST";
@@ -221,14 +222,15 @@ pub fn make_mini_config_custom_instance(
     )
 }
 
-/// Writes a tnsnames.ora resolving `alias` to `endpoint` into a
-/// process-private directory and returns that directory, suitable as
-/// tns_admin. Keeps alias-based tests independent of which reference DB
-/// the endpoint env vars point at.
-pub fn make_endpoint_tns_admin_dir(endpoint: &SqlDbEndpoint, alias: &str) -> std::path::PathBuf {
-    let dir =
-        std::env::temp_dir().join(format!("mk-oracle-test-tns-{}-{alias}", std::process::id()));
-    std::fs::create_dir_all(&dir).expect("failed to create TNS_ADMIN dir");
+/// Writes a tnsnames.ora resolving `alias` to `endpoint` into a temporary
+/// directory and returns that directory, suitable as tns_admin. Keeps
+/// alias-based tests independent of which reference DB the endpoint env vars
+/// point at.
+pub fn make_endpoint_tns_admin_dir(endpoint: &SqlDbEndpoint, alias: &str) -> TempDir {
+    let dir = tempfile::Builder::new()
+        .prefix(&format!("mk-oracle-test-tns-{alias}-"))
+        .tempdir()
+        .expect("failed to create TNS_ADMIN dir");
     let content = format!(
         r"{alias} =
   (DESCRIPTION =
@@ -248,7 +250,7 @@ pub fn make_endpoint_tns_admin_dir(endpoint: &SqlDbEndpoint, alias: &str) -> std
             .as_deref()
             .expect("endpoint must provide a SID"),
     );
-    std::fs::write(dir.join("tnsnames.ora"), content).expect("failed to write tnsnames.ora");
+    std::fs::write(dir.path().join("tnsnames.ora"), content).expect("failed to write tnsnames.ora");
     dir
 }
 

packages/mk-oracle/tests/test_ora_with_db.rs

@@ -466,7 +466,7 @@ oracle:
             &endpoint,
             "FREE",
             Some(InstanceAlias::from("ora_remote".to_string())),
-            &tns_admin,
+            tns_admin.path(),
         );
         let env = Env::default();
         let r = generate_data(&config, &env).await;

packages/mk-sql/tests/common/tools.rs

@@ -236,24 +236,37 @@ pub async fn run_get_version(client: &mut UniClient) -> Option<String> {
     }
 }
 
+/// Directory holding the log file that [`LogMe`] writes.
+///
+/// Reused by every run.
+/// `LogMe` hands the log file to the process-global logger,
+/// which keeps that file open until the test process exits. Windows refuses to
+/// delete an open file, so a temporary directory created for one run cannot be removed by
+/// that same run.
+fn log_dir() -> PathBuf {
+    std::env::temp_dir().join("mk-sql-test-logs")
+}
+
 #[allow(dead_code)]
 pub struct LogMe {
-    temp_dir: TempDir,
+    dir: PathBuf,
     name: String,
 }
 
 #[allow(dead_code)]
 impl LogMe {
     pub fn new(name: &str) -> Self {
-        let dir = create_temp_process_dir();
+        let dir = log_dir();
+        let _ = std::fs::remove_dir_all(&dir);
+        std::fs::create_dir_all(&dir).expect("failed to create log directory");
         Self {
-            temp_dir: dir,
+            dir,
             name: name.to_string(),
         }
     }
 
     pub fn dir(&self) -> &Path {
-        self.temp_dir.path()
+        &self.dir
     }
 
     pub fn start(self, level: log::Level) -> Self {