Skip to content

feat: add dead-letter topic support with automatic IAM permissions#702

Merged
g-ongenae merged 7 commits intoalgoan:masterfrom
Workiz:feat/dead-letter-topic-support
Mar 18, 2026
Merged

feat: add dead-letter topic support with automatic IAM permissions#702
g-ongenae merged 7 commits intoalgoan:masterfrom
Workiz:feat/dead-letter-topic-support

Conversation

@nehoramoshe
Copy link
Copy Markdown
Contributor

  • Add DeadLetterOptions interface and deadLetterOptions field to GooglePubSubOptions
  • Automatically apply deadLetterPolicy when creating new subscriptions
  • Automatically grant roles/pubsub.publisher on the dead-letter topic and roles/pubsub.subscriber on the source subscription to the Pub/Sub service account
  • Resolve numeric GCP project number via Cloud Resource Manager v3 API using the existing auth client (gaxios), avoiding any new dependencies
  • Fully backward compatible: no behavior change when deadLetterOptions is not set

Made-with: Cursor

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

- Add DeadLetterOptions interface and deadLetterOptions field to GooglePubSubOptions
- Automatically apply deadLetterPolicy when creating new subscriptions
- Automatically grant roles/pubsub.publisher on the dead-letter topic and
  roles/pubsub.subscriber on the source subscription to the Pub/Sub service account
- Resolve numeric GCP project number via Cloud Resource Manager v3 API using
  the existing auth client (gaxios), avoiding any new dependencies
- Fully backward compatible: no behavior change when deadLetterOptions is not set

Made-with: Cursor
@nehoramoshe nehoramoshe requested a review from a team as a code owner March 8, 2026 12:44
Copy link
Copy Markdown
Member

@g-ongenae g-ongenae left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. At first view, the change LGTM, but could you add tests to the PR?

Copy link
Copy Markdown
Contributor

@qhdinh qhdinh left a comment

Choose a reason for hiding this comment

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

@nehoramoshe Thanks for the pull request. This is a useful feature.

However, was your intention to allow to override the dead-letter policy for each subscription? or stick with the policy initialized during GoogleCloudPubSub constructor?

Comment on lines +324 to +328
deadLetterPolicy: {
deadLetterTopic: this.deadLetterOptions.deadLetterTopicName,
maxDeliveryAttempts: this.deadLetterOptions.maxDeliveryAttempts ?? 5,
...createOptions?.deadLetterPolicy,
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder about the policy merging order. This way you allow each subscription to override the deadLetterTopic over the one of class instance this.deadLetterOptions.deadLetterTopicName

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is intentional — the spread order allows callers to override the dead-letter policy per subscription via createOptions.deadLetterPolicy if needed. The class-level config acts as a default that can be overridden at the subscription level.

: await sub.create(deadLetterCreateOptions);

if (!exists && this.deadLetterOptions !== undefined) {
await this.setupDeadLetterIamPermissions(subscription, this.deadLetterOptions.deadLetterTopicName);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But here the setup of IAM perssion always stick to the topic of class instance this.deadLetterOptions.deadLetterTopicName

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was fixed in the second commit — setupDeadLetterIamPermissions now receives the resolved topic name per subscription rather than reading from this.deadLetterOptions.deadLetterTopicName directly.

Comment on lines +376 to +386
const url = `https://cloudresourcemanager.googleapis.com/v3/projects/${projectId}`;
const response = await this.client.auth.request<{ name: string }>({ url });
const match = response.data.name.match(/^projects\/(\d+)$/);

if (match === null) {
throw new Error(
`[@algoan/pubsub] Unexpected project resource name format: ${response.data.name}`,
);
}

return match[1];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const url = `https://cloudresourcemanager.googleapis.com/v3/projects/${projectId}`;
const response = await this.client.auth.request<{ name: string }>({ url });
const match = response.data.name.match(/^projects\/(\d+)$/);
if (match === null) {
throw new Error(
`[@algoan/pubsub] Unexpected project resource name format: ${response.data.name}`,
);
}
return match[1];
const url = `https://cloudresourcemanager.googleapis.com/v3/projects/${projectId}`;
const response = await this.client.auth.request<{ projectNumber: string }>({ url });
return response.data.projectNumber;

We can get the project number directly from the API without parsing the regex from project name I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, applied in the latest commit — now using response.data.projectNumber directly instead of parsing the regex from the name field.

Comment on lines +345 to +353
const [topicPolicy] = await deadLetterTopic.iam.getPolicy();
const updatedTopicPolicy = {
...topicPolicy,
bindings: [
...(topicPolicy.bindings ?? []),
{ role: 'roles/pubsub.publisher', members: [serviceAccount] },
],
};
await deadLetterTopic.iam.setPolicy(updatedTopicPolicy);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should check if the binding exists before appending.
If this code runs for multiple subscriptions sharing the same dead-letter topic (very likely), the same (role, members) binding gets added repeatedly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit — before appending a binding we now check if the service account is already a member of that role and skip the setPolicy call if so.

@nehoramoshe nehoramoshe requested review from g-ongenae and qhdinh March 9, 2026 14:46
@nehoramoshe
Copy link
Copy Markdown
Contributor Author

@nehoramoshe Thanks for the pull request. This is a useful feature.

However, was your intention to allow to override the dead-letter policy for each subscription? or stick with the policy initialized during GoogleCloudPubSub constructor?

@qhdinh
The intent is for deadLetterOptions to be a global policy set at constructor level, applied uniformly to all subscriptions. What varies per subscription is the dead-letter topic name — each subscription automatically gets its own -deadletter topic and drain subscription. The ...createOptions?.deadLetterPolicy spread is a low-level escape hatch for edge cases but not the main use case. If per-subscription policy overrides (e.g. different maxDeliveryAttempts per subscription) become a need, that would be a separate feature added to GCSubscriptionOptions.

Copy link
Copy Markdown
Contributor

@qhdinh qhdinh left a comment

Choose a reason for hiding this comment

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

@nehoramoshe Even though you said ...createOptions?.deadLetterPolicy spread is not for the main use, putting it there during config merging mean you allow override the deadLetterTopicName per-subscription. So for example, a subscription override that and created with a new customized deadLetterTopicName, but the setupDeadLetterIamPermissions always stick with the resolvedDeadLetterTopicName as in your recent changes (either deadLetterTopicName provided in instance config at the begining, or auto-named dead letter topic name). That's a mistmatch!

I like your previous implementation better (the version with resolvedDeadLetterTopicName). What was confusing is just whether to always stick with the deadletter topic name provided in the instance config or allow each subscription to override via createOptions.deadLetterPolicy.deadLetterTopicName.

For now, to keep it simple and straighforward, I think we can just remove the spread ...createOptions.deadLetterPolicy during config merging to not allow overriding, and require deadLetterTopicName to be provided during instance initilization. Like that, both subscription creation and IAM setup will stick with deadLetterTopicName in instance config.

In the future, if we want to add support for overriding per subscription, we have to deal with the config fallback gracelly: for subscription creation and IAM setup, if deadLetterTopicName is present in creationOptions take it, otherwise fall back to deadLetterTopicName in the instance config.

Comment on lines +298 to +306
const deadLetterCreateOptions = this.buildDeadLetterCreateOptions(options.create, resolvedDeadLetterTopicName);

const [subscription]: GetSubscriptionResponse | CreateSubscriptionResponse = exists
? await sub.get(options?.get)
: await sub.create(options?.create);
: await sub.create(deadLetterCreateOptions);

if (!exists && resolvedDeadLetterTopicName !== undefined) {
await this.setupDeadLetterIamPermissions(subscription, resolvedDeadLetterTopicName);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I prefer your previous implementation. The resolveDeadLetterTopicName method and the auto-naming feature shouldn't be added, and deadLetterTopicName should go back to being required as in your original implementation.

The real value of this PR is the automatic IAM wiring — that's the painful part GCP doesn't handle. The dead-letter topic itself is infrastructure that should be the user's responsibility to prepare and provide.

The auto-creation causes more problems than it solves:

  • Silently creates GCP resources (topic + drain subscription) the user never asked for, with no configuration, no way to opt out, and no documentation of what was created.
  • Inconsistent behavior — auto-creation only happens when deadLetterTopicName is omitted
  • Auto-derived name ignores subscriptionsPrefix and environment, leading to naming inconsistencies with the rest of the resources managed by this library
  • Adds unnecessary GCP API calls on every listen() invocation, even for already-existing subscriptions

Copy link
Copy Markdown
Contributor Author

@nehoramoshe nehoramoshe Mar 11, 2026

Choose a reason for hiding this comment

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

We kept deadLetterTopicName optional intentionally to support per-subscription isolation. When omitted, each subscription automatically gets its own -deadletter topic. This turned out to be necessary — GCP itself surfaces a diagnostic warning if the dead-letter topic has no subscription, so managing that resource automatically is the right thing to do. Your other concerns about this have been addressed:

  • Naming consistency: the auto-derived name is based on subscriptionName (which already has subscriptionsPrefix and separator applied), so naming is consistent with the rest of the managed resources
  • No API calls for existing subscriptions: DLT creation now happens entirely inside the !exists block — zero overhead when reconnecting to an already-existing subscription
  • Per-subscription override: added deadLetterTopicName to GCSubscriptionOptions so callers can override per subscription when needed, falling back to instance config, then auto-derive

deadLetterPolicy: {
deadLetterTopic: resolvedDeadLetterTopicName,
maxDeliveryAttempts: this.deadLetterOptions.maxDeliveryAttempts ?? 5,
...createOptions?.deadLetterPolicy,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
...createOptions?.deadLetterPolicy,

We should remove this. So now we always stick with the deadLetterOptions in the instance config, not letting for overrding per-subscription

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — the spread has been removed. The dead-letter policy is now always driven by the instance config and cannot be overridden at the raw CreateSubscriptionOptions level.

Comment on lines +406 to +416
const subAlreadyBound = subBindings.some(
(b) => b.role === 'roles/pubsub.subscriber' && b.members?.includes(serviceAccount),
);

if (!subAlreadyBound) {
const updatedSubPolicy = {
...subPolicy,
bindings: [...subBindings, { role: 'roles/pubsub.subscriber', members: [serviceAccount] }],
};
await subscription.iam.setPolicy(updatedSubPolicy);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The subscription policy check is unnecessary because setupDeadLetterIamPermissions is only ever called inside !exists — meaning it runs once, on a brand new subscription that has zero existing bindings. There's nothing to deduplicate.

Only the topic policy needs the check because the dead-letter topic is shared across multiple subscriptions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed the deduplication check on the subscription side since setupDeadLetterIamPermissions is only ever called inside !exists, so the subscription is guaranteed to be brand new with zero existing bindings. The topic-side check is kept since the same dead-letter topic is shared across multiple subscriptions and could accumulate duplicate bindings.

*/
export interface DeadLetterOptions {
/** Optional override for the dead-letter topic name. Defaults to <subscription-name>-deadletter */
deadLetterTopicName?: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
deadLetterTopicName?: string;
deadLetterTopicName: string;

should be required if we configure DeadLetterOptions during GooogleCloudPubSub initialization.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We intentionally kept deadLetterTopicName optional. When omitted, each subscription automatically gets its own isolated dead-letter topic named -deadletter. This is actually the better default behavior — GCP itself warns in diagnostics when a dead-letter topic has no subscription, which would happen if users had to provision their own topics upfront per subscription. We also auto-create a drain subscription on the DLT to prevent message loss (GCP doesn't do this automatically).

Your original concerns about auto-naming have been addressed:

  • Naming consistency: the auto-derived name uses subscriptionName which already has subscriptionsPrefix and separator applied — so it's consistent with all other managed resources
  • No extra API calls for existing subscriptions: DLT creation lives entirely inside the !exists block — zero overhead on reconnect
  • Opt-in only: no behavior change when deadLetterOptions is not configured

If you still prefer requiring deadLetterTopicName, the per-subscription isolation would be lost unless callers wire it up manually for every subscription — which is exactly the painful boilerplate this feature is meant to eliminate.

Copy link
Copy Markdown
Contributor

@qhdinh qhdinh Mar 11, 2026

Choose a reason for hiding this comment

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

which would happen if users had to provision their own topics upfront per subscription

=> not provise their own topics upfront per subscription. What I meant was provision of one instace-level topic when we init the PubSub.

If you still prefer requiring deadLetterTopicName, the per-subscription isolation would be lost unless callers wire it up manually for every subscription — which is exactly the painful boilerplate this feature is meant to eliminate.

Maybe you misunderstood my suggestion. I didn't suggest requiring deadLetterTopicName per subscription. What I suggested was requiring a default deadLetterTopicName (if we add an object deadLetterOptions at the instance-level when we init PubSub at the beginning:

const pubsub: GCPubSub = PubSubFactory.create({
    transport: [Transport.GOOGLE](http://transport.google/)_PUBSUB,
    options: {
      projectId,
      deadLetterOptions: {,
           deadLetterTopicName: "deadletter", // required if `deadLetterOptions` is present
           maxDeliveryAttempts: 8,
    },
  });

So every subscription can rely on this common dead letter topic for their deadletters. They don't have to provide their own deadletter topic. Some subscriptions can override if they have a specified need.

I understand your method. I don't say it's wrong. It just seems a little bit overkill to me. Isn't it simpler to require providing a common deadletter topic name par default when we init PubSub?

If the default common deadletter topic name is not provided, according to your implementation, in case the system has 100 subscriptions, it will automatically create 100 corresponding deadletter topics right? I feel it could pollute the PubSub system. Do you really need to use the case when default common deadletter topic is not provided?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point — to clarify, the current implementation already supports both usage patterns natively:

  • Shared DLT (your suggestion): set deadLetterOptions.deadLetterTopicName: "my-dead-letter" → all subscriptions route failures to the same topic
  • Per-subscription DLT (our need): omit deadLetterTopicName → each subscription automatically gets its own -deadletter topic

In our system we need per-subscription isolation because different subscriptions process different message types. Replaying from a shared DLT would require consumers to filter by CloudPubSubDeadLetterSourceSubscription attribute and route each message to the correct handler — which is exactly the boilerplate this feature is meant to eliminate.

The optional deadLetterTopicName is therefore not about "not having a preference" — it's the intentional API that lets the caller choose: provide a name for shared mode, omit it for isolated mode. I'll add tests and update the README to document both patterns clearly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your clarification!
If there a real need for this use case, I'm okay with it then 😉
Yes updating the README to document how to use this feature would be great 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@qhdinh
Copy link
Copy Markdown
Contributor

qhdinh commented Mar 10, 2026

And you should ask tests for this PR as requested by @g-ongenae 😉

@nehoramoshe nehoramoshe requested a review from qhdinh March 11, 2026 07:40
@g-ongenae
Copy link
Copy Markdown
Member

g-ongenae commented Mar 13, 2026

@nehoramoshe the linter is not passing due to a non null assertions on. Please fix.

@nehoramoshe
Copy link
Copy Markdown
Contributor Author

@nehoramoshe the linter is not passing due to a non null assertions on. Please fix.

Fixed
@g-ongenae

Copy link
Copy Markdown
Contributor

@qhdinh qhdinh left a comment

Choose a reason for hiding this comment

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

@nehoramoshe LGTM, thanks for your contribution!

Copy link
Copy Markdown
Member

@g-ongenae g-ongenae left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I have a just one small change before merging and releasing. (Before diving in the code, I thought a breaking change was introduced.)

Comment thread src/GoogleCloudPubSub/GoogleCloudPubSub.ts
Co-authored-by: Guillaume Ongenae <guillaume.ongenae@gmail.com>
@nehoramoshe
Copy link
Copy Markdown
Contributor Author

Overall LGTM, I have a just one small change before merging and releasing. (Before diving in the code, I thought a breaking change was introduced.)

Done

@nehoramoshe nehoramoshe requested a review from g-ongenae March 18, 2026 10:22
@g-ongenae
Copy link
Copy Markdown
Member

Thanks @nehoramoshe, I'll merge and release it this afternoon.

@g-ongenae g-ongenae merged commit 237412a into algoan:master Mar 18, 2026
6 checks passed
@nehoramoshe
Copy link
Copy Markdown
Contributor Author

nehoramoshe commented Mar 18, 2026

Note on Release Failure ⚠️

Hi @g-ongenae ,

I noticed that the automated release for this PR failed in the CI pipeline (Link to failed Action).

The error is EINVALIDGHTOKEN, which usually means the GH_TOKEN or GITHUB_TOKEN used by semantic-release has expired or lacks write permissions in the repository settings.

Could a maintainer please check the repository secrets/permissions so the new version can be published to NPM? Thanks!

@g-ongenae
Copy link
Copy Markdown
Member

@nehoramoshe I don't have the rights necessary to update the token, I asked my colleagues with the rights but he hadn't got the time to look at it. I look to fix it tomorrow. I'll update you when it's updated.

@algoanbot
Copy link
Copy Markdown

🎉 This PR is included in version 6.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants