feat: expose run(), Args, GIT_HASH, init_observability() as public lib API#46
Open
feat: expose run(), Args, GIT_HASH, init_observability() as public lib API#46
Conversation
added 5 commits
April 16, 2026 09:38
|
Foo Bar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors startup into a public library API with observability hooks and new run entrypoints, exposes CLI-config handling and several modules, adds resilient build/git metadata and UI build path handling, enables optional etcd TLS configuration, and adds a GitHub Actions workflow notifying an EE repo on main/tag pushes. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main()
participant LibRun as aisix::run(config_file)
participant Config as ConfigLoader
participant Obs as init_observability()
participant Proxy as ProxyServer
participant Admin as AdminServer
Main->>LibRun: call with config_file
LibRun->>Config: load configuration
LibRun->>Obs: init_observability()
Obs-->>LibRun: shutdown sender & join handle
LibRun->>Proxy: spawn proxy server
LibRun->>Admin: spawn admin server
LibRun->>LibRun: await servers or ctrl-c (tokio::select!)
alt Shutdown signal
LibRun->>Proxy: request shutdown
LibRun->>Admin: request shutdown
else Server error
LibRun->>LibRun: capture error and initiate shutdown
end
LibRun->>Obs: send shutdown & await join handle
LibRun-->>Main: return Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (3)
src/config/etcd.rs (1)
23-28: Add rustdoc for new public TLS API typesPlease add
///comments forEtcdTlsConfigand its public fields (cert_file,key_file,ca_file) to document expected paths/formats and semantics.✍️ Suggested docs
#[derive(Clone, Debug, Default, Deserialize)] +/// TLS file paths used for securing etcd connections. pub struct EtcdTlsConfig { + /// Path to client certificate file. pub cert_file: Option<String>, + /// Path to client private key file. pub key_file: Option<String>, + /// Path to CA certificate file used to verify etcd server certs. pub ca_file: Option<String>, }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/config/etcd.rs` around lines 23 - 28, Add Rustdoc comments (///) for the public struct EtcdTlsConfig and each public field (cert_file, key_file, ca_file) describing their purpose, expected file paths or formats (e.g., PEM-encoded certificate and private key paths, CA certificate path), and semantics (Option means TLS is optional; when set the client/server should use these files for mutual TLS). Update the doc on the struct to note when TLS is required/used and any format expectations (PEM, path string), and on each field clarify whether it is a filesystem path, optional, and any ordering/usage expectations (e.g., cert_file pairs with key_file)..github/workflows/notify-ee-build.yaml (1)
1-32: Ensure the target repository is configured to handle dispatch events.The workflow dispatches
dev-buildandrelease-buildevents toapi7/aisix-ee, but this workflow will succeed regardless of whether the target repository has workflows configured to handle these events. Verify thatapi7/aisix-eehasrepository_dispatchworkflows listening for both event types to ensure the integration works end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/notify-ee-build.yaml around lines 1 - 32, The workflow dispatches "dev-build" and "release-build" to repository api7/aisix-ee but doesn't verify the target repo is listening; ensure api7/aisix-ee contains repository_dispatch handlers for event-type: dev-build and event-type: release-build (i.e., workflows with on: repository_dispatch and filters for these event-types), confirm the secret EE_DISPATCH_TOKEN exists with repo scope and the target workflows accept the client-payload keys ("sha" and "tag") being sent, and update either side (sender or target) so event-type names and payload fields used in this workflow and the target handlers match exactly.build.rs (1)
7-9: Consider handling missing Git metadata explicitly.By default, vergen-git2 does not fail the build when
.gitis unavailable. Instead, whenGit2Builder::default().sha(true).build()?runs outside a Git repository, it emits a warning and setsVERGEN_GIT_SHAto a default value, allowing the build to succeed. This means library users building from source archives or vendored dependencies will get a fallback hash (likely "unknown" or empty) rather than a build failure.If an empty or placeholder hash is acceptable for your use case, the current code is fine. Otherwise, consider using
option_env!forGIT_HASHinstead ofenv!, or set a custom fallback before emitting by checking for.cargo_vcs_info.json(published crates) or pre-setting the environment variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.rs` around lines 7 - 9, The build currently emits Git metadata via Git2Builder::default().sha(true).build()? but vergen sets a placeholder when .git is missing; update build.rs to detect that case and either fail or set a controlled fallback: after calling Git2Builder::default().sha(true).build() and Emitter::emit(), read the VERGEN_GIT_SHA environment value (or inspect .cargo_vcs_info.json presence) and if it equals the vergen placeholder (empty/"unknown"/default) then either return an Err/panic to fail the build or write a deterministic fallback into the environment before emitting; alternatively change downstream consumption to use option_env!("GIT_HASH") instead of env! so missing/placeholder hash is handled safely—references: Git2Builder::default(), sha(true), build(), Emitter::emit(), and the VERGEN_GIT_SHA / GIT_HASH usage.
🤖 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/config/etcd.rs`:
- Around line 23-37: The EtcdTlsConfig struct is defined and surfaced on
Config.tls but never used by connect_client, so either wire TLS into the client
or fail fast when tls is set; update connect_client (the function that builds
the etcd client) to inspect Config.tls and if Some, apply the
cert_file/key_file/ca_file to the etcd client TLS configuration (or return an
Err immediately if any required TLS file is missing), ensuring
EtcdConfigProvider::new (via create_provider) receives a client that honors TLS;
alternatively, if you choose to disable TLS support for now, modify
connect_client to return an explicit error when Config.tls.is_some() so
misconfigurations fail fast.
In `@src/lib.rs`:
- Around line 127-156: The current run() replaces concrete server errors with a
generic anyhow::bail, losing actionable context; change the exception handling
to capture and propagate the first concrete error instead of setting exception =
true and calling bail. Inside the select! where you call serve_proxy and
serve_admin (and handle tokio::signal::ctrl_c), store the Err(e) into an
Option<anyhow::Error> (or Box<dyn Error>) variable (e.g., first_error) rather
than only logging and flipping exception; after calling
config_provider.shutdown().await and logging, if first_error.is_some() return
Err(first_error.unwrap()) (or convert it to anyhow::Error) so callers of run()
receive the original server/startup error. Ensure you still log errors but do
not replace the original error with the generic message.
- Around line 27-31: Public functions exported from src/ must have the tracing
boundary attribute; add the #[fastrace::trace] attribute above the function
definitions for init_observability and run so they are annotated as tracing
entry points. Locate the pub fn init_observability(...) and the pub fn run(...)
functions and insert the #[fastrace::trace] attribute immediately above each
function signature to satisfy the coding guideline for public entrypoints.
In `@src/main.rs`:
- Around line 11-16: aisix::run(...) is currently awaited with a trailing `?`
which returns early and skips the observability shutdown sequence
(ob_shutdown_signal.send and ob_shutdown_task.await); instead capture the result
of aisix::run(args.config).await into a Result (e.g., let run_res =
aisix::run(...).await), then always call ob_shutdown_signal.send(()) and await
ob_shutdown_task.await.context(...)? to flush observability, and only after that
propagate the run result with run_res? so observability teardown always runs
even if run() failed.
---
Nitpick comments:
In @.github/workflows/notify-ee-build.yaml:
- Around line 1-32: The workflow dispatches "dev-build" and "release-build" to
repository api7/aisix-ee but doesn't verify the target repo is listening; ensure
api7/aisix-ee contains repository_dispatch handlers for event-type: dev-build
and event-type: release-build (i.e., workflows with on: repository_dispatch and
filters for these event-types), confirm the secret EE_DISPATCH_TOKEN exists with
repo scope and the target workflows accept the client-payload keys ("sha" and
"tag") being sent, and update either side (sender or target) so event-type names
and payload fields used in this workflow and the target handlers match exactly.
In `@build.rs`:
- Around line 7-9: The build currently emits Git metadata via
Git2Builder::default().sha(true).build()? but vergen sets a placeholder when
.git is missing; update build.rs to detect that case and either fail or set a
controlled fallback: after calling Git2Builder::default().sha(true).build() and
Emitter::emit(), read the VERGEN_GIT_SHA environment value (or inspect
.cargo_vcs_info.json presence) and if it equals the vergen placeholder
(empty/"unknown"/default) then either return an Err/panic to fail the build or
write a deterministic fallback into the environment before emitting;
alternatively change downstream consumption to use option_env!("GIT_HASH")
instead of env! so missing/placeholder hash is handled safely—references:
Git2Builder::default(), sha(true), build(), Emitter::emit(), and the
VERGEN_GIT_SHA / GIT_HASH usage.
In `@src/config/etcd.rs`:
- Around line 23-28: Add Rustdoc comments (///) for the public struct
EtcdTlsConfig and each public field (cert_file, key_file, ca_file) describing
their purpose, expected file paths or formats (e.g., PEM-encoded certificate and
private key paths, CA certificate path), and semantics (Option means TLS is
optional; when set the client/server should use these files for mutual TLS).
Update the doc on the struct to note when TLS is required/used and any format
expectations (PEM, path string), and on each field clarify whether it is a
filesystem path, optional, and any ordering/usage expectations (e.g., cert_file
pairs with key_file).
🪄 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: 527f5744-fbbe-49b8-8c89-d91dfe76d07e
📒 Files selected for processing (6)
.github/workflows/notify-ee-build.yamlCargo.tomlbuild.rssrc/config/etcd.rssrc/lib.rssrc/main.rs
Comment on lines
+27
to
+31
| /// Initialize observability (logging, tracing, metrics). | ||
| /// | ||
| /// Returns `(shutdown_sender, shutdown_task_handle)`. | ||
| /// Call `shutdown_sender.send(())` to flush and shut down observability. | ||
| pub fn init_observability() -> Result<(oneshot::Sender<()>, tokio::task::JoinHandle<()>)> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read src/lib.rs to verify the functions at the specified lines
fd -t f "lib.rs" src/Repository: api7/aisix
Length of output: 64
🏁 Script executed:
# Read the relevant sections of src/lib.rs
cat -n src/lib.rs | head -50Repository: api7/aisix
Length of output: 1861
🏁 Script executed:
# Read lines around 102-105 for the second function
tail -n +95 src/lib.rs | head -20Repository: api7/aisix
Length of output: 768
Add #[fastrace::trace] to the new public entrypoints.
init_observability and run are now exported from src/, but neither is annotated with the tracing boundary required by the coding guidelines for public functions in src/**/*.rs.
Suggested patch
+#[fastrace::trace]
pub fn init_observability() -> Result<(oneshot::Sender<()>, tokio::task::JoinHandle<()>)> {+#[fastrace::trace]
pub async fn run(config_file: Option<String>) -> Result<()> {Also applies to: line 102-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib.rs` around lines 27 - 31, Public functions exported from src/ must
have the tracing boundary attribute; add the #[fastrace::trace] attribute above
the function definitions for init_observability and run so they are annotated as
tracing entry points. Locate the pub fn init_observability(...) and the pub fn
run(...) functions and insert the #[fastrace::trace] attribute immediately above
each function signature to satisfy the coding guideline for public entrypoints.
Comment on lines
+127
to
+156
| let mut exception = false; | ||
| select! { | ||
| res = tokio::signal::ctrl_c() => { | ||
| if let Err(e) = res { | ||
| error!("Failed to listen for shutdown signal: {}", e); | ||
| exception = true; | ||
| } | ||
| } | ||
| res = serve_proxy(config.clone(), proxy_router.clone()) => { | ||
| if let Err(e) = res { | ||
| error!("Proxy server error: {}", e); | ||
| exception = true; | ||
| } | ||
| } | ||
| res = serve_admin(config.clone(), admin::AppState::new(config, config_provider.clone(), resources, Some(proxy_router))) => { | ||
| if let Err(e) = res { | ||
| error!("Admin server error: {}", e); | ||
| exception = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if let Err(e) = config_provider.shutdown().await { | ||
| error!("Config provider shutdown error: {}", e); | ||
| exception = true; | ||
| } | ||
|
|
||
| info!("Stopping, see you next time!"); | ||
| if exception { | ||
| anyhow::bail!("one or more servers exited with an error"); |
There was a problem hiding this comment.
Return the real server/startup error instead of a generic bailout.
run() is now a library entrypoint, but this block logs the concrete bind/shutdown failure and then replaces it with one or more servers exited with an error. Callers lose the only actionable error context, which makes embedding and troubleshooting much harder.
Suggested direction
- let mut exception = false;
+ let mut run_error: Option<anyhow::Error> = None;
select! {
res = tokio::signal::ctrl_c() => {
if let Err(e) = res {
- error!("Failed to listen for shutdown signal: {}", e);
- exception = true;
+ let err = anyhow::Error::new(e).context("failed to listen for shutdown signal");
+ error!("{err:#}");
+ run_error = Some(err);
}
}
res = serve_proxy(config.clone(), proxy_router.clone()) => {
if let Err(e) = res {
- error!("Proxy server error: {}", e);
- exception = true;
+ let err = anyhow::Error::new(e).context("proxy server error");
+ error!("{err:#}");
+ run_error = Some(err);
}
}
res = serve_admin(config.clone(), admin::AppState::new(config, config_provider.clone(), resources, Some(proxy_router))) => {
if let Err(e) = res {
- error!("Admin server error: {}", e);
- exception = true;
+ let err = anyhow::Error::new(e).context("admin server error");
+ error!("{err:#}");
+ run_error = Some(err);
}
}
}
if let Err(e) = config_provider.shutdown().await {
- error!("Config provider shutdown error: {}", e);
- exception = true;
+ let err = anyhow::Error::new(e).context("config provider shutdown error");
+ error!("{err:#}");
+ run_error.get_or_insert(err);
}
info!("Stopping, see you next time!");
- if exception {
- anyhow::bail!("one or more servers exited with an error");
+ if let Some(err) = run_error {
+ return Err(err);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut exception = false; | |
| select! { | |
| res = tokio::signal::ctrl_c() => { | |
| if let Err(e) = res { | |
| error!("Failed to listen for shutdown signal: {}", e); | |
| exception = true; | |
| } | |
| } | |
| res = serve_proxy(config.clone(), proxy_router.clone()) => { | |
| if let Err(e) = res { | |
| error!("Proxy server error: {}", e); | |
| exception = true; | |
| } | |
| } | |
| res = serve_admin(config.clone(), admin::AppState::new(config, config_provider.clone(), resources, Some(proxy_router))) => { | |
| if let Err(e) = res { | |
| error!("Admin server error: {}", e); | |
| exception = true; | |
| } | |
| } | |
| } | |
| if let Err(e) = config_provider.shutdown().await { | |
| error!("Config provider shutdown error: {}", e); | |
| exception = true; | |
| } | |
| info!("Stopping, see you next time!"); | |
| if exception { | |
| anyhow::bail!("one or more servers exited with an error"); | |
| let mut run_error: Option<anyhow::Error> = None; | |
| select! { | |
| res = tokio::signal::ctrl_c() => { | |
| if let Err(e) = res { | |
| let err = anyhow::Error::new(e).context("failed to listen for shutdown signal"); | |
| error!("{err:#}"); | |
| run_error = Some(err); | |
| } | |
| } | |
| res = serve_proxy(config.clone(), proxy_router.clone()) => { | |
| if let Err(e) = res { | |
| let err = anyhow::Error::new(e).context("proxy server error"); | |
| error!("{err:#}"); | |
| run_error = Some(err); | |
| } | |
| } | |
| res = serve_admin(config.clone(), admin::AppState::new(config, config_provider.clone(), resources, Some(proxy_router))) => { | |
| if let Err(e) = res { | |
| let err = anyhow::Error::new(e).context("admin server error"); | |
| error!("{err:#}"); | |
| run_error = Some(err); | |
| } | |
| } | |
| } | |
| if let Err(e) = config_provider.shutdown().await { | |
| let err = anyhow::Error::new(e).context("config provider shutdown error"); | |
| error!("{err:#}"); | |
| run_error.get_or_insert(err); | |
| } | |
| info!("Stopping, see you next time!"); | |
| if let Some(err) = run_error { | |
| return Err(err); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib.rs` around lines 127 - 156, The current run() replaces concrete
server errors with a generic anyhow::bail, losing actionable context; change the
exception handling to capture and propagate the first concrete error instead of
setting exception = true and calling bail. Inside the select! where you call
serve_proxy and serve_admin (and handle tokio::signal::ctrl_c), store the Err(e)
into an Option<anyhow::Error> (or Box<dyn Error>) variable (e.g., first_error)
rather than only logging and flipping exception; after calling
config_provider.shutdown().await and logging, if first_error.is_some() return
Err(first_error.unwrap()) (or convert it to anyhow::Error) so callers of run()
receive the original server/startup error. Ensure you still log errors but do
not replace the original error with the generic message.
added 4 commits
April 16, 2026 09:57
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib.rs (2)
35-35:⚠️ Potential issue | 🟡 MinorAdd
#[fastrace::trace]to public entrypoints insrc/.Both
runandinit_observabilityare public functions insrc/**/*.rsand should be annotated.Suggested patch
+#[fastrace::trace] pub async fn run(config_file: Option<String>) -> Result<()> {+#[fastrace::trace] pub fn init_observability() -> Result<(oneshot::Sender<()>, tokio::task::JoinHandle<()>)> {As per coding guidelines,
src/**/*.rs: Use #[fastrace::trace] decorator for distributed tracing on public functions.Also applies to: 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` at line 35, Add the #[fastrace::trace] attribute to the public entrypoints to enable distributed tracing: place #[fastrace::trace] immediately above the pub async fn run(...) -> Result<()> declaration (function run) and likewise above the pub fn init_observability(...) signature (function init_observability); also ensure the fastrace crate is available in scope (add/use it if missing) so the attribute compiles.
58-92:⚠️ Potential issue | 🔴 CriticalDo not terminate from library
run(); return the concrete error instead.
run()still exits the process on Line 91 and only tracks a boolean failure state, so embedders cannot handle errors and lose root-cause context.Suggested patch
-use std::{process::exit, sync::Arc}; +use std::sync::Arc;- let mut exception = false; + let mut run_error: Option<anyhow::Error> = None; select! { res = tokio::signal::ctrl_c() => { if let Err(e) = res { - error!("Failed to listen for shutdown signal: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("failed to listen for shutdown signal"); + error!("{err:#}"); + run_error = Some(err); } } res = serve_proxy(config.clone(), proxy_router.clone()) => { if let Err(e) = res { - error!("Proxy server error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("proxy server error"); + error!("{err:#}"); + run_error.get_or_insert(err); } } res = serve_admin(config.clone(), admin::AppState::new(config, config_provider.clone(), resources, Some(proxy_router))) => { if let Err(e) = res { - error!("Admin server error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("admin server error"); + error!("{err:#}"); + run_error.get_or_insert(err); } } } if let Err(e) = config_provider.shutdown().await { - error!("Config provider shutdown error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("config provider shutdown error"); + error!("{err:#}"); + run_error.get_or_insert(err); } @@ - exit(if exception { 1 } else { 0 }); + if let Some(err) = run_error { + return Err(err); + } + Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 58 - 92, The run() function currently calls std::process::exit(...) and uses a boolean exception flag, preventing callers from receiving detailed errors; change run() to return a Result instead of exiting: propagate and convert errors from serve_proxy, serve_admin, tokio::signal::ctrl_c handling, config_provider.shutdown().await, and ob_shutdown_task.await (the places that now set exception or use context()) into a concrete Err variant so callers can handle them, remove the final exit(...) call and instead return Ok(()) on success or Err(...) with the original error(s) when any awaited operation fails; ensure you preserve the existing error contexts (e.g., the .context("failed to shutdown observability") on ob_shutdown_task) and stop using the exception boolean.
🧹 Nitpick comments (2)
src/lib.rs (1)
1-5: Document (or narrow) the new public module surface.These
pub modexports create a wide public API but currently have no///docs. If only specific symbols are intended for consumers, consider keeping modules private and re-exporting only those symbols.As per coding guidelines,
**/*.rs: 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/lib.rs` around lines 1 - 5, The crate root currently exposes broad public modules via the pub mod declarations (admin, config, gateway, proxy, utils) without documentation; either add /// doc comments above each pub mod (admin, config, gateway, proxy, utils) describing the public surface and intended consumer-facing API, or make the modules private (remove pub) and instead re-export only the specific types/functions you intend to expose (e.g., pub use crate::gateway::MyPublicType) so the public API is narrow and every public item has a /// doc comment per the Rust guidelines.build.rs (1)
16-19: Surface the fallback reason in build logs.The current branch hides the actual
emit()error. Emitting a Cargo warning will make CI/debugging much easier.Suggested patch
- if result.is_err() { + if let Err(e) = result { + println!("cargo:warning=failed to emit git SHA via vergen: {}", e); // Emit a placeholder so that code referencing VERGEN_GIT_SHA compiles. println!("cargo:rustc-env=VERGEN_GIT_SHA=unknown"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.rs` around lines 16 - 19, When handling the result from emit(), don't swallow the error: capture the Err value when result.is_err() and emit a Cargo warning that includes the error's Display text so CI/build logs show why emit() failed; keep the existing fallback that prints "cargo:rustc-env=VERGEN_GIT_SHA=unknown" but add a println!("cargo:warning=...") containing the captured error message (referencing the result variable and the VERGEN_GIT_SHA fallback) to surface the failure in build logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib.rs`:
- Line 35: Add the #[fastrace::trace] attribute to the public entrypoints to
enable distributed tracing: place #[fastrace::trace] immediately above the pub
async fn run(...) -> Result<()> declaration (function run) and likewise above
the pub fn init_observability(...) signature (function init_observability); also
ensure the fastrace crate is available in scope (add/use it if missing) so the
attribute compiles.
- Around line 58-92: The run() function currently calls std::process::exit(...)
and uses a boolean exception flag, preventing callers from receiving detailed
errors; change run() to return a Result instead of exiting: propagate and
convert errors from serve_proxy, serve_admin, tokio::signal::ctrl_c handling,
config_provider.shutdown().await, and ob_shutdown_task.await (the places that
now set exception or use context()) into a concrete Err variant so callers can
handle them, remove the final exit(...) call and instead return Ok(()) on
success or Err(...) with the original error(s) when any awaited operation fails;
ensure you preserve the existing error contexts (e.g., the .context("failed to
shutdown observability") on ob_shutdown_task) and stop using the exception
boolean.
---
Nitpick comments:
In `@build.rs`:
- Around line 16-19: When handling the result from emit(), don't swallow the
error: capture the Err value when result.is_err() and emit a Cargo warning that
includes the error's Display text so CI/build logs show why emit() failed; keep
the existing fallback that prints "cargo:rustc-env=VERGEN_GIT_SHA=unknown" but
add a println!("cargo:warning=...") containing the captured error message
(referencing the result variable and the VERGEN_GIT_SHA fallback) to surface the
failure in build logs.
In `@src/lib.rs`:
- Around line 1-5: The crate root currently exposes broad public modules via the
pub mod declarations (admin, config, gateway, proxy, utils) without
documentation; either add /// doc comments above each pub mod (admin, config,
gateway, proxy, utils) describing the public surface and intended
consumer-facing API, or make the modules private (remove pub) and instead
re-export only the specific types/functions you intend to expose (e.g., pub use
crate::gateway::MyPublicType) so the public API is narrow and every public item
has a /// doc comment per the Rust guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1301e12-12b1-4770-ac87-2b0e8f7c8a9c
📒 Files selected for processing (4)
Cargo.tomlbuild.rssrc/lib.rssrc/main.rs
✅ Files skipped from review due to trivial changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
Allow callers that initialise observability themselves (e.g. aisix-ee) to reuse the already-initialised handles instead of calling run(), which would trigger a double-initialisation panic in logforth.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib.rs (2)
173-184:⚠️ Potential issue | 🟠 MajorGuard against missing Tokio runtime in
init_observability().Line 173 uses
tokio::spawn(), which panics when called outside an active Tokio runtime. This is a trap for the public embedder API, especially since the documentation doesn't mention the runtime requirement.Use
Handle::try_current()to check for an active runtime and return an error gracefully instead of allowing a hidden panic.Suggested patch
pub fn init_observability() -> Result<(oneshot::Sender<()>, tokio::task::JoinHandle<()>)> { + let handle = tokio::runtime::Handle::try_current() + .context("init_observability must be called from within a Tokio runtime")?; + use std::{borrow::Cow, time::Duration}; use fastrace::collector::Config; @@ - let shutdown_handle = tokio::spawn(async move { + let shutdown_handle = handle.spawn(async move { let _ = rx.await; fastrace::flush();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 173 - 184, init_observability currently calls tokio::spawn(...) which will panic if there's no active Tokio runtime; modify init_observability to use tokio::runtime::Handle::try_current() and if that returns an Err, return a graceful error instead of calling tokio::spawn. Specifically, check Handle::try_current() before spawning the shutdown task (the block that awaits rx, calls fastrace::flush(), meter_provider.shutdown(), and logger flush/exit), and only call tokio::spawn when a handle is available; otherwise return an Err (or propagate an appropriate error) from init_observability so the caller knows a Tokio runtime is required. Ensure references to shutdown_handle, rx, and meter_provider remain consistent when moving this check.
71-105:⚠️ Potential issue | 🔴 CriticalReplace the
exceptionboolean flag with proper error propagation and return fromrun_with_observability().Line 104 calls
process::exit(...)in a public library function, terminating the host process instead of returning to the caller. This violates theResult<()>contract and breaks the stated embedding goal. Theexceptionflag also loses concrete error information that callers need to handle.Additionally, the public functions
run(),run_with_observability(), andinit_observability()are missing the required#[fastrace::trace]decorator for distributed tracing (persrc/**/*.rscoding guidelines).Suggested patch
- let mut exception = false; + let mut run_error: Option<anyhow::Error> = None; select! { res = tokio::signal::ctrl_c() => { if let Err(e) = res { - error!("Failed to listen for shutdown signal: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("failed to listen for shutdown signal"); + error!("{err:#}"); + run_error = Some(err); } } res = serve_proxy(config.clone(), proxy_router.clone()) => { if let Err(e) = res { - error!("Proxy server error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("proxy server error"); + error!("{err:#}"); + run_error = Some(err); } } res = serve_admin(config.clone(), admin::AppState::new(config, config_provider.clone(), resources, Some(proxy_router))) => { if let Err(e) = res { - error!("Admin server error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("admin server error"); + error!("{err:#}"); + run_error = Some(err); } } } if let Err(e) = config_provider.shutdown().await { - error!("Config provider shutdown error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("config provider shutdown error"); + error!("{err:#}"); + run_error.get_or_insert(err); } info!("Stopping, see you next time!"); let _ = ob_shutdown_signal.send(()); ob_shutdown_task .await .context("failed to shutdown observability")?; - exit(if exception { 1 } else { 0 }); + if let Some(err) = run_error { + return Err(err); + } + + Ok(())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 71 - 105, The code uses an exception boolean and calls process::exit inside run_with_observability(), losing error details and terminating the host; change the select! branches to propagate concrete errors (return Err(...) or use the ? operator) instead of setting exception, aggregate/convert errors from serve_proxy, serve_admin and the ctrl_c listener into a single anyhow::Error (preserving context), call config_provider.shutdown().await? and ob_shutdown_task.await.context(...)?, and then return Ok(()) or Err(error) from run_with_observability() instead of calling process::exit; also add the #[fastrace::trace] attribute to the public functions run(), run_with_observability(), and init_observability() so tracing is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib.rs`:
- Around line 173-184: init_observability currently calls tokio::spawn(...)
which will panic if there's no active Tokio runtime; modify init_observability
to use tokio::runtime::Handle::try_current() and if that returns an Err, return
a graceful error instead of calling tokio::spawn. Specifically, check
Handle::try_current() before spawning the shutdown task (the block that awaits
rx, calls fastrace::flush(), meter_provider.shutdown(), and logger flush/exit),
and only call tokio::spawn when a handle is available; otherwise return an Err
(or propagate an appropriate error) from init_observability so the caller knows
a Tokio runtime is required. Ensure references to shutdown_handle, rx, and
meter_provider remain consistent when moving this check.
- Around line 71-105: The code uses an exception boolean and calls process::exit
inside run_with_observability(), losing error details and terminating the host;
change the select! branches to propagate concrete errors (return Err(...) or use
the ? operator) instead of setting exception, aggregate/convert errors from
serve_proxy, serve_admin and the ctrl_c listener into a single anyhow::Error
(preserving context), call config_provider.shutdown().await? and
ob_shutdown_task.await.context(...)?, and then return Ok(()) or Err(error) from
run_with_observability() instead of calling process::exit; also add the
#[fastrace::trace] attribute to the public functions run(),
run_with_observability(), and init_observability() so tracing is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8853fde8-703c-4240-a272-0ffb77538522
📒 Files selected for processing (2)
build.rssrc/lib.rs
✅ Files skipped from review due to trivial changes (1)
- build.rs
added 2 commits
April 16, 2026 13:17
Allow embedders (e.g. aisix-ee) to load and mutate Config before passing it to the gateway, enabling env-var overrides such as replacing etcd hosts with API7_CONTROL_PLANE_ENDPOINTS.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib.rs (2)
31-65:⚠️ Potential issue | 🟡 MinorAdd
#[fastrace::trace]to every exported entrypoint.The earlier gap is still here, and the two new embedders APIs extend it:
run,run_with_observability,run_with_config, andinit_observabilityare all public functions undersrc/without the required tracing boundary.Suggested patch
+#[fastrace::trace] pub async fn run(config_file: Option<String>) -> Result<()> {+#[fastrace::trace] pub async fn run_with_observability(+#[fastrace::trace] pub async fn run_with_config(+#[fastrace::trace] pub fn init_observability() -> Result<(oneshot::Sender<()>, tokio::task::JoinHandle<()>)> {#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path lines = Path("src/lib.rs").read_text().splitlines() for i, line in enumerate(lines, 1): s = line.strip() if s.startswith("pub ") and "fn " in s: prev = lines[i - 2].strip() if i >= 2 else "" status = "ok" if prev == "#[fastrace::trace]" else "missing" print(f"{i}: {s} -> {status}") PYAs per coding guidelines,
src/**/*.rs: Use#[fastrace::trace]decorator for distributed tracing on public functions.Also applies to: 119-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 31 - 65, Public exported entrypoints run, run_with_observability, run_with_config (and init_observability) are missing the required tracing attribute; add #[fastrace::trace] immediately above each of these function declarations (e.g. above the signatures for run, run_with_observability, run_with_config and the init_observability function) so the functions have the tracing boundary required by the project guidelines.
83-116:⚠️ Potential issue | 🔴 CriticalDon't terminate the host process from the public library runner.
run_with_configstill callsprocess::exitinstead of returning to the caller. That breaks the embedding use-case this PR is adding, and it also drops the actionable startup/shutdown error context that callers need.Suggested direction
- let mut exception = false; + let mut run_error: Option<anyhow::Error> = None; select! { res = tokio::signal::ctrl_c() => { if let Err(e) = res { - error!("Failed to listen for shutdown signal: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("failed to listen for shutdown signal"); + error!("{err:#}"); + run_error = Some(err); } } res = serve_proxy(config.clone(), proxy_router.clone()) => { if let Err(e) = res { - error!("Proxy server error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("proxy server error"); + error!("{err:#}"); + run_error = Some(err); } } res = serve_admin(config.clone(), admin::AppState::new(config, config_provider.clone(), resources, Some(proxy_router))) => { if let Err(e) = res { - error!("Admin server error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("admin server error"); + error!("{err:#}"); + run_error = Some(err); } } } if let Err(e) = config_provider.shutdown().await { - error!("Config provider shutdown error: {}", e); - exception = true; + let err = anyhow::Error::new(e).context("config provider shutdown error"); + error!("{err:#}"); + run_error.get_or_insert(err); } info!("Stopping, see you next time!"); let _ = ob_shutdown_signal.send(()); ob_shutdown_task .await .context("failed to shutdown observability")?; - exit(if exception { 1 } else { 0 }); + if let Some(err) = run_error { + return Err(err); + } + Ok(())
src/main.rsalready returnsrun(args.config).await, so the binary wrapper can own exit-code behavior while the library API stays embeddable.#!/bin/bash set -euo pipefail echo "Locations of process termination calls:" rg -n '\bexit\s*\(' src/lib.rs src/main.rs echo echo "Current public runner body:" sed -n '80,120p' src/lib.rs echo echo "Thin binary wrapper:" sed -n '1,20p' src/main.rs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 83 - 116, The public runner currently calls process::exit at the end (using exit(if exception { 1 } else { 0 })), which terminates the host process; instead make the runner (run_with_config / the public async function shown) return a Result/exit-code to the caller. Remove the call to process::exit and replace it with returning an appropriate Result or integer exit code (e.g., Ok(()) or Err(anyhow::Error) / Ok(0)/Ok(1)), propagate any errors from serve_proxy, serve_admin, config_provider.shutdown().await and ob_shutdown_task.await (they are already being logged) so callers can decide to exit; keep the shutdown signaling (ob_shutdown_signal.send()) and use the existing exception flag to determine the return value. Ensure the binary wrapper (main) performs process::exit based on the returned result.
🧹 Nitpick comments (1)
src/lib.rs (1)
1-5: Document or narrow the new module exports.Making
admin,config,gateway,proxy, andutilspublic turns them into supported library surface, but they are exported without public-item docs. If EE only needs a few types, prefer targetedpub uses; otherwise add docs to each module before treating this as stable API.As per coding guidelines,
**/*.rs: 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/lib.rs` around lines 1 - 5, The current pub mod declarations (admin, config, gateway, proxy, utils) expose entire modules as part of the crate public API without docs; either narrow the export surface by making the modules private (remove pub) and re-export only the specific types/functions the external API needs via targeted pub use (e.g., pub use crate::config::ConfigType; pub use crate::gateway::GatewayClient;) or, if the whole modules must remain public, add /// doc comments on each public module and all public items inside (admin, config, gateway, proxy, utils) to satisfy the /// docs guideline and make the API explicit; update src/lib.rs to reflect the chosen approach and adjust module visibility/exports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib.rs`:
- Around line 31-65: Public exported entrypoints run, run_with_observability,
run_with_config (and init_observability) are missing the required tracing
attribute; add #[fastrace::trace] immediately above each of these function
declarations (e.g. above the signatures for run, run_with_observability,
run_with_config and the init_observability function) so the functions have the
tracing boundary required by the project guidelines.
- Around line 83-116: The public runner currently calls process::exit at the end
(using exit(if exception { 1 } else { 0 })), which terminates the host process;
instead make the runner (run_with_config / the public async function shown)
return a Result/exit-code to the caller. Remove the call to process::exit and
replace it with returning an appropriate Result or integer exit code (e.g.,
Ok(()) or Err(anyhow::Error) / Ok(0)/Ok(1)), propagate any errors from
serve_proxy, serve_admin, config_provider.shutdown().await and
ob_shutdown_task.await (they are already being logged) so callers can decide to
exit; keep the shutdown signaling (ob_shutdown_signal.send()) and use the
existing exception flag to determine the return value. Ensure the binary wrapper
(main) performs process::exit based on the returned result.
---
Nitpick comments:
In `@src/lib.rs`:
- Around line 1-5: The current pub mod declarations (admin, config, gateway,
proxy, utils) expose entire modules as part of the crate public API without
docs; either narrow the export surface by making the modules private (remove
pub) and re-export only the specific types/functions the external API needs via
targeted pub use (e.g., pub use crate::config::ConfigType; pub use
crate::gateway::GatewayClient;) or, if the whole modules must remain public, add
/// doc comments on each public module and all public items inside (admin,
config, gateway, proxy, utils) to satisfy the /// docs guideline and make the
API explicit; update src/lib.rs to reflect the chosen approach and adjust module
visibility/exports accordingly.
added 8 commits
April 16, 2026 13:35
The control-plane etcd certificate has a misconfigured SAN (DnsName(""))
that does not match the server's IP address. rustls rejects this with a
NotValidForNameContext error.
Switch etcd-client from tls (rustls) to tls-openssl (OpenSSL) and set a
custom verify callback that validates the certificate chain against the CA
(preverify_ok) but skips hostname verification, matching the behavior of
reqwest's danger_accept_invalid_certs(true) used by the heartbeat client.
Remove ca_file/cert_file/key_file — use ca_pem/cert_pem/key_pem (String) instead so callers can pass PEM content directly without temp file I/O.
The API7 dp-manager does not implement the Maintenance service; calling status() returns HTTP 404 which tonic misparses as an invalid gRPC compression flag (52 == ASCII '4'). A KV.Get on a dummy key works with any etcd-compatible implementation.
- Add RestEtcdConfigProvider in src/config/rest_etcd.rs that speaks grpc-gateway JSON REST instead of native gRPC, allowing aisix to connect to dp-manager which only exposes /v3/kv/range and /v3/watch as HTTP REST endpoints (not native gRPC) - Key translation: wire prefix /apisix (dp-manager convention) mapped to/from relative keys transparently - Watch stream: parses chunked JSON lines from the grpc-gateway stream with reconnect and revision-based resume - Add run_with_provider() to lib.rs so embedders (aisix-ee) can supply a pre-built ConfigProvider instead of going through create_provider() - Add base64 dependency for grpc-gateway byte field encoding
from_pkcs8_pem only accepts PKCS#8 keys (BEGIN PRIVATE KEY). The CP certificate may use PKCS#1 RSA or EC keys which cause a builder error at request time. Use openssl crate to parse any PEM key and export as PKCS#12 DER, then load via from_pkcs12_der. Also improve watch error logging to show full error chain.
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
2 participants
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
init_observability(),run(),Args, andGIT_HASHfromsrc/main.rsintosrc/lib.rsas public items, soaisix-eecan depend onaisixas a library and call these symbolssrc/main.rsto an 18-line thin wrapperbuild.rsto emitVERGEN_GIT_SHAviavergen-git2, enablingpub const GIT_HASHNotes
run()now returnsErrinstead of callingprocess::exit(1)— library functions should not terminate the processbuild-uifeature stub added to satisfyaisix-ee's Cargo.toml feature requirement when using local path patch during developmentSummary by CodeRabbit
New Features
Chores
Refactor