Skip to content

Commit d4f9e23

Browse files
Soju06claude
andauthored
perf(accounts): make account deletion a fast mark + background batch drain (#1795)
* perf(accounts): move account deletion bulk row work to a background batch worker DELETE /api/accounts/{id} previously detached (or deleted) the account's entire raw history in one transaction while holding the fold-state lock: measured on production, ~313s to soft-detach 133k request_logs rows plus 2x11.6s for usage_history, blocking every fold pass, pinning a pool connection past the QueuePool timeout, and timing out the HTTP client. The API now stamps a durable pending-deletion marker in a millisecond transaction (terminal DEACTIVATED status, immediate listing/serving exclusion, sticky/bridge cleanup, frozen delete_history choice) and a new leader-gated worker drains usage_history, additional_usage_history, and request_logs in 5k-row transactions without the fold-state lock, then finalizes residual rows, the folded-bucket lifecycle mirrors, and the sticky/rollup/account rows in one fold-state-locked transaction with the historical shape. Fold slices interleaving between chunks converge at finalization because every slice holds the fold-state row lock from raw read to commit: it lands either before the mirrors (moved/removed) or after (sees no attributed raw rows), so folded rows never resurrect. Deletion is restart-safe (all progress in the database), idempotent (repeat requests do not escalate the frozen variant), supports both delete_history variants, and is superseded by a credential replacement that clears the marker; the worker abandons superseded accounts before finalizing. Response contract ({"status": "deleted"}) and frontend are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): harden background deletion supersede, exports, fairness, and queue index Address local codex review round 1: - Chunk transactions now re-read the pending marker under the account row lock (PostgreSQL FOR NO KEY UPDATE) so a credential replacement cannot clear the marker between the read and the chunk's row mutations; no chunk can commit row work after a replacement has successfully returned. - Credential-export endpoints (export, auth export, opencode auth export) treat marked accounts as not found: a successful DELETE no longer leaves decrypted tokens retrievable during the background drain window. - Deletion passes round-robin one chunk per pending account and re-scan the pending set between rounds, so one account's multi-minute drain cannot starve another marked account or a delete request landing mid-pass. - Partial index idx_accounts_delete_requested_at ((delete_requested_at, id) WHERE delete_requested_at IS NOT NULL) backs the per-interval pending probe and the queue-order scan; empty in the steady state. OpenSpec delta/design/tasks updated; new integration coverage for export 404s, round-robin interleave, and mid-pass pickup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): wipe tokens, drop key assignments, fence status writes, tighten round-robin Address local codex review round 2: - begin_delete overwrites the stored access/refresh/id token ciphertext with empty-credential ciphertext: the row outlives the DELETE response by the drain duration, and readers that do not know the marker (pre-upgrade replicas' export endpoints during a rolling deploy) must not be able to produce usable credentials from it. Rotation is CAS-guarded on the pre-wipe refresh ciphertext; every supersede path writes fresh material. - begin_delete deletes the account's ApiKeyAccountAssignment rows (the projection the synchronous delete's FK cascade produced): GET /api/api-keys and pooled-usage reads exclude the account immediately, while the key's persisted assignment-scope flag keeps it scoped. Fixes test_deleted_assigned_accounts_do_not_fall_back_to_other_accounts. - update_status / update_status_if_current gain a delete_requested_at IS NULL fence so stale in-flight settlements (e.g. a late 429) cannot replace the terminal DEACTIVATED state and make a marked account selectable mid-drain; credential replacement bypasses these writers and still supersedes. - A drain round now yields after the first NONEMPTY chunk (not the first batch-size-full one), so an account with small tables cannot stack three row-touching transactions into one round. OpenSpec delta/design/tasks updated; new integration coverage for token wipe and the status-write fence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): preserve seat identity before the pending-deletion token wipe Address local codex review round 3: targeted OAuth reauthentication (a promised supersede path) verifies the seat against chatgpt_user_id or, on legacy rows where it was never backfilled, the stored id-token claims. Wiping id_token_encrypted destroyed the only saved seat identity on such rows, so _save_oauth_account derived an empty intended-seat set and raised ReauthSeatMismatchError before replace_reauthorized could clear the marker — the account would finalize despite fresh credentials arriving. begin_delete now backfills chatgpt_user_id from the id-token claims (via resolve_seat_identity, non-secret identity only) in the same transaction, before overwriting the ciphertext; on PostgreSQL the row is held FOR NO KEY UPDATE across the derive-then-write. OpenSpec delta/design/tasks updated; regression test covers the legacy-row backfill through begin_delete. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): legacy-replica supersede via wipe sentinel, reject marked accounts in key assignment Address local codex review round 4: - A credential replacement handled by a pre-upgrade replica during a rolling deploy writes fresh ciphertext but cannot clear marker columns its ORM does not know, so the worker would have drained and finalized a freshly reauthorized account. Every marker re-check (chunk and finalization) now also inspects the refresh ciphertext: non-wiped (or undecryptable) material on a marked row is itself the supersede signal — the worker clears the marker under the account row lock and abandons the deletion instead. - ApiKeysRepository.list_accounts_by_ids rejects marked accounts, so an API-key create/update racing (or following) the DELETE cannot recreate an assignment that would re-surface the deleted account in key listings or pooled-usage projections before finalization. OpenSpec delta/design/tasks updated; regression coverage for mid-drain and pre-finalize legacy supersede and for post-DELETE key assignment rejection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): serialize finalization against in-flight log inserts, atomic assignment marker re-check Address local codex review round 5: - Finalization upgrades the account row to FOR UPDATE (PostgreSQL) after the fold lock, before the residual sweeps. FOR UPDATE conflicts with the KEY SHARE a request-log FK insert takes, so an in-flight stream's log row either commits before the sweep (and is swept) or blocks until the transaction commits and then fails its FK against the deleted row. This closes the window where an insert landing between the sweep and the account-row delete was ON DELETE SET NULL'ed into a live orphan (soft) or survived outright (delete_history). Lock order (identity -> fold -> row exclusive) matches the historical transaction. - replace_account_assignments now inserts through a conditional INSERT..SELECT WHERE delete_requested_at IS NULL (FOR SHARE on PostgreSQL), re-checking the pending-deletion marker atomically with the write: a key create/update whose validation raced the DELETE either commits first (begin_delete's cleanup removes the assignment) or sees the marker and skips the account. OpenSpec delta/design/tasks updated; pg interleaving regression test for the in-flight insert and an atomic re-check test for assignments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): widen wipe sentinel to all token fields, fix assignment lock order Address local codex review round 6: - credentials_replaced_since_wipe now inspects all three token ciphertexts (access/refresh/id): a legal legacy replacement may carry an empty refresh token while providing fresh access/id material, and a refresh-only check would mistake it for the original wipe and finalize a freshly replaced account. - replace_account_assignments acquires the account FOR SHARE locks BEFORE deleting the key's assignment rows, matching begin_delete's account-then-assignment lock order; the previous order (assignment-row delete first, account lock second) formed a cycle with a concurrent begin_delete and deadlocked on PostgreSQL instead of serializing. OpenSpec delta/design updated; regression test for the empty-refresh legacy replacement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(accounts): make supersede-after-partial-drain folded end state an explicit contract Address local codex review round 7: the design claimed the mid-drain folded/raw divergence was 'bounded by drain duration', which is false when a supersede lands after a partial drain — finalization's mirrors never run and rows drained before the replacement keep folded attribution under the revived account permanently. That end state is historically correct (the folded numbers pre-existed the delete; nothing is added or inflated) and cannot double- or under-count a read: below-watermark reads are folded-only, drained below-watermark rows are never re-folded, and drained above-watermark rows fold exactly once under the orphaned dimension. Reconciling at supersede time was rejected — it would drag the fold lock and per-row delta mirroring into every credential-replacement path to 'fix' attribution that is already correct. Design D3 + Risks and the spec supersede requirement now state this contract explicitly, and a regression test pins it (folded counts unchanged by later folds after a partial-drain supersede; raw rows stay detached). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): self-heal unfenced-replica drift per chunk, fast repeat-delete short-circuit, deterministic tests Address local codex review round 8: - Every drain chunk transaction now self-heals drift written by pre-upgrade replicas during a rolling deploy (their writers carry no marker fence): when the marked row's status was replaced (e.g. by a late 429 settlement) or an API-key assignment was recreated, and the token ciphertext is still wiped (i.e. no credential replacement), the chunk re-asserts DEACTIVATED/pending_deletion and re-removes the assignments under the row lock it already holds — bounding any mixed-version drift to one chunk transaction. A DB trigger was rejected as disproportionate. - Repeat DELETE requests short-circuit on an unlocked marker+wipe read before the writer section / row lock, keeping the millisecond fast-path contract while a drain chunk holds the account row for seconds. The short-circuit falls through to the full (re-wipe, re-arm) path when credentials were replaced without clearing the marker. - Deletion tests neutralize the scheduler's startup/interval tick (the async_client lifespan starts the real worker with an inline leader election), removing the race between a tick and mark-state assertions. OpenSpec delta/design/tasks updated; regression tests for the per-chunk self-heal and the non-blocking repeat delete (pg row-lock interleaving). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): 404 every ID-based route for marked accounts, filter unscoped API-key pool Address local codex review round 9: - Every ID-based account route now treats a marked account as absent, as the synchronous delete did once the row was gone: reads (trends, reset-credit views) and action routes (pause, probe, reset-credit consume) go through a marker-aware fetch; mutations (account update, alias, limit-warmup, routing policy) gain an atomic delete_requested_at IS NULL write predicate. Only credential-replacement paths may address the marked row. - ApiKeysRepository.list_all_accounts (unscoped pooled-usage projections and /v1/usage) filters the marker as well: status alone is not enough while unfenced pre-upgrade replicas can briefly replace the terminal status during a rolling deploy. OpenSpec delta/tasks updated; route sweep added to the immediate-mark integration test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(accounts): fence remaining account-ID surfaces, propagate drift-repair invalidation, add migration round-trip test Address local codex review round 10: - Remaining ID-based account surfaces now treat marked rows as absent: dashboard rate-limit reset-credit read/consume routes 404, the settings upstream-proxy binding route reports not-found instead of mutating a deleted account's binding, and /v1 reset-credit redemption treats the marked account exactly like one outside the API-key pool (its credentials are wiped anyway). - After a drain chunk repairs pre-upgrade-replica drift (status resurrection / recreated assignment), the worker now propagates the same cache invalidation as the delete request itself, so replicas that cached the drift stop selecting the wiped account or honoring the stale assignment. - Alembic round-trip coverage for 20260816_000000_add_account_pending_deletion: parent -> revision -> downgrade -> guarded upgrade (pre-existing column) -> head, asserting the marker columns and the partial queue index at each step; wired into the PostgreSQL CI target list. OpenSpec delta/tasks updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(accounts): scope the first-request-wins variant invariant to upgraded replicas Address local codex review round 11: the spec stated first-request-wins unqualified, contradicting the design's own rolling-upgrade rule that a delete handled by a pre-upgrade replica is simply the legacy synchronous delete — whose caller-provided delete_history variant can differ from the frozen first-request choice during the mixed deploy window. The invariant is now explicitly scoped to replicas running this revision, with the mixed-window caveat and the rejection rationale documented: new code cannot retrofit a fence into binaries that predate the marker columns, a DB trigger is disproportionate for a window bounded by the deploy, and a feature gate would add permanent configuration for a transient condition (the production single-replica topology has no mixed window at all). The legacy delete remains a complete, fold-locked, mirror-correct deletion — only the history-policy choice can diverge, and only under contradictory operator repeats inside the window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(db): refuse pending-deletion migration downgrade while deletions are queued Address local codex review round 12: the marker columns are the deletion queue's only durable state — downgrading while a background deletion is pending would silently abandon an acknowledged deletion and hand the parent build unusable (credential-wiped, partially drained) account rows it lists again. The downgrade now raises with the pending count and instructions (let the worker finish, or supersede via credential re-import) instead of dropping the columns; the round-trip migration test covers the refusal and the subsequent clean downgrade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(accounts): pin deletion chunk scans to account-leading indexes, bound chunk transactions The chunk batch subquery (account_id = :id LIMIT n, no ORDER BY) planned as a LIMIT-terminated Seq Scan on the production planner for exactly the large accounts the background drain targets (verified: 606,970-row account -> Seq Scan on all three drain tables): equality folds account_id into a constant, so no pathkey forces the account index and the planner bets on uniformly interleaved matches. That bet loses mid-drain (detached rows form a growing dead prefix the scan must skip) and catastrophically once a table is drained but statistics are stale — every empty probe became a full heap scan, and usage tables were re-probed EVERY round. - Select each batch with an account_id >= :id AND account_id <= :id range (same rows, but account_id survives as the leading sort pathkey) ordered by the target index's exact column order, making the account-leading index the only sort-free plan: idx_usage_account_time, ix_additional_usage_distinct_labels, and the covering idx_logs_account_kind_deleted_latest (index-only scan). Verified against the production planner: all three chunks and the drained-table probes now run as index (or index-only) scans with the account range as Index Cond. - Stop re-probing tables already observed empty within the same pass (rows settling mid-drain are converged by finalization's residual sweep); each probe was a full account-row-locking transaction per table per round. - DELETE_BATCH_SIZE 5k -> 1k: each chunk holds the account row FOR NO KEY UPDATE for its full duration, and the design's own measured detach rate (~23s/10k request_logs) put 5k chunks at ~11.5s, not "a few seconds"; 1k bounds the worst table at ~2.3s so supersedes and fenced settlements wait at most that long. - Pause between row-touching rounds proportionally to round duration (capped) so a multi-hundred-chunk drain leaves the 2-vCPU database headroom instead of running chunk transactions back-to-back. Regression coverage: a structural test pins the range predicate + index-order ORDER BY of every batch builder against the model indexes, a PostgreSQL plan test asserts the batch shape is served by the pinned indexes without a sort or heap scan (including the drained-probe case), and a pass-level test asserts drained tables are probed exactly once per pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: fix ty diagnostics in batch pinning helpers Precise Callable/Table annotations plus index.columns (typed) instead of index.expressions (str union) so the type gate passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent c1caa44 commit d4f9e23

19 files changed

Lines changed: 2979 additions & 28 deletions

File tree

Makefile

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ POSTGRES_PYTEST_TARGETS := \
2020
tests/integration/test_db_commit_durability.py \
2121
tests/test_request_logs_options_api.py \
2222
tests/integration/test_account_usage_rollup.py \
23+
tests/integration/test_account_deletion_background.py \
2324
tests/integration/test_request_usage_time_rollup.py \
2425
tests/integration/test_request_usage_rollup_parity.py \
2526
tests/integration/test_migrations.py::test_request_usage_time_rollups_migration_upgrade_and_downgrade \
@@ -33,6 +34,7 @@ POSTGRES_PYTEST_TARGETS := \
3334
tests/integration/test_repositories.py::test_replace_reauthorized_discards_pending_downgrade_evidence \
3435
tests/integration/test_repositories.py::test_upsert_account_slot_discards_pending_downgrade_evidence_on_reimport \
3536
tests/integration/test_migrations.py::test_account_plan_downgrade_observations_migration_upgrade_and_downgrade \
37+
tests/integration/test_migrations.py::test_account_pending_deletion_migration_upgrade_and_downgrade \
3638
tests/integration/test_usage_repository.py::test_bulk_history_since_primary_query_plan_is_index_only_postgresql \
3739
tests/integration/test_usage_repository.py::test_bulk_history_since_cutoff_query_plan_is_index_only_postgresql \
3840
tests/integration/test_usage_repository.py::test_bulk_history_since_secondary_query_plan_is_index_only_postgresql \
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
"""add account pending-deletion marker columns
2+
3+
Revision ID: 20260816_000000_add_account_pending_deletion
4+
Revises: 20260812_120000_add_sticky_abandonment_scope
5+
Create Date: 2026-08-16
6+
"""
7+
8+
from __future__ import annotations
9+
10+
import sqlalchemy as sa
11+
from alembic import op
12+
13+
revision = "20260816_000000_add_account_pending_deletion"
14+
down_revision = "20260812_120000_add_sticky_abandonment_scope"
15+
branch_labels = None
16+
depends_on = None
17+
18+
_TABLE = "accounts"
19+
_INDEX = "idx_accounts_delete_requested_at"
20+
21+
22+
def _columns(bind) -> set[str]:
23+
return {column["name"] for column in sa.inspect(bind).get_columns(_TABLE)}
24+
25+
26+
def _indexes(bind) -> set[str]:
27+
return {index["name"] for index in sa.inspect(bind).get_indexes(_TABLE)}
28+
29+
30+
def upgrade() -> None:
31+
bind = op.get_bind()
32+
columns = _columns(bind)
33+
if "delete_requested_at" not in columns:
34+
op.add_column(_TABLE, sa.Column("delete_requested_at", sa.DateTime(), nullable=True))
35+
if "delete_history_requested" not in columns:
36+
op.add_column(
37+
_TABLE,
38+
sa.Column(
39+
"delete_history_requested",
40+
sa.Boolean(),
41+
nullable=False,
42+
server_default=sa.false(),
43+
),
44+
)
45+
if _INDEX not in _indexes(bind):
46+
# Pending-deletion queue probe/order support; partial so it is empty
47+
# (and free) in the steady state with no pending deletions.
48+
op.create_index(
49+
_INDEX,
50+
_TABLE,
51+
["delete_requested_at", "id"],
52+
postgresql_where=sa.text("delete_requested_at IS NOT NULL"),
53+
sqlite_where=sa.text("delete_requested_at IS NOT NULL"),
54+
)
55+
56+
57+
def downgrade() -> None:
58+
bind = op.get_bind()
59+
columns = _columns(bind)
60+
if "delete_requested_at" in columns:
61+
# The marker columns are the deletion queue's only durable state:
62+
# dropping them while deletions are queued would silently abandon
63+
# acknowledged deletions and hand the parent build unusable
64+
# (credential-wiped, partially drained) account rows it would list
65+
# again. Refuse instead — let the worker finish (or supersede the
66+
# deletions via re-import/reauth) before downgrading.
67+
pending = bind.execute(
68+
sa.text(f"SELECT COUNT(*) FROM {_TABLE} WHERE delete_requested_at IS NOT NULL") # noqa: S608
69+
).scalar()
70+
if pending:
71+
raise RuntimeError(
72+
f"cannot downgrade {revision}: {pending} account(s) are still queued for "
73+
"background deletion; wait for the deletion worker to finish (or supersede "
74+
"the deletions with a credential re-import) before downgrading"
75+
)
76+
if _INDEX in _indexes(bind):
77+
op.drop_index(_INDEX, table_name=_TABLE)
78+
if "delete_history_requested" in columns:
79+
op.drop_column(_TABLE, "delete_history_requested")
80+
if "delete_requested_at" in columns:
81+
op.drop_column(_TABLE, "delete_requested_at")

