HarperFast / HarperFast/harper

set_configuration overwrites the replicated fan-out message with success, hiding per-node failures

Open
#2,270 0 comments 0 reactions 0 assignees View on GitHub
bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.