fix(backend): リモートサーバーの情報が更新できなくなっていた問題を修正 (#13507)

* fix(backend): fetchInstanceMetadataのLockが永遠に解除されない問題を修正 Co-authored-by: まっちゃとーにゅ <[email protected]> * fix test * fix * comment * comment * improve test --------- Co-authored-by: まっちゃとーにゅ <[email protected]>
misskey-dev · Mar 4, 2024 · 9542cb8 · 9542cb8
1 parent 9834801
commit 9542cb8
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 9 deletions.
diff --git a/packages/backend/src/core/FetchInstanceMetadataService.ts b/packages/backend/src/core/FetchInstanceMetadataService.ts
@@ -51,21 +51,35 @@ export class FetchInstanceMetadataService {
 	}
 
 	@bindThis
-	public async tryLock(host: string): Promise<boolean> {
-		const mutex = await this.redisClient.set(`fetchInstanceMetadata:mutex:${host}`, '1', 'GET');
-		return mutex !== '1';
+	// public for test
+	public async tryLock(host: string): Promise<string | null> {
+		// TODO: マイグレーションなのであとで消す (2024.3.1)
+		this.redisClient.del(`fetchInstanceMetadata:mutex:${host}`);
+
+		return await this.redisClient.set(
+			`fetchInstanceMetadata:mutex:v2:${host}`, '1',
+			'EX', 30, // 30秒したら自動でロック解除 https://github.com/misskey-dev/misskey/issues/13506#issuecomment-1975375395
+			'GET' // 古い値を返す（なかったらnull）
+		);
 	}
 
 	@bindThis
-	public unlock(host: string): Promise<'OK'> {
-		return this.redisClient.set(`fetchInstanceMetadata:mutex:${host}`, '0');
+	// public for test
+	public unlock(host: string): Promise<number> {
+		return this.redisClient.del(`fetchInstanceMetadata:mutex:v2:${host}`);
 	}
 
 	@bindThis
 	public async fetchInstanceMetadata(instance: MiInstance, force = false): Promise<void> {
 		const host = instance.host;
-		// Acquire mutex to ensure no parallel runs
-		if (!await this.tryLock(host)) return;
+
+		// finallyでunlockされてしまうのでtry内でロックチェックをしない
+		// （returnであってもfinallyは実行される）
+		if (!force && await this.tryLock(host) === '1') {
+			// 1が返ってきていたらロックされているという意味なので、何もしない
+			return;
+		}
+
 		try {
 			if (!force) {
 				const _instance = await this.federatedInstanceService.fetch(host);

diff --git a/packages/backend/test/unit/FetchInstanceMetadataService.ts b/packages/backend/test/unit/FetchInstanceMetadataService.ts
@@ -56,6 +56,7 @@ describe('FetchInstanceMetadataService', () => {
 				} else if (token === DI.redis) {
 					return mockRedis;
 				}
+				return null;
 			})
 			.compile();
 
@@ -78,6 +79,7 @@ describe('FetchInstanceMetadataService', () => {
 		httpRequestService.getJson.mockImplementation(() => { throw Error(); });
 		const tryLockSpy = jest.spyOn(fetchInstanceMetadataService, 'tryLock');
 		const unlockSpy = jest.spyOn(fetchInstanceMetadataService, 'unlock');
+
 		await fetchInstanceMetadataService.fetchInstanceMetadata({ host: 'example.com' } as any);
 		expect(tryLockSpy).toHaveBeenCalledTimes(1);
 		expect(unlockSpy).toHaveBeenCalledTimes(1);
@@ -92,6 +94,7 @@ describe('FetchInstanceMetadataService', () => {
 		httpRequestService.getJson.mockImplementation(() => { throw Error(); });
 		const tryLockSpy = jest.spyOn(fetchInstanceMetadataService, 'tryLock');
 		const unlockSpy = jest.spyOn(fetchInstanceMetadataService, 'unlock');
+
 		await fetchInstanceMetadataService.fetchInstanceMetadata({ host: 'example.com' } as any);
 		expect(tryLockSpy).toHaveBeenCalledTimes(1);
 		expect(unlockSpy).toHaveBeenCalledTimes(1);
@@ -104,13 +107,30 @@ describe('FetchInstanceMetadataService', () => {
 		const now = Date.now();
 		federatedInstanceService.fetch.mockResolvedValue({ infoUpdatedAt: { getTime: () => now - 10 * 1000 * 60 * 60 * 24 } } as any);
 		httpRequestService.getJson.mockImplementation(() => { throw Error(); });
+		await fetchInstanceMetadataService.tryLock('example.com');
 		const tryLockSpy = jest.spyOn(fetchInstanceMetadataService, 'tryLock');
 		const unlockSpy = jest.spyOn(fetchInstanceMetadataService, 'unlock');
-		await fetchInstanceMetadataService.tryLock('example.com');
+
 		await fetchInstanceMetadataService.fetchInstanceMetadata({ host: 'example.com' } as any);
-		expect(tryLockSpy).toHaveBeenCalledTimes(2);
+		expect(tryLockSpy).toHaveBeenCalledTimes(1);
 		expect(unlockSpy).toHaveBeenCalledTimes(0);
 		expect(federatedInstanceService.fetch).toHaveBeenCalledTimes(0);
 		expect(httpRequestService.getJson).toHaveBeenCalledTimes(0);
 	});
+
+	test('Do when lock not acquired but forced', async () => {
+		redisClient.set = mockRedis();
+		const now = Date.now();
+		federatedInstanceService.fetch.mockResolvedValue({ infoUpdatedAt: { getTime: () => now - 10 * 1000 * 60 * 60 * 24 } } as any);
+		httpRequestService.getJson.mockImplementation(() => { throw Error(); });
+		await fetchInstanceMetadataService.tryLock('example.com');
+		const tryLockSpy = jest.spyOn(fetchInstanceMetadataService, 'tryLock');
+		const unlockSpy = jest.spyOn(fetchInstanceMetadataService, 'unlock');
+
+		await fetchInstanceMetadataService.fetchInstanceMetadata({ host: 'example.com' } as any, true);
+		expect(tryLockSpy).toHaveBeenCalledTimes(0);
+		expect(unlockSpy).toHaveBeenCalledTimes(1);
+		expect(federatedInstanceService.fetch).toHaveBeenCalledTimes(0);
+		expect(httpRequestService.getJson).toHaveBeenCalled();
+	});
 });