Skip to content

feat: migrate to V4 binary topic format#94

Merged
neekolas merged 6 commits into
mainfrom
04-03-add_support_for_new_topic_formats
Apr 8, 2026
Merged

feat: migrate to V4 binary topic format#94
neekolas merged 6 commits into
mainfrom
04-03-add_support_for_new_topic_formats

Conversation

@neekolas

@neekolas neekolas commented Apr 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Migrate the notification server from V3 string-based topic identifiers (e.g., /xmtp/mls/1/g-{hex}/proto) to V4 binary topics as defined by xmtpd/pkg/topic. This enables support for V4 clients while maintaining full backwards compatibility for existing V3 consumers.

Stack: This PR is stacked on #93 (Go 1.26 upgrade), which is a prerequisite for the xmtpd dependency.

What changed

New: Topic conversion module (pkg/topics/)

  • ParseV3Topic() — converts V3 string topics to *topic.Topic
  • TopicToString() — converts *topic.Topic back to V3 string representation
  • GetMessageTypeFromTopic() — extracts message type from binary topic kind
  • Removed dead code: GetMessageType(), messageTypeByPrefix, HmacUpdates

Proto: New binary topic fields

  • Subscription.topic_bytes (field 4) — binary topic, takes precedence over string topic
  • SubscribeRequest.topics_bytes (field 3) — merged with string topics
  • UnsubscribeRequest.topics_bytes (field 3) — merged with string topics
  • Existing field numbers are unchanged — fully wire-compatible

Database: TEXT → BYTEA migration

  • Migration 00003: Renames topic to topic_legacy, adds topic BYTEA, converts conforming V3 topics inline using lower() for hex normalization, deletes non-conforming rows, deduplicates collapsed case variants, recreates indexes
  • Migration 00004: Drops topic_legacy column
  • SQLC schema and queries updated for BYTEA parameters

Core interfaces (pkg/interfaces/)

  • Subscription.Topic changed from string to *topic.Topic
  • Added Subscription.TopicString for backwards-compatible JSON serialization
  • Custom MarshalJSON emits both "topic" (string) and "topicBytesB64" (base64)
  • Subscriptions interface methods now accept []*topic.Topic

API layer (pkg/api/)

  • normalizeTopics() merges string + binary topic inputs with deduplication
  • normalizeSubscriptionInputs() handles SubscribeWithMetadata with bytes-takes-precedence logic
  • Invalid topics return InvalidArgument with descriptive error messages

Listener (pkg/xmtp/)

  • processEnvelope() converts V3 string topics to binary before subscription lookup
  • Fast-path strings.HasPrefix check before expensive parsing
  • Non-convertible topics logged at Info level and dropped (not an error)
  • getContext() now uses GetMessageTypeFromTopic() for message classification

Delivery services (pkg/delivery/)

  • APNS and FCM payloads include both "topic" (reconstructed V3 string) and "topicBytesB64" (base64-encoded binary)
  • HTTP delivery gets both fields automatically via Subscription.MarshalJSON()

Important design considerations

Intentional compatibility break for non-conforming topics

Non-conforming topics (e.g., non-hex identifiers like test_installation) are deleted from the database during migration and rejected by the API going forward. This is acceptable because non-conforming topics cannot be delivered on the V4 network. Existing tests were updated to use valid hex identifiers.

Hex case normalization

The migration applies lower() inline during conversion without modifying topic_legacy at rest. This means two rows that differ only by hex casing (e.g., g-ABCD vs g-abcd) collapse to the same binary topic. A deduplication step (keep newest by ID) runs before recreating the unique index.

String topic fidelity in delivery payloads

The "topic" string in delivery payloads is now reconstructed from the binary topic via TopicToString(), rather than forwarded verbatim from Envelope.ContentTopic. This means the string will always be lowercase hex. Consumers should treat topic strings as case-insensitive and prefer "topicBytesB64" when available.

Envelope passthrough

The raw Envelope (containing string ContentTopic) is still passed through in SendRequest.Message for payload delivery and idempotency key generation. Only subscription lookups and message classification use the binary topic.

TopicString is stored, not computed

Subscription.TopicString is populated once when reading from DB via TopicToString() and stored on the struct, rather than computed on each access. This avoids repeated hex encoding in delivery hot paths where multiple services read the same subscription.

