Skip to content

Resolve the default config path in the reset command (fixes #1404) - #1423

Open
youdie006 wants to merge 3 commits into
benbjohnson:mainfrom
youdie006:fix/reset-default-config-path
Open

Resolve the default config path in the reset command (fixes #1404)#1423
youdie006 wants to merge 3 commits into
benbjohnson:mainfrom
youdie006:fix/reset-default-config-path

Conversation

@youdie006

Copy link
Copy Markdown

Description

reset only loaded a Litestream config when -config was passed explicitly (cmd/litestream/reset.go:48), so it ignored LITESTREAM_CONFIG and the default config path. Every other subcommand resolves DefaultConfigPath() when -config is empty (databases.go, status.go, replicate.go, restore.go, ltx.go), and DefaultConfigPath() honors LITESTREAM_CONFIG before falling back to /etc/litestream.yml. Because reset skipped that step, dbConfig stayed nil and it operated on the default meta path via litestream.NewDB(dbPath) instead of the meta-path configured for the database (NewDBFromConfig).

This resolves the default config path when -config is not given, matching the other subcommands, but tolerates a missing config file (errors.Is(err, ErrConfigFileNotFound)) so reset still works on a database that is not listed in any config.

Scope: changes only the config-path resolution in reset. Behavior with no config file present is unchanged, and the other subcommands are untouched.

Motivation and Context

reset even advertises the default config path in its own help text, but ignored it — LITESTREAM_CONFIG=cfg litestream reset <db> never saw the config.

Fixes #1404

How Has This Been Tested?

Added TestResetCommand_RunWithConfigEnv (cmd/litestream/reset_test.go): it places the Litestream state at a non-default meta-path discoverable only through a config file, sets LITESTREAM_CONFIG, runs reset without -config, and asserts the state at the configured meta path is removed.

go test ./cmd/litestream/ -run TestResetCommand -v

Verified red/green: the new test fails on main (config ignored → default meta path used) and passes with this change. go build ./..., go vet ./cmd/litestream/, and gofmt are clean (go 1.25).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (would cause existing functionality to not work as expected)

Checklist

  • My code follows the code style of this project (go fmt, go vet)
  • I have tested my changes (go test ./cmd/litestream/)
  • I have updated the documentation accordingly (if needed)

AI disclosure: prepared with AI assistance (Claude Code); the bug was traced against the code, the fix derived, and the red/green test verified before submitting.

reset only loaded a config when -config was passed explicitly, so
LITESTREAM_CONFIG and the default config path were ignored (benbjohnson#1404) -- unlike
the other subcommands, which resolve DefaultConfigPath() when -config is empty.
Without the config, reset operated on the default meta path instead of the one
configured for the database.

Resolve the default config path when -config is not given (matching the other
subcommands) but tolerate a missing config file, since reset can run on a
database that is not listed in any config.

Adds a regression test that fails without the fix.

@corylanou corylanou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing #1404. This PR correctly makes reset honor LITESTREAM_CONFIG and the implicit default config path.

There is one behavior change that needs adjustment: ErrConfigFileNotFound is currently ignored regardless of how the path was selected. As a result, an explicitly missing -config file, which previously returned an error, now silently falls back to the default metadata path.

Please preserve an error for an explicitly supplied -config path. A missing path supplied through LITESTREAM_CONFIG should also return an error once we begin honoring that variable. Only a missing implicit platform default should be tolerated, preserving standalone litestream reset behavior.

Please add tests covering these cases:

  • Missing explicit -config returns an error.
  • Missing LITESTREAM_CONFIG path returns an error.
  • Missing implicit platform default retains standalone reset behavior.

…ault

An explicitly supplied -config path, or one from LITESTREAM_CONFIG, that does
not exist now returns ErrConfigFileNotFound instead of falling back to the
default metadata path. Only a missing implicit platform default is tolerated,
preserving standalone litestream reset <db>.

resolveConfigPath reports whether the path was explicitly selected, and
DefaultConfigPath delegates to it so LITESTREAM_CONFIG is still read in one
place and the other subcommands are unaffected.
@youdie006

Copy link
Copy Markdown
Author

Thanks for the review — you were right, and the consequence was worse than a silent fallback. I probed it before and after:

before:  run_err=<nil>                                   ltx_exists=false
after:   run_err=cannot read config: config file not found: .../missing.yml   ltx_exists=true

With a mistyped -config path the command reported success and removed the local LTX state, because by the time ErrConfigFileNotFound was tested, *configPath had already been overwritten with DefaultConfigPath() and all provenance was gone.

What changed

Config resolution now distinguishes how the path was selected:

path source missing file
explicit -config error
LITESTREAM_CONFIG error
implicit platform default tolerated

resolveConfigPath in cmd/litestream/main.go returns the path along with whether it was explicitly chosen, and DefaultConfigPath delegates to it, so LITESTREAM_CONFIG is still read in exactly one place. reset.go becomes a one-line predicate change.

On your "use an existing helper if there is one" — there wasn't. Every other subcommand (databases.go:28-34, status.go:30-36, replicate.go:84-89, restore.go:107-110, ltx.go:48-56) uses the same two-step if *configPath == "" { *configPath = DefaultConfigPath() } idiom and then hard-fails, so none of them needed the distinction. I added the helper rather than re-reading LITESTREAM_CONFIG inside reset.go, which would have let the two definitions drift. That does put 14 lines in main.go, outside this PR's original two files — if you would rather keep the diff strictly inside reset.go, that is a two-line change and I will redo it.

The other five subcommands are unaffected: same branch order, same values, and their tests pass.

Tests

The three cases you listed:

  • TestResetCommand_RunWithMissingConfigFlag
  • TestResetCommand_RunWithMissingConfigEnv
  • TestResetCommand_RunWithMissingDefaultConfig

The first two assert ErrConfigFileNotFound and that the LTX file survives, so they also fail if the command errors after doing work. The third passes both before and after by design — it is the no-regression guard for standalone reset.

Verified red then green: with reset.go reverted and the tests kept, the two error cases fail with expected ErrConfigFileNotFound, got <nil> while the default-path case still passes; all six pass with the fix. t.Setenv throughout for env isolation.

go test over CI's package list is green, and go vet, gofmt, goimports -local github.com/benbjohnson/litestream and staticcheck are all clean. Not covered here: the Windows default path (main_windows.go:17) — DefaultConfigPath() semantics are unchanged so the risk is low, but this environment is Linux-only.

Disclosure: I use AI assistance in my work, and I review and verify everything before it goes out. The probe and the red-green runs above are my own.

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.

reset: ignores LITESTREAM_CONFIG and the default config path

2 participants