Skip to content

CFE-4681: timer_policy support for classes: promises#6167

Open
nickanderson wants to merge 2 commits into
cfengine:masterfrom
nickanderson:CFE-4681/timer-policy-classes-promises
Open

CFE-4681: timer_policy support for classes: promises#6167
nickanderson wants to merge 2 commits into
cfengine:masterfrom
nickanderson:CFE-4681/timer-policy-classes-promises

Conversation

@nickanderson

@nickanderson nickanderson commented Jun 5, 2026

Copy link
Copy Markdown
Member

Summary

  • Add timer_policy attribute to classes: promise type, allowing users to control whether a persistent class timer resets on re-evaluation (reset) or preserves the original expiry (absolute)
  • Default timer_policy for classes: promises is absolute (PRESERVE) for backward compatibility — historically these promises skip re-evaluation when the class is already defined, so the timer was never reset
  • Bypass the ExpandDeRefPromise skip when timer_policy => "reset" is explicitly set with persistence > 0, allowing the promise to reach VerifyClassPromise so the DB timer can be updated
  • Add second-layer bypass in VerifyClassPromise for the case where EvalClassExpression returns false (class already in context from persistent DB) but timer_policy is reset
  • Fix ValueSizeDB key length to include null terminator (strlen+1), matching ReadDB/WriteDB conventions

Test plan

  • Manual test: timer_policy => "absolute" creates class with "policy preserve", second run skips the promise
  • Manual test: timer_policy => "reset" creates class with "policy reset", second run resets the timer
  • Acceptance test persistent_timer_policy.cf: verifies absolute/preserve log output
  • Acceptance test persistent_timer_policy_reset.cf: verifies timer reset across two agent runs
  • CI acceptance tests pass

Ticket: CFE-4681

🤖 Generated with Claude Code

@cf-bottom

Copy link
Copy Markdown

Thank you for submitting a PR! Maybe @craigcomstock can review this?

@nickanderson nickanderson force-pushed the CFE-4681/timer-policy-classes-promises branch 2 times, most recently from 1ff08f3 to 41cb477 Compare June 7, 2026 23:57
@nickanderson

Copy link
Copy Markdown
Member Author

@cf-bottom jenkins please

@cf-bottom

Copy link
Copy Markdown

@nickanderson nickanderson requested review from larsewi, olehermanse and vpodzime and removed request for larsewi June 8, 2026 14:24

@vpodzime vpodzime left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea here is that the class promise would be in a file that's not loaded at all? How else would one make sure the promise is not evaluated? Classes are part of pre-eval, right? I think we should have a test case for this as well.

Comment thread libpromises/attributes.c Outdated
Comment thread libpromises/eval_context.c
@nickanderson

Copy link
Copy Markdown
Member Author

Thanks for taking a look at my 🤖 ✨ Feel free to be harsh.

So the idea here is that the class promise would be in a file that's not loaded at all? How else would one make sure the promise is not evaluated?

So, the thing is that currently/prior to this PR there are two ways to define a persistent class.

  1. Via a classes promise.
classes:
  "persistent"
    persistence => "100";

Currently there is no way to extend the time remaining that the persistent class should be defined. It The classes promise itself won't even trigger if the promiser is a defined class (optimization for not doing work you don't need to do), so you have to wait for the class to go beyond the timer and then be undefined, so it may then not be defined for some time during next policy evaluation (e.g. maybe it was guarded behind a separate class that was the result of a promise so that it wasnt resolved during pre-eval) until the classes promise is encountered and evaluated.

  1. Via a classes body / promise result.
body classes results_persist_timer(class_prefix, persist_min, timer)
{
      inherit_from => results( "namespace", "$(class_prefix)" );
      persist_time => "$(persist_min)";

      #  (extend|absolute) whether to extend the time left on persistence or not
      timer_policy => "extend"; 
}

Classes bodies already have the ability to either extend the timer or not. Extending the timer helps to avoid that hole where a class might not be defined for a period of time until the promise re-defines it.

Classes are part of pre-eval, right? I think we should have a test case for this as well.

Yes, but only classes in common bundles are part of pre-eval: https://docs.cfengine.com/docs/lts/reference/language-concepts/policy-evaluation/#agent-pre-evaluation-step

The following steps are executed per-bundle for each file parsed, in this order:

if it’s a common bundle, evaluate vars promises
if it’s a common bundle, evaluate classes promises
evaluate vars promises (for details see PolicyResolve() in the C code)

@nickanderson nickanderson force-pushed the CFE-4681/timer-policy-classes-promises branch from 41cb477 to 4c500a8 Compare June 11, 2026 13:11
@nickanderson

Copy link
Copy Markdown
Member Author

On testing that the promise isn't evaluated: added persistent_timer_policy.cf as the absolute (default) counterpart. It runs the agent twice and asserts that on the second run the classes: promise is skipped in ExpandDeRefPromise (Skipping evaluation of classes promise … is already set) and the timer is not reset — i.e. the promise is genuinely not evaluated once the class is loaded from the persistent DB.

You're right that classes are pre-eval, but only for common bundles; the skip itself happens in ExpandDeRefPromise, which applies to both pre-eval and normal agent-bundle evaluation, so the test uses an agent bundle.

I also tightened the reset test so it actually catches a broken existing-record lookup (confirmed: it fails without the ValueSizeDB fix and passes with it), and moved both tests' two sub-agent runs into the test bundle to remove a spurious FAIL report during the agent's convergence passes.

@nickanderson

Copy link
Copy Markdown
Member Author

@cf-bottom jenkins please

@cf-bottom

Copy link
Copy Markdown

@vpodzime vpodzime left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks good to me except for the first commit message which is too long and confusing (at least to me) -- I'd not refer to code that was never executed due to a bug in an if condition as dead code. It can easily just say that it fixes how the key is looked up which actually enables the behavior relying on its existence in the DB and the related value.

nickanderson and others added 2 commits June 11, 2026 10:57
…ntSave

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) <noreply@anthropic.com>
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"). The default is "absolute" (PRESERVE)
for backward compatibility: classes: promises historically skip
re-evaluation when the class is already defined, so the timer was never
reset.

- Bypass the ExpandDeRefPromise skip when timer_policy is explicitly
  set to "reset" with persistence > 0, allowing the promise to reach
  VerifyClassPromise so the DB timer can be updated.
- Add second-layer bypass in VerifyClassPromise for the case where
  EvalClassExpression returns false (class already in context from the
  persistent DB) but timer_policy is "reset" -- write the DB entry to
  reset the timer.
- Acceptance tests: persistent_timer_policy_reset.cf verifies the timer
  resets across agent runs; persistent_timer_policy.cf verifies that
  with the default "absolute" policy the promise is skipped (not
  evaluated) on a subsequent run and the timer is preserved. Both run
  their two sub-agent invocations in the test bundle to avoid a spurious
  FAIL report during the agent's convergence passes.

Ticket: CFE-4681
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nickanderson nickanderson force-pushed the CFE-4681/timer-policy-classes-promises branch from 4c500a8 to e5ec752 Compare June 11, 2026 15:57
@nickanderson

Copy link
Copy Markdown
Member Author

@vpodzime Good call — reworded the first commit. Dropped the "dead code" framing and shortened it: the subject now points at the actual bug (the key length used in the lookup), and the body explains it in terms of the behavior the fix enables — finding the existing record so the preserve/reset logic can act on it — rather than describing a dead branch. Force-pushed (diff unchanged).

@nickanderson

Copy link
Copy Markdown
Member Author

@cf-bottom jenkins please

@cf-bottom

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants