From 17f6a251dcc05dca3331f1ffea80a2af2e3e09fa Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Mon, 27 Jul 2026 10:49:50 -0700 Subject: [PATCH] catch more specific errors --- cmd/dbc/add.go | 9 +- cmd/dbc/helpers.go | 28 ++++++ cmd/dbc/install.go | 5 +- cmd/dbc/lock_error_test.go | 161 ++++++++++++++++++++++++++++++ cmd/dbc/remove.go | 5 +- cmd/dbc/sync.go | 5 +- cmd/dbc/uninstall.go | 5 +- internal/fslock/fslock.go | 7 +- internal/fslock/fslock_test.go | 34 +++++++ internal/fslock/fslock_unix.go | 6 +- internal/fslock/fslock_windows.go | 3 +- 11 files changed, 247 insertions(+), 21 deletions(-) create mode 100644 cmd/dbc/lock_error_test.go diff --git a/cmd/dbc/add.go b/cmd/dbc/add.go index a0e04cd9..eaeb606c 100644 --- a/cmd/dbc/add.go +++ b/cmd/dbc/add.go @@ -27,7 +27,6 @@ import ( "charm.land/lipgloss/v2" "github.com/Masterminds/semver/v3" "github.com/columnar-tech/dbc/config" - "github.com/columnar-tech/dbc/internal/fslock" "github.com/columnar-tech/dbc/internal/jsonschema" "github.com/pelletier/go-toml/v2" ) @@ -136,9 +135,9 @@ func (m addModel) Init() tea.Cmd { // read, a concurrent writer using os.Create could expose partial // contents to this decode path. lockPath := filepath.Join(filepath.Dir(p), ".dbc.project.lock") - readLock, err := fslock.Acquire(lockPath, 10*time.Second) + readLock, err := acquireLock(lockPath, 10*time.Second) if err != nil { - return fmt.Errorf("another dbc operation is in progress: %w", err) + return err } f, err := os.Open(p) if err != nil { @@ -241,9 +240,9 @@ func (m addModel) Init() tea.Cmd { // Re-read the file under the lock so a concurrent // `dbc add`/`dbc remove` that landed while we were doing the // registry lookup above doesn't get clobbered. - lock, err := fslock.Acquire(lockPath, 10*time.Second) + lock, err := acquireLock(lockPath, 10*time.Second) if err != nil { - return fmt.Errorf("another dbc operation is in progress: %w", err) + return err } defer lock.Release() diff --git a/cmd/dbc/helpers.go b/cmd/dbc/helpers.go index 594a0089..70ee08bb 100644 --- a/cmd/dbc/helpers.go +++ b/cmd/dbc/helpers.go @@ -18,10 +18,38 @@ import ( "errors" "fmt" "os" + "path/filepath" + "runtime" + "time" + "github.com/columnar-tech/dbc/internal/fslock" "github.com/pelletier/go-toml/v2" ) +// elevationHint names the privilege-escalation mechanism for the host platform. +func elevationHint() string { + if runtime.GOOS == "windows" { + return "re-running from an Administrator terminal" + } + return "re-running with sudo" +} + +func acquireLock(lockPath string, timeout time.Duration) (fslock.Lock, error) { + lock, err := fslock.Acquire(lockPath, timeout) + if err == nil { + return lock, nil + } + if errors.Is(err, fslock.ErrLockContended) { + return fslock.Lock{}, fmt.Errorf("another dbc operation is in progress: %w", err) + } + if errors.Is(err, os.ErrPermission) { + return fslock.Lock{}, fmt.Errorf( + "cannot write to %s: permission denied.\nThis command requires elevated privileges; try %s.", + filepath.Dir(lockPath), elevationHint()) + } + return fslock.Lock{}, fmt.Errorf("could not acquire lock in %s: %w", filepath.Dir(lockPath), err) +} + func wrapWithRegistryContext(err, registryErr error) error { if registryErr != nil { return fmt.Errorf("%w\n\nNote: Some driver registries were unavailable:\n%s", err, registryErr.Error()) diff --git a/cmd/dbc/install.go b/cmd/dbc/install.go index 33d470d7..be06fd35 100644 --- a/cmd/dbc/install.go +++ b/cmd/dbc/install.go @@ -32,7 +32,6 @@ import ( "github.com/Masterminds/semver/v3" "github.com/columnar-tech/dbc" "github.com/columnar-tech/dbc/config" - "github.com/columnar-tech/dbc/internal/fslock" "github.com/columnar-tech/dbc/internal/jsonschema" ) @@ -274,9 +273,9 @@ func (m progressiveInstallModel) Init() tea.Cmd { lockDir = parent } lockPath := filepath.Join(lockDir, ".dbc.install.lock") - lock, err := fslock.Acquire(lockPath, 10*time.Second) + lock, err := acquireLock(lockPath, 10*time.Second) if err != nil { - return fmt.Errorf("another dbc operation is in progress: %w", err) + return err } defer lock.Release() diff --git a/cmd/dbc/lock_error_test.go b/cmd/dbc/lock_error_test.go new file mode 100644 index 00000000..fad5551b --- /dev/null +++ b/cmd/dbc/lock_error_test.go @@ -0,0 +1,161 @@ +// Copyright 2026 Columnar Technologies Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "errors" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/columnar-tech/dbc/internal/fslock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// makeLockPathUnwritable seeds a read-only file at the lock path so that +// opening it with O_RDWR fails with a permission error. This reproduces the +// open() failure a user hits when the install directory requires elevation +// (e.g. C:\Program Files\ADBC\Drivers for --level system) without the test +// needing to be elevated or to manipulate ACLs. +// +// Returns false if the platform/filesystem/user does not enforce the +// restriction, so callers can skip rather than assert on a non-reproduction. +func makeLockPathUnwritable(t *testing.T, lockPath string) bool { + t.Helper() + + if os.Geteuid() == 0 { + t.Log("running as root/elevated: mode bits do not deny access") + return false + } + require.NoError(t, os.WriteFile(lockPath, nil, 0o400)) + require.NoError(t, os.Chmod(lockPath, 0o400)) + t.Cleanup(func() { _ = os.Chmod(lockPath, 0o600) }) + + // Confirm the restriction actually bites before relying on it. + f, err := os.OpenFile(lockPath, os.O_RDWR, 0o600) + if err == nil { + f.Close() + t.Log("filesystem does not enforce the read-only bit for this user") + return false + } + if !errors.Is(err, os.ErrPermission) { + t.Logf("expected a permission error, got: %v", err) + return false + } + return true +} + +// TestAcquireLockPermissionErrorIsNotReportedAsContention is the regression +// test for the reported bug: +// +// PS C:\Users\Bryce> dbc uninstall --level system foo +// Error: another dbc operation is in progress: fslock: open +// C:\Program Files\ADBC\Drivers\.dbc.install.lock: ... Access is denied. +// +// No other dbc process was running. The install directory required +// Administrator rights, so creating the lock file failed with +// ERROR_ACCESS_DENIED — and every lock call site blanket-wrapped *any* +// Acquire failure as "another dbc operation is in progress". That sent the +// user hunting for a phantom concurrent process instead of telling them to +// elevate. +// +// Before the fix this test fails on the first assertion: the message contains +// "another dbc operation is in progress". +func TestAcquireLockPermissionErrorIsNotReportedAsContention(t *testing.T) { + dir := t.TempDir() + lockPath := filepath.Join(dir, ".dbc.install.lock") + if !makeLockPathUnwritable(t, lockPath) { + t.Skip("cannot make lock path unwritable on this platform/user") + } + + _, err := acquireLock(lockPath, 100*time.Millisecond) + require.Error(t, err, "acquiring a lock in an unwritable location must fail") + + assert.NotContains(t, err.Error(), "another dbc operation is in progress", + "a permission failure must NOT be reported as a concurrent dbc operation; "+ + "nothing was holding the lock") + + // The message must instead point at the real cause and the real remedy. + assert.Contains(t, err.Error(), "permission denied", + "error should name the actual cause") + assert.Contains(t, err.Error(), dir, + "error should name the directory that could not be written") + assert.Contains(t, err.Error(), "elevated privileges", + "error should explain that elevation is required") + + if runtime.GOOS == "windows" { + assert.Contains(t, err.Error(), "Administrator", + "on Windows the remedy is an Administrator terminal, not sudo") + assert.NotContains(t, err.Error(), "sudo", + "must not suggest sudo on Windows") + } else { + assert.Contains(t, err.Error(), "sudo", + "on Unix the remedy is sudo") + } +} + +// TestAcquireLockGenuineContentionStillReported guards the other side of the +// fix: a real concurrent holder must still produce the "another dbc operation +// is in progress" message. A fix that simply dropped that message would make +// the test above pass while destroying the diagnostic it exists to provide. +func TestAcquireLockGenuineContentionStillReported(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), ".dbc.install.lock") + + held, err := fslock.Acquire(lockPath, 5*time.Second) + require.NoError(t, err) + defer held.Release() //nolint:errcheck + + _, err = acquireLock(lockPath, 100*time.Millisecond) + require.Error(t, err, "lock is held, so acquisition must fail") + assert.Contains(t, err.Error(), "another dbc operation is in progress", + "genuine contention must still be reported as a concurrent operation") +} + +// TestUninstallReportsElevationNotContention drives the actual `dbc uninstall` +// command path — the exact invocation from the bug report — rather than just +// the helper, so a regression at the call site is caught even if the helper +// stays correct. install/add/remove/sync share the same helper. +func TestUninstallReportsElevationNotContention(t *testing.T) { + dir := t.TempDir() + t.Setenv("ADBC_DRIVER_PATH", dir) + + lockPath := filepath.Join(dir, ".dbc.install.lock") + if !makeLockPathUnwritable(t, lockPath) { + t.Skip("cannot make lock path unwritable on this platform/user") + } + + m := UninstallCmd{Driver: "foo"}.GetModelCustom(testBaseModel()) + init, ok := m.(interface{ Init() tea.Cmd }) + require.True(t, ok) + + cmd := init.Init() + require.NotNil(t, cmd) + msg := cmd() + + err, isErr := msg.(error) + require.True(t, isErr, "uninstall must surface an error, got %T: %v", msg, msg) + + assert.NotContains(t, err.Error(), "another dbc operation is in progress", + "uninstall must not blame a phantom concurrent dbc process for a "+ + "permission failure") + assert.True(t, + strings.Contains(err.Error(), "elevated privileges"), + "uninstall should tell the user to elevate; got: %v", err) +} diff --git a/cmd/dbc/remove.go b/cmd/dbc/remove.go index 91329de3..dfa306c4 100644 --- a/cmd/dbc/remove.go +++ b/cmd/dbc/remove.go @@ -23,7 +23,6 @@ import ( "time" tea "charm.land/bubbletea/v2" - "github.com/columnar-tech/dbc/internal/fslock" "github.com/columnar-tech/dbc/internal/jsonschema" "github.com/pelletier/go-toml/v2" ) @@ -77,9 +76,9 @@ func (m removeModel) Init() tea.Cmd { } lockPath := filepath.Join(filepath.Dir(p), ".dbc.project.lock") - lock, err := fslock.Acquire(lockPath, 10*time.Second) + lock, err := acquireLock(lockPath, 10*time.Second) if err != nil { - return fmt.Errorf("another dbc operation is in progress: %w", err) + return err } defer lock.Release() diff --git a/cmd/dbc/sync.go b/cmd/dbc/sync.go index 6907943c..ab8337bc 100644 --- a/cmd/dbc/sync.go +++ b/cmd/dbc/sync.go @@ -31,7 +31,6 @@ import ( "charm.land/lipgloss/v2" "github.com/columnar-tech/dbc" "github.com/columnar-tech/dbc/config" - "github.com/columnar-tech/dbc/internal/fslock" "github.com/columnar-tech/dbc/internal/jsonschema" "github.com/pelletier/go-toml/v2" ) @@ -157,9 +156,9 @@ func (s syncModel) Init() tea.Cmd { } lockPath := filepath.Join(filepath.Dir(p), ".dbc.project.lock") - lock, err := fslock.Acquire(lockPath, 10*time.Second) + lock, err := acquireLock(lockPath, 10*time.Second) if err != nil { - return fmt.Errorf("another dbc operation is in progress: %w", err) + return err } defer lock.Release() diff --git a/cmd/dbc/uninstall.go b/cmd/dbc/uninstall.go index 631e7863..5e87e0cf 100644 --- a/cmd/dbc/uninstall.go +++ b/cmd/dbc/uninstall.go @@ -23,7 +23,6 @@ import ( tea "charm.land/bubbletea/v2" "github.com/columnar-tech/dbc/config" - "github.com/columnar-tech/dbc/internal/fslock" "github.com/columnar-tech/dbc/internal/jsonschema" ) @@ -80,9 +79,9 @@ func (m uninstallModel) Init() tea.Cmd { lockDir = parent } lockPath := filepath.Join(lockDir, ".dbc.install.lock") - lock, err := fslock.Acquire(lockPath, 10*time.Second) + lock, err := acquireLock(lockPath, 10*time.Second) if err != nil { - return fmt.Errorf("another dbc operation is in progress: %w", err) + return err } defer lock.Release() return m.startUninstall() diff --git a/internal/fslock/fslock.go b/internal/fslock/fslock.go index cd6f330a..4684a6bb 100644 --- a/internal/fslock/fslock.go +++ b/internal/fslock/fslock.go @@ -16,10 +16,15 @@ // access to shared resources across processes. package fslock -import "os" +import ( + "errors" + "os" +) // Lock represents an acquired advisory file lock. type Lock struct { f *os.File path string } + +var ErrLockContended = errors.New("lock is held by another process") diff --git a/internal/fslock/fslock_test.go b/internal/fslock/fslock_test.go index 629d229b..52d52207 100644 --- a/internal/fslock/fslock_test.go +++ b/internal/fslock/fslock_test.go @@ -15,6 +15,7 @@ package fslock_test import ( + "errors" "os" "path/filepath" "sync" @@ -116,4 +117,37 @@ func TestAcquireTimeout(t *testing.T) { if err == nil { t.Fatal("expected timeout error, got nil") } + // Verify the error type is ErrLockContended + if !errors.Is(err, fslock.ErrLockContended) { + t.Fatalf("timeout error must wrap ErrLockContended, got: %v", err) + } +} + +func TestAcquireUnwritableDirIsNotContention(t *testing.T) { + // Simulate a lock failure that's due to permissions instead of actual lock + // contention by creating a read-only file and later trying to lock on it + path := filepath.Join(t.TempDir(), "test.lock") + if err := os.WriteFile(path, nil, 0o400); err != nil { + t.Fatalf("seed lock file: %v", err) + } + if err := os.Chmod(path, 0o400); err != nil { + t.Fatalf("chmod: %v", err) + } + t.Cleanup(func() { os.Chmod(path, 0o600) }) + + // Skip if running with elevated privs + if os.Geteuid() == 0 { + t.Skip("running as root/elevated: mode bits do not deny access") + } + + _, err := fslock.Acquire(path, 100*time.Millisecond) + if err == nil { + t.Skip("filesystem does not enforce the read-only bit for this user") + } + if !errors.Is(err, os.ErrPermission) { + t.Skipf("expected a permission error from open, got: %v", err) + } + if errors.Is(err, fslock.ErrLockContended) { + t.Fatalf("permission failure must not be classified as contention: %v", err) + } } diff --git a/internal/fslock/fslock_unix.go b/internal/fslock/fslock_unix.go index 51246c68..5c089841 100644 --- a/internal/fslock/fslock_unix.go +++ b/internal/fslock/fslock_unix.go @@ -46,7 +46,8 @@ func Acquire(path string, timeout time.Duration) (Lock, error) { // Reopen and try again within the remaining budget. continue } - return Lock{}, fmt.Errorf("fslock: could not acquire lock on %s within %s: %w", path, timeout, err) + return Lock{}, fmt.Errorf("fslock: could not acquire lock on %s within %s (%v): %w", + path, timeout, err, ErrLockContended) } return Lock{}, err } @@ -73,7 +74,8 @@ func lockFile(f *os.File, path string, deadline time.Time) (Lock, error) { return Lock{f: f, path: path}, nil } if time.Now().After(deadline) { - return Lock{}, fmt.Errorf("fslock: could not acquire lock on %s: %w", path, err) + return Lock{}, fmt.Errorf("fslock: could not acquire lock on %s (%v): %w", + path, err, ErrLockContended) } time.Sleep(50 * time.Millisecond) } diff --git a/internal/fslock/fslock_windows.go b/internal/fslock/fslock_windows.go index 123fd7e2..d8d504dd 100644 --- a/internal/fslock/fslock_windows.go +++ b/internal/fslock/fslock_windows.go @@ -44,7 +44,8 @@ func Acquire(path string, timeout time.Duration) (Lock, error) { } if time.Now().After(deadline) { f.Close() - return Lock{}, fmt.Errorf("fslock: could not acquire lock on %s within %s: %w", path, timeout, err) + return Lock{}, fmt.Errorf("fslock: could not acquire lock on %s within %s (%v): %w", + path, timeout, err, ErrLockContended) } time.Sleep(50 * time.Millisecond) }