Fix lingering live queries keys in Redis (#33928)
Resolves #33254 This can be reproduced locally by running the following "high load" test: Run 500 hosts using osquery-perf: ``` go run ./cmd/osquery-perf --enroll_secret ... \ --host_count 500 \ --server_url https://localhost:8080 \ --live_query_fail_prob 0.0 \ --live_query_no_results_prob 0.0 \ --orbit_prob 0.0 \ --http_message_signature_prob 0.0 ``` Run `stress_test_live_queries.sh`: ``` #!/bin/bash while true; do curl -v -k -X POST -H "Authorization: Bearer $TEST_TOKEN" https://localhost:8080/api/latest/fleet/queries/$SAVED_QUERY_ID/run -d '{"host_ids": [<500 comma-separated host ids>]}' done ``` Use "Redis Insight" or the like and you will start to see `livequery:{$CAMPAIGN_ID}` keys with `No limit` (which is the bug): <img width="1380" height="227" alt="Screenshot 2025-10-07 at 3 10 26 PM" src="https://github.com/user-attachments/assets/30434348-3217-40c4-8ebc-bab5ceb4daa9" /> - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Prevent lingering Redis keys for live queries by ensuring keys are cleaned up and not recreated when completing/canceling non-existent queries. - Improves resource usage and avoids stale state in live query processing. - Tests - Added tests verifying proper retrieval/completion behavior and that no Redis key is created for non-existent live queries. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Ian Littman <iansltx@gmail.com>
This commit is contained in:
co-authored by
Ian Littman
parent
53f74e3ebc
commit
b5626e17e6
@@ -0,0 +1 @@
|
||||
* Fixed a bug in live queries that caused `livequery:{$CAMPAIGN_ID}` Redis keys to not be cleaned up or expire.
|
||||
@@ -19,6 +19,7 @@ var testFunctions = [...]func(*testing.T, fleet.LiveQueryStore){
|
||||
testLiveQueryExpiredQuery,
|
||||
testLiveQueryOnlyExpired,
|
||||
testLiveQueryCleanupInactive,
|
||||
testLiveQuerySetBitOnlyIfKeyExists,
|
||||
}
|
||||
|
||||
func testLiveQuery(t *testing.T, store fleet.LiveQueryStore) {
|
||||
@@ -221,3 +222,45 @@ func testLiveQueryCleanupInactive(t *testing.T, store fleet.LiveQueryStore) {
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, m)
|
||||
}
|
||||
|
||||
func testLiveQuerySetBitOnlyIfKeyExists(t *testing.T, store fleet.LiveQueryStore) {
|
||||
// Create a live query campaign.
|
||||
err := store.RunQuery("test", "SELECT 1;", []uint{1})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Get the query for the host.
|
||||
queries, err := store.QueriesForHost(1)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t,
|
||||
map[string]string{
|
||||
"test": "SELECT 1;",
|
||||
},
|
||||
queries,
|
||||
)
|
||||
|
||||
// Mark query as completed by host.
|
||||
err = store.QueryCompletedByHost("test", 1)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Query should not be returned anymore as it was marked as completed for this host.
|
||||
queries, err = store.QueriesForHost(1)
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, queries)
|
||||
|
||||
// A host could be attempting to write a result for a query that was already deleted.
|
||||
err = store.QueryCompletedByHost("test-2", 1)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Let's test that such key was not created.
|
||||
|
||||
// get a raw Redis connection to make direct checks
|
||||
pool := store.(*redisLiveQuery).pool
|
||||
conn := redis.ConfigureDoer(pool, pool.Get())
|
||||
t.Cleanup(func() {
|
||||
conn.Close()
|
||||
})
|
||||
|
||||
n, err := redigo.Int(conn.Do("EXISTS", queryKeyPrefix+"{test-2}"))
|
||||
require.NoError(t, err)
|
||||
require.Zero(t, n)
|
||||
}
|
||||
|
||||
@@ -263,8 +263,17 @@ func (r *redisLiveQuery) QueryCompletedByHost(name string, hostID uint) error {
|
||||
|
||||
targetKey, _ := generateKeys(name)
|
||||
|
||||
// Update the bitfield for this host.
|
||||
if _, err := conn.Do("SETBIT", targetKey, hostID, 0); err != nil {
|
||||
// Update the bitfield for this host only if the key exists.
|
||||
// If the key doesn't exist (e.g. query marked as completed or cancelled)
|
||||
// then we don't want to call SETBIT because it will create a new
|
||||
// key (that won't expire and linger "forever").
|
||||
const setBitScript = `
|
||||
if redis.call('EXISTS', KEYS[1]) == 1 then
|
||||
return redis.call('SETBIT', KEYS[1], ARGV[1], ARGV[2])
|
||||
else
|
||||
return nil
|
||||
end`
|
||||
if _, err := conn.Do("EVAL", setBitScript, 1, targetKey, hostID, 0); err != nil {
|
||||
return fmt.Errorf("setbit query key: %w", err)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user