feat(wrap): make PTY ring-buffer configurable via env vars#1166
feat(wrap): make PTY ring-buffer configurable via env vars#1166
Conversation
geoffjay
left a comment
There was a problem hiding this comment.
Review: feat(wrap) — PTY ring-buffer configurable via env vars
Stack position: Standalone — branches directly off feature/autonomous-pipeline (confirmed via git-spice log short). No parent PR to review first.
🔴 Blocking — zero-capacity panic on AGENTD_WRAP_CHANNEL_CAPACITY=0
tokio::sync::broadcast::channel panics when capacity is 0 (documented requirement: capacity ≥ 1). The current parsing chain:
let channel_capacity = env::var("AGENTD_WRAP_CHANNEL_CAPACITY")
.ok()
.and_then(|v| v.parse::<usize>().ok())
.unwrap_or(DEFAULT_CHANNEL_CAPACITY);...does not protect against 0. parse::<usize>().ok() returns Some(0) for the string "0", so unwrap_or is never reached. The 0 flows through PtyBackend::new_with_config → PtyOutputStream::new → broadcast::channel(0) at pty_stream.rs:108. The service panics on the first call to create_session, not at startup — making it a silent runtime trap.
Fix — fail loudly at startup (preferred; matches how DockerBackend misconfiguration is handled in the same file):
let channel_capacity = env::var("AGENTD_WRAP_CHANNEL_CAPACITY")
.ok()
.and_then(|v| v.parse::<usize>().ok())
.unwrap_or(DEFAULT_CHANNEL_CAPACITY);
if channel_capacity == 0 {
anyhow::bail!("AGENTD_WRAP_CHANNEL_CAPACITY must be at least 1 (broadcast::channel panics on 0)");
}Or clamp silently with .map(|v| v.max(1)) before unwrap_or if silent recovery is preferred.
Note: AGENTD_WRAP_HISTORY_BYTES=0 is safe — the ring buffer degrades to no-replay without panicking.
Also needs a test — e.g., verify that new_with_config("t", 0, 1024) either panics-with-message or is rejected before construction reaches PtyOutputStream::new.
🟡 Non-blocking — orchestrator binary uses PtyBackend::new() with hardcoded defaults
crates/orchestrator/src/main.rs:103 also constructs PtyBackend when AGENTD_BACKEND=pty:
Arc::new(PtyBackend::new("agentd-orch")) // still uses hardcoded defaultsSetting AGENTD_WRAP_HISTORY_BYTES / AGENTD_WRAP_CHANNEL_CAPACITY has no effect on the orchestrator's PTY backend. If this is intentional scope, update the doc comments to say "wrap service only". Otherwise, file a follow-up issue — don't expand scope here.
🟡 Non-blocking — silent fallback for unparseable env var values
AGENTD_WRAP_HISTORY_BYTES=2gb silently falls to the default with no log output. A tracing::warn! on the None branch would help operators debug misconfiguration. Easy to add alongside the fix above.
✅ Everything else looks correct
new_with_configcleanly delegates fromnew()— backward compat preserved, all existing tests unaffected.- Private fields accessed from within
mod tests { use super::*; }— idiomatic Rust. - Env var reading scoped to the PTY match arm only — Docker, Tmux, Subprocess unaffected.
- Doc comments in
main.rsandpty.rsare accurate and well-placed. - Two new unit tests correctly exercise both construction paths.
- Diff is tightly scoped — zero unrelated changes.
| let channel_capacity = env::var("AGENTD_WRAP_CHANNEL_CAPACITY") | ||
| .ok() | ||
| .and_then(|v| v.parse::<usize>().ok()) | ||
| .unwrap_or(DEFAULT_CHANNEL_CAPACITY); |
There was a problem hiding this comment.
🔴 Blocking: broadcast::channel(0) panics. parse::<usize>().ok() returns Some(0) for "0" so unwrap_or is never reached — the zero flows to pty_stream.rs:108 and panics on first create_session call.
Fix:
.and_then(|v| v.parse::<usize>().ok())
.map(|v| v.max(1)) // broadcast::channel(0) panics; clamp to ≥ 1
.unwrap_or(DEFAULT_CHANNEL_CAPACITY);Or fail at startup with anyhow::bail!("AGENTD_WRAP_CHANNEL_CAPACITY must be at least 1").
…eview feedback) - Clamp AGENTD_WRAP_CHANNEL_CAPACITY to ≥ 1 with a tracing::warn\! (broadcast::channel(0) panics) - Add defensive assert in PtyBackend::new_with_config with a clear message - Add should_panic test: pty_backend_new_with_config_zero_capacity_panics - Emit tracing::warn\! when either env var is set to an unparseable value - Clarify doc comments: these env vars apply to agentd-wrap service only Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This change is part of the following stack: Change managed by git-spice. |
Add
AGENTD_WRAP_HISTORY_BYTESandAGENTD_WRAP_CHANNEL_CAPACITYenvironment variables to tune the PTY output ring-buffer size and broadcast channel capacity at service startup, replacing hardcoded defaults.Changes
PtyBackend::new_with_config(prefix, channel_capacity, max_history_bytes)constructor stores and threads config into everyPtyOutputStreamPtyBackend::new()retains default behaviour (DEFAULT_HISTORY_BYTES/DEFAULT_CHANNEL_CAPACITY)main.rsreads both env vars at startup (PTY backend only) and logs the resolved valuesmain.rsupdated to list the new env varspty_backend_new_uses_default_buffer_configandpty_backend_new_with_config_applies_custom_valuesTest plan
cargo test -p wrap— 135 tests passcargo clippy -p wrap -- -D warnings— cleancargo fmt --check -p wrap— cleanAGENTD_BACKEND=pty AGENTD_WRAP_HISTORY_BYTES=1048576 AGENTD_WRAP_CHANNEL_CAPACITY=512logs resolved values on startupCloses #1164
🤖 Generated with Claude Code