Skip to content

Accept duplicate replay uploads without a read-back - #833

Open
Neonforge98 wants to merge 4 commits into
developfrom
fix/replay-upload-conflict
Open

Accept duplicate replay uploads without a read-back#833
Neonforge98 wants to merge 4 commits into
developfrom
fix/replay-upload-conflict

Conversation

@Neonforge98

@Neonforge98 Neonforge98 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Problem

The cloud sync engine can get stuck re-uploading the same replay segments forever. Replay objects are content-addressed (the segment hash is part of the object key) and the storage policy grants INSERT but never UPDATE, so resuming an interrupted push re-uploads existing names and gets a duplicate rejection — Supabase reports it as HTTP 409, or as a 400 envelope carrying {"statusCode":"409","error":"Duplicate","code":"KeyAlreadyExists"}.

The old code refused to trust the rejection and confirmed existence with a HEAD read-back first. But member reads walk the session read ladder, which denies replay reads on metadata-only shares — for those sessions the probe can never succeed, so a segment that is already stored was treated as a failed upload on every engine pass, forever. In a long-lived workspace this produced a permanent per-pass error loop (cloud push failed for session …: replay object upload failed with 400 … KeyAlreadyExists) and the affected sessions' pushes never concluded.

Fix

Split the rejection classification in uploadReplayObject:

  • A duplicate-name rejection (plain 409, or a 400/403 whose body says Duplicate / KeyAlreadyExists / already exists) now counts as success outright — content addressing guarantees the stored bytes are the ones we were sending, and no read-back is required.
  • The genuinely ambiguous case — a 400/403 RLS denial, which uses the same wording for "blocked implied update on an existing name" and "you may not write here at all" — keeps the HEAD probe, so real authorization failures still surface.

Verification

  • Unit: 11 tests on the storage client, including a new regression for the exact production shape (400-wrapped KeyAlreadyExists with an unreadable object) asserting success with no probe call.
  • Live: on a workspace where three sessions had been stuck in the loop (reproducing on every pass since days ago), a build with this change runs clean — the duplicate spam is gone and pushes proceed past the previously-stuck segments.

Retention parking (second commit)

Removing the duplicate false-failure exposed the loop's other layer: sessions past the org's retention window fail their push with ORG2_RETENTION_EXPIRED, and the engine re-walked the full doomed upload chain (a dozen storage POSTs plus the rewrite RPCs) for each of them on every pass. Retention only recedes further within a signed-in run, so the retry can never succeed.

The push loop now parks a session when its push fails with ORG2_RETENTION_EXPIRED — one journal entry (session_retention_parked) plus a log line, then the session is skipped for the rest of the run. The parked set clears on resetSyncState() (sign-in cycle, endpoint switch, app restart), which is also the boundary where an entitlement upgrade would re-evaluate.

Live-verified on the same workspace: each of the three expired sessions walks its final failing push exactly once and logs cloud push parked for retention-expired session …; subsequent passes are silent — no repeated uploads, no repeated RPC failures. Unit tests cover both directions (parks on ORG2_RETENTION_EXPIRED with the journal entry; other codes keep retrying).

Desktop notification action APIs (third commit)

Every desktop launch logged two errors from the Team Inbox notification wiring: register_action_types and the action listener are mobile-only in the notification plugin — its desktop invoke_handler registers just notify/permission commands, so both calls can only fail with "Command not found". The action button they were meant to provide has never existed on desktop.

The two service entry points are now inert seams (kept for a future mobile target) instead of doomed plugin calls. Plain notifications, sounds, and the dock badge are untouched; the only observable change is that the two boot errors are gone — verified live on a fresh launch, and the Settings test notification still sends successfully through the plugin path.

Boot-time 401 storm (fourth commit)

Every cold start with an aged JWT fired a burst of unauthorized RPCs — get_cloud_capabilities (team-inbox and realtime probes) and cloud_list_team_inbox_mentions went out with the persisted, possibly-expired token before anything refreshed it, contributing a steady stream of 401s to the backend error rate on every client launch.

Both surfaces now resolve token freshness before their first request: the team-inbox mentions client routes every RPC through a freshestToken choke point (refresh + CAS write-back via the existing getFreshCloudAccessToken idiom, falling back to the passed token outside a running store), and the realtime capability probe uses the same ensureFreshSession + commitRefreshedAuth dance its sibling entitlement refresher already used.

Live-verified by force-expiring the persisted token and cold-starting: zero 401s in the boot console, and the org-session listings succeed on the silently refreshed token.

Together with the retention parking and duplicate-upload fixes above, this removes the three client-side error generators observed against the managed backend: the per-pass Postgres RPC failure storm from expired sessions, the storage duplicate-upload rejections, and the cold-start 401 burst.

Pre-commit hook ran. Total eslint: 5, total circular: 0
Pre-commit hook ran. Total eslint: 5, total circular: 0
Pre-commit hook ran. Total eslint: 5, total circular: 0
Pre-commit hook ran. Total eslint: 5, total circular: 0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant