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

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

Follow-up PR: #6375

Depends on: https://github.com/cfengine/enterprise/pull/1004

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

craigcomstock

looks good generally

Better concurrency for cf-reactor static state - 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>

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>

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

olehermanse · 2026-10-05T12:36:57Z

@cf-bottom jenkins, please

cf-bottom · 2026-10-05T12:38:13Z

Sure, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/14750/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14750/

victormlg · 2026-10-05T14:42:24Z

Sure, I triggered a build:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/14750/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14750/

Added a fix for the windows failure in enterprise. Restarted it

craigcomstock · 2026-10-05T15:10:07Z

@victormlg for a rebuild in jenkins, it would be nice if we updated the PR with the links... so here I will do it manually:

Jenkins: https://ci.cfengine.com/job/pr-pipeline/14753/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14753/

craigcomstock · 2026-10-05T15:11:35Z

+  included file COSL.txt.
+*/
+
+/* ScheduleAgentOperations() and the promise actuator it uses: this is what


maybe say something more clear here like "This file, agent_operations.c, implements ScheduleAgentOperations() and ..."

craigcomstock · 2026-10-05T15:12:26Z

+ * 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(). */


Maybe refer to a follow-up/todo ticket to refactor/move this code to libpromises.

craigcomstock · 2026-10-05T15:15:13Z

+int CFA_BACKGROUND = 0; /* GLOBAL_X */
+int CFA_BACKGROUND_LIMIT = 1; /* GLOBAL_P */
+
+Item *PROCESSREFRESH = NULL; /* GLOBAL_P */


what do these comments indicate? We might want to include a reference to GLOBALS file as well in each of these comments, even expanding them to GLOBAL_P is set during policy parsing and is OK where-as GLOBAL_X is unclassified and dealt with on a case-by-case basis and so here we should describe the case/reason why this global is OK.

The header file explains does an OK job of explaining the GLOBAL_P globals:

extern int CFA_BACKGROUND; /* GLOBAL_X */ extern int CFA_BACKGROUND_LIMIT; /* GLOBAL_P, body agent control: max_children */ extern Item *PROCESSREFRESH; /* GLOBAL_P, body agent control: refresh_processes */

I see that CFA_BACKGROUND is a counter and is involved primarily in the function ParallelFindAndVerifyFilesPromises() and also referenced in cf-agent.c for a call to EndAudit(ctx, CFA_BACKGROUND) which then calls LogTotalCompliance(version, background_tasks (the value of CFA_BACKGROUND sent in EndAudit()).

LogTotalCompliance() is an enterprise only feature.

The introduction of CFA_BACKGROUND was for "protection against overt use of parallelism" in this commit from Mark in 2009. fd38443

CFA is a short term for cf-agent I believe since in cf3.defs.h in the above commit we also have cfex_ prefix for cf-execd enums.

I wasn't aware of this low-level implementation detail yet and wonder if it is exercised or not.

I think it would be helpful to come back with another commit that adds annotations to this code explaining a few things that we find during review.

craigcomstock · 2026-10-05T15:51:57Z

+    return DefaultVarPromise(ctx, pp);
+}
+
+PromiseResult ScheduleAgentOperations(EvalContext *ctx, const Bundle *bp)


Since this is the crux of this file I think this function deserves some explanation of what it does and such.

I see you have a comment above at the top, that's probably good. Maybe a comment here would include some sort of "API" like doc tags and such since it is expected to be used in multiple places now: cf-agent and cf-reactor.

craigcomstock · 2026-10-05T15:54:11Z

