From 67c7a0eb7ea47cfbd4f8a62bc3e2bfaac6656808 Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 2 Sep 2026 15:25:39 +0800 Subject: [PATCH 1/3] fix: release idle upstream conns on egress Close; skip unreadable subdirs in fs.find egress: each sandbox's Proxy owned two http.Transports with no idle timeout, and Close never closed their idle pool, so a released claim left its upstream keep-alive connections and their read loops behind. Close now drops the idle pool and both transports expire idle connections after 90 s. silkd: fs.find returned a terminal error frame when read_dir failed on a subdirectory it had already queued, while an unreadable file was skipped; a tree changing under a running find (a build removing a directory) lost the whole search mid-stream. Only the root directory is fatal now; a queued subdirectory that cannot be opened is skipped. --- sandboxd/egress/proxy.go | 10 +++++++++- sandboxd/egress/proxy_test.go | 37 +++++++++++++++++++++++++++++++++++ silkd/src/find.rs | 24 ++++++++++++++--------- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index b96142eb..2f4ad5b3 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -12,10 +12,13 @@ import ( "slices" "strings" "sync" + "time" "github.com/cocoonstack/sandbox/sandboxd/utils" ) +const idleConnTimeout = 90 * time.Second + // hopHeaders are hop-by-hop and proxy-scoped headers this hop owns: stripped // from forwarded requests and from relayed responses rather than passed on. var hopHeaders = []string{ @@ -85,7 +88,7 @@ func New(sandbox, tenant string, policy Evaluator, secrets Secrets, ca *CA, dial dial: dial, // The stdlib default of 2 idle conns per host re-dials bursty // same-host plaintext traffic. - tr: &http.Transport{DialContext: dial, MaxIdleConnsPerHost: 8}, + tr: &http.Transport{DialContext: dial, MaxIdleConnsPerHost: 8, IdleConnTimeout: idleConnTimeout}, conns: map[net.Conn]struct{}{}, } if ca != nil { @@ -93,6 +96,7 @@ func New(sandbox, tenant string, policy Evaluator, secrets Secrets, ca *CA, dial DialContext: dial, TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12}, MaxIdleConnsPerHost: 8, + IdleConnTimeout: idleConnTimeout, } p.leaves = map[string]*tls.Certificate{} } @@ -118,6 +122,10 @@ func (p *Proxy) Close() { for _, conn := range conns { _ = conn.Close() } + p.tr.CloseIdleConnections() + if p.mitmTr != nil { + p.mitmTr.CloseIdleConnections() + } } // track registers conn for Close; false means already closed, conn closed instead. diff --git a/sandboxd/egress/proxy_test.go b/sandboxd/egress/proxy_test.go index d6471c25..2e9c3a8a 100644 --- a/sandboxd/egress/proxy_test.go +++ b/sandboxd/egress/proxy_test.go @@ -52,6 +52,43 @@ func TestForwardAllowInjectsSecretAndOverwritesGuestHeader(t *testing.T) { } } +func TestCloseReleasesIdleUpstreamConns(t *testing.T) { + closed := make(chan struct{}, 4) + upstream := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = io.WriteString(w, "hello") + })) + upstream.Config.ConnState = func(_ net.Conn, st http.ConnState) { + if st == http.StateClosed { + closed <- struct{}{} + } + } + upstream.Start() + defer upstream.Close() + + policy := Policy{Allow: []Rule{{Host: "api.internal"}}} + p := New("sb_1", "acme", policy, nil, nil, fixedDial(upstream.Listener.Addr().String()), nil) + front := httptest.NewServer(p) + defer front.Close() + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://api.internal/x", nil) + if err != nil { + t.Fatalf("build request: %v", err) + } + resp, err := proxyClient(t, front.URL).Do(req) + if err != nil { + t.Fatalf("proxied GET: %v", err) + } + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + + p.Close() + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("upstream connection still open after Close") + } +} + func TestForwardNeverInjectsInterceptSecret(t *testing.T) { reached := false upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/silkd/src/find.rs b/silkd/src/find.rs index 92df425e..7fe6f945 100644 --- a/silkd/src/find.rs +++ b/silkd/src/find.rs @@ -32,17 +32,16 @@ pub async fn find( Some(Ok(re)) => Some(re), Some(Err(e)) => return proto::error_frame(w, ErrorKind::BadRequest, e.to_string()).await, }; - let mut stack = vec![path]; - while let Some(dir) = stack.pop() { - let mut rd = match fs::read_dir(&dir).await { - Ok(rd) => rd, - Err(e) => return err_frame(w, &e, "read_dir").await, - }; + let mut rd = match fs::read_dir(&path).await { + Ok(rd) => rd, + Err(e) => return err_frame(w, &e, "read_dir").await, + }; + let mut stack = Vec::new(); + loop { loop { let ent = match rd.next_entry().await { Ok(Some(ent)) => ent, - Ok(None) => break, - Err(e) => return err_frame(w, &e, "read_dir").await, + _ => break, }; let Ok(ft) = ent.file_type().await else { continue; @@ -54,8 +53,15 @@ pub async fn find( scan_file(w, &re, &p).await?; } } + rd = loop { + let Some(dir) = stack.pop() else { + return proto::write_frame(w, &Response::Done).await; + }; + if let Ok(rd) = fs::read_dir(&dir).await { + break rd; + } + }; } - proto::write_frame(w, &Response::Done).await } /// Rewrites every `pattern` match to `replacement` in each of `files`, From 8f5ea8eae0ae1e472169f09c666e4b017396895d Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 2 Sep 2026 15:45:05 +0800 Subject: [PATCH 2/3] fix: keep a root iteration error fatal in fs.find; mirror the skip rule in the fake A read_dir error on the requested root after partial enumeration was folded into the subdirectory skip and answered done with a truncated result; the root stays fatal, only queued subdirectories are skipped. The Go silkdtest fake now applies the same rule so the SDK tests see what silkd does. --- sdk/go/silkd/silkdtest/fake.go | 10 ++++++++-- silkd/src/find.rs | 6 +++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/sdk/go/silkd/silkdtest/fake.go b/sdk/go/silkd/silkdtest/fake.go index 15c10965..97afbe48 100644 --- a/sdk/go/silkd/silkdtest/fake.go +++ b/sdk/go/silkd/silkdtest/fake.go @@ -287,9 +287,15 @@ func (f *Fake) fsFind(conn net.Conn, req *wire.FsFind) { if req.Glob != "" { nameRe = globRegexp(req.Glob) } - walkErr := filepath.WalkDir(f.abs(req.Path), func(path string, d fs.DirEntry, err error) error { - if err != nil || d.IsDir() { + root := f.abs(req.Path) + walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + switch { + case err != nil && path == root: return err + case err != nil && d != nil && d.IsDir(): + return fs.SkipDir + case err != nil || d.IsDir(): + return nil } if nameRe != nil && !nameRe.MatchString(d.Name()) { return nil diff --git a/silkd/src/find.rs b/silkd/src/find.rs index 7fe6f945..fe667d7c 100644 --- a/silkd/src/find.rs +++ b/silkd/src/find.rs @@ -36,12 +36,15 @@ pub async fn find( Ok(rd) => rd, Err(e) => return err_frame(w, &e, "read_dir").await, }; + let mut root = true; let mut stack = Vec::new(); loop { loop { let ent = match rd.next_entry().await { Ok(Some(ent)) => ent, - _ => break, + Ok(None) => break, + Err(e) if root => return err_frame(w, &e, "read_dir").await, + Err(_) => break, }; let Ok(ft) = ent.file_type().await else { continue; @@ -53,6 +56,7 @@ pub async fn find( scan_file(w, &re, &p).await?; } } + root = false; rd = loop { let Some(dir) = stack.pop() else { return proto::write_frame(w, &Response::Done).await; From 36ad88d57293a529b5ae7cec45cfcc5b5b1e7c7c Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 2 Sep 2026 18:26:50 +0800 Subject: [PATCH 3/3] review: the find walk back to one loop, PathBuf on the stack, the MITM transport cloned The nested-loop rewrite hid the success return three levels deep; the original while-let carries the root-fatal / subdirectory-skip split as two match guards. The directory stack held re-encoded Strings while ent.path() already owned a PathBuf. The fake's SkipDir branch could not differ from nil: WalkDir revisits a directory only after its ReadDir failed. The MITM transport re-spelled the base transport's fields. --- sandboxd/egress/proxy.go | 8 ++------ sdk/go/silkd/silkdtest/fake.go | 13 +++++++------ silkd/src/find.rs | 26 ++++++++++---------------- 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/sandboxd/egress/proxy.go b/sandboxd/egress/proxy.go index 2f4ad5b3..65b0a2e6 100644 --- a/sandboxd/egress/proxy.go +++ b/sandboxd/egress/proxy.go @@ -92,12 +92,8 @@ func New(sandbox, tenant string, policy Evaluator, secrets Secrets, ca *CA, dial conns: map[net.Conn]struct{}{}, } if ca != nil { - p.mitmTr = &http.Transport{ - DialContext: dial, - TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12}, - MaxIdleConnsPerHost: 8, - IdleConnTimeout: idleConnTimeout, - } + p.mitmTr = p.tr.Clone() + p.mitmTr.TLSClientConfig = &tls.Config{MinVersion: tls.VersionTLS12} p.leaves = map[string]*tls.Certificate{} } return p diff --git a/sdk/go/silkd/silkdtest/fake.go b/sdk/go/silkd/silkdtest/fake.go index 97afbe48..4ff21542 100644 --- a/sdk/go/silkd/silkdtest/fake.go +++ b/sdk/go/silkd/silkdtest/fake.go @@ -289,12 +289,13 @@ func (f *Fake) fsFind(conn net.Conn, req *wire.FsFind) { } root := f.abs(req.Path) walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - switch { - case err != nil && path == root: - return err - case err != nil && d != nil && d.IsDir(): - return fs.SkipDir - case err != nil || d.IsDir(): + if err != nil { + if path == root { + return err + } + return nil + } + if d.IsDir() { return nil } if nameRe != nil && !nameRe.MatchString(d.Name()) { diff --git a/silkd/src/find.rs b/silkd/src/find.rs index fe667d7c..b069ae55 100644 --- a/silkd/src/find.rs +++ b/silkd/src/find.rs @@ -2,7 +2,7 @@ //! rewrites matches in named files. Regex handling lives here so agents pass a //! pattern as data rather than shell-quoting it through exec. -use std::path::Path; +use std::path::{Path, PathBuf}; use regex::Regex; use tokio::fs; @@ -32,13 +32,14 @@ pub async fn find( Some(Ok(re)) => Some(re), Some(Err(e)) => return proto::error_frame(w, ErrorKind::BadRequest, e.to_string()).await, }; - let mut rd = match fs::read_dir(&path).await { - Ok(rd) => rd, - Err(e) => return err_frame(w, &e, "read_dir").await, - }; + let mut stack = vec![PathBuf::from(path)]; let mut root = true; - let mut stack = Vec::new(); - loop { + while let Some(dir) = stack.pop() { + let mut rd = match fs::read_dir(&dir).await { + Ok(rd) => rd, + Err(e) if root => return err_frame(w, &e, "read_dir").await, + Err(_) => continue, + }; loop { let ent = match rd.next_entry().await { Ok(Some(ent)) => ent, @@ -51,21 +52,14 @@ pub async fn find( }; let p = ent.path(); if ft.is_dir() { - stack.push(p.to_string_lossy().into_owned()); + stack.push(p); } else if ft.is_file() && name_matches(&p, name_re.as_ref()) { scan_file(w, &re, &p).await?; } } root = false; - rd = loop { - let Some(dir) = stack.pop() else { - return proto::write_frame(w, &Response::Done).await; - }; - if let Ok(rd) = fs::read_dir(&dir).await { - break rd; - } - }; } + proto::write_frame(w, &Response::Done).await } /// Rewrites every `pattern` match to `replacement` in each of `files`,