Fix OAuth credential lock recovery and deletion errors

Author: rustyconover

CMakeLists.txt

@@ -425,6 +425,25 @@ install(
   LIBRARY DESTINATION "${INSTALL_LIB_DIR}"
   ARCHIVE DESTINATION "${INSTALL_LIB_DIR}")
 
+# Native Windows credential tests use real DPAPI files and separate processes.
+# Keep this independent of the POSIX-only launcher/unit-test target below.
+option(BUILD_VGI_OAUTH_STORE_TESTS "Build native Windows OAuth credential tests" OFF)
+if(WIN32 AND BUILD_VGI_OAUTH_STORE_TESTS)
+  find_package(Python3 REQUIRED COMPONENTS Interpreter)
+  add_executable(vgi_oauth_store_probe test/support/oauth_store_probe.cpp)
+  target_include_directories(vgi_oauth_store_probe PRIVATE src/include duckdb/src/include)
+  target_link_libraries(vgi_oauth_store_probe
+    ${EXTENSION_NAME}
+    duckdb_static
+    dummy_static_extension_loader)
+  add_custom_target(test_vgi_oauth_store
+    COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/test/test_oauth_store_windows.py
+      --probe $<TARGET_FILE:vgi_oauth_store_probe>
+    DEPENDS vgi_oauth_store_probe
+    USES_TERMINAL
+    VERBATIM)
+endif()
+
 # Optional C++ unit-test binary for the launcher's pure helpers.  Built when
 # ``BUILD_VGI_UNIT_TESTS=ON`` is passed to CMake (or as an env var).  Pure
 # helpers are intentionally DuckDB-free (they throw std::invalid_argument and

src/include/vgi_oauth_store.hpp

@@ -26,6 +26,8 @@ bool LoadOAuthRefreshToken(const std::string &canonical_key, const OAuthRefreshC
 void StoreOAuthRefreshToken(const std::string &canonical_key, const OAuthRefreshContext &binding,
                             const std::string &cache_mode, const std::string &refresh_token);
 
+// Missing credentials are already deleted. Backend errors propagate in both
+// auto and persistent modes so logout cannot report success with a saved token.
 void DeleteOAuthRefreshToken(const std::string &canonical_key, const std::string &cache_mode);
 
 } // namespace vgi

src/vgi_oauth.cpp

