fix(pubsub): fix inefficient trigger id generation - #1531
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds xxhash import and a new SubscriptionDataSource.HashTriggerInput method; Resolver.prepareTrigger now accepts the data source and uses its HashTriggerInput to seed the trigger hash before finalizing triggerID; both sync and async subscription resolvers pass the source into prepareTrigger. ChangesSubscription Trigger Hashing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/resolve/resolve.go`:
- Around line 1283-1295: The custom hasher path
(SubscriptionTriggerHasher.ProvideTriggerHashInput) may have written partial
bytes into keyGen before returning an error, so the fallback that writes raw
input into the same keyGen will mix partial custom data with raw input; to fix
it, ensure keyGen is reset to a clean state before performing the raw-input
fallback: detect when ProvideTriggerHashInput returns a non-nil error and either
call keyGen.Reset()/Reinitialize the hasher or create a fresh hasher instance
before executing keyGen.Write(input), keeping the existing usedHasher boolean
logic intact (refer to ProvideTriggerHashInput, SubscriptionTriggerHasher and
keyGen).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6fdbf3e-8197-40db-ae84-25f14fecf445
📒 Files selected for processing (2)
v2/pkg/engine/resolve/datasource.gov2/pkg/engine/resolve/resolve.go
All subscription datasources now implement the new method
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/resolve/resolve.go`:
- Line 1281: The call to source.TriggerIDInput currently discards its returned
error; update prepareTrigger to capture and propagate that error (e.g., err :=
source.TriggerIDInput(ctx, input, keyGen); if err != nil { return nil, err } or
return err consistent with prepareTrigger's signature) and then update its
callers (the functions that invoke prepareTrigger) to handle the propagated
error (either return it up the stack or log with context and continue). If you
prefer not to change the signature, at minimum log the error with context
(including trigger/identity info) instead of using a blank assignment; reference
source.TriggerIDInput and prepareTrigger to locate the call and adjust the two
call sites that currently assume no error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e916087f-38d7-42a1-a5f4-32ade3354538
📒 Files selected for processing (5)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/resolve/datasource.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolver_subscription_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/resolve/datasource.go`:
- Around line 21-23: prepareTrigger currently ignores the error returned by
HashTriggerInput (call site: _ = source.HashTriggerInput(...)), violating the
contract; change prepareTrigger to capture the error from
source.HashTriggerInput(ctx, input, xxh), return that error (or wrap it with
context) and abort subscription creation so triggerID is not computed when
HashTriggerInput fails; update prepareTrigger's signature and its callers to
propagate the error up the call chain (or, alternatively, if failures are
impossible, remove the error return from HashTriggerInput and update its
implementations accordingly), referencing the HashTriggerInput method and
prepareTrigger function and ensuring any code computing triggerID only runs when
no error is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33546748-5a1b-486f-aee2-26023a45b9b3
📒 Files selected for processing (5)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/resolve/datasource.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/resolve_test.gov2/pkg/engine/resolve/resolver_subscription_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/resolve/resolve_test.go
- v2/pkg/engine/resolve/resolve.go
Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [2.4.6](v2.4.5...v2.4.6) (2026-06-12) ### Bug Fixes * **pubsub:** fix inefficient trigger id generation ([#1531](#1531)) ([595681c](595681c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: wundergraph-bot[bot] <285992168+wundergraph-bot[bot]@users.noreply.github.com>
Backwards compatible change, to let datasources implement the new interface
SubscriptionTriggerHasher. If a datasource implements this interface the methodProvideTriggerHashInputis used by the subscription trigger generation to create a trigger id, instead of theinput.inputcontains various data and it might not make sense for every data source to use it because it contains more data than needed for proper trigger deduplication. One such example is the pubsub datasource, implemented by the router.If a datasource does not implement this optional interface it falls back to the way it is before this change: Use
input.Tests are provided in the router pull request, which uses the new interface.
wundergraph/cosmo#2950
@coderabbitai summary
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.