Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n type constraint Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion audit, and test patterns Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…name placeholders Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nts, eliminate dead wrapper - Move buildPlacementKey to workbooks utils (cache layer stays storage-only) - getCachedWorkbooksByPlacement now accepts string key instead of PlacementQuery - Remove getCachedOrFetch middle wrapper in ContestTaskCache (delegates directly to Cache.getOrFetch) - Export DEFAULT_CACHE_TTL to eliminate HOUR_MS duplication across domain cache modules - Drop what-only TSDoc from Cache<T> (keep only non-obvious constraint on set()) - byUserCache.clear() → delete(BY_USER_KEY) for single-key intent clarity - Remove stray console.log from createWorkBook Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ch completion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Changesサーバーサイドキャッシュ基盤とドメイン統合
Sequence Diagram(s)sequenceDiagram
participant Service as Service層
participant DomainCache as domain/cache.ts
participant CacheClass as Cache.getOrFetch
participant DB as Prisma DB
rect rgba(100, 149, 237, 0.5)
note over Service,DB: 読み取りフロー（キャッシュ経由）
Service->>DomainCache: getCached*(fetchFn)
DomainCache->>CacheClass: getOrFetch(key, fetchFn)
alt キャッシュヒット
CacheClass-->>DomainCache: キャッシュ値
else インフライト共有
CacheClass-->>DomainCache: 既存 Promise
else キャッシュミス
CacheClass->>DB: fetchFn() → DB クエリ
DB-->>CacheClass: 結果
CacheClass->>CacheClass: cache.set(key, 結果)
CacheClass-->>DomainCache: 結果
end
DomainCache-->>Service: Map / List
end
rect rgba(205, 92, 92, 0.5)
note over Service,DB: 書き込みフロー（無効化）
Service->>DB: create / update / delete
DB-->>Service: 完了
Service->>DomainCache: invalidate*Caches()
DomainCache->>CacheClass: cache.delete(key) / cache.clear()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/votes/server/cache.ts`:
- Around line 4-5: The constant names VOTE_STATS_TTL_MS and KEY violate the
camelCase naming convention required by the coding guidelines. Rename
VOTE_STATS_TTL_MS to voteStatsTtlMs and KEY to key (or a more descriptive
camelCase name like voteCacheKey if appropriate) to comply with the TypeScript
naming standards for variables and constants in the codebase.
In `@src/lib/clients/cache.ts`:
- Around line 98-103: In-flight fetch promises can recache stale values after
cache invalidation because the then handler in the fetchFn promise completion
(lines 98-103) always calls set() regardless of whether the cache was
invalidated after the fetch started. Implement a generation token mechanism
where each invalidate*Caches() call increments a generation counter, store the
generation at the time the promise is created, and only allow set() to proceed
if the stored generation matches the current generation. Apply the same
generation token check to the delete() operation around lines 126-128 to ensure
inflight cleanup also respects invalidation boundaries.
- Around line 85-107: The getOrFetch method can have the inflight map polluted
if the set() call throws an "Invalid cache key" error, because the
inflight.delete(key) in the success handler won't be executed. Move the key
validation to the beginning of the getOrFetch method before creating the
promise, and add a finally block to the promise chain to ensure
inflight.delete(key) is always called regardless of whether the promise resolves
or rejects, preventing the inflight map from being left with orphaned entries.
In `@src/lib/server/tasks/cache.ts`:
- Around line 6-7: The constant names TASK_MAP_KEY and MERGED_KEY violate the
repository's camelCase naming convention for variables in TypeScript files.
Rename TASK_MAP_KEY to taskMapKey and MERGED_KEY to mergedKey to comply with the
coding guidelines, and update all references to these constants throughout the
file to use the new camelCase names.
- Around line 24-27: The invalidateTaskCaches function in
src/lib/server/tasks/cache.ts uses delete() to clear cached values, but this
does not clear the in-flight requests stored in the underlying cache client from
src/lib/clients/cache.ts. This causes a race condition where completing
in-flight fetches can re-cache stale data. Modify the invalidateTaskCaches
function to not only delete the cached entries (tasksCache and mergedTasksCache)
but also clear the associated inflight promises for both TASK_MAP_KEY and
MERGED_KEY so that subsequent requests will trigger fresh fetches instead of
using stale cached data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43e91752-a08d-4c0d-8a70-2175bb1e3824
📒 Files selected for processing (21)
.claude/rules/server-cache.md.claude/rules/testing.mdsrc/features/votes/server/cache.test.tssrc/features/votes/server/cache.tssrc/features/votes/services/vote_grade.test.tssrc/features/votes/services/vote_grade.tssrc/features/votes/services/vote_statistics.test.tssrc/features/votes/services/vote_statistics.tssrc/features/workbooks/server/cache.test.tssrc/features/workbooks/server/cache.tssrc/features/workbooks/services/workbooks.test.tssrc/features/workbooks/services/workbooks.tssrc/features/workbooks/utils/workbooks.test.tssrc/features/workbooks/utils/workbooks.tssrc/lib/clients/cache.test.tssrc/lib/clients/cache.tssrc/lib/clients/cache_strategy.tssrc/lib/server/tasks/cache.test.tssrc/lib/server/tasks/cache.tssrc/lib/services/tasks.tssrc/test/lib/services/tasks.test.ts
Comment on lines
+85
to
+107
| async getOrFetch(key: string, fetchFn: () => Promise<T>): Promise<T> { | ||
| const cached = this.get(key); | ||
|
|
||
| if (cached !== undefined) { | ||
| return cached; | ||
| } | ||
|
|
||
| const pending = this.inflight.get(key); | ||
|
|
||
| if (pending) { | ||
| return pending; | ||
| } | ||
|
|
||
| const promise = fetchFn().then( | ||
| (result) => { | ||
| this.set(key, result); | ||
| this.inflight.delete(key); | ||
| return result; | ||
| }, | ||
| (error) => { | ||
| this.inflight.delete(key); | ||
| throw error; | ||
| }, |
Contributor
There was a problem hiding this comment.
getOrFetch はキー検証を先に行ってください
- Line 100 の
set()でInvalid cache keyが投げられると、then成功ハンドラ内のinflight.deleteが実行されず残留します。 - 結果として不要な
fetchFn実行や、同一キーでの失敗再利用が発生します。
修正案（事前検証 + finally で確実に inflight 解放）
async getOrFetch(key: string, fetchFn: () => Promise<T>): Promise<T> {
+ if (!key || typeof key !== 'string' || key.length > 255) {
+ throw new Error('Invalid cache key');
+ }
const cached = this.get(key);
if (cached !== undefined) {
return cached;
}
const pending = this.inflight.get(key);
if (pending) {
return pending;
}
- const promise = fetchFn().then(
- (result) => {
- this.set(key, result);
- this.inflight.delete(key);
- return result;
- },
- (error) => {
- this.inflight.delete(key);
- throw error;
- },
- );
+ const promise = fetchFn()
+ .then((result) => {
+ this.set(key, result);
+ return result;
+ })
+ .finally(() => {
+ this.inflight.delete(key);
+ });
this.inflight.set(key, promise);
return promise;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async getOrFetch(key: string, fetchFn: () => Promise<T>): Promise<T> { | |
| const cached = this.get(key); | |
| if (cached !== undefined) { | |
| return cached; | |
| } | |
| const pending = this.inflight.get(key); | |
| if (pending) { | |
| return pending; | |
| } | |
| const promise = fetchFn().then( | |
| (result) => { | |
| this.set(key, result); | |
| this.inflight.delete(key); | |
| return result; | |
| }, | |
| (error) => { | |
| this.inflight.delete(key); | |
| throw error; | |
| }, | |
| async getOrFetch(key: string, fetchFn: () => Promise<T>): Promise<T> { | |
| if (!key || typeof key !== 'string' || key.length > 255) { | |
| throw new Error('Invalid cache key'); | |
| } | |
| const cached = this.get(key); | |
| if (cached !== undefined) { | |
| return cached; | |
| } | |
| const pending = this.inflight.get(key); | |
| if (pending) { | |
| return pending; | |
| } | |
| const promise = fetchFn() | |
| .then((result) => { | |
| this.set(key, result); | |
| return result; | |
| }) | |
| .finally(() => { | |
| this.inflight.delete(key); | |
| }); | |
| this.inflight.set(key, promise); | |
| return promise; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/clients/cache.ts` around lines 85 - 107, The getOrFetch method can
have the inflight map polluted if the set() call throws an "Invalid cache key"
error, because the inflight.delete(key) in the success handler won't be
executed. Move the key validation to the beginning of the getOrFetch method
before creating the promise, and add a finally block to the promise chain to
ensure inflight.delete(key) is always called regardless of whether the promise
resolves or rejects, preventing the inflight map from being left with orphaned
entries.
Comment on lines
+98
to
+103
| const promise = fetchFn().then( | ||
| (result) => { | ||
| this.set(key, result); | ||
| this.inflight.delete(key); | ||
| return result; | ||
| }, |
Contributor
There was a problem hiding this comment.
無効化後に進行中フェッチが古い値を再キャッシュします
Line 98-103 で進行中 Promise 完了時に常に set しており、Line 126-128 の delete() は inflight/世代管理に触れないため、invalidate*Caches() 後でも古い結果が TTL ぶん残る競合が起きます（書き込み直後の stale read）。
修正案（世代トークンで無効化後の書き戻しを抑止）
export class Cache<T extends {}> {
private cache: Map<string, CacheEntry<T>> = new Map();
private inflight = new Map<string, Promise<T>>();
+ private keyGeneration = new Map<string, number>();
async getOrFetch(key: string, fetchFn: () => Promise<T>): Promise<T> {
+ const generationAtStart = this.keyGeneration.get(key) ?? 0;
const cached = this.get(key);
if (cached !== undefined) {
return cached;
}
const pending = this.inflight.get(key);
if (pending) {
return pending;
}
- const promise = fetchFn().then(
- (result) => {
- this.set(key, result);
- this.inflight.delete(key);
- return result;
- },
- (error) => {
- this.inflight.delete(key);
- throw error;
- },
- );
+ const promise = fetchFn()
+ .then((result) => {
+ if ((this.keyGeneration.get(key) ?? 0) === generationAtStart) {
+ this.set(key, result);
+ }
+ return result;
+ })
+ .finally(() => {
+ this.inflight.delete(key);
+ });
this.inflight.set(key, promise);
return promise;
}
delete(key: string): void {
this.cache.delete(key);
+ this.keyGeneration.set(key, (this.keyGeneration.get(key) ?? 0) + 1);
}
}Also applies to: 126-128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/clients/cache.ts` around lines 98 - 103, In-flight fetch promises can
recache stale values after cache invalidation because the then handler in the
fetchFn promise completion (lines 98-103) always calls set() regardless of
whether the cache was invalidated after the fetch started. Implement a
generation token mechanism where each invalidate*Caches() call increments a
generation counter, store the generation at the time the promise is created, and
only allow set() to proceed if the stored generation matches the current
generation. Apply the same generation token check to the delete() operation
around lines 126-128 to ensure inflight cleanup also respects invalidation
boundaries.
Comment on lines
+24
to
+27
| export function invalidateTaskCaches(): void { | ||
| tasksCache.delete(TASK_MAP_KEY); | ||
| mergedTasksCache.delete(MERGED_KEY); | ||
| } |
Contributor
There was a problem hiding this comment.
無効化中の in-flight フェッチで古い値が再キャッシュされます。
Line 24-26 の delete() ベース無効化は、src/lib/clients/cache.ts の delete() が inflight を消さないため、更新直後でも旧データが戻る競合が発生します（createTask/updateTask 後に stale が TTL まで残る可能性）。
修正案（根本対処）
--- a/src/lib/clients/cache.ts
+++ b/src/lib/clients/cache.ts
@@
clear(): void {
this.cache.clear();
+ this.inflight.clear();
}
delete(key: string): void {
this.cache.delete(key);
+ this.inflight.delete(key);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/tasks/cache.ts` around lines 24 - 27, The invalidateTaskCaches
function in src/lib/server/tasks/cache.ts uses delete() to clear cached values,
but this does not clear the in-flight requests stored in the underlying cache
client from src/lib/clients/cache.ts. This causes a race condition where
completing in-flight fetches can re-cache stale data. Modify the
invalidateTaskCaches function to not only delete the cached entries (tasksCache
and mergedTasksCache) but also clear the associated inflight promises for both
TASK_MAP_KEY and MERGED_KEY so that subsequent requests will trigger fresh
fetches instead of using stale cached data.
.finally transparently re-throws on rejection, so error propagation is preserved while removing the duplicate inflight.delete call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #3706
Summary by CodeRabbit