fix(byoo-otel-collector): log otelcol-contrib exit error on shutdown by mesutoezdil · Pull Request #219 · NVIDIA/nvcf

mesutoezdil · 2026-07-16T21:52:01Z

TL;DR

runSecretsCheckLoop discarded the error from Process.Wait after signaling otelcol-contrib shutdown or restart, silently dropping non-zero and signal exits. Now logged, with tests for both paths. go build, go vet, go test pass.

Issues

Closes #267

Summary by CodeRabbit

Bug Fixes
- Improved shutdown and restart monitoring by preserving and reporting process exit errors instead of silently discarding them.
Tests
- Added automated coverage to verify that process exit errors are logged correctly and that successful exits produce no unnecessary logs.

coderabbitai · 2026-07-17T15:36:23Z

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

The CLI now captures and logs errors returned by otelCollectorProc.Wait() during termination-signal shutdown and secret-file-change restarts. Bazel configuration and table-driven tests validate error-producing and clean process exits.

Changes

otelcol-contrib lifecycle handling

Layer / File(s)	Summary
Capture process wait errors `src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go`	A process-waiting helper logs non-nil wait errors during interrupt shutdown and secret-change restart paths.
Validate wait error logging `src/compute-plane-services/byoo-otel-collector/internal/cli/BUILD.bazel`, `src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go`	A Bazel test target and table-driven tests cover error-producing and clean process exits.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 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 title clearly states the main change: logging otelcol-contrib exit errors on shutdown.
Linked Issues check	✅ Passed	The code now logs Process.Wait errors during interrupt shutdown and secret-triggered restarts as requested in `#267`.
Out of Scope Changes check	✅ Passed	The test target and test file are supporting changes for the requested fix, not unrelated scope.
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/byoo-otel-collector/internal/cli/cli.go`:
- Around line 75-77: Add regression tests for the otel collector wait-error
handling in both interrupt shutdown and secret-triggered restart, configuring
fakeProcess.waitErr and asserting the logger records the error from
otelCollectorProc.Wait. Cover both affected wait paths and preserve existing
behavior; only omit tests if the merge-request description documents the
specific reason.

🪄 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: e1f70d70-bbc0-4932-8c8d-ae11fce4db5c

📥 Commits

Reviewing files that changed from the base of the PR and between ed2f0bd and 01dc720.

📒 Files selected for processing (1)

src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go

coderabbitai

Actionable comments posted: 3

🤖 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/byoo-otel-collector/internal/cli/cli_test.go`:
- Around line 46-48: Update the tests around the interrupt and secret-triggered
restart cases to exercise their real shutdown and restart control-flow branches,
including the corresponding runSecretsCheckLoop call sites, rather than invoking
waitAndLogProcessExit directly. Alternatively, explicitly rename them as
helper-level tests to reflect their limited coverage.
- Around line 53-54: Update the test setup around logger.Logger to capture its
original value before replacing it with the observer logger, then register
t.Cleanup to restore the original global logger after the test completes.
- Around line 58-64: Update the test’s log assertions after logs.TakeAll() to
inspect the captured entry for non-nil wait-error cases. In addition to
verifying the expected entry count, assert that its message or error field
contains tt.waitErr.Error(), while preserving the existing no-entry assertion
when tt.wantLog is false.

🪄 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: e0f3122e-3480-4364-8603-51e034e155b5

📥 Commits

Reviewing files that changed from the base of the PR and between 01dc720 and c3f24fb.

📒 Files selected for processing (3)

src/compute-plane-services/byoo-otel-collector/internal/cli/BUILD.bazel
src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go
src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go

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

src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go

kristinapathak · 2026-07-20T16:52:10Z

This looks good; can you make a github issue for it?

mesutoezdil · 2026-07-20T16:57:01Z

Filed #267 and linked it in the description.

kristinapathak · 2026-07-21T05:41:51Z

@mesutoezdil , can you please rebase off of main? It should fix the codeql failures.

mesutoezdil · 2026-07-21T08:28:56Z

done, thx