@@ -2022,19 +2022,20 @@ std::string OAuthCatalogAuth::HandleUnauthorized(const OAuthChallenge &challenge
 					// registered OAuth client — falling through would mask the
 					// real error behind a misleading "token exchange failed" message.
 					std::string refresh_err = e.what();
-					if (refresh_err.find("invalid_grant") != std::string::npos) {
-						VGI_STDERR_DEBUG("[VGI] oauth.clearing_stale_refresh_token\n");
-						if (!canonical_key.empty()) {
-							DeleteOAuthRefreshToken(canonical_key, cache_mode_);
-						}
-					}
+					const bool invalid_grant = refresh_err.find("invalid_grant") != std::string::npos;
 					if (!lock.owns_lock()) lock.lock();
 					if (WasCleared()) throw;
 					// Clear stale refresh token on invalid_grant
-					if (refresh_err.find("invalid_grant") != std::string::npos) {
+					if (invalid_grant) {
 						state->token.refresh_token.clear();
 					}
+					// Finish the state transition before deletion, which can itself
+					// fail. Otherwise other connections remain stuck IN_PROGRESS.
 					StoreFailed(refresh_err);
+					if (invalid_grant && !canonical_key.empty()) {
+						VGI_STDERR_DEBUG("[VGI] oauth.clearing_stale_refresh_token\n");
+						DeleteOAuthRefreshToken(canonical_key, cache_mode_);
+					}
 					throw;
 				} catch (...) {
 					// Non-std::exception throw (rare, but possible from
@@ -2148,14 +2149,16 @@ std::string OAuthCatalogAuth::HandleUnauthorized(const OAuthChallenge &challenge
 void OAuthCatalogAuth::ClearTokens() {
 	std::lock_guard<std::mutex> binding_lock(binding_mutex_);
 	std::string key;
+	uint64_t logout_epoch;
 	{
 		std::lock_guard<std::mutex> lock(state_->mutex);
-		state_->epoch++;
+		logout_epoch = ++state_->epoch;
 		if (!state_->refresh_ctx.issuer.empty()) {
 			key = OAuthSessionKey(profile_, state_->refresh_ctx);
 		}
 		state_->token = OAuthTokenSet();
-		state_->refresh_ctx = OAuthRefreshContext();
+		// Retain the binding until deletion succeeds so a failed logout can be
+		// retried against the same stored credential.
 		state_->status = AuthState::Status::IDLE;
 		state_->owner = std::thread::id();
 		state_->error_message.clear();
@@ -2165,6 +2168,12 @@ void OAuthCatalogAuth::ClearTokens() {
 		auto lease = AcquireOAuthCredentialLease(key, cache_mode_);
 		DeleteOAuthRefreshToken(key, cache_mode_);
 	}
+	{
+		std::lock_guard<std::mutex> lock(state_->mutex);
+		if (state_->epoch == logout_epoch && state_->status == AuthState::Status::IDLE) {
+			state_->refresh_ctx = OAuthRefreshContext();
+		}
+	}
 }
 
 bool OAuthCatalogAuth::GetTokenInfo(TokenInfo &info) {

src/vgi_oauth_store.cpp

@@ -231,7 +231,11 @@ class WindowsLease final : public OAuthCredentialLease {
 	explicit WindowsLease(const std::string &key) {
 		std::string name = "Local\\QueryFarm.VGI.OAuth." + VgiSha256Hex(key);
 		handle_ = CreateMutexA(nullptr, FALSE, name.c_str());
-		if (!handle_ || WaitForSingleObject(handle_, INFINITE) != WAIT_OBJECT_0) {
+		const auto result = handle_ ? WaitForSingleObject(handle_, INFINITE) : WAIT_FAILED;
+		// WAIT_ABANDONED grants ownership after the previous owner exits. Keep
+		// the lease; the caller reloads and validates the protected record before
+		// using it, and the destructor must release this ownership normally.
+		if (result != WAIT_OBJECT_0 && result != WAIT_ABANDONED) {
 			if (handle_)
 				CloseHandle(handle_);
 			throw IOException("VGI OAuth: could not acquire credential cache lease");
@@ -307,6 +311,9 @@ void PlatformStore(const std::string &key, const std::string &data) {
 void PlatformDelete(const std::string &key) {
 	std::error_code ec;
 	std::filesystem::remove(StorePath(key), ec);
+	if (ec) {
+		throw IOException("VGI OAuth: DPAPI cache delete failed (Windows error %d)", ec.value());
+	}
 }
 
 void PlatformPrune() {
@@ -389,12 +396,9 @@ void DeleteOAuthRefreshToken(const std::string &key, const std::string &mode) {
 	if (!WantsPersistence(mode))
 		return;
 #if defined(__APPLE__) || defined(_WIN32)
-	try {
-		PlatformDelete(key);
-	} catch (...) {
-		if (mode != "auto")
-			throw;
-	}
+	// Forgetting an existing credential must report failure even in auto mode:
+	// falling back to memory would leave the saved token available to reconnect.
+	PlatformDelete(key);
 #endif
 }
 

test/README.md

@@ -8,4 +8,25 @@ make test
 or
 ```bash
 make test_debug
-```
+```
+
+## Windows OAuth credential storage
+
+Configure the extension build with `-DBUILD_VGI_OAUTH_STORE_TESTS=ON`, then run:
+
+```powershell
+cmake --build build/release --config Release --target test_vgi_oauth_store
+```
+
+This target needs Python 3 and runs the production credential store in separate
+Windows processes. It checks DPAPI persistence and replacement, concurrent
+updates, recovery after a lock owner crashes, and deletion errors in both
+`auto` and `persistent` modes. An isolated `LOCALAPPDATA` directory contains only
+synthetic credentials and is removed after each test. The suite does not contact
+an OAuth provider or access existing user credentials.
+
+To rerun an already-built probe directly:
+
+```powershell
+python test/test_oauth_store_windows.py --probe build/release/extension/vgi/vgi_oauth_store_probe.exe
+```

test/support/oauth_store_probe.cpp

@@ -0,0 +1,75 @@
+// © Copyright 2026 Query Farm LLC - https://query.farm
+// Child process for the native Windows OAuth store tests.
+
+#include "vgi_oauth.hpp"
+#include "vgi_oauth_store.hpp"
+
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+#include <windows.h>
+
+#include <chrono>
+#include <iostream>
+#include <stdexcept>
+#include <string>
+#include <thread>
+
+using namespace duckdb::vgi;
+
+static void Check(bool condition, const char *message) {
+	if (!condition) {
+		throw std::runtime_error(message);
+	}
+}
+
+static OAuthRefreshContext TestBinding() {
+	OAuthRefreshContext ctx;
+	ctx.token_endpoint = "https://issuer.example.invalid/token";
+	ctx.issuer = "https://issuer.example.invalid";
+	ctx.resource = "https://resource.example.invalid/vgi";
+	ctx.client_id = "test-client";
+	ctx.scope = "openid offline_access";
+	return ctx;
+}
+
+int main(int argc, char **argv) {
+	try {
+		Check(argc >= 4, "Usage: oauth_store_probe operation key mode [value]");
+		const std::string operation = argv[1], key = argv[2], mode = argv[3];
+		const auto binding = TestBinding();
+		auto lease = AcquireOAuthCredentialLease(key, mode);
+		// All persistent test cases have an available backend. In particular,
+		// auto must not silently fall back after encountering an abandoned mutex.
+		Check(bool(lease) == (mode == "persistent" || mode == "auto"), "Unexpected credential lease availability");
+		if (operation == "store") {
+			Check(argc == 5, "Missing synthetic token");
+			StoreOAuthRefreshToken(key, binding, mode, argv[4]);
+		} else if (operation == "read") {
+			Check(argc == 5, "Missing expected token");
+			std::string token;
+			Check(LoadOAuthRefreshToken(key, binding, mode, token), "Credential missing");
+			Check(token == argv[4], "Unexpected stored token");
+		} else if (operation == "absent") {
+			std::string token;
+			Check(!LoadOAuthRefreshToken(key, binding, mode, token), "Credential unexpectedly present");
+		} else if (operation == "delete") {
+			DeleteOAuthRefreshToken(key, mode);
+		} else if (operation == "increment") {
+			std::string token;
+			Check(LoadOAuthRefreshToken(key, binding, mode, token), "Counter missing");
+			std::this_thread::sleep_for(std::chrono::milliseconds(75));
+			StoreOAuthRefreshToken(key, binding, mode, std::to_string(std::stoi(token) + 1));
+		} else if (operation == "hold") {
+			std::cout << "LOCKED" << std::endl;
+			Sleep(INFINITE);
+		} else {
+			throw std::runtime_error("Unknown operation");
+		}
+		std::cout << "OK " << operation << '\n';
+		return 0;
+	} catch (const std::exception &e) {
+		std::cerr << e.what() << '\n';
+		return 1;
+	}
+}