Test plan

  • 70 tests pass (go test -p 1 ./...)
  • golangci-lint run — no issues
  • go build ./... and ./dev/build — binary compiles
  • Topic conversion: 14 tests covering ParseV3Topic, TopicToString, GetMessageTypeFromTopic
  • Migration: data conversion test with conforming, non-conforming, and case-duplicate rows
  • SQLC: BYTEA round-trip test
  • Subscriptions: 9 tests including nil topic guard
  • API: 19 tests including string topics, bytes topics, merged/deduped, invalid, empty, SubscribeWithMetadata precedence
  • Listener: 4 tests with binary topic conversion and non-convertible topic drop
  • Delivery: APNS and FCM payload tests verifying dual topic fields
  • Interfaces: JSON marshaling tests for both populated and nil topic cases

🤖 Generated with Claude Code

Note

Migrate subscription topics from text to binary V4 format

  • Adds topic_bytes / topics_bytes binary fields to the Subscription, SubscribeRequest, and UnsubscribeRequest proto messages, allowing clients to pass V4 binary topics alongside or instead of string topics.
  • Adds DB migration 00004_binary_topics.up.sql that converts subscriptions.topic from TEXT to BYTEA, parsing V3 topic strings into kind-prefixed bytes, deleting non-convertible rows, and deduplicating on (installation_id, topic).
  • Updates the Subscriptions interface and all implementations so Subscribe, Unsubscribe, and GetSubscriptions accept *topic.Topic objects instead of strings; SQL queries are updated to use bytea[] parameters throughout.
  • Adds topics.ParseV3Topic and topics.TopicToString helpers for converting between string and structured topic representations; removes the old GetMessageType(envelope) function in favour of GetMessageTypeFromTopic(*topic.Topic).
  • Updates APNS and FCM delivery to populate the notification topic field from Subscription.Topic rather than envelope.ContentTopic.
  • Risk: the up migration deletes rows whose topic cannot be parsed as a valid V3 topic string and collapses duplicates, which is irreversible without the down migration.

Macroscope summarized c7d7e68.

@neekolas
neekolas requested a review from a team as a code owner April 4, 2026 02:35
Comment thread pkg/db/migrations/00004_binary_topics.up.sql Outdated
Comment thread pkg/subscriptions/subscriptions.go
Comment thread pkg/db/migrations/migrations.go
@macroscopeapp

macroscopeapp Bot commented Apr 4, 2026

Copy link
Copy Markdown

Approvability

Verdict: Needs human review

This PR implements a significant migration from string-based to binary topic format, involving a database schema migration that transforms existing data, changes to core subscription interfaces, new API fields, and a new external dependency. These are substantial runtime behavior changes with schema impact. Additionally, there is an unresolved review comment identifying a potential bug in error handling.

You can customize Macroscope's approvability policy. Learn more.

Comment thread pkg/api/api_test.go
@neekolas neekolas mentioned this pull request Apr 4, 2026
3 tasks
@neekolas
neekolas force-pushed the 04-03-add_support_for_new_topic_formats branch from 0faf45b to 8043a6b Compare April 4, 2026 03:33
@neekolas
neekolas force-pushed the 04-03-upgrade-go-1.26 branch from d1cc14b to 529e2f5 Compare April 4, 2026 03:33
Comment thread pkg/db/migrations/00005_drop_topic_legacy.down.sql Outdated
Comment thread pkg/db/migrations/00004_binary_topics.down.sql
@neekolas
neekolas force-pushed the 04-03-add_support_for_new_topic_formats branch from d3d3967 to 2e9f455 Compare April 4, 2026 06:17
@neekolas
neekolas changed the base branch from 04-03-upgrade-go-1.26 to graphite-base/94 April 5, 2026 17:54
@neekolas
neekolas force-pushed the 04-03-add_support_for_new_topic_formats branch 2 times, most recently from 8c776b6 to 70db0e2 Compare April 5, 2026 17:56
@neekolas
neekolas force-pushed the 04-03-add_support_for_new_topic_formats branch from 70db0e2 to c7d7e68 Compare April 8, 2026 14:44
@neekolas
neekolas changed the base branch from graphite-base/94 to 04-03-test_infra_cleanup April 8, 2026 14:44

