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

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 @@ -12,6 +12,7 @@
- チャート生成時にinstance.suspentionStateに置き換えられたinstance.isSuspendedが参照されてしまう問題を修正
- Feat: レートリミット制限に引っかかったときに`Retry-After`ヘッダーを返すように (#13949)
- Fix: アンテナ・クリップ・リスト・ウェブフックがロールポリシーの上限より一つ多く作れてしまうのを修正 (#14036)
- Fix: FTT有効時、タイムライン用エンドポイントで`sinceId`にキャッシュ内最古のものより古いものを指定した場合に正しく結果が返ってこない問題を修正

## 2024.5.0

Expand Down
12 changes: 4 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,12 +64,11 @@ export class FanoutTimelineEndpointService {
const redisResult = await this.fanoutTimelineService.getMulti(ps.redisTimelines, ps.untilId, ps.sinceId);

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

redisResultIds.sort(idCompare);
noteIds = redisResultIds.slice(0, ps.limit);
const redisResultIds = Array.from(new Set(redisResult.flat(1))).sort(idCompare);

shouldFallbackToDb = shouldFallbackToDb || (noteIds.length === 0);
let noteIds = redisResultIds.slice(0, ps.limit);
const oldestNoteId = ascending ? redisResultIds[0] : redisResultIds[redisResultIds.length - 1];
const shouldFallbackToDb = noteIds.length === 0 || ps.sinceId != null && ps.sinceId < oldestNoteId;
Copy link
Member

Choose a reason for hiding this comment

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

useDbFallbackがfalseのときはps.sinceId != null && ps.sinceId < oldestNoteIdの条件でdbfallbackにならないほうがいいかもしれない?情報がないよりはあったほうがマシかは考える余地はあるけど。

(#13495 の実装参照)

Copy link
Contributor

Choose a reason for hiding this comment

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

L59のps.useDbFallbackのチェックでDBフォールバックしないようになっているので問題ないと思います

if (!ps.useDbFallback) ps.dbFallback = () => Promise.resolve([]);

Copy link
Member

Choose a reason for hiding this comment

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

fallbackしようとしてfallbackから取得できないと、FTTから欠けながら取得できるはずのデータが取得できないのがあまり良くない可能性があるっという意味です

Copy link
Contributor

Choose a reason for hiding this comment

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

あーなるほど(たしかに)

Copy link
Contributor Author

@zyoshoka zyoshoka Jul 27, 2024

Choose a reason for hiding this comment

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

(別 issue で議論したほうがいいかもしれませんが)enableFanoutTimelineDbFallback オプション自体の有用性があまりない気が薄々しています。仮にオフにしていたとしても channels/timelineusers/notes ではその値は使われず必ずフォールバックされますし、その他のエンドポイントでも untilId 付きの絞り込みとかで limit を満たす量のノートがキャッシュになかった場合にはこの部分:

const gotFromDb = await this.getAndFilterFromDb(noteIds, filter, idCompare);

で結局フォールバックされるようになっていて、 どの場合なら必ずフォールバックしてどの場合なら enableFanoutTimelineDbFallback を尊重するかの基準が微妙になっています。この仕様に追従するなら、このケースでも新しいノート返したり空配列を返したりするよりは、強制的にフォールバックするほうが自然なのかな…と思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

このPR以前の場合はredisにある中で一番古いノートからが多分帰ってきてると思います(未検証)

それは sinceId の話で、untilId の場合は空配列になっていると思います(未検証)

Copy link
Member

Choose a reason for hiding this comment

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

untilIdはそうですね。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

となると enableFanoutTimelineDbFallback をオフにしている場合キャッシュにないノートを untilId に指定して notes/local-timeline を叩いたときには(この PR 以前から)空配列が返ってくる仕様になっていた訳で、そうすると sinceId 付きで叩いたときに空配列が返るのも合理的ではありますね。

あー語弊があったかと思うので補足すると、sinceId のときだけ新しいノートを返すのも変なので空配列が返ってくる仕様に統一するのが合理的なんじゃないかという意味でした

Copy link
Member

Choose a reason for hiding this comment

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

あー、そういうことか。たしかにどっちも正しいのはそうですが本当に条件に合致するものがない(untilId以前はない)のとあるけどかけてるかもしれない(sinceId以降は穴があるかもだけど情報がある)なので帰ったほうが嬉しいことはあると思います

Copy link
Contributor Author

Choose a reason for hiding this comment

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

まあ折角キャッシュにあるのに返してくれないのが微妙なのは確かですね。とはいえその場合には「正しく取得できなかった可能性がある」といった旨の表示をフロント側でしたほうが親切かなと思います


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 @@ -1272,6 +1280,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