Skip to content

Exclude timeout exception - #927

Merged
fogelito merged 3 commits into
mainfrom
disable-timeout-retries
Jul 27, 2026
Merged

Exclude timeout exception#927
fogelito merged 3 commits into
mainfrom
disable-timeout-retries

Conversation

@fogelito

@fogelito fogelito commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes
    • Updated transaction retry behavior so timeout errors no longer trigger retries.
    • Improved rollback handling to prevent inconsistent transaction state and ensure the original action error is propagated when rollback also fails.
    • Ensured Redis rollback failures clear pending transaction journal state to avoid stale rollback frames.
  • Tests
    • Added/expanded unit tests covering timeout and duplicate non-retry behavior, generic retry exhaustion, rollback-failure propagation, and Redis journal cleanup.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: dc714e34-8567-43f5-b0bc-69bf409010e4

📥 Commits

Reviewing files that changed from the base of the PR and between fedf587 and 3a51f51.

📒 Files selected for processing (2)
  • src/Database/Adapter/Redis.php
  • tests/unit/TransactionRetryTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unit/TransactionRetryTest.php

📝 Walkthrough

Walkthrough

Adapter and Mongo transaction retries now immediately rethrow TimeoutException. Rollback failures reset transaction state, and tests cover retry classification, exception propagation, and Redis journal cleanup.

Changes

Transaction retry handling

Layer / File(s) Summary
Rollback failure state handling
src/Database/Adapter.php, src/Database/Adapter/Redis.php
Rollback errors are captured or rethrown after transaction state cleanup, with final exception handling selecting the captured rollback error when retries are exhausted.
Non-retriable timeout classification
src/Database/Adapter.php, src/Database/Adapter/Mongo.php
TimeoutException is immediately rethrown instead of entering the retry path.
Retry behavior tests
tests/unit/TransactionRetryTest.php
Tests cover non-retriable timeout and duplicate failures, generic retry exhaustion, rollback-error propagation, and Redis journal cleanup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: abnegate

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: timeout exceptions are no longer retried.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch disable-timeout-retries

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Updates transaction failure handling across adapters:

  • Treats timeout exceptions as non-retriable in the base and Mongo transaction wrappers.
  • Resets transaction state after rollback failures and preserves rollback errors for exhausted retryable failures.
  • Clears Redis transaction journal state when rollback replay fails.
  • Adds unit coverage for retry policy and rollback cleanup.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/Database/Adapter.php Updates shared retry classification and rollback-failure state handling; no eligible follow-up finding was established.
src/Database/Adapter/Mongo.php Aligns Mongo timeout retry behavior with the shared adapter policy.
src/Database/Adapter/Redis.php Clears the transaction counter and journal stack together after rollback replay fails.
tests/unit/TransactionRetryTest.php Adds focused coverage for retry counts, non-retriable exceptions, and rollback cleanup.

Reviews (3): Last reviewed commit: "fix comments add tests" | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/Database/Adapter.php`:
- Around line 459-460: Update the exception handling around
rollbackTransaction() so non-retriable $action exceptions, including
LimitException and TimeoutException, are rethrown immediately even when rollback
cleanup fails. Preserve the rollback attempt, but ensure rollback failure
handling does not retry or mask these action exceptions; retain existing
behavior for retriable actions.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 16dca011-c9e9-421b-bea3-d4908278a2ec

📥 Commits

Reviewing files that changed from the base of the PR and between 0797ba6 and c1a3194.

📒 Files selected for processing (3)
  • src/Database/Adapter.php
  • src/Database/Adapter/Mongo.php
  • tests/unit/TransactionRetryTest.php

Comment thread src/Database/Adapter.php

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@src/Database/Adapter/Redis.php`:
- Around line 4003-4011: Update the rollback failure handling around
rollbackJournal() to clear all remaining journal frames in $journalStack before
resetting $inTransaction to 0 and rethrowing the exception. Preserve the
existing successful rollback behavior and SQL/Mongo transaction-depth contract.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f920dd6f-3b56-4712-9c9a-23de7bb97fe0

📥 Commits

Reviewing files that changed from the base of the PR and between c1a3194 and fedf587.

📒 Files selected for processing (3)
  • src/Database/Adapter.php
  • src/Database/Adapter/Redis.php
  • tests/unit/TransactionRetryTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unit/TransactionRetryTest.php

Comment thread src/Database/Adapter/Redis.php
@fogelito
fogelito merged commit 7fd133e into main Jul 27, 2026
39 of 40 checks passed
@fogelito
fogelito deleted the disable-timeout-retries branch July 27, 2026 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants