Skip to content

Commit 3b35fa1

Browse files
committed
fix: clean up Pub/Sub Redis keys on deletion, refresh stale instances, and localize UI labels
- Delete oapp:pub:{id} set when removing a pubsub-type app to prevent Redis key leaks - Replace running PubSubInstance on update() so config changes take effect without restart - Reuse update() in start() to eliminate duplicated instance creation - Wrap expiration labels in gt.gettext/ngettext for proper i18n pluralization - Downgrade Pub/Sub start failure log from fatal to error (worker continues running)
1 parent 0a885cc commit 3b35fa1

File tree

5 files changed

+63
-10
lines changed

5 files changed

+63
-10
lines changed

lib/oauth/pubsub/google.js

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -312,14 +312,15 @@ class GooglePubSub {
312312
// Backfill apps with baseScopes === 'pubsub' that are missing from the subscribers set
313313
let apps = await oauth2Apps.backfillPubSubApps();
314314
for (let app of apps) {
315-
this.pubSubInstances.set(app, new PubSubInstance(this, { app }));
315+
await this.update(app);
316316
}
317317
}
318318

319319
async update(app) {
320-
if (!this.pubSubInstances.has(app)) {
321-
this.pubSubInstances.set(app, new PubSubInstance(this, { app }));
320+
if (this.pubSubInstances.has(app)) {
321+
await this.remove(app);
322322
}
323+
this.pubSubInstances.set(app, new PubSubInstance(this, { app }));
323324
}
324325

325326
async remove(app) {

lib/oauth2-apps.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -804,6 +804,10 @@ class OAuth2AppsHandler {
804804
pipeline = pipeline.srem(this.getPubsubAppKey(appData.pubSubApp), id);
805805
}
806806

807+
if (appData.baseScopes === 'pubsub') {
808+
pipeline = pipeline.del(this.getPubsubAppKey(id));
809+
}
810+
807811
let deleteResult = await pipeline.exec();
808812

809813
let hasError = (deleteResult[0] && deleteResult[0][0]) || (deleteResult[1] && deleteResult[1][0]);

lib/routes-ui.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2646,20 +2646,21 @@ return true;`
26462646
// undefined - no data yet (app predates this feature or ensurePubsub hasn't run)
26472647
// null - indefinite (no TTL set, ensurePubsub confirmed this)
26482648
// "Ns" - TTL in seconds (e.g. "2678400s" for 31 days)
2649+
let gt = request.app.gt;
26492650
for (let app of data.apps) {
26502651
if (!app.pubSubSubscription) {
26512652
app.expirationLabel = '';
26522653
} else if (!('subscriptionExpiration' in (app.meta || {}))) {
2653-
app.expirationLabel = 'Unknown';
2654+
app.expirationLabel = gt.gettext('Unknown');
26542655
} else if (app.meta.subscriptionExpiration === null) {
2655-
app.expirationLabel = 'Indefinite';
2656+
app.expirationLabel = gt.gettext('Indefinite');
26562657
} else {
26572658
let seconds = parseInt(app.meta.subscriptionExpiration, 10);
26582659
if (seconds && seconds > 0) {
26592660
let days = Math.round(seconds / 86400);
2660-
app.expirationLabel = days === 1 ? '1 day' : `${days} days`;
2661+
app.expirationLabel = util.format(gt.ngettext('%d day', '%d days', days), days);
26612662
} else {
2662-
app.expirationLabel = 'Indefinite';
2663+
app.expirationLabel = gt.gettext('Indefinite');
26632664
}
26642665
}
26652666
}

test/pubsub-recovery-test.js

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ function createMockRedis() {
8888
ops.push(() => 0);
8989
return this;
9090
},
91-
del: function () {
92-
ops.push(() => 0);
91+
del: function (key) {
92+
ops.push(() => redis.del(key));
9393
return this;
9494
},
9595
expire: function () {
@@ -145,6 +145,18 @@ function createMockRedis() {
145145
}
146146
return removed;
147147
},
148+
del: async key => {
149+
let existed = 0;
150+
if (mockSets[key]) {
151+
delete mockSets[key];
152+
existed = 1;
153+
}
154+
if (mockRedisData[key]) {
155+
delete mockRedisData[key];
156+
existed = 1;
157+
}
158+
return existed;
159+
},
148160
exists: async () => 0,
149161
get: async () => null,
150162
set: async () => 'OK',
@@ -790,6 +802,23 @@ test('Pub/Sub subscription recovery tests', async t => {
790802
assert.strictEqual(instance.stopped, true, 'instance should be stopped');
791803
assert.strictEqual(pubsub.pubSubInstances.size, 0, 'instance should be removed from map');
792804
});
805+
806+
await t2.test('update() replaces existing instance with a fresh one', async () => {
807+
let pubsub = new GooglePubSub({ call: async () => {} });
808+
await pubsub.update('refresh-test');
809+
let firstInstance = pubsub.pubSubInstances.get('refresh-test');
810+
assert.ok(firstInstance, 'instance should exist after first update');
811+
812+
await pubsub.update('refresh-test');
813+
let secondInstance = pubsub.pubSubInstances.get('refresh-test');
814+
assert.ok(secondInstance, 'instance should exist after second update');
815+
assert.notStrictEqual(firstInstance, secondInstance, 'instance should be a new object');
816+
assert.strictEqual(firstInstance.stopped, true, 'old instance should be stopped');
817+
assert.strictEqual(secondInstance.stopped, false, 'new instance should not be stopped');
818+
assert.strictEqual(pubsub.pubSubInstances.size, 1, 'map should still have exactly one entry');
819+
820+
pubsub.stopAll();
821+
});
793822
});
794823

795824
// --- _recoveryBackoffMs() tests ---
@@ -852,6 +881,24 @@ test('Pub/Sub subscription recovery tests', async t => {
852881
})();
853882
});
854883

884+
await t2.test('del() deletes oapp:pub:{id} set when app has baseScopes pubsub', async () => {
885+
let appId = 'del-pubsub-provider';
886+
let pubsubAppKey = `${REDIS_PREFIX}oapp:pub:${appId}`;
887+
let appFields = { id: appId, baseScopes: 'pubsub', provider: 'gmailService' };
888+
889+
mockSets[indexKey] = new Set([appId]);
890+
mockSets[subscribersKey] = new Set([appId]);
891+
mockSets[pubsubAppKey] = new Set(['subscriber-1', 'subscriber-2']);
892+
mockRedisData[dataKey] = { [`${appId}:data`]: msgpack.encode(appFields) };
893+
894+
await withMockedOauth2Apps({ get: async () => ({ ...appFields }) }, async () => {
895+
let result = await oauth2Apps.del(appId);
896+
assert.strictEqual(result.id, appId, 'should return correct id');
897+
assert.ok(!mockSets[subscribersKey] || !mockSets[subscribersKey].has(appId), 'app should be removed from subscriber set');
898+
assert.ok(!mockSets[pubsubAppKey], 'oapp:pub:{id} set should be deleted');
899+
})();
900+
});
901+
855902
await t2.test('del() without pubSubApp skips pubsub app set removal', async () => {
856903
let appId = 'del-no-pubsub-app';
857904
let appFields = { id: appId, baseScopes: 'api', provider: 'gmail' };

workers/webhooks.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,7 @@ googlePubSub
620620
logger.info({ msg: 'Started processing Google pub/sub' });
621621
})
622622
.catch(err => {
623-
logger.fatal({ msg: 'Failed to start processing Google pub/sub', err });
623+
logger.error({ msg: 'Failed to start processing Google pub/sub', err });
624624
});
625625

626626
logger.info({ msg: 'Started Webhooks worker thread', version: packageData.version });

0 commit comments

Comments
 (0)