fix(feed): bound the stream catch-up replay to the newest items - #134
Conversation
StreamFeedUpdates replayed every item newer than the caller's since_timestamp with no limit. That value is client-supplied and never validated, so an old (or deliberately ancient) one made the server materialise the caller's whole retained history into a List and write all of it to the stream — the one listing path in the API without the ClampPageSize discipline every other one follows, and an easy way for an authenticated client to amplify memory use across several streams. Cap the replay at the newest MaxCatchUpItems. Delivery order is unchanged: the query orders newest-first so the cap keeps the newest items, and the write loop walks it in reverse to keep replaying oldest-first, which is what leaves the newest item on top of the client's list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
Adds a regression test for the unbounded catch-up replay fixed in 5663caa: with more than MaxCatchUpItems (100) matching items, StreamFeedUpdates's catch-up query must return exactly the newest 100, and the write loop must still deliver them oldest-first on the wire (reversing the newest-first query result), which is what leaves the newest article on top of the client's list after each prepend. Follows this file's existing convention of replicating the exact production query against IntegrationFixture's real SQL Server database rather than invoking FeedServiceImpl through gRPC plumbing, since no such harness exists in this test project yet. Skips silently without TEST_MSSQL_CONNECTION_STRING, same as every other test in this file — not runnable against a live DB in this sandbox, so verified compiling and passing (skipped) via `dotnet test`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
|
Warning Review limit reachedNext included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
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 |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
Independent architect review of this PR found the regression test duplicated FeedServiceImpl's query and write-loop logic with its own hardcoded copy of the cap value, so it couldn't actually catch the service changing (e.g. reverting the Take, or the cap value drifting). Expose MaxCatchUpItems as internal (InternalsVisibleTo already covers the test project) and reference it directly. Also adds the ThenByDescending(Id) tiebreaker the review flagged: without it, which items land inside the cap on a PublishedAt tie is non-deterministic, and the strict `>` catch-up predicate never re-delivers a loser on a later reconnect — same tiebreaker already used by cursor pagination, and backed by the same (PublishedAt, Id) index. Minor: reworded the "gets from normal paging" comment — paging continues from the oldest item via nextCursor, so a gap past the cap isn't actually refilled by paging, only by a reload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
Code Review ✅ Approved 1 resolved / 1 findingsBounds the stream catch-up replay to the newest 100 items, preventing a client-supplied ✅ 1 resolved✅ Quality: Catch-up test duplicates logic instead of calling StreamFeedUpdates
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
2 participants
Summary
StreamFeedUpdates's catch-up replay (for a client'ssince_timestampon reconnect) had no cap — a client-supplied, unvalidated, old (or hostile)since_timestampcould make the server materialize the caller's entire retained history into aListand write all of it to the stream, the one listing path in the API without theClampPageSizediscipline every other one followsMaxCatchUpItems, matchingPagingDefaults.MaxPageSize); delivery order is unchanged — the query orders newest-first so the cap keeps the newest items, and the write loop walks the capped list in reverse to preserve oldest-first delivery (the client prepends each item as it arrives, so the newest lands on top last)Related issue
Found by
architectduring this cycle's/maintainrefactor analysis. A dedicatedsecurity-engineerglance on this specific fix (requested since it's security-adjacent) approved it as-is — cap value matches existing pagination conventions, the query is fully index-backed (Subscription.UserId,FeedItem.SubscriptionId,FeedItem.(PublishedAt, Id)), the endpoint requires authentication ([Authorize]at the class level, no[AllowAnonymous]override), andsince_timestampparsing already fails safely via aTryParseguard. That review separately flagged a genuine but distinct pre-existing gap — no rate limiting on this streaming endpoint against reconnect-spam — filed as #132, out of scope for this targeted fix.Type of change
Checklist
dotnet build --configuration Releasepasses with 0 errors, 0 warningscd src/pressmark-web && npm run buildpasses with 0 TypeScript errors (unaffected by this branch)FeedIntegrationTests.StreamCatchUp_CapsAtMaxItems_DeliveredOldestFirst, replicating the exact query and write-loop logic with 150 items to prove the cap and delivery order; 119/119 backend tests pass (note: like the rest of that fixture, the new test needsTEST_MSSQL_CONNECTION_STRINGto actually execute against a live SQL Server — it wasn't available in the environment that authored/verified it, same limitation as every other test in that file)t('ns:key')— no UI touchedNotes for reviewers
Independently verified (build, full test suite, format) before opening this PR, separately from the
architect/security-engineer/testeragents that authored the fix, security review, and test.🤖 Generated with Claude Code
https://claude.ai/code/session_01Hnoh6YfJwp63szeP8ZFqVg
Generated by Claude Code