fix(orchestrator): stop nbd read flood for permanently-missing build objects#3295
fix(orchestrator): stop nbd read flood for permanently-missing build objects#3295AdaAibaby wants to merge 3 commits into
Conversation
…objects When a build object is absent from the store (ErrObjectNotExist), the Linux NBD kernel driver retries the failing block read in a tight loop. Each retry reaches DiffStore.GetOrCreate, which did not cache negative results, so every retry triggered a storage 404 lookup — producing hundreds of ERROR log lines per second and burning GCS/S3 quota. Two fixes: 1. Negative cache in DiffStore.GetOrCreate (cache.go) Store a short-lived (60 s) negative entry when createDiff returns ErrObjectNotExist. Subsequent calls for the same key return immediately without hitting storage. The 60 s TTL lets the cache recover if the build object is re-uploaded (e.g. after a rebuild). The double-check inside the singleflight closure prevents a race where two goroutines both miss the outer check. 2. Downgrade log level for permanent failures (dispatch.go) Distinguish ErrObjectNotExist (permanent, expected when a build is gone) from transient backend errors. The former is now logged at WARN with a clearer message; transient errors remain at ERROR. This keeps dashboards and alerting focused on actionable signals. Fixes e2b-dev#3294
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a negative cache to the DiffStore to prevent repeated storage 404s during kernel NBD retries when a build object is permanently missing. It also downgrades the logging level from error to warning for missing build objects in the NBD dispatch loop. The reviewer pointed out that using sync.Map for the negative cache could lead to an unbounded memory leak since there is no background eviction for entries that are never requested again, and suggested using ttlcache.Cache instead to handle automatic expiration and simplify the code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…iction
Replace the hand-rolled sync.Map + negativeCacheEntry struct (with manual
time.Now().Before(expiresAt) expiry checks) with a
ttlcache.Cache[DiffStoreKey, struct{}] instance that matches the positive
cache pattern already used in DiffStore.
Benefits:
- Automatic background eviction: ttlcache.Start() purges expired entries
periodically, eliminating the unbounded memory growth that occurs when
build keys are never re-requested after the initial 404.
- Simpler read path: s.negativeCache.Has(key) replaces the Load +
type-assert + time comparison.
- Consistent lifecycle: negativeCache is started in Start() and stopped
in Close() alongside the positive cache, keeping the two in lockstep.
- Add() now deletes the negative cache entry for the key before inserting
into the positive cache, so a re-uploaded build is immediately visible
without waiting for TTL expiry.
Add three unit tests covering the core negative-cache invariants:
- NegativeCacheShortCircuits: create is called once; subsequent calls
within TTL return ErrObjectNotExist without invoking create again.
- NegativeCacheNotSetForOtherErrors: transient errors do not populate
the negative cache; create is retried on every call.
- AddClearsNegativeCache: Add() removes the negative entry so that the
next GetOrCreate returns the just-added diff without a create call.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa90480044
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…id Add race
A race exists between Add and the negative cache insertion in GetOrCreate:
1. GetOrCreate enters initGroup.Do and calls create(ctx) — a slow
storage probe that eventually returns ErrObjectNotExist.
2. Concurrently, Add(diff) runs for the same key:
negativeCache.Delete(key) // no-op — nothing there yet
cache.Set(key, diff) // diff is now in positive cache
3. create returns ErrObjectNotExist.
4. GetOrCreate installs a negative cache entry.
5. For the next 60 s, GetOrCreate returns ErrObjectNotExist even
though the diff is present in the positive cache.
Fix: before writing the negative entry, recheck the positive cache.
If Add has already populated it, skip the negative insertion.
A narrow window remains (Add between the recheck and the Set), but
it is far smaller than the original window (the entire create duration)
and bounded by ttlcache eviction.
Add TestGetOrCreate_AddRaceWithStorageProbe which reproduces the
scenario by blocking create until Add has fired, then asserting that
the diff installed by Add is reachable immediately afterward.
2 participants
Summary
Fixes #3294
When a build object is permanently absent from the store (), the Linux NBD kernel driver retries the failing block read in a tight loop. Each retry previously reached , which did not cache negative results, so every retry triggered a fresh GCS/S3 404 lookup — producing 150+ ERROR log lines/second and burning object-store quota.
Two targeted fixes:
**Negative cache in ** (): Store a short-lived (60 s) negative entry when returns . Subsequent calls for the same key short-circuit immediately without hitting storage. The TTL allows recovery if the build object is re-uploaded (e.g. after a rebuild). A double-check inside the closure prevents a race where two goroutines both miss the outer negative-cache check.
Log-level downgrade for permanent failures (): Distinguish (permanent, expected when a build is gone) from transient backend errors. The former is now logged at with a clearer message; transient errors remain at . This keeps dashboards and alerting focused on actionable signals.
Changed files
Test plan
FAIL — all tests pass