fix(metrics): fallback to mock writer on file open failure to prevent panic by abhaygoudannavar · Pull Request #596 · urunc-dev/urunc

abhaygoudannavar · 2026-05-01T09:41:20Z

Description

When timestamps are enabled in the urunc configuration (/etc/urunc/config.toml) but the target file path is invalid or the parent directory does not exist, NewZerologMetrics() returns nil. Because the caller assigns this directly without a nil check, any subsequent metrics.Capture() calls result in a nil pointer dereference, crashing urunc silently.

This PR fixes the issue by returning a &mockWriter{} (safe no-op fallback) and logging a warning instead of returning nil on file open failure. This ensures urunc gracefully disables metrics rather than panicking.

Related issues

Fixes fix(metrics): NewZerologMetrics returns nil on file open failure causing panic #595

How was this tested?

Enabled timestamps in /etc/urunc/config.toml with enabled = true and destination set to a non-existent directory (/var/log/urunc/timestamps.log).
Ran sudo nerdctl run --rm --runtime io.containerd.urunc.v2 harbor.nbfc.io/nubificus/urunc/nginx-qemu-unikraft-initrd:latest.
Verified that urunc starts normally, logs a warning about the file, and gracefully disables metrics without crashing.
Ran make unittest to ensure no existing tests were broken.

LLM usage

No usage

Checklist

I have read the contribution guide.
The linter passes locally (make lint).
The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
If LLMs were used: I have read the llm policy.

netlify · 2026-05-01T09:41:26Z

✅ Deploy Preview for urunc canceled.

Name	Link
🔨 Latest commit	`49d40bd`
🔍 Latest deploy log	https://app.netlify.com/projects/urunc/deploys/6a3a8414f2ea8f00083ce1d0

cmainas · 2026-05-05T12:04:47Z

Hello @abhaygoudannavar ,

please do not overwrite the PR description. Read the contribution guide for more information.

abhaygoudannavar · 2026-05-06T20:27:33Z

@cmainas i have updated the PR description as asked for now it can be merged.

cmainas

Hello @abhaygoudannavar ,

you have linked a PR instead of the issue in the description. Also, I left a comment for the unit test.

abhaygoudannavar · 2026-05-10T10:22:04Z

@cmainas i have done the changes asked for.

