feat(config): -logs-dir flag to relocate logs.db out of the config dir#4536
feat(config): -logs-dir flag to relocate logs.db out of the config dir#4536mickgvirtu wants to merge 1 commit into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Changeslogs.db Directory Override
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config_test.go (1)
19348-19369: ⚡ Quick winAdd one non-SQLite explicit
logs_storeprecedence subtest.This new test covers explicit SQLite precedence, but the stack behavior also includes explicit non-SQLite config ignoring
logsDir. Adding that case here would lock the full contract and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/lib/config_test.go` around lines 19348 - 19369, The existing test subtest only covers explicit SQLite logs_store configuration taking precedence over the logsDir override. Add a new subtest using t.Run after the current one that tests a non-SQLite explicit logs_store type (replace logstore.LogStoreTypeSQLite with an alternative type) to verify that non-SQLite explicit configurations also take precedence over the logsDir parameter. Use similar test structure and assertions as the SQLite case to ensure the explicit non-SQLite config is honored and the logsDir override is ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 995-1009: The current logic in the else if block that compares
sqliteConfig.Path with logsDBPath and updates the path always overrides stored
SQLite paths when logsDirOverridden is true, but this should only apply to
implicit default paths. To fix this, add an additional check before the
assignment sqliteConfig.Path = logsDBPath to determine whether the stored path
is an explicitly configured operator-defined path versus the implicit default
path. Only apply the override if the stored path matches the implicit default,
allowing explicit logs_store configuration to take precedence over the -logs-dir
override.
---
Nitpick comments:
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 19348-19369: The existing test subtest only covers explicit SQLite
logs_store configuration taking precedence over the logsDir override. Add a new
subtest using t.Run after the current one that tests a non-SQLite explicit
logs_store type (replace logstore.LogStoreTypeSQLite with an alternative type)
to verify that non-SQLite explicit configurations also take precedence over the
logsDir parameter. Use similar test structure and assertions as the SQLite case
to ensure the explicit non-SQLite config is honored and the logsDir override is
ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 80654b3f-3962-4767-9374-67576c09447c
📒 Files selected for processing (4)
transports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/main.gotransports/bifrost-http/server/server.go
logs.db is derived as a sibling of config.db (Join(configDirPath, "logs.db")), forcing the config and the large, churning request-log DB onto one directory. Add a -logs-dir flag (default from BIFROST_LOGS_DIR env, matching the -host/BIFROST_HOST precedence) threaded through Server.LogsDir -> LoadConfig. Applies to the default SQLite logs store; when the logs store is explicitly configured in config.json or is non-SQLite the override is ignored with a warning rather than silently. Adds a test for relocation and the empty-default case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mickgvirtu
force-pushed
the
pr-logs-dir-flag
branch
from
bbeec04 to
6df1ab4
Compare
|
Thanks for the review. Addressed in the latest push:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if logsDirOverridden && logStoreConfig != nil { | ||
| if logStoreConfig.Type != logstore.LogStoreTypeSQLite { | ||
| logger.Warn("logs dir override ignored: stored logs store is %s, not SQLite (-logs-dir / BIFROST_LOGS_DIR only relocates the SQLite logs.db)", logStoreConfig.Type) | ||
| } else if sqliteConfig, ok := logStoreConfig.Config.(*logstore.SQLiteConfig); !ok { | ||
| logger.Warn("logs dir override ignored: stored SQLite logs store has an unexpected config payload; cannot relocate logs.db") | ||
| } else if sqliteConfig.Path == "" || sqliteConfig.Path == defaultSQLiteLogsDBPath { | ||
| if sqliteConfig.Path != logsDBPath { | ||
| logger.Info("logs dir override: relocating logs.db from stored path %s to %s", sqliteConfig.Path, logsDBPath) | ||
| sqliteConfig.Path = logsDBPath | ||
| } | ||
| } else if sqliteConfig.Path != logsDBPath { | ||
| logger.Warn("logs dir override ignored: stored SQLite path %s appears explicitly configured; keeping it unchanged", sqliteConfig.Path) | ||
| } | ||
| } |
There was a problem hiding this comment.
-logs-dir is silently one-shot after the first startup
On the first startup with -logs-dir /mnt/a, logStoreConfig is nil (nothing in DB yet), so the relocation block is skipped and a new config with Path: /mnt/a/logs.db is created and then persisted to config.db via UpdateLogsStoreConfig at line 1069. On the second startup with -logs-dir /mnt/b, GetLogsStoreConfig returns {Path: "/mnt/a/logs.db"}. The guard at line 1014 only permits relocation when the stored path is empty or equals defaultSQLiteLogsDBPath (i.e. $configDir/logs.db). Since /mnt/a/logs.db matches neither, control falls to line 1019 and logs a warning: "stored SQLite path appears explicitly configured; keeping it unchanged". The flag has no effect and /mnt/a/logs.db is persisted back again.
The code cannot distinguish "this path was written by a previous -logs-dir invocation" from "this path was explicitly set by the operator via the dashboard/API". The simplest fix is: when logsDirOverridden is true, always replace the stored path with logsDBPath — the flag is the operator's explicit runtime intent. Operators who want a pinned path via config.json are already protected by the explicit-config-wins branch earlier in this function. A subtest for "second startup with a changed logs dir" would catch this regression.
2 participants
Summary
logs.dbis created as a sibling ofconfig.dbin the app dir, forcing the config and the large, churning request-log DB onto the same directory/volume. This adds a-logs-dirflag (default fromBIFROST_LOGS_DIR, mirroring the-host/BIFROST_HOSTprecedence) so the two can live on independent mounts — useful for putting logs on separate or ephemeral storage.Changes
-logs-dirflag threadedmain.go → Server.LogsDir → LoadConfig(ctx, configDir, logsDir).config.jsonor is non-SQLite, the override is ignored with a warning (not silently), so the operator gets feedback.Type of change
Affected areas
How to test
go test ./transports/bifrost-http/lib/ -run TestLoadConfig_LogsDirRelocatesLogsDBCovers relocation, the empty-default case, and explicit-config-wins.