Add policy run tracking for failing-policy automations - #45918
Add policy run tracking for failing-policy automations#45918juan-fdz-hawa wants to merge 12 commits into
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request implements database persistence and orchestration for policy automation execution outcomes. It introduces Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.go`:
- Around line 16-25: The UNIQUE KEY uk_policy_host_passed on the policy_runs
table prevents multiple pass/fail transitions for the same (policy_id, host_id);
remove that UNIQUE constraint (or replace it with a non-unique KEY) in the
migration AddPolicyRunsAndAutomationExecutions.go so policy_runs can record
repeated fail→pass→fail cycles, and mirror the same change in
server/datastore/mysql/schema.sql (remove or downgrade uk_policy_host_passed to
a normal index and keep idx_policy_created and the foreign keys intact).
In `@server/datastore/mysql/policy_automation_executions.go`:
- Around line 20-36: The chunked write loop in RecordPolicyRuns currently
commits per chunk; wrap the entire loop in a single transactional withRetryTxx
so the whole logical batch is all-or-nothing: start a transaction via
withRetryTxx, iterate over the runs invoking recordPolicyRunsChunk (adjust it to
accept/use the transaction or expose a tx-taking variant), and commit/rollback
once after the full loop instead of per chunk; apply the same transactional
wrapping change to CreatePolicyAutomationExecutions (and its chunk helper) so
both methods perform their full chunked loops inside one withRetryTxx
transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2b6bd29-026e-411e-99df-5ae39d36bdc7
📥 Commits
Reviewing files that changed from the base of the PR and between 60fca4b and 0bb440d188a60b5d2e35168b165e5be624d9b9f2.
📒 Files selected for processing (30)
changes/45477-45478-policy-status-page-backend-modelscmd/fleet/cron.goserver/cron/calendar_cron.goserver/cron/calendar_cron_policy_recording_test.goserver/cron/calendar_cron_test.goserver/datastore/mysql/activities.goserver/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.goserver/datastore/mysql/policy_automation_executions.goserver/datastore/mysql/policy_automation_executions_test.goserver/datastore/mysql/policy_automation_integration_test.goserver/datastore/mysql/policy_run_id_pipeline_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/scripts.goserver/datastore/mysql/software_installers.goserver/fleet/datastore.goserver/fleet/policies.goserver/fleet/scripts.goserver/fleet/software_installer.goserver/mock/datastore_mock.goserver/policy_automation_batch/batch.goserver/policy_automation_batch/batch_test.goserver/service/osquery.goserver/webhooks/failing_policies.goserver/webhooks/failing_policies_test.goserver/worker/jira.goserver/worker/jira_test.goserver/worker/policy_automation_lifecycle_test.goserver/worker/worker.goserver/worker/zendesk.goserver/worker/zendesk_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45918 +/- ##
==========================================
- Coverage 66.89% 66.89% -0.01%
==========================================
Files 2783 2760 -23
Lines 221736 221457 -279
Branches 11392 10915 -477
==========================================
- Hits 148335 148148 -187
+ Misses 60000 59868 -132
- Partials 13401 13441 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/webhooks/failing_policies.go (1)
88-107: 💤 Low valueConsider extracting the finalize-and-log pattern into a helper.
The pattern of calling
UpdatePolicyAutomationExecutionsStatusByBatchand logging errors is repeated at lines 89-91, 100-102, and 105-107. Extracting this into a small helper would reduce duplication and make future changes easier:♻️ Optional refactor to reduce duplication
finalizeBatch := func(outcomeErr error) { if updErr := ds.UpdatePolicyAutomationExecutionsStatusByBatch(ctx, batchID, outcomeErr); updErr != nil { logger.ErrorContext(ctx, "failed to update policy automation executions status", "batch_id", batchID.String(), "err", updErr) } } // Then use it: if err := json.Marshal(&payload); err != nil { finalizeBatch(err) return ctxerr.Wrap(ctx, err, "marshal failing policies payload") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/webhooks/failing_policies.go` around lines 88 - 107, The code repeats calls to ds.UpdatePolicyAutomationExecutionsStatusByBatch(...) with identical logger.ErrorContext logging; extract a small helper (e.g., finalizeBatch := func(outcomeErr error) { if updErr := ds.UpdatePolicyAutomationExecutionsStatusByBatch(ctx, batchID, outcomeErr); updErr != nil { logger.ErrorContext(ctx, "failed to update policy automation executions status", "batch_id", batchID.String(), "err", updErr) } }) and replace the three repeated blocks (the error path after marshal, the http.PostJSONWithTimeout error path where you call http.MaskURLError, and the final success update) with calls to finalizeBatch(err) or finalizeBatch(nil) as appropriate to remove duplication while keeping existing behavior.server/worker/jira.go (1)
476-478: 💤 Low valueConsider operational implications of nested failures.
If
RecordPolicyAutomationBatchsucceeds butQueueJobfails, and thenUpdatePolicyAutomationExecutionsStatusByBatchalso fails, the automation execution records will remain in "pending" state indefinitely with no retry mechanism to correct them.While best-effort error handling is reasonable here, this scenario could lead to stale pending records accumulating over time. Consider documenting this trade-off or adding operational monitoring for executions stuck in pending state beyond a threshold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/worker/jira.go` around lines 476 - 478, The current flow can leave executions stuck in "pending" if RecordPolicyAutomationBatch succeeds but QueueJob and then UpdatePolicyAutomationExecutionsStatusByBatch both fail; update the code around RecordPolicyAutomationBatch, QueueJob and UpdatePolicyAutomationExecutionsStatusByBatch to add a retry/backoff when the status update fails (or alternately persist a failure marker and emit a metric/alert) so pending records are retried or surfaced: implement a small retry loop with exponential backoff for UpdatePolicyAutomationExecutionsStatusByBatch (or enqueue a compensating background job) and emit a counter/metric/log with enough context (batchID) when retries are exhausted to allow operational detection of stuck executions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/policy_automation_executions.go`:
- Around line 191-213: The current RecordPolicyAutomationBatch implementation
calls ds.RecordPolicyRuns and then ds.CreatePolicyAutomationExecutions in
separate transactions so partial success can leave runs without executions; make
the operation atomic by performing both phases in a single DB transaction: start
a transaction (or use the datastore's transactional helper) inside
RecordPolicyAutomationBatch, call RecordPolicyRuns and
CreatePolicyAutomationExecutions using that transaction/context (or convert
those helpers to accept a tx or add new tx-aware variants), and commit only
after both succeed (rollback on any error) so that either both runs and
executions are persisted or neither are.
---
Nitpick comments:
In `@server/webhooks/failing_policies.go`:
- Around line 88-107: The code repeats calls to
ds.UpdatePolicyAutomationExecutionsStatusByBatch(...) with identical
logger.ErrorContext logging; extract a small helper (e.g., finalizeBatch :=
func(outcomeErr error) { if updErr :=
ds.UpdatePolicyAutomationExecutionsStatusByBatch(ctx, batchID, outcomeErr);
updErr != nil { logger.ErrorContext(ctx, "failed to update policy automation
executions status", "batch_id", batchID.String(), "err", updErr) } }) and
replace the three repeated blocks (the error path after marshal, the
http.PostJSONWithTimeout error path where you call http.MaskURLError, and the
final success update) with calls to finalizeBatch(err) or finalizeBatch(nil) as
appropriate to remove duplication while keeping existing behavior.
In `@server/worker/jira.go`:
- Around line 476-478: The current flow can leave executions stuck in "pending"
if RecordPolicyAutomationBatch succeeds but QueueJob and then
UpdatePolicyAutomationExecutionsStatusByBatch both fail; update the code around
RecordPolicyAutomationBatch, QueueJob and
UpdatePolicyAutomationExecutionsStatusByBatch to add a retry/backoff when the
status update fails (or alternately persist a failure marker and emit a
metric/alert) so pending records are retried or surfaced: implement a small
retry loop with exponential backoff for
UpdatePolicyAutomationExecutionsStatusByBatch (or enqueue a compensating
background job) and emit a counter/metric/log with enough context (batchID) when
retries are exhausted to allow operational detection of stuck executions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd0b9888-f5ed-4cb8-879a-270374bbca11
📥 Commits
Reviewing files that changed from the base of the PR and between 0bb440d188a60b5d2e35168b165e5be624d9b9f2 and b6d76d117356b32a02cd2bd3e656946f014be7ba.
📒 Files selected for processing (26)
server/cron/calendar_cron.goserver/cron/calendar_cron_policy_recording_test.goserver/cron/calendar_cron_test.goserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.goserver/datastore/mysql/policy_automation_executions.goserver/datastore/mysql/policy_automation_executions_test.goserver/datastore/mysql/policy_automation_integration_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/strutil.goserver/fleet/calendar_events.goserver/fleet/calendar_events_test.goserver/fleet/datastore.goserver/fleet/policies.goserver/mock/datastore_mock.goserver/policy_automation_batch/automationtest/automationtest.goserver/service/calendar/calendar.goserver/service/osquery.goserver/webhooks/failing_policies.goserver/webhooks/failing_policies_test.goserver/worker/jira.goserver/worker/jira_test.goserver/worker/policy_automation_lifecycle_test.goserver/worker/zendesk.goserver/worker/zendesk_test.go
✅ Files skipped from review due to trivial changes (2)
- server/datastore/mysql/strutil.go
- server/mock/datastore_mock.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/datastore/mysql/policy_automation_executions.go (1)
314-322: 💤 Low valueHardcoded
'success'SQL literal duplicates the Go constant.The status transition WHERE clause compares the bound
statusagainst the string literal'success'. Iffleet.PolicyAutomationStatusSuccessever changes its underlying string value, this literal silently desyncs and the "failure → success upgrade" path stops working without a compile-time signal. Consider passing the success-sentinel by value or composing the literal from the constant.♻️ Suggested tweak
- if _, err := ds.writer(ctx).ExecContext(ctx, - `UPDATE policy_automation_executions - SET status = ?, error_message = ? - WHERE batch_id = ? - AND (status = 'pending' OR (status = 'failure' AND ? = 'success'))`, - status, errPtr, batchID[:], status, + if _, err := ds.writer(ctx).ExecContext(ctx, + `UPDATE policy_automation_executions + SET status = ?, error_message = ? + WHERE batch_id = ? + AND (status = ? OR (status = ? AND ? = ?))`, + status, errPtr, batchID[:], + fleet.PolicyAutomationStatusPending, + fleet.PolicyAutomationStatusFailure, + status, fleet.PolicyAutomationStatusSuccess, ); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/policy_automation_executions.go` around lines 314 - 322, The SQL WHERE clause hardcodes the string 'success' which duplicates the Go constant and can drift; change the query to compare against a bound parameter (or interpolate the constant) instead of the literal and pass fleet.PolicyAutomationStatusSuccess (or its value) as the extra ExecContext argument; update the ExecContext call around ds.writer(ctx).ExecContext(...) that currently binds status, errPtr, batchID[:] so the final argument is fleet.PolicyAutomationStatusSuccess (or construct the SQL using the constant) to ensure the "failure → success" transition uses the centralized fleet.PolicyAutomationStatusSuccess value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/datastore/mysql/policy_automation_executions.go`:
- Around line 314-322: The SQL WHERE clause hardcodes the string 'success' which
duplicates the Go constant and can drift; change the query to compare against a
bound parameter (or interpolate the constant) instead of the literal and pass
fleet.PolicyAutomationStatusSuccess (or its value) as the extra ExecContext
argument; update the ExecContext call around ds.writer(ctx).ExecContext(...)
that currently binds status, errPtr, batchID[:] so the final argument is
fleet.PolicyAutomationStatusSuccess (or construct the SQL using the constant) to
ensure the "failure → success" transition uses the centralized
fleet.PolicyAutomationStatusSuccess value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fff97ad6-da9a-428c-b722-cd1557614939
📥 Commits
Reviewing files that changed from the base of the PR and between b6d76d117356b32a02cd2bd3e656946f014be7ba and 4938d38ec8d327670018be58edff8de7bfbb19e8.
📒 Files selected for processing (1)
server/datastore/mysql/policy_automation_executions.go
4938d38 to
9ce1c4a
Compare
9ce1c4a to
d1451d3
Compare
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
server/datastore/mysql/policy_automation_executions.go (1)
61-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the transition write + failing-ID lookup atomic.
If one chunk commits and a later chunk or the trailing
lookupFailingPolicyRunRefsfails, this host check-in is persisted as a mixed snapshot. Retrying then replays against partially updatedpolicy_runs, which can skewconsecutive_failuresand change which policies look newly failing. Wrap the whole loop and the follow-up lookup in a single transaction.Suggested fix shape
- writer := ds.writer(ctx) - for chunkStart := 0; chunkStart < len(pending); chunkStart += policyAutomationBatchSize { - ... - if _, err := writer.ExecContext(ctx, query, args...); err != nil { - return nil, ctxerr.Wrap(ctx, err, "upsert policy_runs") - } - } - - failingIDs := make(map[uint]uint, len(newFailing)) - if len(newFailing) > 0 { - refs, err := lookupFailingPolicyRunRefs(ctx, writer, newFailing, []uint{hostID}) - ... - } - return failingIDs, nil + failingIDs := make(map[uint]uint, len(newFailing)) + if err := ds.withTx(ctx, func(tx sqlx.ExtContext) error { + for chunkStart := 0; chunkStart < len(pending); chunkStart += policyAutomationBatchSize { + ... + if _, err := tx.ExecContext(ctx, query, args...); err != nil { + return ctxerr.Wrap(ctx, err, "upsert policy_runs") + } + } + + if len(newFailing) == 0 { + return nil + } + refs, err := lookupFailingPolicyRunRefs(ctx, tx, newFailing, []uint{hostID}) + if err != nil { + return ctxerr.Wrap(ctx, err, "select failing policy_runs ids") + } + for _, r := range refs { + failingIDs[r.PolicyID] = r.RunID + } + return nil + }); err != nil { + return nil, err + } + return failingIDs, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/policy_automation_executions.go` around lines 61 - 99, The multi-chunk INSERT loop and the subsequent lookupFailingPolicyRunRefs must be executed inside a single DB transaction to avoid partial commits; change the code to begin a transaction (e.g. tx, err := writer.BeginTx(ctx, nil) or the datasource's transaction helper), replace uses of writer.ExecContext with tx.ExecContext (and pass tx to lookupFailingPolicyRunRefs), perform the loop and the failing-ID lookup using the tx, rollback on any error and commit after the lookup succeeds, and return wrapped errors as before; ensure variables like writer, tx, lookupFailingPolicyRunRefs, pending, policyAutomationBatchSize and hostID are used to locate where to start and end the transaction.
🧹 Nitpick comments (3)
server/datastore/mysql/hosts_test.go (1)
9132-9133: ⚡ Quick winAssert the transition row exists before delete to prove cascade coverage.
At Line 9132, the test seeds transitions but doesn’t verify the seed actually produced a
policy_runsrow. If this call becomes a no-op, the cascade path may not be exercised.Suggested assertion
_, err = ds.RecordPolicyTransitions(context.Background(), host.ID, map[uint]*bool{policy.ID: new(false)}, []uint{policy.ID}) require.NoError(t, err) + + var policyRunCount int + err = ds.writer(context.Background()).Get(&policyRunCount, `SELECT COUNT(*) FROM policy_runs WHERE host_id = ?`, host.ID) + require.NoError(t, err) + require.Greater(t, policyRunCount, 0, "expected seeded policy_runs row before host deletion")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/hosts_test.go` around lines 9132 - 9133, After calling ds.RecordPolicyTransitions(host.ID, map[uint]*bool{policy.ID: new(false)}, []uint{policy.ID}) add an explicit assertion that a row was inserted into the policy_runs table for that host and policy before performing the delete to prove cascade behavior; locate the test near RecordPolicyTransitions and query the datastore (e.g., via an existing helper like GetPolicyRuns / GetPolicyRun or a direct query against policy_runs) to assert a policy_runs row exists for host.ID and policy.ID, failing the test if not present, then continue with the delete/cascade assertions.server/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.go (1)
23-34: ⚡ Quick winAdd a host-scoped index on
policy_runs.The only secondary key here is
(policy_id, host_id), which won't help the host-deletion cleanup this stack introduces withWHERE host_id = ?. On a large table that turns host cleanup into a full scan.Proposed change
CREATE TABLE policy_runs ( id BIGINT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, policy_id INT UNSIGNED NOT NULL, host_id INT UNSIGNED NOT NULL, old_status TINYINT(1) NULL, new_status TINYINT(1) NOT NULL, consecutive_failures INT UNSIGNED NOT NULL DEFAULT 0, created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, UNIQUE KEY uk_policy_run (policy_id, host_id), + KEY idx_policy_runs_host_id (host_id), CONSTRAINT fk_policy_runs_policy FOREIGN KEY (policy_id) REFERENCES policies (id) ON DELETE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;Please mirror the same index in
server/datastore/mysql/schema.sql.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.go` around lines 23 - 34, The migration creates policy_runs with only a UNIQUE KEY uk_policy_run (policy_id, host_id) which doesn't efficiently support WHERE host_id = ? queries used for host-scoped cleanup; add a non-unique index on host_id (e.g., INDEX idx_policy_runs_host_id (host_id)) to the CREATE TABLE in 20260519100000_AddPolicyRunsAndAutomationExecutions.go and mirror that same INDEX addition in server/datastore/mysql/schema.sql so host deletions use an indexed lookup rather than a full table scan.server/fleet/datastore.go (1)
1565-1592: ⚡ Quick winAlign the godoc with the batch-oriented API.
Line 1565 mentions
policy_ids, but this method takesruns []PolicyRunRef, and Lines 1569-1571 describe updating a single row even thoughbatchIDrepresents a batch of execution rows. That mismatch is easy to cargo-cult into datastore/mock implementations and callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/fleet/datastore.go` around lines 1565 - 1592, The docstring is inconsistent with the function signatures: change the CreatePolicyAutomationExecutions comment to refer to the provided runs []PolicyRunRef (not policy_ids) and describe that it checks whether the policy_runs referenced by those PolicyRunRef entries were already processed; also update the UpdatePolicyAutomationExecutions comment to make clear that batchID identifies a batch of execution rows (not a single row), that the call updates all rows in that batch (or is a no-op if batchID is uuid.Nil or the batch does not exist), and retain the monotonic state-machine rules (pending→success|failure, failure→success, first failure message wins, success terminal) but phrase them in batch terms so callers and mocks implementers are not misled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changes/45477-45478-policy-status-page-backend-models`:
- Line 1: The description that says policy_automation_executions records
outcomes "per host" is incorrect; update the text to state that
policy_automation_executions is a batch-level table that stores batch_id,
status, and error_text, and clarify that per-host linkage is achieved via
policy_runs and the join table policy_runs_to_policy_automation_executions
(i.e., per-host results are derived by joining policy_runs →
policy_runs_to_policy_automation_executions → policy_automation_executions).
Ensure the names policy_automation_executions, batch_id, policy_runs, and
policy_runs_to_policy_automation_executions are used in the revised description.
In `@server/cron/calendar_cron.go`:
- Around line 423-440: The code sets batchRecorded = true before
fetching/creating the persisted batch, which can skip future recordings if an
error occurs; update the logic in the ensureRecorded()/closure so that
batchRecorded is only set to true after CreatePolicyAutomationExecutions
succeeds (i.e. after recErr == nil and batchID = recordedBatch), and move any
early returns to before setting batchRecorded so failed GetFailingPolicyRuns or
CreatePolicyAutomationExecutions do not mark the batch as recorded.
- Around line 641-671: The code currently returns early on
ds.GetFailingPolicyRuns error which aborts the rest of the calendar automation;
instead, log the error but do not return—set refs to nil (or an empty slice) and
continue to call CreatePolicyAutomationExecutions so the automation and
subsequent event creation still run. Update the block around
GetFailingPolicyRuns (referencing GetFailingPolicyRuns, refs,
CreatePolicyAutomationExecutions, batchID) to remove the return, ensure refs is
initialized when the lookup fails, and preserve the existing logging and
downstream UpdatePolicyAutomationExecutions/error handling.
In
`@server/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.go`:
- Around line 45-54: Add a foreign key constraint on batch_id in the
policy_runs_to_policy_automation_executions join table so the DB enforces
referential integrity to policy_automation_executions; modify the CREATE TABLE
for policy_runs_to_policy_automation_executions to replace or augment KEY
idx_batch_id (batch_id) with a CONSTRAINT like
fk_policy_runs_join_tbl_execution_batch FOREIGN KEY (batch_id) REFERENCES
policy_automation_executions (batch_id) ON DELETE CASCADE (or the appropriate
delete behavior), ensuring batch_id is constrained to existing execution
batches.
In `@server/datastore/mysql/policy_automation_executions.go`:
- Around line 123-154: The code only chunks chunkSide but still passes the
entire fixedSide into each SQL statement (variables: chunkSide, fixedSide,
chunkPlaceholders, fixedPlaceholders, args, query, policyAutomationBatchSize),
so large fixedSide can produce huge queries; fix by chunking fixedSide as well
(e.g., add an outer or inner loop over fixedStart with step
policyAutomationBatchSize), build fixedPlaceholders and corresponding args for
each fixed chunk, and compose the query using the current chunkCol IN
(chunkPlaceholders) AND fixedCol IN (fixedPlaceholders) for each pair of chunks;
alternatively replace the IN-lists with a temp table/join approach if preferred.
---
Duplicate comments:
In `@server/datastore/mysql/policy_automation_executions.go`:
- Around line 61-99: The multi-chunk INSERT loop and the subsequent
lookupFailingPolicyRunRefs must be executed inside a single DB transaction to
avoid partial commits; change the code to begin a transaction (e.g. tx, err :=
writer.BeginTx(ctx, nil) or the datasource's transaction helper), replace uses
of writer.ExecContext with tx.ExecContext (and pass tx to
lookupFailingPolicyRunRefs), perform the loop and the failing-ID lookup using
the tx, rollback on any error and commit after the lookup succeeds, and return
wrapped errors as before; ensure variables like writer, tx,
lookupFailingPolicyRunRefs, pending, policyAutomationBatchSize and hostID are
used to locate where to start and end the transaction.
---
Nitpick comments:
In `@server/datastore/mysql/hosts_test.go`:
- Around line 9132-9133: After calling ds.RecordPolicyTransitions(host.ID,
map[uint]*bool{policy.ID: new(false)}, []uint{policy.ID}) add an explicit
assertion that a row was inserted into the policy_runs table for that host and
policy before performing the delete to prove cascade behavior; locate the test
near RecordPolicyTransitions and query the datastore (e.g., via an existing
helper like GetPolicyRuns / GetPolicyRun or a direct query against policy_runs)
to assert a policy_runs row exists for host.ID and policy.ID, failing the test
if not present, then continue with the delete/cascade assertions.
In
`@server/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.go`:
- Around line 23-34: The migration creates policy_runs with only a UNIQUE KEY
uk_policy_run (policy_id, host_id) which doesn't efficiently support WHERE
host_id = ? queries used for host-scoped cleanup; add a non-unique index on
host_id (e.g., INDEX idx_policy_runs_host_id (host_id)) to the CREATE TABLE in
20260519100000_AddPolicyRunsAndAutomationExecutions.go and mirror that same
INDEX addition in server/datastore/mysql/schema.sql so host deletions use an
indexed lookup rather than a full table scan.
In `@server/fleet/datastore.go`:
- Around line 1565-1592: The docstring is inconsistent with the function
signatures: change the CreatePolicyAutomationExecutions comment to refer to the
provided runs []PolicyRunRef (not policy_ids) and describe that it checks
whether the policy_runs referenced by those PolicyRunRef entries were already
processed; also update the UpdatePolicyAutomationExecutions comment to make
clear that batchID identifies a batch of execution rows (not a single row), that
the call updates all rows in that batch (or is a no-op if batchID is uuid.Nil or
the batch does not exist), and retain the monotonic state-machine rules
(pending→success|failure, failure→success, first failure message wins, success
terminal) but phrase them in batch terms so callers and mocks implementers are
not misled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e8d1159-593e-442b-af16-82bcf7f8f529
📥 Commits
Reviewing files that changed from the base of the PR and between 4938d38ec8d327670018be58edff8de7bfbb19e8 and 86713aa.
📒 Files selected for processing (17)
changes/45477-45478-policy-status-page-backend-modelscmd/fleet/cron.goserver/cron/calendar_cron.goserver/cron/calendar_cron_policy_recording_test.goserver/cron/calendar_cron_test.goserver/datastore/mysql/activities.goserver/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/migrations/tables/20260519100000_AddPolicyRunsAndAutomationExecutions.goserver/datastore/mysql/policy_automation_executions.goserver/datastore/mysql/policy_automation_executions_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/scripts.goserver/datastore/mysql/software_installers.goserver/fleet/calendar_events.goserver/fleet/calendar_events_test.goserver/fleet/datastore.go
Closes #45477 Introduces the backend models behind the policy status page: - New `policy_runs` table captures policy run outcomes; `policy_automation_executions` records the success/failure of each webhook, Jira, Zendesk, and calendar dispatch and links back to the originating run. - New nullable `policy_run_id` on `host_script_results`, `host_software_installs`, and their upcoming-activity tables attributes script and software installs back to the run that triggered them.
656f88c to
7a03e26
Compare
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
First pass done.
Left a few comments and questions.
(And you'll need to merge main because it's been a while since this was opened.)
| policy_id INT UNSIGNED NOT NULL, | ||
| host_id INT UNSIGNED NOT NULL, | ||
| old_status TINYINT(1) NULL, | ||
| new_status TINYINT(1) NOT NULL, |
There was a problem hiding this comment.
Why NOT NULL? (In policy_membership a NULL value means the policy ran but the query failed e.g. typo.)
There was a problem hiding this comment.
if the query failed won't policyResults be nil here? If that's the case then RecordPolicyTransitions won't ever be called, meaning new_status will always be either true or false.
| // | ||
| // consecutive_failures stores, welp the number of consecutive failures. | ||
| if _, err := tx.Exec(` | ||
| CREATE TABLE policy_runs ( |
There was a problem hiding this comment.
I would call it host_policy_runs (to follow pattern of other host tables with host_ids).
| @@ -0,0 +1,3 @@ | |||
| - Added `policy_automation_executions`, a batch-level table that records `batch_id`, `status`, and `error_message` for each dispatched failing-policy automation (webhook, Jira, Zendesk, calendar, conditional access). | |||
| - Added `policy_runs` table — one row per `(policy, host)` pair — tracking `old_status`, `new_status`, and `consecutive_failures` as each host's policy state evolves. | |||
| - Stamped a nullable `policy_run_id` on `host_script_results`, `host_software_installs`, `script_upcoming_activities`, and `software_install_upcoming_activities` so script and software installs triggered by a failing policy can be attributed back to a policy run. | |||
There was a problem hiding this comment.
How about VPP tables? VPP apps can be linked to policies (host_vpp_software_installs and vpp_app_upcoming_activities)
There was a problem hiding this comment.
Missed that, will add it
| // of how large either input grows. The outer loop chunks the larger side and | ||
| // the inner loop chunks the smaller side, so realistic 1×N callers issue one | ||
| // query per chunk on the N side and no extra queries on the 1 side. | ||
| func lookupFailingPolicyRunRefs( |
There was a problem hiding this comment.
After some discussions with Claude it seems there's no real benefit to the outer vs inner sides.
❯ so the swap is mostly cosmetic?
⏺ Yes — for this function, the swap is essentially cosmetic. Query count is symmetric in every shape (ceil(L/B) × ceil(S/B) either way), and per-statement IN-list contents are identical regardless of which loop
level slices them. So neither round-trip count nor optimizer plan depends on the swap.
The load-bearing optimization is the double chunking — bounding each statement at ~2×B placeholders keeps every query under eq_range_index_dive_limit so the planner keeps doing per-value index dives on
uk_policy_run.
The swap's only real value is readability: "outer loop walks the big list, inner is the small one" reads more naturally than the reverse, and it makes the docstring's "one query per chunk on the N side" framing
honest. If you deleted the swap and hard-coded outerCol = "policy_id", the function would perform identically — you'd just have a slightly awkward 1-iteration outer loop in host-heavy cases.
❯ Should we remove the swap optimization to keep the implementation simple
⏺ Yes, I'd simplify it. The swap adds ~6 lines of branching + 4 parallel-tracked variables (outerSide/innerSide/outerCol/innerCol) for zero behavioral benefit. A fixed orientation reads much cleaner:
for pStart := 0; pStart < len(policyIDs); pStart += policyAutomationBatchSize {
pChunk := policyIDs[pStart:min(pStart+policyAutomationBatchSize, len(policyIDs))]
for hStart := 0; hStart < len(hostIDs); hStart += policyAutomationBatchSize {
hChunk := hostIDs[hStart:min(hStart+policyAutomationBatchSize, len(hostIDs))]
// build "policy_id IN (pChunk) AND host_id IN (hChunk) AND new_status = false"
...
}
}
The load-bearing optimization (double chunking) is preserved. You also get to delete the long docstring paragraph explaining the swap, which is a hint the abstraction was carrying more justification than
weight.
One nit before you cut: double-check the chunked-branch test (policy_automation_executions_test.go around line 1927 in the diff) doesn't assert on which side gets chunked outer — if it just counts total queries
or checks result content, you're fine.
| // ticket covering N hosts, one calendar event covering N policies, etc), so that's why we | ||
| // need the concept of a batch_id on this table. | ||
| if _, err := tx.Exec(` | ||
| CREATE TABLE policy_automation_executions ( |
There was a problem hiding this comment.
Could policy_automation_executions be merged with policy_runs_to_policy_automation_executions? Maybe there's a reason I missed on why they are separate tables.
There was a problem hiding this comment.
This normalization keeps the amount of I/O small. Keep in mind that we can store potentially a large amount of data in policy_automation_executions.error_message and the cardinality of policy_runs_to_policy_automation_executions is one per host per policy
| } | ||
|
|
||
| svc.setHostConditionalAccessAsync(hostID, hostPlatform, hostConditionalAccessStatus, mdmEnrolled, hostIsCompliantInFleet) | ||
| // Build the executions list only when we're transitioning *to* a |
There was a problem hiding this comment.
Conditional access is sort of a different automation in that Fleet will also send a request to Entra if the policies are now passing (so this automation runs when policies pass too).
I'm not sure if we want to surface that anywhere. Maybe given it's just this one automation that does this we can ignore the fail->compliant scenario.
There was a problem hiding this comment.
That will need some product input IMHO - I think the current spec only contemplates automations errors in the context of policy failures (so I'm not sure how that will surface in the UI).
We can extend the current model to keep track of those in the future, but then we will need to figure out whether we want to track the 'fail' and 'pass' automations as separate entries in the policy_automation_executions table or as a single entry (similar to what we do with batch automations).
There was a problem hiding this comment.
I'm ok with this as-is (only on errors), we can iterate later.
3 participants
Related issues:
Resolves #45477
Partially addresses #45478
Introduces the backend models behind the policy status page:
policy_runstable captures (policy, host, passed) outcomes;policy_automation_executionsrecords the success/failure of each webhook, Jira, Zendesk, and calendar dispatch and links back to the originating run.policy_run_idonhost_script_results,host_software_installs, and their upcoming-activity tables attributes script and software installs back to the run that triggered them.server/policy_automationpackage withRecordandFinalizeconsolidates the dispatch-time and completion-time lifecycle across every automation surface.Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit