Skip to content

fix: return error when lock acquisition fails instead of nil - #2635

Open
raman1236 wants to merge 1 commit into
diggerhq:developfrom
raman1236:fix/lock-failure-returns-error
Open

raman1236 wants to merge 1 commit into
diggerhq:developfrom
raman1236:fix/lock-failure-returns-error

Conversation

@raman1236

Copy link
Copy Markdown

Problem

When LockingExecutorWrapper.Plan(), Apply(), or Destroy() fail to acquire a lock (locked == false), they return a nil error. This causes the caller run() function in cli/pkg/digger/digger.go to silently succeed — the switch case falls through to the default return &execution.DiggerExecutorResult{}, "", nil, reporting success to CI even though no actual plan/apply/destroy was performed.

This means CI builds pass even when a lock could not be acquired, giving users a false sense that their infrastructure changes were processed.

Root Cause

In libs/execution/execution.go:

  • Plan() returned nil, false, false, plan, "", nil when locked == false
  • Apply() returned nil, false, "couldn't lock ", nil when locked == false
  • Destroy() returned false, nil when locked == false

All three methods returned nil error on lock failure.

Fix

Return a descriptive fmt.Errorf("failed to acquire lock for project") error from all three methods when locked == false, so callers can detect and properly report the lock failure.

Tests

Added 4 new unit tests:

  • TestLockFailurePlanReturnsError — verifies Plan returns error on lock failure
  • TestLockFailureApplyReturnsError — verifies Apply returns error on lock failure
  • TestLockFailureDestroyReturnsError — verifies Destroy returns error on lock failure
  • TestLockSuccessDelegatesToExecutor — verifies all three methods delegate correctly when lock succeeds

All tests pass.

Fixes #1366

When LockingExecutorWrapper.Plan(), Apply(), or Destroy() fail to
acquire a lock (locked=false), they previously returned nil error.
This caused the caller run() to silently succeed, making CI builds
pass even though no actual plan/apply/destroy was performed.

Return a descriptive error so callers can detect and report the
lock failure properly.

Fixes diggerhq#1366
@raman1236

Copy link
Copy Markdown
Author

Friendly ping — this PR fixes a bug where lock acquisition failures return nil instead of an error, which can lead to silent data races. Would appreciate a review when convenient. Thanks!

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.

Build succeeds despite lock acquisition failing

2 participants