Skip to content

Conversation

@adenchen123
Copy link
Contributor

@adenchen123 adenchen123 commented Dec 4, 2025

PR Type

Enhancement


Description

  • Add per-minute crontab scheduling API endpoint for external triggers

  • Introduce trigger control flags for watcher and OpenAPI sources

  • Implement cron expression validation for one-minute intervals

  • Refactor crontab execution logic with improved service scoping


Diagram Walkthrough

flowchart LR
  A["CrontabItem Model"] -->|"Add TriggerByWatcher<br/>TriggerByOpenAPI"| B["Enhanced Configuration"]
  C["CrontabItemExtension"] -->|"CheckNextOccurrenceEveryOneMinute()"| D["Cron Validation"]
  E["CrontabController"] -->|"New /crontab/scheduling-per-minute"| F["Per-Minute Scheduler"]
  F -->|"Filter & Execute"| D
  B -->|"Filter Allowed Items"| F
Loading

File Walkthrough

Relevant files
Enhancement
CrontabItem.cs
Add trigger control flags to CrontabItem model                     

src/Infrastructure/BotSharp.Abstraction/Crontab/Models/CrontabItem.cs

  • Add TriggerByWatcher boolean property (default true) to control
    watcher-based triggering
  • Add TriggerByOpenAPI boolean property to control OpenAPI-based
    triggering
  • Both properties are JSON-serializable with appropriate naming
    conventions
+6/-0     
CrontabItemExtension.cs
Implement one-minute interval cron validation logic           

src/Infrastructure/BotSharp.Core.Crontab/Services/CrontabItemExtension.cs

  • Create new extension method CheckNextOccurrenceEveryOneMinute() for
    CrontabItem
  • Strip seconds from cron expression and parse with NCrontab library
  • Validate if next occurrence falls within the past minute window
  • Return boolean indicating whether execution should occur
+26/-0   
CrontabController.cs
Add per-minute scheduling endpoint and refactor execution

src/Infrastructure/BotSharp.OpenAPI/Controllers/Crontab/CrontabController.cs

  • Add new SchedulingCrontab() endpoint at /crontab/scheduling-per-minute
    for per-minute scheduling
  • Implement GetCrontabItems() helper method to filter crontabs by
    OpenAPI trigger flag
  • Implement ExecuteTimeArrivedItem() helper method with proper service
    scoping
  • Refactor existing RunCrontab() to use new helper methods
  • Add logging for crontab execution lifecycle
+48/-8   
Configuration changes
BotSharp.OpenAPI.csproj
Add crontab core dependency and fix formatting                     

src/Infrastructure/BotSharp.OpenAPI/BotSharp.OpenAPI.csproj

  • Add project reference to BotSharp.Core.Crontab for crontab extension
    services
  • Fix XML declaration formatting (remove BOM character)
+2/-1     

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Limited Auditing: New scheduling and execution endpoints log start/complete and errors but do not include
user identity or structured audit details to reconstruct who triggered executions.

Referred Code
[HttpPost("/crontab/scheduling-per-minute")]
public async Task SchedulingCrontab()
{
    var allowedCrons = await GetCrontabItems();

    foreach (var item in allowedCrons)
    {
        if (item.CheckNextOccurrenceEveryOneMinute())
        {
            _logger.LogInformation("Crontab: {0}, One occurrence was matched, Beginning execution...", item.Title);
            Task.Run(() => ExecuteTimeArrivedItem(item));
        }
    }
}

