fix: atomic cache writes and thread-safe repository lifecycle state

[vazarkevych]

Summary

Thread-safety fixes in the parts of the SDK that are genuinely shared across threads.

Rebased on main, which landed overlapping work in #234. This PR now contains only what #234 did not cover.

Atomic cache writes: FileCachingManagerImpl

The main fix here, and untouched by #234.

• Atomic publish. saveContent writes to a temp file and publishes it with Files.move(..., ATOMIC_MOVE), so a concurrent reader can never observe a half-written payload.
• No leftover temp files. The temp file is cleaned up when the write or the move fails, so *.tmp files do not accumulate.
• Serialized access. saveContent and loadCache are serialized on a private lock.

Covered by FileCachingManagerImplTest.

Repository lifecycle state: GBFeaturesRepository

Complements #234 rather than replacing it: that PR made the payload atomic via FeatureSnapshot; this one does the same for the lifecycle fields it did not touch.

Field(s)	Now
initialized, sseAllowed	AtomicBoolean
expiresAt	AtomicLong
sseHttpClient, sseEventSource, cacheManager, pollScheduler, sseRetryScheduler	AtomicReference
refreshStrategy, sseRequest	volatile

• No check-then-act. schedulePolling() and SSE client setup use compareAndSet, so two threads cannot both create a scheduler or client.
• Single teardown. shutdown() uses getAndSet(null), so each resource is torn down exactly once.

Public API is unchanged. The atomics stay private behind explicit getters that keep the original return types (getParsedFeatures() → Map, getInitialized() → Boolean, getExpiresAt() → Long, …). This addresses the earlier review comment about Lombok exposing the wrappers.

Shared helper: GrowthBookUtils

fireSubscriptions is extracted from GrowthBook and GrowthBookClient into one utility. It keeps the atomic compute() update and the null-key sentinel that #234 introduced, so key-less experiments still dedupe and still fire callbacks.

NativeJavaGbFeatureRepository

Fixes GbCacheManager initialization order.

Needs a decision: GrowthBook (single-user client)

This PR makes the single-user client safe to use from several threads:

• ConcurrentHashMap / CopyOnWriteArrayList for assignments and callbacks;
• volatile on the mutable fields;
• the substantive change: getEvaluationContext() now returns a per-call copy with its own StackContext instead of resetting the shared one. StackContext is per-evaluation scratch state (cycle detection, memoized results), so sharing it lets concurrent evaluations corrupt each other.

Warning

#234 took the opposite position and documented GrowthBook as single-threaded by design, directing shared/long-lived use to GrowthBookClient. Git merged both sides cleanly, so the javadoc and the implementation now disagree.

Please tell us which you want:

1. Keep it — I will update the class javadoc to match the implementation.
2. Drop it — I will remove the GrowthBook changes and keep the javadoc from fix: concurrency groundwork for the multi-user client and features repository #234; this PR then covers only the cache manager and the repository.

[madhuchavva]

Public getter compatibility issue found.

[madhuchavva]

Requesting changes for the public API compatibility issue noted inline.

[andywhite37]

@greptile review

[greptile-apps]

Confidence Score: 4/5

[High risk] Adds thread-safety to cache and repository lifecycle state.

The PR does not appear safe to merge while concurrent repositories can lose a cache write in their shared default directory.

_{Reviews (4) · Last reviewed commit: "Merge branch 'main' into fix/improve-thr..." · Reviewed by Greptile}

[vazarkevych]

All blocking items are addressed

[andywhite37]

@greptile review

[greptile-apps]

Shutdown test hides fetch failures This new test initializes the repository against an unmocked localhost endpoint, then catches and ignores every failure. It depends on real network behavior and can pass despite an unexpected initialization error. Repository instructions require unit tests to avoid real network access and prohibit catch-and-ignore; this requirement must be satisfied before merging.

Context Used: AGENTS.md (source)

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/src/test/java/growthbook/sdk/java/GBFeaturesRepositoryTest.java
Line: 628-631

Comment:
**Shutdown test hides fetch failures** This new test initializes the repository against an unmocked localhost endpoint, then catches and ignores every failure. It depends on real network behavior and can pass despite an unexpected initialization error. Repository instructions require unit tests to avoid real network access and prohibit catch-and-ignore; this requirement must be satisfied before merging.

**Context Used:** AGENTS.md ([source](https://github.com/growthbook/growthbook-sdk-java/blob/main/AGENTS.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[andywhite37]

Fixed at 4341f4ac: the test (now in GBFeaturesRepositoryMainBehaviorTest) no longer calls initialize(), so there's no network call and no empty catch. It runs in about 1ms.

[andywhite37]

The atomic cache writes (FileCachingManagerImpl) and the repository lifecycle atomics look good. The concurrent-safety review findings appear resolved.

Decision on GrowthBook: we'll stay with #234's approach (option 2). GrowthBook stays single-threaded by design, as its javadoc says, and GrowthBookClient is the class for shared use across threads. Please drop the GrowthBook thread-safety changes and keep the shared GrowthBookUtils.fireSubscriptions. The per-call StackContext also wouldn't make GrowthBook thread-safe on its own, because the user context it shares is still written to during evaluation (inline).

One test issue remains (inline), plus a few optional cleanups.

Testing:

• ./gradlew build on JDK 17 at 830a700f passed locally: 500 tests, 0 failures, 4 skipped. CI is green on JDK 8, 11 and 16 and on Linux and Windows.
• Ad hoc: one thread wrote 200 KB payloads while a second FileCachingManagerImpl read the same file. Over 27k reads there were no torn reads and no leftover .tmp files.
• Manual: ran against a real GrowthBook Cloud connection with SSE. SSE connected, a published flag change arrived live, and shutdown() disconnected it. The JVM's exit after shutdown matched main (about 57s, OkHttp's idle threads).
• Not tested: SSE reconnect after a dropped connection; the fallback when the filesystem doesn't support atomic moves.

[andywhite37]

@greptile review

[greptile-apps]

Poller starts after shutdown

If shutdown() runs while initialize() is fetching features, shutdown can remove the current poller before initialization reaches this compare-and-set. Initialization then installs a new poller without checking shuttingDown, so it keeps making network requests after teardown and is never stopped. SSE startup has the same gap: it can publish a connection after shutdown has cleared it.

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/src/main/java/growthbook/sdk/java/repository/GBFeaturesRepository.java
Line: 680-684

Comment:
**Poller starts after shutdown**

If `shutdown()` runs while `initialize()` is fetching features, shutdown can remove the current poller before initialization reaches this compare-and-set. Initialization then installs a new poller without checking `shuttingDown`, so it keeps making network requests after teardown and is never stopped. SSE startup has the same gap: it can publish a connection after shutdown has cleared it.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[andywhite37]

Agreed this race exists, but it looks like it predates this PR. Main's schedulePolling() and SSE setup also have no shuttingDown check, so a shutdown during initialize() can leave a poller behind there too. We'd like to track it as a follow-up rather than expand this PR.

[greptile-apps]

Comments Outside Diff

These findings could not be posted inline.

•  Nested evaluations lose stack state lib/src/main/java/growthbook/sdk/java/GrowthBook.java:254 ▶

  A feature-usage callback or plugin runs during evaluation and can evaluate another feature on the same GrowthBook. That nested call replaces the stack on the shared EvaluationContext. When the outer evaluation resumes, its cycle-detection and memoization state has been discarded, so it can produce an incorrect feature result.

[andywhite37]

Thanks for the updates. All five earlier comments look addressed:

• The GrowthBook changes are reverted, so it stays single-threaded as in #234.
• The polling shutdown test no longer makes a network call.
• The leftover comments, the sticky-bucket javadoc and the imports are cleaned up.

The merge with #215 looks right: main's refreshFeatures(..., source) calls are kept, with the atomic fields applied on top.

One optional note is inline. Greptile's two new findings both seem to predate this PR, so they look like follow-ups:

• The shutdown-during-initialize() poller race (replied inline).
• Its outside-diff note about nested evaluations resetting GrowthBook's shared stack. That code is unchanged from main.

Testing:

• ./gradlew build on JDK 17 at 4341f4ac passed locally: 604 tests, 0 failures, 4 skipped. CI is green on JDK 8, 11 and 16 and on Linux and Windows.
• Manual: ran against a real GrowthBook Cloud connection with SSE. SSE connected, a published flag change arrived live, and shutdown() disconnected it. The JVM exited at the same time as on main.
• Not tested: SSE reconnect after a dropped connection; the fallback when the filesystem doesn't support atomic moves.

[andywhite37]

Perceived severity: Low · Optional

After #215, GrowthBookClient checks for changed experiment assignments in its own ExperimentSubscriptionManager, so GrowthBook is now the only caller of this helper. That makes it new public API, which AGENTS.md treats as a contract, without a second caller to justify sharing it. The comment saying GrowthBookClient uses it is also out of date. GrowthBook's HashMap already kept null and "" keys apart before this PR.

Evidence: Code search: GrowthBook.java:366 is the only call site, and ExperimentSubscriptionManager.publishIfChanged has its own copy of the logic.

Suggested fix: Either keep fireSubscriptions as a private method on GrowthBook and drop the public helper, or have ExperimentSubscriptionManager call the helper so there's one implementation. Either way, update the comment to match.

[andywhite37]

@greptile review

[greptile-apps]

Shared cache writes can fail

Two repositories can create separate cache managers for the same default directory, but this lock protects only one manager. If one manager clears the cache while the other is writing a temporary file, it can delete that file before the move. The save then fails, leaving the next process without the expected cached payload when it needs an offline fallback.

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/src/main/java/growthbook/sdk/java/sandbox/FileCachingManagerImpl.java
Line: 31

Comment:
**Shared cache writes can fail**

Two repositories can create separate cache managers for the same default directory, but this lock protects only one manager. If one manager clears the cache while the other is writing a temporary file, it can delete that file before the move. The save then fails, leaving the next process without the expected cached payload when it needs an offline fallback.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[andywhite37]

Approving: the requested changes are in, and the build, CI and a manual SSE check against GrowthBook Cloud all pass at 4341f4ac.

Follow-ups, all of which already exist on main (none block this PR):

• Poller/SSE start after shutdown: shutdown() during initialize() can leave a poller or SSE client running. Both start paths need a shuttingDown check.
• SSE shutdown clears the durable cache: it wipes the cache directory, which other repositories may share, removing the next process's offline fallback. This also covers Greptile's note about one manager's clear racing another's save.
• Keyless experiments in GrowthBookClient: ExperimentSubscriptionManager maps a null key to "", so a keyless experiment and one keyed "" still share an entry and one callback is suppressed. This PR fixed that for GrowthBook only.
• Nested evaluation in GrowthBook: a callback that evaluates a feature during another evaluation resets the shared stack.
• Optional cleanup: the single-caller public GrowthBookUtils.fireSubscriptions (inline comment above).

Reviewer on leave, and I believe feedback was addressed. We can re-asses when she gets back.