-
Notifications
You must be signed in to change notification settings - Fork 3
fix: handle corrupted config files gracefully instead of crashing #120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Use default values when config options are missing instead of throwing - Regenerate config automatically when options are missing or malformed - Add TRUNCATE_EXISTING to prevent leftover data from corrupting config - Remove System.exit(1) on config parse errors to allow recovery Fixes imthosea#115
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves config file handling by gracefully recovering from corrupted or incomplete config files instead of crashing the application. The key changes include removing System.exit(1) calls on config errors, automatically detecting missing options, and regenerating the config file when necessary.
Key Changes:
- Added
hasMissingOptionsflag to track when config options are missing - Changed exception handling to log warnings and use default values instead of throwing exceptions or exiting
- Added
TRUNCATE_EXISTINGflag to prevent partial writes from corrupting config files
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ConfigLoadContext.java | Added hasMissingOptions tracking, removed System.exit(1) on config load failure, changed missing option handling from throwing exceptions to logging warnings |
| Config.java | Updated config regeneration logic to trigger when hasMissingOptions is true, added TRUNCATE_EXISTING flag to file write operation |
Note: There is a critical issue with the implementation - while missing config options are now handled gracefully, malformed config values (invalid booleans, invalid numbers, negative numbers) still throw IllegalStateException in the boolOrDefault() and intOrDefault() methods. This means corrupted config files with malformed values will still crash instead of gracefully degrading as intended by the PR.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String str = properties.getProperty(name); | ||
| if(str == null) { | ||
| throw new IllegalStateException("Config option " + name + " not found"); | ||
| LOGGER.warn("Config option {} not found - using default value. Config will be regenerated.", name); | ||
| hasMissingOptions = true; | ||
| } | ||
| return str; |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The str() method now returns null when a config option is not found and sets hasMissingOptions = true. However, the callers of this method (boolOrDefault at line 42 and intOrDefault at line 55) still throw IllegalStateException for malformed values instead of gracefully falling back to defaults. This creates an inconsistency where missing options are handled gracefully, but malformed options still cause exceptions. For complete graceful degradation as described in the PR, those methods should also handle malformed values by logging warnings, setting hasMissingOptions = true, and returning the default value.
| public boolean hasMissingOptions = false; | ||
|
|
||
| public ConfigLoadContext() { |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hasMissingOptions field should be initialized with an explicit default value even though false is the default for boolean fields in Java. This makes the intent clearer and follows defensive programming practices, especially since this field is being used to determine whether to regenerate the config file.
| public boolean hasMissingOptions = false; | |
| public ConfigLoadContext() { | |
| public boolean hasMissingOptions; | |
| public ConfigLoadContext() { | |
| this.hasMissingOptions = false; |
| LOGGER.error("Failed to load config from " + FILE + ". " + | ||
| "If you need to, you can delete the file to generate a new one.", e); | ||
| System.exit(1); | ||
| LOGGER.error("Failed to load config from " + FILE + ". Config will be regenerated with default values.", e); |
Copilot
AI
Dec 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message states "Config will be regenerated with default values" but this is not entirely accurate. The config regeneration logic is in Config.java and depends on checking hasMissingOptions. However, when loadProp() returns null due to an exception, the properties field is set to null, and str() will return null for all config reads. While the config will eventually be regenerated (due to fromExistingFile() returning false), the message could be more precise by stating "Attempting to use default values and regenerate config" to better reflect the actual behavior.
| LOGGER.error("Failed to load config from " + FILE + ". Config will be regenerated with default values.", e); | |
| LOGGER.error("Failed to load config from " + FILE + ". Attempting to use default values and regenerate config.", e); |
Fixes #115