Crash at startup when local storage directory is not writable #1603
Conversation
Signed-off-by: Pranav-0440 <pranavghorpade61@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds fail-fast startup validation for ovsx.storage.local.directory so the server errors early when local storage is configured but not writable, avoiding later inconsistent runtime behavior (Issue #1158).
Changes:
- Add a
@PostConstructstartup check that ensures the configured local storage directory exists and is writable. - Throw a clear startup exception when the directory cannot be created or written to.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package org.eclipse.openvsx.storage; | ||
| import jakarta.annotation.PostConstruct; | ||
|
|
There was a problem hiding this comment.
There’s no blank line between the package declaration and the first import. Other classes in this package keep a blank line after package ...; (e.g., AwsStorageService.java:11-13), and this can trip formatting/checkstyle rules.
| // Verify directory is writable by creating a temp file | ||
| Path testFile = dir.resolve(".ovsx_write_test"); | ||
| Files.writeString(testFile, "test"); | ||
| Files.deleteIfExists(testFile); |
There was a problem hiding this comment.
The writability check uses a fixed filename (.ovsx_write_test) and Files.writeString(...), which will overwrite an existing file and can also follow a pre-existing symlink. This can cause false negatives (directory is writable but the file isn’t) and introduces a symlink/clobber risk. Prefer creating a new temp file in the directory (e.g., Files.createTempFile(dir, ...) / CREATE_NEW) and ensure it’s cleaned up in a finally block.
| // Verify directory is writable by creating a temp file | |
| Path testFile = dir.resolve(".ovsx_write_test"); | |
| Files.writeString(testFile, "test"); | |
| Files.deleteIfExists(testFile); | |
| // Verify directory is writable by creating and removing a temp file | |
| Path testFile = null; | |
| try { | |
| testFile = Files.createTempFile(dir, ".ovsx_write_test", null); | |
| Files.writeString(testFile, "test"); | |
| } finally { | |
| if (testFile != null) { | |
| Files.deleteIfExists(testFile); | |
| } | |
| } |
| } catch (Exception e) { | ||
| throw new IllegalStateException( | ||
| "Local storage directory is not writable: " + storageDirectory + | ||
| ". Please check permissions for 'ovsx.storage.local.directory'.", | ||
| e |
There was a problem hiding this comment.
Catching Exception and always reporting "directory is not writable" can be misleading for cases like an invalid path (InvalidPathException) or other configuration errors. Consider catching the expected I/O/security exceptions for the permissions check, and include the resolved absolute path in the message to make operator diagnosis easier.
There was a problem hiding this comment.
there are ways to test the permissions on the local storage directory instead of trying to create a file in it which is the preferred way to do that.
| @PostConstruct | ||
| public void validateStorageDirectory() { | ||
| if (!isEnabled()) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| Path dir = Path.of(storageDirectory).toAbsolutePath(); | ||
|
|
||
| // Ensure directory exists | ||
| Files.createDirectories(dir); | ||
|
|
||
| // Verify directory is writable by creating a temp file | ||
| Path testFile = dir.resolve(".ovsx_write_test"); | ||
| Files.writeString(testFile, "test"); | ||
| Files.deleteIfExists(testFile); | ||
|
|
||
| } catch (Exception e) { | ||
| throw new IllegalStateException( | ||
| "Local storage directory is not writable: " + storageDirectory + | ||
| ". Please check permissions for 'ovsx.storage.local.directory'.", | ||
| e | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This introduces new startup behavior (fail-fast validation) but there are no focused tests exercising the writable vs non-writable cases for local storage. Given the existing test coverage for other storage services, it would be good to add tests that assert the app context fails to start (or the validator throws) when the directory is not writable, and succeeds when it is.
| } catch (Exception e) { | ||
| throw new IllegalStateException( | ||
| "Local storage directory is not writable: " + storageDirectory + | ||
| ". Please check permissions for 'ovsx.storage.local.directory'.", | ||
| e |
There was a problem hiding this comment.
there are ways to test the permissions on the local storage directory instead of trying to create a file in it which is the preferred way to do that.
Signed-off-by: Pranav-0440 <pranavghorpade61@gmail.com>
|
Thanks @netomi for the feedback. That makes sense. |
Removed duplicate import of jakarta.annotation.PostConstruct.
|
Hi @netomi , |
2 participants
When ovsx.storage.local.directory is configured but not writable,
the application starts successfully and later fails at runtime
with inconsistent behavior.
This change adds a startup validation to ensure the local storage
directory exists and is writable. If not, the application fails fast
with a clear error message.
This prevents corrupted state and improves operator feedback.
fixes - #1158