Skip to content

Fix unbounded event Hub, System.nanoTime in hooks, integer overflow, and vacuous test assertions#108

Merged
EtaCassiopeia merged 2 commits into
fix/codebase-review-fixesfrom
fix/codebase-review-remaining
May 12, 2026
Merged

Fix unbounded event Hub, System.nanoTime in hooks, integer overflow, and vacuous test assertions#108
EtaCassiopeia merged 2 commits into
fix/codebase-review-fixesfrom
fix/codebase-review-remaining

Conversation

@EtaCassiopeia
Copy link
Copy Markdown
Owner

@EtaCassiopeia EtaCassiopeia commented Mar 31, 2026

Summary

Hub.unbounded replaced with Hub.dropping(256)

FeatureFlagsState used Hub.unbounded for provider events, growing without limit when subscribers are slow or absent. Replaced with Hub.dropping(256) — missing an intermediate event is acceptable since the next evaluation picks up current state.

System.nanoTime replaced with Clock.nanoTime in hooks

structuredLogging and metricsDetailed hooks called java.lang.System.nanoTime() directly in after/error callbacks, bypassing ZIO's Clock and breaking testability with TestClock. Now uses Clock.nanoTime consistently (matching before which already did).

ContextConverter integer overflow for large values

valueToAttribute converted whole-number doubles via .toInt, silently overflowing for values > Int.MaxValue (e.g., Unix timestamps). Now produces LongValue for values outside Int range.

Java hook error callback ClassCastException

wrapJavaHook used asInstanceOf[Exception] on the error cause, which throws ClassCastException for Error subclasses. Now wraps non-Exception Throwable in RuntimeException.

Registry tapBoth with no-op error arm

FeatureFlagRegistryLive.setProvider used tapBoth with _ => ZIO.unit as the error handler. Replaced with .tap which expresses the intent more clearly.

CircuitBreaker.transitionToHalfOpen demotes Closed circuit

The CAS loop's case _ arm matched Closed, demoting a healthy circuit to half-open. Added explicit case Closed => done = true to preserve healthy state.

HoconProvider.reload silently serves stale config on failure

If ConfigFactory.load() threw during reload, provider state stayed READY with stale config. Now sets state to ERROR on failure and READY on success.

EnvVarProvider dead AtomicReference field

state: AtomicReference[ProviderState] was declared but never updated. Replaced getState with a direct ProviderState.READY return and removed the unused field.

TestFeatureProvider failureProbability accepts invalid values

setFailureProbability accepted values outside [0.0, 1.0] — e.g., 1.5 caused constant failures with no indication why. Values are now clamped to [0.0, 1.0].

Vacuous test assertions replaced

Multiple HookSpec tests for logging/structuredLogging had assertTrue(true) as the only assertion. Now assert that hookData contains the expected start time entry, or use assertCompletes.

@EtaCassiopeia EtaCassiopeia changed the title Fix remaining issues from codebase review Fix unbounded event Hub, System.nanoTime in hooks, integer overflow, and vacuous test assertions Mar 31, 2026
@EtaCassiopeia EtaCassiopeia merged commit 6efd729 into fix/codebase-review-fixes May 12, 2026
@EtaCassiopeia EtaCassiopeia deleted the fix/codebase-review-remaining branch May 12, 2026 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant