tapcfg: validate SQLite temp dir at startup and add opt out flag #1886
Conversation
Summary of ChangesHello @ffranr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable startup check to validate the writability of SQLite's temporary directory, preventing opaque runtime errors. The implementation is sound and includes a useful opt-out flag. My review focuses on removing a piece of leftover code, improving an error message for better diagnostics, and refactoring a function to enhance readability and maintainability.
| // SQLite will use the first available temp directory; we mirror that | ||
| // behavior by trying environment variables and standard fallback | ||
| // directories in order. | ||
| var errs []string | ||
| if dir := os.Getenv("SQLITE_TMPDIR"); dir != "" { | ||
| if err := ensureDirWritable(dir); err != nil { | ||
| err = fmt.Errorf("(env=SQLITE_TMPDIR) %w", err) | ||
| errs = append(errs, err.Error()) | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| if dir := os.Getenv("TMPDIR"); dir != "" { | ||
| if err := ensureDirWritable(dir); err != nil { | ||
| err = fmt.Errorf("(env=TMPDIR) %w", err) | ||
| errs = append(errs, err.Error()) | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| for _, dir := range []string{"/var/tmp", "/usr/tmp", "/tmp"} { | ||
| if err := ensureDirWritable(dir); err != nil { | ||
| errs = append(errs, err.Error()) | ||
| continue | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| return fmt.Errorf("no writable temp directory found; attempts=%s", | ||
| strings.Join(errs, "; ")) |
There was a problem hiding this comment.
The logic for checking different temporary directories is a bit repetitive. You can refactor this to be more DRY by using a slice of structs to hold the directories and their sources, and then iterating over them. This also makes it easier to add more directories to check in the future and ensures consistent error reporting.
// SQLite will use the first available temp directory; we mirror that
// behavior by trying environment variables and standard fallback
// directories in order.
var errs []string
type dirSource struct {
path string
source string
}
sources := []dirSource{
{path: os.Getenv("SQLITE_TMPDIR"), source: "env=SQLITE_TMPDIR"},
{path: os.Getenv("TMPDIR"), source: "env=TMPDIR"},
{path: "/var/tmp", source: "fallback=/var/tmp"},
{path: "/usr/tmp", source: "fallback=/usr/tmp"},
{path: "/tmp", source: "fallback=/tmp"},
}
for _, s := range sources {
if s.path == "" {
continue
}
if err := ensureDirWritable(s.path); err != nil {
errs = append(errs, fmt.Errorf("(%s) %w", s.source, err).Error())
} else {
return nil
}
}
return fmt.Errorf("no writable temp directory found; attempts=%s",
strings.Join(errs, "; "))
Pull Request Test Coverage Report for Build 19701408303Details
💛 - Coveralls |
jtobin
left a comment
jtobin
left a comment
There was a problem hiding this comment.
Nice one. There's that one conditional block artifact that should be removed, but otherwise LGTM. 👍
Adds logic to validate writable temp directories for SQLite operations during initialization. Handles environment variables like `SQLITE_TMPDIR` and `TMPDIR`, and falls back to standard temp directories.
Add a configuration flag to disable the SQLite temporary directory writability check performed at startup.
ffranr
force-pushed
the
wip/check-tmp-env-writable
branch
from
786fd89 to
f16b0cd
Compare
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
This PR addresses #1869, where a user hit
SQLITE_IOERR_GETTEMPPATH (6410)duringtapdoperation. The goal is to fail fast at startup if SQLite cannot access a writable temporary directory, instead of surfacing a late, opaque I/O error.Summary of changes
Validate SQLite temp directory at startup
tapdinitialization, determine candidate temp directories using:SQLITE_TMPDIRandTMPDIR/tmpor similar paths are not writable, which would later causeSQLITE_IOERR_GETTEMPPATH.Add config flag to skip the check
Introduce a new tapcfg option to skip the SQLite temp directory writability check at startup.
This gives operators an escape hatch in environments where:
Rationale
SQLITE_IOERR_GETTEMPPATH (6410)typically indicates that SQLite’s VFS cannot determine or use a suitable directory for temporary files.