byoo-otel-collector, nvcf-unbound, and the four workers (worker-init, worker-llm-credentials, worker-task, worker-utils) each have their own MODULE.bazel and a public OCI base (nvcr.io/nvidia/distroless/go, digest anonymously pullable), but were never added as matrix rows. So a change touching only one of them (e.g. #219 for byoo, which even edits cli_test.go) matched no row, produced an empty matrix, and skipped Bazel entirely with a green check. Add them so their build and tests run on PRs and main. nvsnap remains the only intentional exclusion (private, non-public OCI base; see the detect comment). Rows start with tests enabled; any that fail tests in the GHA environment will be flipped to build-only with a tracking issue, per the documented policy. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

runSecretsCheckLoop discarded the error from Process.Wait() after both a graceful shutdown and a secrets-triggered restart, so a non-zero or signal exit from otelcol-contrib was silently dropped. Log it instead. Verified with go build, go vet, go test ./..., and golangci-lint --enable-only errcheck, all clean. NO-REF Signed-off-by: mesutoezdil <mesudozdil@gmail.com>

Extract the duplicated wait-and-log logic behind otelCollectorProc.Wait() into waitAndLogProcessExit, taking a small interface instead of the concrete *os.Process so both the interrupt and secret-triggered restart paths can be exercised with a fake in a unit test. JIRA: NO-REF NVBug: none Signed-off-by: mesutoezdil <mesudozdil@gmail.com>

Clarify the test names as helper-level coverage, restore the global logger after each case, and assert the logged message contains the underlying wait error text instead of only counting log entries. JIRA: NO-REF NVBug: none Signed-off-by: mesutoezdil <mesudozdil@gmail.com>

mesutoezdil requested a review from a team as a code owner July 16, 2026 21:52

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread src/compute-plane-services/byoo-otel-collector/internal/cli/cli.go Outdated

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go Outdated

Comment thread src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go

Comment thread src/compute-plane-services/byoo-otel-collector/internal/cli/cli_test.go

mesutoezdil mentioned this pull request Jul 20, 2026

byoo-otel-collector: otelcol-contrib exit error silently discarded on shutdown and restart #267

Closed

mesutoezdil force-pushed the mesutoezdil/fix/byoo-otel-collector-wait-error branch from fac8528 to dbcfddc Compare July 21, 2026 08:22

kristinapathak enabled auto-merge July 21, 2026 16:00

balajinvda mentioned this pull request Jul 21, 2026

ci(bazel): run root tests, quarantine flaky go-lib, add missing service rows #311

Merged

auto-merge was automatically disabled July 21, 2026 17:51
Head branch was pushed to by a user without write access

mesutoezdil force-pushed the mesutoezdil/fix/byoo-otel-collector-wait-error branch from dbcfddc to abd320b Compare July 21, 2026 17:51

mesutoezdil added 3 commits July 21, 2026 20:28

mesutoezdil force-pushed the mesutoezdil/fix/byoo-otel-collector-wait-error branch from abd320b to 101666b Compare July 21, 2026 18:28

kristinapathak approved these changes Jul 21, 2026

View reviewed changes

kristinapathak added this pull request to the merge queue Jul 21, 2026

Merged via the queue into NVIDIA:main with commit adc62ed Jul 21, 2026
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(byoo-otel-collector): log otelcol-contrib exit error on shutdown#219

fix(byoo-otel-collector): log otelcol-contrib exit error on shutdown#219
kristinapathak merged 3 commits into
NVIDIA:mainfrom
mesutoezdil:mesutoezdil/fix/byoo-otel-collector-wait-error

mesutoezdil commented Jul 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 17, 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!

Uh oh!

Uh oh!

Uh oh!

kristinapathak commented Jul 20, 2026

Uh oh!

mesutoezdil commented Jul 20, 2026

Uh oh!

kristinapathak commented Jul 21, 2026

Uh oh!

mesutoezdil commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

TL;DR

Issues

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 17, 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!

Uh oh!

Uh oh!

Uh oh!

kristinapathak commented Jul 20, 2026

Uh oh!

mesutoezdil commented Jul 20, 2026

Uh oh!

kristinapathak commented Jul 21, 2026

Uh oh!

mesutoezdil commented Jul 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mesutoezdil commented Jul 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading