Repository navigation
Conversation
Two streams of one risk pass at the same time. Each job deletes its own link and counts the rest; under READ COMMITTED neither sees the other's uncommitted delete, so both count one link left and neither remediates the risk. It stays open with no link, where no later passing evidence can reach it. The test runs the second job between the first job's count and its commit, and fails until the next commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rQ3hHdBbTmRTNGECpX8JK
resolveRiskEvidenceLink now takes the risk's id and locks the risk with SELECT ... FOR UPDATE before it deletes its stream's link and counts the rest, so the resolution of the risk's other stream counts only after this one commits, and the last one remediates the risk. It also acts on the status read under the lock rather than the one the caller read earlier: a risk closed or remediated in the meantime is left alone, and an accepted risk keeps its status (the evidence_recovered event is emitted). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rQ3hHdBbTmRTNGECpX8JK
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change addresses the concurrent resolution race, and no merge-blocking issue was identified. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: locking the risk while resolving its evidence links to prevent the concurrency race.
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
I’m a rabbit with a test to run,
Two streams hop in, then both are done.
The risk is locked and checked with care,
No evidence links remain to share.
I nibble greens and cheer the race,
All jobs return in steady pace.
Comment @coderabbitai help to get the list of available commands.
gusfcarvalho
left a comment
gusfcarvalho
left a comment
There was a problem hiding this comment.
ccf-review: REQUEST_CHANGES
Note
🤖 This review was generated by the CCF Review skill, performed with LLMs (Claude). A CCF maintainer read and triaged every finding before posting; severities come from the shared CCF review rule catalog, not from the model's judgement. If anything looks wrong, please reply on the thread.
1 Must-fix, 1 Should-fix.
| if err != nil { | ||
| return fmt.Errorf("failed to lock risk: %w", err) | ||
| } | ||
| // Closed or remediated since the caller read it: nothing to resolve. |
There was a problem hiding this comment.
[Must-fix] CORE-TEST-001 · New behaviour without a test
Besides the race fix, this PR changes which status the resolver acts on: it now skips a risk that became closed/remediated after the batch read, and takes the risk-accepted branch (audit event, no status change) when a user accepted the risk while the job was in flight. Before, the stale batch status was used, so an in-flight job could remediate a just-accepted risk. The new race test only covers two passing streams on an open risk; none of the status re-check paths has a test that fails without this change.
Why: Untested behaviour regresses silently and the next refactor removes it without anyone noticing.
Fix: Add integration cases using the same callback technique (hook the batch risk load in handleEvidenceResolution, then change the status): (a) risk closed in between → link kept, status stays closed; (b) risk set to risk-accepted in between → link removed, status stays risk-accepted, evidence_recovered event emitted.
ccf-review · fb722c0f09b9 · rules@992d25d050e9
🤖 Generated by the CCF Review skill (LLM-assisted), triaged by a CCF maintainer.
| return fmt.Errorf("failed to lock risk: %w", err) | ||
| } | ||
| // Closed or remediated since the caller read it: nothing to resolve. | ||
| if risk.Status == string(risks.RiskStatusClosed) || risk.Status == string(risks.RiskStatusRemediated) { |
There was a problem hiding this comment.
[Should-fix] CORE-DUP-001 · Logic or literal duplicated where it must stay in sync
status == closed || status == remediated is now written twice in the worker: the pre-filter on the unlocked batch read in handleEvidenceResolution (line 1092) and the authoritative check under the lock here. The risks package already names this predicate as the unexported isTerminalRiskStatus (internal/service/relational/risks/scores.go:183). If a terminal status is added and only the caller-side copy is updated, an in-flight job can again act on a risk that moved to that status: a narrow version of the stale-status bug this PR fixes.
Why: Copies drift: one gets fixed or changed and the other does not.
Fix: Export the existing helper (risks.IsTerminalRiskStatus) and call it at both worker sites instead of repeating the comparison.
ccf-review · 8ca676f95194 · rules@992d25d050e9
🤖 Generated by the CCF Review skill (LLM-assisted), triaged by a CCF maintainer.
|
Hi @puzza007 ! Thanks for this fix! There's just one test case we might be missing - and one nit :) |
This branch has not been deployed
2 participants
Bug
When a risk's evidence streams pass, the worker deletes that stream's link to the risk, counts the risk's remaining links, and marks the risk remediated if none are left. If two passing streams for the same risk are processed at the same time, each job deletes its own link but still sees the other's link, because the other job's delete hasn't committed yet. Neither job remediates the risk, so it stays open with no links left, and no later evidence can close it.
Fix
resolveRiskEvidenceLinknow locks the risk row (SELECT … FOR UPDATE) at the start of its transaction. The second job waits until the first commits, then counts zero links and remediates the risk. It also acts on the status read under the lock, so a risk closed or remediated in the meantime is left alone.Test
TestConcurrentPassingStreamsRemediateTheRisk(integration) uses a GORM callback to start the second job right after the first job's link count. Onmainit fails with the risk stillopenand no links. With the fix it passes. The test is in the first commit and the fix in the second.Summary by CodeRabbit