Skip to content

Add Reddit FCM push registration - #5

Open
Fizzadar wants to merge 1 commit into
mainfrom
codex/reddit-push-registration
Open

Fizzadar wants to merge 1 commit into
mainfrom
codex/reddit-push-registration

Conversation

@Fizzadar

@Fizzadar Fizzadar commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

The bridge can now register wrapper-provided FCM tokens using Reddit Android 2026.41.0's RegisterMobilePushToken GraphQL operation and sender ID 933360113277. Registration persists a separate device ID, reuses it when the FCM token rotates, and retries once after refreshing expired Reddit credentials.

The APK exposes general Reddit notification registration with no chat-only filter. Document the chat notification types for receiver-side filtering and the separate token-health acknowledgement flow. Account notification preferences are unchanged.

Live checks verified authentication with a web-minted chat token and recognition of the persisted operation using deliberately incomplete requests. Successful token binding, FCM delivery, health-check acknowledgements, and logout deregistration remain unverified with a real receiving relay; health-check handling is not implemented in this registration callback.

Validation on the branch based on main:

  • go test -race ./...
  • go vet ./... and staticcheck ./...
  • go build ./cmd/reddit and git diff --check
  • Tests cover request/response handling, redirects, credential-safe errors, cancellation, persistent device identity, token rotation, and expired-credential refresh.

Related to PLAT-39279: Reverse Reddit local push notifications.

@indent

indent Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Issues

1 potential issue found:

  • Push registration saves UserLoginMetadata (including the HiddenRooms map) from the push goroutine while the sync loop mutates it without holding saveMu. This can cause a concurrent map iteration/write fatal error and crash the bridge. Guard all r.meta reads, writes and saves with one mutex.
    Found by Indent Review Agent

↑ Select any checkbox above to have Indent auto-fix the issue

CI Checks

All CI checks passed on 69e0776.

Review agents

Select any unchecked box below to run or rerun that agent.

Found issues (1)
  • Indent Review Agent · Push registration can crash the bridge via a concurrent map read/write on UserLoginMetadata.
Full results

Indent Review Agent

  • Summary: Push registration can crash the bridge via a concurrent map read/write on UserLoginMetadata.
  • Last ran on commit: 69e07769
  • Latest result
    {
      "summary": "Push registration can crash the bridge via a concurrent map read/write on UserLoginMetadata.",
      "findings": [
        {
          "fix": "Make every read and write of `r.meta` go through one mutex. Hold a single meta lock (or `saveMu`) around the sync loop's `NextBatch`/`RoomStateVersion`/`HiddenRooms` mutations and around `Save`, and have `pushDeviceID` and `refreshChatToken` take the same lock for their mutations. An alternative is to snapshot or clone the metadata under the lock before marshaling.",
          "line": 59,
          "path": "pkg/connector/push.go",
          "issue": "Before this PR, the only code that mutated and saved `r.meta` was the sync goroutine: `chatsync.go` writes `NextBatch`/`RoomStateVersion` (line 103) and the `HiddenRooms` map (lines 200-208) without holding `saveMu`. `RegisterPushNotifications` runs on a different goroutine (the bridgev2 push-registration request). From there, `pushDeviceID` calls `userLogin.Save` (line 59), and the auth retry calls `refreshChatToken` -> `saveMeta`, which also writes `Credentials`, `ChatTokenExpiry` and `CookiesJSON`. `Save` JSON-marshals the whole `UserLoginMetadata`, including `HiddenRooms`. `saveMu` only serializes saves against each other, not against the sync loop's writes. If push registration runs while `handleSync` is adding or deleting a hidden room (a first registration with no device ID yet, or any registration that hits a 401), Go hits `concurrent map iteration and map write`. That is a fatal runtime error and takes down the whole bridge process. The scalar fields race in the same way, and push's save can also persist a sync cursor that the sync loop later rolls back in memory.",
          "title": "Push registration saves user-login metadata concurrently with the sync loop's unlocked map writes",
          "severity": "critical"
        }
      ]
    }

Comment thread pkg/connector/push.go
defer r.saveMu.Unlock()
if r.meta.PushDeviceID == "" {
r.meta.PushDeviceID = uuid.NewString()
if err := r.userLogin.Save(ctx); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Bug · Push registration saves user-login metadata concurrently with the sync loop's unlocked map writes

userLogin.Save marshals the whole UserLoginMetadata, including the HiddenRooms map, but saveMu only serializes saves against each other. The sync loop in chatsync.go mutates NextBatch, RoomStateVersion and HiddenRooms without holding it. RegisterPushNotifications runs on a separate goroutine, so a first registration, or any registration that refreshes credentials via saveMeta, can overlap a sync write. That can hit Go's fatal concurrent map iteration and map write and crash the bridge. It can also persist a sync cursor that the sync loop later rolls back in memory. Guard all r.meta access with one mutex, or clone the metadata under the lock before marshaling.

Ref: pkg/connector/chatsync.go lines ~103 and ~200-208.

Found by Indent Review Agent

@linear-code

linear-code Bot commented Oct 15, 2026

Copy link
Copy Markdown

PLAT-39279

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant