Skip to content

CFE-4726: Fixed a commands: promise being reported as compliant after exec_timeout fired - #6299

Open
djbclark wants to merge 2 commits into
cfengine:masterfrom
djbclark:fix/exec-timeout-promise-outcome
Open

CFE-4726: Fixed a commands: promise being reported as compliant after exec_timeout fired#6299
djbclark wants to merge 2 commits into
cfengine:masterfrom
djbclark:fix/exec-timeout-promise-outcome

Conversation

@djbclark

@djbclark djbclark commented Aug 17, 2026

Copy link
Copy Markdown

A commands: promise whose exec_timeout fires is reported as compliant.
The promise result is decided entirely by the child's wait status:
RepairExec() hands it to VerifyCommandRetcode(), and with the default
kept_returncodes an exit status of 0 is reported as repaired — aggregate
compliance 100%, promise_repaired set, repair_timeout not set, and
PromiseResultIsOK() true. Nothing consults whether the alarm fired.

ACTION_RESULT_TIMEOUT is declared in cf-agent/verify_exec.c and
VerifyExecPromise() has a case for it, but no path in the file ever returns
it, so PROMISE_RESULT_TIMEOUT is unreachable for this promise type.

This is a fail-open rather than a reporting nit: a check run under
exec_timeout cannot be distinguished from one that passed, so a policy keying
a later promise off if_ok, if_repaired or depends_on treats "the check
never finished"
as "the check succeeded".

Reproducer

Verified on master 17eb78e6d, macOS 26.6.1 arm64. The command has to exit
during the termination ladder rather than before the timeout:

body common control { bundlesequence => { "t" }; }
body contain c { useshell => "noshell"; exec_timeout => "2"; }
bundle agent t
{
  commands:
      "/bin/sh" arglist => { "-c", "sleep 2.4; exit 0" }, contain => c;
}
verbose: Time out of process 9943
verbose: A: Promise REPAIRED
verbose: A: Aggregate compliance (promises kept/repaired) for bundle 't' = 100.0%

The timeout is logged and then discarded.

This is not a narrow race. The shell is signalled, its own child keeps running,
the shell reaps it and exits 0, and that 0 is what the promise is judged on.
Shortening the termination ladder narrows the window but cannot close it,
because the exit status of a command that was killed is not a reliable report
of whether it was killed.

The change

TimeOut() records that it fired, in a volatile sig_atomic_t written from the
SIGALRM handler; SetTimeOut() clears it so the flag describes only the
command just run. RepairExec() classifies on that flag in preference to the
exit status, and returns ACTION_RESULT_TIMEOUT.

The flag is sampled immediately after cf_pclose() returns, and never earlier.
The output read loop ends as soon as the command closes its output, which it can
do long before it exits, so the alarm may not fire until cf_pclose() is
already waiting for the child — sampling before that wait misses exactly the
shape this is meant to catch.

TimeOut() also records whether it actually had a process to signal.
cf_pclose() clears ALARM_PID before waiting, so a command that closes its
output and then outlives its timeout reaches TimeOut() with nothing to signal:
the alarm fires, the ALARM_PID == -1 branch runs, and the command runs to
completion untouched. Reporting that as a termination would be a false statement
in an error message about a fail-open, so the two cases are worded differently:

Command '...' exceeded exec_timeout of 2 seconds and was terminated
Command '...' exceeded exec_timeout of 2 seconds; it was NOT terminated and ran to completion

Both are PROMISE_RESULT_TIMEOUT at 0% compliance; only the wording differs.

PROMISE_RESULT_TIMEOUT already flows correctly through PromiseResultUpdate()
(CHANGE + TIMEOUT -> TIMEOUT), so only the missing classification and
return had to be supplied.

Known limits, stated up front

  • background => "true" is unchanged. The parent still reports the promise
    kept, so this does not make exec_timeout visible on that path.
  • A residual window between cf_pclose() returning and the alarm being
    disarmed, in which a just-on-time completion could be labelled a timeout.
    Closing it means blocking SIGALRM, then alarm(0), then sampling. Not done
    here.
  • That a timed-out command is not always terminated is a separate defect,
    not addressed here. The "NOT terminated" wording exists to make it visible
    rather than silently mislabelled.

Tests

Five acceptance tests in the new tests/acceptance/08_commands/04_exec_timeout/
(following the existing 01_modules / 02_syntax / 03_shells numbering):
the sleep 2.4 shape; the same command finishing inside its timeout (still
repaired on 0, failed on non-zero); kept_returncodes => { "0" } not
resurrecting "kept"; the output-closed shape; and two sequential commands:
proving the flag does not leak to the next promise.

All five report through dcs_all_classes() from dcs.sub.cf, except the
kept_returncodes test — classes bodies cannot compose, so it carries a local
copy of that body.

Verified on this branch, macOS 26.6.1 arm64:

make -j2                     rc=0 (2 warnings, both pre-existing:
                             evalfunction.c:674, variable.c:296)
the five tests, fixed        5 passed, 0 failed  (53s)

Discrimination checked by reverting only the three source files to stock
17eb78e6d and keeping the new tests:

the five tests, unfixed      4 FAIL, 1 Pass

The one that still passes is within_timeout_normal_outcomes.cf, the
normal-path guard — correct, since it pins behaviour this change does not alter.
The sources were then restored and confirmed byte-identical by sha256, with a
clean tree and a clean rebuild.

Notes

Cut from master 17eb78e6d, which is the commit this was verified against. The
only drift to current master is the libntech submodule bump in #6297, which does
not touch any path in this change.

Tracked as CFE-4726.

