From c90ccbabc38276d856e6cb0842e6e90830e4d9b3 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 11 Jun 2026 00:22:26 -0500 Subject: [PATCH 1/3] Fixed key length in existing-record lookup in EvalContextHeapPersistentSave ValueSizeDB() was called with strlen(key), but WriteDB(), ReadDB() and DeleteDB() store and look up keys with strlen(key) + 1 (including the terminating NUL), and LMDB matches keys by exact byte length. The lookup therefore never found the stored record and always reported size 0. Passing strlen(key) + 1 makes the lookup match the key as written. This lets EvalContextHeapPersistentSave() find an existing record and act on it: preserve an already-set, unexpired timer (CONTEXT_STATE_POLICY_PRESERVE) and log "Resetting" rather than "Creating" on a RESET save. Ticket: CFE-4681 Co-Authored-By: Claude Opus 4.8 (1M context) --- libpromises/eval_context.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libpromises/eval_context.c b/libpromises/eval_context.c index 76c771388a..1c82b1dcb2 100644 --- a/libpromises/eval_context.c +++ b/libpromises/eval_context.c @@ -735,7 +735,7 @@ void EvalContextHeapPersistentSave(EvalContext *ctx, const char *name, unsigned // first see if we have an existing record, and if we should bother to update { - int existing_info_size = ValueSizeDB(dbp, key, strlen(key)); + int existing_info_size = ValueSizeDB(dbp, key, strlen(key) + 1); if (existing_info_size > 0) { PersistentClassInfo *existing_info = xcalloc(existing_info_size, 1); From 53afadc4f0686e0563b3ad84bb5e344ce8fd07c9 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 11 Jun 2026 00:22:34 -0500 Subject: [PATCH 2/3] CFE-4681: timer_policy support for classes: promises Add a timer_policy attribute to classes: promises, controlling whether a persistent class timer resets on re-evaluation ("reset") or preserves the original expiry ("absolute"). Defaults to "absolute" for backward compatibility; planned to change to "reset" in 3.28.0 to match classes bodies, keeping "absolute" on the 3.24 and 3.27 LTS backports. timer_policy without persistence is now an error, since it only governs the persistence timer. Ticket: CFE-4681 Changelog: Title Co-Authored-By: Claude Opus 4.8 (1M context) --- libpromises/attributes.c | 24 ++++- libpromises/cf3.defs.h | 1 + libpromises/mod_common.c | 1 + libpromises/promises.c | 25 ++++- libpromises/verify_classes.c | 46 +++++++++- .../01_basic/persistent_timer_policy.cf | 92 +++++++++++++++++++ .../01_basic/persistent_timer_policy.cf.sub | 15 +++ .../01_basic/persistent_timer_policy_reset.cf | 83 +++++++++++++++++ .../persistent_timer_policy_reset.cf.sub | 16 ++++ .../timer_policy_requires_persistence.cf | 49 ++++++++++ .../timer_policy_requires_persistence.cf.sub | 14 +++ tests/unit/eval_context_test.c | 46 ++++++++++ 12 files changed, 403 insertions(+), 9 deletions(-) create mode 100644 tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf create mode 100644 tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf.sub create mode 100644 tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf create mode 100644 tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf.sub create mode 100644 tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf create mode 100644 tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf.sub diff --git a/libpromises/attributes.c b/libpromises/attributes.c index b9fe8674ba..cdb87cc63d 100644 --- a/libpromises/attributes.c +++ b/libpromises/attributes.c @@ -1132,6 +1132,26 @@ ContextConstraint GetContextConstraints(const EvalContext *ctx, const Promise *p a.expression = NULL; a.persistent = PromiseGetConstraintAsInt(ctx, "persistence", pp); + { + const char *tp = PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR); + if (StringEqual(tp, "reset")) + { + a.timer = CONTEXT_STATE_POLICY_RESET; + } + else + { + /* Default to PRESERVE (absolute) for backward compatibility: + * classes: promises historically skip re-evaluation when the + * class is already defined, so the timer was never reset. + * + * NOTE: this default is planned to change to RESET in 3.28.0 + * (CFE-4681) to align with classes bodies, whose timer_policy + * already defaults to reset. The absolute default is retained on + * the 3.24 and 3.27 LTS backports. */ + a.timer = CONTEXT_STATE_POLICY_PRESERVE; + } + } + { const char *context_scope = PromiseGetConstraintAsRval(pp, "scope", RVAL_TYPE_SCALAR); a.scope = ContextScopeFromString(context_scope); @@ -1143,7 +1163,9 @@ ContextConstraint GetContextConstraints(const EvalContext *ctx, const Promise *p for (int k = 0; CF_CLASSBODY[k].lval != NULL; k++) { - if (strcmp(cp->lval, "persistence") == 0 || strcmp(cp->lval, "scope") == 0) + if (StringEqual(cp->lval, "persistence") || + StringEqual(cp->lval, "scope") || + StringEqual(cp->lval, "timer_policy")) { continue; } diff --git a/libpromises/cf3.defs.h b/libpromises/cf3.defs.h index 71ca9f7f3b..7a25a2452a 100644 --- a/libpromises/cf3.defs.h +++ b/libpromises/cf3.defs.h @@ -1210,6 +1210,7 @@ typedef struct ContextScope scope; int nconstraints; int persistent; + PersistentClassPolicy timer; } ContextConstraint; /*************************************************************************/ diff --git a/libpromises/mod_common.c b/libpromises/mod_common.c index 3d41cd4270..105e47fa86 100644 --- a/libpromises/mod_common.c +++ b/libpromises/mod_common.c @@ -226,6 +226,7 @@ const ConstraintSyntax CF_CLASSBODY[] = ConstraintSyntaxNewContext("not", "Evaluate the negation of string expression in normal form", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("select_class", "Select one of the named list of classes to define based on host identity. Default value: random_selection", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("xor", "Combine class sources with XOR", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewOption("timer_policy", "absolute,reset", "Whether a persistent class restarts its counter when rediscovered. Default value: absolute", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; diff --git a/libpromises/promises.c b/libpromises/promises.c index 4c917ef581..9e955f5496 100644 --- a/libpromises/promises.c +++ b/libpromises/promises.c @@ -705,16 +705,31 @@ Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded) pcopy->org_pp = pp->org_pp; // if this is a class promise, check if it is already set, if so, skip + // Exception: persistent classes with timer_policy => "reset" must not + // be skipped — the promise needs to fire so the timer gets reset. if (strcmp("classes", PromiseGetPromiseType(pp)) == 0) { if (IsDefinedClass(ctx, CanonifyName(pcopy->promiser))) { - Log(LOG_LEVEL_DEBUG, - "Skipping evaluation of classes promise as class '%s' is already set", - CanonifyName(pcopy->promiser)); + const char *tp = PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR); + int persistence = PromiseGetConstraintAsInt(ctx, "persistence", pp); - *excluded = true; - return pcopy; + if (StringEqual(tp, "reset") && persistence > 0) + { + Log(LOG_LEVEL_DEBUG, + "Class '%s' is already set but timer_policy is reset" + " — allowing promise evaluation to reset persistence timer", + CanonifyName(pcopy->promiser)); + } + else + { + Log(LOG_LEVEL_DEBUG, + "Skipping evaluation of classes promise as class '%s' is already set", + CanonifyName(pcopy->promiser)); + + *excluded = true; + return pcopy; + } } } diff --git a/libpromises/verify_classes.c b/libpromises/verify_classes.c index 7f903e2eb6..afb70fff9d 100644 --- a/libpromises/verify_classes.c +++ b/libpromises/verify_classes.c @@ -73,8 +73,48 @@ PromiseResult VerifyClassPromise(EvalContext *ctx, const Promise *pp, ARG_UNUSED return PROMISE_RESULT_FAIL; } - if (a.context.expression == NULL || - EvalClassExpression(ctx, a.context.expression, pp)) + /* timer_policy only governs the persistence timer, so it is meaningless + * without 'persistence'. Error rather than silently ignoring it. */ + if (a.context.persistent <= 0 && + PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR) != NULL) + { + cfPS(ctx, LOG_LEVEL_ERR, PROMISE_RESULT_FAIL, pp, &a, + "Cannot use 'timer_policy' without 'persistence' in classes promise for '%s'", + pp->promiser); + return PROMISE_RESULT_FAIL; + } + + bool class_expression_true = + (a.context.expression == NULL || + EvalClassExpression(ctx, a.context.expression, pp)); + + /* When a persistent class is already defined (loaded from DB), + * EvalClassExpression short-circuits and returns false. If the + * timer_policy is "reset", we still need to reset the persistence + * timer in the DB even though the class is already in the context. */ + if (!class_expression_true && + a.context.persistent > 0 && + a.context.timer == CONTEXT_STATE_POLICY_RESET && + IsDefinedClass(ctx, pp->promiser)) + { + StringSet *tags = StringSetNew(); + StringSetAdd(tags, xstrdup("source=promise")); + for (const Rlist *rp = PromiseGetConstraintAsList(ctx, "meta", pp); rp; rp = rp->next) + { + StringSetAdd(tags, xstrdup(RlistScalarValue(rp))); + } + Log(LOG_LEVEL_VERBOSE, + "C: + Resetting persistent class timer: '%s' (%d minutes)", + pp->promiser, a.context.persistent); + Buffer *buf = StringSetToBuffer(tags, ','); + EvalContextHeapPersistentSave(ctx, pp->promiser, a.context.persistent, + CONTEXT_STATE_POLICY_RESET, BufferData(buf)); + BufferDestroy(buf); + StringSetDestroy(tags); + return PROMISE_RESULT_NOOP; + } + + if (class_expression_true) { if (a.context.expression == NULL) { @@ -131,7 +171,7 @@ PromiseResult VerifyClassPromise(EvalContext *ctx, const Promise *pp, ARG_UNUSED pp->promiser, a.context.persistent); Buffer *buf = StringSetToBuffer(tags, ','); EvalContextHeapPersistentSave(ctx, pp->promiser, a.context.persistent, - CONTEXT_STATE_POLICY_RESET, BufferData(buf)); + a.context.timer, BufferData(buf)); BufferDestroy(buf); } if (inserted && (comment != NULL)) diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf new file mode 100644 index 0000000000..30997d2a92 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf @@ -0,0 +1,92 @@ +####################################################### +# +# CFE-4681: classes: promises with timer_policy => "absolute" (default) +# +# Verify that timer_policy => "absolute" (the default) on a classes: +# promise preserves the persistence timer across agent runs. +# +# This is the counterpart to persistent_timer_policy_reset.cf and +# directly demonstrates the "promise is not evaluated" behaviour: +# once the persistent class is loaded from the DB on a subsequent +# run, the classes: promise is skipped in ExpandDeRefPromise (it is +# never handed to VerifyClassPromise), so the timer is left untouched. +# +# First run: expect "Creating persistent class ... policy preserve" +# Second run: expect "Skipping evaluation of classes promise ... is +# already set" and NO "Resetting persistent class". +# +# Note: the skip message is logged at LOG_LEVEL_DEBUG, so the second +# run uses -d. Both sub-agent runs happen in the test bundle so that +# the check bundle only evaluates assertions -- this avoids a spurious +# FAIL report during the agent's convergence passes. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + # Remove the persistent class DB to ensure a clean state. + files: + "$(sys.workdir)/state/cf_state.lmdb" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb-lock" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb.lock" + delete => tidy; +} + +bundle agent test +{ + meta: + "description" -> { "CFE-4681" } + string => "timer_policy => absolute (default) on classes: promises preserves the timer; the promise is skipped (not evaluated) when the class is already defined"; + + commands: + # First run: define the persistent class with timer_policy => "absolute". + !first_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_policy_run1.log 2>&1" + contain => in_shell, + classes => always("first_done"); + + # Second run: the class is already loaded from the persistent DB, so with + # timer_policy => absolute the classes: promise must be skipped (not + # evaluated) and the timer must not be reset. Run with -d so the + # DEBUG-level skip message is captured. + first_done.!second_done:: + "$(sys.cf_agent) -Kd -f $(this.promise_filename).sub > $(G.testdir)/timer_policy_run2.log 2>&1" + contain => in_shell, + classes => always("second_done"); +} + +bundle agent check +{ + classes: + # Run 1 creates the persistent class with the preserve (absolute) policy. + "create_ok" expression => regline(".*Creating persistent class.*timer_policy_test_class.*policy preserve.*", + "$(G.testdir)/timer_policy_run1.log"); + # Run 2 must skip the promise because the class is already defined. + "skip_ok" expression => regline(".*Skipping evaluation of classes promise as class.*timer_policy_test_class.*is already set.*", + "$(G.testdir)/timer_policy_run2.log"); + # Run 2 must NOT reset the timer (no EvalContextHeapPersistentSave call). + "reset_seen" expression => regline(".*Resetting persistent class.*timer_policy_test_class.*", + "$(G.testdir)/timer_policy_run2.log"); + "ok" expression => "create_ok.skip_ok.!reset_seen"; + + reports: + DEBUG.!create_ok:: + "FAIL: first run did not log 'Creating persistent class ... policy preserve'"; + DEBUG.!skip_ok:: + "FAIL: second run did not skip the classes promise (expected 'Skipping evaluation of classes promise ... is already set')"; + DEBUG.reset_seen:: + "FAIL: second run reset the timer despite timer_policy => absolute"; + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf.sub b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf.sub new file mode 100644 index 0000000000..ca6dbb9c87 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy.cf.sub @@ -0,0 +1,15 @@ +body common control +{ + bundlesequence => { run }; +} + +bundle agent run +{ + classes: + # Define persistent class with timer_policy => "absolute" + # This stores CONTEXT_STATE_POLICY_PRESERVE in the DB + "timer_policy_test_class" + expression => "any", + persistence => "120", + timer_policy => "absolute"; +} diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf new file mode 100644 index 0000000000..925fb38c9e --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf @@ -0,0 +1,83 @@ +####################################################### +# +# CFE-4681: classes: promises with timer_policy => "reset" +# +# Verify that timer_policy => "reset" on a classes: promise +# causes the persistence timer to be reset on subsequent +# agent runs, even though the class is already defined +# (loaded from the persistent DB). +# +# First run: expect "Creating persistent class" +# Second run: expect "Resetting persistent class" (not skipped) +# +# Both sub-agent runs happen in the test bundle so that the check +# bundle only evaluates assertions -- this avoids a spurious FAIL +# report during the agent's convergence passes. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + # Remove the persistent class DB to ensure a clean state. + files: + "$(sys.workdir)/state/cf_state.lmdb" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb-lock" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb.lock" + delete => tidy; +} + +bundle agent test +{ + meta: + "description" -> { "CFE-4681" } + string => "timer_policy => reset on classes: promises resets the persistence timer on subsequent runs"; + + commands: + # First run: define the persistent class. + !first_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_reset_run1.log 2>&1" + contain => in_shell, + classes => always("first_done"); + + # Second run: the class already exists in the DB; timer_policy => reset + # must cause the promise to be evaluated and the timer to be reset. + first_done.!second_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_reset_run2.log 2>&1" + contain => in_shell, + classes => always("second_done"); +} + +bundle agent check +{ + classes: + "first_ok" expression => regline(".*Creating persistent class.*timer_reset_test_class.*", + "$(G.testdir)/timer_reset_run1.log"); + # Match the EvalContextHeapPersistentSave message specifically (it + # reports "... timer to N minutes (was M remaining)"). This only + # appears when the existing DB record is actually found, so it would + # NOT match the "C: + Resetting persistent class timer: ..." progress + # line that VerifyClassPromise logs regardless. This distinction is + # what makes the test catch a broken existing-record lookup. + "second_ok" expression => regline(".*Resetting persistent class 'timer_reset_test_class' timer to.*", + "$(G.testdir)/timer_reset_run2.log"); + "ok" expression => "first_ok.second_ok"; + + reports: + DEBUG.!first_ok:: + "FAIL: first run did not log 'Creating persistent class'"; + DEBUG.!second_ok:: + "FAIL: second run did not log 'Resetting persistent class' (short-circuit not bypassed)"; + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf.sub b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf.sub new file mode 100644 index 0000000000..a5624056a7 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_reset.cf.sub @@ -0,0 +1,16 @@ +body common control +{ + bundlesequence => { run }; +} + +bundle agent run +{ + classes: + # Define persistent class with timer_policy => "reset" + # On second run, the timer should be reset even though + # the class is already defined from the persistent DB. + "timer_reset_test_class" + expression => "any", + persistence => "120", + timer_policy => "reset"; +} diff --git a/tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf b/tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf new file mode 100644 index 0000000000..3e8359c40f --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf @@ -0,0 +1,49 @@ +####################################################### +# +# CFE-4681: classes: promises reject timer_policy without persistence +# +# timer_policy only governs the persistence timer, so using it on a +# classes: promise that has no 'persistence' attribute has no meaning. +# Verify that doing so is an error (PROMISE_RESULT_FAIL) rather than +# being silently ignored. +# +# The offending policy is run in a sub-agent so the check bundle only +# evaluates assertions against its captured output. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent test +{ + meta: + "description" -> { "CFE-4681" } + string => "Using timer_policy without persistence on a classes: promise is an error"; + + commands: + !done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_policy_no_persistence.log 2>&1" + contain => in_shell, + classes => always("done"); +} + +bundle agent check +{ + classes: + "error_ok" expression => regline(".*Cannot use 'timer_policy' without 'persistence'.*timer_policy_no_persistence_class.*", + "$(G.testdir)/timer_policy_no_persistence.log"); + "ok" expression => "error_ok"; + + reports: + DEBUG.!error_ok:: + "FAIL: did not log the expected 'timer_policy without persistence' error"; + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf.sub b/tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf.sub new file mode 100644 index 0000000000..4487729528 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/timer_policy_requires_persistence.cf.sub @@ -0,0 +1,14 @@ +body common control +{ + bundlesequence => { run }; +} + +bundle agent run +{ + classes: + # timer_policy only governs the persistence timer, so using it + # without 'persistence' is an error (PROMISE_RESULT_FAIL). + "timer_policy_no_persistence_class" + expression => "any", + timer_policy => "reset"; +} diff --git a/tests/unit/eval_context_test.c b/tests/unit/eval_context_test.c index e47b6e1a4b..77234beda8 100644 --- a/tests/unit/eval_context_test.c +++ b/tests/unit/eval_context_test.c @@ -152,6 +152,51 @@ void test_eval_with_token_from_list(void) StringSetDestroy(time_classes); } +static void test_persistent_class_timer_policy(void) +{ + EvalContext *ctx = EvalContextNew(); + + /* Save a persistent class with PRESERVE policy, 60 minute TTL */ + EvalContextHeapPersistentSave(ctx, "timer_test", 60, + CONTEXT_STATE_POLICY_PRESERVE, "tag1"); + + /* Verify the class loads correctly after PRESERVE save */ + EvalContextHeapPersistentLoadAll(ctx); + + { + const Class *cls = EvalContextClassGet(ctx, "default", "timer_test"); + assert_true(cls != NULL); + assert_string_equal("timer_test", cls->name); + } + + /* Save again with PRESERVE -- the function should early-return + * (class is preserved, not expired, same tags), leaving the DB + * record unchanged. We verify by loading persistent classes and + * checking the class is still defined. */ + EvalContextHeapPersistentSave(ctx, "timer_test", 60, + CONTEXT_STATE_POLICY_PRESERVE, "tag1"); + + /* Class should still be defined after the second PRESERVE save */ + { + const Class *cls = EvalContextClassGet(ctx, "default", "timer_test"); + assert_true(cls != NULL); + assert_string_equal("timer_test", cls->name); + } + + /* Save with RESET policy -- the record SHOULD be overwritten. + * The class should still be loadable afterward. */ + EvalContextHeapPersistentSave(ctx, "timer_test", 60, + CONTEXT_STATE_POLICY_RESET, "tag1"); + + { + const Class *cls = EvalContextClassGet(ctx, "default", "timer_test"); + assert_true(cls != NULL); + assert_string_equal("timer_test", cls->name); + } + + EvalContextDestroy(ctx); +} + int main() { PRINT_TEST_BANNER(); @@ -160,6 +205,7 @@ int main() const UnitTest tests[] = { unit_test(test_class_persistence), + unit_test(test_persistent_class_timer_policy), unit_test(test_changes_chroot), unit_test(test_eval_with_token_from_list), }; From c5ab252c11057240598bdebdfed1b1b552f995c3 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 11 Jun 2026 11:59:39 -0500 Subject: [PATCH 3/3] Changed default timer_policy for classes: promises to reset CFE-4681 added a timer_policy attribute to classes: promises with the default "absolute" (preserve) for backward compatibility. For 3.28.0 the default becomes "reset", aligning classes: promises with classes bodies (whose timer_policy has always defaulted to reset) and matching the common expectation that a persistent class which keeps being rediscovered should keep persisting rather than lapsing. - GetContextConstraints(): default the resolved timer policy to RESET; only an explicit timer_policy => "absolute" selects PRESERVE. - ExpandDeRefPromise(): an already-defined persistent class is no longer skipped unless timer_policy => "absolute" is set, so the default reset can refresh the timer. - Update the classes: promise syntax doc default to "reset". - Acceptance test persistent_timer_policy_default.cf: a classes: promise with no timer_policy resets the timer on a subsequent run. This is a master-only change. The timer_policy attribute itself is being backported to the 3.27.x and 3.24.x LTS streams (3.27.2, 3.24.5) with the long-standing "absolute" default preserved, so behavior there is unchanged and reset remains opt-in. Ticket: CFE-4681 Co-Authored-By: Claude Opus 4.8 (1M context) --- libpromises/attributes.c | 19 ++-- libpromises/mod_common.c | 2 +- libpromises/promises.c | 6 +- .../persistent_timer_policy_default.cf | 89 +++++++++++++++++++ .../persistent_timer_policy_default.cf.sub | 16 ++++ 5 files changed, 118 insertions(+), 14 deletions(-) create mode 100644 tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf create mode 100644 tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf.sub diff --git a/libpromises/attributes.c b/libpromises/attributes.c index cdb87cc63d..ddc3138e8e 100644 --- a/libpromises/attributes.c +++ b/libpromises/attributes.c @@ -1134,21 +1134,18 @@ ContextConstraint GetContextConstraints(const EvalContext *ctx, const Promise *p { const char *tp = PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR); - if (StringEqual(tp, "reset")) + if (StringEqual(tp, "absolute")) { - a.timer = CONTEXT_STATE_POLICY_RESET; + a.timer = CONTEXT_STATE_POLICY_PRESERVE; } else { - /* Default to PRESERVE (absolute) for backward compatibility: - * classes: promises historically skip re-evaluation when the - * class is already defined, so the timer was never reset. - * - * NOTE: this default is planned to change to RESET in 3.28.0 - * (CFE-4681) to align with classes bodies, whose timer_policy - * already defaults to reset. The absolute default is retained on - * the 3.24 and 3.27 LTS backports. */ - a.timer = CONTEXT_STATE_POLICY_PRESERVE; + /* Default to RESET: a persistent class that keeps being + * rediscovered should keep persisting. This aligns classes: + * promises with classes bodies, whose timer_policy has always + * defaulted to reset. Set timer_policy => "absolute" to keep + * the original expiry instead. */ + a.timer = CONTEXT_STATE_POLICY_RESET; } } diff --git a/libpromises/mod_common.c b/libpromises/mod_common.c index 105e47fa86..7b1a59967a 100644 --- a/libpromises/mod_common.c +++ b/libpromises/mod_common.c @@ -226,7 +226,7 @@ const ConstraintSyntax CF_CLASSBODY[] = ConstraintSyntaxNewContext("not", "Evaluate the negation of string expression in normal form", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("select_class", "Select one of the named list of classes to define based on host identity. Default value: random_selection", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewContextList("xor", "Combine class sources with XOR", SYNTAX_STATUS_NORMAL), - ConstraintSyntaxNewOption("timer_policy", "absolute,reset", "Whether a persistent class restarts its counter when rediscovered. Default value: absolute", SYNTAX_STATUS_NORMAL), + ConstraintSyntaxNewOption("timer_policy", "absolute,reset", "Whether a persistent class restarts its counter when rediscovered. Default value: reset", SYNTAX_STATUS_NORMAL), ConstraintSyntaxNewNull() }; diff --git a/libpromises/promises.c b/libpromises/promises.c index 9e955f5496..93e3bc7739 100644 --- a/libpromises/promises.c +++ b/libpromises/promises.c @@ -705,8 +705,10 @@ Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded) pcopy->org_pp = pp->org_pp; // if this is a class promise, check if it is already set, if so, skip - // Exception: persistent classes with timer_policy => "reset" must not + // Exception: persistent classes with the reset timer policy must not // be skipped — the promise needs to fire so the timer gets reset. + // reset is the default, so only an explicit timer_policy => "absolute" + // restores the skip. if (strcmp("classes", PromiseGetPromiseType(pp)) == 0) { if (IsDefinedClass(ctx, CanonifyName(pcopy->promiser))) @@ -714,7 +716,7 @@ Promise *ExpandDeRefPromise(EvalContext *ctx, const Promise *pp, bool *excluded) const char *tp = PromiseGetConstraintAsRval(pp, "timer_policy", RVAL_TYPE_SCALAR); int persistence = PromiseGetConstraintAsInt(ctx, "persistence", pp); - if (StringEqual(tp, "reset") && persistence > 0) + if (!StringEqual(tp, "absolute") && persistence > 0) { Log(LOG_LEVEL_DEBUG, "Class '%s' is already set but timer_policy is reset" diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf new file mode 100644 index 0000000000..fca2f2cda6 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf @@ -0,0 +1,89 @@ +####################################################### +# +# CFE-4681: classes: promises default timer_policy is "reset" +# +# Verify that a classes: promise with NO timer_policy attribute +# resets the persistence timer on subsequent agent runs (the +# default is "reset"), even though the class is already defined +# (loaded from the persistent DB). +# +# This is the counterpart to persistent_timer_policy.cf, which sets +# timer_policy => "absolute" explicitly and expects the promise to +# be skipped. Here we omit the attribute entirely and expect the +# reset behaviour from the default. +# +# First run: expect "Creating persistent class" +# Second run: expect "Resetting persistent class" (not skipped) +# +# Both sub-agent runs happen in the test bundle so that the check +# bundle only evaluates assertions -- this avoids a spurious FAIL +# report during the agent's convergence passes. +# +####################################################### + +body common control +{ + inputs => { "../../default.sub.cf" }; + bundlesequence => { default("$(this.promise_filename)") }; + version => "1.0"; +} + +bundle agent init +{ + # Remove the persistent class DB to ensure a clean state. + files: + "$(sys.workdir)/state/cf_state.lmdb" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb-lock" + delete => tidy; + "$(sys.workdir)/state/cf_state.lmdb.lock" + delete => tidy; +} + +bundle agent test +{ + meta: + "description" -> { "CFE-4681" } + string => "classes: promises default to timer_policy => reset, resetting the persistence timer on subsequent runs"; + + commands: + # First run: define the persistent class (no timer_policy). + !first_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_default_run1.log 2>&1" + contain => in_shell, + classes => always("first_done"); + + # Second run: the class already exists in the DB; the default + # timer_policy (reset) must cause the promise to be evaluated and + # the timer to be reset. + first_done.!second_done:: + "$(sys.cf_agent) -Kv -f $(this.promise_filename).sub > $(G.testdir)/timer_default_run2.log 2>&1" + contain => in_shell, + classes => always("second_done"); +} + +bundle agent check +{ + classes: + "first_ok" expression => regline(".*Creating persistent class.*timer_default_test_class.*", + "$(G.testdir)/timer_default_run1.log"); + # Match the EvalContextHeapPersistentSave message specifically (it + # reports "... timer to N minutes (was M remaining)"). This only + # appears when the existing DB record is actually found, so it would + # NOT match the "C: + Resetting persistent class timer: ..." progress + # line that VerifyClassPromise logs regardless. This distinction is + # what makes the test catch a broken existing-record lookup. + "second_ok" expression => regline(".*Resetting persistent class 'timer_default_test_class' timer to.*", + "$(G.testdir)/timer_default_run2.log"); + "ok" expression => "first_ok.second_ok"; + + reports: + DEBUG.!first_ok:: + "FAIL: first run did not log 'Creating persistent class'"; + DEBUG.!second_ok:: + "FAIL: second run did not log 'Resetting persistent class' (default reset not applied)"; + ok:: + "$(this.promise_filename) Pass"; + !ok:: + "$(this.promise_filename) FAIL"; +} diff --git a/tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf.sub b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf.sub new file mode 100644 index 0000000000..be070503f2 --- /dev/null +++ b/tests/acceptance/02_classes/01_basic/persistent_timer_policy_default.cf.sub @@ -0,0 +1,16 @@ +body common control +{ + bundlesequence => { run }; +} + +bundle agent run +{ + classes: + # Define a persistent class with NO timer_policy attribute. + # The default is "reset", so on the second run the timer should + # be reset even though the class is already defined from the + # persistent DB. + "timer_default_test_class" + expression => "any", + persistence => "120"; +}