Skip to content

fix: migrate spec POJOs to AtomicReference + immutable snapshots (S3077)#423

Open
jlouvel wants to merge 1 commit intomainfrom
fix/sonar-volatile-spec
Open

fix: migrate spec POJOs to AtomicReference + immutable snapshots (S3077)#423
jlouvel wants to merge 1 commit intomainfrom
fix/sonar-volatile-spec

Conversation

@jlouvel
Copy link
Copy Markdown
Contributor

@jlouvel jlouvel commented May 6, 2026

Related Issue

Closes #422


What does this PR do?

Phase 2 of sonar-bug-remediation. Resolves SonarQube rule java:S3077"Non-thread-safe fields should not be volatile" — across 27 spec POJO classes (44 bug occurrences from run #1516, plus surrounding non-flagged volatile fields on the same classes for consistency).

Each volatile field is replaced with an AtomicReference<T> (or AtomicBoolean / AtomicInteger for primitive cases) holding an immutable snapshot for collections (List.copyOf / Map.copyOf). This:

  1. Satisfies S3077 — no volatile remains on any non-thread-safe type.
  2. Provides genuine thread-safety — readers always see a fully-constructed value; mutations are atomic at the reference level via set / updateAndGet.
  3. Unlocks future write paths — fluent Java builders and Control-port runtime spec edits get a lock-free atomic-swap primitive for free.
  4. Preserves the public Jackson-facing API — getters/setters still expose plain String / List<T> / Map<K,V>. Jackson never sees an AtomicReference.

Patterns applied (documented in the blueprint):

  • Pattern A — scalar (String, Integer, custom Spec): AtomicReference<T> with plain getter/setter.
  • Pattern BList<T>: stored as List.copyOf(...) snapshot or CopyOnWriteArrayList inside an AtomicReference.
  • Pattern CMap<K, V>: stored as Map.copyOf(...) snapshot inside an AtomicReference. setParameter-style mutations use updateAndGet for lock-free copy-on-write.

Lazy initializers (e.g. ControlServerSpec.getManagement(), ObservabilityMetricsSpec.getLocal(), ControlManagementSpec.getLogs()) now use compareAndSet to remain thread-safe.

Files migrated (27)

Package Files
io.naftiko.spec NaftikoSpec, OperationSpec
io.naftiko.spec.aggregates AggregateSpec, AggregateFunctionSpec
io.naftiko.spec.consumes.http HttpClientSpec, HttpClientResourceSpec, HttpClientOperationSpec, OAuth2AuthenticationSpec
io.naftiko.spec.exposes ServerSpec, ServerCallSpec
io.naftiko.spec.exposes.control ControlServerSpec, ControlManagementSpec
io.naftiko.spec.exposes.mcp McpServerToolSpec, McpServerResourceSpec
io.naftiko.spec.exposes.rest RestServerOperationSpec, RestServerResourceSpec, RestServerStepSpec
io.naftiko.spec.exposes.skill ExposedSkillSpec, SkillToolSpec
io.naftiko.spec.observability ObservabilitySpec, ObservabilityMetricsSpec, ObservabilityTracesSpec, ObservabilityExportersSpec
io.naftiko.spec.scripting OperationStepScriptSpec
io.naftiko.spec.util BindingSpec, OperationStepCallSpec, StructureSpec

Engine-layer classes (Phase 3) and char[] auth password fields (Phase 4) are tracked separately.

Note on PR ordering

This branch was cut from fix/sonar-major-bugs (Phase 1, PR #420) but the two phases touch disjoint files, so this PR can be reviewed independently. Once #420 merges, this branch will be rebased onto main.


Tests

New test

  • SpecFieldThreadSafetyTest — a structural meta-test that uses reflection to verify:
    • No covered spec class declares any volatile field (guards against S3077 regression).
    • Each non-abstract class still has a usable default constructor (preserves Jackson compatibility).
    • Parameterized over all 27 migrated classes — 54 cases total, all passing.

This test would have failed 26 times before the migration (every class except ObservabilityExportersSpec already migrated as the canonical proof-of-pattern), proving the fix is real.

Existing tests

The existing extensive Jackson round-trip and engine-wiring suite (ObservabilitySpecTest, TelemetryBootstrapTest, BindingSpecTest, InputParameterRoundTripTest, etc.) continues to pass — confirming that Jackson sees no behavioural difference and the engine reads values correctly through the new accessor shape.

Full test results

Tests run: 977, Failures: 14, Errors: 0, Skipped: 5

The 14 failures are pre-existing and unrelated to this change — they affect Step{2-8,10}Shipyard*IntegrationTest and stem from non-deterministic LLM responses (the model occasionally returns prose like "The …" instead of JSON, triggering Unrecognized token 'The'). These were already failing on main before this PR.


Checklist


Agent Context

agent_name: GitHub Copilot
llm: claude-opus-4.7
tool: copilot-chat
confidence: high
source_event: SonarQube quality gate run #1516 (main @ dcfd01c)
discovery_method: code_review
review_focus: |
  - Pattern A/B/C consistency across all 27 files
  - Lazy initializer compareAndSet correctness in ControlServerSpec / ObservabilityMetricsSpec / ControlManagementSpec
  - @JsonProperty kebab-case alias preserved on both getter and setter for ExposedSkillSpec
  - SpecFieldThreadSafetyTest reflection-based assertions

@jlouvel jlouvel self-assigned this May 6, 2026
@jlouvel jlouvel requested a review from eskenazit May 6, 2026 22:11
@jlouvel
Copy link
Copy Markdown
Contributor Author

jlouvel commented May 6, 2026

@eskenazit This will need to be rebased on main once PR #420 is merged

Resolves SonarQube rule java:S3077 (Non-thread-safe fields should not be volatile) across 27 spec POJO classes (44 bug occurrences). Each volatile field is replaced with an AtomicReference / AtomicBoolean / AtomicInteger holding an immutable snapshot for collections (List.copyOf / Map.copyOf).

Adds SpecFieldThreadSafetyTest as a structural meta-test that fails if any covered class re-introduces volatile, guarding against regression. Phase 2 of sonar-bug-remediation. Closes #422.
@jlouvel jlouvel force-pushed the fix/sonar-volatile-spec branch from 06ea1c1 to 4ac2dd9 Compare May 6, 2026 23:48
@jlouvel
Copy link
Copy Markdown
Contributor Author

jlouvel commented May 6, 2026

Rebased onto main (force-with-lease push).

This branch was originally cut from fix/sonar-major-bugs (PR #420), which made #420 a hard predecessor. To keep the four Sonar PRs disjoint and independently mergeable, I rebased this branch directly onto main and dropped the inherited 466ea54 commit.

Result:

Order of merge no longer matters between #420 and #423.

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: 44 spec POJO fields use volatile on non-thread-safe types (Sonar S3077)

1 participant