Update schedule counter behavior

[jnog]

What is being changed?

The schedule counter increments every query execution. This is true even when a query has no new results to log. The Current behavior, which increments the query counter every execution, makes it difficult to determine whether or not a differential result was missed.

Change the counter behavior so only when a differential results is calculated the counter increments. With this change the counter represents the order in which differential results should be replayed to recreate the point in time state.

Looking for initial thoughts on this change. The documentation and original PR #3651 suggest the counter is only ever useful to detect the initial results. Changing the counter behavior this way doesn't break this use case.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Jonathan Nogueira (8b84095)

[directionless]

This came up in office hours today.

I am in favor of redefining counter to be monotonically increasing based on results (what this does).

I recognize this breaks an unlikely use case where someone might have been using this to track executions. But that data is better accessed through the osquery_schedule table, and less code is better.

[theopolis]

Close+reopen to retrigger build, which may fail again and is a KP.

[theopolis]

I am rebasing the PR to (hopefully) fix the KP with the build status reporting.

[theopolis]

Ok so the test failures are not because of our CI infra. This change needs to be reflecting in at least one unit test:

32: [ RUN      ] QueryTests.test_query_name_updated
32: I0525 17:57:05.653411 118736320 query.cpp:112] Storing initial results for new scheduled query: will_update_query
32: I0525 17:57:05.653671 118736320 query.cpp:121] Scheduled query has been updated: will_update_query
32: /Users/runner/runners/2.169.0/work/1/s/osquery/core/tests/query_tests.cpp:143: Failure
32: Expected equality of these values:
32:   counter
32:     Which is: 128
32:   0UL
32:     Which is: 0

Here is the documentation for building and testing locally: https://osquery.readthedocs.io/en/latest/development/building/#testing

[theopolis]

Please see the unit test failures.

[directionless]

I think the the test is correct. If I understand this correctly addNewResults takes a pointer for counter, but in the case of no update_db, counter is never zeroed. This is a pretty simple patch, but I think we probably need a little more tests before I'd feel confident. Though maybe test_add_and_get_current_results is enough

[theopolis]

@directionless, I think I missed the original discussion, but this change seems reasonable. I only read the code but it seems to be correct. You're call if we want this in 4.4.0.

[theopolis]

Let's deep dive into the test.

[theopolis]

Ok @jnog, @directionless, I think this is the best approach to making the change. We only increment the counter when results are emitted, otherwise we might reset the counter.

[directionless]

Thanks for fixing that @theopolis