mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-11 01:51:34 -10:00
fix(plugins): replace plugin state on Windows while a plugin UI grant holds the file open (CW-7956)
On Windows, disabling or revoking a plugin whose UI had just been opened was
refused with 409:
disabled failed for 'panel': failed to atomically and durably persist
...\runtime\plugins\state.json: Access is denied. (0x80070005)
A plugin UI grant keeps the plugin state file open for up to an hour, so that
every asset request can prove the state file is still the one the grant was
minted against. The Windows publication replaced the file with MoveFileExW, and
Windows refuses that replacement for as long as any handle is open on the
destination, whatever its share mode. The bounded retry around it was sized for
a scanner or an ACL pass that lets go within about a second, so it ran out and
the refusal surfaced. While any grant was live, every state change of every
plugin was refused, from this process or another: trust, enable, disable,
revoke and the state prune of an uninstall.
When MoveFileExW answers access denied, publication now opens the hardened
temporary and falls back to std::fs::rename, which replaces the destination
with POSIX rename semantics. A reader that shares delete access keeps its
handle on the superseded file and no longer blocks the replacement, which is
what Unix already did. That path does not write through, so the published file
is flushed through the handle opened before the rename, and a flush failure is
reported. A holder that denies delete sharing still gets the bounded retry and
then the refusal, with the stable file untouched.
The grant code is unchanged. After a replacement the grant's handle identifies
the superseded file, its identity check fails, and the grant is stale, as on
Unix.
Not covered: a volume without POSIX rename (FAT, exFAT, some network shares)
keeps the old refusal while a grant is open.
The plugin UI tests now print the response body when disable, enable or revoke
is refused; a failing run used to show only the status.
No released version is affected: the grant arrived after v0.10.2, so there is
no CHANGELOG line.
Evidence
- Hosted Windows before the fix (run 42972872196, the two test changes alone):
the new registry test failed with "failed to atomically and durably persist
...\state.json: Access is denied. (0x80070005)" (3 run, 2 passed, 1 failed),
and plugin_ui ran 3, passed 1, failed 2 with the 409 above for disable and
the same error for revoke.
- Hosted Windows with this change on main ecfbdbccaa (run 42978481553):
codewhale-plugins --lib 200 run, 200 passed, 1 skipped; plugin_ui 3 run, 3
passed under nextest in the hermetic test home, and "test result: ok. 3
passed; 0 failed" under cargo test. Run 42975064244 gave the same result on
main 8ddd9025c0.
- Linux build farm with this change on main ecfbdbccaa: nextest 214 run, 214
passed (3 plugin_ui, 211 codewhale-plugins). On main 8ddd9025c0, clippy for
both crates with all targets and all features was clean.
- cargo fmt --all -- --check and scripts/preflight.sh --check pass.
Co-Authored-By: Claude Fable 5.2 <[email protected]>
This commit is contained in:
1 parent
ecfbdbccaa
commit
fc78dc86d8
2 files changed
+65
-9
No files matched your search
@@ -135,6 +135,11 @@ async fn ui(
|
||||
Ok((status, response.json().await?))
|
||||
}
|
||||
|
||||
async fn mutate(client: &reqwest::Client, url: String) -> Result<(StatusCode, String)> {
|
||||
let response = client.post(url).send().await?;
|
||||
Ok((response.status(), response.text().await?))
|
||||
}
|
||||
|
||||
fn refusal(status: u16, body: &serde_json::Value) -> (u16, String) {
|
||||
(
|
||||
status,
|
||||
@@ -370,14 +375,14 @@ async fn plugin_ui_dsh_client_serves_admitted_seats_and_denies_and_revokes() ->
|
||||
.is_client_error()
|
||||
);
|
||||
|
||||
let disabled = client.post(format!("{base}/{name}/disable")).send().await?;
|
||||
assert_eq!(disabled.status(), StatusCode::OK);
|
||||
let (status, body) = mutate(&client, format!("{base}/{name}/disable")).await?;
|
||||
assert_eq!(status, StatusCode::OK, "{body}");
|
||||
for url in [&asset_url, &route] {
|
||||
let response = client.get(url).header("If-Match", &revision).send().await?;
|
||||
assert_eq!(response.status(), StatusCode::PRECONDITION_FAILED, "{url}");
|
||||
}
|
||||
let enabled = client.post(format!("{base}/{name}/enable")).send().await?;
|
||||
assert_eq!(enabled.status(), StatusCode::OK);
|
||||
let (status, body) = mutate(&client, format!("{base}/{name}/enable")).await?;
|
||||
assert_eq!(status, StatusCode::OK, "{body}");
|
||||
let stale = client
|
||||
.get(&asset_url)
|
||||
.header("If-Match", &revision)
|
||||
@@ -509,8 +514,8 @@ async fn plugin_ui_native_client_serves_only_the_detail_seat_and_is_revoked() ->
|
||||
let body: serde_json::Value = routed.json().await?;
|
||||
assert_eq!(body["error"]["code"], "plugin_route_not_admitted");
|
||||
|
||||
let revoked = client.post(format!("{base}/{name}/revoke")).send().await?;
|
||||
assert_eq!(revoked.status(), StatusCode::OK);
|
||||
let (status, body) = mutate(&client, format!("{base}/{name}/revoke")).await?;
|
||||
assert_eq!(status, StatusCode::OK, "{body}");
|
||||
let after = client
|
||||
.get(&asset)
|
||||
.header("If-Match", &revision)
|
||||
|
||||
@@ -1208,7 +1208,7 @@ fn persist_plugin_state_with_directory_sync(
|
||||
#[cfg(windows)]
|
||||
fn persist_plugin_state(mut temporary: tempfile::TempPath, path: &Path) -> Result<(), String> {
|
||||
use std::os::windows::ffi::OsStrExt as _;
|
||||
use windows::Win32::Foundation::WIN32_ERROR;
|
||||
use windows::Win32::Foundation::{ERROR_ACCESS_DENIED, WIN32_ERROR};
|
||||
use windows::Win32::Storage::FileSystem::{
|
||||
FILE_ATTRIBUTE_NORMAL, FILE_ATTRIBUTE_TEMPORARY, MOVEFILE_REPLACE_EXISTING,
|
||||
MOVEFILE_WRITE_THROUGH, MoveFileExW, SetFileAttributesW,
|
||||
@@ -1254,7 +1254,19 @@ fn persist_plugin_state(mut temporary: tempfile::TempPath, path: &Path) -> Resul
|
||||
} {
|
||||
// MoveFileExW reports an HRESULT; the shared classifier reads the
|
||||
// Win32 code it wraps.
|
||||
let backoff = WIN32_ERROR::from_error(&error).and_then(|code| {
|
||||
let code = WIN32_ERROR::from_error(&error);
|
||||
if code == Some(ERROR_ACCESS_DENIED)
|
||||
&& let Ok(Some(source)) = open_existing_regular_file(&temporary_path, true)
|
||||
&& fs::rename(&temporary_path, path).is_ok()
|
||||
{
|
||||
temporary.disable_cleanup(true);
|
||||
return source.sync_all().map_err(|error| {
|
||||
format!(
|
||||
"plugin state was published but its durability could not be confirmed: {error}"
|
||||
)
|
||||
});
|
||||
}
|
||||
let backoff = code.and_then(|code| {
|
||||
codewhale_util::utils::windows_publish_retry_delay(
|
||||
&std::io::Error::from_raw_os_error(code.0 as i32),
|
||||
attempt,
|
||||
@@ -3060,7 +3072,10 @@ pub fn capture_plugin_state_authority(
|
||||
|
||||
#[cfg(test)]
|
||||
mod state_publication_tests {
|
||||
use super::{PluginStateFile, harden_plugin_state_file, save_state_with_hardener};
|
||||
use super::{
|
||||
PluginStateFile, harden_plugin_state_file, open_existing_regular_file,
|
||||
save_state_with_hardener,
|
||||
};
|
||||
|
||||
fn prepare_private_directory(_path: &std::path::Path) {
|
||||
#[cfg(unix)]
|
||||
@@ -3093,6 +3108,42 @@ mod state_publication_tests {
|
||||
assert_eq!(entries, [std::ffi::OsString::from("state-鲸.json")]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn state_publication_replaces_a_stable_file_that_a_reader_still_holds_open() {
|
||||
use std::io::Read as _;
|
||||
|
||||
let directory = tempfile::tempdir().unwrap();
|
||||
prepare_private_directory(directory.path());
|
||||
let state_path = directory.path().join("state.json");
|
||||
std::fs::write(&state_path, b"old-authoritative-state").unwrap();
|
||||
let mut held = open_existing_regular_file(&state_path, false)
|
||||
.unwrap()
|
||||
.unwrap();
|
||||
|
||||
save_state_with_hardener(
|
||||
&state_path,
|
||||
&PluginStateFile::default(),
|
||||
harden_plugin_state_file,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let mut published = open_existing_regular_file(&state_path, false)
|
||||
.unwrap()
|
||||
.unwrap();
|
||||
assert!(!codewhale_util::files::same_file(&held, &published).unwrap());
|
||||
let mut superseded = String::new();
|
||||
held.read_to_string(&mut superseded).unwrap();
|
||||
assert_eq!(superseded, "old-authoritative-state");
|
||||
let mut current = String::new();
|
||||
published.read_to_string(&mut current).unwrap();
|
||||
assert!(current.contains("\"schema_version\": 1"));
|
||||
let entries = std::fs::read_dir(directory.path())
|
||||
.unwrap()
|
||||
.map(|entry| entry.unwrap().file_name())
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(entries, [std::ffi::OsString::from("state.json")]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failed_temp_hardening_never_publishes_new_plugin_state() {
|
||||
let directory = tempfile::tempdir().unwrap();
|
||||
|
||||
Reference in new issue
Block a user