Skip to content

fix(proxy): recover goal restarts from unavailable owners - #1679

Open
leventov wants to merge 8 commits into
Soju06:mainfrom
leventov:fix/recover-restarted-conversation-affinity
Open

fix(proxy): recover goal restarts from unavailable owners#1679
leventov wants to merge 8 commits into
Soju06:mainfrom
leventov:fix/recover-restarted-conversation-affinity

Conversation

@leventov

@leventov leventov commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

A Codex conversation restart can resend a self-contained thread under the same process-session identifier after its legacy owner exhausts quota. Raw legacy codex_session rows are intentionally hard, so ordinary requests must fail closed; before this change, that same row also trapped an explicit, self-contained goal restart on an unavailable account.

This PR adds one proof-gated direct-routing exception. A request may retire an unavailable raw legacy owner only when it carries Codex's recognized goal-continuation marker and its canonical upstream Responses payload is account-neutral and self-contained. Retirement is compare-and-set, policy-scoped, and request-owned. Ordinary, incremental, file-pinned, conversation-bound, and unresolved-tool requests remain fail-closed.

HTTP bridge reuse/replacement is deliberately split into the dependent #1680. No public wire format, setting, or default timeout changes. One nullable sticky-session column records source-qualified abandonment without backfilling historical rows.

Linked issue: none exists for this incident-derived defect; routed regression coverage exercises the public Codex Responses and direct WebSocket paths.

Behavior and safety

  • Recognize the existing goal-continuation context only after canonicalizing the request to the upstream Responses body.
  • Permit guarded retirement only for a raw legacy process-session row whose owner is durably PAUSED, RATE_LIMITED, or QUOTA_EXCEEDED.
  • Require the persisted owner to be inside authenticated account-assignment and security-policy scope, computed before model and service-tier eligibility.
  • Scope abandonment to process-session interpretation while retaining the raw account as hard ownership for an equal explicit turn-state value.
  • Compare mapping owner and unavailable status atomically; a concurrent owner change or recovery wins.
  • Exclude a successfully retired owner from the remainder of the same selection, even when selection loaded a stale ACTIVE account snapshot.
  • Preserve request-scoped authority through direct WebSocket selection and strip only proxy-generated turn state when the restart changes accounts.
  • Keep every unproved or account-dependent continuation hard owner-bound.

OpenSpec

  • Behavior is specified by the archived, verified change at openspec/changes/archive/2026-08-10-recover-restarted-conversation-affinity/.
  • Stable requirements and rationale are synchronized in openspec/specs/sticky-session-operations/.
  • Parent requirements cover canonical classification, scoped CAS retirement, stale-selection exclusion, and direct WebSocket state provenance.
  • HTTP bridge requirements are isolated in the stacked fix(http-bridge): preserve goal-restart recovery across reconnects #1680 delta.

Origin and concurrent work

Landed lineage:

Concurrent work reviewed for overlap:

Review findings addressed

Concrete review findings incorporated in this object:

  1. Retirement is limited to the authenticated account-policy scope.
  2. Successful retirement overrides stale account-selection snapshots.
  3. Accepted compatibility and transport-envelope fields classify through the canonical upstream request body.
  4. Proxy-generated direct-WebSocket turn state does not cross the account change, while client-supplied state remains hard.
  5. Raw-key abandonment retains colliding explicit turn-state ownership.
  6. Model and service-tier eligibility do not narrow authenticated mutation authority.

The cloud Codex review was clean at 65103b40. Current-head review was requested after the rebase, but the fork connector rejected it with an account-linking error; there are no unresolved non-outdated review threads.

Current-main rebase

