Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ dashmap = "6.1.0"
io-uring = "0.7.9"
tokio-tungstenite = "0.28"
sha2 = "0.10"
hmac = "0.12"
hex = "0.4"
semver = "1"
iroh = { version = "=1.0.0-rc.0" }
iroh-blobs = { version = "=0.101.0" }
Expand Down
6 changes: 6 additions & 0 deletions config/default.toml
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,12 @@ init_timeout_secs = 60
# envd readiness polling interval in milliseconds.
poll_ms = 3

[sandbox]
# Optional secret used to derive per-sandbox envd access tokens. When unset,
# AgentENV creates a node-local seed under $AENV_HOME/secrets. Configure the
# same explicit value on every node before enabling cross-node sandbox recovery.
# access_token_hash_seed = "replace-with-a-secret"

[orchestrator]
# Expired-sandbox eviction interval in milliseconds (1000 = 1 second).
auto_evict_interval_ms = 1000
Expand Down
24 changes: 21 additions & 3 deletions crates/aenv/src/client/files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::progress::TransferProgress;
const API_KEY_HEADER: &str = "X-API-Key";
const SANDBOX_ID_HEADER: &str = "x-agentenv-sandbox-id";
const TARGET_PORT_HEADER: &str = "x-agentenv-target-port";
const ACCESS_TOKEN_HEADER: &str = "X-Access-Token";
const FILESYSTEM_SERVICE: &str = "filesystem.Filesystem";
const MAX_ERROR_BODY_BYTES: usize = 64 * 1024;
pub(crate) const TRANSFER_IDLE_TIMEOUT: Duration = Duration::from_secs(60);
Expand All @@ -31,7 +32,12 @@ pub struct EnvdFilesClient {
}

