Skip to content

Fix retry loop skipping when AgentRunMaxRetries is negative#1905

Merged
vcastellm merged 2 commits intofeature/e2e-testingfrom
copilot/sub-pr-1895-another-one
Jan 29, 2026
Merged

Fix retry loop skipping when AgentRunMaxRetries is negative#1905
vcastellm merged 2 commits intofeature/e2e-testingfrom
copilot/sub-pr-1895-another-one

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 29, 2026

Proposed changes

When AgentRunMaxRetries is set to a negative value (via config or CLI flag), the retry loop in GRPCClient.AgentRun would be skipped entirely (for attempt := 0; attempt <= maxRetries where maxRetries < 0). This causes lastErr to remain nil, and the function returns success without ever attempting to run the job.

Changes:

  • Clamp maxRetries to 0 before the retry loop to guarantee at least one execution attempt
  • Clamped value propagates to all derived fields (total_attempts in logs, backoff calculations)
  • Fix unrelated syntax error in api_test.go (extra closing brace) that was blocking tests
maxRetries := grpcc.agent.config.AgentRunMaxRetries
// Clamp maxRetries to 0 if negative to ensure at least one attempt is made
if maxRetries < 0 {
    maxRetries = 0
}

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…tive

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jan 29, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • 127.0.0.10
    • Triggering command: /tmp/go-build139554034/b001/dkron.test /tmp/go-build139554034/b001/dkron.test -test.testlogfile=/tmp/go-build139554034/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -test.run=TestGRPC .cfg ux_amd64/vet -p /kubernetes/type-l -lang=go1.24 ux_amd64/vet -o n47k/askgCWYVBoxX54Jan47k pkg/mod/k8s.io/api@v0.22.2/rbac/-ifaceassert ux_amd64/vet -p github.com/softl-l -lang=go1.16 ux_amd64/vet (packet block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Add comprehensive E2E test suite for Dkron API Fix retry loop skipping when AgentRunMaxRetries is negative Jan 29, 2026
Copilot AI requested a review from vcastellm January 29, 2026 09:52
@vcastellm vcastellm marked this pull request as ready for review January 29, 2026 20:37
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 29, 2026

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

  • 🔍 Trigger a full review

Comment @coderabbitai help to get the list of available commands and usage tips.

@vcastellm vcastellm merged commit aa2a5ff into feature/e2e-testing Jan 29, 2026
5 checks passed
@vcastellm vcastellm deleted the copilot/sub-pr-1895-another-one branch January 29, 2026 20:37
vcastellm added a commit that referenced this pull request Jan 29, 2026
* Add comprehensive E2E test suite for Dkron API

- Create e2e test infrastructure using Bats (Bash Automated Testing System)
- Add Docker Compose setup for isolated e2e testing environment
- Add 71 tests covering:
  - Health, cluster, leader, and debug endpoints (19 tests)
  - Job CRUD operations including timezone, retries, concurrency (24 tests)
  - Job execution and scheduling (17 tests)
  - Pause/unpause functionality (11 tests)
- Add helper functions for API requests, response parsing, and assertions
- Add test runner script with options for filtering, verbose output, and debugging
- Add Makefile targets: e2e, e2e-quick, e2e-debug

Key findings documented in tests:
- Disabled jobs cannot be manually triggered via /run endpoint
- Pause only blocks new job creation, not scheduled execution
- Timezone jobs require 6-field cron expressions (with seconds)

* Add Algolia site verification meta tag

* Apply suggestion from @coderabbitai[bot]

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Add GitHub Actions workflow for E2E tests (#1899)

* Initial plan

* Add GitHub Actions workflow for E2E tests

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Fix workflow: remove invalid tags-ignore and use apt for bats

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Add explicit permissions to e2e workflow

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Merge main branch into E2E test suite PR (#1902)

* Initial plan

* Merge main branch into PR branch - resolve conflicts

Merged latest changes from main branch (commit 28f91f1) into the E2E test PR branch.

Conflict resolution:
- Accepted main branch version for all source files (Go code, UI, configs)
- Preserved E2E test infrastructure (e2e/ directory and all test files)
- Added e2e, e2e-quick, e2e-debug targets back to Makefile
- Kept .github/workflows/e2e.yml workflow file

New features from main now included:
- Execution statistics storage and dashboard graph
- Plugin health monitoring
- Retry logic with exponential backoff for AgentRun RPC
- Various dependency updates

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Update E2E test to use bare endpoint /v1/jobs/:job (#1897)

* Initial plan

* Update first test to use bare endpoint /v1/jobs/:job

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Fix flaky E2E test using deterministic polling instead of fixed sleep (#1898)

* Initial plan

* Replace sleep 3 with wait_for_execution for deterministic test behavior

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Update dkron/api_test.go

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Fix syntax error in TestHealthEndpoint causing compilation failure (#1906)

* Initial plan

* Fix syntax error in api_test.go - remove duplicate closing brace

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Fix retry loop skipping when AgentRunMaxRetries is negative (#1905)

* Initial plan

* Fix retry loop to ensure at least one attempt when maxRetries is negative

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Enhance E2E test to validate failed execution success field (#1904)

* Initial plan

* Enhance failed job execution test to assert success=false

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Remove beta dependency by downgrading protoreflect to v1.17.0 (#1903)

* Initial plan

* Downgrade protoreflect from v1.18.0 to v1.17.0 to remove beta v2 dependency

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Add comprehensive E2E test suite for Dkron API (#1907)

* Initial plan

* Merge main branch into E2E test suite PR

Merged latest changes from main branch (commit 2b5f80b) into the E2E test PR branch.

Conflict resolution strategy:
- Accepted main branch version for all source files (Go code, UI, configs)
- Preserved E2E test infrastructure (e2e/ directory and all test files)
- Added e2e, e2e-quick, e2e-debug targets back to Makefile
- Kept .github/workflows/e2e.yml workflow file

New features from main now included:
- Redesigned UI with modern professional styling (#1894)
- Execution statistics storage and dashboard graph (#1896)
- Plugin health monitoring in /health endpoint (#1884)
- Retry logic with exponential backoff for AgentRun RPC (#1882)
- Various dependency updates

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

* Fix unresolved merge conflict in ui/src/layout/index.css

Replaced file with clean version from main branch to remove conflict markers.

Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vcastellm <47026+vcastellm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants