fix(proxy): reconcile owner replicas after leader purges abandoned bridge sessions - #1716
fix(proxy): reconcile owner replicas after leader purges abandoned bridge sessions#1716Soju06 wants to merge 14 commits into
Conversation
…idge sessions The leader's abandoned-session purge deletes durable HTTP-bridge rows only; the owner replica's in-memory session — and the account stream lease it may hold — survives, which is issue #1354's observed 'http_bridge_sessions empty but in-memory stream cap full' state. The purge now bumps a new http_bridge_purge cache-invalidation namespace, and on that bump every replica reconciles quiescent in-memory bridge sessions (no pending work, no admission waiter, no handoff, no unanchored reservation) against the durable table: sessions whose rows are gone are detached and closed, releasing their leases. The close skips the durable release (row already gone) and writes no account health, matching idle-eviction semantics. Fixes #1354 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
![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: 9940e719b8
ℹ️ 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".
…closes - Re-validate the full candidate predicate under the lock: a session that gained an unanchored reservation or re-claimed a fresh durable row while lookup_sessions() was awaited is no longer detached. - Run the orphan closes in a tracked background task so slow upstream-reader cancels never pin the sole cache-invalidation poller; sessions are already detached, so a concurrent bump cannot double-process them and shutdown drains the tracked task. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96fd237e6f
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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". |
Komzpa's drain/activity registration and bump persistence are kept; this fixes the three CI gates that were red on that head: - mixin.py had grown to 2515 lines against the 2436 architecture ratchet. Move the reconcile into _service/http_bridge/purge_reconcile.py following the existing module decomposition, and factor the duplicated candidate predicate into one _purge_reconcile_eligible_durable_id() helper used by both the initial scan and the under-lock re-validation, so the two can no longer drift. mixin.py is back to 2427 lines. - ty: assert Mock.await_args is not None before reading .kwargs. - The namespace enumeration in test_cache_invalidation_bus.py needed the new http_bridge_purge namespace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@Komzpa thanks — both of your changes are real gaps I missed and they stay: the synchronous Your head was red on three gates, now fixed in 36f5d0e:
All four gates pass locally (ruff, ruff format, @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36f5d0e585
ℹ️ 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".
purge_abandoned_before commits its deletions in batches inside the loop, so bumping only after it returns leaves every already-committed batch unsignalled when the run is cancelled or the process dies mid-purge — the owner replicas then keep exactly the orphaned sessions and stream leases this change exists to release, until the lease TTL. The repository now takes an on_batch_committed callback invoked after each batch commits, and the scheduler's _signal_abandoned_bridge_purge persists the bump synchronously per batch, falling back to the poller retry queue on a failed or raising write. A signalling failure never aborts the purge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1f77c0394
ℹ️ 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".
The signal callback runs only after its batch is committed, so a graceful shutdown that cancels the cleanup scheduler mid-write would drop the signal for rows that are durably gone — and CancelledError is a BaseException, so it bypassed the failure fallback entirely. Shield the bump and drain it within a bounded grace before propagating the cancellation, queueing the retry when it still does not land. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92d891df3b
ℹ️ 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".
…p task - Release every detached orphan's account stream lease up front instead of inside the serial close loop: releasing the cap slot is the point of the pass, and a close can block on an upstream-reader cancellation, so later orphans' capacity was held for as long as earlier readers took to unwind. Idempotent with the close's own release; one failure cannot strand the rest. - Cancel the shielded bump task when the drain grace expires, logging its outcome, rather than leaving it running untracked for loop teardown to cancel silently after the poller has already stopped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30e900d70f
ℹ️ 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".
…oller stop - The lease-release await sat between detaching the orphans and creating the tracked close task, so a concurrent shutdown could see an empty registry and finish its only bridge-cleanup drain before the task existed — and a cancellation in that window dropped the task entirely, leaking the detached sessions' upstream sockets. Detach and track are now adjacent; the releases moved inside the tracked task, still ahead of every close. - CacheInvalidationPoller.stop() cancelled its polling task without flushing queued bumps, and main.py stops the poller right after the schedulers, so a bump that failed over to the retry queue during shutdown was discarded and peers kept stale caches. stop() now runs a final flush, logging whatever cannot be flushed. This applies to every namespace, not just the purge signal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae32c507fc
ℹ️ 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".
A session handed to an anchored request is not yet visible as busy: the request sets last_used_at at handoff but only increments admission_waiter_count once it reaches submit, and an anchored reuse can await the retry-circuit lookup in between. Idle eviction is protected from that window by its idle TTL; this pass exists to beat that TTL, so without its own floor it could close a session out from under a live request. Add a 30s recency floor to the candidate predicate. Any session this pass legitimately targets has been abandoned for hours — the purge only deletes rows whose activity predates the retention cutoff — so the floor costs nothing while closing the handoff race. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43a7477b2b
ℹ️ 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".
The final flush added for the purge signal awaited the same database that can block a caller's synchronous bump, with no deadline, so an unresponsive driver could hold shutdown indefinitely — past both the drain budget and the bounded grace that queued the fallback in the first place. Bound it with _STOP_FLUSH_TIMEOUT_SECONDS, cancel the abandoned write with a done-callback logging its outcome, and restore the pending markers on timeout or failure: _flush_pending_bumps clears each marker before awaiting its write, so an abandoned flush would otherwise lose the namespace it was working on rather than leaving it queued and logged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae42938459
ℹ️ 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".
Derive the purge reconcile's idle floor from proxy_request_budget_seconds instead of a fixed 30s: that budget bounds the whole request, so a handoff whose submit has not happened within it belongs to a request that already failed. No arbitrary constant has to be assumed safe against a slow intermediate lookup (e.g. the pre-created retry-circuit cooldown). _flush_pending_bumps clears each marker before awaiting its write, so a cancelled write left the namespace neither written nor pending. stop() cancels the polling task by design, so this silently dropped whatever the poll loop was mid-write on — before the final flush could see it. Restore the marker on any abort, and handle CancelledError in the stop flush too: it is a BaseException, so it bypassed the restore while the finally block still cancelled the write, and stop() has already cleared _task so a retried stop() would never revisit it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93893ffa4c
ℹ️ 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".
The bridge runs its own budget (http_responses_session_bridge_request_budget_seconds, 7200s) rather than the general proxy budget (600s), so the floor expired while an anchored reuse was still inside its actual deadline. This also fixes the unit failures on CI: time.monotonic() counts from an arbitrary origin, and on a freshly booted container it can be smaller than the budget, so tests relying on the session factory's fixed last_used_at=1.0 were machine-dependent. Reconcile candidates are now backdated relative to the current clock via a shared helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e30863ff3
ℹ️ 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".
The previous commit's floor (the 7200s bridge budget) exceeded the 3600s abandoned-row retention cutoff, so the single bump the purge emits skipped every session aged between them — and no later purge re-signals an already deleted row, so those sessions were never reconciled at all. The two constraints cannot both be met by outwaiting the request budget. Correctness against the pre-submit window comes from the durable liveness check instead: an anchored reuse renews the session's durable row under the bridge lock before it yields, so a session a request is about to use is found live and skipped. The idle floor is now a secondary guard for the narrow residue, clamped to half the retention cutoff so no deployment can configure the pass into a no-op. Invariant covered by a test. Also own the cancelled final-flush unwind with a bounded await rather than returning while cancel() is still in flight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4956f5446
ℹ️ 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".
| if _service_time().monotonic() - session.last_used_at < min_idle_seconds: | ||
| # Recently handed to a request that has not reached submit yet. | ||
| return None |
There was a problem hiding this comment.
When the leader's delete wins the race with an anchored reuse, _renew_durable_http_bridge_lease() returns normally after renew_live_session() returns None, so the claimed durable-liveness guard is absent while the request still receives the local session. A bump processed within this 30-second floor skips that now-orphaned session permanently because deleting the row produces no later signal; if processing is delayed past the floor while the request is still in a pre-submit lookup, reconciliation instead closes the selected session. Fresh evidence beyond the earlier floor thread is the final helper's explicit missing-row fallthrough at helpers.py:2102-2109; retain a request-owned marker through submission rather than relying on this one-shot age test.
AGENTS.md reference: AGENTS.md:L111-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepting this one rather than patching around it again — you have now made the same point three times and been right each time, and this evidence (_renew_durable_http_bridge_lease() returning normally when renew_live_session() yields None, so the reuse proceeds with no live durable row) is what finally kills the last heuristic: the durable liveness check cannot be the primary guard if the leader can win that race.
Combined with the earlier finding that the floor must sit below the retention cutoff to work at all, there is no time-based value that is simultaneously long enough to cover an unbounded pre-submit window and short enough to reconcile anything. Only an explicit request-ownership marker established atomically at handoff satisfies both — which is what you proposed at the start.
That marker belongs in _get_or_create_http_bridge_session() and every submit/failure path that must clear it, and leaking it on an error path would make sessions permanently un-evictable — strictly worse than the bug being fixed. That is a deliberate change to the bridge hot path, not a review-loop patch, so I am leaving it to the maintainer rather than landing it autonomously. Marking the PR draft with a summary; leaving this thread open as the blocking item.
| finally: | ||
| session.account_lease = None |
There was a problem hiding this comment.
When release_account_lease() raises for one detached orphan, the finally block clears its only lease reference even though the load balancer still counts that lease. The subsequent _close_http_bridge_session() therefore sees None and cannot retry the release, leaving that account's stream capacity consumed until stale-lease reclamation—the exact condition this reconciliation is intended to repair. Clear session.account_lease only after a successful release so the later close can retry, while continuing the loop for the remaining sessions.
AGENTS.md reference: AGENTS.md:L111-L115
Useful? React with 👍 / 👎.
…e close retry - The scheduler's abandoned-signal path cancelled the shielded write and returned while cancellation was merely requested; own the unwind with a bounded await, matching the poller's stop path. - A failing release_account_lease cleared the session's only lease reference even though the load balancer still counts that lease, denying the close its retry and stranding the slot until the TTL. Keep the reference on failure and clear it only on success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Status: draft — blocked on a design decision, not on polishMarking this draft after 11 review rounds. Everything except one item is done and green, but that item is a genuine design question that should not be answered inside a review loop. What is settled and working
The blocking itemThe reconcile must not detach a session a request has been handed but has not yet submitted on. I tried three guards and codex correctly refuted each:
The constraints are irreconcilable with any time-based value: it must exceed an unbounded window and stay under the retention cutoff. Only an explicit request-ownership marker set atomically at handoff satisfies both — which is what codex proposed in the first round. Why I stopped hereThat marker means mutating @Soju06 two options:
I lean toward (2): it makes each change reviewable on its own terms, and the marker is independently useful — idle eviction relies on the idle TTL for the same protection today. The |
|
Closing in favour of #1747, which fixes the same issue in +127 lines with no new machinery. The premise this PR was built on was wrong, and @Soju06 caught it: releasing an account lease is a counter decrement that cannot disturb an already-dispatched request, and — decisively — fix (a) (#1476) already made leases turn-scoped. Once the capacity rationale went, the remaining value was closing orphaned sockets on a replica that has stopped receiving traffic — and the existing idle sweep already does that, with the idle-TTL guard against the pre-submit handoff race that this PR spent eleven rounds failing to cover with heuristics. It was simply never being run off the request path. #1747 does that and nothing else. @Komzpa your two commits here were correct fixes to real gaps and I'm sorry they go down with the branch — the bump-persistence one in particular exposed a genuine pre-existing defect in the invalidation bus ( The codex findings on this PR were all legitimate and each one moved the analysis forward — the last of them is what proved no time-based guard could work here. |
2 participants
Summary
Implements fix (b) of the approved design on #1354 (fix (a), turn-scoped leases, landed in #1476). The leader's
purge_abandoned_beforedeletes durable HTTP-bridge rows only — the owner replica's in-memory session, and any account stream lease it holds, survives. That is exactly the reported "http_bridge_sessionsis empty but the in-memory stream cap stays full until restart" state.Fixes #1354.
How
purge_abandoned_beforedeleting >0 rows now bumps a newhttp_bridge_purgenamespace on the existing cache-invalidation bus (leader-side, collapses per poll cycle).reconcile_purged_http_bridge_sessions(): quiescent in-memory sessions (no pending/queued work, no admission waiter, no handoff in progress, no unanchored reservation, durable claim present) are bulk-looked-up bydurable_session_id; sessions whose rows are gone are re-validated under the bridge lock, detached, and closed — releasing their account stream lease immediately instead of waiting out the 900s in-memory lease TTL.release_durable_session=False(the row is already gone) and writes no account error health — identical semantics to idle eviction. Sessions with live rows or in-flight work are untouched.Ownership/settlement invariants (review trapdoors)
durable_session_id), and the abandoned purge itself only removes rows with expired leases older than the retention cutoff.OpenSpec
openspec/changes/reconcile-purged-bridge-sessions/— ADDED requirement onsticky-session-operationswith four scenarios (orphan release, live-row survival, in-flight exemption, no-op purge). Validation passes.Tests
tests/unit/test_proxy_http_bridge.py).http_bridge_purgeexactly when the abandoned purge deleted rows (tests/unit/test_sticky_session_cleanup_scheduler.py).No new settings; zero-config.
🤖 Generated with Claude Code