Skip to content

Commit b9742e0

Browse files
bchaliosclaude
andcommitted
fix(rootfs): skip unsealed deferred diffs during cache eviction
The single disk-space eviction goroutine calls FileSize on the oldest evictable cache entry; for a still-sealing deferred rootfs diff that blocks on the background seal (WaitWithContext with the eviction loop's long-lived ctx), stalling all reclamation during exactly the high-disk-pressure window eviction exists to relieve. Add a non-blocking deferredDiff.sealed() check (via the SetOnce Done channel) and skip not-yet-sealed diffs in eviction, like pinned entries — so eviction neither blocks on the seal nor evicts a fresh, in-flight snapshot; the diff becomes evictable once its seal resolves. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
1 parent 34ac5c2 commit b9742e0

4 files changed

Lines changed: 93 additions & 0 deletions

File tree

‎packages/orchestrator/pkg/sandbox/build/cache.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,22 @@ func NewDiffStore(
9898
// buildData will be deleted by calling buildData.Close()
9999
defer ds.resetDelete(item.Key())
100100

101+
// A deferred diff whose background seal hasn't resolved would block Close()
102+
// on the reflink; close it off the eviction goroutine so this callback
103+
// isn't stalled. Bounded (the seal always resolves) and only reachable if
104+
// the TTL is shortened below the seal time — the disk-pressure eviction
105+
// path already skips unsealed diffs (see deferredDiff.sealed()).
106+
if dd, ok := buildData.(*deferredDiff); ok && !dd.sealed() {
107+
logCtx := context.WithoutCancel(ctx)
108+
go func() {
109+
if closeErr := dd.Close(); closeErr != nil {
110+
logger.L().Warn(logCtx, "failed to close unsealed deferred diff", zap.Any("item_key", item.Key()), zap.Error(closeErr))
111+
}
112+
}()
113+
114+
return
115+
}
116+
101117
if closeErr := buildData.Close(); closeErr != nil {
102118
logger.L().Warn(ctx, "failed to cleanup build data cache for item", zap.Any("item_key", item.Key()), zap.Error(closeErr))
103119
}
@@ -311,6 +327,14 @@ func (s *DiffStore) deleteOldestFromCache(ctx context.Context) (suc bool, e erro
311327
return true
312328
}
313329

330+
// Skip a deferred diff whose background rootfs seal hasn't resolved yet:
331+
// FileSize below would block on the seal (stalling the sole eviction
332+
// goroutine), and a fresh, still-sealing snapshot is exactly what a
333+
// just-resumed peer needs. It becomes evictable once the seal resolves.
334+
if dd, ok := item.Value().(*deferredDiff); ok && !dd.sealed() {
335+
return true
336+
}
337+
314338
sfSize, err := item.Value().FileSize(ctx)
315339
if err != nil {
316340
logger.L().Warn(ctx, "failed to get size of deleted item from cache", zap.Error(err))

‎packages/orchestrator/pkg/sandbox/build/cache_test.go‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
blockmetrics "github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/block/metrics"
3232
"github.com/e2b-dev/infra/packages/shared/pkg/featureflags"
3333
"github.com/e2b-dev/infra/packages/shared/pkg/storage/header"
34+
"github.com/e2b-dev/infra/packages/shared/pkg/utils"
3435
)
3536

3637
const (
@@ -716,3 +717,43 @@ func mustParseCfg(t *testing.T) cfg.Config {
716717

717718
return c
718719
}
720+
721+
// A deferred rootfs diff whose background seal hasn't resolved must be skipped
722+
// by disk-pressure eviction: its data methods (FileSize) block on the seal, so
723+
// evicting it would stall the sole eviction goroutine, and a fresh, still-sealing
724+
// snapshot is exactly what a just-resumed peer needs. Once the seal resolves the
725+
// entry becomes evictable like any other.
726+
func TestDiffStoreEvictionSkipsUnsealedDeferredDiff(t *testing.T) {
727+
t.Parallel()
728+
cachePath := t.TempDir()
729+
730+
c, err := cfg.Parse()
731+
require.NoError(t, err)
732+
flags := flagsWithMaxBuildCachePercentage(t, 100)
733+
store, err := NewDiffStore(c, flags, cachePath, 60*time.Second, 4*time.Second)
734+
require.NoError(t, err)
735+
736+
// Oldest entry: a deferred rootfs diff whose background seal hasn't resolved.
737+
promise := utils.NewSetOnce[Diff]()
738+
deferred := NewDeferredDiff(GetDiffStoreKey("deferred-unsealed", Rootfs), blockSize, promise)
739+
store.Add(deferred)
740+
741+
// A newer, ordinary diff behind it.
742+
newer := newRootFSDiff(t, cachePath, "seal-newer")
743+
store.Add(newer)
744+
745+
// Eviction must skip the unsealed deferred diff and fall through to the
746+
// next-oldest instead of blocking on the seal.
747+
ok, err := store.deleteOldestFromCache(t.Context())
748+
require.NoError(t, err)
749+
assert.True(t, ok)
750+
assert.False(t, store.isBeingDeleted(deferred.CacheKey()), "unsealed deferred diff must be skipped")
751+
assert.True(t, store.isBeingDeleted(newer.CacheKey()), "eviction must fall through to the next-oldest")
752+
753+
// Once the seal resolves, the deferred diff is evictable like any other entry.
754+
require.NoError(t, promise.SetValue(newRootFSDiff(t, cachePath, "seal-resolved")))
755+
ok, err = store.deleteOldestFromCache(t.Context())
756+
require.NoError(t, err)
757+
assert.True(t, ok)
758+
assert.True(t, store.isBeingDeleted(deferred.CacheKey()), "sealed deferred diff must be evictable")
759+
}

‎packages/orchestrator/pkg/sandbox/build/deferred_diff.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,19 @@ func (d *deferredDiff) BlockSize() int64 {
5353
return d.blockSize
5454
}
5555

56+
// sealed reports, without blocking, whether the background seal has resolved the
57+
// promise (with the materialized diff or an error). Cache eviction uses it to
58+
// skip a not-yet-sealed diff rather than block on the data methods (which wait
59+
// on the seal) or evict a fresh, still-in-flight snapshot.
60+
func (d *deferredDiff) sealed() bool {
61+
select {
62+
case <-d.inner.Done:
63+
return true
64+
default:
65+
return false
66+
}
67+
}
68+
5669
func (d *deferredDiff) CachePath(ctx context.Context) (string, error) {
5770
inner, err := d.inner.WaitWithContext(ctx)
5871
if err != nil {

‎packages/orchestrator/pkg/sandbox/build/deferred_diff_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,21 @@ import (
1111
"github.com/e2b-dev/infra/packages/shared/pkg/utils"
1212
)
1313

14+
// TestDeferredDiff_SealedIsNonBlocking verifies sealed() reports readiness
15+
// without blocking on the promise, so cache eviction can skip an unsealed diff
16+
// instead of stalling on it.
17+
func TestDeferredDiff_SealedIsNonBlocking(t *testing.T) {
18+
t.Parallel()
19+
20+
inner := utils.NewSetOnce[Diff]()
21+
d := NewDeferredDiff(GetDiffStoreKey("build-id", Rootfs), 4096, inner).(*deferredDiff)
22+
23+
require.False(t, d.sealed(), "unresolved promise must read as not sealed")
24+
25+
require.NoError(t, inner.SetValue(&NoDiff{}))
26+
require.True(t, d.sealed(), "resolved promise must read as sealed")
27+
}
28+
1429
// makeLocalDiff writes `data` to a fresh local diff file and materializes it.
1530
func makeLocalDiff(t *testing.T, blockSize int64, data []byte) Diff {
1631
t.Helper()

0 commit comments

Comments
 (0)