fix(nvca): keep termination queue error in status check by mesutoezdil · Pull Request #363 · NVIDIA/nvcf

mesutoezdil · 2026-07-22T20:45:10Z

A termination queue pull error was being dropped before the status check.
Kept the termination queue error instead of letting the creation queue call overwrite it. Added a regression test for this case.

Closes #362

Summary by CodeRabbit

Bug Fixes
- Improved queue synchronization so a failure receiving from the termination queue correctly results in a non-OK queue manager status, even if other queue operations succeed.
Tests
- Added coverage to ensure termination-queue receive failures are not masked by other queue processing.
Chores
- Enhanced the queue client mock to support deterministic receive failures for a specified queue URL.

coderabbitai · 2026-07-22T20:45:31Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

QueueManager.SyncQueues preserves termination-queue pull errors when combining creation-matrix errors for status evaluation. The queue mock supports targeted receive failures, covered by a regression test.

Changes

Queue error status

Layer / File(s)	Summary
Configure targeted queue failures `src/compute-plane-services/nvca/pkg/queue/mock/mock.go`	Adds queue-specific failure configuration and an early error return in `ReceiveMessage`.
Combine queue error states `src/compute-plane-services/nvca/pkg/nvca/queue_manager.go`, `src/compute-plane-services/nvca/pkg/nvca/queue_manager_test.go`	Combines creation-matrix and termination-queue errors without masking either condition, and verifies the termination failure leaves status non-OK.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: estroz

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The Conventional Commits fix(nvca) title accurately describes the main bug fix in the diff.
Linked Issues check	✅ Passed	The change preserves termination queue errors for status checks and adds a regression test, matching issue `#362`.
Out of Scope Changes check	✅ Passed	The mock client and test additions directly support the reported fix and do not appear unrelated.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/compute-plane-services/nvca/pkg/nvca/queue_manager.go`:
- Around line 452-455: Add regression coverage for SyncQueues covering the case
where termination queue pulling fails while creation pulling succeeds. Assert
that the queue manager remains non-OK after the queue-error merge, using the
existing test helpers and setup patterns for SyncQueues.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e2440762-205e-441e-a612-803e6c451915

📥 Commits

Reviewing files that changed from the base of the PR and between 44c5fac and b5e303a.

📒 Files selected for processing (1)

src/compute-plane-services/nvca/pkg/nvca/queue_manager.go

coderabbitai

🧹 Nitpick comments (1)

src/compute-plane-services/nvca/pkg/queue/mock/mock.go (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add individual Godoc comments for both exported fields.

Document each exported field with a comment beginning with its name, especially FailReceiveErr, which currently has no dedicated comment.

As per coding guidelines, exported Go symbols in src/compute-plane-services/nvca/**/*.go require Godoc comments.
Proposed documentation update
-	// If set, ReceiveMessage returns FailReceiveErr for FailReceiveQueueURL.
+	// FailReceiveQueueURL is the queue URL for which ReceiveMessage should fail.
 	FailReceiveQueueURL string
+	// FailReceiveErr is returned by ReceiveMessage for FailReceiveQueueURL.
 	FailReceiveErr error
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/compute-plane-services/nvca/pkg/queue/mock/mock.go` around lines 41 - 44,
Add separate Godoc comments for the exported fields FailReceiveQueueURL and
FailReceiveErr in the mock configuration, with each comment beginning with its
corresponding field name and describing its purpose.
Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/compute-plane-services/nvca/pkg/queue/mock/mock.go`:
- Around line 41-44: Add separate Godoc comments for the exported fields
FailReceiveQueueURL and FailReceiveErr in the mock configuration, with each
comment beginning with its corresponding field name and describing its purpose.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c57bd701-d383-4951-bd8a-577f1dd3eb52

📥 Commits

Reviewing files that changed from the base of the PR and between c510ab8 and 63900f2.

📒 Files selected for processing (3)

src/compute-plane-services/nvca/pkg/nvca/queue_manager.go
src/compute-plane-services/nvca/pkg/nvca/queue_manager_test.go
src/compute-plane-services/nvca/pkg/queue/mock/mock.go

🚧 Files skipped from review as they are similar to previous changes (2)

src/compute-plane-services/nvca/pkg/nvca/queue_manager_test.go
src/compute-plane-services/nvca/pkg/nvca/queue_manager.go

mesutoezdil · 2026-07-23T17:54:13Z

@Max-NV @nvjmcnamee could you take a look when you get a chance? Thanks!

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/compute-plane-services/nvca/pkg/queue/mock/mock.go`:
- Around line 41-44: Add Godoc comments for both exported fields
FailReceiveQueueURL and FailReceiveErr in the mock queue configuration, ensuring
each comment clearly begins with its corresponding field name and documents its
purpose.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5591fbf4-0f05-444f-a7ee-96ebaad7e092

