fix(daemon): recover stale serial locks#978
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis PR adds serial-session client metadata tracking, pending-attach tracking, expanded lock-status and clear-locks API models, a request-driven ChangesSerial lock diagnostics and recovery
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DaemonClient
participant LocksHandler
participant SerialManager
CLI->>DaemonClient: ClearLocksRequest(serial, stale, port, client_id, force)
DaemonClient->>LocksHandler: POST /api/locks/clear (request)
LocksHandler->>LocksHandler: clear project locks
LocksHandler->>LocksHandler: evaluate live/stale/force per session
LocksHandler->>SerialManager: close matching sessions
LocksHandler-->>DaemonClient: ClearLocksResponse(counts, cleared_sessions, refused)
DaemonClient-->>CLI: print cleared counts, sessions, warnings
sequenceDiagram
participant PythonClient
participant WebsocketHandler
participant DaemonContext
participant SerialManager
PythonClient->>WebsocketHandler: Attach(client_metadata)
WebsocketHandler->>DaemonContext: begin_pending_serial_attach
WebsocketHandler->>DaemonContext: update_pending_serial_attach(client_id, port)
WebsocketHandler->>SerialManager: open_port(client_metadata)
WebsocketHandler->>SerialManager: attach_reader(client_metadata)
WebsocketHandler->>DaemonContext: end_pending_serial_attach
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zackees
force-pushed
the
feat/976-serial-lock-recovery
branch
from
7a7a1c5 to
3302e9d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fbuild-serial/src/manager.rs (1)
667-674: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
client_metadataentries are never cleaned up on detach — stale/unbounded growth.
detach_readerandrelease_writerupdatelast_activity_atbut never remove the corresponding entry fromsession.client_metadata. Sinceget_port_sessions()(Line 971-978) builds theclientslist straight fromclient_metadatawithout cross-checkingreader_client_ids/writer_client_id, a client that has fully detached keeps appearing infbuild daemon locksoutput for as long as the session stays open. For a port that never goes fully idle (frequent reconnects with newclient_ids beforeclose_port_after_grace_if_idlefires), this map grows unbounded for the daemon's lifetime and misleads operators diagnosing "who's actually attached."Suggest removing the metadata entry once a client_id is neither a reader nor the writer.
🧹 Proposed fix: clean up client_metadata when a client fully detaches
pub fn detach_reader(&self, port: &str, client_id: &str) { let session_key = self.resolve_port_key(port); if let Some(mut session) = self.sessions.get_mut(&session_key) { session.reader_client_ids.remove(client_id); session.last_activity_at = now_unix_secs(); + if session.writer_client_id.as_deref() != Some(client_id) { + session.client_metadata.remove(client_id); + } } }pub fn release_writer(&self, port: &str, client_id: &str) { let session_key = self.resolve_port_key(port); if let Some(mut session) = self.sessions.get_mut(&session_key) { if session.writer_client_id.as_deref() == Some(client_id) { session.writer_client_id = None; session.last_activity_at = now_unix_secs(); + if !session.reader_client_ids.contains(client_id) { + session.client_metadata.remove(client_id); + } } } }Also applies to: 880-889
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-serial/src/manager.rs` around lines 667 - 674, `detach_reader` (and the corresponding writer-release path) only updates `last_activity_at` and leaves stale entries behind in `session.client_metadata`, so fully detached clients keep showing up in `get_port_sessions()` and the map can grow without bound. Update `detach_reader` and `release_writer` to remove the `client_id` from `client_metadata` once that client is no longer present in `reader_client_ids` and is not the active `writer_client_id`, while keeping the existing session lookup via `resolve_port_key` and `sessions.get_mut` intact.
🧹 Nitpick comments (3)
crates/fbuild-cli/src/cli/daemon_cmd.rs (2)
777-780: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for
format_age_seconds.It's a small pure function (clamp negative to zero, delegate to
format_uptime) that directly drives user-facing output for lock ages; a quick test would guard against regressions.As per coding guidelines, "Write tests in Rust and follow TDD... test real behavior... instead of mocks when exercising real integration boundaries" — this is a pure function with no integration boundary, so a plain
#[test]suffices.✅ Suggested test
#[cfg(test)] mod age_format_tests { use super::format_age_seconds; #[test] fn clamps_negative_to_zero() { assert_eq!(format_age_seconds(-5.0), "0s"); } #[test] fn formats_positive_age() { assert_eq!(format_age_seconds(90.0), "1m 30s"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-cli/src/cli/daemon_cmd.rs` around lines 777 - 780, Add a unit test coverage for the pure helper `format_age_seconds` in `daemon_cmd.rs`, since it clamps negative values and delegates to `format_uptime`. Create a small `#[cfg(test)]` module near `format_age_seconds` with plain `#[test]` cases that verify a negative input is clamped to zero and a normal positive value is formatted through `format_uptime` as expected.Source: Coding guidelines
363-410: 📐 Maintainability & Code Quality | 🔵 TrivialConsider renaming the inner loop variable.
for client in &lock.clientsshadows the outerclient: &DaemonClientparameter. It compiles fine (the compiler would catch any accidental misuse sinceSerialClientLockInfohas noDaemonClientmethods), but it's a readability trap for future edits inside this block.♻️ Suggested rename
- for client in &lock.clients { - let pid = client + for serial_client in &lock.clients { + let pid = serial_client .pid .map(|pid| pid.to_string()) .unwrap_or_else(|| "unknown".to_string()); - let alive = client + let alive = serial_client .process_alive .map(|alive| if alive { "alive" } else { "dead" }) .unwrap_or("unknown"); output::result(format!( " client {} pid={} ({})", - client.client_id, pid, alive + serial_client.client_id, pid, alive )); - if let Some(exe) = &client.exe { + if let Some(exe) = &serial_client.exe { output::result(format!(" exe: {}", exe)); } - if let Some(cwd) = &client.cwd { + if let Some(cwd) = &serial_client.cwd { output::result(format!(" cwd: {}", cwd)); } - if let Some(argv) = &client.argv { + if let Some(argv) = &serial_client.argv { if !argv.is_empty() { output::result(format!(" argv: {}", argv.join(" "))); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-cli/src/cli/daemon_cmd.rs` around lines 363 - 410, The inner loop variable in the lock-details rendering block shadows the outer DaemonClient reference, which hurts readability and can confuse future edits. Rename the loop variable in the section that iterates over lock.clients to something distinct (for example, client_info or lock_client) and update the uses within that loop so the outer client binding remains clearly distinguishable.crates/fbuild-serial/src/manager/tests.rs (1)
152-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct test for the
client_metadataupsert path.Every updated call here passes
Nonefor the newclient_metadataargument, soopen_port/attach_reader's actual upsert-into-session.client_metadatabehavior (Lines 106-110, 656-660) isn't covered at this layer — only indirectly via a different crate's handler tests that writeclient_metadatadirectly rather than through these APIs.As per coding guidelines, "Write tests in Rust and follow TDD: red → green → refactor, test real behavior... instead of mocks." A small unit test calling
open_port/attach_readerwithSome(SerialClientMetadata { .. })and asserting the result viaget_port_sessions()would close this gap.Also applies to: 229-229, 387-387, 440-440
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fbuild-serial/src/manager/tests.rs` around lines 152 - 156, The current tests only exercise open_port and attach_reader with None for client_metadata, so the session.client_metadata upsert path in those APIs is not directly covered. Add a focused unit test in manager/tests.rs that calls open_port or attach_reader with Some(SerialClientMetadata { .. }) and then verifies the stored value through get_port_sessions(), using the real manager methods rather than writing metadata directly. Update the affected test cases around open_port and attach_reader so the new client_metadata behavior is explicitly asserted.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fbuild-cli/src/cli/daemon_cmd.rs`:
- Around line 446-474: The refusal reasons are being emitted twice in
run_daemon_clear_locks because result.message already includes the joined
refused text and the loop over result.refused warns each item again. Update the
output in run_daemon_clear_locks so refusals are shown only once: either keep
output::result(result.message) and remove the per-item warning loop, or make the
daemon-side clear_locks response message concise and rely on the refused list
for details. Use the run_daemon_clear_locks and clear_locks handler behavior to
keep the formatting consistent.
- Around line 114-129: Make `--stale` imply serial cleanup in
`DaemonAction::ClearLocks` within `daemon_cmd.rs`: the current
`ClearLocksRequest` construction passes `serial` and `stale` independently, so
`run_daemon_clear_locks` still misses stale serial sessions when only `--stale`
is set. Update the request building so the `serial` field is derived from
`serial || stale` while leaving the rest of the `ClearLocksRequest` fields
unchanged, ensuring `fbuild daemon clear-locks --stale` reaches the
serial-session cleanup path as intended.
In `@crates/fbuild-daemon/src/handlers/locks.rs`:
- Around line 141-159: The Windows PID liveness check in the lock handler treats
any OpenProcess failure as dead, which incorrectly marks access-denied processes
as stale. Update the logic in the Windows branch of the PID check in locks
handling so that ERROR_ACCESS_DENIED from
OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, ...) is treated as alive, while
only true “missing process” cases return false. Use the existing Windows FFI
symbols OpenProcess, CloseHandle, and the session_stale_reason flow to keep
active serial sessions from being cleared.
In `@crates/fbuild-python/src/messages.rs`:
- Around line 12-38: ClientMetadata::current() is collecting sensitive process
details that are later exposed by the lock-status API, so redact or omit them
before serialization. Update ClientMetadata and its current() constructor to
avoid storing raw argv at minimum, and trim or remove exe and cwd if they are
not strictly needed for GET /api/locks/status. Ensure any code that reads or
displays ClientMetadata uses the sanitized fields only.
---
Outside diff comments:
In `@crates/fbuild-serial/src/manager.rs`:
- Around line 667-674: `detach_reader` (and the corresponding writer-release
path) only updates `last_activity_at` and leaves stale entries behind in
`session.client_metadata`, so fully detached clients keep showing up in
`get_port_sessions()` and the map can grow without bound. Update `detach_reader`
and `release_writer` to remove the `client_id` from `client_metadata` once that
client is no longer present in `reader_client_ids` and is not the active
`writer_client_id`, while keeping the existing session lookup via
`resolve_port_key` and `sessions.get_mut` intact.
---
Nitpick comments:
In `@crates/fbuild-cli/src/cli/daemon_cmd.rs`:
- Around line 777-780: Add a unit test coverage for the pure helper
`format_age_seconds` in `daemon_cmd.rs`, since it clamps negative values and
delegates to `format_uptime`. Create a small `#[cfg(test)]` module near
`format_age_seconds` with plain `#[test]` cases that verify a negative input is
clamped to zero and a normal positive value is formatted through `format_uptime`
as expected.
- Around line 363-410: The inner loop variable in the lock-details rendering
block shadows the outer DaemonClient reference, which hurts readability and can
confuse future edits. Rename the loop variable in the section that iterates over
lock.clients to something distinct (for example, client_info or lock_client) and
update the uses within that loop so the outer client binding remains clearly
distinguishable.
In `@crates/fbuild-serial/src/manager/tests.rs`:
- Around line 152-156: The current tests only exercise open_port and
attach_reader with None for client_metadata, so the session.client_metadata
upsert path in those APIs is not directly covered. Add a focused unit test in
manager/tests.rs that calls open_port or attach_reader with
Some(SerialClientMetadata { .. }) and then verifies the stored value through
get_port_sessions(), using the real manager methods rather than writing metadata
directly. Update the affected test cases around open_port and attach_reader so
the new client_metadata behavior is explicitly asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1a32704-1f88-46f2-af57-77ef837380a7
📒 Files selected for processing (18)
crates/fbuild-cli/src/cli/args.rscrates/fbuild-cli/src/cli/daemon_cmd.rscrates/fbuild-cli/src/daemon_client.rscrates/fbuild-cli/src/daemon_client/types.rscrates/fbuild-daemon/src/context.rscrates/fbuild-daemon/src/handlers/locks.rscrates/fbuild-daemon/src/handlers/operations/deploy.rscrates/fbuild-daemon/src/handlers/operations/monitor.rscrates/fbuild-daemon/src/handlers/websockets.rscrates/fbuild-daemon/src/models.rscrates/fbuild-python/src/async_serial_monitor.rscrates/fbuild-python/src/messages.rscrates/fbuild-python/src/serial_monitor.rscrates/fbuild-serial/src/lib.rscrates/fbuild-serial/src/manager.rscrates/fbuild-serial/src/manager/tests.rscrates/fbuild-serial/src/messages.rscrates/fbuild-serial/src/session.rs
| DaemonAction::ClearLocks { | ||
| serial, | ||
| stale, | ||
| port, | ||
| client_id, | ||
| force, | ||
| } => { | ||
| let request = daemon_client::ClearLocksRequest { | ||
| serial, | ||
| stale, | ||
| port, | ||
| client_id, | ||
| force, | ||
| }; | ||
| run_daemon_clear_locks(&client, request).await?; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== daemon_cmd.rs ==\n'
sed -n '90,150p' crates/fbuild-cli/src/cli/daemon_cmd.rs | cat -n
printf '\n== locks.rs ==\n'
sed -n '1,260p' crates/fbuild-daemon/src/handlers/locks.rs | cat -n
printf '\n== ClearLocksRequest references ==\n'
rg -n "ClearLocksRequest|session_matches_request|clear-locks|stale" crates/fbuild-cli crates/fbuild-daemon -SRepository: FastLED/fbuild
Length of output: 28246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locks.rs (clear_locks section) ==\n'
sed -n '238,330p' crates/fbuild-daemon/src/handlers/locks.rs | cat -n
printf '\n== args.rs (ClearLocks action) ==\n'
sed -n '710,750p' crates/fbuild-cli/src/cli/args.rs | cat -n
printf '\n== daemon_client/types.rs (ClearLocksRequest) ==\n'
sed -n '300,335p' crates/fbuild-cli/src/daemon_client/types.rs | cat -n
printf '\n== clear_locks tests in locks.rs ==\n'
sed -n '440,535p' crates/fbuild-daemon/src/handlers/locks.rs | cat -nRepository: FastLED/fbuild
Length of output: 10294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== daemon_client clear_locks methods ==\n'
sed -n '530,565p' crates/fbuild-cli/src/daemon_client.rs | cat -n
printf '\n== CLI docs around clear-locks ==\n'
sed -n '720,745p' crates/fbuild-cli/src/cli/args.rs | cat -n
printf '\n== MCP tool clear_stale_locks ==\n'
sed -n '210,240p' crates/fbuild-cli/src/mcp/tools.rs | cat -nRepository: FastLED/fbuild
Length of output: 4095
Make --stale imply serial cleanup here. ClearLocksRequest only matches stale serial sessions when both serial and stale are set and neither --port nor --client-id is present, so fbuild daemon clear-locks --stale still returns no matching serial sessions found. Set serial: serial || stale at this call site so the flag behaves as advertised.
Proposed fix
let request = daemon_client::ClearLocksRequest {
- serial,
+ serial: serial || stale,
stale,
port,
client_id,
force,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DaemonAction::ClearLocks { | |
| serial, | |
| stale, | |
| port, | |
| client_id, | |
| force, | |
| } => { | |
| let request = daemon_client::ClearLocksRequest { | |
| serial, | |
| stale, | |
| port, | |
| client_id, | |
| force, | |
| }; | |
| run_daemon_clear_locks(&client, request).await?; | |
| } | |
| DaemonAction::ClearLocks { | |
| serial, | |
| stale, | |
| port, | |
| client_id, | |
| force, | |
| } => { | |
| let request = daemon_client::ClearLocksRequest { | |
| serial: serial || stale, | |
| stale, | |
| port, | |
| client_id, | |
| force, | |
| }; | |
| run_daemon_clear_locks(&client, request).await?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-cli/src/cli/daemon_cmd.rs` around lines 114 - 129, Make
`--stale` imply serial cleanup in `DaemonAction::ClearLocks` within
`daemon_cmd.rs`: the current `ClearLocksRequest` construction passes `serial`
and `stale` independently, so `run_daemon_clear_locks` still misses stale serial
sessions when only `--stale` is set. Update the request building so the `serial`
field is derived from `serial || stale` while leaving the rest of the
`ClearLocksRequest` fields unchanged, ensuring `fbuild daemon clear-locks
--stale` reaches the serial-session cleanup path as intended.
| pub async fn run_daemon_clear_locks( | ||
| client: &DaemonClient, | ||
| request: daemon_client::ClearLocksRequest, | ||
| ) -> fbuild_core::Result<()> { | ||
| if !client.health().await { | ||
| output::result("daemon is not running"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let result = client.clear_locks().await?; | ||
| let result = client.clear_locks_with(&request).await?; | ||
| output::result(&result.message); | ||
| if result.cleared_count > 0 { | ||
| output::result(format!("Cleared {} lock(s)", result.cleared_count)); | ||
| } | ||
| if result.cleared_project_count > 0 { | ||
| output::result(format!( | ||
| "Cleared {} project lock(s)", | ||
| result.cleared_project_count | ||
| )); | ||
| } | ||
| if result.cleared_serial_count > 0 { | ||
| output::result(format!( | ||
| "Cleared serial sessions: {}", | ||
| result.cleared_serial_sessions.join(", ") | ||
| )); | ||
| } | ||
| for refusal in result.refused { | ||
| output::warn(refusal); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Refusal reasons are printed twice.
The daemon's message field already embeds joined refused reasons when any exist (e.g. "Cleared {n} lock(s); {refused.join(\"; \")}", per the clear_locks handler shown in context). This function prints result.message first, then loops over result.refused again, printing each reason a second time via output::warn. Users will see the same refusal text twice for the same run.
♻️ Proposed fix (only warn separately; keep message concise)
- let result = client.clear_locks_with(&request).await?;
- output::result(&result.message);
- if result.cleared_count > 0 {
+ let result = client.clear_locks_with(&request).await?;
+ if result.cleared_count > 0 {
+ output::result(&result.message);
output::result(format!("Cleared {} lock(s)", result.cleared_count));
+ } else {
+ output::result(&result.message);
}Alternatively, have the daemon return a message that omits the per-item refusal text and rely solely on the refused list for details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-cli/src/cli/daemon_cmd.rs` around lines 446 - 474, The refusal
reasons are being emitted twice in run_daemon_clear_locks because result.message
already includes the joined refused text and the loop over result.refused warns
each item again. Update the output in run_daemon_clear_locks so refusals are
shown only once: either keep output::result(result.message) and remove the
per-item warning loop, or make the daemon-side clear_locks response message
concise and rely on the refused list for details. Use the run_daemon_clear_locks
and clear_locks handler behavior to keep the formatting consistent.
| #[cfg(windows)] | ||
| { | ||
| const PROCESS_QUERY_LIMITED_INFORMATION: u32 = 0x1000; | ||
| type Handle = *mut std::ffi::c_void; | ||
| #[link(name = "kernel32")] | ||
| extern "system" { | ||
| fn OpenProcess(desired_access: u32, inherit_handle: i32, process_id: u32) -> Handle; | ||
| fn CloseHandle(handle: Handle) -> i32; | ||
| } | ||
| unsafe { | ||
| let h = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); | ||
| if h.is_null() { | ||
| false | ||
| } else { | ||
| CloseHandle(h); | ||
| true | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant function/branches.
ast-grep outline crates/fbuild-daemon/src/handlers/locks.rs --view expanded || true
echo
echo "=== Relevant lines in crates/fbuild-daemon/src/handlers/locks.rs ==="
sed -n '120,180p' crates/fbuild-daemon/src/handlers/locks.rs
echo
echo "=== Callers/usages of is_pid_alive and stale handling ==="
rg -n "is_pid_alive|stale_reason|--stale|session_stale_reason|live_refusal_reason|clear_locks" crates/fbuild-daemon/src/handlers/locks.rsRepository: FastLED/fbuild
Length of output: 5013
🌐 Web query:
OpenProcess GetLastError ERROR_ACCESS_DENIED process exists but access denied documentation
💡 Result:
When the OpenProcess function fails with ERROR_ACCESS_DENIED, it confirms that the target process exists but the caller lacks the necessary security permissions to open a handle with the requested access rights [1][2]. The Windows system determines success by comparing the access rights requested in the dwDesiredAccess parameter against the target process's discretionary access control list (DACL) [3]. If the security descriptor of the target process does not permit the requested access for your security token, the operation will be denied [3][4]. Common causes for receiving ERROR_ACCESS_DENIED include: 1. Insufficient Privileges: You are requesting more access rights than your current security context is granted [3][5]. For example, using PROCESS_ALL_ACCESS often fails because it requests a broad set of permissions, including some that might be restricted [4]. It is considered best practice to request only the specific access rights required for your operation [3][5]. 2. Protected System Processes: Certain critical processes, such as the System process or CSRSS (Client/Server Runtime Subsystem) processes, have internal security restrictions that prevent user-level applications from opening them, regardless of the caller's privileges [2]. 3. Integrity Levels: Windows uses Mandatory Integrity Control. If your process is running at a lower integrity level than the target process, you may be denied access even if you are an administrator [3]. To troubleshoot or mitigate this: - Audit your requested access rights: Try requesting only the minimum rights necessary (e.g., PROCESS_QUERY_LIMITED_INFORMATION or PROCESS_QUERY_INFORMATION) rather than PROCESS_ALL_ACCESS [3][5]. - Use MAXIMUM_ALLOWED: You can request the MAXIMUM_ALLOWED access flag, which instructs the system to grant the highest level of access available to the caller for that object [5]. - Check Privileges: Ensure your process has the necessary privileges enabled (such as SeDebugPrivilege) if you are attempting to interact with system-level processes, although this does not override all security descriptors [4].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocess
- 2: https://github.com/MicrosoftDocs/sdk-api/blob/docs/sdk-api-src/content/processthreadsapi/nf-processthreadsapi-openprocess.md
- 3: https://learn.microsoft.com/en-us/windows/win32/procthread/process-security-and-access-rights
- 4: https://devblogs.microsoft.com/oldnewthing/20151210-00/?p=92451
- 5: https://learn.microsoft.com/en-us/windows/win32/secauthz/requesting-access-rights-to-an-object
Handle ERROR_ACCESS_DENIED on Windows as “alive,” not “dead.”
OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, …) can fail with ERROR_ACCESS_DENIED even when the PID still exists; returning false here makes session_stale_reason treat a live session as stale, so clear_locks --stale can close an active serial session.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-daemon/src/handlers/locks.rs` around lines 141 - 159, The
Windows PID liveness check in the lock handler treats any OpenProcess failure as
dead, which incorrectly marks access-denied processes as stale. Update the logic
in the Windows branch of the PID check in locks handling so that
ERROR_ACCESS_DENIED from OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, ...) is
treated as alive, while only true “missing process” cases return false. Use the
existing Windows FFI symbols OpenProcess, CloseHandle, and the
session_stale_reason flow to keep active serial sessions from being cleared.
| #[derive(Debug, Clone, Serialize)] | ||
| pub(crate) struct ClientMetadata { | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub pid: Option<u32>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub exe: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub cwd: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub argv: Option<Vec<String>>, | ||
| } | ||
|
|
||
| impl ClientMetadata { | ||
| pub(crate) fn current() -> Self { | ||
| Self { | ||
| pid: Some(std::process::id()), | ||
| exe: std::env::current_exe() | ||
| .ok() | ||
| .map(|p| p.to_string_lossy().into_owned()), | ||
| cwd: std::env::current_dir() | ||
| .ok() | ||
| .map(|p| p.to_string_lossy().into_owned()), | ||
| argv: Some(std::env::args().collect()), | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files of interest:\n'
git ls-files crates/fbuild-python/src/messages.rs crates/fbuild-python/src | sed -n '1,120p'
printf '\nOutline for crates/fbuild-python/src/messages.rs:\n'
ast-grep outline crates/fbuild-python/src/messages.rs --view expanded || true
printf '\nSearch for ClientMetadata usage and lock/session exposure:\n'
rg -n "ClientMetadata|lock_status_reports_serial_owner_metadata_and_age|daemon locks|Attach|session state|owner metadata|argv|cwd|exe" crates/fbuild-python -S
printf '\nRead crates/fbuild-python/src/messages.rs:\n'
cat -n crates/fbuild-python/src/messages.rs | sed -n '1,220p'
printf '\nSearch for daemon API / locks rendering:\n'
rg -n "locks|owner metadata|metadata|ClientMetadata|session" crates -SRepository: FastLED/fbuild
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant slices from crates/fbuild-daemon/src/handlers/locks.rs:\n'
sed -n '1,220p' crates/fbuild-daemon/src/handlers/locks.rs | cat -n
printf '\n---\n'
sed -n '220,380p' crates/fbuild-daemon/src/handlers/locks.rs | cat -n
printf '\nSearch for auth/access control around locks endpoints and API routes:\n'
rg -n "lock_status|clear_locks|Authorization|auth|middleware|tower_http|api/locks/status|api/locks/clear" crates/fbuild-daemon/src crates/fbuild-daemon/README.md -S
printf '\nRelevant context from route registration in crates/fbuild-daemon/src/main.rs:\n'
sed -n '120,170p' crates/fbuild-daemon/src/main.rs | cat -nRepository: FastLED/fbuild
Length of output: 22346
Redact lock-status client metadata
ClientMetadata::current() captures full argv, cwd, and exe, and GET /api/locks/status returns them verbatim. That exposes command-line secrets and local path details to any process/user that can reach the daemon. Redact argv at minimum, and consider trimming or omitting path fields before persisting or displaying this metadata.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/fbuild-python/src/messages.rs` around lines 12 - 38,
ClientMetadata::current() is collecting sensitive process details that are later
exposed by the lock-status API, so redact or omit them before serialization.
Update ClientMetadata and its current() constructor to avoid storing raw argv at
minimum, and trim or remove exe and cwd if they are not strictly needed for GET
/api/locks/status. Ensure any code that reads or displays ClientMetadata uses
the sanitized fields only.
Add owner metadata, pending attach details, and targeted serial lock cleanup so fbuild daemon locks and clear-locks can diagnose and recover stale serial sessions without external process inspection. Closes #976
zackees
force-pushed
the
feat/976-serial-lock-recovery
branch
from
3302e9d to
b866495
Compare
Pin FastLED to fbuild 2.4.0 for daemon serial lock diagnostics and recovery commands shipped by FastLED/fbuild#978, with the release unblocked by FastLED/fbuild#979 and #980.
1 participant
Summary
fbuild daemon lockswith serial owner, age, activity, byte-count, pending attach, and client process details.clear-locksserial cleanup flags for stale sessions, port/client-id targeting, and guarded force close.Closes #976
Verification
soldr cargo fmt --all --checkgit diff --checksoldr cargo check --target-dir target-codex-976 -p fbuild-serial --no-default-featuressoldr cargo check --target-dir target-codex-976 -p fbuild-daemon --no-default-featuressoldr cargo check --target-dir target-codex-976 -p fbuild-cli --no-default-featuressoldr cargo check --target-dir target-codex-976 -p fbuild-python --no-default-featuressoldr cargo test --target-dir target-codex-976 -p fbuild-serial --no-default-features messagessoldr cargo test --target-dir target-codex-976 -p fbuild-serial --no-default-features manager::testssoldr cargo test --target-dir target-codex-976 -p fbuild-daemon --no-default-features handlers::locks::testssoldr cargo test --target-dir target-codex-976 -p fbuild-cli --no-default-featuressoldr cargo build --target-dir target-codex-976 -p fbuild-cli --no-default-featuresfbuild daemon clear-locks --helpshows--serial,--stale,--port,--client-id, and--forcefbuild daemon locksexits cleanly withdaemon is not runningwhen no daemon is activeSummary by CodeRabbit
New Features
Bug Fixes