private async Task<List<CrontabItem>> GetCrontabItems(string? title = null)
{
    var crontabService = _services.GetRequiredService<ICrontabService>();
    var crons = await crontabService.GetCrontable();
    var allowedCrons = crons.Where(cron => cron.TriggerByOpenAPI).ToList();



 ... (clipped 23 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing Validation: The cron parsing logic does not handle null/empty or malformed Cron strings and can throw
at Parse without try/catch or input validation.

Referred Code
public static bool CheckNextOccurrenceEveryOneMinute(this CrontabItem item)
{
    // strip seconds from cron expression
    item.Cron = string.Join(" ", item.Cron.Split(' ').TakeLast(5));
    var schedule = CrontabSchedule.Parse(item.Cron, new CrontabSchedule.ParseOptions
    {
        IncludingSeconds = false // Ensure you account for seconds
    });

    var currentTime = DateTime.UtcNow;

    // Check if there has been an execution point within the past minute.
    var oneMinuteAgo = currentTime.AddMinutes(-1);
    var nextOccurrenceFromPast = schedule.GetNextOccurrence(oneMinuteAgo);

    // If the next execution point falls within the past minute up to the present, then it matches.
    return nextOccurrenceFromPast > oneMinuteAgo && nextOccurrenceFromPast <= currentTime;
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input Validation: The new per-minute scheduling endpoint and RunCrontab filter rely on cron data and title
without explicit validation or authorization context for external triggers.

Referred Code
[HttpPost("/crontab/scheduling-per-minute")]
public async Task SchedulingCrontab()
{
    var allowedCrons = await GetCrontabItems();

    foreach (var item in allowedCrons)
    {
        if (item.CheckNextOccurrenceEveryOneMinute())
        {
            _logger.LogInformation("Crontab: {0}, One occurrence was matched, Beginning execution...", item.Title);
            Task.Run(() => ExecuteTimeArrivedItem(item));
        }
    }
}

private async Task<List<CrontabItem>> GetCrontabItems(string? title = null)
{
    var crontabService = _services.GetRequiredService<ICrontabService>();
    var crons = await crontabService.GetCrontable();
    var allowedCrons = crons.Where(cron => cron.TriggerByOpenAPI).ToList();



 ... (clipped 7 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Adopt a robust background job library

Replace the custom, non-durable scheduling mechanism, which uses an external
poller and Task.Run, with a dedicated background job library like Hangfire or
Quartz.NET. This will improve reliability and scalability.

Examples:

src/Infrastructure/BotSharp.OpenAPI/Controllers/Crontab/CrontabController.cs [40-52]
    public async Task SchedulingCrontab()
    {
        var allowedCrons = await GetCrontabItems();

        foreach (var item in allowedCrons)
        {
            if (item.CheckNextOccurrenceEveryOneMinute())
            {
                _logger.LogInformation("Crontab: {0}, One occurrence was matched, Beginning execution...", item.Title);
                Task.Run(() => ExecuteTimeArrivedItem(item));

 ... (clipped 3 lines)

Solution Walkthrough:

Before:

// In CrontabController.cs
[HttpPost("/crontab/scheduling-per-minute")]
public async Task SchedulingCrontab()
{
    var allowedCrons = await GetCrontabItems();

    foreach (var item in allowedCrons)
    {
        if (item.CheckNextOccurrenceEveryOneMinute())
        {
            // Fire-and-forget task, not durable
            Task.Run(() => ExecuteTimeArrivedItem(item));
        }
    }
}

After:

// In Startup.cs or Program.cs (conceptual)
public void ConfigureServices(IServiceCollection services)
{
    // Configure a robust background job library
    services.AddHangfire(config => ...); 
    services.AddHangfireServer();
}

// Job registration logic
public void RegisterBackgroundJobs()
{
    var crons = crontabService.GetCrontable();
    foreach(var item in crons.Where(c => c.TriggerByOpenAPI))
    {
        // Schedule job with the library, which handles persistence and execution
        RecurringJob.AddOrUpdate(
            item.Title, 
            () => crontabService.ScheduledTimeArrived(item), 
            item.Cron);
    }
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a fragile "fire-and-forget" scheduling implementation using Task.Run which lacks durability and scalability, and proposes a robust, industry-standard alternative.

High
Possible issue
Improve background task execution handling

In SchedulingCrontab, replace the "fire-and-forget" Task.Run in the loop with a
more robust pattern. Collect all tasks in a list and await them with
Task.WhenAll to improve concurrency management and error handling.

src/Infrastructure/BotSharp.OpenAPI/Controllers/Crontab/CrontabController.cs [44-51]

+var tasks = new List<Task>();
 foreach (var item in allowedCrons)
 {
     if (item.CheckNextOccurrenceEveryOneMinute())
     {
         _logger.LogInformation("Crontab: {0}, One occurrence was matched, Beginning execution...", item.Title);
-        Task.Run(() => ExecuteTimeArrivedItem(item));
+        tasks.Add(ExecuteTimeArrivedItem(item));
     }
 }
 
+if (tasks.Any())
+{
+    await Task.WhenAll(tasks);
+}
+
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies the risks of "fire-and-forget" tasks, such as unhandled exceptions crashing the application. Using Task.WhenAll provides a much more robust solution for managing concurrent tasks and handling errors.

Medium
Avoid modifying the input object

In CheckNextOccurrenceEveryOneMinute, avoid modifying the input item.Cron
property directly. Instead, assign the modified cron expression to a local
variable to prevent side effects.

src/Infrastructure/BotSharp.Core.Crontab/Services/CrontabItemExtension.cs [7-24]

 public static bool CheckNextOccurrenceEveryOneMinute(this CrontabItem item)
 {
     // strip seconds from cron expression
-    item.Cron = string.Join(" ", item.Cron.Split(' ').TakeLast(5));
-    var schedule = CrontabSchedule.Parse(item.Cron, new CrontabSchedule.ParseOptions
+    var cronExpression = string.Join(" ", item.Cron.Split(' ').TakeLast(5));
+    var schedule = CrontabSchedule.Parse(cronExpression, new CrontabSchedule.ParseOptions
     {
         IncludingSeconds = false // Ensure you account for seconds
     });
 
     var currentTime = DateTime.UtcNow;
 
     // Check if there has been an execution point within the past minute.
     var oneMinuteAgo = currentTime.AddMinutes(-1);
     var nextOccurrenceFromPast = schedule.GetNextOccurrence(oneMinuteAgo);
 
     // If the next execution point falls within the past minute up to the present, then it matches.
     return nextOccurrenceFromPast > oneMinuteAgo && nextOccurrenceFromPast <= currentTime;
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a side effect in an extension method, where the input object's Cron property is modified. The proposed fix to use a local variable makes the method more robust and prevents potential bugs.

Medium
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants