Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(backend): fallback if sinceId is older than the oldest in cache when using FTT (#14061) #210

Merged
merged 1 commit into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
### Client

### Server
- Fix: FTT有効時、タイムライン用エンドポイントで`sinceId`にキャッシュ内最古のものより古いものを指定した場合に正しく結果が返ってこない問題を修正

## 2024.5.0-kinel.3

Expand Down
13 changes: 5 additions & 8 deletions packages/backend/src/core/FanoutTimelineEndpointService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,6 @@ export class FanoutTimelineEndpointService {

@bindThis
private async getMiNotes(ps: TimelineOptions): Promise<MiNote[]> {
let noteIds: string[];
let shouldFallbackToDb = false;

// 呼び出し元と以下の処理をシンプルにするためにdbFallbackを置き換える
if (!ps.useDbFallback) ps.dbFallback = () => Promise.resolve([]);

Expand All @@ -67,18 +64,18 @@ export class FanoutTimelineEndpointService {
const redisResult = await this.fanoutTimelineService.getMulti(ps.redisTimelines, ps.untilId, ps.sinceId);

// 取得したredisResultのうち、2つ以上ソースがあり、1つでも空であればDBにフォールバックする
shouldFallbackToDb = ps.useDbFallback && (redisResult.length > 1 && redisResult.some(ids => ids.length === 0));
let shouldFallbackToDb = ps.useDbFallback && (redisResult.length > 1 && redisResult.some(ids => ids.length === 0));

// 取得したresultの中で最古のIDのうち、最も新しいものを取得
const thresholdId = redisResult.map(ids => ids[0]).sort()[0];

// TODO: いい感じにgetMulti内でソート済だからuniqするときにredisResultが全てソート済なのを利用して再ソートを避けたい
const redisResultIds = shouldFallbackToDb ? [] : Array.from(new Set(redisResult.flat(1)));
const redisResultIds = shouldFallbackToDb ? [] : Array.from(new Set(redisResult.flat(1))).sort(idCompare);

redisResultIds.sort(idCompare);
noteIds = redisResultIds.filter(id => id >= thresholdId).slice(0, ps.limit);
let noteIds = redisResultIds.filter(id => id >= thresholdId).slice(0, ps.limit);

shouldFallbackToDb = shouldFallbackToDb || (noteIds.length === 0);
const oldestNoteId = ascending ? redisResultIds[0] : redisResultIds[redisResultIds.length - 1];
Copy link
Author

Choose a reason for hiding this comment

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

redisResultIdsが[]のとき、oldestNoteIdはundefined

shouldFallbackToDb = shouldFallbackToDb || (noteIds.length === 0) || (ps.sinceId != null && ps.sinceId < oldestNoteId);
Copy link
Author

Choose a reason for hiding this comment

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

String < undefinedの比較はfalse

Copy link
Collaborator

Choose a reason for hiding this comment

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

upstreamの問題 && 家のサーバーで問題にならないからいいけどここの(ps.sinceId != null && ps.sinceId < oldestNoteId)ps.useDbFallback &&取ったほうがいいかもね。


if (!shouldFallbackToDb) {
let filter = ps.noteFilter ?? (_note => true);
Expand Down
35 changes: 35 additions & 0 deletions packages/backend/test/e2e/timelines.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
// pnpm jest -- e2e/timelines.ts

import * as assert from 'assert';
import { Redis } from 'ioredis';
import { loadConfig } from '@/config.js';
import { api, post, randomString, sendEnvUpdateRequest, signup, sleep, uploadUrl } from '../utils.js';

function genHost() {
Expand All @@ -17,7 +19,13 @@ function waitForPushToTl() {
return sleep(500);
}

let redisForTimelines: Redis;

describe('Timelines', () => {
beforeAll(() => {
redisForTimelines = new Redis(loadConfig().redisForTimelines);
});

describe('Home TL', () => {
test.concurrent('自分の visibility: followers なノートが含まれる', async () => {
const [alice] = await Promise.all([signup()]);
Expand Down Expand Up @@ -1373,6 +1381,33 @@ describe('Timelines', () => {

assert.strictEqual(res.body.some((note: any) => note.id === bobNote.id), false);
});

/** @see https://github.com/misskey-dev/misskey/issues/14000 */
test.concurrent('FTT: sinceId にキャッシュより古いノートを指定しても、sinceId による絞り込みが正しく動作する', async () => {
const alice = await signup();
const noteSince = await post(alice, { text: 'Note where id will be `sinceId`.' });
const note1 = await post(alice, { text: '1' });
const note2 = await post(alice, { text: '2' });
await redisForTimelines.del('list:userTimeline:' + alice.id);
const note3 = await post(alice, { text: '3' });

const res = await api('users/notes', { userId: alice.id, sinceId: noteSince.id });
assert.deepStrictEqual(res.body, [note1, note2, note3]);
});

test.concurrent('FTT: sinceId にキャッシュより古いノートを指定しても、sinceId と untilId による絞り込みが正しく動作する', async () => {
const alice = await signup();
const noteSince = await post(alice, { text: 'Note where id will be `sinceId`.' });
const note1 = await post(alice, { text: '1' });
const note2 = await post(alice, { text: '2' });
await redisForTimelines.del('list:userTimeline:' + alice.id);
const note3 = await post(alice, { text: '3' });
const noteUntil = await post(alice, { text: 'Note where id will be `untilId`.' });
await post(alice, { text: '4' });

const res = await api('users/notes', { userId: alice.id, sinceId: noteSince.id, untilId: noteUntil.id });
assert.deepStrictEqual(res.body, [note3, note2, note1]);
});
});

// TODO: リノートミュート済みユーザーのテスト
Expand Down
Loading