fix(cloud): close the gaps the account-deletion review turned up

Eight findings from re-reading the erasure path, each one something that would
have shown up in production rather than in a test:

- The local purge missed two stores. SaaSShorts outputs (output/saas_<id>) and
  generated thumbnails record ownership in their own in-memory dicts, not in
  the .owner file, so neither was erased. Thumbnails are the serious half: the
  hourly sweep deliberately skips their directory, so a deleted user's
  generated images stayed on disk forever, publicly served at /thumbnails/.
- The purge ran on the event loop. rmtree over gigabytes of video stalled every
  other request on the process; it goes through asyncio.to_thread now.
- Paths from those dicts are now resolved through _safe_under, so a session id
  or output_dir containing ".." deletes nothing.
- Deleting a user made a webhook path reachable that never was: _apply_topup
  takes the user id from Stripe metadata, so a top-up completing around the
  deletion inserted a row pointing at a user that no longer exists. The FK
  violation 500s the webhook, and since the event is only recorded after
  handle_event returns, Stripe retries the same doomed insert for three days.
  It confirms the row exists first.
- The "why are you leaving" box was free text stored in a record that
  deliberately outlives the account, which is how personal data ends up in one
  by accident. It is a closed list now, which also makes twenty deletions a
  month countable instead of readable.
- Two concurrent deletes wrote two erasure records and sent two goodbye emails.
  The DELETE's rowcount now decides which call is the real one; the UI holds a
  ref so a double click doesn't fire a second Stripe cancel and R2 purge.
- The delete button armed itself when the confirm box was empty, if the page
  ever rendered before /api/me resolved.
- The goodbye email claimed we keep "a one-way hash of your email and nothing
  else", which was not true of the Stripe reference beside it. Fixed in the
  email and in the privacy policy, EN and ES.

Also: skip the Upload-Post call when no managed key is configured, and say
plainly in the in-flight comment that it covers metered work only.

tests/test_account_local_purge.py covers tenant isolation across all three
stores and both traversal attempts; test_account_erasure.py gains the
confirmation gate, the API-key refusal, and the already-cancelled-at-Stripe
case that would otherwise trap a user forever.

Author: juancarlos.cavero

CLAUDE.md

@@ -217,11 +217,22 @@ added after a table shipped exists in the models but not in production.
 `tests/test_account_erasure.py` fails if a new table references `users.id`
 without joining that list.
 
+`app.py` registers a callback for the local working files, which record
+ownership three different ways: the `.owner` file clip jobs write (so jobs
+recovered from disk after a restart count too), `saas_jobs`, and
+`thumbnail_sessions`. That last one is the only thing that ever deletes
+generated thumbnails: the hourly sweep skips their directory and they are
+served publicly at `/thumbnails/`.
+
 What deliberately survives: the Stripe customer and its invoices (6-year
 retention, Spanish commercial law) and one `account_deletions` row holding a
 sha256 of the email as proof the erasure happened, itself purged after 5 years.
-`app.py` registers a callback so the local `output/` and `uploads/` working
-files go too, instead of waiting for the hourly sweep.
+The "why are you leaving" answer is a closed list (`DELETION_REASONS`), never
+free text — anything the user could type would land in a row designed to
+outlive them. Deleting users also made one webhook path reachable that never
+was before: `_apply_topup` reads the user id from Stripe metadata, so it now
+confirms the row still exists before inserting, or the FK violation makes
+Stripe retry the same doomed event for three days.
 
 ### Concurrency Model
 Async job queue with semaphore-based concurrency control. Configure via `MAX_CONCURRENT_JOBS` env var (default: 5). Jobs auto-cleanup after 1 hour.

app.py

@@ -1218,45 +1218,99 @@ async def _settle_reservation(job_id):
     except Exception as e:
         print(f"⚠️  Reservation settle error for {job_id}: {e}")
 
