fix(worker): lock the risk while resolving its evidence links by puzza007 · Pull Request #490 · compliance-framework/api

puzza007 · 2026-10-11T04:48:32Z

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

resolveRiskEvidenceLink now 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. On main it fails with the risk still open and no links. With the fix it passes. The test is in the first commit and the fix in the second.

Summary by CodeRabbit

Bug Fixes
- Improved concurrent evidence resolution so eligible risks are remediated reliably without leaving evidence links behind.
- Prevented resolution from changing risks that become closed or accepted during processing; closed risks retain their evidence links.
- Ensured terminal risks have no open risk score snapshots.
Tests
- Added coverage for simultaneous resolutions and for risks changing status while resolution is in progress, including verification of evidence links and recovery events.

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

coderabbitai · 2026-10-11T04:48:51Z

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 944ac1bb-d546-462a-b982-1aecdd168770

📥 Commits

Reviewing files that changed from the base of the PR and between c93cc32 and 6eda4c2.

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

Use the checkbox below for a quick retry:

🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 16b0b3c4-d8e5-48a9-b7e9-a978910e5031

📥 Commits

Reviewing files that changed from the base of the PR and between 2d45d7e and c93cc32.

📒 Files selected for processing (3)

internal/service/relational/risks/scores.go
internal/service/worker/risk_evidence_worker.go
internal/service/worker/risk_resolution_race_integration_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The resolver now locks and reads a risk before resolving evidence links. Integration tests cover concurrent satisfied-evidence jobs and status changes during resolution.

Changes: Risk Evidence Resolution

Layer / File(s)	Summary
Shared terminal-status check `internal/service/relational/risks/scores.go`, `internal/service/worker/risk_evidence_worker.go`	The exported `IsTerminalRiskStatus` helper identifies closed and remediated risks. Snapshot recording and evidence resolution use the helper.
Locked resolution and race tests `internal/service/worker/risk_evidence_worker.go`, `internal/service/worker/risk_resolution_race_integration_test.go`, `internal/service/worker/risk_evidence_integration_helpers_test.go`	The resolver accepts a risk UUID and checks the locked risk record before resolving evidence links. Integration tests cover concurrent jobs and status changes during resolution. A test helper creates and persists stream evidence.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: gusfcarvalho

Merge Risk: ⚪ Minimal · up to c93cc

The change fixes a race where concurrent passing evidence streams could leave a risk open with no evidence links. Tests cover the concurrent case and closed or accepted risks changing during resolution. No actionable merge-blocking risk remains.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

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 4 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
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 reported race.
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.

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.

❤️ Share

_{A rabbit checks the risk in place
While evidence links join the race
Two jobs finish, the tests confirm
Closed risks keep their stated term
I nibble greens and watch locks turn}

_{Comment @coderabbitai help to get the list of available commands.}

gusfcarvalho

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.

gusfcarvalho · 2026-10-12T10:41:04Z

Hi @puzza007 ! Thanks for this fix! There's just one test case we might be missing - and one nit :)

Resolution checked "closed or remediated" twice, on its unlocked read and under the risk lock, apart from the risks package's own helper. Export IsTerminalRiskStatus and use it at both sites, so a new terminal status cannot reach one check and miss the other. Lock the risk the way the rest of the code does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Resolution now acts on the status it reads under the risk lock. Change the status right after its unlocked read of the linked risks: a closed risk keeps its link and stays closed, and an accepted risk loses its link, stays accepted and gets an evidence_recovered event. Both fail on the code before the lock, which remediated the risk from its stale status. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

puzza007 · 2026-10-13T07:49:23Z

Hey @gusfcarvalho! Hopefully the last two commits should do it

gusfcarvalho

ccf-review: APPROVE

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.

no findings.

Re-review at c93cc32: both earlier threads are fixed and resolved.

CORE-TEST-001: TestResolutionLeavesRiskClosedMeanwhile and TestResolutionKeepsRiskAcceptedMeanwhile cover the status re-read under the lock.
CORE-DUP-001: both worker sites now use risks.IsTerminalRiskStatus.

Verified locally: build, vet, lint and swag are clean, and TestRiskResolutionRaceIntegrationSuite passes. With main's worker code, all three tests fail as expected. Thanks for the fix!

gusfcarvalho · 2026-10-13T09:21:56Z

yup those would do! Thanks! <3

🤖 I have created a release *beep* *boop* --- ## [0.21.1](v0.21.0...v0.21.1) (2026-10-14) ### Bug Fixes * **deps:** update module github.com/moby/go-archive to v0.3.0 [security] ([#492](#492)) ([e71ddf2](e71ddf2)) * **deps:** update module golang.org/x/crypto to v0.56.0 [security] ([#494](#494)) ([9b8abe7](9b8abe7)) * **deps:** update module golang.org/x/mod to v0.40.0 [security] ([#495](#495)) ([f968a6b](f968a6b)) * **deps:** update module golang.org/x/net to v0.60.0 [security] ([#501](#501)) ([bdd97c6](bdd97c6)) * **worker:** lock the risk while resolving its evidence links ([#490](#490)) ([b9958ae](b9958ae)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: ccf-release-bot[bot] <253916425+ccf-release-bot[bot]@users.noreply.github.com>

puzza007 and others added 2 commits October 11, 2026 14:34

gusfcarvalho requested changes Oct 12, 2026

View reviewed changes

Comment thread internal/service/worker/risk_evidence_worker.go

Comment thread internal/service/worker/risk_evidence_worker.go Outdated

puzza007 and others added 2 commits October 13, 2026 20:32

gusfcarvalho approved these changes Oct 13, 2026

View reviewed changes

Merge branch 'main' into fix/risk-resolution-row-lock

6eda4c2

gusfcarvalho enabled auto-merge (squash) October 13, 2026 09:21

gusfcarvalho disabled auto-merge October 13, 2026 09:29

gusfcarvalho merged commit b9958ae into compliance-framework:main Oct 13, 2026
14 checks passed

ccf-release-bot Bot mentioned this pull request Oct 13, 2026

chore(main): release 0.21.1 #499

Merged

Navigation Menu

Repository navigation

fix(worker): lock the risk while resolving its evidence links - #490

fix(worker): lock the risk while resolving its evidence links#490
gusfcarvalho merged 5 commits into
compliance-framework:mainfrom
puzza007:fix/risk-resolution-row-lock

puzza007 commented Oct 11, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 11, 2026 •

edited

Loading

Review skipped

Uh oh!

gusfcarvalho left a comment

Uh oh!

Uh oh!

Uh oh!

gusfcarvalho commented Oct 12, 2026

Uh oh!

puzza007 commented Oct 13, 2026

Uh oh!

gusfcarvalho left a comment

Uh oh!

gusfcarvalho commented Oct 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

puzza007 commented Oct 11, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Fix

Test

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

gusfcarvalho left a comment

Choose a reason for hiding this comment

ccf-review: REQUEST_CHANGES

Uh oh!

Uh oh!

Uh oh!

gusfcarvalho commented Oct 12, 2026

Uh oh!

puzza007 commented Oct 13, 2026

Uh oh!

gusfcarvalho left a comment

Choose a reason for hiding this comment

ccf-review: APPROVE

Uh oh!

gusfcarvalho commented Oct 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

puzza007 commented Oct 11, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 11, 2026 •

edited

Loading