Corrected the PR description to correctly link the original issue (fix(metrics): NewZerologMetrics returns nil on file open failure causing panic #595).
Updated the unit test to use require.NotNil as suggested.

cmainas

Hello @abhaygoudannavar ,

thank you for the changes. I have added a comment.

cmainas

Hello @abhaygoudannavar ,

it seems there was a misunderstanding. Maybe I di not place the comment in the correct line. The check for the nil return should remain, the call later to Capture should get removed. We should not test by checking if the test crashes.

abhaygoudannavar · 2026-05-15T14:17:09Z

@cmainas Thanks for the clarification, I've updated the test to explicitly check for a non-nil writer instead of relying on the panic recovery. The PR has been updated.

cmainas

Hello @abhaygoudannavar ,

why were the asserts got removed?

abhaygoudannavar · 2026-05-20T18:08:10Z

Hey @cmainas Updated the tests to use assert.Equal and require.NotNil/require.NoError from testify instead of manual checks. The asserts were removed unintentionally in the initial commit fixed now.v

cmainas · 2026-05-21T07:57:05Z

Thank you @abhaygoudannavar for the changes. Could you please rebase over main so the CI can recognize you?

abhaygoudannavar · 2026-05-24T21:09:35Z

@cmainas i have rebased over the main.

cmainas · 2026-06-22T18:22:49Z

Hello @abhaygoudannavar ,

could you rebase over main and squash the non-unit test commits (or squash everything)?

abhaygoudannavar · 2026-06-23T06:11:49Z

@cmainas i have rebased over the main and squashed the non unit test commits.
let me know if anything else is needed.

cmainas

Hello @abhaygoudannavar ,

thank you for the rebase. Just two more small comments and we are good to go.

cmainas · 2026-06-23T08:22:45Z

 		file, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666)
 		if err != nil {
-			return nil
+			logrus.Warnf("Failed to open metrics file %s: %v. Metrics will be disabled.", target, err)


It would be better to return the error and log it form the caller, rather than importing logrus here. This will allow us in the future to handle this error differently.

cmainas · 2026-06-23T08:25:59Z

+	// Create the metrics writer with timestamps enabled but an invalid path
+	writer := NewZerologMetrics(true, invalidPath, "container-test")
+
+	require.NotNil(t, writer, "expected non-nil writer (fallback to mockWriter), got nil")


Another useful check here would be to ensure that writer is mockWriter.

When timestamps are enabled but the target file cannot be opened, NewZerologMetrics returned nil. This caused a nil pointer dereference on subsequent metrics.Capture() calls. This fix gracefully degrades to a no-op mockWriter and logs a warning. Adds a regression test to verify the fallback behavior. Fixes: urunc-dev#595 Signed-off-by: abhaygoudannavar <abhaysgoudnvr@gmail.com>

abhaygoudannavar · 2026-06-23T13:08:14Z

@cmainas i have addressed both the comments, All unit tests pass. The commit has been squashed and rebased over main.

cmainas

Thank you @abhaygoudannavar for the changes.

When timestamps are enabled but the target file cannot be opened, NewZerologMetrics returned nil. This caused a nil pointer dereference on subsequent metrics.Capture() calls. This fix gracefully degrades to a no-op mockWriter and logs a warning. Adds a regression test to verify the fallback behavior. PR: #596 Fixes: #595 Signed-off-by: abhaygoudannavar <abhaysgoudnvr@gmail.com> Reviewed-by: Charalampos Mainas <cmainas@nubificus.co.uk> Approved-by: Charalampos Mainas <cmainas@nubificus.co.uk>

abhaygoudannavar mentioned this pull request May 3, 2026

metrics: fall back to mockWriter on file open failure #608

Closed

neo-0007 mentioned this pull request May 6, 2026

docs: Developer Guide > Execution time breakdown > How to enable timestamping outdated #624

Open

cmainas requested changes May 8, 2026

View reviewed changes

Comment thread internal/metrics/metrics_test.go Outdated

abhaygoudannavar force-pushed the one/metrics-file branch from 51ba5c5 to 8a31ff9 Compare May 10, 2026 10:17

cmainas reviewed May 14, 2026

View reviewed changes

Comment thread internal/metrics/metrics_test.go Outdated

abhaygoudannavar force-pushed the one/metrics-file branch 2 times, most recently from 2b92199 to 05344a3 Compare May 14, 2026 21:06

abhaygoudannavar requested a review from cmainas May 14, 2026 21:07

cmainas reviewed May 15, 2026

View reviewed changes

Comment thread internal/metrics/metrics_test.go Outdated

cmainas mentioned this pull request May 15, 2026

FIX - implement OCI runtime state command [Enhancement] #621

Closed

4 tasks

abhaygoudannavar force-pushed the one/metrics-file branch from 9a4d910 to 852d6d6 Compare May 15, 2026 14:15

abhaygoudannavar requested a review from cmainas May 15, 2026 14:18

abhaygoudannavar force-pushed the one/metrics-file branch from 852d6d6 to f5df606 Compare May 15, 2026 14:37

cmainas reviewed May 18, 2026

View reviewed changes

Comment thread internal/metrics/metrics_test.go Outdated

abhaygoudannavar force-pushed the one/metrics-file branch from a636578 to dcef7e4 Compare May 20, 2026 18:05

abhaygoudannavar force-pushed the one/metrics-file branch from dcef7e4 to 8238532 Compare May 24, 2026 21:07

abhaygoudannavar force-pushed the one/metrics-file branch from 8238532 to fae8707 Compare June 23, 2026 06:06

abhaygoudannavar requested a review from cmainas June 23, 2026 06:06

abhaygoudannavar force-pushed the one/metrics-file branch from fae8707 to 3fd847e Compare June 23, 2026 06:09

cmainas added the ok-to-test label Jun 23, 2026

cmainas reviewed Jun 23, 2026

View reviewed changes

abhaygoudannavar force-pushed the one/metrics-file branch from 3fd847e to 49d40bd Compare June 23, 2026 13:03

cmainas added the takeover label Jun 23, 2026

urunc-bot Bot changed the base branch from main to main-pr596 June 23, 2026 15:49

cmainas approved these changes Jun 23, 2026

View reviewed changes

cmainas merged commit 598a4c2 into urunc-dev:main-pr596 Jun 23, 2026
49 of 52 checks passed

cmainas mentioned this pull request Jun 23, 2026

fix(metrics): NewZerologMetrics returns nil on file open failure causing panic #595

Closed

Chennamma-Hotkar mentioned this pull request Jun 26, 2026

docs(timestamps): fix outdated enabling timestamping section #787

Open

4 tasks

abhaygoudannavar deleted the one/metrics-file branch July 1, 2026 10:59

Uh oh!

Conversation

abhaygoudannavar commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

How was this tested?

LLM usage

Checklist

Uh oh!

netlify Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for urunc canceled.

Uh oh!

cmainas commented May 5, 2026

Uh oh!

abhaygoudannavar commented May 6, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhaygoudannavar commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhaygoudannavar commented May 15, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhaygoudannavar commented May 20, 2026

Uh oh!

cmainas commented May 21, 2026

Uh oh!

abhaygoudannavar commented May 24, 2026

Uh oh!

cmainas commented Jun 22, 2026

Uh oh!

abhaygoudannavar commented Jun 23, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

cmainas Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

cmainas Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

abhaygoudannavar commented Jun 23, 2026

Uh oh!

cmainas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

abhaygoudannavar commented May 1, 2026 •

edited

Loading

netlify Bot commented May 1, 2026 •

edited

Loading

abhaygoudannavar commented May 10, 2026 •

edited

Loading