Rebased on current main at 7c467198 (#1677), which already includes #1657 and its Alembic merge head. The additive migration is renamed to 20260812_120000_add_sticky_abandonment_scope and now follows 20260812_000000_merge_recovery_dispatch_and_hourly_cancelled_heads, preserving a single-head graph.

Validation

affected suites: 1,195 passed
load-balancer refresh signature regressions: 87 passed, 3 skipped
Ruff check and format: passed
type and architecture/import checks: passed
additive migration upgrade/downgrade regression: passed
OpenSpec validation: 49 specs passed, 0 failed
git diff --check: passed

The first current-head cloud run passed unit, core integration, PostgreSQL, migration, Ruff, type, packaging, Docker, and browser jobs. Its bridge shard had one isolated bridge_instance_mismatch in test_v1_responses_http_bridge_classifies_responses_lite_developer_interleaved_full_resend[unowned-developer-message]; that exact test passes locally on 736dd4cd, and the same bridge suite is green on the stacked #1680 head. The fork author cannot use GitHub's failed-job rerun action without upstream repository administration.

Screenshots / output

No dashboard-visible change.

Before:

marked self-contained restart + unavailable raw owner -> hard-affinity failure

After:

verified marked restart -> guarded CAS retirement -> eligible replacement owner
ordinary/account-dependent request -> unchanged fail-closed ownership

Simplicity

  • Zero configuration; no setting or environment variable.
  • One additive nullable migration with no historical backfill; no setup step, README section, or dashboard surface.
  • Reuses existing replay classification, sticky tombstones, and selection.
  • No new retry budget or timeout.

Checklist

  • Conventional Commit PR title.
  • OpenSpec behavior is present, synchronized, verified, and archived.
  • Direct route, direct WebSocket, scoped-policy, stale-snapshot, source-provenance, model-authority, and migration regressions are included.
  • Simplicity gates P1-P5 reviewed.
  • CHANGELOG.md was not edited.
  • Current-head cloud CI green.
  • Current-head Codex review evidence clean or all findings addressed.

@leventov
leventov force-pushed the fix/recover-restarted-conversation-affinity branch from 2412c6d to a8e8199 Compare August 10, 2026 12:08
@Komzpa

Komzpa commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd554303b9

ℹ️ 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".

Comment thread app/modules/proxy/affinity.py
Comment thread app/modules/proxy/_service/websocket/mixin.py
Comment thread app/modules/proxy/_load_balancer/sticky_selection.py Outdated
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 10, 2026
@github-actions github-actions Bot added the db migration PR changes Alembic database migrations; maintainer must coordinate merge order label Aug 11, 2026
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 11, 2026
@Komzpa

Komzpa commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 65103b4099

ℹ️ 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".

@Komzpa Komzpa added 🤖 codex: ok [@codex review] says no issues found. and removed 🤖 codex: ok [@codex review] says no issues found. labels Aug 11, 2026
@Soju06

Soju06 commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Gate check: codex is clean on 65103b4 and CI Required is green, but #1696 (merged after your last CI run) added migration 20260811_000000_add_hourly_rollup_cancelled_count on the same parent (20260806_120000_add_http_bridge_owner_process_epoch) that your 20260811_000000_add_sticky_abandonment_scope revises. Merging as-is would create two Alembic heads. Please re-parent your migration's down_revision onto 20260811_000000_add_hourly_rollup_cancelled_count (and consider renaming the file to a later timestamp so two 20260811_000000_* files don't coexist), then rebase onto current main so the alembic check and CI re-run on the new head. I'll merge this ahead of #1680 once that's green.

@Soju06

Soju06 commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Correction to the re-parent target: #1657 just merged, so main's Alembic head is now 20260812_000000_merge_recovery_dispatch_and_hourly_cancelled_heads — please re-parent down_revision onto that (not 20260811_000000_add_hourly_rollup_cancelled_count as I said above).

@leventov

Copy link
Copy Markdown
Contributor Author

Rebased onto current main at 7c467198 (#1677, including #1657). The migration is now 20260812_120000_add_sticky_abandonment_scope with down_revision = 20260812_000000_merge_recovery_dispatch_and_hourly_cancelled_heads; the migration policy reports one head and the additive round-trip regression passes. New head: 736dd4cd.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@leventov

Copy link
Copy Markdown
Contributor Author

@Soju06 @Komzpa please review

@Soju06

Soju06 commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Re-parent target update (second correction, sorry for the churn): #1618 just merged and its migration sits on the previous head, so main's Alembic head is now 20260806_000000_add_anonymous_telemetry (note: the revision ID is dated earlier than its parent 20260812_000000_merge_recovery_dispatch_and_hourly_cancelled_heads — go by the graph, not the filename date). Please point down_revision there, or add a merge revision, so the single-head CI check passes after update-branch.

Permit an explicit, account-neutral Codex goal restart to retire an unchanged legacy owner only while its persisted account status is unavailable. Preserve fail-closed routing for all other continuity evidence and guard retirement with one compare-and-set tombstone.
@leventov
leventov force-pushed the fix/recover-restarted-conversation-affinity branch from 736dd4c to 431492f Compare August 12, 2026 11:44
@leventov

Copy link
Copy Markdown
Contributor Author

Addressed in 431492f8. I rebased #1679 onto current main at debd7cf6 (#1618), then re-parented 20260812_120000_add_sticky_abandonment_scope onto the graph head 20260806_000000_add_anonymous_telemetry (including the migration docstring). A direct descendant is sufficient here; no merge revision is needed. Local verification reports one Alembic head, successful upgrade to head, migration_policy=ok, schema_drift=none, and the focused migration tests pass (2 passed).

@leventov

Copy link
Copy Markdown
Contributor Author

Follow-up on the failed unit shard: fixed in 318694e7. The telemetry migration regression from #1618 asserted that 20260806_000000_add_anonymous_telemetry remained the repository head after upgrade head; #1679 now correctly adds a descendant, so that invariant was stale. The test now compares the upgrade result with the inspected graph head while retaining the telemetry upgrade, default, and downgrade assertions. Focused telemetry, sticky-scope, and migration-policy tests pass: 3 passed. Fresh CI is running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

db migration PR changes Alembic database migrations; maintainer must coordinate merge order

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants