Conversation
pracucci
force-pushed
the
update-dskit
branch
from
e017e19 to
e551748
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if !state.isExpired() { | ||
| state.lastSeen = now | ||
| m.connectionsMu.Unlock() | ||
| return false |
There was a problem hiding this comment.
Missing idleExpired flag reset allows premature cache eviction
Low Severity
In removeExpiredConnection, when a request arrives for an existing non-expired connection, state.lastSeen is updated but state.idleExpired is not reset to false. The comment on removeIdleExpiredConnections explicitly states the entry is deleted "if no request arrived in between," implying a request arrival should clear the idle mark. Without the reset, a connection previously marked idle that later receives a request will be immediately evicted on the next idle pass (skipping the two-phase protection), causing it to get a fresh TTL when the next request arrives — contradicting the intended lifetime enforcement.
Additional Locations (1)
There was a problem hiding this comment.
Will look at this separately. Not a blocker.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
Or push these changes by commenting: Preview (37227820df)diff --git a/vendor/github.com/grafana/dskit/middleware/http_connection_ttl_middleware.go b/vendor/github.com/grafana/dskit/middleware/http_connection_ttl_middleware.go
--- a/vendor/github.com/grafana/dskit/middleware/http_connection_ttl_middleware.go
+++ b/vendor/github.com/grafana/dskit/middleware/http_connection_ttl_middleware.go
@@ -160,6 +160,7 @@
}
if !state.isExpired() {
state.lastSeen = now
+ state.idleExpired = false
m.connectionsMu.Unlock()
return false
} |
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
update-dskit
branch
from
e551748 to
91c0ab6
Compare
| * [FEATURE] Ingester: Added experimental support to run ingesters with no tokens in the ring when ingest storage is enabled. You can set `-ingester.ring.num-tokens=0` to enable this feature. #14024 | ||
| * [FEATURE] Store-gateway: Add `-store-gateway.sharding-ring.excluded-zones` flag to exclude specific zones from the store-gateway ring. #14120 | ||
| * [FEATURE] Ingest storage: Add `-ingest-storage.kafka.sasl-mechanism` flag supporting more ways to authenticate with Kafka. #14307 #14344 | ||
| * [FEATURE] Ingest storage: Add `-ingest-storage.kafka.sasl-mechanism` flag to use SCRAM to authenticate with Kafka. #14307 |
There was a problem hiding this comment.
Duplicate CHANGELOG entry for sasl-mechanism feature
Low Severity
A duplicate changelog entry was added for the -ingest-storage.kafka.sasl-mechanism flag. Line 47 already documents this feature referencing both #14307 and #14344, while the newly added line 48 describes the same flag with slightly different wording and only references #14307. This appears to be accidentally committed.
2 participants
What this PR does
Update dskit to get grafana/dskit#916.
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.