Skip to content

Fix nil derefs in job run logging and tmpdir leak in terraform import#5141

Open
arsenyinfo wants to merge 2 commits intomainfrom
fix/job-run-nil-derefs-and-tmpdir-leak
Open

Fix nil derefs in job run logging and tmpdir leak in terraform import#5141
arsenyinfo wants to merge 2 commits intomainfrom
fix/job-run-nil-derefs-and-tmpdir-leak

Conversation

@arsenyinfo
Copy link
Copy Markdown
Contributor

Changes

Three small bug fixes surfaced by a code review pass:

  • bundle/run/job.go — guard task.State in isFailed/isSuccess and run.State in logFailedTasks against nil. The same file's onProgress already does this, so the inconsistency is a latent panic if the Jobs API ever returns a run/task with no state.
  • bundle/run/job.go — in the --no-wait path, check the error from GetRun before dereferencing details.RunPageUrl. Previously a failed GetRun would panic on the next line.
  • bundle/deploy/terraform/import.go — move defer os.RemoveAll(tmpDir) to immediately after MkdirTemp succeeds. It was placed after two early-return paths (tf.Import / tf.Plan failures), so the temp dir leaked on those failures.

Why

All three are unambiguous defects: two nil-pointer panic sites and one resource leak. No behavior change on the happy path.

Tests

  • Existing tests in bundle/run and bundle/deploy/terraform cover the happy paths and continue to pass behaviorally; the defended branches were previously panic / leak paths with no coverage.
  • Local go build is currently blocked by a flaky toolchain proxy (503 on proxy.golang.org); relying on CI to validate.

@arsenyinfo arsenyinfo requested a review from pietern April 30, 2026 09:46
Copy link
Copy Markdown
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

This doesn't seem to be user facing, so I removed the changelog entry.

@pietern pietern temporarily deployed to test-trigger-is May 8, 2026 13:02 — with GitHub Actions Inactive
@pietern pietern temporarily deployed to test-trigger-is May 8, 2026 13:02 — with GitHub Actions Inactive
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.

2 participants