ENT-14194: Run policy on event by victormlg · Pull Request #6362 · cfengine/core

victormlg · 2026-09-22T17:31:56Z

No description provided.

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

+    VarRefDestroy(ref);
+}
+
+static PromiseResult KeepAgentPromise(EvalContext *ctx, const Promise *pp, ARG_UNUSED void *param)


Signed-off-by: Victor Moene <victor.moene@northern.tech>

larsewi

Please do a review with Claude. For me it found a few things that seem plausible:

A remote copy_from in a then bundle crashes cf-reactor
State builds up across repeated runs of the same bundle
The EvalAborted branch is dead code

Signed-off-by: Victor Moene <victor.moene@northern.tech>

Promises are skipped to prevent running them several times. However, on event, we want to run them every single time: - we clear the promise lock cache - we set the default if_elapsed time for bundles run from an events promise to be 0, so it doesn't skip the promises. Fixed also connection cache and custom promise prologue and epilogue. Clear function cache before "then" bundle run Signed-off-by: Victor Moene <victor.moene@northern.tech>

- No need for map anymore, since watcher keeps track of the promise - No need to keep key as variable anymore, since we do not use a map - No need to run bundle inside watcher.c - No risk of running a bundle from a wrong key between policy reads Signed-off-by: Victor Moene <victor.moene@northern.tech>

Signed-off-by: Victor Moene <victor.moene@northern.tech>

The issue is that classes and variables declared once will stay declared until the next policy reload. So a class can stay defined between runs, when the state it checks changed. Instead, the state of all vars and classes is saved in a snapshot after a policy reload, and before every "then" bundle run, we restore the snapshort, and the defined classes and vars of the ran bundles will stay until the next event triggered run. Signed-off-by: Victor Moene <victor.moene@northern.tech>

Instead of destroying all watchers, it now keeps the state, such that, even if an event happens during policy reload, it will get detected by comparing the previous state. Signed-off-by: Victor Moene <victor.moene@northern.tech>

+ * removes the ones defined by the previous 'then' bundle runs, defines again
+ * the ones they cancelled and sets back the values they changed. Then
+ * reloads the persistent classes that have not expired. */
+static void ContextSnapshotRestore(EvalContext *ctx)


craigcomstock

looks good generally

craigcomstock · 2026-10-01T16:55:01Z

+    "meta",
+    "vars",
+    "defaults",
+    "classes",                  /* Maelstrom order 2 */


what is this comment? maelstrom order 2?

ah, I see, existing code, copied over... git log can tell us.

funny, this array has been moved BEFORE! from libpromises/constants.c to cf-agent/cf-agent.c: 43035ad

and then that was reverted: 48fe18f

constants.c is rather old, was renamed from src/constants.c to libpromises/constants.c here: 11e2c21#diff-efd2f7f382b829b3e9838d3c76e8221d965eee910658777d3ca9e777c5b4d4cb (Jan 4, 2013)

I couldn't find a commit that added this fun comment. Also didn't find it in https://ftp.gnu.org/gnu/cfengine/ (1.4.0 or 2.0.6 (the oldest and newest sources there)).

craigcomstock · 2026-10-01T17:42:58Z

+ * AGENT_TYPESEQUENCE order, keeping each one with KeepAgentPromise()). It is
+ * kept separate from cf-agent.c so that other components (e.g. cf-reactor,
+ * running the bundle named in an events promise's "then") can run a bundle
+ * the same way cf-agent does, without linking cf-agent's main(). */


This feels like a fairly disruptive change. I wonder if you could instead factor out cf-agent's main() and friends from cf-agent.c say into a cf-agent-main.c and that might be a smaller change that impacts less functional code.

I asked claude to refactor this way and the result looks about as invasive: https://github.com/craigcomstock/core/pull/new/ent-14463-part-3-alt-refactor, maybe take a look and see what you think.

craigcomstock · 2026-10-01T18:19:46Z


-    const Bundle *bundle = NULL;
-    if (bundle_name != NULL)
+    const Bundle *bp = NULL;


why the change from bundle to less obvious bp? is this short for something new?

were you trying to convey "bundle pointer" like "pp" is "promise pointer"?

craigcomstock · 2026-10-01T18:24:10Z

+
+    if (EvalAborted(ctx))
+    {
+        Log(LOG_LEVEL_ERR, "Eval aborted");


what does "eval aborted" mean? might be nice to explaing more about this.

craigcomstock · 2026-10-01T18:27:39Z

+    EvalContextStackPushBundleSectionFrame(ctx, pp->parent_section);
+    ExpandPromise(ctx, pp, KeepEventsPromiseOnEvent, &event);
+    EvalContextStackPopFrame(ctx);
+    EvalContextStackPopFrame(ctx);


might be nice to add a comment after each about which stack frame you think you are popping: bundle section and then bundle I assume.

craigcomstock · 2026-10-01T18:30:24Z

+        const Watcher *w = MapGet(watchers_by_key, key);

-        if (bundle == NULL)
+        if (w == NULL)


might as well call this watcher instead of w.

craigcomstock · 2026-10-01T19:04:38Z

+    }
+    VariableTableIteratorDestroy(snapshot_iter);
+}
+