impl EnvdFilesClient {
fn new(base_url: &str, api_key: &str, sandbox_id: &str) -> Result<Self> {
fn new(
base_url: &str,
api_key: &str,
sandbox_id: &str,
envd_access_token: Option<&str>,
) -> Result<Self> {
let mut headers = HeaderMap::new();
headers.insert(
API_KEY_HEADER,
Expand All @@ -42,6 +48,12 @@ impl EnvdFilesClient {
HeaderValue::from_str(sandbox_id).context("invalid sandbox ID header value")?,
);
headers.insert(TARGET_PORT_HEADER, HeaderValue::from_static(ENVD_PORT_STR));
if let Some(token) = envd_access_token {
let mut token =
HeaderValue::from_str(token).context("invalid envd access token header value")?;
token.set_sensitive(true);
headers.insert(ACCESS_TOKEN_HEADER, token);
}
Comment thread
LSX-s-Software marked this conversation as resolved.
headers.insert(
USER_AGENT,
HeaderValue::from_str(&format!("aenv/{}", env!("CARGO_PKG_VERSION")))
Expand All @@ -60,7 +72,7 @@ impl EnvdFilesClient {
Ok(Self {
base_url: base_url.trim_end_matches('/').to_string(),
http: client,
transport: Transport::new(base_url, api_key, sandbox_id)?,
transport: Transport::new(base_url, api_key, sandbox_id, envd_access_token)?,
})
}

Expand Down Expand Up @@ -298,7 +310,13 @@ fn format_envd_response_error(status: reqwest::StatusCode, content: &str) -> any

impl Client {
pub fn files(&self, sandbox_id: &str) -> Result<EnvdFilesClient> {
EnvdFilesClient::new(&self.base, &self.api_key, sandbox_id)
let sandbox = self.get_sandbox(sandbox_id)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[performance · medium]
get_sandbox performs a synchronous ureq network request (with the client's potentially long request timeout). Both current callers invoke files() from async run_async functions, so this blocks a Tokio worker thread and can stall unrelated tasks/progress handling. Make token resolution asynchronous (for example, expose an async files constructor using an async HTTP client or spawn_blocking) or fetch the sandbox metadata before entering the runtime.

Suggestion:

Suggested change
let sandbox = self.get_sandbox(sandbox_id)?;
// Resolve sandbox metadata asynchronously before constructing this client.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[performance · medium]
This introduces blocking ureq network I/O inside Client::files. Both current callers invoke files() from run_async on the CLI's single-threaded Tokio runtime, so this request blocks the runtime (and cannot be cancelled) for up to the agent timeout. Keep this constructor free of I/O by passing the already-fetched access token into it, or fetch sandbox details before entering block_on; alternatively, use an async HTTP client and make this API async.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

performance · medium
get_sandbox uses the synchronous ureq client, so this newly added call performs a blocking network request inside upload/download's async run_async task. A slow or unreachable control-plane request can block the Tokio runtime for the ureq timeout (up to 120 seconds), delaying unrelated async work. Resolve the token before entering the async runtime, or provide/use an async sandbox-detail request instead.

Suggestion:

Suggested change
let sandbox = self.get_sandbox(sandbox_id)?;
let sandbox = self.get_sandbox_async(sandbox_id).await?;

EnvdFilesClient::new(
Comment on lines +313 to +314

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

performance · medium
get_sandbox performs a synchronous ureq network request, but files() is called from the async upload/download paths. This newly introduced call therefore blocks the Tokio runtime thread for up to the client's connect/request timeout, potentially stalling other async work (and is especially problematic on a single-thread runtime). Fetch the sandbox detail before entering run_async, or execute this synchronous lookup via spawn_blocking/provide an async client method.

&self.base,
&self.api_key,
sandbox_id,
sandbox.envd_access_token.as_deref(),
)
}
}

Expand Down
8 changes: 6 additions & 2 deletions crates/aenv/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,12 @@ impl Client {
})
}

pub fn transport(&self, sandbox_id: &str) -> Result<Transport> {
Transport::new(&self.base, &self.api_key, sandbox_id)
pub fn transport(
&self,
sandbox_id: &str,
envd_access_token: Option<&str>,
) -> Result<Transport> {
Transport::new(&self.base, &self.api_key, sandbox_id, envd_access_token)
}

fn url(&self, path: &str) -> String {
Expand Down
49 changes: 31 additions & 18 deletions crates/aenv/src/client/sandboxes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ pub struct NewSandbox<'a> {
pub template_id: &'a str,
#[serde(skip_serializing_if = "Option::is_none")]
pub timeout: Option<u32>,
#[serde(skip_serializing_if = "Option::is_none")]
pub secure: Option<bool>,
}

#[derive(Debug, Serialize)]
Expand All @@ -23,12 +25,16 @@ pub struct NewColdSandbox<'a> {
pub memory_mb: Option<u32>,
#[serde(skip_serializing_if = "Option::is_none", rename = "diskSizeMB")]
pub disk_size_mb: Option<u32>,
#[serde(skip_serializing_if = "Option::is_none")]
pub secure: Option<bool>,
}

#[derive(Debug, Deserialize)]
#[derive(Deserialize)]
pub struct Sandbox {
#[serde(rename = "sandboxID")]
pub sandbox_id: String,
#[serde(default, rename = "envdAccessToken")]
pub envd_access_token: Option<String>,
}

#[derive(Debug, Serialize)]
Expand All @@ -37,9 +43,11 @@ pub struct RefreshSandbox {
pub duration: Option<u32>,
}

#[derive(Debug, Deserialize)]
#[derive(Deserialize)]
pub struct SandboxDetail {
pub state: String,
#[serde(default, rename = "envdAccessToken")]
pub envd_access_token: Option<String>,
}

#[derive(Debug, Deserialize, Serialize)]
Expand All @@ -65,14 +73,20 @@ pub struct ListedSandbox {
}

impl Client {
pub fn create_sandbox(&self, template_id: &str, timeout: Option<u32>) -> Result<String> {
pub fn create_sandbox(
&self,
template_id: &str,
timeout: Option<u32>,
secure: bool,
) -> Result<Sandbox> {
let body = NewSandbox {
template_id,
timeout,
secure: secure.then_some(true),
};
let resp = handle_status(self.post("/sandboxes").send_json(&body))?;
let sandbox: Sandbox = resp.into_json()?;
Ok(sandbox.sandbox_id)
Ok(sandbox)
}

pub fn create_cold_sandbox(
Expand All @@ -82,17 +96,19 @@ impl Client {
cpu_count: Option<u32>,
memory_mb: Option<u32>,
disk_size_mb: Option<u32>,
) -> Result<String> {
secure: bool,
) -> Result<Sandbox> {
let body = NewColdSandbox {
image,
timeout,
cpu_count,
memory_mb,
disk_size_mb,
secure: secure.then_some(true),
};
let resp = handle_status(self.post("/sandboxes-cold").send_json(&body))?;
let sandbox: Sandbox = resp.into_json()?;
Ok(sandbox.sandbox_id)
Ok(sandbox)
}

pub fn list_sandboxes(&self) -> Result<Vec<ListedSandbox>> {
Expand Down Expand Up @@ -128,6 +144,11 @@ impl Client {
Ok(Some(detail.state))
}

pub fn get_sandbox(&self, id: &str) -> Result<SandboxDetail> {
let resp = handle_status(self.get(&format!("/sandboxes/{id}")).call())?;
Ok(resp.into_json()?)
}

/// `connect` resumes a paused sandbox or extends the TTL of a running one.
pub fn connect_sandbox(&self, id: &str, timeout: u32) -> Result<Sandbox> {
let resp = handle_status(
Expand All @@ -153,18 +174,6 @@ impl Client {
)?;
Ok(())
}

pub async fn envd_ready_with_timeout(
&self,
sandbox_id: &str,
timeout: Duration,
) -> Result<bool> {
let transport = self.transport(sandbox_id)?;
match tokio::time::timeout(timeout, transport.ready()).await {
Ok(Ok(())) => Ok(true),
Ok(Err(_)) | Err(_) => Ok(false),
}
}
}

#[cfg(test)]
Expand All @@ -176,11 +185,13 @@ mod tests {
let body = NewSandbox {
template_id: "base-template",
timeout: Some(300),
secure: Some(true),
};

let value = serde_json::to_value(body).unwrap();
assert_eq!(value["templateID"], "base-template");
assert_eq!(value["timeout"], 300);
assert_eq!(value["secure"], true);
assert!(value.get("cpuCount").is_none());
assert!(value.get("memoryMB").is_none());
}
Expand All @@ -193,6 +204,7 @@ mod tests {
cpu_count: Some(2),
memory_mb: Some(1024),
disk_size_mb: Some(8192),
secure: Some(true),
};

let value = serde_json::to_value(body).unwrap();
Expand All @@ -201,6 +213,7 @@ mod tests {
assert_eq!(value["cpuCount"], 2);
assert_eq!(value["memoryMB"], 1024);
assert_eq!(value["diskSizeMB"], 8192);
assert_eq!(value["secure"], true);
assert!(value.get("templateID").is_none());
}

Expand Down
19 changes: 11 additions & 8 deletions crates/aenv/src/commands/connect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ pub fn run(args: Args) -> Result<()> {
}

pub(crate) async fn attach(client: &Client, sandbox_id: &str) -> Result<i32> {
client.connect_sandbox(sandbox_id, DEFAULT_TIMEOUT_SECS)?;
let sandbox = client.connect_sandbox(sandbox_id, DEFAULT_TIMEOUT_SECS)?;

let (cols, rows) = terminal_size();
let transport = Arc::new(client.transport(sandbox_id)?);
let transport = Arc::new(client.transport(sandbox_id, sandbox.envd_access_token.as_deref())?);
let mut last_error = None;
let mut started = None;
for shell in ["/bin/bash", "/bin/sh"] {
Expand Down Expand Up @@ -116,10 +116,11 @@ pub(crate) async fn attach(client: &Client, sandbox_id: &str) -> Result<i32> {
reconnect_rx,
reconnect_ack_tx,
);
let resize_task = resize_loop(transport, selector, state.clone());
let resize_task = resize_loop(transport.clone(), selector, state.clone());
let watchdog_task = watchdog_loop(
client.clone(),
sandbox_id.to_string(),
transport,
state.clone(),
reconnect_tx,
reconnect_ack_rx,
Expand Down Expand Up @@ -681,6 +682,7 @@ async fn resize_loop(
async fn watchdog_loop(
client: Client,
sandbox_id: String,
transport: Arc<Transport>,
state: SessionState,
reconnect_tx: mpsc::UnboundedSender<()>,
mut reconnect_ack_rx: mpsc::UnboundedReceiver<()>,
Expand Down Expand Up @@ -722,7 +724,7 @@ async fn watchdog_loop(
});
}

match envd_ready_probe(client.clone(), sandbox_id.clone()).await {
match envd_ready_probe(Arc::clone(&transport)).await {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[performance · low]
The readiness probe does not retain or share ownership of the transport, so cloning the Arc on every watchdog iteration adds an unnecessary atomic reference-count operation. Borrow the transport for the duration of the awaited probe instead (envd_ready_probe(&transport) and async fn envd_ready_probe(transport: &Transport)).

Suggestion:

Suggested change
match envd_ready_probe(Arc::clone(&transport)).await {
match envd_ready_probe(&transport).await {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[performance · low]
The probe only borrows the transport while awaiting ready(), so cloning the Arc on every watchdog iteration is unnecessary. Accept &Transport (or &Arc<Transport>) in envd_ready_probe and pass transport.as_ref() here; this avoids repeated atomic reference-count updates and expresses the ownership requirement more clearly.

Suggestion:

Suggested change
match envd_ready_probe(Arc::clone(&transport)).await {
match envd_ready_probe(transport.as_ref()).await {

Ok(true) => {
unhealthy_since = None;
if matches!(previous_health, ProbeStatus::Unhealthy) {
Expand Down Expand Up @@ -809,10 +811,11 @@ fn request_reconnect_once(
*recovered_health = false;
}

async fn envd_ready_probe(client: Client, sandbox_id: String) -> Result<bool> {
client
.envd_ready_with_timeout(&sandbox_id, RECONNECT_PROBE_TIMEOUT)
.await
async fn envd_ready_probe(transport: Arc<Transport>) -> Result<bool> {
match tokio::time::timeout(RECONNECT_PROBE_TIMEOUT, transport.ready()).await {
Comment on lines +814 to +815

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

performance · low
The probe only borrows Transport, so taking an Arc by value forces an unnecessary atomic refcount increment on every watchdog iteration (Arc::clone(&transport)). Accept &Transport instead and call it as envd_ready_probe(&transport); the borrow remains valid for the awaited call because the watchdog owns the Arc.

Suggestion:

Suggested change
async fn envd_ready_probe(transport: Arc<Transport>) -> Result<bool> {
match tokio::time::timeout(RECONNECT_PROBE_TIMEOUT, transport.ready()).await {
async fn envd_ready_probe(transport: &Transport) -> Result<bool> {
match tokio::time::timeout(RECONNECT_PROBE_TIMEOUT, transport.ready()).await {

Ok(Ok(())) => Ok(true),
Ok(Err(_)) | Err(_) => Ok(false),
}
}

fn should_refresh_session_keepalive(state: &SessionState, last_activity: &mut u64) -> bool {
Expand Down
3 changes: 2 additions & 1 deletion crates/aenv/src/commands/exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ async fn run_async(client: Client, args: Args) -> Result<i32> {
.ok_or_else(|| anyhow::anyhow!("missing command"))?;
let rest: Vec<String> = cmd_iter.collect();

let transport = client.transport(&args.sandbox_id)?;
let sandbox = client.get_sandbox(&args.sandbox_id)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[performance · medium]
get_sandbox performs a synchronous HTTP request (ureq::Request::call) from inside this async function, which can block the Tokio runtime thread while the control-plane request is in flight. Fetch the sandbox before entering block_on, or run this call via spawn_blocking (using an owned/cloned client and ID) so unrelated async work is not stalled.

Suggestion:

Suggested change
let sandbox = client.get_sandbox(&args.sandbox_id)?;
let sandbox_id = args.sandbox_id.clone();
let sandbox = tokio::task::spawn_blocking({
let client = client.clone();
move || client.get_sandbox(&sandbox_id)
})
.await??;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

performance · medium
get_sandbox performs a synchronous ureq network request, but this function is running inside Tokio's async runtime. A slow or stalled API response can block the runtime thread for the client's timeout (up to 120 seconds), delaying the subsequent envd stream and any other runtime work. Fetch the sandbox detail before entering run_async, or execute this blocking request via spawn_blocking/an async HTTP client.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

performance · medium
get_sandbox performs a synchronous ureq network request, but this call is now made inside run_async before the first .await. That blocks the Tokio executor for the request timeout (up to the configured 120 seconds), and would stall any other work sharing this runtime. Fetch the sandbox/token before entering block_on, use an async HTTP client, or move this blocking operation to tokio::task::spawn_blocking.

Suggestion:

Suggested change
let sandbox = client.get_sandbox(&args.sandbox_id)?;
let sandbox = tokio::task::spawn_blocking({
let client = client.clone();
let sandbox_id = args.sandbox_id.clone();
move || client.get_sandbox(&sandbox_id)
})
.await??;

let transport = client.transport(&args.sandbox_id, sandbox.envd_access_token.as_deref())?;
Comment on lines +30 to +31

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[performance · medium]
get_sandbox performs a synchronous ureq network call (with a timeout of up to 120 seconds) inside run_async, which runs on the CLI's current-thread Tokio runtime. This can block the runtime and prevent async cancellation or other runtime work from progressing. Fetch the sandbox before entering block_on and pass its access token into run_async, or provide an async HTTP implementation for this request.

Suggestion:

Suggested change
let sandbox = client.get_sandbox(&args.sandbox_id)?;
let transport = client.transport(&args.sandbox_id, sandbox.envd_access_token.as_deref())?;
let transport = client.transport(&args.sandbox_id, envd_access_token.as_deref())?;

let req = build_start_request(StartOpts {
cmd: &cmd,
args: rest,
Expand Down
30 changes: 23 additions & 7 deletions crates/aenv/src/commands/start.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ pub struct Args {
/// Start directly from an external OCI image instead of a template/snapshot
#[arg(long)]
cold: bool,
/// Require an envd access token for sandbox control communication
#[arg(long)]
secure: bool,
/// Sandbox TTL in seconds
#[arg(long, default_value_t = super::DEFAULT_TIMEOUT_SECS)]
timeout: u32,
Expand All @@ -46,20 +49,22 @@ fn parse_disk_size_mb(value: &str) -> std::result::Result<u32, String> {

pub fn run(args: Args) -> Result<()> {
let client = Client::from_env()?;
let sandbox_id = if args.cold {
let sandbox = if args.cold {
client.create_cold_sandbox(
&args.target,
Some(args.timeout),
args.resources.cpu_count,
args.resources.memory_mb,
args.disk_size_mb,
args.secure,
)?
} else {
if args.resources.is_set() || args.disk_size_mb.is_some() {
anyhow::bail!("--cpu-count, --memory-mb, and --disk-size-mb require --cold");
}
client.create_sandbox(&args.target, Some(args.timeout))?
client.create_sandbox(&args.target, Some(args.timeout), args.secure)?
};
let sandbox_id = sandbox.sandbox_id;

if args.detach {
println!("{}", sandbox_id);
Expand All @@ -68,18 +73,29 @@ pub fn run(args: Args) -> Result<()> {

println!("Started sandbox {}", sandbox_id);
let rt = super::tokio_rt()?;
rt.block_on(wait_for_envd(&client, &sandbox_id))?;
rt.block_on(wait_for_envd(
&client,
&sandbox_id,
sandbox.envd_access_token.as_deref(),
))?;
let code = rt.block_on(super::connect::attach(&client, &sandbox_id))?;
std::process::exit(code);
}

async fn wait_for_envd(client: &Client, sandbox_id: &str) -> Result<()> {
async fn wait_for_envd(
client: &Client,
sandbox_id: &str,
envd_access_token: Option<&str>,
) -> Result<()> {
let deadline = Instant::now() + ENVD_READY_TIMEOUT;
while Instant::now() < deadline {
if matches!(
client
.envd_ready_with_timeout(sandbox_id, ENVD_READY_PROBE_TIMEOUT)
.await,
tokio::time::timeout(
ENVD_READY_PROBE_TIMEOUT,
client.transport(sandbox_id, envd_access_token)?.ready(),
)
.await
.map(|result| result.is_ok()),
Ok(true)
) {
return Ok(());
Expand Down
Loading
Loading