Skip to content

fix: data loss when jira ticket lefts board - #129

Merged
flacatus merged 7 commits into
konflux-ci:mainfrom
rsoaresd:fix_data_loss
Aug 19, 2026
Merged

fix: data loss when jira ticket lefts board#129
flacatus merged 7 commits into
konflux-ci:mainfrom
rsoaresd:fix_data_loss

Conversation

@rsoaresd

@rsoaresd rsoaresd commented Jul 9, 2026

Copy link
Copy Markdown

Summary

For example, RHIDP-15165 was assigned to RHDH AI but then later to RHDH Frontend Plugins & UI and closed, but devlake didn't update it in the RHDH AI, it still keeps it there:

jira:JiraIssue:10:6317248  →  IN_PROGRESS  (connection 10, board 10725, RHDH AI)
jira:JiraIssue:13:6317248  →  DONE         (connection 13, board 11525, RHDH F&UI)

This happens because devlake relies on Jira's board API that only returns issues that currently belong to that board. Since RHIDP-15165 is now on board 11525, board 10725's endpoint simply won't include it in the response, so it will not update since it does not look to the previously known issues. We need a cleanup mechanism

For more info, check https://redhat-internal.slack.com/archives/C066UL7C0QH/p1783350712079609

Issue ticket number and link

DPROD-1338

@rsoaresd
rsoaresd requested a review from a team as a code owner July 9, 2026 10:50

@mfrancisc mfrancisc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for looking into it 🙏

I've left few comments.

Comment thread backend/plugins/jira/tasks/stale_board_issue_cleaner.go Outdated
// CleanupStaleBoardIssues batch-checks all issues in _tool_jira_board_issues against the
// board API (100 per request via issue IN (...) JQL) and removes any that are no longer
// returned. Covers both open and closed issues that moved to a different board or team.
func CleanupStaleBoardIssues(taskCtx plugin.SubTaskContext) errors.Error {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to add few unit tests for this function ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Comment thread backend/plugins/jira/tasks/stale_board_issue_cleaner.go Outdated
Comment thread backend/plugins/jira/impl/impl.go Outdated
Comment thread backend/plugins/jira/tasks/stale_board_issue_cleaner.go Outdated
@rsoaresd
rsoaresd requested a review from mfrancisc July 21, 2026 15:21
@rsoaresd

Copy link
Copy Markdown
Author

@mfrancisc thank you so much for your suggestions! Addressed

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 3:21 PM UTC · Ended 3:22 PM UTC
Commit: 37b10e4 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:22 PM UTC · Completed 3:36 PM UTC
Commit: 37b10e4 · View workflow run →

@qodo-app-for-konflux-ci

qodo-app-for-konflux-ci Bot commented Jul 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Empty page triggers deletions ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New
Description
fetchBoardMembership breaks pagination and returns an empty/partial membership map when the API
returns issues=[], even if total>0. CleanupStaleBoardIssues treats missing keys as stale and
deletes the board association, so a single inconsistent/partial response can incorrectly remove
valid associations for that board.
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[R197-198]

+			if len(result.Issues) == 0 || startAt+len(result.Issues) >= result.Total {
+				break
Relevance

●● Moderate

Data-loss risk plausible, but no close historical precedent on this Jira empty-page scenario.

PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The pagination loop breaks immediately on an empty issues page (new behavior at line 197), which
can leave onBoard empty/partial. The cleanup loop deletes associations for any issue key not
present in onBoard, so an empty/partial map will be interpreted as “not on board” and cause
deletions. The new test explicitly encodes the total>0 + empty issues scenario and expects an
empty map with no error, which would lead to deletions upstream.

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197-199]
backend/plugins/jira/tasks/stale_board_issue_cleaner.go[87-92]
backend/plugins/jira/tasks/stale_board_issue_cleaner_test.go[217-245]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`fetchBoardMembership` currently stops pagination when `len(result.Issues) == 0`, even if the response indicates there are more results (`result.Total > startAt`). This can return an empty/partial `onBoard` map, which causes `CleanupStaleBoardIssues` to delete board associations for all missing keys.

## Issue Context
The cleanup task is enabled-by-default and performs destructive deletes based on `onBoard[issueKey]` membership.

## Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197-199]

## Suggested fix
Treat an empty page as an invalid/incomplete membership result when `result.Total > startAt` (or when `result.Total > 0` but `len(result.Issues)==0`), and **do not** proceed with deletions.

Concrete options:
1) Return an error so the task retries and does not delete anything on that run.
2) Return `(nil, nil)` (similar to the 404 behavior) and have the caller skip cleanup while logging a warning about the inconsistent response.

