Fix #6586: Reset retries and set UP status for successful push heartb… #6647
Conversation
…h heartbeats - Reset retries counter to 0 when push heartbeat is received - Set monitor status to UP and update message - Ensure proper state management for push-type monitors
|
@CommanderStorm Could you plz review my PR? |
CommanderStorm
left a comment
CommanderStorm
left a comment
There was a problem hiding this comment.
I would expect this to be a fix in the following file instead
uptime-kuma/server/routers/api-router.js
Line 47 in 0c35ce1
Since this is resonably convoluted code, could you try to add a testcase for this?
CommanderStorm
added
the
pr:please address review comments
…es in push API - Revert incorrect fix from server/model/monitor.js - Fix determineStatus function in server/routers/api-router.js to properly handle PENDING + maxretries exceeded case - When previousHeartbeat.status === PENDING, status === DOWN, and retries >= maxretries, set status to DOWN and reset retries to 0 instead of incrementing retries - Add comprehensive test suite to verify the fix and prevent regression - This ensures push monitors properly reset retries when recovering from failure states
| * @param isUpsideDown | ||
| * @param bean | ||
| */ | ||
| function determineStatus(status, previousHeartbeat, maxretries, isUpsideDown, bean) { |
There was a problem hiding this comment.
you need to import functions to use them
|
@CommanderStorm I've reverted incorrect fix from server/model/monitor.js and applied correct fix to server/routers/api-router.js: |
- Simplify test structure following backend-test README guidelines - Use descriptive test names with function() behavior format - Remove redundant test cases and focus on core issue scenarios - Use assert.strictEqual instead of assert.equal for consistency - Remove unnecessary comments and duplicate flipStatus function
tellorian
force-pushed
the
fix/issue-6586-push-monitor-retries
branch
from
f4811d9 to
645d313
Compare
|
@CommanderStorm I've cleanned up test file following best practices |
There was a problem hiding this comment.
as said: please rename the test test/backend-test/test-push-api.js and import determineStatus. Currently, this is not really a test of what you did.
Also, please see the warnings in the files changed tab
- Extract determineStatus function from api-router.js to separate module (server/determine-status.js) - Rename test-push-api-comprehensive.js to test-determineStatus.js for clarity - Update test to import determineStatus from dedicated module instead of defining duplicate - Remove unused imports (flipStatus, MAINTENANCE) from test file - Fix linting issues and ensure all tests pass This change makes the determineStatus function properly testable and modular, eliminating code duplication between the test and implementation.
|
@CommanderStorm I've made a new commit, could you please review it? |
There was a problem hiding this comment.
please explain why you did not just import from the backend
There was a problem hiding this comment.
By creating server/determine-status.js:
✅ Clean import: const { determineStatus } = require("../../server/determine-status")
✅ No server overhead: Just the function and its minimal dependencies
✅ Proper unit test: Tests only the function logic
✅ Reusable: Function can now be imported anywhere without pulling in router code
There was a problem hiding this comment.
What I was asking for was a straightforward, technical explanation of the decision in the context of this codebase. The previous response listed benefits, but it didn’t clearly explain what concrete issue exists with importing from the backend here, or why this change is necessary rather than just stylistic.
Clean import
And that is not possible via the api-server?
No server overhead
What are you talking about?
Proper unit test: Tests only the function logic
And why do we need to move the function for this?
Reusable
Where would you like to use it and why?
2 participants
ℹ️ To keep reviews fast and effective, please make sure you’ve read our pull request guidelines
📝 Summary of changes done and why they are done
📋 Related issues
📄 Checklist
Please follow this checklist to avoid unnecessary back and forth (click to expand)
I understand that I am responsible for and able to explain every line of code I submit.
📷 Screenshots or Visual Changes
Contribution by Gittensor, see my contribution statistics at https://gittensor.io/miners/details?githubId=170527342