fix(#52): opt-in retry ceiling for transact/readTransact/watch loops - #79
Merged
Conversation
added 2 commits
August 21, 2026 08:55
…retry loops The convenience retry loops in Database::transact(), readTransact(), and the four watch() helpers were unbounded 'while (true)' cycles that relied entirely on FoundationDB's fdb_transaction_on_error() to eventually surface a non-retryable error. A persistently conflicting workload could therefore spin indefinitely under the default. Fix introduces an opt-in, process-wide ceiling (FoundationDB:: defaultTransactionRetryLimit(int) and ::defaultTransaction TimeoutSeconds(float)) and routes all six retry sites through a shared Database::runWithRetry() helper, which throws a new TransactionRetryLimitExceededException carrying the actual attempt count and elapsed wall-clock seconds when either ceiling is hit. The pure Database::checkRetryLimit() predicate is exposed as static so the bounded-retry decision can be unit-tested without FFI. Both ceilings default to 0 / 0.0 (unbounded), preserving the historical 'FDB decides when to stop retrying' semantics for users who do not opt in. Negative values throw \\InvalidArgumentException at configuration time so a typo cannot silently disable the ceiling. Coverage: - tests/Unit/TransactionRetryLimitTest.php: 28 cases (predicate, configuration validation, exception fields, message). - tests/Integration/TransactionRetryLimitTest.php: 6 cases (transact/readTransact/watch honour limits, default-unbounded preserved, wall-clock ceiling terminates, reset()). Docs: docs/transactions.md adds 'Bounded retry' section; CHANGELOG entry at the top of [Unreleased].
The integration tests rescued from the abandoned branch injected a synthetic FDBException(1007) into the callback and relied on the native fdb_transaction_on_error() to resolve. A fabricated error code describes an error the transaction never experienced, so the native future may never resolve - in practice the suite hung indefinitely against a live cluster (first observed: no output within 10 minutes). Rewrite the scenarios around deterministic REAL conflicts: the callback reads a key, an interferer transaction commits a change to it, then the callback writes and commits - producing genuine not_committed (1020) retries through the real on_error() path. - attempt ceiling: interferer fires every round; limit=2 aborts with TransactionRetryLimitExceededException after exactly 3 retries - default unbounded ceiling: interference stops after 3 rounds; loop recovers and commits without any library exception - wall-clock ceiling: unbounded attempts + 50ms budget terminates the loop while conflicts keep coming - readTransact/watch: smoke coverage that both paths work with a ceiling configured (snapshot reads cannot conflict) Against master (no fix) the file errors on the missing configuration API; with the fix all five cases pass in ~0.3s.
5 tasks
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.
Closes #52
Problem
Database::transact(),readTransact()and the fourwatch*helpers spun unboundedwhile (true)loops relying entirely onfdb_transaction_on_error()to eventually surface anon-retryable error. A persistently conflicting workload could spin indefinitely — there was no
library-level ceiling and no convenient default
RETRY_LIMIT/TIMEOUT.Change
Opt-in, process-wide ceilings (default behaviour unchanged):
FoundationDB::defaultTransactionRetryLimit(int)0= unbounded< 0⇒InvalidArgumentException; otherwise loop throws after N retriesFoundationDB::defaultTransactionTimeoutSeconds(float)0.0= unbounded< 0.0⇒InvalidArgumentException; otherwise loop throws when budget exhaustedAll six retry sites route through a shared
Database::runWithRetry(); when a ceiling is hit itthrows the new
TransactionRetryLimitExceededExceptioncarrying the actual retry count andelapsed wall-clock seconds. The pure predicate
Database::checkRetryLimit()is public static forFFI-free unit testing. Ceilings are cleared by
FoundationDB::reset().Integration tests rewritten (important)
The rescued integration tests injected a synthetic
FDBException(1007)into the callback andrelied on the native
on_error()future to resolve — a fabricated error describes an error thetransaction never experienced, and in practice the suite hung indefinitely against a live
cluster (no output within 10 minutes). They are rewritten around deterministic real conflicts
(read → interferer commits → write+commit ⇒ genuine
not_committedretries): ceiling exhaustion,default-unbounded recovery, wall-clock termination, plus readTransact/watch smoke coverage.
Against master the file errors on the missing API; with the fix all five cases pass in ~0.3s.
Verification
composer lint(PHPCS + Rector + PHPStan L9)composer test:unit