fix(stream): don't suppress digest reverts in snapshot stream#5923
Conversation
The digest check r.GetDigest() == snap.GetDigest() was meant to suppress the first snapshot when the client already has that state. However it also suppressed any later snapshot whose digest happened to equal the original request digest (e.g. after a git rollback), leaving clients stuck on stale data indefinitely. Guard the check with lastDigest == nil so it only fires before the first update is sent; subsequent deduplication is handled by the existing hash-based comparison. Signed-off-by: Immanuel Tikhonov <[email protected]> Signed-off-by: immanuwell <[email protected]>
dosubot
Bot
added
the
size:XS
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: comment
The PR fixes a subtle regression with digest reverts in EvaluationSnapshotNamespaceStream—after the first snapshot, all future snapshots (including rollbacks) are correctly delivered. Logic and test coverage are sound, but the in-line comment would benefit from a brief explanation of the bug and reason for the guard. No blocking issues.
internal/server/evaluation/client/server.go
- major (L166): The suppression check for sending snapshots now correctly guards with 'lastDigest == nil', ensuring only the initial snapshot is skipped if the client already has it. However, the comment clarifying this logic could be expanded to reference the regression scenario, guiding future maintainers about why the check is limited this way. Suggest elaborating the in-line comment to briefly explain the reverted digest bug and its impact.
🤖 Automated review by the Flipt PR review agent.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2 #5923 +/- ##
==========================================
+ Coverage 61.05% 61.28% +0.23%
==========================================
Files 141 141
Lines 14211 14208 -3
==========================================
+ Hits 8676 8707 +31
+ Misses 4809 4772 -37
- Partials 726 729 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- replace SHA1 hash-based dedup with plain string comparison - initialize lastDigest from request digest - validate request digest is 40 characters when present Signed-off-by: Roman Dmytrenko <[email protected]>
dosubot
Bot
added
size:S
There was a problem hiding this comment.
Verdict: comment
The PR fixes the digest revert regression in EvaluationSnapshotNamespaceStream. The new logic ensures snapshots are delivered correctly after rollbacks, and test coverage is comprehensive. Only suggestion: elaborate the suppression logic comment so future readers grasp the rationale. No blocking issues.
internal/server/evaluation/client/server.go
- major (L166): The new guard on snapshot suppression ('lastDigest == nil && r.GetDigest() == snap.GetDigest()') correctly addresses the regression where a digest revert could cause the stream to skip sending a needed update. However, the corresponding comment is terse—future maintainers would benefit from an in-line explanation referencing the digest revert scenario and why the suppression is limited to the initial snapshot. Suggest: expand the in-line comment to briefly describe the bug and the reasoning behind the guarded check, e.g., 'Only suppress the initial snapshot if the client already has it; subsequent digest reverts must always be delivered, otherwise the client could be stuck with stale state.'
🤖 Automated review by the Flipt PR review agent.
erka
left a comment
erka
left a comment
There was a problem hiding this comment.
thank you @immanuwell for your work and for your attention to detail.
There was a problem hiding this comment.
Verdict: comment
The PR fixes a regression in EvaluationSnapshotNamespaceStream, ensuring snapshots are always sent to the client after digest reverts. Logic is sound and tests are thorough. The only actionable suggestion is to expand the code comment on the suppression check so future maintainers understand its rationale. No blocking issues.
internal/server/evaluation/client/server.go
- major (L166): The suppression logic for sending snapshots, now guarded with 'lastDigest == nil && r.GetDigest() == snap.GetDigest()', correctly ensures digest reverts are not skipped after the first snapshot. However, the in-line comment describing this guard is concise and risks confusion for future maintainers. Please expand the comment to briefly explain that this check prevents clients from getting stuck with stale state during a digest revert, referencing the bug that was fixed (regression #5909). This gives historical context and aids clarity.
🤖 Automated review by the Flipt PR review agent.
3 participants
What
EvaluationSnapshotNamespaceStreamusesr.GetDigest() == snap.GetDigest()to skip sending a snapshot the client already has. Good idea, but that check fires on every future snapshot with the same digest - not just the initial one.The bug
Say client connects with digest
d1. Stream goes:d1-> skipped (correct, client has it)d2-> sent (client now has d2)d1(git rollback) -> skipped! (but client has d2 now, not d1)Client is stuck on stale state and won't recover until the digest changes again.
How to reproduce
Run the new test:
It fails without the fix.
Fix
Guard the check with
lastDigest == nilso it only suppresses the first snapshot (before anything has been sent). After that, the existing hash-based dedup handles everything, including reverts.Regression introduced in #5909.