src: clamp idle_diff before computing proc_diff #372
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds defensive clamping in event-loop metric calculations: clamp Changes
Sequence Diagram(s)sequenceDiagram
participant UV as uv_metrics_cb_
participant Stats as get_event_loop_stats_
participant State as loop vars/times
UV->>State: read loop_idle_time, loop_duration
alt loop_idle_time > loop_duration
UV->>State: loop_idle_time = loop_duration
Note right of State: clamp to prevent unsigned underflow
end
Stats->>State: read loop_diff, idle_diff
alt idle_diff > loop_diff
Stats->>State: idle_diff = loop_diff
Note right of State: clamp to avoid bogus utilization
end
Stats->>State: compute proc_diff and utilization metrics
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
RafaelGSS
approved these changes
Sep 29, 2025
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nsolid/nsolid_api.cc(2 hunks)
🔇 Additional comments (1)
src/nsolid/nsolid_api.cc (1)
2165-2171: LGTM! Clamping prevents unsigned underflow.The clamping logic correctly handles the clock skew issue where
idle_diffcan be slightly larger thanloop_diffon some platforms. This prevents unsigned underflow in theproc_diffcalculation on line 2171 and eliminates the bogus utilization values reported in issue #15.
src/nsolid/nsolid_api.cc
Outdated
Comment on lines
2202
to
2219
| // Check current_hrtime_ >= exit_time to avoid unsigned underflow when | ||
| // exit_time is slightly larger due to clock skew (observed on Windows). | ||
| // Without this check, the subtraction can produce bogus values in the | ||
| // millions of seconds range. | ||
| fprintf(stderr, "[DEBUG] exit_time=%llu, prev_call_time=%llu, current_hrtime=%llu, rolling_est_lag=%f\n", | ||
| (unsigned long long)exit_time, | ||
| (unsigned long long)stor->prev_call_time_, | ||
| (unsigned long long)stor->current_hrtime_, | ||
| envinst->rolling_est_lag_); | ||
| stor->loop_estimated_lag = | ||
| (exit_time > 0 && stor->prev_call_time_ > exit_time) ? | ||
| (exit_time > 0 && stor->prev_call_time_ > exit_time && | ||
| stor->current_hrtime_ >= exit_time) ? | ||
| (stor->current_hrtime_ - exit_time) / 1e6 : envinst->rolling_est_lag_; | ||
| fprintf(stderr, "[DEBUG] loop_estimated_lag=%f (condition: exit_time>0=%d, prev>exit=%d, current>=exit=%d)\n", | ||
| stor->loop_estimated_lag, | ||
| exit_time > 0, | ||
| stor->prev_call_time_ > exit_time, | ||
| stor->current_hrtime_ >= exit_time); |
There was a problem hiding this comment.
Remove debug logging before merging.
The additional guard stor->current_hrtime_ >= exit_time correctly prevents unsigned underflow, but the debug fprintf statements on lines 2206-2210 and 2215-2219 must be removed before merging to production:
- These debug logs will pollute stderr in production environments
- Performance concern: metrics collection runs frequently (every event loop iteration)
- Portability:
%llumay not be correct foruint64_ton all platforms (considerPRIu64from<cinttypes>)
Apply this diff to remove the debug logging:
- fprintf(stderr, "[DEBUG] exit_time=%llu, prev_call_time=%llu, current_hrtime=%llu, rolling_est_lag=%f\n",
- (unsigned long long)exit_time,
- (unsigned long long)stor->prev_call_time_,
- (unsigned long long)stor->current_hrtime_,
- envinst->rolling_est_lag_);
stor->loop_estimated_lag =
(exit_time > 0 && stor->prev_call_time_ > exit_time &&
stor->current_hrtime_ >= exit_time) ?
(stor->current_hrtime_ - exit_time) / 1e6 : envinst->rolling_est_lag_;
- fprintf(stderr, "[DEBUG] loop_estimated_lag=%f (condition: exit_time>0=%d, prev>exit=%d, current>=exit=%d)\n",
- stor->loop_estimated_lag,
- exit_time > 0,
- stor->prev_call_time_ > exit_time,
- stor->current_hrtime_ >= exit_time);
stor->loop_idle_percent = (1 - stor->loop_utilization) * 100;📝 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.
Suggested change
| // Check current_hrtime_ >= exit_time to avoid unsigned underflow when | |
| // exit_time is slightly larger due to clock skew (observed on Windows). | |
| // Without this check, the subtraction can produce bogus values in the | |
| // millions of seconds range. | |
| fprintf(stderr, "[DEBUG] exit_time=%llu, prev_call_time=%llu, current_hrtime=%llu, rolling_est_lag=%f\n", | |
| (unsigned long long)exit_time, | |
| (unsigned long long)stor->prev_call_time_, | |
| (unsigned long long)stor->current_hrtime_, | |
| envinst->rolling_est_lag_); | |
| stor->loop_estimated_lag = | |
| (exit_time > 0 && stor->prev_call_time_ > exit_time) ? | |
| (exit_time > 0 && stor->prev_call_time_ > exit_time && | |
| stor->current_hrtime_ >= exit_time) ? | |
| (stor->current_hrtime_ - exit_time) / 1e6 : envinst->rolling_est_lag_; | |
| fprintf(stderr, "[DEBUG] loop_estimated_lag=%f (condition: exit_time>0=%d, prev>exit=%d, current>=exit=%d)\n", | |
| stor->loop_estimated_lag, | |
| exit_time > 0, | |
| stor->prev_call_time_ > exit_time, | |
| stor->current_hrtime_ >= exit_time); | |
| // Check current_hrtime_ >= exit_time to avoid unsigned underflow when | |
| // exit_time is slightly larger due to clock skew (observed on Windows). | |
| // Without this check, the subtraction can produce bogus values in the | |
| // millions of seconds range. | |
| stor->loop_estimated_lag = | |
| (exit_time > 0 && stor->prev_call_time_ > exit_time && | |
| stor->current_hrtime_ >= exit_time) ? | |
| (stor->current_hrtime_ - exit_time) / 1e6 : envinst->rolling_est_lag_; | |
| stor->loop_idle_percent = (1 - stor->loop_utilization) * 100; |
🤖 Prompt for AI Agents
In src/nsolid/nsolid_api.cc around lines 2202 to 2219, remove the two debug
fprintf blocks (the one printing
exit_time/prev_call_time/current_hrtime/rolling_est_lag and the one printing
loop_estimated_lag and the condition flags) so they don't pollute stderr or
impact performance; keep the added guard (stor->current_hrtime_ >= exit_time)
and the loop_estimated_lag computation unchanged, and if any future logging is
needed use a proper logging macro or PRIu64 via <cinttypes> rather than raw
fprintf with %llu.
santigimeno
force-pushed
the
santi/clamp_loop_delay
branch
from
5666509 to
890194b
Compare
Clamp idle time values before subtracting from loop duration to avoid unsigned underflow when clock skew causes idle time to be slightly larger than total loop time. This manifests on Windows when the loop is nearly 100% idle. Applied in two locations: - uv_metrics_cb_: prevents bogus rolling_est_lag_ values - get_event_loop_stats_: prevents bogus loop_utilization values Fixes: nodesource/nsolid-private#15
santigimeno
force-pushed
the
santi/clamp_loop_delay
branch
from
890194b to
a4e371e
Compare
EHortua
approved these changes
Oct 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Avoid unsigned underflow when loop_diff is slightly larger than idle_diff due to clock skew on idle loops.
Fixes: https://github.com/nodesource/nsolid-private/issues/15
Summary by CodeRabbit