app/db/models.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,20 @@ class Account(Base):
137137
server_default=false(),
138138
nullable=False,
139139
)
140+
# Pending-deletion marker: set by the fast DELETE path, consumed by the
141+
# background deletion worker, cleared only by a credential replacement
142+
# (re-import/reauth) that supersedes the deletion. Non-NULL rows are
143+
# hidden from account listings and are already unroutable (the fast path
144+
# also sets status=DEACTIVATED).
145+
delete_requested_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True)
146+
# Frozen at the first delete request (repeat requests do not escalate):
147+
# True selects the history-deleting variant in the background worker.
148+
delete_history_requested: Mapped[bool] = mapped_column(
149+
Boolean,
150+
default=False,
151+
server_default=false(),
152+
nullable=False,
153+
)
140154

141155
api_key_assignments: Mapped[list["ApiKeyAccountAssignment"]] = relationship(
142156
"ApiKeyAccountAssignment",
@@ -2121,6 +2135,17 @@ class HttpBridgeRetryCircuit(Base):
21212135
postgresql_include=["used_percent", "reset_at", "window_minutes", "id"],
21222136
)
21232137
Index("idx_accounts_email", Account.email)
2138+
# Pending-deletion queue: every replica probes ``delete_requested_at IS NOT
2139+
# NULL LIMIT 1`` each worker interval and the leader orders the queue by
2140+
# (delete_requested_at, id); the partial index keeps both reads off the full
2141+
# accounts table and is empty in the steady state (no pending deletions).
2142+
Index(
2143+
"idx_accounts_delete_requested_at",
2144+
Account.delete_requested_at,
2145+
Account.id,
2146+
postgresql_where=text("delete_requested_at IS NOT NULL"),
2147+
sqlite_where=text("delete_requested_at IS NOT NULL"),
2148+
)
21242149
Index("idx_api_keys_name", ApiKey.name)
21252150
Index("idx_logs_account_time", RequestLog.account_id, RequestLog.requested_at)
21262151
Index("idx_logs_model_source_time", RequestLog.model_source_id, RequestLog.requested_at)

