Decision stream: move to chunked transfer by default#4413
Decision stream: move to chunked transfer by default#4413
Conversation
blotus
commented
Apr 7, 2026
blotus
commented
- Always send the decisions list in chunk in the decisions stream.
- Avoid some useless copies to intermediate structs
- Use a single JSON encoder to encode each decision
|
@blotus: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
|
@blotus: There are no area labels on this PR. You can add as many areas as you see fit.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4413 +/- ##
==========================================
+ Coverage 63.54% 63.74% +0.19%
==========================================
Files 476 476
Lines 33841 33819 -22
==========================================
+ Hits 21505 21558 +53
+ Misses 10201 10114 -87
- Partials 2135 2147 +12
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:
|
|
/kind enhancement |
github-actions
bot
added
kind/enhancement
There was a problem hiding this comment.
Pull request overview
Moves the v1 decisions streaming endpoint to always use a chunked/streaming JSON response and reduces per-decision allocations while querying/encoding decisions.
Changes:
- Deprecates the
chunked_decisions_streamfeature flag and removes the non-chunked streaming path. - Updates decision DB query helpers to accept a shared
nowtimestamp and selects only required fields. - Streams decisions using a reusable JSON encoder and avoids intermediate slice/marshal copies.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/fflag/crowdsec.go | Deprecates the chunked decisions stream feature flag. |
| pkg/database/decisions.go | Adds now time.Time to decision-query methods and narrows selected columns. |
| pkg/apiserver/controllers/v1/decisions.go | Implements always-streaming decisions response and per-decision encoding. |
Comments suppressed due to low confidence (1)
pkg/apiserver/controllers/v1/decisions.go:295
- Setting the
Transfer-Encoding: chunkedheader explicitly is unnecessary (net/http will choose the right framing) and can be problematic under HTTP/2 whereTransfer-Encodingis not valid. Consider removing this header and relying on streaming writes/flushes to trigger chunked transfer on HTTP/1.1.
gctx.Writer.Header().Set("Content-Type", "application/json")
gctx.Writer.Header().Set("Transfer-Encoding", "chunked")
gctx.Writer.WriteHeader(http.StatusOK)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func formatOneDecision(dbDecision *ent.Decision) *models.Decision { | ||
| duration := dbDecision.Until.Sub(time.Now().UTC()).Round(time.Second).String() | ||
|
|
There was a problem hiding this comment.
formatOneDecision recomputes time.Now().UTC() per decision. In the streaming path this means Duration values can drift within a single response (and diverge from the now timestamp used for the DB queries). Consider passing the stream's reference time (now) into formatOneDecision (and reusing the helper from FormatDecisions) so durations are consistent across all decisions in the response.
| for { | ||
| if lastId > 0 { | ||
| lastIdStr := strconv.Itoa(lastId) | ||
| filters["id_gt"] = []string{lastIdStr} | ||
| } |
There was a problem hiding this comment.
writeStartupDecisions mutates the shared filters map by setting filters["id_gt"] for pagination, but never clears it. Since the same filters map is reused later in the same request (e.g., for the expired decisions query), the leftover id_gt can unintentionally filter out results and produce incomplete deleted output. Consider working on a local copy of filters inside this function (or delete(filters, "id_gt") before returning / before the next query).
| for { | ||
| if lastId > 0 { | ||
| lastIdStr := strconv.Itoa(lastId) | ||
| filters["id_gt"] = []string{lastIdStr} | ||
| } |
There was a problem hiding this comment.
writeDeltaDecisions paginates by mutating the shared filters map (filters["id_gt"]), but doesn't reset it. Because filters is reused for the subsequent expired-decisions stream in the same response, the stale id_gt value can cause the expired query to start already filtered and miss deletions. Consider copying filters before modifying it (or explicitly clearing id_gt at the start/end of the function).
