fix(client): flush the written binary and back off the Windows service start (os error 32) - #2230
mikhailm-coder wants to merge 2 commits into
Conversation
…e start (os error 32) A Service-type tool update on Windows wrote the new agent.exe and called SCM StartService in the same second; all 3 attempts failed within ~1 s with os error 32 while our own write handle or the AV scan still held the file, and the update was reported failed with the binary already on disk. - binary_writer::write_executable: flush, sync_all under a 30 s timeout, explicit drop before permissions and the "Binary written" line - binary_writer::wait_until_executable_unlocked (Windows): exclusive-open probe, 250 ms polls up to 30 s, run before start in finalize and orphan remediation (skipped when SCM already reports the service running); on give-up it names the holder via Restart Manager - system_service::start_service (Windows): 1, 2, 4, 8, 15 s backoff; try_start_service_windows returns the Win32 code so 1060/1058/2/3/1072 fail immediately; a start call that times out or a RUNNING confirmation that SCM stops answering also bails, since a parked StartService keeps its permit in the 4-slot SCM pool and retrying would starve other callers - tests for the writer close, the probe and the pure retry policy Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| binary_writer::wait_until_executable_unlocked( | ||
| exec_path, | ||
| std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS), | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] binary_writer::wait_until_executable_unlocked call is not gated for non-Windows targets in orphan remediation path
In service.rs's orphan remediation method (restart_service_on_new_binary-like helper), the new call to binary_writer::wait_until_executable_unlocked(exec_path, ...) is unconditional, but wait_until_executable_unlocked is defined with #[cfg(target_os = "windows")] in binary_writer.rs. The use crate::platform::binary_writer; import for this call site is itself gated with #[cfg(target_os = "windows")] (see the diff's added #[cfg(target_os = "windows")] use crate::platform::binary_writer;), but the macOS branch also imports binary_writer unconditionally via #[cfg(target_os = "macos")] use crate::platform::{binary_writer, remove_app_bundle_path};. On macOS, binary_writer is imported but the call to wait_until_executable_unlocked at line 93 has no cfg guard, so it will fail to compile on macOS/Linux where the function does not exist under that name (only compiled for windows). This will break the macOS and Linux builds.
Evidence
binary_writer::wait_until_executable_unlocked(
exec_path,
std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS),
)
.await;
📝 Committable suggestion
| binary_writer::wait_until_executable_unlocked( | |
| exec_path, | |
| std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS), | |
| ) | |
| .await; | |
| #[cfg(target_os = "windows")] | |
| binary_writer::wait_until_executable_unlocked( | |
| exec_path, | |
| std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS), | |
| ) | |
| .await; |
🤖 Prompt for AI agents
In clients/openframe-client/src/platform/tool_updater/service.rs around lines 93-97, address this code-review finding: binary_writer::wait_until_executable_unlocked call is not gated for non-Windows targets in orphan remediation path.
In service.rs's orphan remediation method (`restart_service_on_new_binary`-like helper), the new call to `binary_writer::wait_until_executable_unlocked(exec_path, ...)` is unconditional, but `wait_until_executable_unlocked` is defined with `#[cfg(target_os = "windows")]` in binary_writer.rs. The `use crate::platform::binary_writer;` import for this call site is itself gated with `#[cfg(target_os = "windows")]` (see the diff's added `#[cfg(target_os = "windows")] use crate::platform::binary_writer;`), but the macOS branch also imports `binary_writer` unconditionally via `#[cfg(target_os = "macos")] use crate::platform::{binary_writer, remove_app_bundle_path};`. On macOS, `binary_writer` is imported but the call to `wait_until_executable_unlocked` at line 93 has no cfg guard, so it will fail to compile on macOS/Linux where the function does not exist under that name (only compiled for windows). This will break the macOS and Linux builds.
The flagged code:
```
binary_writer::wait_until_executable_unlocked(
exec_path,
std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS),
)
.await;
```
A suggested replacement for the flagged range:
#[cfg(target_os = "windows")]
binary_writer::wait_until_executable_unlocked(
exec_path,
std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS),
)
.await;
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 72 — react 👍/👎 to teach the reviewer
There was a problem hiding this comment.
Leaving as-is: this call is inside remediate_orphaned_service, which is itself declared #[cfg(target_os = "windows")] (service.rs lines 52-53), so the whole function, including this call, only compiles on Windows. Host (macOS) cargo clippy --all-targets -- -D warnings and the build are clean locally; the Test Rust (macos-latest) job runs the same.
| } else { | ||
| // A running service would hold the exe itself, so only probe when SCM says it is stopped. | ||
| binary_writer::wait_until_executable_unlocked( | ||
| &exec_path, | ||
| std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS), | ||
| ) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
🦩 🔴 [error/action_required] EXEC_UNLOCK_WAIT_SECS import and second unlocked-wait call site not gated for non-Windows targets
The EXEC_UNLOCK_WAIT_SECS import in service.rs is #[cfg(target_os = "windows")], so referencing it directly at the second call site (in the ToolUpdater::apply/finalize path, else-branch calling binary_writer::wait_until_executable_unlocked(&exec_path, ...)) without a matching cfg guard will fail to compile on macOS/Linux, since neither the constant nor the function exists there. This mirrors the same defect as the orphan-remediation call site.
Evidence
} else {
// A running service would hold the exe itself, so only probe when SCM says it is stopped.
binary_writer::wait_until_executable_unlocked(
&exec_path,
std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS),
)
.await;
}
📝 Committable suggestion
| } else { | |
| // A running service would hold the exe itself, so only probe when SCM says it is stopped. | |
| binary_writer::wait_until_executable_unlocked( | |
| &exec_path, | |
| std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS), | |
| ) | |
| .await; | |
| } | |
| } else { | |
| // A running service would hold the exe itself, so only probe when SCM says it is stopped. | |
| #[cfg(target_os = "windows")] | |
| binary_writer::wait_until_executable_unlocked( | |
| &exec_path, | |
| std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS), | |
| ) | |
| .await; | |
| } |
🤖 Prompt for AI agents
In clients/openframe-client/src/platform/tool_updater/service.rs around lines 216-223, address this code-review finding: EXEC_UNLOCK_WAIT_SECS import and second unlocked-wait call site not gated for non-Windows targets.
The `EXEC_UNLOCK_WAIT_SECS` import in service.rs is `#[cfg(target_os = "windows")]`, so referencing it directly at the second call site (in the ToolUpdater::apply/finalize path, else-branch calling `binary_writer::wait_until_executable_unlocked(&exec_path, ...)`) without a matching cfg guard will fail to compile on macOS/Linux, since neither the constant nor the function exists there. This mirrors the same defect as the orphan-remediation call site.
The flagged code:
```
} else {
// A running service would hold the exe itself, so only probe when SCM says it is stopped.
binary_writer::wait_until_executable_unlocked(
&exec_path,
std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS),
)
.await;
}
```
A suggested replacement for the flagged range:
} else {
// A running service would hold the exe itself, so only probe when SCM says it is stopped.
#[cfg(target_os = "windows")]
binary_writer::wait_until_executable_unlocked(
&exec_path,
std::time::Duration::from_secs(EXEC_UNLOCK_WAIT_SECS),
)
.await;
}
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 72 — react 👍/👎 to teach the reviewer
There was a problem hiding this comment.
Leaving as-is: this else branch is inside the existing #[cfg(target_os = "windows")] { ... } block of finalize (the one that starts with let exec_path = self.resolve_executable_path(tool);), so neither EXEC_UNLOCK_WAIT_SECS nor wait_until_executable_unlocked is referenced on macOS/Linux. Host (macOS) clippy and build pass locally.
| last_err = match failure { | ||
| ServiceStartFailure::Transient(message) => message, | ||
| ServiceStartFailure::Permanent(message) => anyhow::bail!( | ||
| "Failed to start service {} on attempt {}/{}, not retrying a permanent SCM error: {}", | ||
| service_name, | ||
| attempt, | ||
| total_attempts, | ||
| message | ||
| ), | ||
| ServiceStartFailure::ScmUnresponsive(message) => anyhow::bail!( | ||
| "Failed to start service {} on attempt {}/{}, not retrying while SCM is unresponsive: {}", | ||
| service_name, | ||
| attempt, | ||
| total_attempts, | ||
| message | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] ServiceStartFailure::ScmUnresponsive is treated as fatal (bail!) instead of retried, unlike its old behavior
Previously, when the SCM was unresponsive (could not confirm RUNNING (SCM unresponsive)) or the scm_call itself timed out, the loop would still retry up to SERVICE_START_MAX_ATTEMPTS times with a fixed PROCESS_CHECK_INTERVAL_MS delay. In the new code, ServiceStartFailure::ScmUnresponsive immediately calls anyhow::bail! and aborts the whole retry loop on the very first occurrence, rather than backing off and retrying like the previous implementation did. This is a meaningful behavior regression: a single transient SCM stall (e.g. the SCM being briefly busy) will now abort service startup entirely instead of retrying, which could cause tool updates to fail unnecessarily on flaky machines.
Evidence
last_err = match failure {
ServiceStartFailure::Transient(message) => message,
ServiceStartFailure::Permanent(message) => anyhow::bail!(
"Failed to start service {} on attempt {}/{}, not retrying a permanent SCM error: {}",
service_name,
attempt,
total_attempts,
message
),
ServiceStartFailure::ScmUnresponsive(message) => anyhow::bail!(
"Failed to start service {} on attempt {}/{}, not retrying while SCM is unresponsive: {}",
service_name,
🤖 Prompt for AI agents
In clients/openframe-client/src/platform/system_service.rs around lines 185-201, address this code-review finding: ServiceStartFailure::ScmUnresponsive is treated as fatal (bail!) instead of retried, unlike its old behavior.
Previously, when the SCM was unresponsive (`could not confirm RUNNING (SCM unresponsive)`) or the scm_call itself timed out, the loop would still retry up to SERVICE_START_MAX_ATTEMPTS times with a fixed PROCESS_CHECK_INTERVAL_MS delay. In the new code, `ServiceStartFailure::ScmUnresponsive` immediately calls `anyhow::bail!` and aborts the whole retry loop on the very first occurrence, rather than backing off and retrying like the previous implementation did. This is a meaningful behavior regression: a single transient SCM stall (e.g. the SCM being briefly busy) will now abort service startup entirely instead of retrying, which could cause tool updates to fail unnecessarily on flaky machines.
The flagged code:
```
last_err = match failure {
ServiceStartFailure::Transient(message) => message,
ServiceStartFailure::Permanent(message) => anyhow::bail!(
"Failed to start service {} on attempt {}/{}, not retrying a permanent SCM error: {}",
service_name,
attempt,
total_attempts,
message
),
ServiceStartFailure::ScmUnresponsive(message) => anyhow::bail!(
"Failed to start service {} on attempt {}/{}, not retrying while SCM is unresponsive: {}",
service_name,
attempt,
total_attempts,
message
),
};
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 55 — react 👍/👎 to teach the reviewer
There was a problem hiding this comment.
Leaving as-is, deliberately (called out in the PR description). A stall shorter than SERVICE_START_CALL_TIMEOUT_SECS (30 s) never surfaces as ScmUnresponsive: the first attempt simply waits it out inside scm_call_timed. Only a 30 s+ non-answer, or two consecutive 10 s status-query timeouts after a successful start call, lands in this arm, and that is the wedged-SCM case: every retried StartService parks one more blocking thread holding one of the 4 SCM_MAX_IN_FLIGHT permits until SCM answers, starving stop_service and status queries for every other tool (the TATS02 pattern). The outer layers (tool restart's second try, TOOL_UPDATE redelivery, mesh self-heal) still retry. If the old retry is preferred, switching this arm to Transient is a one-word change.
There was a problem hiding this comment.
Reversed most of this in 17a256a — you were right that the blanket bail was a regression. Now only a start call still parked inside SCM after its 30 s cap skips the retry, because that thread holds one of four permits until Windows returns and five more would drain the pool. Two changes: a busy pool (no call issued, nothing parked) is retried again, via a typed PermitPoolError::is_busy() instead of a bare anyhow; and could not confirm RUNNING is retried again, since the start call there already succeeded and re-issuing it just costs an already-running no-op. Belt and braces, finalize now treats a start error as success when the tool process is running — a process scan, so it needs no SCM and works exactly when SCM is what failed.
…ess scan over a wedged SCM Narrows the no-retry rule added in the previous commit. Only a start call still parked inside SCM justifies bailing; the other two cases were regressions against the old 3-attempt loop. - timed_permit_pool: typed PermitPoolError with is_busy(). A busy pool issued no call, so nothing is parked and a retry costs the pool nothing; a timed-out call left a blocking thread holding a permit. Display text is unchanged. - system_service: classify_pool_failure() retries the busy-pool case and bails only on a parked start call. "could not confirm RUNNING" goes back to being retried: the start call itself succeeded there, so re-issuing it costs one already-running no-op, while failing reports a service that is very likely up. - tool_updater/service.rs: finalize now treats a start error as success when the tool's process is running. The check is a process scan, so it needs no SCM and works precisely when SCM is the thing that failed. - tests for the busy-vs-parked distinction and the classification Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed 17a256a since the last pass. It narrows the no-retry rule that the |
🦩 Flamingo Code Review1 finding(s) — 0 action required · 1 recommended · 0 informational Mode: advisory · 1 defect(s) outside any rule Inline comments: 1 new Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-21 11:15 UTC · updated 2026-09-21 11:16 UTC · workflow run |
| /// A busy pool issued no call, so nothing is parked and a retry is free; a timed-out call left a thread in SCM. | ||
| #[cfg(target_os = "windows")] | ||
| fn classify_pool_failure(e: PermitPoolError) -> ServiceStartFailure { | ||
| let message = e.to_string(); | ||
| if e.is_busy() { | ||
| ServiceStartFailure::Transient(message) | ||
| } else { | ||
| ServiceStartFailure::ScmUnresponsive(message) | ||
| } | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] ServiceStartFailure::ScmUnresponsive causes start_service to fail fast even though the service may already be running
The comment on the None branch of wait_for_service_running_windows ("The start call itself succeeded, so retry: re-issuing it costs one already-running no-op") documents that when confirmation cannot be obtained the service may actually be up, yet this path is classified as Transient and retried — that's fine. But ScmUnresponsive (raised only from classify_pool_failure when a scm_call_timed call itself times out, i.e. the start call, not the confirmation) causes an immediate bail! without ever calling wait_for_service_running_windows to check if the service actually did come up despite SCM being wedged. The caller in tool_updater/service.rs does have a fallback (checking is_installed_tool_running), which mitigates this at that call site, but start_service itself now fails permanently on the very first SCM hang even though a subsequent probe might show success. This is a behavior change from the old code (which retried on any error) and could cause tool updates to report failure when the service is in fact starting. Worth flagging for review since it changes reliability characteristics under load (busy/wedged SCM), which is exactly the scenario this PR targets.
Evidence
/// A busy pool issued no call, so nothing is parked and a retry is free; a timed-out call left a thread in SCM.
#[cfg(target_os = "windows")]
fn classify_pool_failure(e: PermitPoolError) -> ServiceStartFailure {
let message = e.to_string();
if e.is_busy() {
ServiceStartFailure::Transient(message)
} else {
ServiceStartFailure::ScmUnresponsive(message)
}
}
🤖 Prompt for AI agents
In clients/openframe-client/src/platform/system_service.rs around lines 141-150, address this code-review finding: ServiceStartFailure::ScmUnresponsive causes start_service to fail fast even though the service may already be running.
The comment on the `None` branch of `wait_for_service_running_windows` ("The start call itself succeeded, so retry: re-issuing it costs one already-running no-op") documents that when confirmation cannot be obtained the service may actually be up, yet this path is classified as `Transient` and retried — that's fine. But `ScmUnresponsive` (raised only from `classify_pool_failure` when a `scm_call_timed` call itself times out, i.e. the *start* call, not the confirmation) causes an immediate `bail!` without ever calling `wait_for_service_running_windows` to check if the service actually did come up despite SCM being wedged. The caller in `tool_updater/service.rs` does have a fallback (checking `is_installed_tool_running`), which mitigates this at that call site, but `start_service` itself now fails permanently on the very first SCM hang even though a subsequent probe might show success. This is a behavior change from the old code (which retried on any error) and could cause tool updates to report failure when the service is in fact starting. Worth flagging for review since it changes reliability characteristics under load (busy/wedged SCM), which is exactly the scenario this PR targets.
The flagged code:
```
/// A busy pool issued no call, so nothing is parked and a retry is free; a timed-out call left a thread in SCM.
#[cfg(target_os = "windows")]
fn classify_pool_failure(e: PermitPoolError) -> ServiceStartFailure {
let message = e.to_string();
if e.is_busy() {
ServiceStartFailure::Transient(message)
} else {
ServiceStartFailure::ScmUnresponsive(message)
}
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 30 — react 👍/👎 to teach the reviewer
There was a problem hiding this comment.
Valid observation, leaving as-is. The probe you suggest would itself go through the same permit pool, so on a wedged SCM it hangs too: up to two more 10 s status queries, each parking another blocking thread, out of only four slots. That is the cost this branch exists to avoid, and it would tell us nothing in the case it is meant to cover.
There is no SCM-free check available inside start_service, which only has a service name. That is why the fallback lives in the callers, where an executable path is in hand and the check is a plain process scan: ServiceToolUpdater::finalize (added in 17a256a) and tool_restart_service (already had one). Those are the two paths where a false failure costs something, because they are what records and publishes the tool version.
The remaining callers are fine without it. verify_service_running on the install path is meant to fail here — it keeps the record Installing so the install retries, and on that retry it does a status query first and returns Ok if the service did come up, so it self-corrects. remediate_orphaned_service, rollback and restart_all only log. Worst case for a parked start call is now one held permit instead of the old three.
2 participants
Motivation
During the mesh 0.0.28 rollout on 15 Sep, a Service-type tool update on Windows wrote the new
agent.exeand called SCMStartServicein the same second. All three start attempts failed within ~1 s withos error 32(sharing violation). The update was reported as failed even though the binary was on disk, theTOOL_UPDATEmessage stayed unacked and was redelivered every 2 minutes, and the new version was never recorded. On every machine checked, a start of the same binary issued 7-20 s later succeeded.Two things hold the freshly written exe: our own
tokio::fs::Filewrite handle (writes are queued to a blocking thread; the function never flushed and let the handle drop with the last chunk possibly still in flight), and antivirus scanning the new executable. The retry budget of 3 x 500 ms could not outlast either.Changes
binary_writer::write_executable:flush()(this also surfaces the last chunk's write error, which was previously lost),sync_all()under a 30 s timeout (warn and continue on a slow disk), and an explicitdrop(file)before permissions are set and before theBinary writtenline, so that line now means our handle is closed. Covers the updater, the initial install and asset writes.binary_writer::wait_until_executable_unlocked(Windows): polls an exclusive-share open (share_mode(0)) every 250 ms for up to 30 s until nobody holds the file, and logs how long it waited. Any error other than os 32 lets the start proceed and fail with the real reason. If it gives up, the log names the holder via the existing Restart Manager helper (a filter-driver/AV hold shows as "no process").ServiceToolUpdater::finalizeandremediate_orphaned_servicerun the probe right beforestart_service. Infinalizeit is skipped when SCM already reports the service running (SCM recovery restarted it on the new binary), since that process holds the exe itself and the probe would only burn the 30 s.system_service::start_service(Windows): the 3 x 500 ms loop becomes a 1, 2, 4, 8, 15 s backoff, so 6 attempts with ~30 s of waiting (SERVICE_START_RETRY_DELAYS_SECSreplacesSERVICE_START_MAX_ATTEMPTS).try_start_service_windowsnow returns aServiceStartFailureclassified by Win32 code: 1060 (does not exist), 1058 (disabled), 2/3 (image path missing) and 1072 (marked for delete) fail immediately, because no retry converts those into a start. Everything else retries, including 32, 1053, 1055, 5, a busy permit pool and the "did not reach RUNNING" branch. TheStart attempt N/total for service X failed: ...log format is unchanged (total is now 6, so Loki searches pinned on/3need updating); the chosen delay is logged.ServiceToolUpdater::finalizetreats a start error as success when the tool's process is running. That check is a process scan against the executable path, so it needs no SCM and works precisely when SCM is the thing that failed.TimedPermitPoolnow returns a typedPermitPoolErrorinstead of a bareanyhow, so callers can tell a busy pool (no call issued, nothing parked, free to retry) from a timed-out call (a blocking thread is parked holding a permit). Display text is unchanged, so existing log lines and searches still match.start_servicecaller (tool restart, run manager, mesh self-heal, own-service nudge, install verification) gets the backoff without changes.config::service_stop.One deliberate deviation from the task text
A start call that is still parked inside SCM after its 30 s cap does not get retried. That is the only case where a retry provably adds harm: the parked thread keeps one of the four SCM permits until Windows returns, so retrying it up to five more times can drain the pool and starve every other SCM caller, which is the amplification pattern seen on TATS02. Everything else that was retried before is still retried. A short SCM stall never reaches this branch, because the call simply waits inside its own 30 s cap first. The outer layers (tool restart's second try,
TOOL_UPDATEredelivery, mesh self-heal) still retry, and with the running check above, a service that did come up is no longer reported as failed.Tests
binary_writer_tests: a 3 MB write (several tokio write chunks) re-reads byte-for-byte and, on Windows, becomes exclusively openable within a bounded poll (a single instant open would be flaky under the runner's AV scan-on-close). Windows-only: the probe returns false while a handle is held and true promptly once released; a missing file passes through immediately.system_service_tests: the backoff schedule and the transient-vs-permanent classification by OS code (platform-independent, so they run in both CI jobs), plus Windows-only checks thatfrom_scmclassifies 1060 as permanent and 32 as transient keeping the stage prefix, and that a busy pool retries while a parked start call bails.timed_permit_pool_tests: a parked call and a busy pool are distinguishable, and the pool recovers once the blocked closure returns.windows-latestjob.Verification
cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean on macOS and forx86_64-pc-windows-gnu--skip test_ensure_admin); the previous push was green onwindows-latesttoo, with all Windows-only tests executed--features bin --target x86_64-pc-windows-gnu) greenflush/sync_all/drop semantics and the CreateFile sharing rules the probe relies on were verified against the library sourceLeft as is, for the record: the permanent-code list is exactly the task's five codes (193, 1069, 1075, 1115 would also qualify; say the word and I add them); the legacy Artifactory update path still writes without a flush (Standard tools only, relaunched by the run manager); a finalize failure still does not roll back; there are no Progress acks during a long update, so an update running past the 120 s ack wait can still be redelivered mid-flight.
CU-86akkdez7
🤖 Generated with Claude Code