Support replicated: true on set_configuration#1556
Conversation
Opt-in cluster-wide fan-out via server.replication.replicateOperation, mirroring drop_schema. Peers receive replicated: false and apply locally; per-node outcomes are returned on response.replicated. The replicated flag is stripped from configFields so it is never written to the config file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…peration default-on Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces opt-in replication for the set_configuration operation. When the replicated flag is provided, the configuration changes are fanned out to all cluster nodes via server.replication.replicateOperation. The replicated parameter is stripped from the configuration fields to prevent it from being written to the configuration file. Unit tests and design documentation have been added to cover this new behavior. Feedback suggests avoiding direct mutation of the response object returned by replicateOperation to prevent potential runtime errors if the object is frozen.
|
Reviewed; no blockers found. |
…lag (#660) Non-clustered instance: replicated: true rejects explicitly (base replication stub), the flag is never persisted to config, and the origin-first local write still applies. Cluster fan-out coverage belongs to harper-pro's cluster harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
heskew
left a comment
There was a problem hiding this comment.
I found one issue worth fixing before merge:
config/configUtils.ts:836 gates fan-out on if (replicated), but set_configuration does not validate this operation-control field before applying the local write. A request with replicated: "false" still enters this branch, so open-source Harper applies the config change locally and then rejects with Replication not implemented; with a real replication implementation it would fan out unexpectedly. Other replicated surfaces validate this as a boolean, so this should reject non-booleans before updateConfigValue(...) or only fan out on replicated === true, with a test for the string "false" case.
🤖 Posted by Codex (gpt-5.5) on Nathan's behalf
…e local write (#660) A string "false" is truthy — it would apply config locally and then fan out (or reject on OSS) unexpectedly. Enforce boolean per other replicated surfaces (analyticsValidator) before updateConfigValue so malformed values have no side effects. Unit + integration coverage for the string case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in 76d953c: |
kriszyp
left a comment
There was a problem hiding this comment.
Approving — correct opt-in config fan-out mirroring drop_schema: origin-first, loop-prevented (peers get replicated:false), super_user-gated + re-validated on peers, fails closed on bad input. The node-local-key guard is tracked as CORE-3018 (so a blanket replicate of ports/paths stays a known follow-up). Thanks @heskew!
— 🤖 KrAIs (Kris's review assistant)
Closes #660 (CORE-2939).
What
set_configurationnow acceptsreplicated: trueand fans the same config change out to all cluster nodes in one Ops API call, mirroring thedrop_schemapattern (server.replication.replicateOperation). Driven by a customer updatingcorsAccessListper-node from automation pipelines.replicated: false, so they apply locally without re-replicating.response.replicated[](peer failures are embedded as{status: 'failed', reason, node}— the fan-out itself never rejects).replicatedis added to the handler's destructure strip-list so it is never written toharperdb-config.yaml(on origin or peers).replicated: truerejects withReplication not implemented.(existing base-server stub behavior).Integration tests (apiTests/configuration.test.mjs) pin the single-node contract:
replicated: truewithout clustering rejects explicitly, and the flag is never persisted to config. Real cluster fan-out verification belongs to harper-pro's cluster harness (companion candidate). 5 new unit tests cover: fan-out + per-node results, flag stripping, no-flag no-op, peer-sidereplicated: false, and local-failure-before-fan-out. DESIGN.md gets a note on the non-obviousreplicateOperationcontract (see below).Where to look
replicateOperationfans out wheneverreq.replicated !== false— absence means "replicate" (the DDL contractdrop_schemarelies on). Calling it unconditionally would have silently made everyset_configurationreplicate by default. Henceif (replicated)and the DESIGN.md note.set_configuration {replicated: true}+restart_service {service: "http", replicated: true}compose as two calls (the latter already restarts nodes sequentially — effectively rolling). Documented as the workflow; not verified in a live multi-node cluster in this PR.message— identical todrop_schema's existing contract; operators must checkresponse.replicated[]. Flagged by review, kept pattern-consistent; docs point at the array. Open to making it louder if you'd rather.Review/verification notes
configUtilsunit tests pass. The broaderunit:main/unit:resourcessuites carry pre-existing environment failures on this machine (present without this change, none config-related) — CI is the clean-room run for those.replicated: trueonset_configurationdocumentation#559.Follow-up candidates (out of scope): the Ops API
replicated-support audit floated in #660, and per-field replicability metadata under CORE-3018.Generated by an LLM (Claude Fable 5), reviewed per the cross-model flow above.
🤖 Generated with Claude Code