📥 Commits

Reviewing files that changed from the base of the PR and between 63900f2 and d108d3d.

📒 Files selected for processing (3)

src/compute-plane-services/nvca/pkg/nvca/queue_manager.go
src/compute-plane-services/nvca/pkg/nvca/queue_manager_test.go
src/compute-plane-services/nvca/pkg/queue/mock/mock.go

🚧 Files skipped from review as they are similar to previous changes (2)

src/compute-plane-services/nvca/pkg/nvca/queue_manager.go
src/compute-plane-services/nvca/pkg/nvca/queue_manager_test.go

The creation queue call reassigned anyQueuePullError with :=, so a termination queue pull error was dropped and status could report OK when it should not. Added a mock queue error hook and a regression test that fails the termination queue pull while the creation queue pull succeeds, asserting status stays not OK. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>

mesutoezdil requested a review from a team as a code owner July 22, 2026 20:45

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread src/compute-plane-services/nvca/pkg/nvca/queue_manager.go

mesutoezdil force-pushed the mesutoezdil/fix/nvca-queue-pull-error-overwrite branch 2 times, most recently from f770bd3 to 63900f2 Compare July 23, 2026 17:48

coderabbitai Bot reviewed Jul 23, 2026

View reviewed changes

mesutoezdil force-pushed the mesutoezdil/fix/nvca-queue-pull-error-overwrite branch from 63900f2 to d108d3d Compare July 23, 2026 18:19

coderabbitai Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread src/compute-plane-services/nvca/pkg/queue/mock/mock.go

mesutoezdil force-pushed the mesutoezdil/fix/nvca-queue-pull-error-overwrite branch from d108d3d to ae81725 Compare July 23, 2026 21:28

mesutoezdil requested a review from a team as a code owner July 23, 2026 21:28

mesutoezdil requested a review from estroz July 23, 2026 21:28

mesutoezdil force-pushed the mesutoezdil/fix/nvca-queue-pull-error-overwrite branch 3 times, most recently from 2073e37 to 2c686bc Compare July 24, 2026 08:27

mesutoezdil force-pushed the mesutoezdil/fix/nvca-queue-pull-error-overwrite branch from 9eaf866 to 596c6ce Compare July 24, 2026 10:22

vrv3814 approved these changes Jul 24, 2026

View reviewed changes

Merge branch 'main' into mesutoezdil/fix/nvca-queue-pull-error-overwrite

ee0a222

vrv3814 enabled auto-merge July 24, 2026 10:46

vrv3814 added this pull request to the merge queue Jul 24, 2026

Merged via the queue into NVIDIA:main with commit e9e0c6f Jul 24, 2026
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(nvca): keep termination queue error in status check#363

fix(nvca): keep termination queue error in status check#363
vrv3814 merged 2 commits into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/fix/nvca-queue-pull-error-overwrite

mesutoezdil commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

mesutoezdil commented Jul 23, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mesutoezdil commented Jul 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mesutoezdil commented Jul 23, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mesutoezdil commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading