validation: degrade locked logs db - #1
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57a3ea962c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let pool_result = SqlitePoolOptions::new() | ||
| .max_connections(1) | ||
| .connect_with(options) |
There was a problem hiding this comment.
When startup falls back because the persistent logs database is locked, this pool retains SQLx's default idle timeout and maximum connection lifetime. Once the sole connection is reaped or recycled, SQLite destroys its connection-local :memory: database; the replacement connection has no migrated logs table, so subsequent inserts are discarded by the log writer and log queries return no such table. Disable connection recycling and keep the connection alive, or use an in-memory database strategy that survives replacement connections.
Useful? React with 👍 / 👎.
| let fallback_pool = config | ||
| .open_logs_db(&runtime_logs_migrator(), /*telemetry_override*/ None) |
There was a problem hiding this comment.
In the locked-database scenario, this test calls the private SqliteConfig::open_logs_db helper directly, so it does not exercise the user-facing StateRuntime::init path, construction with the fallback pool, or startup log maintenance. Add an integration-style test that locks the logs database, initializes StateRuntime, and performs a log operation.
AGENTS.md reference: AGENTS.md:L112-L121
Useful? React with 👍 / 👎.
| .fetch_one(&fallback_pool) | ||
| .await | ||
| .expect("query fallback log schema"); | ||
| assert_eq!(fallback_has_logs, 1, "fallback store should be migrated"); |
There was a problem hiding this comment.
The new test module uses the standard assert_eq! macro rather than importing pretty_assertions::assert_eq, contrary to the repository's test assertion convention and resulting in less useful failure diffs.
AGENTS.md reference: AGENTS.md:L210-L214
Useful? React with 👍 / 👎.
1 participant
Validation-only PR for
fix/35555-nongating-locked-logs-db. Closed without merge after collecting fork CI evidence. The source artifact remainscharle-z:fix/35555-nongating-locked-logs-db; upstream evidence is recorded in openai#35555.