Update the unit test `TestFetchBoardMembership_EmptyPageBreaksLoop` accordingly to assert the new behavior (error or nil map), rather than asserting `onBoard` is empty with no error.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Loads all board issues 🐞 Bug ➹ Performance
Description
CleanupStaleBoardIssues loads every issue association for the board into memory via db.All before
batching API checks, which can significantly increase memory usage and runtime on large boards. This
can destabilize workers (slowdowns/OOM) for boards with many historical associations.
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[R63-74]

+	var allBoardIssues []struct {
+		IssueKey string
+		IssueId  uint64
+	}
+	if err := db.All(&allBoardIssues,
+		dal.Select("ji.issue_key, ji.issue_id"),
+		dal.From("_tool_jira_board_issues tbi"),
+		dal.Join("JOIN _tool_jira_issues ji ON ji.issue_id = tbi.issue_id AND ji.connection_id = tbi.connection_id"),
+		dal.Where("tbi.connection_id = ? AND tbi.board_id = ?", connectionId, boardId),
+	); err != nil {
+		return errors.Default.Wrap(err, "failed to query board issues")
+	}
Relevance

●● Moderate

Some perf scoping work merged (PR #99), but no history enforcing avoiding db.All for large result
sets.

PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new task loads all board issues into an in-memory slice using db.All, and the DAL contract
warns this method should be used cautiously for big result sets.

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[63-74]
backend/core/dal/dal.go[124-131]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CleanupStaleBoardIssues` uses `db.All` to materialize all board issue associations into a slice before any batching. For large boards, this can cause high memory usage and long runtimes.

### Issue Context
The DAL interface explicitly warns that `All` should be used cautiously for large result sets; a cursor-based approach is preferred.

### Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[63-76]
- backend/core/dal/dal.go[124-131]

### Suggested fix
- Replace `db.All(&allBoardIssues, ...)` with `db.Cursor(...)` and iterate rows to process in fixed-size chunks (e.g., accumulate 100 keys, call `fetchBoardMembership`, then clear chunk).
- Alternatively, page the DB query (ORDER BY issue_id with LIMIT/OFFSET or keyset pagination) to bound memory.
- Keep the API batching, but avoid holding the entire association list in memory at once.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Expensive stale issue refetch 🐞 Bug ☼ Reliability
Description
For each stale association, CleanupStaleBoardIssues calls collectAndExtractSingleIssue, which
fetches the full issue with expand=changelog and extracts/persists multiple entities. If many
issues leave a board at once, this can greatly increase Jira API load and task runtime (and may
trigger rate limiting/timeouts).
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[R87-99]

+	removed := 0
+	for _, bi := range allBoardIssues {
+		if onBoard[bi.IssueKey] {
+			continue
+		}
+
+		logger.Info("issue %s is no longer on board %d, updating state and removing association", bi.IssueKey, boardId)
+
+		// Re-fetch from Jira to update _tool_jira_issues with current status/team/resolution
+		if updateErr := collectAndExtractSingleIssue(taskCtx, data, db, bi.IssueKey); updateErr != nil {
+			logger.Warn(updateErr, "failed to update issue state for %s, will still remove board association", bi.IssueKey)
+		}
+
Relevance

●● Moderate

Repo has optimized Jira task scope for performance (PR #99), but no clear precedent rejecting
per-issue refetch patterns.

PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Cleanup invokes collectAndExtractSingleIssue for each stale issue, and that helper explicitly
requests expand=changelog, reads the whole response, and performs wide extraction work beyond what
cleanup needs.

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[87-99]
backend/plugins/jira/tasks/parent_issue_collector.go[134-206]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When an issue is deemed stale, the cleanup path calls `collectAndExtractSingleIssue`, which requests `expand=changelog` and performs a broad extraction/save. This is heavy for the cleanup use-case (which only needs to refresh the issue's current status/team/resolution).

### Issue Context
The cleanup loop may process many stale issues in one run; doing a full issue+changelog fetch per stale issue can make the cleanup step disproportionately expensive.

### Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[87-99]
- backend/plugins/jira/tasks/parent_issue_collector.go[134-206]

### Suggested fix
- Add a purpose-built `refreshSingleIssueState` helper used by cleanup that:
 - omits `expand=changelog`
 - requests only required fields (via `fields=...`) needed to update `_tool_jira_issues` state
 - updates only the relevant issue columns (instead of extracting/saving comments/worklogs/changelogs/users)
- Optionally cap work per run (max stale updates) and/or add bounded concurrency with rate-limit handling if parallelizing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

4. Jira divergence not documented ✓ Resolved 📘 Rule violation § Compliance ⭐ New
Description
New/modified upstream-divergence files under backend/plugins/jira/ are not documented in
docs/upstream-diffs.md. This increases rebase risk because the local override is not tracked with
required metadata.
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197]

+			if len(result.Issues) == 0 || startAt+len(result.Issues) >= result.Total {
Relevance

● Weak

Similar request to document upstream diffs in docs/upstream-diffs.md was rejected.

PR-#125

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance ID 1360 requires documenting new upstream divergences for modified files outside owned
plugin directories. docs/upstream-diffs.md contains a Jira divergence section listing other Jira
files, but it does not list the newly added/modified stale_board_issue_cleaner files.

Rule 1360: Document new upstream divergences in docs/upstream-diffs.md
backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197-197]
backend/plugins/jira/tasks/stale_board_issue_cleaner_test.go[18-31]
docs/upstream-diffs.md[53-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Files modified outside owned plugin directories must be documented in `docs/upstream-diffs.md` when they introduce new local divergences.

## Issue Context
`docs/upstream-diffs.md` already tracks other `backend/plugins/jira/...` divergences, but does not include these newly added/modified files.

## Fix Focus Areas
- docs/upstream-diffs.md[53-67]
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197-197]
- backend/plugins/jira/tasks/stale_board_issue_cleaner_test.go[18-31]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Jira plugin modified outside owned 📘 Rule violation ⌂ Architecture ⭐ New
Description
The PR adds/modifies code under backend/plugins/jira/, which is outside the allowed owned plugin
directories. This violates the rule restricting changes to only backend/plugins/aireview/,
backend/plugins/codecov/, or backend/plugins/testregistry/.
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197]

+			if len(result.Issues) == 0 || startAt+len(result.Issues) >= result.Total {
Relevance

● Weak

Similar ownership-rule violation suggestion was previously rejected by reviewers.

PR-#97

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance ID 1105 forbids modifying upstream code outside the owned plugin directories. The changed
lines are in backend/plugins/jira/..., which is outside the permitted paths.

Rule 1105: Do not modify upstream code outside owned plugin directories
backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197-197]
backend/plugins/jira/tasks/stale_board_issue_cleaner_test.go[18-31]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR modifies `backend/plugins/jira/...`, which is not an owned plugin directory per policy.

## Issue Context
Compliance requires all PR modifications to be confined to `backend/plugins/aireview/`, `backend/plugins/codecov/`, or `backend/plugins/testregistry/`.

## Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[197-197]
- backend/plugins/jira/tasks/stale_board_issue_cleaner_test.go[18-31]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Unbounded HTTP body read 🐞 Bug ☼ Reliability
Description
fetchBoardMembership reads the entire HTTP response body with io.ReadAll without a size cap. If
Jira returns an unexpectedly large payload, this can cause avoidable memory spikes during cleanup.
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[R172-181]

+			if resp.StatusCode != http.StatusOK {
+				_ = resp.Body.Close()
+				return nil, errors.Default.New(fmt.Sprintf("unexpected status %d from board issues API", resp.StatusCode))
+			}
+
+			blob, readErr := errors.Convert01(io.ReadAll(resp.Body))
+			_ = resp.Body.Close()
+			if readErr != nil {
+				return nil, errors.Default.Wrap(readErr, "failed to read board response")
+			}
Relevance

●●● Strong

Team previously accepted limiting HTTP body reads via io.LimitReader around io.ReadAll (PR #97).

PR-#97

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The code path reads the full response body into memory without any upper bound before unmarshalling
JSON.

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[155-191]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`fetchBoardMembership` uses `io.ReadAll(resp.Body)` without limiting the amount of data read, which can amplify the impact of unexpectedly large responses.

### Issue Context
While `maxResults=100` and `fields=id,key` should usually keep responses small, adding a hard cap improves robustness.

### Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[172-181]

### Suggested fix
- Replace `io.ReadAll(resp.Body)` with `io.ReadAll(io.LimitReader(resp.Body, <maxBytes>))` (e.g., 5–10MB), and return a clear error if the limit is hit.
- (Optional) Use a shared helper for bounded response reading if the repo has one.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials
✅ Compliance rules (platform): 113 rules

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 578f4e1

Results up to commit 6fa26b4 ⚖️ Balanced


🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Loads all board issues 🐞 Bug ➹ Performance
Description
CleanupStaleBoardIssues loads every issue association for the board into memory via db.All before
batching API checks, which can significantly increase memory usage and runtime on large boards. This
can destabilize workers (slowdowns/OOM) for boards with many historical associations.
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[R63-74]

+	var allBoardIssues []struct {
+		IssueKey string
+		IssueId  uint64
+	}
+	if err := db.All(&allBoardIssues,
+		dal.Select("ji.issue_key, ji.issue_id"),
+		dal.From("_tool_jira_board_issues tbi"),
+		dal.Join("JOIN _tool_jira_issues ji ON ji.issue_id = tbi.issue_id AND ji.connection_id = tbi.connection_id"),
+		dal.Where("tbi.connection_id = ? AND tbi.board_id = ?", connectionId, boardId),
+	); err != nil {
+		return errors.Default.Wrap(err, "failed to query board issues")
+	}
Relevance

●● Moderate

Some perf scoping work merged (PR #99), but no history enforcing avoiding db.All for large result
sets.

PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new task loads all board issues into an in-memory slice using db.All, and the DAL contract
warns this method should be used cautiously for big result sets.

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[63-74]
backend/core/dal/dal.go[124-131]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CleanupStaleBoardIssues` uses `db.All` to materialize all board issue associations into a slice before any batching. For large boards, this can cause high memory usage and long runtimes.

### Issue Context
The DAL interface explicitly warns that `All` should be used cautiously for large result sets; a cursor-based approach is preferred.

### Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[63-76]
- backend/core/dal/dal.go[124-131]

### Suggested fix
- Replace `db.All(&allBoardIssues, ...)` with `db.Cursor(...)` and iterate rows to process in fixed-size chunks (e.g., accumulate 100 keys, call `fetchBoardMembership`, then clear chunk).
- Alternatively, page the DB query (ORDER BY issue_id with LIMIT/OFFSET or keyset pagination) to bound memory.
- Keep the API batching, but avoid holding the entire association list in memory at once.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Expensive stale issue refetch 🐞 Bug ☼ Reliability
Description
For each stale association, CleanupStaleBoardIssues calls collectAndExtractSingleIssue, which
fetches the full issue with expand=changelog and extracts/persists multiple entities. If many
issues leave a board at once, this can greatly increase Jira API load and task runtime (and may
trigger rate limiting/timeouts).
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[R87-99]

+	removed := 0
+	for _, bi := range allBoardIssues {
+		if onBoard[bi.IssueKey] {
+			continue
+		}
+
+		logger.Info("issue %s is no longer on board %d, updating state and removing association", bi.IssueKey, boardId)
+
+		// Re-fetch from Jira to update _tool_jira_issues with current status/team/resolution
+		if updateErr := collectAndExtractSingleIssue(taskCtx, data, db, bi.IssueKey); updateErr != nil {
+			logger.Warn(updateErr, "failed to update issue state for %s, will still remove board association", bi.IssueKey)
+		}
+
Relevance

●● Moderate

Repo has optimized Jira task scope for performance (PR #99), but no clear precedent rejecting
per-issue refetch patterns.

PR-#99

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Cleanup invokes collectAndExtractSingleIssue for each stale issue, and that helper explicitly
requests expand=changelog, reads the whole response, and performs wide extraction work beyond what
cleanup needs.

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[87-99]
backend/plugins/jira/tasks/parent_issue_collector.go[134-206]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When an issue is deemed stale, the cleanup path calls `collectAndExtractSingleIssue`, which requests `expand=changelog` and performs a broad extraction/save. This is heavy for the cleanup use-case (which only needs to refresh the issue's current status/team/resolution).

### Issue Context
The cleanup loop may process many stale issues in one run; doing a full issue+changelog fetch per stale issue can make the cleanup step disproportionately expensive.

### Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[87-99]
- backend/plugins/jira/tasks/parent_issue_collector.go[134-206]

### Suggested fix
- Add a purpose-built `refreshSingleIssueState` helper used by cleanup that:
 - omits `expand=changelog`
 - requests only required fields (via `fields=...`) needed to update `_tool_jira_issues` state
 - updates only the relevant issue columns (instead of extracting/saving comments/worklogs/changelogs/users)
- Optionally cap work per run (max stale updates) and/or add bounded concurrency with rate-limit handling if parallelizing.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational
3. Unbounded HTTP body read 🐞 Bug ☼ Reliability
Description
fetchBoardMembership reads the entire HTTP response body with io.ReadAll without a size cap. If
Jira returns an unexpectedly large payload, this can cause avoidable memory spikes during cleanup.
Code

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[R172-181]

+			if resp.StatusCode != http.StatusOK {
+				_ = resp.Body.Close()
+				return nil, errors.Default.New(fmt.Sprintf("unexpected status %d from board issues API", resp.StatusCode))
+			}
+
+			blob, readErr := errors.Convert01(io.ReadAll(resp.Body))
+			_ = resp.Body.Close()
+			if readErr != nil {
+				return nil, errors.Default.Wrap(readErr, "failed to read board response")
+			}
Relevance

●●● Strong

Team previously accepted limiting HTTP body reads via io.LimitReader around io.ReadAll (PR #97).

PR-#97

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The code path reads the full response body into memory without any upper bound before unmarshalling
JSON.

backend/plugins/jira/tasks/stale_board_issue_cleaner.go[155-191]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`fetchBoardMembership` uses `io.ReadAll(resp.Body)` without limiting the amount of data read, which can amplify the impact of unexpectedly large responses.

### Issue Context
While `maxResults=100` and `fields=id,key` should usually keep responses small, adding a hard cap improves robustness.

### Fix Focus Areas
- backend/plugins/jira/tasks/stale_board_issue_cleaner.go[172-181]

### Suggested fix
- Replace `io.ReadAll(resp.Body)` with `io.ReadAll(io.LimitReader(resp.Body, <maxBytes>))` (e.g., 5–10MB), and return a clear error if the limit is hit.
- (Optional) Use a shared helper for bounded response reading if the repo has one.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@fullsend-ai-review

fullsend-ai-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [race condition / data consistency] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:148 — A 404 response on any pagination page causes fetchBoardMembership to return (nil, nil), discarding all previously accumulated onBoard results. The caller interprets a nil map as "board not found" and skips cleanup entirely. While this is safe/conservative behavior (no data is deleted), a transient 404 on any single page request silently aborts the entire cleanup — even when earlier batches succeeded. Consider differentiating between a 404 on the first request (genuinely deleted board) and a 404 on later pages (likely transient error — should return an error instead).

Low

  • [error handling gap] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:79 — When collectAndExtractSingleIssue fails, the code logs a warning and proceeds to delete the board association. The issue retains stale status/resolution data in _tool_jira_issues while the association is removed. This follows the established codebase pattern (same approach in parent_issue_collector.go), and the issue will be refreshed on the next collection cycle for its new board.

  • [API contract / performance] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:76 — Each stale issue triggers an individual collectAndExtractSingleIssue API call. For boards with many stale issues after a large reorganization, this could result in hundreds of sequential API calls. Consistent with existing codebase conventions in parent_issue_collector.go.

  • [naming-convention] backend/plugins/jira/tasks/stale_board_issue_cleaner.go — The file uses a _cleaner.go suffix, which is the first of its kind in the codebase (existing patterns are _collector.go, _extractor.go, _convertor.go). The naming is semantically accurate for a cleanup task — just worth noting as establishing a new convention.

Previous run

Review

Findings

Medium

  • [edge-case] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:143 — The inner pagination loop in fetchBoardMembership increments startAt by the fixed constant staleBoardIssueCheckBatchSize (100) instead of by the number of results actually returned. The Jira Agile API does not guarantee returning exactly maxResults items per page — a board-level property can cap results below the requested maxResults. If Jira returns fewer than 100 items in a page (e.g., 50 when the board caps at 50) but total exceeds 100, the next request uses startAt=100 instead of startAt=50, skipping issues at positions 50–99. Those skipped issues would be absent from the onBoard map and incorrectly treated as no longer on the board, causing their associations to be deleted — the very data loss scenario this PR aims to fix.
    Remediation: Change the inner pagination loop to increment startAt by len(result.Issues) instead of staleBoardIssueCheckBatchSize. The termination condition startAt+len(result.Issues) >= result.Total already uses the actual count and remains correct.

Low

  • [injection-vuln] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:133 — Issue keys from _tool_jira_issues.issue_key are interpolated directly into a JQL issue IN (...) clause via strings.Join(keys, ",") without quoting or validation. While keys originate from Jira API responses and the risk is low (requires a compromised/corrupted DB), quoting or validating keys against ^[A-Z][A-Z0-9_]+-\d+$ would be a defense-in-depth improvement.

  • [scope-boundary] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:99 — The cleanup calls collectAndExtractSingleIssue for each stale issue, which re-syncs the full issue state (including changelogs, comments, worklogs, and users) from Jira before removing the board association. This adds API calls proportional to the number of stale issues and goes beyond the stated scope of removing stale associations. The rationale for this re-fetch should be documented in a code comment.

  • [error-handling] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:56 — The function loads all board issues into memory at once via db.All(). For boards with tens of thousands of issues, this could consume significant memory. This is a known pattern used elsewhere in the codebase (e.g., parent_issue_collector.go).

  • [error-handling] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:101 — When removeBoardAssociation fails, the function logs a warning and continues but does not aggregate failures. If all removals fail, the function returns nil (success). Callers cannot distinguish "nothing was stale" from "everything failed."

  • [naming-convention] backend/plugins/jira/tasks/stale_board_issue_cleaner.go — The file name stale_board_issue_cleaner.go is slightly longer than the established pattern in this package (e.g., issue_collector.go, parent_issue_collector.go), though multi-word descriptors do appear elsewhere.

  • [variable-extraction-pattern] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:35 — Local variable extraction order is data, db, logger while some files in this package use data, logger, db. The codebase is inconsistent on this point.

  • [subtask-meta-naming] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:23 — SubTaskMeta.Name cleanupStaleBoardIssues is longer than typical conventions (collectIssues, convertBoard), though the Stale qualifier is semantically important to distinguish this from a general board-issues operation.

  • [constant-naming] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:20 — The constant staleBoardIssueCheckBatchSize uses Go-conventional camelCase for unexported constants, while existing exported constants in this package use SCREAMING_SNAKE_CASE. The new constant is unexported, so camelCase follows Go convention.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

Review

Findings

Medium

  • [edge-case] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:91 — When collectAndExtractSingleIssue fails (API error, rate limit, issue deleted), the code logs a warning and still proceeds to delete the board association. If the failure was transient, the issue's metadata in _tool_jira_issues retains stale values (old status, team, resolution) while the board association is permanently removed. On subsequent syncs, the issue won't be re-checked since it's no longer in _tool_jira_board_issues, leaving permanently stale metadata with no recovery path.
    Remediation: Consider skipping the association removal when the re-fetch fails, or add a retry mechanism for issues removed without a successful state update.

  • [data-deletion-safety] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:33 — The subtask is enabled by default and deletes board-issue associations for any issue the Jira API does not return. While 404 responses correctly skip cleanup and non-200 responses return an error, a 200 response with an empty or truncated issues array (due to rate limiting, permission changes, or Jira server bugs) would cause all board-issue associations to be removed — the exact data loss scenario this PR aims to fix.
    Remediation: Add a safety threshold: if a large percentage of board issues (e.g., >50%) would be removed in a single run, log a warning and abort rather than proceeding with mass deletion.

  • [commit-trailer-missing] — Commits introducing upstream-divergent changes should include an Upstream-Status: trailer per AGENTS.md. This PR modifies the upstream Jira plugin without this trailer.
    Remediation: Add Upstream-Status: Pending or Upstream-Status: Konflux-specific trailer to the commit message.

  • [missing-documentation-update] docs/upstream-diffs.md:91 — The upstream-diffs.md file documents Konflux-specific additions to the Jira plugin's SubTaskMetas() list (currently only CollectParentIssuesMeta). This PR adds CleanupStaleBoardIssuesMeta to the same list but does not update the rebase notes to track this new divergence.
    Remediation: Update the Jira plugin section in docs/upstream-diffs.md to document both CollectParentIssuesMeta and CleanupStaleBoardIssuesMeta as Konflux-specific subtasks to watch during upstream rebases.

Low

  • [JQL-injection] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:131 — Issue keys from the database are concatenated directly into a JQL query string without validation. While keys originate from prior Jira API responses stored locally (not direct user input), adding format validation (e.g., ^[A-Z][A-Z0-9_]+-\d+$) would provide defense-in-depth. This follows an existing pattern in parent_issue_collector.go.

  • [error-handling] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:77db.All() loads all board issues into memory at once. For boards with tens of thousands of issues, consider cursor-based iteration or chunked processing to reduce memory pressure.

  • [API-rate-limiting] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:88 — For each stale issue, collectAndExtractSingleIssue makes an individual synchronous API call. After a team reorganization moving many issues, this could hit Jira rate limits. This follows the existing pattern in parent_issue_collector.go.

  • [naming-conventions] backend/plugins/jira/tasks/stale_board_issue_cleaner.go — The Cleanup action prefix is intentionally distinct from the existing Collect/Convert/Extract pattern since this is a different operation type, which is acceptable. However, the file name stale_board_issue_cleaner.go includes the adjective prefix stale_ which is inconsistent with the <entity>_<action>.go pattern used by other task files (e.g., issue_type_collector.go, board_convertor.go).
    Remediation: Consider renaming to board_issue_cleaner.go to better match the established pattern.

Previous run (3)

Review

Findings

High

  • [upstream-divergence-not-tracked] backend/plugins/jira/impl/impl.go:145 — This PR modifies the Jira plugin, which is not an owned plugin (no backend/plugins/jira/AGENTS.md exists). Per AGENTS.md, all modifications outside owned plugin directories must be tracked in docs/upstream-diffs.md with files changed, reason, upstream status, upstream PR link, owner, and rebase notes. The existing docs/upstream-diffs.md already tracks prior impl.go changes but does not include this new subtask registration or the new stale_board_issue_cleaner.go file.
    Remediation: Add an entry to docs/upstream-diffs.md documenting: files changed (impl/impl.go, tasks/stale_board_issue_cleaner.go), reason (fix data loss when tickets move between boards / DPROD-1338), upstream status, and rebase notes (watch for changes to SubTaskMetas() registration list).

Medium

  • [api-cost] backend/plugins/jira/tasks/stale_board_issue_cleaner.go — For each stale issue, collectAndExtractSingleIssue makes a synchronous API call (GET /api/2/issue/{key}?expand=changelog) one-by-one. Combined with the batch-check calls in fetchBoardMembership, a board with N issues and S stale issues incurs at least ceil(N/100) + S API calls per sync run. With EnabledByDefault: true, this runs on every sync. For boards with thousands of issues and hundreds of stale ones, this can significantly increase sync time and risk hitting Jira API rate limits. The synchronous Get() bypasses the ApiAsyncClient's worker/rate-limit scheduler.
    Remediation: Consider batching the collectAndExtractSingleIssue calls using the async client's worker pool (consistent with how collectors elsewhere in the plugin work), or add a log warning when the stale count exceeds a threshold.

Low

  • [performance] backend/plugins/jira/tasks/stale_board_issue_cleaner.go — Creating a transaction per stale issue in a loop is expensive. For S stale issues, S transactions are opened and committed sequentially. Consider collecting all stale issue IDs first and performing a bulk delete.
  • [edge-case] backend/plugins/jira/tasks/stale_board_issue_cleaner.godb.All(&allBoardIssues, ...) loads all issue keys and IDs for the board into memory at once. Extremely large boards (10,000+ issues) could cause memory pressure, though each struct is lightweight.
  • [test-inadequate] backend/plugins/jira/tasks/stale_board_issue_cleaner_test.go — Tests cover only fetchBoardMembership (8 test functions). The main CleanupStaleBoardIssues function — containing transaction logic, domain ID generation, collectAndExtractSingleIssue integration, and the deletion flow — has no test coverage.
  • [design-smell] backend/plugins/jira/impl/impl.go:145 — The new CleanupStaleBoardIssuesMeta is registered after ExtractEpics but before CollectAccounts with no comment explaining why this position is required. A brief comment would aid maintainability.
  • [pattern-inconsistency] backend/plugins/jira/tasks/stale_board_issue_cleaner.go — Explicit tx.Rollback() call on transaction error. While defensively correct (used in other plugins like q_dev), no other file in backend/plugins/jira/tasks/ uses transactions, making this an unfamiliar pattern for Jira plugin maintainers. Consider adding a comment explaining the rollback convention.
Previous run (4)

Review

Findings

Medium

  • [logic-error] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:155 — The inner pagination loop in fetchBoardMembership breaks when startAt+len(result.Issues) >= result.Total. If the Jira board API returns 0 issues in a page but total remains greater than startAt (e.g., due to permission filtering where total counts issues the current user cannot see), the loop will spin indefinitely — startAt increments by 100 each iteration but len(result.Issues) is 0, so the break condition is never satisfied until startAt alone exceeds total, wasting API calls on empty pages.
    Remediation: Add a guard to break the inner loop when len(result.Issues) == 0 to prevent infinite pagination through empty pages.

Low

  • [error-handling] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:96 — When collectAndExtractSingleIssue fails for a stale issue, the code logs a warning but proceeds to delete the board association. The issue retains its last-known state in _tool_jira_issues rather than being updated with current data from Jira. This is a documented design tradeoff (the primary goal is removing the stale association), but downstream domain-layer conversions will use the last-known state.

  • [test-adequacy] backend/plugins/jira/tasks/stale_board_issue_cleaner.go — No unit or integration tests are provided for CleanupStaleBoardIssues or fetchBoardMembership. Key branches — 404 handling, transaction rollback, batch pagination, empty board, and the collectAndExtractSingleIssue failure path — would benefit from test coverage.

  • [edge-case] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:70 — The SQL query uses INNER JOIN between _tool_jira_board_issues and _tool_jira_issues. Orphan board-issue associations (where issue_id has no corresponding row in _tool_jira_issues) are silently excluded and will never be cleaned up by this subtask. This is a reasonable design choice — the task needs issue_key to verify against the board API — but worth noting.

  • [edge-case] backend/plugins/jira/tasks/stale_board_issue_cleaner.go:67 — The function loads all board issues into memory at once via db.All. For boards with tens of thousands of issues this could cause memory pressure, though the loaded struct is small (string + uint64) and db.All is used elsewhere in this package (e.g., parent_issue_collector.go).


Labels: PR fixes a data loss bug in the Jira plugin's board-issue synchronization

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment bug Something isn't working labels Jul 21, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Jul 27, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 3:21 PM UTC · Ended 3:37 PM UTC
Commit: 37b10e4 · View workflow run →

Comment thread backend/plugins/jira/tasks/stale_board_issue_cleaner.go Outdated
@qodo-app-for-konflux-ci

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit d83dfa1

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jul 27, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 3:21 PM UTC · Completed 3:37 PM UTC
Commit: 37b10e4 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 8:51 AM UTC · Ended 9:07 AM UTC

Commit: 9ee3c25 · View workflow run →

@codecov-commenter

codecov-commenter commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 40.80%. Comparing base (37adf74) to head (578f4e1).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #129   +/-   ##
=======================================
  Coverage   40.80%   40.80%           
=======================================
  Files         147      147           
  Lines       10189    10189           
=======================================
  Hits         4158     4158           
  Misses       5927     5927           
  Partials      104      104           
Flag Coverage Δ
e2e-go 9.64% <ø> (ø)
unit-tests-python 55.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37adf74...578f4e1. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review August 18, 2026 09:07

Superseded by updated review

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Aug 18, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:51 AM UTC · Completed 9:07 AM UTC

Commit: 9ee3c25 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

🤖 Review · ⚠️ Cancelled · Started 10:23 AM UTC · Ended 10:28 AM UTC

Commit: 9ee3c25 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:29 AM UTC · Completed 10:45 AM UTC

Commit: 9ee3c25 · View workflow run →

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Aug 18, 2026

@mfrancisc mfrancisc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job 👍

Thanks for addressing my comments and sorry for the slow review.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:43 PM UTC · Completed 1:56 PM UTC

Commit: 9ee3c25 · View workflow run →

@fullsend-ai-review
fullsend-ai-review Bot dismissed their stale review August 18, 2026 13:56

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Aug 18, 2026
@flacatus

Copy link
Copy Markdown
Member

/ok-to-test

@flacatus
flacatus merged commit e93d5a6 into konflux-ci:main Aug 19, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants