[rqd] Add config option for default NIMBY lock at startup#2247
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a machine-level config Changes
Sequence DiagramsequenceDiagram
participant Config as Configuration File
participant Machine as MachineMonitor
participant Nimby as Nimby
participant State as LockState
Config->>Machine: load nimby_mode & nimby_lock_by_default
Machine->>Machine: initial_nimby_state(config)
alt nimby_mode && nimby_lock_by_default
Machine->>State: initialize as NimbyLocked
Machine->>Machine: lock_all_cores()
else
Machine->>State: initialize as Open
end
Machine->>Nimby: check has_activity_been_recorded()
alt activity recorded && idle
Nimby-->>Machine: activity recorded
Machine->>State: transition to Open when idle
else
Nimby-->>Machine: no activity recorded
Machine->>State: remain NimbyLocked
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
officialasishkumar
requested review from
DiegoTavares,
lithorus and
ramonfigueiredo
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/crates/rqd/src/system/machine.rs`:
- Around line 106-113: The current seeding uses nimby_state for last_lock_state
which allows a default-locked machine (nimby_lock_by_default) to be auto-opened
on inactivity; change the initialization and transition checks so that when
MachineConfig.nimby_lock_by_default is true you (1) initialize last_lock_state
explicitly to LockState::NimbyLocked instead of copying nimby_state, and (2)
update the state-machine transition that currently maps (false,
LockState::NimbyLocked) -> Open to require an explicit unlock (e.g., check
config.nimby_lock_by_default or a new explicit-unlock flag) before moving
NimbyLocked -> Open; reference symbols: initial_nimby_state, last_lock_state,
nimby_state, MachineConfig, and the transition handling that checks activity +
LockState to implement this behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d266d3f-b32f-48af-ae74-daa1cfe0364c
📒 Files selected for processing (5)
rust/config/rqd.dummy.cuebot.yamlrust/config/rqd.local.cuebot.yamlrust/config/rqd.yamlrust/crates/rqd/src/config/mod.rsrust/crates/rqd/src/system/machine.rs
There was a problem hiding this comment.
Pull request overview
Adds a new RQD configuration option to control the initial NIMBY lock state so hosts can report nimby_locked immediately on startup (when NIMBY mode is enabled), and updates sample configs and tests accordingly.
Changes:
- Add
machine.nimby_lock_by_defaulttoMachineConfig(defaultfalse) and document it in shipped YAML configs. - Seed
MachineMonitor’s initialnimby_statefrom config and use it in the first startup report. - Add unit tests for config parsing and initial lock-state selection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/crates/rqd/src/system/machine.rs | Seeds initial NIMBY state from config and uses it in the initial startup host report; adds unit tests for initial state selection. |
| rust/crates/rqd/src/config/mod.rs | Adds nimby_lock_by_default to MachineConfig defaults and adds config load tests. |
| rust/config/rqd.yaml | Documents the new nimby_lock_by_default option in the sample config. |
| rust/config/rqd.local.cuebot.yaml | Adds an example commented setting for nimby_lock_by_default. |
| rust/config/rqd.dummy.cuebot.yaml | Adds an example commented setting for nimby_lock_by_default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Starts an async loop that will update the machine state every `monitor_interval_seconds`. | ||
| pub async fn start(&self, startup_flag: oneshot::Sender<()>) -> Result<()> { | ||
| let report_client = self.report_client.clone(); | ||
| #[allow(unused_assignments)] | ||
| let mut nimby_locked = false; | ||
|
|
||
| #[cfg(feature = "nimby")] | ||
| { | ||
| nimby_locked = *self.nimby_state.read().await == LockState::NimbyLocked; | ||
| } | ||
|
|
||
| let host_state = { | ||
| let system_lock = self.system_manager.lock().await; | ||
| Self::inspect_host_state(&self.maching_config, &system_lock, false)? | ||
| Self::inspect_host_state(&self.maching_config, &system_lock, nimby_locked)? | ||
| }; |
There was a problem hiding this comment.
When nimby_lock_by_default seeds nimby_state as NimbyLocked, the startup path only reports nimby_locked but doesn’t apply the same side effects as a lock transition (locking cores / killing running frames). Because handle_nimby_state_change treats (true, NimbyLocked) as “continues locked”, those side effects will never run for a host that starts in the locked state. Consider applying the lock actions once during startup when the initial state is locked (before sending the startup report), so the daemon’s internal state (core reservations / running frames) is consistent with the reported lock state.
There was a problem hiding this comment.
Valid concern, fixed. The startup path now explicitly calls lock_all_cores() when the initial nimby state is NimbyLocked. This ensures the daemon's internal core reservations are consistent with the reported lock state right from startup, rather than relying on a state transition that would never fire for the "continues locked" case.
| #[allow(unused_assignments)] | ||
| let mut nimby_locked = false; | ||
|
|
||
| #[cfg(feature = "nimby")] | ||
| { | ||
| nimby_locked = *self.nimby_state.read().await == LockState::NimbyLocked; | ||
| } |
There was a problem hiding this comment.
The #[allow(unused_assignments)] + let mut nimby_locked = false; pattern is duplicated (also appears in collect_host_report) and is only needed because the initial assignment is overwritten under the nimby feature. To avoid the lint suppression and reduce duplication, consider defining nimby_locked with #[cfg(feature = "nimby")] / #[cfg(not(feature = "nimby"))] split bindings, or extracting a small helper that returns the current nimby_locked boolean.
| #[allow(unused_assignments)] | |
| let mut nimby_locked = false; | |
| #[cfg(feature = "nimby")] | |
| { | |
| nimby_locked = *self.nimby_state.read().await == LockState::NimbyLocked; | |
| } | |
| #[cfg(feature = "nimby")] | |
| let nimby_locked = *self.nimby_state.read().await == LockState::NimbyLocked; | |
| #[cfg(not(feature = "nimby"))] | |
| let nimby_locked = false; |
There was a problem hiding this comment.
Good suggestion, applied exactly as proposed. Both occurrences (in start() and collect_host_report()) now use paired #[cfg(feature = "nimby")] / #[cfg(not(feature = "nimby"))] bindings, eliminating the #[allow(unused_assignments)] suppression and the mutable rebinding pattern.
The NIMBY state machine had two issues when a host started in NimbyLocked state via the nimby_lock_by_default config option: 1. Immediate auto-unlock: is_user_active() returns false when no user interaction has ever been recorded (epoch=0). The state machine treated this the same as "user became idle", causing the (false, NimbyLocked) -> Open transition to fire on the very first monitor tick. Fix: add Nimby::has_activity_been_recorded() and use a match guard so the unlock transition only fires after real user activity has been observed and then ceased. 2. Missing startup side effects: the startup path reported nimby_locked=true to Cuebot but never called lock_all_cores(), so cores remained bookable despite the locked state. The periodic handle_nimby_state_change only runs side effects on the Open -> NimbyLocked transition, never for "continues locked". Fix: explicitly lock all cores during startup when nimby_locked is true. Also cleaned up the duplicated #[allow(unused_assignments)] pattern for the nimby_locked variable in both start() and collect_host_report(), replacing it with paired #[cfg] / #[cfg(not)] bindings. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
DiegoTavares
left a comment
DiegoTavares
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
DiegoTavares
merged commit a008061
into
AcademySoftwareFoundation:master
Related Issues
Fixes #2195
Summarize your change.
Adds a new
machine.nimby_lock_by_defaultsetting to Rust RQD so a workstation can reportnimby_lockedimmediately at startup when NIMBY mode is enabled. The machine monitor now seeds its initial lock state from config, uses that state in the first startup report, and the shipped sample configs document the new option.It also adds unit tests for config parsing and initial lock-state selection.
Validation
cargo test -p rqd initial_nimby_state -- --nocapturecargo test -p rqd nimby_lock_by_default -- --nocaptureSummary by CodeRabbit
New Features
Chores