neekolas commented Apr 8, 2026

Copy link
Copy Markdown
Collaborator Author

Merge activity

  • Apr 8, 5:03 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 8, 5:05 PM UTC: Graphite rebased this pull request as part of a merge.
  • Apr 8, 5:05 PM UTC: @neekolas merged this pull request with Graphite.

@neekolas
neekolas changed the base branch from 04-03-test_infra_cleanup to graphite-base/94 April 8, 2026 17:04
@neekolas
neekolas changed the base branch from graphite-base/94 to main April 8, 2026 17:04
neekolas and others added 4 commits April 8, 2026 17:04
Replace V3 string-based topic identifiers with V4 binary topics across
the entire notification pipeline. Adds parallel bytes fields to the
protobuf API for backwards compatibility, converts the database topic
column from TEXT to BYTEA via migration, and updates all internal
services to work exclusively with *topic.Topic from xmtpd.

- Add ParseV3Topic, TopicToString, GetMessageTypeFromTopic conversions
- Add topic_bytes/topics_bytes fields to proto definitions
- Migration 00003: convert TEXT topics to BYTEA with hex normalization
- Migration 00004: drop legacy topic_legacy column
- Update Subscription/SubscriptionInput types to use *topic.Topic
- Add topic normalization and validation at API boundary
- Update listener to convert string topics to binary before processing
- Add dual topic fields (string + base64 bytes) to APNS/FCM delivery
- Remove dead code: GetMessageType, HmacUpdates, isV3Topic

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Renumber migrations: 00003→00004 (binary_topics), 00004→00005 (drop_topic_legacy)
- Adapt subscriptions service to new batch query + RunInTx pattern
- Update SQLC batch queries for bytea[] topic parameters
- Fix installations test to use *topic.Topic
- Remove deleted schema.sql (SQLC now reads from migrations)
- Regenerate SQLC queries

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion

Combine migrations 00004 and 00005 into a single 00004_binary_topics
migration since the two-step split adds no value for a stop-and-restart
deployment model.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
neekolas and others added 2 commits April 8, 2026 17:04
- Migration regex: use ([0-9a-f]{2})+ to reject odd-length hex strings
- Add nil guard to GetSubscriptions for *topic.Topic parameter
- Strengthen Unsubscribe_BytesTopics test: assert topic kind and bytes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move test/helpers.go to pkg/testutils/db.go, add TestLogger and
MustParseTopic helpers, convert topics_test.go to table-driven tests,
standardize t.Cleanup in api_test.go, replace logging.CreateLogger
with zaptest in all Branch 1 test files.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@neekolas
neekolas force-pushed the 04-03-add_support_for_new_topic_formats branch from c7d7e68 to 3dac7c0 Compare April 8, 2026 17:04
@neekolas
neekolas merged commit 1772f8a into main Apr 8, 2026
8 checks passed
@neekolas
neekolas deleted the 04-03-add_support_for_new_topic_formats branch April 8, 2026 17:05
Comment thread pkg/xmtp/listener.go

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Low

At line 192, return err returns the last err value from the loop, but this is wrong in two ways: (1) if a deliver call fails and logs an error, execution continues and the stale err is returned at the end, causing startMessageWorkers to log a duplicate error for a failure that was already handled; (2) if the final iteration skips delivery via shouldDeliver (hitting continue), err still holds the error from a previous iteration's failed deliver, so processEnvelope returns a stale error for a message that was otherwise processed successfully.

-	return err
+	return nil
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file pkg/xmtp/listener.go around line 192:

At line 192, `return err` returns the last `err` value from the loop, but this is wrong in two ways: (1) if a `deliver` call fails and logs an error, execution continues and the stale `err` is returned at the end, causing `startMessageWorkers` to log a duplicate error for a failure that was already handled; (2) if the final iteration skips delivery via `shouldDeliver` (hitting `continue`), `err` still holds the error from a *previous* iteration's failed `deliver`, so `processEnvelope` returns a stale error for a message that was otherwise processed successfully.

Evidence trail:
pkg/xmtp/listener.go lines 180-192 (loop with deliver and return err), pkg/xmtp/listener.go lines 127-139 (startMessageWorkers logs returned error). Reviewed at REVIEWED_COMMIT.

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.

2 participants