CRE Gate and Limit Handling Improvements - #23458
Conversation
|
✅ No conflicts with other open PRs targeting |
|
I see you updated files related to
|
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM — changes touch core workflow-engine execution control-flow (early returns, limiter fallbacks, metering lifecycle), which can affect reliability/observability across all runs.
This PR adjusts the workflows v2 engine to fail soft when dynamic limit reads error (use static defaults instead of dropping runs/events), and to surface previously-silent early drops by emitting Started/Finished events plus a new fallback metric.
Changes:
- Convert several limiter read error paths (trigger queue age, execution timeout, log event cap sizing, execution response size, etc.) from “drop/return” to “fallback to static default + metric”.
- Add
platform_engine_limit_read_fallback_totalmetric (labeled by limit key) and wire it into the engine paths that now fail-soft. - Improve observability for early execution abandonment (e.g., metering reserve failure) by emitting Started/Finished even when returning before the “normal” emit points; add regression tests for these drop paths.
Areas needing scrupulous human review:
Engine.startExecutionearly-return behavior (newemitDroppedExecutionhelper + meteringEnd()guarded-defer): verify event emission, store state transitions, and metering report lifecycle are correct for every return path (including sharding denials, reserve failures, trigger-index conversion failures, and timeout paths).- Limiter fallback defaults: confirm each fallback uses the intended settings key + default value for the specific limiter being read, and that the new fallback metric won’t introduce excessive label cardinality.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/services/workflows/v2/engine.go | Implements fail-soft limiter reads, adds early-drop Started/Finished emission, and ensures metering reports are ended on early returns. |
| core/services/workflows/v2/engine_drop_paths_test.go | Adds regression tests for limiter read fallbacks and early-drop observability/metering cleanup paths. |
| core/services/workflows/v2/config.go | Tracks effective limiter defaults used to construct EngineLimiters to support correct fallback values. |
| core/services/workflows/monitoring/trigger_drop_reason.go | Documents legacy drop reasons retained for dashboard compatibility (now expected to fall to 0). |
| core/services/workflows/monitoring/monitoring.go | Registers and emits the new limit-read-fallback metric counter. |
| core/platform/monitoring.go | Adds limitKey observability label constant for the new metric. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
e41d103 to
4b4895b
Compare
4b4895b to
368c962
Compare
tarcisiozf
changed the title
| } | ||
| // A settings read failure is not an expiry: run the execution rather than | ||
| // dropping a customer's trigger event over a transient config read. | ||
| e.logger().Errorw("Failed to check trigger event queue age limit; proceeding with execution", "err", ageErr) |
There was a problem hiding this comment.
Should this block still end in a continue?
There was a problem hiding this comment.
No, the idea of these changes is exactly the opposite: do not drop a user event unless it's a true abort case, failing to read the settings in this case can be transient and we are tolerating it.
There was a problem hiding this comment.
What I'm going to now is to add a counter here so we can track how many hits we get.
|
This pull request introduces improvements to how configuration setting (CRE) gates and limits are handled across several components, focusing on more robust error handling, consistent gate evaluation, and improved observability. The changes standardize the use of helper functions for gate checks, ensure soft-fail behavior (falling back to defaults) when reading certain limits fails, and add new metrics for monitoring such fallbacks. Additionally, there is a refactor to how trigger event aging is managed in the workflow engine.
CRE Gate and Limit Handling Improvements:
Limitand custom error handling with the sharedcrelimits.GateOpenandcrelimits.GateAllowshelpers, ensuring consistent behavior and error logging for CRE gates.Metrics and Observability:
limitReadFallbackTotal) and corresponding label (limitKey) to monitor cases where a limit read fails and the system falls back to its static default, increasing visibility into configuration issues.Workflow Engine Refactor and Behavior:
Internal Refactoring:
EngineLimitersto track and expose the effective default settings used for limits, supporting the new fallback logic and making configuration more transparent.These changes collectively make the system more resilient to configuration read errors, provide better diagnostics, and improve the consistency of gate and limit enforcement.