Repository navigation
feat(locks): add auto-renewing context - #83
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b99abd5967
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except (KeyError, TypeError, ValueError, VolcanoError) as error: | ||
| release_error = error |
There was a problem hiding this comment.
If cleanup raises an exception outside this narrow tuple, it replaces an already-active exception from the context body, contradicting the documented guarantee that the body exception takes precedence. For example, clearing the client's current service credential makes release() raise RuntimeError from _service_token(), which masks the body's original failure; catch general cleanup exceptions here and suppress them whenever body_failed is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head. Cleanup now suppresses any ordinary release exception only when a body or renewal failure already has precedence; otherwise release exceptions still propagate. The regression uses an unexpected RuntimeError from release.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e1fa95bfa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| body_failed = False | ||
| try: | ||
| self._prepare_guard(key, guard, ttl=ttl) | ||
| renewer.start() |
There was a problem hiding this comment.
Once this starts the background renewer, an ordinary exception outside _lock_worker.LockRenewer._renew_once()'s narrow whitelist terminates the thread without marking the guard lost. For example, if _service_token() raises RuntimeError during renewal, guard.lost and wait_lost() do not report the failed renewal until the local lease eventually expires, so protected work may continue after automatic renewal has already stopped; record every ordinary renewal exception on the guard so callers can stop promptly and the context can raise it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head. The worker now records and suppresses any ordinary exception across its full run boundary, while preserving the original exception on the guard. Focused tests cover both request and scheduling failures.
| with suppress(Exception): | ||
| self.release(key, guard.lease) | ||
| else: | ||
| self.release(key, guard.lease) |
There was a problem hiding this comment.
When the caller retains the yielded guard or another worker is blocked in guard.wait_lost(), a successful context exit releases the server lease here but leaves the guard reporting lost == False until its old local deadline. Such a worker can therefore continue protected work after this context has relinquished ownership; successful cleanup should transition or close the guard and wake its loss waiters.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head. Context cleanup now closes the guard in a finally block after the release attempt, waking waiters and making retained guards report lost without manufacturing a renewal failure.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
with_lock()context manager over the reviewed guard and renewal workerLockGuardlease-loss observationThis is the final integration slice replacing #71; scheduling, guard state, and worker mechanics landed independently.
Verification
uv run python scripts/check_openapi.pyuv run ruff check .uv run ruff format --check .uv run mypyuv run pyrightuv run pytest tests/unit -quv run behave features/contract --dry-run --no-snippetsuv run python -m build