+*/
+
+/* ScheduleAgentOperations() and the promise actuator it uses: this is what
+ * cf-agent does with a single bundle (evaluate its promises in


maybe "this is the function that cf-agent uses to evaluate a single bundle and it's promises in AGENT_TYPESEQUENCE order" or similar... as a way of making it clear what this function does with less "technical/internal" language like "promise actuator".

craigcomstock · 2026-10-05T15:57:47Z

+}
+
+PromiseResult ScheduleAgentOperations(EvalContext *ctx, const Bundle *bp)
+// NB - this function can be called recursively through "methods"


a follow up tidy commit might change NB to just NOTE: I assume this NB means nota bene, latin for note well or pay close attention.

craigcomstock · 2026-10-05T16:03:08Z

+    int save_pr_notkept = PR_NOTKEPT;
+    struct timespec start = BeginMeasure();
+
+    if (PROCESSREFRESH == NULL || (PROCESSREFRESH && IsRegexItemIn(ctx, PROCESSREFRESH, bp->name)))


would be nice to know what manages PROCESSREFRESH global, I see that cf-agent.c does and there is a bit of TODO in that code in KeepControlPromises()

1027 if (strcmp(cp->lval, CFA_CONTROLBODY[AGENT_CONTROL_REFRESH_PROCESSES].lval) == 0) 1028 { 1029 Log(LOG_LEVEL_VERBOSE, "Setting refresh_processes when starting to..."); 1030 for (const Rlist *rp = value; rp != NULL; rp = rp->next) 1031 { 1032 Log(LOG_LEVEL_VERBOSE, "%s", RlistScalarValue(rp)); 1033 // TODO: why is this only done in verbose mode? 1034 // original commit says 'optimization'. 1035 if (LogGetGlobalLevel() >= LOG_LEVEL_VERBOSE) 1036 { 1037 PrependItem(&PROCESSREFRESH, RlistScalarValue(rp), NULL); 1038 } 1039 } 1040 continue; 1041 }

I see we have a condition of PROCESREFRESH == NULL so no impact here when we call this function from cf-reactor(or other binaries) which will likely not set the control promise to a value.

craigcomstock · 2026-10-05T16:16:24Z

+                {
+                    EvalContextStackPopFrame(ctx);
+                    NoteBundleCompliance(bp, save_pr_kept, save_pr_repaired, save_pr_notkept, start);
+                    return result;


I notice that custom promise types don't create/delete a "type context" with NewTypeContext() and DeleteTypeContext(). I see that function only does anything for the deprecated environments promise type and the not so often used storage promise type. Would be nice to know why this type context is skipped for custom promise types. It does seem to be mostly a no-op practically speaking.

craigcomstock · 2026-10-05T16:17:30Z

+            result = PromiseResultUpdate(result, promise_result);
+            if (EvalAborted(ctx) || BundleAbort(ctx))
+            {
+                EvalContextStackPopFrame(ctx);


Here also, we are not calling the "type context" functions. I wonder if we should just remove them entirely or go ahead and include them always. One or the other seems more correct.

craigcomstock · 2026-10-05T17:21:09Z

+            break;
+        }
+        default:
+            /* TODO is CF_DATA_TYPE_NONE acceptable? Today all meta variables


we should ensure a ticket exists and is referenced here for this todo. add in a separate commit, like a batch of tidying i suggested.

craigcomstock

a bit more, will continue later today

craigcomstock · 2026-10-05T17:24:53Z

+    struct timespec start = BeginMeasure();
+    PromiseResult result = PROMISE_RESULT_NOOP;
+
+    if (strcmp("meta", PromiseGetPromiseType(pp)) == 0 ||


this would read easier if we captured the promise type string in a local var.

craigcomstock · 2026-10-05T17:27:48Z

+    {
+        if (!LoadProcessTable())
+        {
+            Log(LOG_LEVEL_ERR, "Unable to read the process table - cannot keep processes: type promises");


seems like it would be better for VerifyProcessesPromise() to handle the proc load function call and return fail

craigcomstock · 2026-10-05T17:29:14Z

+        result = VerifyProcessesPromise(ctx, pp);
+        if (result != PROMISE_RESULT_SKIPPED)
+        {
+            EndMeasurePromise(start, pp);


why are we not calling EndMeasurePromise() for the types above here?

craigcomstock · 2026-10-05T17:30:27Z

+        result = PROMISE_RESULT_NOOP;
+    }
+
+    BannerStatusEnd(result, PromiseGetPromiseType(pp), pp->promiser);


why not refactor all of the EndMeasurePromise function calls to here?

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 force-pushed the ENT-14463-part-3 branch 2 times, most recently from 1bb39a3 to 5a8c477 Compare October 1, 2026 18:13

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

View reviewed changes

Comment thread cf-reactor/reactor_transform.c Fixed

craigcomstock approved these changes Oct 1, 2026

View reviewed changes

larsewi reviewed Oct 2, 2026

View reviewed changes

Comment thread cf-reactor/reactor_transform.c Outdated

Comment thread libpromises/attributes.c Outdated

victormlg force-pushed the ENT-14463-part-3 branch from 5a8c477 to 58f4f25 Compare October 5, 2026 09:18

victormlg added 3 commits October 5, 2026 12:44

Fix watcher test

d02b0bb

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

victormlg force-pushed the ENT-14463-part-3 branch from 58f4f25 to d02b0bb Compare October 5, 2026 10:45

olehermanse requested review from craigcomstock and larsewi October 5, 2026 12:18

olehermanse self-requested a review October 5, 2026 12:37

craigcomstock reviewed Oct 5, 2026

View reviewed changes

Conversation

victormlg commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

craigcomstock 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!

olehermanse commented Oct 5, 2026

Uh oh!

cf-bottom commented Oct 5, 2026

Uh oh!

victormlg commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craigcomstock commented Oct 5, 2026

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigcomstock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

victormlg commented Sep 22, 2026 •

edited

Loading

victormlg commented Oct 5, 2026 •

edited

Loading