mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
perf(config): defer remote sandbox hostname lookup (#28542)
## Why [#18763](https://github.com/openai/codex/pull/18763) added canonical hostname resolution for `remote_sandbox_config`. Requirements composition currently performs that synchronous DNS lookup on every fresh process, even when none of the loaded requirements layers contains `[[remote_sandbox_config]]`. On hosts with slow local DNS resolution, this can add several seconds to Codex startup. ## What - defer hostname resolution until a parsed requirements layer actually contains `remote_sandbox_config` - cache the resolver result once per requirements composition, preserving the existing single-lookup behavior across multiple layers - keep the existing FQDN resolution and per-layer requirements precedence unchanged - cover both the ordinary no-lookup path and the multi-layer single-lookup path ## How to Test On a host where local canonical-name resolution is slow: 1. Start Codex without `[[remote_sandbox_config]]` in any managed requirements layer and confirm startup no longer waits for hostname resolution. 2. Add a matching `[[remote_sandbox_config]]` entry and confirm its `allowed_sandbox_modes` still overrides the layer's top-level value. 3. Add remote sandbox entries to multiple requirements layers and confirm precedence remains unchanged while the hostname is resolved only once. Targeted tests: - `just test -p codex-config hostname_resolver` - `just test -p codex-config` (181 passed)
This commit is contained in:
1 parent
1b24ba912a
commit
40e7dda4d2
4 files changed
+128
-10
No files matched your search
@@ -10,6 +10,8 @@ use winapi_util::sysinfo::get_computer_name;
|
|||||||
|
|
||||||
static HOST_NAME: LazyLock<Option<String>> = LazyLock::new(compute_host_name);
|
static HOST_NAME: LazyLock<Option<String>> = LazyLock::new(compute_host_name);
|
||||||
|
|
||||||
|
/// Returns a process-cached canonical hostname, falling back to the normalized
|
||||||
|
/// kernel hostname. The first call on Unix may perform blocking DNS resolution.
|
||||||
pub fn host_name() -> Option<String> {
|
pub fn host_name() -> Option<String> {
|
||||||
HOST_NAME.clone()
|
HOST_NAME.clone()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -54,7 +54,7 @@ pub(super) struct ComposableRequirementsLayer {
|
|||||||
impl ComposableRequirementsLayer {
|
impl ComposableRequirementsLayer {
|
||||||
pub(super) fn from_entry(
|
pub(super) fn from_entry(
|
||||||
layer: RequirementsLayerEntry,
|
layer: RequirementsLayerEntry,
|
||||||
hostname: Option<&str>,
|
hostname_resolver: &dyn Fn() -> Option<String>,
|
||||||
) -> Result<Self, RequirementsCompositionError> {
|
) -> Result<Self, RequirementsCompositionError> {
|
||||||
let RequirementsLayerEntry {
|
let RequirementsLayerEntry {
|
||||||
source,
|
source,
|
||||||
@@ -70,7 +70,13 @@ impl ComposableRequirementsLayer {
|
|||||||
(regular_toml, requirements)
|
(regular_toml, requirements)
|
||||||
};
|
};
|
||||||
|
|
||||||
requirements.apply_remote_sandbox_config(hostname);
|
// Hostname lookup is configuration-driven and may block on DNS, so only
|
||||||
|
// resolve it when this layer contains hostname-based sandbox selectors.
|
||||||
|
let hostname = requirements
|
||||||
|
.remote_sandbox_config
|
||||||
|
.as_ref()
|
||||||
|
.and_then(|_| hostname_resolver());
|
||||||
|
requirements.apply_remote_sandbox_config(hostname.as_deref());
|
||||||
materialize_remote_sandbox_config(&mut regular_toml, &requirements)?;
|
materialize_remote_sandbox_config(&mut regular_toml, &requirements)?;
|
||||||
strip_special_fields(&mut regular_toml);
|
strip_special_fields(&mut regular_toml);
|
||||||
|
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ use crate::ConfigRequirementsWithSources;
|
|||||||
use crate::RequirementSource;
|
use crate::RequirementSource;
|
||||||
use crate::Sourced;
|
use crate::Sourced;
|
||||||
use crate::merge::merge_toml_values;
|
use crate::merge::merge_toml_values;
|
||||||
|
use std::cell::OnceCell;
|
||||||
use std::io;
|
use std::io;
|
||||||
use thiserror::Error;
|
use thiserror::Error;
|
||||||
use toml::Value as TomlValue;
|
use toml::Value as TomlValue;
|
||||||
@@ -57,29 +58,59 @@ impl From<RequirementsCompositionError> for io::Error {
|
|||||||
pub fn compose_requirements(
|
pub fn compose_requirements(
|
||||||
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
||||||
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
||||||
let hostname = crate::host_name();
|
compose_requirements_with_hostname_resolver(layers, crate::host_name)
|
||||||
compose_requirements_for_hostname(layers, hostname.as_deref())
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
pub(super) fn compose_requirements_for_hostname(
|
pub(super) fn compose_requirements_for_hostname(
|
||||||
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
||||||
hostname: Option<&str>,
|
hostname: Option<&str>,
|
||||||
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
||||||
compose_requirements_for_hostname_and_hook_directory(
|
let hostname = hostname.map(str::to_string);
|
||||||
|
compose_requirements_with_hostname_resolver_and_hook_directory(
|
||||||
layers,
|
layers,
|
||||||
hostname,
|
move || hostname.clone(),
|
||||||
HookDirectoryField::current_platform(),
|
HookDirectoryField::current_platform(),
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
pub(super) fn compose_requirements_for_hostname_and_hook_directory(
|
pub(super) fn compose_requirements_for_hostname_and_hook_directory(
|
||||||
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
||||||
hostname: Option<&str>,
|
hostname: Option<&str>,
|
||||||
hook_directory_field: HookDirectoryField,
|
hook_directory_field: HookDirectoryField,
|
||||||
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
||||||
|
let hostname = hostname.map(str::to_string);
|
||||||
|
compose_requirements_with_hostname_resolver_and_hook_directory(
|
||||||
|
layers,
|
||||||
|
move || hostname.clone(),
|
||||||
|
hook_directory_field,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn compose_requirements_with_hostname_resolver(
|
||||||
|
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
||||||
|
hostname_resolver: impl Fn() -> Option<String>,
|
||||||
|
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
||||||
|
compose_requirements_with_hostname_resolver_and_hook_directory(
|
||||||
|
layers,
|
||||||
|
hostname_resolver,
|
||||||
|
HookDirectoryField::current_platform(),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn compose_requirements_with_hostname_resolver_and_hook_directory(
|
||||||
|
layers: impl IntoIterator<Item = RequirementsLayerEntry>,
|
||||||
|
hostname_resolver: impl Fn() -> Option<String>,
|
||||||
|
hook_directory_field: HookDirectoryField,
|
||||||
|
) -> Result<Option<ConfigRequirementsWithSources>, RequirementsCompositionError> {
|
||||||
|
// Evaluate every layer in this composition against the same hostname while
|
||||||
|
// keeping resolution lazy when no layer needs remote sandbox matching.
|
||||||
|
let hostname = OnceCell::new();
|
||||||
|
let cached_hostname_resolver = || hostname.get_or_init(&hostname_resolver).clone();
|
||||||
let mut stack = RequirementsLayerStack::new(hook_directory_field);
|
let mut stack = RequirementsLayerStack::new(hook_directory_field);
|
||||||
for layer in layers {
|
for layer in layers {
|
||||||
stack.add_layer(layer, hostname)?;
|
stack.add_layer(layer, &cached_hostname_resolver)?;
|
||||||
}
|
}
|
||||||
stack.compose()
|
stack.compose()
|
||||||
}
|
}
|
||||||
@@ -100,10 +131,12 @@ impl RequirementsLayerStack {
|
|||||||
fn add_layer(
|
fn add_layer(
|
||||||
&mut self,
|
&mut self,
|
||||||
layer: RequirementsLayerEntry,
|
layer: RequirementsLayerEntry,
|
||||||
hostname: Option<&str>,
|
hostname_resolver: &dyn Fn() -> Option<String>,
|
||||||
) -> Result<(), RequirementsCompositionError> {
|
) -> Result<(), RequirementsCompositionError> {
|
||||||
self.layers
|
self.layers.push(ComposableRequirementsLayer::from_entry(
|
||||||
.push(ComposableRequirementsLayer::from_entry(layer, hostname)?);
|
layer,
|
||||||
|
hostname_resolver,
|
||||||
|
)?);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ use super::super::hooks::HookDirectoryField;
|
|||||||
use super::RequirementsCompositionError;
|
use super::RequirementsCompositionError;
|
||||||
use super::compose_requirements_for_hostname;
|
use super::compose_requirements_for_hostname;
|
||||||
use super::compose_requirements_for_hostname_and_hook_directory;
|
use super::compose_requirements_for_hostname_and_hook_directory;
|
||||||
|
use super::compose_requirements_with_hostname_resolver;
|
||||||
use crate::ConfigRequirementsToml;
|
use crate::ConfigRequirementsToml;
|
||||||
use crate::ConfigRequirementsWithSources;
|
use crate::ConfigRequirementsWithSources;
|
||||||
use crate::RequirementSource;
|
use crate::RequirementSource;
|
||||||
@@ -10,6 +11,7 @@ use crate::Sourced;
|
|||||||
use codex_protocol::protocol::AskForApproval;
|
use codex_protocol::protocol::AskForApproval;
|
||||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
|
use std::cell::Cell;
|
||||||
use std::collections::BTreeMap;
|
use std::collections::BTreeMap;
|
||||||
|
|
||||||
fn layer(id: &str, name: &str, contents: &str) -> RequirementsLayerEntry {
|
fn layer(id: &str, name: &str, contents: &str) -> RequirementsLayerEntry {
|
||||||
@@ -552,6 +554,81 @@ allowed_sandbox_modes = ["read-only"]
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn hostname_resolver_is_not_called_without_remote_sandbox_config() {
|
||||||
|
let calls = Cell::<usize>::default();
|
||||||
|
let composed = compose_requirements_with_hostname_resolver(
|
||||||
|
vec![layer(
|
||||||
|
"req",
|
||||||
|
"No remote selector",
|
||||||
|
r#"
|
||||||
|
allowed_sandbox_modes = ["read-only"]
|
||||||
|
"#,
|
||||||
|
)],
|
||||||
|
|| {
|
||||||
|
calls.set(calls.get() + 1);
|
||||||
|
Some("build-01.example.com".to_string())
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.expect("compose requirements")
|
||||||
|
.expect("requirements present")
|
||||||
|
.into_toml();
|
||||||
|
|
||||||
|
assert_eq!(calls.get(), 0);
|
||||||
|
assert_eq!(
|
||||||
|
composed,
|
||||||
|
expected_requirements(
|
||||||
|
r#"
|
||||||
|
allowed_sandbox_modes = ["read-only"]
|
||||||
|
"#
|
||||||
|
)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn hostname_resolver_is_called_once_for_multiple_remote_sandbox_layers() {
|
||||||
|
let calls = Cell::<usize>::default();
|
||||||
|
let composed = compose_requirements_with_hostname_resolver(
|
||||||
|
vec![
|
||||||
|
layer(
|
||||||
|
"req_low",
|
||||||
|
"Low",
|
||||||
|
r#"
|
||||||
|
[[remote_sandbox_config]]
|
||||||
|
hostname_patterns = ["build-*.example.com"]
|
||||||
|
allowed_sandbox_modes = ["read-only"]
|
||||||
|
"#,
|
||||||
|
),
|
||||||
|
layer(
|
||||||
|
"req_high",
|
||||||
|
"High",
|
||||||
|
r#"
|
||||||
|
[[remote_sandbox_config]]
|
||||||
|
hostname_patterns = ["build-*.example.com"]
|
||||||
|
allowed_sandbox_modes = ["workspace-write"]
|
||||||
|
"#,
|
||||||
|
),
|
||||||
|
],
|
||||||
|
|| {
|
||||||
|
calls.set(calls.get() + 1);
|
||||||
|
Some("build-01.example.com".to_string())
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.expect("compose requirements")
|
||||||
|
.expect("requirements present")
|
||||||
|
.into_toml();
|
||||||
|
|
||||||
|
assert_eq!(calls.get(), 1);
|
||||||
|
assert_eq!(
|
||||||
|
composed,
|
||||||
|
expected_requirements(
|
||||||
|
r#"
|
||||||
|
allowed_sandbox_modes = ["workspace-write"]
|
||||||
|
"#
|
||||||
|
)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn rules_are_appended_in_priority_order() {
|
fn rules_are_appended_in_priority_order() {
|
||||||
let composed = compose(vec![
|
let composed = compose(vec![
|
||||||
|
|||||||
Reference in new issue
Block a user