[codex] Fix finalization deadlock and surface stuck-head context - #1666
Open
MuncleUscles wants to merge 10 commits into
Open
[codex] Fix finalization deadlock and surface stuck-head context#1666MuncleUscles wants to merge 10 commits into
MuncleUscles wants to merge 10 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds explicit DockerHub/release secret declarations and action pinning in workflows; implements CTE-based stuck-head detection with per-transaction details in consensus health, surfaces those rows in cached health payloads, integrates orphaned/stuck events into usage metrics, adjusts health degradation logic, and expands DB/unit tests including finalization-starvation coverage. ChangesCI/Workflow Security Hardening
Consensus Health: Stuck-Head Detection and Metrics Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
MuncleUscles
changed the title
MuncleUscles
marked this pull request as ready for review
June 8, 2026 21:25
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/protocol_rpc/health.py (1)
371-374:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTop-level
/healthorphaned gate is still hard-coded to3.
_check_consensus_healthnow usesHEALTH_DEGRADED_AT_STUCK_HEADS, but_run_health_checksstill applies>= 3. Custom thresholds will produce inconsistent service status/issue tags.Suggested fix
- if consensus_health.get("total_orphaned_transactions", 0) >= 3: + if consensus_status == "degraded" or consensus_health.get( + "total_orphaned_transactions", 0 + ) >= int(os.environ.get("HEALTH_DEGRADED_AT_STUCK_HEADS", "3")):Also applies to: 631-632
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/protocol_rpc/health.py` around lines 371 - 374, The top-level /health check in _run_health_checks is using a hard-coded threshold (>= 3) for consensus_health["total_orphaned_transactions"]; replace that literal with the centralized constant HEALTH_DEGRADED_AT_STUCK_HEADS (the same constant used by _check_consensus_health) so custom thresholds are honored; update both occurrences (the block referencing consensus_health.get("total_orphaned_transactions", 0) at the shown location and the similar check around lines 631-632) and ensure the constant is imported/available in this module and used for the comparison and any related status/issue tagging.
🧹 Nitpick comments (1)
.github/workflows/frontend-unit-tests.yml (1)
35-35: 💤 Low valueConsider adding a comment explaining the non-blocking rationale.
Setting
fail_ci_if_error: falseimproves operational resilience by preventing Codecov service outages from blocking CI. An inline comment would help future maintainers understand this trade-off.📝 Suggested documentation
- fail_ci_if_error: false + fail_ci_if_error: false # Don't block CI on Codecov service issues🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/frontend-unit-tests.yml at line 35, Add an inline comment next to the YAML key fail_ci_if_error: false explaining the non-blocking rationale (e.g., to avoid blocking CI during Codecov outages and prefer passing CI while still reporting coverage issues), so future maintainers understand the trade-off and where to change behavior if desired; reference the fail_ci_if_error setting and keep the comment concise and concrete about why it's set to false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/protocol_rpc/health.py`:
- Line 661: STUCK_HEAD_EVENT_LIMIT is currently read from env and can be set to
0 which disables the SQL LIMIT and causes stuck_head_contracts to be 0 even when
issues exist; change the logic that derives/uses event_limit so it never becomes
0 when you intend to count events—either coerce STUCK_HEAD_EVENT_LIMIT to at
least 1 (e.g., event_limit = max(1, STUCK_HEAD_EVENT_LIMIT)) or treat 0 as
meaning "no-limit" and adjust the query-building so it omits a LIMIT clause
rather than injecting LIMIT 0; update all places that use event_limit (the env
constant STUCK_HEAD_EVENT_LIMIT and the code paths that build the stuck-head
query and compute stuck_head_contracts) to follow the chosen behavior.
In `@tests/db-sqlalchemy/test_finalization_starvation.py`:
- Line 21: Update the ambiguous multiplication character in the docstring: find
the string containing "N×finality_window" (in
tests/db-sqlalchemy/test_finalization_starvation.py) and replace the Unicode
multiplication sign "×" with a plain ASCII "x" so it reads "N x finality_window"
to satisfy the linter (RUF002).
In `@tests/unit/test_rpc_health_genvm_tracking.py`:
- Around line 188-200: The helper _install_fake_consensus_db lacks type
annotations; update its signature to add types for all parameters (e.g.,
monkeypatch: pytest.MonkeyPatch or MonkeyPatch, recovery_storm_count: int = 0,
max_recovery_count: int = 0, exhausted_rows: Optional[int] = None,
backlog_count: int = 0, oldest_created_at: Optional[datetime] = None,
oldest_backlog_age_seconds: Optional[int] = None, progress_epoch: int = 1,
progress_error: bool = False) and add the return annotation -> None; also add
any needed imports (from typing import Optional and from datetime import
datetime or appropriate types) at the top of the test file so the new
annotations resolve.
In `@tests/unit/test_usage_metrics_service.py`:
- Line 56: Add the missing return type hint to the async test function by
changing the signature of async def
test_system_health_metrics_include_stuck_head_events() to include -> None (i.e.,
async def test_system_health_metrics_include_stuck_head_events() -> None:) so it
conforms to the repository typing rule for Python functions.
---
Outside diff comments:
In `@backend/protocol_rpc/health.py`:
- Around line 371-374: The top-level /health check in _run_health_checks is
using a hard-coded threshold (>= 3) for
consensus_health["total_orphaned_transactions"]; replace that literal with the
centralized constant HEALTH_DEGRADED_AT_STUCK_HEADS (the same constant used by
_check_consensus_health) so custom thresholds are honored; update both
occurrences (the block referencing
consensus_health.get("total_orphaned_transactions", 0) at the shown location and
the similar check around lines 631-632) and ensure the constant is
imported/available in this module and used for the comparison and any related
status/issue tagging.
---
Nitpick comments:
In @.github/workflows/frontend-unit-tests.yml:
- Line 35: Add an inline comment next to the YAML key fail_ci_if_error: false
explaining the non-blocking rationale (e.g., to avoid blocking CI during Codecov
outages and prefer passing CI while still reporting coverage issues), so future
maintainers understand the trade-off and where to change behavior if desired;
reference the fail_ci_if_error setting and keep the comment concise and concrete
about why it's set to false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ffce8bc-5dec-4982-9bbc-116d39411b6c
📒 Files selected for processing (11)
.github/workflows/docker-build-and-push-all.yml.github/workflows/frontend-unit-tests.yml.github/workflows/manual-docker-release.yml.github/workflows/release-from-tag.ymlbackend/consensus/worker.pybackend/protocol_rpc/health.pybackend/services/usage_metrics_service.pytests/db-sqlalchemy/test_finalization_starvation.pytests/db-sqlalchemy/test_health_orphan_detection.pytests/unit/test_rpc_health_genvm_tracking.pytests/unit/test_usage_metrics_service.py
| PRE_CONSENSUS_BACKLOG_STATUSES_SQL = ( | ||
| "'PENDING','ACTIVATED','PROPOSING','COMMITTING','REVEALING'" | ||
| ) | ||
| STUCK_HEAD_EVENT_LIMIT = int(os.environ.get("HEALTH_STUCK_HEAD_EVENT_LIMIT", "10")) |
Contributor
There was a problem hiding this comment.
Guard HEALTH_STUCK_HEAD_EVENT_LIMIT from disabling stuck-head counting.
With event_limit=0, LIMIT 0 yields no rows, so stuck_head_contracts becomes 0 even when stuck heads exist. That can mask real degradation.
Suggested fix
- STUCK_HEAD_EVENT_LIMIT = int(os.environ.get("HEALTH_STUCK_HEAD_EVENT_LIMIT", "10"))
+ STUCK_HEAD_EVENT_LIMIT = max(
+ 1, int(os.environ.get("HEALTH_STUCK_HEAD_EVENT_LIMIT", "10"))
+ )Also applies to: 705-755
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/protocol_rpc/health.py` at line 661, STUCK_HEAD_EVENT_LIMIT is
currently read from env and can be set to 0 which disables the SQL LIMIT and
causes stuck_head_contracts to be 0 even when issues exist; change the logic
that derives/uses event_limit so it never becomes 0 when you intend to count
events—either coerce STUCK_HEAD_EVENT_LIMIT to at least 1 (e.g., event_limit =
max(1, STUCK_HEAD_EVENT_LIMIT)) or treat 0 as meaning "no-limit" and adjust the
query-building so it omits a LIMIT clause rather than injecting LIMIT 0; update
all places that use event_limit (the env constant STUCK_HEAD_EVENT_LIMIT and the
code paths that build the stuck-head query and compute stuck_head_contracts) to
follow the chosen behavior.
| eligible statuses (ACCEPTED/UNDETERMINED/*_TIMEOUT). | ||
| 4. Safety-net log fires when an ACCEPTED tx's wait exceeds N×finality_window. | ||
| 5. Recovered PENDING txs are deprioritized across contracts so other | ||
| 5. Safety-net log fires when an ACCEPTED tx's wait exceeds N×finality_window. |
Contributor
There was a problem hiding this comment.
Replace ambiguous multiplication character in the docstring.
N×finality_window triggers Ruff RUF002; use plain x to avoid lint churn.
Suggested fix
-5. Safety-net log fires when an ACCEPTED tx's wait exceeds N×finality_window.
+5. Safety-net log fires when an ACCEPTED tx's wait exceeds Nxfinality_window.📝 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
| 5. Safety-net log fires when an ACCEPTED tx's wait exceeds N×finality_window. | |
| 5. Safety-net log fires when an ACCEPTED tx's wait exceeds Nxfinality_window. |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 21-21: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/db-sqlalchemy/test_finalization_starvation.py` at line 21, Update the
ambiguous multiplication character in the docstring: find the string containing
"N×finality_window" (in tests/db-sqlalchemy/test_finalization_starvation.py) and
replace the Unicode multiplication sign "×" with a plain ASCII "x" so it reads
"N x finality_window" to satisfy the linter (RUF002).
Source: Linters/SAST tools
Comment on lines
+188
to
+200
| def _install_fake_consensus_db( | ||
| self, | ||
| monkeypatch, | ||
| *, | ||
| recovery_storm_count=0, | ||
| max_recovery_count=0, | ||
| exhausted_rows=None, | ||
| backlog_count=0, | ||
| oldest_created_at=None, | ||
| oldest_backlog_age_seconds=None, | ||
| progress_epoch=1, | ||
| progress_error=False, | ||
| ): |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unit/test_rpc_health_genvm_tracking.py"
# Basic info
echo "== File =="
ls -l "$FILE"
echo
# Show the helper signature area (around provided line range)
echo "== Helper signature (lines 160-240) =="
nl -ba "$FILE" | sed -n '160,240p'
# Look for other helper signatures and typing patterns in this file
echo
echo "== Other function defs with/without type hints =="
rg -n "def _install_fake_consensus_db|def " "$FILE"
# Specifically check for type comments/annotations patterns within this file
echo
echo "== Search for '->' return annotations and ':' parameter annotations =="
rg -n "->|:\s*(pytest|int|bool|list|dict|str|float|None|Optional|\| None)" "$FILE" || trueRepository: genlayerlabs/genlayer-studio
Length of output: 280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unit/test_rpc_health_genvm_tracking.py"
echo "== File =="
ls -l "$FILE"
echo
echo "== Helper signature area (lines 160-240) =="
cat -n "$FILE" | sed -n '160,240p'
echo
echo "== Function defs in this file =="
rg -n "^\s*def\s" "$FILE"
echo
echo "== Return/param annotation presence =="
rg -n "->|:\s*(int|bool|str|float|dict|list|tuple|set|Optional|pytest\.MonkeyPatch|None|object|\| None)" "$FILE" || true
echo
echo "== Specific helper signature block =="
rg -n "def _install_fake_consensus_db\(" -n "$FILE" && \
sed -n '160,240p' "$FILE"Repository: genlayerlabs/genlayer-studio
Length of output: 8563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unit/test_rpc_health_genvm_tracking.py"
echo "== Search for any return annotations (->) =="
rg -n -- "->" "$FILE" || true
echo
echo "== Search for any parameter type annotations (e.g., ': int', ': bool', ': str') =="
rg -n -- ":\s*(int|bool|str|float|dict|list|tuple|set|Optional|Any|object)\b" "$FILE" || true
echo
echo "== Search for pytest.MonkeyPatch annotations usage =="
rg -n -- "pytest\.MonkeyPatch" "$FILE" || true
echo
echo "== Check for from __future__ import annotations =="
rg -n -- "from __future__ import annotations" "$FILE" || trueRepository: genlayerlabs/genlayer-studio
Length of output: 308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unit/test_rpc_health_genvm_tracking.py"
echo "== File header (first 80 lines) =="
sed -n '1,80p' "$FILE"Repository: genlayerlabs/genlayer-studio
Length of output: 3335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/unit/test_rpc_health_genvm_tracking.py"
echo "== Show _install_fake_consensus_db body where monkeypatch is used (lines 188-420) =="
cat -n "$FILE" | sed -n '188,420p'
echo
echo "== Count occurrences of 'monkeypatch' in this file =="
rg -n "monkeypatch" "$FILE"Repository: genlayerlabs/genlayer-studio
Length of output: 11052
Add type hints to _install_fake_consensus_db helper signature
tests/unit/test_rpc_health_genvm_tracking.py (lines 188-200): _install_fake_consensus_db defines parameters and no return annotation; add type hints (including -> None) to match the repository typing rule.
Suggested fix
- def _install_fake_consensus_db(
- self,
- monkeypatch,
+ def _install_fake_consensus_db(
+ self,
+ monkeypatch: pytest.MonkeyPatch,
*,
- recovery_storm_count=0,
- max_recovery_count=0,
- exhausted_rows=None,
- backlog_count=0,
- oldest_created_at=None,
- oldest_backlog_age_seconds=None,
- progress_epoch=1,
- progress_error=False,
- ):
+ recovery_storm_count: int = 0,
+ max_recovery_count: int = 0,
+ exhausted_rows: list[SimpleNamespace] | None = None,
+ backlog_count: int = 0,
+ oldest_created_at: object | None = None,
+ oldest_backlog_age_seconds: int | None = None,
+ progress_epoch: int = 1,
+ progress_error: bool = False,
+ ) -> None:📝 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
| def _install_fake_consensus_db( | |
| self, | |
| monkeypatch, | |
| *, | |
| recovery_storm_count=0, | |
| max_recovery_count=0, | |
| exhausted_rows=None, | |
| backlog_count=0, | |
| oldest_created_at=None, | |
| oldest_backlog_age_seconds=None, | |
| progress_epoch=1, | |
| progress_error=False, | |
| ): | |
| def _install_fake_consensus_db( | |
| self, | |
| monkeypatch: pytest.MonkeyPatch, | |
| *, | |
| recovery_storm_count: int = 0, | |
| max_recovery_count: int = 0, | |
| exhausted_rows: list[SimpleNamespace] | None = None, | |
| backlog_count: int = 0, | |
| oldest_created_at: object | None = None, | |
| oldest_backlog_age_seconds: int | None = None, | |
| progress_epoch: int = 1, | |
| progress_error: bool = False, | |
| ) -> None: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_rpc_health_genvm_tracking.py` around lines 188 - 200, The
helper _install_fake_consensus_db lacks type annotations; update its signature
to add types for all parameters (e.g., monkeypatch: pytest.MonkeyPatch or
MonkeyPatch, recovery_storm_count: int = 0, max_recovery_count: int = 0,
exhausted_rows: Optional[int] = None, backlog_count: int = 0, oldest_created_at:
Optional[datetime] = None, oldest_backlog_age_seconds: Optional[int] = None,
progress_epoch: int = 1, progress_error: bool = False) and add the return
annotation -> None; also add any needed imports (from typing import Optional and
from datetime import datetime or appropriate types) at the top of the test file
so the new annotations resolve.
Source: Coding guidelines
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_system_health_metrics_include_stuck_head_events(): |
Contributor
There was a problem hiding this comment.
Add a return type hint to the new async test function (Line 56).
Please annotate the test as -> None to satisfy the repository typing rule for Python functions.
Suggested patch
`@pytest.mark.asyncio`
-async def test_system_health_metrics_include_stuck_head_events():
+async def test_system_health_metrics_include_stuck_head_events() -> None:As per coding guidelines: "**/*.py: Include type hints in all Python code".
📝 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
| async def test_system_health_metrics_include_stuck_head_events(): | |
| `@pytest.mark.asyncio` | |
| async def test_system_health_metrics_include_stuck_head_events() -> None: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_usage_metrics_service.py` at line 56, Add the missing return
type hint to the async test function by changing the signature of async def
test_system_health_metrics_include_stuck_head_events() to include -> None (i.e.,
async def test_system_health_metrics_include_stuck_head_events() -> None:) so it
conforms to the repository typing rule for Python functions.
Source: Coding guidelines
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
PENDING/ACTIVATEDhead could defer to a younger eligible finalization thatclaim_next_finalizationcould not legally claim yet./health/consensusorphaned-transaction reporting with the main/healthstuck-head detector.instanceHealthEventsso alerts can include affected transaction and contract context.Root Cause
claim_next_transactiondeferred anyPENDING/ACTIVATEDcandidate when an eligible finalization existed on the same contract.claim_next_finalization, correctly, refuses to claim a finalization while an older non-terminal tx exists for that contract. If the finalization was younger than the candidate, both claim paths could reject work: transaction claiming deferred to finalization, and finalization was blocked by the older candidate. The fix makes transaction claiming defer only to older eligible finalizations.The latest
stuck_finalizationsalert exposed a separate observability gap: health only returnedstuck_finalization_count, so Slack could not name the rows. This PR now includes a boundedstuck_finalization_transactionssample with tx hash, contract, status, age,blocked_at, andworker_id.Validation
./.venv/bin/python -m py_compile backend/consensus/worker.py backend/protocol_rpc/health.py backend/services/usage_metrics_service.py tests/db-sqlalchemy/test_finalization_starvation.py tests/db-sqlalchemy/test_health_orphan_detection.py tests/unit/test_usage_metrics_service.py./.venv/bin/python -m pytest tests/unit/test_usage_metrics_service.py -q- 2 passed./.venv/bin/python -m pytest tests/db-sqlalchemy/test_finalization_starvation.py -q- 11 passed./.venv/bin/python -m pytest tests/db-sqlalchemy/test_health_orphan_detection.py -q- 23 passed/Users/edgars/Dev/genlayer-studio/.venv/bin/python -m py_compile backend/protocol_rpc/health.py tests/db-sqlalchemy/test_health_orphan_detection.py/Users/edgars/Dev/genlayer-studio/.venv/bin/python -m black --check backend/protocol_rpc/health.py tests/db-sqlalchemy/test_health_orphan_detection.py/Users/edgars/Dev/genlayer-studio/.venv/bin/python -m pytest tests/db-sqlalchemy/test_health_orphan_detection.py -k stuck_finalization- blocked locally becausePOSTGRES_URLis not set in this environmentSummary by CodeRabbit
New Features
Bug Fixes
Behavioral
Tests
Chores