Happy to reshape this — split the tests out, close the residual window in the
same PR, or adjust the two message wordings — if you'd prefer it differently.

…out fired

A commands: promise whose exec_timeout fired was still judged solely on the
child's wait status. RepairExec() handed that status to VerifyCommandRetcode(),
and with the default kept_returncodes an exit status of 0 was reported as
repaired: aggregate compliance 100%, promise_repaired set, repair_timeout not
set, and PromiseResultIsOK() true. Nothing consulted whether the alarm had
fired.

ACTION_RESULT_TIMEOUT was declared in cf-agent/verify_exec.c and
VerifyExecPromise() had a case for it, but no path in the file ever returned
it, so PROMISE_RESULT_TIMEOUT was unreachable for this promise type.

This is a fail-open rather than a reporting nit. A check run under exec_timeout
cannot be distinguished from one that passed, so a policy keying a later
promise off if_ok, if_repaired or depends_on treats "the check never finished"
as "the check succeeded".

The exit status of a signalled command does not say whether it ran to
completion: the shell is signalled, its own child keeps running, the shell
reaps it and exits 0, and that 0 is what the promise was judged on. Shortening
the termination ladder narrows the window but cannot close it.

TimeOut() now records that it fired, in a volatile sig_atomic_t written from
the SIGALRM handler, and SetTimeOut() clears it so the flag describes only the
command just run. RepairExec() classifies on that flag in preference to the
exit status, and returns ACTION_RESULT_TIMEOUT.

The flag is sampled immediately after cf_pclose() returns, and never earlier.
The output read loop ends as soon as the command closes its output, which it
can do long before it exits, so the alarm may not fire until cf_pclose() is
already waiting for the child. Sampling before that wait misses exactly the
shape this is meant to catch.

TimeOut() also records whether it actually had a process to signal.
cf_pclose() clears ALARM_PID before waiting, so a command that closes its
output and then outlives its timeout reaches TimeOut() with nothing to signal:
the alarm fires, the ALARM_PID == -1 branch runs, and the command runs to
completion untouched. Reporting that as a termination would be a false
statement in an error message about a fail-open, so the two cases are worded
differently:

  Command '...' exceeded exec_timeout of 2 seconds and was terminated
  Command '...' exceeded exec_timeout of 2 seconds; it was NOT terminated
      and ran to completion

Both are PROMISE_RESULT_TIMEOUT at 0% compliance; only the wording differs.
That a timed-out command is not always terminated is a separate defect, not
addressed here; the second wording makes it visible rather than mislabelled.

PROMISE_RESULT_TIMEOUT already flowed correctly through PromiseResultUpdate()
(CHANGE + TIMEOUT -> TIMEOUT), so only the missing classification and return
had to be supplied.

Two limits are known and unchanged. A promise with background => "true" is
still reported by the parent as kept, so this does not make exec_timeout
visible on that path. And there is a residual window between cf_pclose()
returning and the alarm being disarmed in which a just-on-time completion could
be labelled a timeout; closing it means blocking SIGALRM, then alarm(0), then
sampling.

Commands finishing within their timeout are unaffected: exit 0 still repairs
the promise and a non-zero exit still fails it.

Ticket: CFE-4726
Changelog: Title
Five tests in the new tests/acceptance/08_commands/04_exec_timeout/, covering
the outcome classification of a commands: promise run under exec_timeout:

- timeout_overrides_exit_zero.cf: a fired timeout is reported as
  repair_timeout even though the command exits 0. Without the fix the exit
  status wins and the promise is reported repaired at 100% compliance.
- within_timeout_normal_outcomes.cf: an armed timeout that does not fire
  leaves the outcome to the exit status -- 0 repairs the promise, non-zero
  fails it. Guards the normal path.
- timeout_overrides_kept_returncodes.cf: kept_returncodes => { "0" } does not
  resurrect "kept" when the timeout fired.
- timeout_after_output_closed.cf: the timeout is detected even when the command
  closes its output long before it exits, so the alarm fires only once the
  agent is already waiting for the child. Deliberately slow (about 12
  seconds): with output closed there is no process registered for the alarm to
  signal, so the child runs to completion.
- timeout_does_not_leak_to_next_promise.cf: a fired timeout is charged to the
  promise whose command timed out; the next commands promise, armed with its
  own timeout, still comes out repaired.

All five report through dcs_all_classes() from dcs.sub.cf, except the
kept_returncodes test: classes bodies cannot compose, so it carries a local
copy of that body with kept_returncodes added.

Ticket: CFE-4726
Changelog: none
djbclark added a commit to frdminc/tendcf that referenced this pull request Aug 17, 2026
Also corrects two stale statements the register itself carried:

- The B-8 Item cell still said the promise is reported **kept**. The panel
  retracted that on 2026-08-17 -- the default outcome for exit 0 is
  *repaired*, so the accurate word is **compliant**. Same class of error as
  the never-refile-body-verbatim rule guards against, just in our own record.
- B-8's "still needs its own second opinion" note and the stock-libntech
  gate were both discharged and are now marked so. The gate is genuinely met:
  the core-acceptance worktree carries stock pointer 5b5d04e1, which is what
  upstream master 17eb78e6d records.

The Fix cell listed only 326bcdb8d; it now names all four commits including
the acceptance tests.
@djbclark

Copy link
Copy Markdown
Author

Merged into #6305 along with a process-group fix that touches the same functions (SetTimeOut()/TimeOut()/cf-agent/verify_exec.c). If this lands first, #6305 rebases down to just the process-group commits; otherwise #6305 supersedes it.

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.

1 participant