+def _owned_by(record, uid: str) -> bool:
+    owner = record.get('user_id') if isinstance(record, dict) else None
+    return owner is not None and str(owner) == uid
+
+
+def _rm_under(base_dir: str, relative: str):
+    """Remove a file or directory, refusing anything that escapes ``base_dir``."""
+    target = _safe_under(base_dir, relative)
+    if not target or target == os.path.realpath(base_dir):
+        return
+    if os.path.isdir(target):
+        shutil.rmtree(target, ignore_errors=True)
+    else:
+        try:
+            os.remove(target)
+        except OSError:
+            pass
+
+
 def _purge_local_jobs_for_user(user_id) -> int:
-    """Delete this user's job dirs, source uploads and in-memory job records.
+    """Delete this user's working files and in-memory records from local disk.
 
     Called by cloud/account.py when an account is erased. The durable copies
     live on R2 and are deleted there; these are the working files on the API's
     own disk, which would otherwise sit around until the one-hour cleanup sweep
-    — a long time to keep the videos of someone who just asked to be forgotten.
+    — and thumbnails not even then, because that sweep skips their directory.
+
+    Three stores, because each records ownership differently:
+      - clip jobs: the ``.owner`` file every managed job writes, so jobs
+        recovered from disk after a restart (no in-memory record) count too;
+      - SaaSShorts jobs (``output/saas_<id>``): ``saas_jobs`` only, no marker
+        file, so a restart loses the link and those age out on the sweep;
+      - thumbnail sessions (``output/thumbnails/<id>`` plus the source video in
+        ``uploads/``): likewise in-memory only.
 
-    Ownership comes from the ``.owner`` file each managed job writes, so jobs
-    recovered from disk after a restart (no in-memory record) are covered too.
+    Blocking: rmtree over gigabytes of video. Callers must run it in a thread.
     """
     uid = str(user_id)
-    job_ids = {jid for jid, job in list(jobs.items())
-               if job.get('user_id') is not None and str(job['user_id']) == uid}
+    removed = 0
+
+    job_ids = {jid for jid, job in list(jobs.items()) if _owned_by(job, uid)}
+    thumbs_dir_name = os.path.basename(THUMBNAILS_DIR)
     try:
         entries = os.listdir(OUTPUT_DIR)
     except OSError:
         entries = []
     for job_id in entries:
-        owner_path = os.path.join(OUTPUT_DIR, job_id, ".owner")
+        # Never a job, and it backs a StaticFiles mount: deleting the directory
+        # itself 500s every /thumbnails request until the process restarts.
+        if job_id == thumbs_dir_name:
+            continue
         try:
-            with open(owner_path) as f:
+            with open(os.path.join(OUTPUT_DIR, job_id, ".owner")) as f:
                 if f.read().strip() == uid:
                     job_ids.add(job_id)
         except OSError:
             continue
 
     for job_id in job_ids:
-        shutil.rmtree(os.path.join(OUTPUT_DIR, job_id), ignore_errors=True)
+        _rm_under(OUTPUT_DIR, job_id)
         jobs.pop(job_id, None)
         # Source uploads are named "<job_id>_<filename>" (see /api/process).
         for path in glob.glob(os.path.join(UPLOAD_DIR, f"{glob.escape(job_id)}_*")):
             try:
                 os.remove(path)
             except OSError:
                 pass
-    if job_ids:
-        print(f"🗑️  Purged {len(job_ids)} local job dir(s) for erased user {uid}.")
-    return len(job_ids)
+        removed += 1
+
+    for jid, job in list(saas_jobs.items()):
+        if not _owned_by(job, uid):
+            continue
+        out = job.get('output_dir')
+        if out:
+            _rm_under(OUTPUT_DIR, os.path.basename(out))
+        saas_jobs.pop(jid, None)
+        removed += 1
+
+    for sid, sess in list(thumbnail_sessions.items()):
+        if not _owned_by(sess, uid):
+            continue
+        # Generated thumbnails are served publicly at /thumbnails/<id>/... and
+        # nothing else ever deletes them.
+        _rm_under(THUMBNAILS_DIR, sid)
+        video_path = sess.get('video_path')
+        if video_path:
+            _rm_under(UPLOAD_DIR, os.path.basename(video_path))
+        thumbnail_sessions.pop(sid, None)
+        removed += 1
+
+    if removed:
+        print(f"🗑️  Purged {removed} local work item(s) for erased user {uid}.")
+    return removed
 
 
 @asynccontextmanager

cloud/account.py

@@ -22,8 +22,10 @@
 
 What deliberately survives: the Stripe customer and its invoices (six-year
 retention under Spanish commercial law, privacy policy §5) and one
-[AccountDeletion] row holding a sha256 of the email — proof the erasure
-happened, with no readable identity in it.
+[AccountDeletion] row proving the erasure happened — a sha256 of the email
+rather than the address, and no free text anywhere in it. The confirmation
+email tells the user both, because a record they were never told about is not
+accountability, it is retention.
 """
 import asyncio
 import hashlib
@@ -56,6 +58,17 @@
     Subscription, ApiKey, SignupAttribution, UploadPostProfile,
 )
 
+# The optional "why are you leaving" answer, as a closed list. It was a free
+# text box first, and that was wrong: whatever the user types lands in a record
+# that deliberately outlives their account, so a single "my name is X, please
+# confirm" turns the one row meant to hold no readable identity into retained
+# personal data. A fixed vocabulary cannot do that, and twenty deletions a month
+# are far more legible counted than read.
+DELETION_REASONS = (
+    "too_expensive", "not_using_it", "clip_quality", "missing_feature",
+    "found_alternative", "privacy", "other",
+)
+
 router = APIRouter()
 
 # Job dirs on the API's own disk belong to app.py, which cannot be imported from
@@ -141,6 +154,8 @@ async def _delete_upload_post_profile(user_id):
     import httpx
     from .social_profiles import API_BASE, _auth_headers
 
+    if not settings.managed_upload_post_key:
+        return
     async with database.session() as session:
         prof = await session.get(UploadPostProfile, user_id)
         if prof is None:
@@ -160,16 +175,31 @@ async def _delete_upload_post_profile(user_id):
         print(f"⚠️  Upload-Post profile delete failed for {username}: {e}")
 
 
-async def _erase_rows(user_id, email, stripe_customer_id, plan, r2_deleted, reason):
+async def _erase_rows(user_id, email, stripe_customer_id, plan, r2_deleted, reason) -> bool:
     """Swap the account for its erasure record, in one transaction.
 
     All-or-nothing on purpose: a half-erased account is worse than an un-erased
     one, because the user is told they are gone while some of their rows are
     not. Magic-link tokens are keyed by email rather than user id, so they need
     their own statement on top of [USER_OWNED_TABLES].
+
+    Returns whether this call is the one that removed the account. Two requests
+    can race here — a double-click, or two tabs — and everything before this
+    point is idempotent, but the erasure record is not: writing it from the
+    loser would leave two rows for one deletion, possibly disagreeing about the
+    reason. Postgres serialises the two DELETEs, so the loser sees rowcount 0
+    and writes nothing.
     """
     async with database.session() as session:
         async with session.begin():
