fix: Prevent infinite loop in repository configuration on failures#896
fix: Prevent infinite loop in repository configuration on failures#896
Conversation
Fixed critical bug in get_future_results() where calling result.result() before checking for exceptions would crash the entire process when encountering failed repositories (e.g., archived repos). The crash would trigger auto-restart (Docker/systemd), creating an infinite crash-restart loop that prevented any repositories from being configured. Changes: - Check for exceptions using try-except BEFORE calling result.result() - Use logger.exception() for proper traceback capture - Continue processing remaining repositories on individual failures - Improved error message with actionable guidance Fixes: Archon task 3f6a599b-7d4d-4626-9b72-d367c525060e
Walkthroughget_future_results in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
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 (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (7)📓 Common learnings📚 Learning: 2024-10-15T10:37:45.791ZApplied to files:
📚 Learning: 2024-11-26T14:30:22.906ZApplied to files:
📚 Learning: 2024-10-14T14:13:21.316ZApplied to files:
📚 Learning: 2024-10-09T09:16:45.452ZApplied to files:
📚 Learning: 2024-10-29T08:09:57.157ZApplied to files:
📚 Learning: 2024-10-14T14:12:28.924ZApplied to files:
🧬 Code graph analysis (2)webhook_server/tests/test_webhook.py (1)
webhook_server/tests/test_helpers_sanitization.py (1)
🔇 Additional comments (5)
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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
/verified |
![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)
webhook_server/utils/helpers.py (1)
482-496: Critical fix correctly implemented! Exception handling prevents the crash-restart loop.The try/except wrapper around
result.result()properly catches exceptions from failed futures, andlogger.exception()captures full tracebacks. The loop continues processing remaining repositories, which is exactly the desired resilience behavior.Optional: Consider more explicit tuple unpacking for readability.
Current implementation at lines 488-490:
_res = result.result() _log = _res[2] _log(_res[1])Could be slightly clearer as:
success, message, log_func = result.result() log_func(message)This makes the tuple structure more obvious, though the current approach is perfectly fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webhook_server/utils/helpers.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webhook_server/utils/helpers.py (2)
webhook_server/tests/test_github_api.py (1)
logger(74-75)webhook_server/tests/test_helpers.py (6)
result(299-300)result(310-311)result(321-322)exception(302-303)exception(313-314)exception(324-325)
🔇 Additional comments (1)
webhook_server/utils/helpers.py (1)
471-481: LGTM! Clear documentation of exception handling behavior.The expanded docstring effectively communicates the new resilience behavior and provides helpful context about potential failure scenarios.
- Fix DummyFutureException to properly raise exceptions in tests - Add assertions to verify exception handling works correctly - Improve tuple unpacking readability (success, message, logger_func) Addresses CodeRabbit review comments on fix/repository-settings-infinite-loop
myakove-bot
added
size/M
and removed
verified
approved-myakove
commented-coderabbitai[bot]
size/S
labels
|
/verified |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webhook_server/utils/helpers.py (1)
484-494: Exception handling fix is correct, but unused variable can be optimized.The try-except block properly catches exceptions from
result.result()and logs them with full traceback context vialogger.exception(). The loop continues processing remaining futures, which prevents the infinite crash-restart loop described in the PR objectives.However, the
successvariable on line 488 is unpacked but never used. Consider using tuple unpacking with underscore:- success, message, logger_func = result.result() + _success, message, logger_func = result.result()Or if the success value might be needed for future enhancements:
- success, message, logger_func = result.result() + success, message, logger_func = result.result() # noqa: RUF059
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webhook_server/tests/test_helpers.py(1 hunks)webhook_server/utils/helpers.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:2089-2100
Timestamp: 2024-10-29T08:09:57.157Z
Learning: In `webhook_server_container/libs/github_api.py`, when the function `_keep_approved_by_approvers_after_rebase` is called, existing approval labels have already been cleared after pushing new changes, so there's no need to check for existing approvals within this function.
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 612
File: webhook_server_container/libs/github_api.py:925-926
Timestamp: 2024-10-29T10:42:50.163Z
Learning: In `webhook_server_container/libs/github_api.py`, the method `self._keep_approved_by_approvers_after_rebase()` must be called after removing labels when synchronizing a pull request. Therefore, it should be placed outside the `ThreadPoolExecutor` to ensure it runs sequentially after label removal.
Learnt from: myakove
Repo: myk-org/github-webhook-server PR: 586
File: webhook_server_container/libs/github_api.py:1947-1956
Timestamp: 2024-10-08T09:19:56.185Z
Learning: In `webhook_server_container/libs/github_api.py`, the indentation style used in the `set_pull_request_automerge` method is acceptable as per the project's coding standards.
🧬 Code graph analysis (2)
webhook_server/utils/helpers.py (1)
webhook_server/tests/test_helpers.py (6)
result(299-300)result(310-311)result(321-322)exception(302-303)exception(313-314)exception(324-325)
webhook_server/tests/test_helpers.py (1)
webhook_server/utils/helpers.py (1)
get_future_results(471-494)
🪛 Ruff (0.14.3)
webhook_server/utils/helpers.py
488-488: Unpacked variable success is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
webhook_server/tests/test_helpers.py
322-322: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
webhook_server/tests/test_helpers.py (2)
319-328: Excellent fix for the test mock!The
DummyFutureExceptionclass now correctly simulatesconcurrent.futures.Futurebehavior by raising the exception fromresult()instead of returning a tuple. This addresses the previous review concern and ensures the exception handling path inget_future_resultsis properly exercised.
331-348: Test coverage is now comprehensive.The test properly validates the exception handling path by:
- Verifying
logger.exceptionis called with the correct message- Confirming that processing continues for remaining futures (success and fail futures have their messages logged)
- Ensuring the exception future doesn't set a log attribute (as expected when exception is raised)
This fully addresses the concerns raised in the previous review.
webhook_server/utils/helpers.py (2)
471-481: Well-documented behavioral change.The updated docstring clearly explains that the function handles partial failures gracefully and continues processing on exceptions. The notes about worker thread crashes on archived repositories or API permission issues provide helpful context.
482-482: Logger initialization is appropriate.Creating a shared logger for all exception logging is the correct approach, ensuring consistent logging behavior across all repository configuration tasks.
Fixed all 36 RUF059 linting errors by replacing unused variables in tuple unpacking with '_' placeholder. This improves code clarity by explicitly marking values that are intentionally ignored. Changes: - Updated test files to use '_' for unused tuple values - Enabled RUF059 rule in pyproject.toml to prevent future violations - Improved helper functions to discard unused return values - Applied .get() pattern in edge case validation Affected files: - pyproject.toml: Added RUF059 to linting rules - test_helpers_sanitization.py: 2 fixes (stderr unused) - test_runner_handler.py: 20 fixes (rc/out/err unused) - test_webhook.py: 10 fixes (message/log_func unused) - test_edge_cases_validation.py: 2 fixes (dict.get pattern) - helpers.py: 1 fix (success unused) - app_utils.py: 1 fix (str() to \!s conversion)
myakove-bot
removed
verified
approved-myakove
commented-coderabbitai[bot]
size/M
labels
|
/verified |
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
Fixed critical bug in get_future_results() where calling result.result() before checking for exceptions would crash the entire process when encountering failed repositories (e.g., archived repos).
The crash would trigger auto-restart (Docker/systemd), creating an infinite crash-restart loop that prevented any repositories from being configured.
Changes:
Fixes: Archon task 3f6a599b-7d4d-4626-9b72-d367c525060e
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores
Behavior