app/main.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
from app.core.utils.time import utcnow
6464
from app.db.session import SessionLocal, close_db, close_session, init_background_db, init_db
6565
from app.modules.accounts import api as accounts_api
66+
from app.modules.accounts.deletion import build_account_deletion_scheduler
6667
from app.modules.accounts.repository import AccountsRepository
6768
from app.modules.accounts.usage_rollup_scheduler import build_account_usage_rollup_scheduler
6869
from app.modules.api_keys import api as api_keys_api
@@ -488,6 +489,7 @@ async def lifespan(app: FastAPI):
488489
automations_scheduler = build_automations_scheduler()
489490
rate_limit_reset_credits_scheduler = build_rate_limit_reset_credits_scheduler()
490491
account_usage_rollup_scheduler = build_account_usage_rollup_scheduler()
492+
account_deletion_scheduler = build_account_deletion_scheduler()
491493
data_retention_scheduler = build_data_retention_scheduler()
492494
telemetry_scheduler = build_telemetry_scheduler()
493495
start_live_usage_ingestor()
@@ -501,6 +503,7 @@ async def lifespan(app: FastAPI):
501503
await automations_scheduler.start()
502504
await rate_limit_reset_credits_scheduler.start()
503505
await account_usage_rollup_scheduler.start()
506+
await account_deletion_scheduler.start()
504507
await data_retention_scheduler.start()
505508
await telemetry_scheduler.start()
506509
if settings.metrics_enabled and PROMETHEUS_AVAILABLE:
@@ -718,6 +721,7 @@ async def _activate_bridge_membership(svc: RingMembershipService, iid: str) -> N
718721
await stop_live_usage_ingestor()
719722
await rate_limit_reset_credits_scheduler.stop()
720723
await account_usage_rollup_scheduler.stop()
724+
await account_deletion_scheduler.stop()
721725
await data_retention_scheduler.stop()
722726
await telemetry_scheduler.stop()
723727
# Release the scheduler leader lease only after every leader-gated

0 commit comments

Comments
 (0)