Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860)#867
Closed
peco-engineer-bot[bot] wants to merge 2 commits into
Closed
Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860)#867peco-engineer-bot[bot] wants to merge 2 commits into
peco-engineer-bot[bot] wants to merge 2 commits into
Conversation
…publish The engineer-bot author run on issue #860 failed at "Open / update fix PR": publish's leftover safety-net found poetry.lock + pyproject.toml unstaged and not in the agent's touched_files, so it refused to open the PR — even though the agent's fix + tests were correct (494 passed). Root cause: the shared setup-poetry action runs `poetry lock` (to reconcile the lock with the internal JFrog source it injects), which REWRITES the tracked poetry.lock / pyproject.toml in the working tree. That churn is harmless for the 5 real-CI consumers of the action (they commit nothing) but poisons the two engineer-bot workflows, which publish a PR from the working tree. Fix (engineer-bot side only — leave the shared action untouched): after the Poetry setup, `git checkout -- poetry.lock pyproject.toml` to drop the churn once the venv is built, so only the agent's edits remain in the tree at publish time. Applied to both engineer-bot.yml (author) and engineer-bot-followup.yml. Signed-off-by: eric-wang-1990 <e.wang@databricks.com> Co-authored-by: Isaac
…ry-After to delay_max (60s floor) (#860) Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot
Bot
added
the
engineer-bot
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
Contributor
|
This is a test PR, close for now |
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
Automated fix for #860 — Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor).
Changed
proposed_wait = max(proposed_wait, self.delay_max)tomin(...)inDatabricksRetryPolicy.sleep_for_retry(src/databricks/sql/auth/retry.py:300) sodelay_maxacts as a ceiling on the honored Retry-After / backoff wait rather than a floor, matching the siblingget_backoff_time. The three reproducing tests now pass and the full unit suite stays green.Root cause & plan
Root cause: In
src/databricks/sql/auth/retry.py,DatabricksRetryPolicy.sleep_for_retry(line 300) computesproposed_wait = max(proposed_wait, self.delay_max). This usesdelay_maxas a FLOOR rather than a CEILING, so any proposed wait smaller thandelay_max(default 60s) is inflated up todelay_max. This inverts the intent documented on the siblingget_backoff_time, which correctly clamps withmin(proposed_backoff, self.delay_max). Because this overriddensleep_for_retryalways sleeps and returns True, it governs BOTH the serverRetry-Afterpath and the exponential-backoff (no-header) path — so aRetry-After: 2becomes a 60s sleep, and a 1s backoff also becomes a 60s sleep. Fix: changemaxtominsodelay_maxcaps the wait.Files:
src/databricks/sql/auth/retry.py,tests/unit/test_retry.pyPlanned coverage:
Files changed
src/databricks/sql/auth/retry.pytests/unit/test_retry.pyTest plan
tests/unit/test_retry.py::TestRetry::test_sleep__retry_after_header_honored_not_inflated— fails (red) against the original code, passes (green) after the fixtests/unit/test_retry.py::TestRetry::test_sleep__no_retry_after— fails (red) against the original code, passes (green) after the fixtests/unit/test_retry.py::TestRetry::test_sleep__no_retry_after_header__multiple_retries— fails (red) against the original code, passes (green) after the fix🤖 Generated by engineer-bot (bug-fix flow) — review before merge.