Skip to content

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

Open
mesutoezdil wants to merge 1 commit into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/fix/nvca-queue-pull-error-overwrite
Open

fix(nvca): keep termination queue error in status check#363
mesutoezdil wants to merge 1 commit into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/fix/nvca-queue-pull-error-overwrite

Conversation

@mesutoezdil

@mesutoezdil mesutoezdil commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Why

A termination queue pull error was being dropped before the status check.

What changed

Kept the termination queue error instead of letting the creation queue call overwrite it. Added a regression test for this case.

Testing

go build, go vet, go test ./pkg/nvca all pass.

Closes #362

Summary by CodeRabbit

Summary by CodeRabbit

  • Bug Fixes

    • Fixed queue synchronization so termination-queue retrieval failures correctly keep the queue manager in a non-OK status.
    • Prevented creation-matrix errors from masking earlier termination-queue error detection during overall status evaluation.
  • Tests

    • Added coverage to confirm termination queue pull failures are not overridden.
    • Enhanced the queue client mock to deterministically simulate message-receive failures for targeted queue URLs.

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

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

QueueManager.SyncQueues now preserves termination-queue pull errors while incorporating creation-matrix errors into the final status decision. 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

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes preserve termination queue pull errors in SyncQueues and add a regression test for issue #362.
Out of Scope Changes check ✅ Passed The test and mock updates directly support the queue-status fix and do not introduce unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title follows Conventional Commits and accurately describes the bug fix to preserve termination queue errors in status checks.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot 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.

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

Comment thread src/compute-plane-services/nvca/pkg/nvca/queue_manager.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
mesutoezdil force-pushed the mesutoezdil/fix/nvca-queue-pull-error-overwrite branch from b5e303a to c510ab8 Compare July 22, 2026 21:34
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.

nvca: termination queue error dropped from status check

1 participant