+            for model in USER_OWNED_TABLES:
+                await session.execute(
+                    delete(model).where(model.user_id == user_id))
+            await session.execute(
+                delete(MagicLinkToken).where(MagicLinkToken.email == email))
+            result = await session.execute(delete(User).where(User.id == user_id))
+            if result.rowcount == 0:
+                return False
             session.add(AccountDeletion(
                 former_user_id=str(user_id),
                 email_sha256=email_fingerprint(email),
@@ -178,12 +208,7 @@ async def _erase_rows(user_id, email, stripe_customer_id, plan, r2_deleted, reas
                 r2_objects_deleted=r2_deleted,
                 reason=reason,
             ))
-            for model in USER_OWNED_TABLES:
-                await session.execute(
-                    delete(model).where(model.user_id == user_id))
-            await session.execute(
-                delete(MagicLinkToken).where(MagicLinkToken.email == email))
-            await session.execute(delete(User).where(User.id == user_id))
+    return True
 
 
 # --------------------------------------------------------------------------- #
@@ -194,7 +219,7 @@ class DeleteAccountRequest(BaseModel):
     # there is no password to re-enter, so this is the strongest thing we can
     # ask for without mailing a second token to a user who is leaving anyway.
     confirm_email: str
-    reason: Optional[str] = None
+    reason: Optional[str] = None   # one of DELETION_REASONS, or ignored
 
 
 @router.delete("/api/account")
@@ -211,10 +236,15 @@ async def delete_account(payload: DeleteAccountRequest, request: Request):
         if row is None:
             raise HTTPException(status_code=404, detail="Account not found.")
         email, stripe_customer_id = row.email, row.stripe_customer_id
-        # A reserved ledger row means a render is in flight. Erasing now would
-        # delete the output from under a job that is still writing it, and the
+        # A reserved ledger row means metered work is in flight. Erasing now
+        # would pull the output from under a job still writing it, and the
         # reservation would never settle. Minutes are refunded on failure, so
         # waiting costs the user nothing.
+        #
+        # This covers everything that costs minutes, which is everything long
+        # enough to matter. A free action (burning captions on an untranslated
+        # clip) writes no ledger row and so is not covered: worst case it fails
+        # on a missing file, in a tab whose session is about to end anyway.
         in_flight = (await session.execute(
             select(func.count(UsageLedger.id)).where(
                 UsageLedger.user_id == user.id, UsageLedger.status == "reserved")
@@ -249,15 +279,20 @@ async def delete_account(payload: DeleteAccountRequest, request: Request):
 
     if _local_purge is not None:
         try:
-            _local_purge(user.id)
+            # rmtree over gigabytes of video: off the event loop, or every other
+            # request on the process waits for this user's disk to clear.
+            await asyncio.to_thread(_local_purge, user.id)
         except Exception as e:
             print(f"⚠️  Local job purge failed for {user.id}: {e}")
 
     await _delete_upload_post_profile(user.id)
 
-    reason = (payload.reason or "").strip()[:500] or None
+    # Anything not in the closed list is dropped rather than rejected: a
+    # mismatched client must not be able to fail a deletion over a label.
+    reason = payload.reason if payload.reason in DELETION_REASONS else None
     try:
-        await _erase_rows(user.id, email, stripe_customer_id, plan, r2_deleted, reason)
+        erased = await _erase_rows(
+            user.id, email, stripe_customer_id, plan, r2_deleted, reason)
     except Exception as e:
         # The content is already gone; only the account row survived. Say that
         # rather than a bare 500 — the user needs to know a retry is safe and
@@ -269,6 +304,10 @@ async def delete_account(payload: DeleteAccountRequest, request: Request):
     # ``email`` was read before the delete on purpose: from here on the address
     # exists nowhere in our systems, and the goodbye email still has to go out.
 
+    if not erased:
+        # A concurrent request got there first and has already sent the email.
+        return {"deleted": True, "r2_objects_deleted": r2_deleted}
+
     print(f"🗑️  Account erased: {user.id} (plan={plan}, r2_objects={r2_deleted})")
 
     # Everything below is after-the-fact and must never fail the deletion.

cloud/billing.py

@@ -282,7 +282,20 @@ async def _apply_topup(session_obj: dict):
             )).scalar_one_or_none()
             if existing:
                 return
+            # The id rides in Stripe's metadata, so it can name an account that
+            # no longer exists: a checkout completed moments before the user
+            # erased themselves, or any webhook Stripe retries afterwards.
+            # Inserting it blind trips the foreign key, and because the event is
+            # only recorded after handle_event returns, the 500 makes Stripe
+            # retry the same doomed insert for three days.
             user_id = (session_obj.get("metadata") or {}).get("user_id")
+            if user_id:
+                try:
+                    user_id = (await s.execute(
+                        select(User.id).where(User.id == user_id)
+                    )).scalar_one_or_none()
+                except Exception:
+                    user_id = None  # not a uuid at all
             if not user_id:
                 user_id = await _user_id_for_customer(s, session_obj.get("customer"))
             if not user_id:

cloud/emails.py

@@ -156,9 +156,11 @@ async def send_account_deleted_email(email: str):
         <p>Two things we keep, and why:</p>
         <ul style="line-height:1.7;padding-left:20px">
           <li><strong>Your invoices</strong>, for six years &mdash; Spanish
-              commercial law requires it.</li>
-          <li><strong>A record that this deletion happened</strong>, holding a
-              one-way hash of your email address and nothing else.</li>
+              commercial law requires it, and the Stripe customer reference
+              that finds them goes with it.</li>
+          <li><strong>A record that this deletion happened</strong>, for five
+              years, holding a one-way hash of your email address instead of
+              the address itself.</li>
         </ul>
         <p>You can sign up again any time with the same address; it will be a
            brand-new, empty account.</p>

cloud/models.py

@@ -228,9 +228,13 @@ class AccountDeletion(Base):
 
     GDPR Art. 17 erasure has an accountability twin (Art. 5.2): if a former user
     later claims we never deleted their account, the only way to answer is a
-    record that outlives the deletion. So this holds no readable identity — a
-    sha256 of the account email, which can confirm "yes, this address was
-    deleted on this date" without storing the address itself.
+    record that outlives the deletion. The identifying field is therefore a
+    sha256 of the account email, which confirms "yes, this address was deleted
+    on this date" without storing the address itself.
+
+    ``reason`` is one label from ``account.DELETION_REASONS``, never free text:
+    anything the user could type would land in a row that deliberately outlives
+    their account, which is the opposite of what this row is for.
 
     ``stripe_customer_id`` is the one exception and it is deliberate: the
     invoices behind it must be kept for six years under Spanish commercial law,
@@ -249,5 +253,5 @@ class AccountDeletion(Base):
     stripe_customer_id = Column(Text, nullable=True)
     plan_at_deletion = Column(String(20), nullable=True)
     r2_objects_deleted = Column(Integer, nullable=True)
-    reason = Column(Text, nullable=True)                 # optional free-text the user typed
+    reason = Column(String(32), nullable=True)           # one of account.DELETION_REASONS
     deleted_at = Column(DateTime(timezone=True), server_default=func.now())

dashboard/seo/legal.js

@@ -613,7 +613,8 @@ Delete account</strong> in the dashboard does it yourself, immediately and
 permanently, taking your projects, clips, transcripts, API keys and social
 connections with it and cancelling any active subscription. Two things survive
 it, both listed in section 5: your invoices, which tax law requires us to keep,
-and a one-way hash of your email address recording that the deletion happened.
+and a record that the deletion happened, which identifies you by a one-way hash
+of your email address rather than by the address itself.
 If you believe we are mishandling your data, you can complain to the Spanish
 supervisory authority (AEPD, aepd.es) or to the authority of your own EU
 country.</p>
@@ -750,8 +751,9 @@ forma inmediata y permanente, y se lleva por delante tus proyectos, clips,
 transcripciones, claves de API y conexiones con redes sociales, además de
 cancelar cualquier suscripción activa. Solo sobreviven dos cosas, ambas
 recogidas en el apartado 5: tus facturas, que la normativa fiscal nos obliga a
-conservar, y un hash irreversible de tu dirección de email que acredita que la
-eliminación se produjo. Si crees que tratamos mal tus datos, puedes reclamar
+conservar, y un registro de que la eliminación se produjo, que te identifica
+mediante un hash irreversible de tu dirección de email y no mediante la
+dirección en sí. Si crees que tratamos mal tus datos, puedes reclamar
 ante la Agencia Española de Protección de Datos (AEPD, aepd.es) o ante la
 autoridad de tu país de la UE.</p>