feat: add instance id and run id#67
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a new instance subsystem that initializes a run UUID and a persistent (or fallback in-memory) instance UUID. The application startup ( Changes
Sequence DiagramsequenceDiagram
participant App as Application (run)
participant Observ as Observability
participant Instance as Instance Module (init)
participant FS as File System (/tmp/aisix_instance_id)
participant Lock as OnceLock (state)
App->>Observ: init_observability()
Observ-->>App: observability ready
App->>Instance: call instance::init()
Instance->>Lock: set RUN_ID (generate UUID)
Lock-->>Instance: RUN_ID stored
Instance->>FS: read /tmp/aisix_instance_id
alt file exists and non-empty
FS-->>Instance: return persisted ID
Instance->>Lock: set INSTANCE_ID (persisted)
else file missing or empty
Instance->>Instance: generate INSTANCE_ID (UUID)
Instance->>FS: create parent dirs & write ID
alt write succeeds
FS-->>Instance: written
Instance->>Lock: set INSTANCE_ID (persisted)
else write fails
FS-->>Instance: error
Instance->>Instance: log warning (non-persistent)
Instance->>Lock: set INSTANCE_ID (in-memory)
end
end
Instance-->>App: return Result (ok / error)
App->>App: continue loading configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 18 minutes and 54 seconds. 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: 2
🧹 Nitpick comments (4)
src/utils/mod.rs (1)
1-3: Document the new public module.
pub mod instance;exposes a new public surface, but there’s no///module doc yet. Please add a short summary before exporting it.As per coding guidelines,
Use /// for doc comments on public items in Rust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/mod.rs` around lines 1 - 3, Add a short module-level documentation comment for the newly exported public module by placing a /// doc comment immediately above the pub mod instance; declaration in src/utils/mod.rs; the comment should be a one-line summary (using /// as required for public items) describing the purpose of the instance module so the public surface is documented.src/utils/instance.rs (3)
22-33: Avoid panicking in the public getters.
run_id()andinstance_id()expect()initialization, so any new call site that reaches them beforeinit()will crash the process. Consider returning a borrowed string,Option, orResultinstead of hard-expecting initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/instance.rs` around lines 22 - 33, The public getters run_id() and instance_id() currently call .expect(...) on the global RUN_ID and INSTANCE_ID, which panics if init() hasn't run; change their signatures to return a non-panicking type (e.g. Option<String> or Result<String, InitError>) that safely propagates “not initialized” to callers, update the body to map the get().cloned() into that type instead of .expect(), and update any callers to handle the Option/Result accordingly; reference the symbols RUN_ID, INSTANCE_ID, run_id(), instance_id() (and the existing init() that sets them) when making the changes.
7-7: Don't persist a supposedly stable ID in/tmp.
/tmp/aisix_instance_idcan be wiped on reboot or by tmp cleanup, so the instance ID will rotate even when persistence works. If the ID is meant to survive restarts, make the path configurable or move it to a durable data directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/instance.rs` at line 7, The constant ID_FILE pointing to "/tmp/aisix_instance_id" is unsafe for a persistent instance ID; change ID_FILE from a hardcoded const to a configurable path (read from an env var like AISIX_INSTANCE_ID_PATH) and fall back to a durable location such as XDG data dir or /var/lib/<app> (use the directories or dirs crate to compute XDG_DATA_HOME/aisix and create it if missing). Update functions that read/write the instance ID (the code using ID_FILE) to accept the resolved path or load it at startup so the ID survives reboots and tmp cleanup; ensure directory creation and file permission errors are handled and documented in the config/env fallback logic.
94-136: Usepretty_assertions::assert_eqin the test assertions.The equality checks in lines 107, 126, 127, and 131 should use
pretty_assertions::assert_eqinstead of the standardassert_eq!macro for clearer test failure output. The crate already depends onpretty_assertions = "1.4"and other test files throughout the codebase already follow this pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/instance.rs` around lines 94 - 136, Add an import for pretty_assertions' assert_eq and replace the std macro uses with it: inside the tests module (mod tests) add "use pretty_assertions::assert_eq;" and change the equality assertions in reuses_existing_file (the assert_eq comparing resolve_instance_id_from_path(&path) to "existing-id") and in writes_new_id_when_file_absent (the assert_eq comparing std::fs::read_to_string(&path) to id) to use that imported assert_eq function; leave non-equality asserts (e.g., assert!(!id.is_empty())) as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib.rs`:
- Around line 37-41: The startup logs from instance::init() are lost because
run() initializes observability after calling instance::init(); to fix,
initialize observability first by calling init_observability() (capturing
ob_shutdown_signal/ob_shutdown_task) before invoking instance::init(), or
alternatively silence logging in instance::init(); update run() so the
observability setup (init_observability) occurs prior to instance::init() and
preserve the existing context() error handling around both calls (referencing
run, instance::init, and init_observability to locate the changes).
In `@src/utils/instance.rs`:
- Around line 35-56: resolve_instance_id_from_path currently treats any
read_id_file error as if the file was missing and returns a fresh UUID; change
it to propagate non-NotFound I/O errors instead of collapsing them into a new
id. Specifically, replace the current if-let Ok(Some(id)) / if-let Ok(())
pattern with an explicit match on read_id_file(path) so that match Ok(Some(id))
returns Ok(id), Ok(None) falls through to generate a new UUID and attempt
write_id_file(path, &id) (preserving the info/warn logging behavior), and Err(e)
immediately returns Err(e) from resolve_instance_id_from_path; keep
write_id_file behavior unchanged.
---
Nitpick comments:
In `@src/utils/instance.rs`:
- Around line 22-33: The public getters run_id() and instance_id() currently
call .expect(...) on the global RUN_ID and INSTANCE_ID, which panics if init()
hasn't run; change their signatures to return a non-panicking type (e.g.
Option<String> or Result<String, InitError>) that safely propagates “not
initialized” to callers, update the body to map the get().cloned() into that
type instead of .expect(), and update any callers to handle the Option/Result
accordingly; reference the symbols RUN_ID, INSTANCE_ID, run_id(), instance_id()
(and the existing init() that sets them) when making the changes.
- Line 7: The constant ID_FILE pointing to "/tmp/aisix_instance_id" is unsafe
for a persistent instance ID; change ID_FILE from a hardcoded const to a
configurable path (read from an env var like AISIX_INSTANCE_ID_PATH) and fall
back to a durable location such as XDG data dir or /var/lib/<app> (use the
directories or dirs crate to compute XDG_DATA_HOME/aisix and create it if
missing). Update functions that read/write the instance ID (the code using
ID_FILE) to accept the resolved path or load it at startup so the ID survives
reboots and tmp cleanup; ensure directory creation and file permission errors
are handled and documented in the config/env fallback logic.
- Around line 94-136: Add an import for pretty_assertions' assert_eq and replace
the std macro uses with it: inside the tests module (mod tests) add "use
pretty_assertions::assert_eq;" and change the equality assertions in
reuses_existing_file (the assert_eq comparing
resolve_instance_id_from_path(&path) to "existing-id") and in
writes_new_id_when_file_absent (the assert_eq comparing
std::fs::read_to_string(&path) to id) to use that imported assert_eq function;
leave non-equality asserts (e.g., assert!(!id.is_empty())) as-is.
In `@src/utils/mod.rs`:
- Around line 1-3: Add a short module-level documentation comment for the newly
exported public module by placing a /// doc comment immediately above the pub
mod instance; declaration in src/utils/mod.rs; the comment should be a one-line
summary (using /// as required for public items) describing the purpose of the
instance module so the public surface is documented.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fdc4a0b-56b6-40a4-855c-60040ba32093
📒 Files selected for processing (3)
src/lib.rssrc/utils/instance.rssrc/utils/mod.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/instance.rs (2)
110-151: Usepretty_assertions::assert_eq!for better test output.Per coding guidelines, tests should use
pretty_assertions::assert_eqfor improved diff output on failures.♻️ Suggested change
#[cfg(test)] mod tests { use std::path::PathBuf; + use pretty_assertions::assert_eq; + use super::*;As per coding guidelines: "Use pretty_assertions::assert_eq and assert_matches::assert_matches for better test output"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/instance.rs` around lines 110 - 151, Replace the standard assert_eq! calls in the tests with pretty_assertions::assert_eq! to get better diff output: update the tests reuses_existing_file and writes_new_id_when_file_absent to use pretty_assertions::assert_eq! (or add `use pretty_assertions::assert_eq;` in the test module) while leaving other asserts as-is; ensure the symbol resolve_instance_id_from_path remains unchanged and only the assertion macro is swapped.
13-20: Consider early-return if already initialized.If
init()is called a second time,resolve_instance_id_from_path()still performs file I/O and could return an error even thoughINSTANCE_IDis already populated.♻️ Suggested improvement
pub fn init() -> Result<()> { + if RUN_ID.get().is_some() && INSTANCE_ID.get().is_some() { + return Ok(()); + } + RUN_ID.get_or_init(|| Uuid::new_v4().to_string()); let instance_id = resolve_instance_id_from_path(Path::new(ID_FILE))?; INSTANCE_ID.get_or_init(|| instance_id); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/instance.rs` around lines 13 - 20, The init() function should early-return if INSTANCE_ID is already set to avoid calling resolve_instance_id_from_path() (which does file I/O) on subsequent calls; add a check like INSTANCE_ID.get().is_some() and return Ok(()) before calling resolve_instance_id_from_path(Path::new(ID_FILE)), keep RUN_ID initialization via RUN_ID.get_or_init(|| Uuid::new_v4().to_string()) as-is, and only call INSTANCE_ID.get_or_init(|| instance_id) when INSTANCE_ID is not present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/instance.rs`:
- Around line 110-151: Replace the standard assert_eq! calls in the tests with
pretty_assertions::assert_eq! to get better diff output: update the tests
reuses_existing_file and writes_new_id_when_file_absent to use
pretty_assertions::assert_eq! (or add `use pretty_assertions::assert_eq;` in the
test module) while leaving other asserts as-is; ensure the symbol
resolve_instance_id_from_path remains unchanged and only the assertion macro is
swapped.
- Around line 13-20: The init() function should early-return if INSTANCE_ID is
already set to avoid calling resolve_instance_id_from_path() (which does file
I/O) on subsequent calls; add a check like INSTANCE_ID.get().is_some() and
return Ok(()) before calling resolve_instance_id_from_path(Path::new(ID_FILE)),
keep RUN_ID initialization via RUN_ID.get_or_init(|| Uuid::new_v4().to_string())
as-is, and only call INSTANCE_ID.get_or_init(|| instance_id) when INSTANCE_ID is
not present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ea31b2e-8a82-4175-ac81-c3442da60747
📒 Files selected for processing (1)
src/utils/instance.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests