Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
No pull request description was provided, so its relevance to the changeset cannot be assessed. | Add a concise description of the update failure backoff and entrypoint permission checks. |
-
Check name Status Explanation Title check The title clearly summarises the two main changes: update failure backoff and entrypoint permission checks. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
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 @coderabbitai help to get the list of available commands.
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
613eeb97-9166-4a0a-8fbb-1519e0be2476
📒 Files selected for processing (8)
README.mddocker-compose.ymlentrypoint.shrenovate.jsonsrc/index.pysrc/process_manager.pytests/test_index.pytests/test_process_manager.py
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
| def _record_attempt(self, outcome: UpdateOutcome): | ||
| failed = outcome is UpdateOutcome.FAILED | ||
| if outcome is UpdateOutcome.UPDATED and index.age_seconds() >= config.parse_interval(config.UPDATE_INTERVAL): | ||
| logger.error("Index timestamp did not advance after the update, treating the attempt as failed") | ||
| failed = True | ||
|
|
||
| self._update_failures = self._update_failures + 1 if failed else 0 | ||
| self._last_attempt_at = time.time() | ||
| index.record_update_attempt(self._update_failures) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '295,355p' src/process_manager.py
sed -n '1,115p' src/index.py
rg -n 'def age_seconds|_maybe_update|_record_attempt|POLL_BACKOFF_SECONDS' src/process_manager.py src/index.py tests/test_process_manager.pyRepository: rtuszik/photon-docker
Length of output: 8610
🏁 Script executed:
sed -n '1,80p' src/process_manager.py
sed -n '230,345p' src/process_manager.py
sed -n '455,635p' tests/test_process_manager.py
git diff --no-ext-diff --unified=30 52856fcbc2c1a1a1126270c5a8ccce22b648ac91 726666664a8d2f5b43de9b9a180ce8aa8fccc0af -- src/process_manager.py src/index.py tests/test_process_manager.pyRepository: rtuszik/photon-docker
Length of output: 42026
🏁 Script executed:
printf '%s\n' '--- process manager ---'
sed -n '1,80p' src/process_manager.py
sed -n '230,345p' src/process_manager.py
printf '%s\n' '--- tests ---'
sed -n '455,635p' tests/test_process_manager.py
printf '%s\n' '--- diff ---'
git diff --no-ext-diff --unified=30 52856fcbc2c1a1a1126270c5a8ccce22b648ac91 726666664a8d2f5b43de9b9a180ce8aa8fccc0af -- src/process_manager.py src/index.py tests/test_process_manager.pyRepository: rtuszik/photon-docker
Length of output: 42276
Record an UPDATED attempt when index age cannot be checked.
index.age_seconds() can raise OSError after run_update() returns UPDATED. _maybe_update catches the exception only after _record_attempt exits, so the attempt state remains unchanged. If the update remains due, the next poll can retry without the intended backoff.
Treat an unverified age as not stale, then record the UPDATED attempt.
Suggested fix
--- "a/src/process_manager.py"
+++ "b/src/process_manager.py"
@@ -323,11 +323,17 @@
except Exception:
logger.exception("Scheduled update check failed")
def _record_attempt(self, outcome: UpdateOutcome):
failed = outcome is UpdateOutcome.FAILED
- if outcome is UpdateOutcome.UPDATED and index.age_seconds() >= config.parse_interval(config.UPDATE_INTERVAL):
- logger.error("Index timestamp did not advance after the update, treating the attempt as failed")
- failed = True
+ if outcome is UpdateOutcome.UPDATED:
+ try:
+ stale = index.age_seconds() >= config.parse_interval(config.UPDATE_INTERVAL)
+ except OSError as e:
+ logger.warning(f"Could not verify index age after update: {e}")
+ stale = False
+ if stale:
+ logger.error("Index timestamp did not advance after the update, treating the attempt as failed")
+ failed = True
self._update_failures = self._update_failures + 1 if failed else 0
self._last_attempt_at = time.time()📝 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.
| def _record_attempt(self, outcome: UpdateOutcome): | |
| failed = outcome is UpdateOutcome.FAILED | |
| if outcome is UpdateOutcome.UPDATED and index.age_seconds() >= config.parse_interval(config.UPDATE_INTERVAL): | |
| logger.error("Index timestamp did not advance after the update, treating the attempt as failed") | |
| failed = True | |
| self._update_failures = self._update_failures + 1 if failed else 0 | |
| self._last_attempt_at = time.time() | |
| index.record_update_attempt(self._update_failures) | |
| def _record_attempt(self, outcome: UpdateOutcome): | |
| failed = outcome is UpdateOutcome.FAILED | |
| if outcome is UpdateOutcome.UPDATED: | |
| try: | |
| stale = index.age_seconds() >= config.parse_interval(config.UPDATE_INTERVAL) | |
| except OSError as e: | |
| logger.warning(f"Could not verify index age after update: {e}") | |
| stale = False | |
| if stale: | |
| logger.error("Index timestamp did not advance after the update, treating the attempt as failed") | |
| failed = True | |
| self._update_failures = self._update_failures + 1 if failed else 0 | |
| self._last_attempt_at = time.time() | |
| index.record_update_attempt(self._update_failures) |
1 participant
No description provided.