HarperFast / HarperFast/harper
set_configuration overwrites the replicated fan-out message with success, hiding per-node failures
- Dominant language
- JavaScript
- Stars
- 89
- Forks
- 10
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 200
Description
## Summary
On a `replicated: true` `set_configuration`, the origin applies locally, fans out, and then overwrites the fan-out response's `message` with the unconditional success string. A peer that rejected the change is only visible if the caller reads `response.replicated`.
```js
const response = await server.replication.replicateOperation(setConfigJson);
response.message = CONFIGURE_SUCCESS_RESPONSE; // overwrites, regardless of per-node outcomes
return response;
```
## Failure scenario
A rolling upgrade, made concrete by #2266's fix. Before it, a peer whose `CONFIG_PARAM_MAP` lacks a param silently ignored it; after it, that peer returns a hard 400. So during a mixed-version window:
1. The origin (new version) accepts a param the not-yet-upgraded peers don't know.
2. It applies locally and fans out.
3. Each peer 400s the name.
4. `response.message` still reads `Configuration successfully set. You must restart Harper for new config settings to take effect.`
The cluster is now divergently configured, and the top-level response says it worked. The same shape applies to any per-node failure — an unwritable config file, a peer mid-restart — not just a version skew.
## Scope
Pre-existing, introduced with `replicated: true` support (#660), and independent of #2266 — but #2266 makes it materially more likely to fire, because a peer that used to ignore an unknown name now fails on it. Worth deciding together with #2266's release line.
The local write also lands before the fan-out (origin-first, pinned by `integrationTests/apiTests/configuration.test.mjs`), so there is no rollback of the origin when peers reject. That ordering is deliberate and not what this issue asks to change; the ask is that the *response* stop claiming success when it has per-node failures in hand.
## Suggested shape
Derive `message` from the per-node outcomes rather than assigning the success constant unconditionally — success only when every node succeeded, otherwise a message that says how many nodes failed and points at `replicated`. Alternatively return a non-2xx when any peer failed, though that is a bigger contract change for callers that currently treat 200 as "origin applied".
## Related
- #2266 — unrecognized params now 400 on the node that receives them, which is what makes the skew reachable.
- #2269 — a resolved param whose write throws is swallowed and still reported as success (same false-success family, local rather than cluster).
- #1950 — a skipped write reported as success via a stale config-object comparison.
Contributor guide
Research direction
Start at server.replication.replicateOperation and trace where response.message is assigned after the replicated set_configuration fan-out. Read integrationTests/apiTests/configuration.test.mjs for the origin-first behavior and add coverage for per-node failures. Done means the response no longer reports unconditional success when replicated contains failed nodes, while preserving the existing ordering.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, node.js
- Domain
- api, backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 72/100