Skip to content

feat(agentd): introduce AgentdConfig to read env vars once at startup#507

Open
dijdzv wants to merge 3 commits intosuperradcompany:mainfrom
dijdzv:fix/flaky-pty-reader-test
Open

feat(agentd): introduce AgentdConfig to read env vars once at startup#507
dijdzv wants to merge 3 commits intosuperradcompany:mainfrom
dijdzv:fix/flaky-pty-reader-test

Conversation

@dijdzv
Copy link
Copy Markdown
Contributor

@dijdzv dijdzv commented Apr 5, 2026

Summary

test_pty_reader_drains_ready_fd fails intermittently (~10%) under cargo test in crates/agentd/. test_request_user_overrides_env_default sets MSB_USER=0:0 via set_var, which leaks into the parallel PTY test's fork() — the child picks it up, attempts a privilege switch, and _exit(1)s.

The root cause is that resolve_requested_user reads ENV_USER as a fallback when ExecRequest.user is None. But the SDK already resolves config.user into ExecRequest.user in build_exec_request(), so this env var fallback never fires in normal SDK/CLI usage — it's effectively dead code.

Changes

  • session.rs: Remove ENV_USER fallback from resolve_requested_user, relying solely on ExecRequest.user
  • session.rs: Remove test_request_user_overrides_env_default (the code path it tested was removed above)
  • session.rs: Reduce PTY test sleep (1s → 0.1s) — original delay was unnecessary for verifying output ordering

Verification

Before: 3 failures in 30 runs
After: 0 failures in 80 runs

@appcypher
Copy link
Copy Markdown
Member

appcypher commented Apr 5, 2026

You are right that ENV_USER seems unnecessary because we use SandboxConfig.user anyway. But ENV_USER serves as a defense-in-depth mechanism on the guest side.

ENV_USER is a sandbox-level property on the guest, not an exec-level one, and even if the host forgets to set it in their ExecRequest, the guest should always use their ENV_USER from sandbox creation.

As for the flaky PTY failures, I believe the root cause is reading/setting the same env vars in multiple unit tests, which is thread unsafe. So there is an opportunity for a proper fix here.

Maybe we should introduce AgentdConfig where the env vars are read into when agentd starts so we don't have to go through std::env::var every single time.

@dijdzv
Copy link
Copy Markdown
Contributor Author

dijdzv commented Apr 6, 2026

Replaced the ENV_USER removal with AgentdConfig — reads all MSB_* env vars once at startup. This preserves the ENV_USER defense-in-depth fallback while eliminating the repeated std::env::var calls that caused the flaky test.

dijdzv added 3 commits April 6, 2026 17:33
The SDK already resolves config.user into ExecRequest.user before sending — this env var fallback never fires in normal SDK/CLI usage and caused an env race that made the PTY test flaky.
@dijdzv dijdzv force-pushed the fix/flaky-pty-reader-test branch from b487fab to 7ec3813 Compare April 6, 2026 17:37
@dijdzv dijdzv changed the title fix(agentd): remove dead ENV_USER fallback that caused flaky PTY test feat(agentd): introduce AgentdConfig to read env vars once at startup Apr 6, 2026
Comment on lines +52 to +63
pub fn from_env() -> Self {
Self {
block_root: read_env(ENV_BLOCK_ROOT),
mounts: read_env(ENV_MOUNTS),
tmpfs: read_env(ENV_TMPFS),
hostname: read_env(ENV_HOSTNAME),
net: read_env(ENV_NET),
net_ipv4: read_env(ENV_NET_IPV4),
net_ipv6: read_env(ENV_NET_IPV6),
user: read_env(ENV_USER),
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just one more nit 😅.

It would be nice if all the env parsing operations are moved into this file as well.

So we parse all the env vars once in from_env and fields in AgentdConfig can be the actual types. Something like

pub(crate) struct AgentdConfig<'a> { 
    block_root: Option<BlockRootSpec<'a>>, 
    mounts: Option<VolumeMountSpec<'a>>, 
    ... 
}`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants