Skip to content

fix: handle permission denied when reading PG_VERSION#196

Merged
Justin Pye (bernielomax) merged 1 commit into
mainfrom
devin/1777017313-fix-pgversion-permission-denied
Apr 30, 2026
Merged

fix: handle permission denied when reading PG_VERSION#196
Justin Pye (bernielomax) merged 1 commit into
mainfrom
devin/1777017313-fix-pgversion-permission-denied

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Fixes abctl local install failing with "permission denied" when reading PG_VERSION from the host filesystem.

Resolves https://github.com/airbytehq/oncall/issues/12079:

Related: airbytehq/airbyte#76971

Problem

The EnablePsql17() function in internal/service/manager.go calls pgdata.Version(), which uses os.ReadFile() to read PG_VERSION from the host filesystem. The pgdata directory is created by the PostgreSQL container with UID/GID 70 and drwx------ (0700) permissions. When the host user (e.g., UID 1000 on Ubuntu/WSL2) cannot traverse the directory, os.ReadFile returns fs.ErrPermission.

The error handling at manager.go:243 only accounted for fs.ErrNotExist but not fs.ErrPermission, so the permission-denied error propagated as an unrecoverable failure.

Fix

Added fs.ErrPermission handling alongside fs.ErrNotExist in EnablePsql17(). When we cannot read PG_VERSION due to permissions, it is safe to proceed with enabling Postgres 17 — the same behavior as when the file does not exist.

// Before:
if err != nil && !errors.Is(err, fs.ErrNotExist) {

// After:
if err != nil && !errors.Is(err, fs.ErrNotExist) && !errors.Is(err, fs.ErrPermission) {

Test Coverage

  • Added internal/pgdata/version_test.go with tests for: reading valid versions, non-existent directory (fs.ErrNotExist), permission denied (fs.ErrPermission), and older Postgres versions.
  • Added TestEnablePsql17 in internal/service/manager_test.go with subtests for: non-existent directory (returns true), permission denied (returns true), version 17 (returns true), and version 13 (returns false).
  • All tests pass locally.

Review & Testing Checklist for Human

  • Verify the fix logic: fs.ErrPermission should be treated identically to fs.ErrNotExist in the EnablePsql17() function
  • Reproduce on Ubuntu/WSL2 with Docker Desktop: run abctl local install, confirm it no longer fails with "permission denied"
  • Verify that existing Postgres 16 data directories (readable) still correctly return false from EnablePsql17()

Notes

Link to Devin session: https://app.devin.ai/sessions/537624ae41b54ae18f6b9d5e96b190dd

The EnablePsql17() function reads PG_VERSION from the host filesystem
via os.ReadFile(). The pgdata directory is created by the PostgreSQL
container with UID/GID 70 and drwx------ (0700) permissions. When the
host user cannot traverse the directory, os.ReadFile returns
fs.ErrPermission. The existing error handling only accounted for
fs.ErrNotExist, causing the permission-denied error to propagate as
an unrecoverable failure during 'abctl local install'.

Treat fs.ErrPermission the same as fs.ErrNotExist: proceed with
enabling Postgres 17, since being unable to read PG_VERSION is
equivalent to the file not existing for version detection purposes.

Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@bernielomax Justin Pye (bernielomax) marked this pull request as ready for review April 30, 2026 16:24
@bernielomax

Copy link
Copy Markdown
Contributor

While this looks good. I am a little confused by the tests. Why the (what appears to be) dup coverage in internal/pgdata when the fix was made internal/service?

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Good question! The internal/pgdata/version_test.go tests cover the pgdata.Client.Version() method itself — verifying that it correctly propagates fs.ErrNotExist and fs.ErrPermission through the error chain (via fmt.Errorf("%w", ...)). This is important because EnablePsql17() relies on errors.Is() to match those sentinel errors through the wrapping, so these tests confirm the contract that makes the fix work.

The TestEnablePsql17 tests in internal/service/manager_test.go then cover the integration — verifying that EnablePsql17() returns true (rather than an error) when it encounters permission denied, non-existent dirs, etc.

They're complementary rather than duplicative: the pgdata tests verify error propagation, the service tests verify error handling. That said, if you'd prefer I remove the pgdata tests to keep the scope tighter, happy to do so.


Devin session

@bernielomax Justin Pye (bernielomax) merged commit aa203f6 into main Apr 30, 2026
3 checks passed
@bernielomax Justin Pye (bernielomax) deleted the devin/1777017313-fix-pgversion-permission-denied branch April 30, 2026 16:31
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.

1 participant