all of this snapshot code feels a bit generic. I wonder if most of it could live in a more shared location like libpromises?

craigcomstock · 2026-10-01T19:10:28Z

@@ -0,0 +1,602 @@
+# Makefile.in generated by automake 1.16.5 from Makefile.am.


asan makefile should not be committed.

larsewi · 2026-10-02T09:30:26Z

    EvalContextSetBundleArgs(ctx, NULL);
    EndBundleBanner(bp);

-    if (EvalAborted(ctx))


Maybe this should have not been added to begin with in the previous commit?

larsewi · 2026-10-02T09:37:20Z

+static bool ifelapsed_pushed = false;
+static int pushed_ifelapsed = 0;


We should really try to avoid these static variables. Global variables are evil. And making them file private does not help that much. What if you return then from the push function and pass them to the pop function? Also the wording push and pop makes me think it is a stack, which this is not.

How about

int old_value = OverrideIfelapsed(0); /* Do your thing */ RestoreIfelapsed(old_value);

github-advanced-security Bot found potential problems Sep 22, 2026

View reviewed changes

victormlg changed the title ~~ENT-14463: Run policy on event~~ ENT-14194: Run policy on event Sep 24, 2026

victormlg force-pushed the ENT-14463-part-3 branch 2 times, most recently from b7d8916 to 33ee80a Compare September 29, 2026 11:26

github-advanced-security Bot found potential problems Sep 29, 2026

View reviewed changes

Comment thread cf-reactor/reactor_transform.c Fixed

Comment thread cf-reactor/reactor_transform.c Fixed

Comment thread cf-reactor/reactor_transform.c Fixed

victormlg force-pushed the ENT-14463-part-3 branch 3 times, most recently from f98bd05 to 36fa38f Compare September 30, 2026 15:11

Moved ScheduleAgentOperation to agent_operation.c

433fff5

Signed-off-by: Victor Moene <victor.moene@northern.tech>

victormlg force-pushed the ENT-14463-part-3 branch from 36fa38f to 0f805bb Compare September 30, 2026 15:17

victormlg marked this pull request as ready for review September 30, 2026 15:18

github-advanced-security Bot found potential problems Sep 30, 2026

View reviewed changes

Comment thread cf-reactor/reactor_transform.c Fixed

Comment thread cf-reactor/reactor_transform.c Fixed

Comment thread cf-reactor/reactor_transform.c Fixed

victormlg requested review from craigcomstock and larsewi October 1, 2026 06:15

larsewi reviewed Oct 1, 2026

View reviewed changes

Comment thread cf-reactor/reactor_transform.c

Comment thread cf-reactor/reactor_transform.c Outdated

Comment thread cf-reactor/reactor_transform.c Outdated

victormlg added 4 commits October 1, 2026 15:24

Run bundles on event

f6e1c9c

Signed-off-by: Victor Moene <victor.moene@northern.tech>

Fix watcher test

4f8c52e

Signed-off-by: Victor Moene <victor.moene@northern.tech>

victormlg force-pushed the ENT-14463-part-3 branch from 0f805bb to 1bb39a3 Compare October 1, 2026 18:01

victormlg added 2 commits October 1, 2026 20:13

Made cf-reactor reuse an event state from a policy reload

5a8c477

Instead of destroying all watchers, it now keeps the state, such that, even if an event happens during policy reload, it will get detected by comparing the previous state. Signed-off-by: Victor Moene <victor.moene@northern.tech>

victormlg force-pushed the ENT-14463-part-3 branch from 1bb39a3 to 5a8c477 Compare October 1, 2026 18:13

github-advanced-security Bot found potential problems Oct 1, 2026

View reviewed changes

craigcomstock approved these changes Oct 1, 2026

View reviewed changes

larsewi reviewed Oct 2, 2026

View reviewed changes

		@@ -0,0 +1,602 @@
		# Makefile.in generated by automake 1.16.5 from Makefile.am.

		static bool ifelapsed_pushed = false;
		static int pushed_ifelapsed = 0;

Conversation

victormlg commented Sep 22, 2026

Uh oh!

github-advanced-security AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

larsewi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craigcomstock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants