feat: idle mode #177
feat: idle mode #177
Conversation
WalkthroughAdds adaptive polling to Postman: tracks inactivity via a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CmdSrc as ExternalCommandSource
participant Postman
participant Queue as CommandQueue
participant Timer as KeepAliveTimer
participant Net as Network
Note over Postman,Timer: Postman maintains idle_ticks_count and keep_alive_interval
CmdSrc->>Postman: enqueue command
Postman->>Queue: check for commands
alt commands present
Postman->>Postman: keep_alive_interval = ACTIVE_INTERVAL_SECS (1)\nidle_ticks_count = 0
Postman->>Timer: schedule next tick (1s)
else no commands
Postman->>Postman: idle_ticks_count += 1
alt idle_ticks_count >= IDLE_THRESHOLD_TICKS
Postman->>Postman: keep_alive_interval = IDLE_INTERVAL_SECS (20)\nidle_ticks_count = 0
Postman->>Timer: schedule next tick (20s)
else
Postman->>Timer: schedule next tick (current interval)
end
end
Timer->>Postman: tick
Postman->>Net: send keep-alive / perform update
Net-->>Postman: response
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
smithd/src/postman/mod.rs (1)
90-92: Consider moving constants to module or impl level.The constants are correctly defined, but they're currently function-scoped. Consider moving them to module level or as associated constants of the
Postmanimpl block for better visibility and potential reusability.Example:
+const IDLE_INTERVAL_SECS: u64 = 20; +const ACTIVE_INTERVAL_SECS: u64 = 1; +const IDLE_THRESHOLD_TICKS: u32 = 60; + struct Postman { shutdown: ShutdownSignals, ...Or as associated constants:
impl Postman { + const IDLE_INTERVAL_SECS: u64 = 20; + const ACTIVE_INTERVAL_SECS: u64 = 1; + const IDLE_THRESHOLD_TICKS: u32 = 60; + fn new(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smithd/src/postman/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
smithd/src/postman/mod.rs (1)
smithd/src/commander/mod.rs (1)
execute_api_batch(246-253)
🔇 Additional comments (3)
smithd/src/postman/mod.rs (3)
28-28: LGTM!The field addition is appropriate for tracking consecutive idle ticks.
54-54: LGTM!Proper initialization of the idle tick counter.
94-94: LGTM!Proper initialization using the idle interval constant.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
smithd/src/postman/mod.rs (1)
128-139: Optional: Consider incrementing counter only in active mode.While the current implementation is correct, you could optimize by only incrementing
idle_ticks_countwhen in active mode, avoiding unnecessary counting during idle periods:} else { - self.idle_ticks_count += 1; - if self.idle_ticks_count >= IDLE_THRESHOLD_TICKS { - let current_period = keep_alive_interval.period(); - if current_period.as_secs() != IDLE_INTERVAL_SECS { - info!("Switching to idle polling mode (20 second interval)"); - keep_alive_interval = time::interval(Duration::from_secs(IDLE_INTERVAL_SECS)); - keep_alive_interval.reset(); - } - self.idle_ticks_count = 0; + let current_period = keep_alive_interval.period(); + if current_period.as_secs() == ACTIVE_INTERVAL_SECS { + self.idle_ticks_count += 1; + if self.idle_ticks_count >= IDLE_THRESHOLD_TICKS { + info!("Switching to idle polling mode (20 second interval)"); + keep_alive_interval = time::interval(Duration::from_secs(IDLE_INTERVAL_SECS)); + keep_alive_interval.reset(); + self.idle_ticks_count = 0; + } } }This eliminates the counter cycling 0→59 every 20 minutes while already idle. However, the current explicit approach is also acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smithd/src/postman/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
smithd/src/postman/mod.rs (1)
smithd/src/commander/mod.rs (1)
execute_api_batch(246-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
smithd/src/postman/mod.rs (5)
90-92: LGTM! Constants align with PR objectives.The constants correctly implement the intended behavior: 20-second idle polling, 1-second active polling, and 60 ticks (= 60 seconds at active rate) before returning to idle.
28-28: Appropriate field addition.The
idle_ticks_count: u32field correctly tracks consecutive ticks without commands, with sufficient range for the counter.
94-94: Correct initial state.Starting with
IDLE_INTERVAL_SECSproperly implements the "Default Idle Mode" requirement from the PR objectives.
117-127: Active mode switching implemented correctly.The logic properly:
- Resets the idle counter when commands arrive
- Checks if already in active mode before switching (avoiding unnecessary interval recreation)
- Uses
reset()to start the new interval fresh- Logs the transition for observability
128-139: Idle mode logic is correct and addresses previous review concern.The implementation properly handles the transition to idle mode:
- Increments counter when no commands arrive
- When threshold is reached, checks if already in idle mode before switching
- Importantly, line 137 resets
idle_ticks_count = 0unconditionally (outside the inner conditional), which fixes the unbounded counter growth issue flagged in the previous reviewThe behavior is correct: in active mode, 60 ticks = 60 seconds of inactivity triggers the switch to idle, as intended.
Co-authored-by: Søren Hansen <soerenhh@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
smithd/src/postman/mod.rs (2)
90-95: LGTM! Interval setup is correct.The constants clearly define the polling behavior, and starting in idle mode (20s interval) with
MissedTickBehavior::Skipaligns with the PR objectives and prevents API spam if the system is slow.💡 Optional: Consider module-level constants for better organization
Moving the constants to module level (above the
Postmanstruct) would improve organization and make them easier to tune:+const IDLE_INTERVAL_SECS: u64 = 20; +const ACTIVE_INTERVAL_SECS: u64 = 1; +const IDLE_THRESHOLD_TICKS: u32 = 60; + struct Postman { ...Then remove the
constdeclarations insiderun().
118-140: Logic is correct and handles mode transitions properly.The implementation correctly:
- Switches from idle to active immediately when commands arrive
- Switches from active to idle after 60 seconds of inactivity
- Resets
idle_ticks_countunconditionally at line 138 (fixing the unbounded growth issue from past reviews)💡 Optional: Eliminate unnecessary counter increments in idle mode
Currently,
idle_ticks_countincrements even when already in idle mode, wasting CPU cycles every 20 seconds. Consider only incrementing the counter when in active mode:} else { - self.idle_ticks_count += 1; - if self.idle_ticks_count >= IDLE_THRESHOLD_TICKS { - let current_period = keep_alive_interval.period(); - if current_period.as_secs() != IDLE_INTERVAL_SECS { - info!("Switching to idle polling mode (20 second interval)"); - keep_alive_interval = time::interval(Duration::from_secs(IDLE_INTERVAL_SECS)); - keep_alive_interval.reset(); - } - self.idle_ticks_count = 0; + let current_period = keep_alive_interval.period(); + if current_period.as_secs() == ACTIVE_INTERVAL_SECS { + self.idle_ticks_count += 1; + if self.idle_ticks_count >= IDLE_THRESHOLD_TICKS { + info!("Switching to idle polling mode (20 second interval)"); + keep_alive_interval = time::interval(Duration::from_secs(IDLE_INTERVAL_SECS)); + keep_alive_interval.reset(); + self.idle_ticks_count = 0; + } } }This avoids counting and resetting every 60 ticks (20 minutes) when already idle.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
smithd/src/postman/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
smithd/src/postman/mod.rs (1)
smithd/src/commander/mod.rs (1)
execute_api_batch(246-253)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
smithd/src/postman/mod.rs (2)
28-28: LGTM!The
idle_ticks_countfield is appropriately typed asu32and integrates cleanly into the struct.
54-54: LGTM!Initialization to
0is correct for tracking idle ticks from the start.
|
|
||
| let mut keep_alive_interval = time::interval(Duration::from_secs(20)); | ||
| const IDLE_INTERVAL_SECS: u64 = 20; | ||
| const ACTIVE_INTERVAL_SECS: u64 = 1; |
There was a problem hiding this comment.
This could potentially bombard the api when you multiply across all the devices. Think we should be pretty careful here. What is the reason for this change?
There was a problem hiding this comment.
Perhaps we should reconsider our polling setup altogether if this is a requirement. Why not just use websockets at this point, or a broker of some sort if we want to deal with that
2 participants
Idle Mode (20 seconds):
Active Mode (1 second):
Back to Idle: