Skip to content

fix: migrate engine fields to AtomicReference snapshots (S3077, Phase 3)#425

Open
jlouvel wants to merge 2 commits intomainfrom
fix/sonar-volatile-engine
Open

fix: migrate engine fields to AtomicReference snapshots (S3077, Phase 3)#425
jlouvel wants to merge 2 commits intomainfrom
fix/sonar-volatile-engine

Conversation

@jlouvel
Copy link
Copy Markdown
Contributor

@jlouvel jlouvel commented May 6, 2026

Summary

Phase 3 of the Sonar bug remediation blueprint. Migrates the remaining 13 volatile fields across 5 engine-layer classes from the volatile keyword to AtomicReference<T> (or static AtomicReference for resettable singletons) holding immutable snapshots.

This is the engine-layer counterpart to #423 (spec POJOs), and addresses the same SonarQube rule java:S3077. The two PRs touch disjoint files and can land in any order.

Closes #424.

Files migrated

File Fields Pattern
io.naftiko.Capability spec, serverAdapters, clientAdapters, aggregates, bindings, scriptingSpec, stepHandlerRegistry A (scalar), B (List wrapping CopyOnWriteArrayList), C (Map snapshot)
io.naftiko.engine.consumes.ClientAdapter capability, spec A
io.naftiko.engine.exposes.mcp.McpServerAdapter stdioHandler, stdioThread A
io.naftiko.engine.scripting.ScriptStepExecutor scriptingSpec A + per-call snapshot in execute()
io.naftiko.engine.observability.TelemetryBootstrap static instance static AtomicReference (singleton is reset at runtime by init/forTesting/reset, so the IODH idiom does not fit)

Why AtomicReference and not the holder idiom

  • All instance fields are mutated post-construction (setters, hot-reload-ready).
  • TelemetryBootstrap.instance is explicitly reset at runtime by init(String, ObservabilitySpec), init(OpenTelemetry) and reset() — the initialization-on-demand holder idiom assumes a write-once singleton, which does not match this lifecycle.

Notes for reviewers

  • ScriptStepExecutor.execute() now snapshots scriptingSpec once at entry into a local of the same name, so all subsequent reads in the method observe a consistent view for the duration of the call (avoids races with a future hot-reload setter).
  • Capability constructor builds local list variables fully before publishing them via AtomicReference#set — avoids partial-state observation if the constructor throws.
  • List slots wrap CopyOnWriteArrayList inside the AtomicReference so request-handling iteration remains lock-free even if a future hot-reload swaps the slot.
  • ClientAdapter#setSpec(HttpClientSpec) keeps its subtype-narrowing parameter (intentional, predates this PR).

Test plan

  • New meta-test EngineFieldThreadSafetyTest (5 parameterized cases) — reflects over each migrated class and asserts no field is volatile. Acts as a regression guard.
  • Full local suite: 926 tests, 14 failures, 0 errors. The 14 failures are the pre-existing LLM-flaky tutorial integration tests (Step{2-8,10}Shipyard*IntegrationTest) that return prose like "The..." instead of JSON, causing Unrecognized token 'The' — same set as on main and unaffected by this change.
  • Bug Workflow followed: failing test committed first (test: add failing meta-test...), then fix (fix: migrate engine fields...).

jlouvel added 2 commits May 6, 2026 18:23
Phase 3 of the Sonar bug remediation blueprint. Migrates the remaining 13 volatile fields across 5 engine classes (Capability, ClientAdapter, McpServerAdapter, ScriptStepExecutor, TelemetryBootstrap) from `volatile` keyword to `AtomicReference<T>` with immutable snapshots. Static singleton TelemetryBootstrap.instance becomes a static AtomicReference because it is reset at runtime by init/forTesting/reset (the IODH idiom does not fit a resettable singleton).

Pattern applied:

- Scalar references: AtomicReference<T> with .get()/.set()

- List<T> fields: AtomicReference<List<T>> wrapping CopyOnWriteArrayList for lock-free iteration

- Map<K,V> fields: AtomicReference<Map<K,V>> with HashMap snapshot built before publication

- ScriptStepExecutor.execute() now snapshots the spec once at entry to observe a consistent view for the call duration

Closes #424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: 13 engine-layer fields use volatile on non-thread-safe types (Sonar S3077)

1 participant