Skip to content

CBG-5838: wait for the local replicator state in ISGR upsert tests - #8767

Merged
torcolvin merged 6 commits into
mainfrom
CBG-5838
Oct 2, 2026
Merged

torcolvin merged 6 commits into
mainfrom
CBG-5838

Conversation

@torcolvin

@torcolvin torcolvin commented Sep 8, 2026 •

Copy link
Copy Markdown
Collaborator

CBG-5838

TestRequireReplicatorStoppedBeforeUpsert failed in CI with a 400 on the upsert that follows the stop:

Messages:   	Response status 400 "Bad Request" (expected 200 "OK")
            	for PUT <http://127.0.0.1/db/_replication/replication1> : {"error":"Bad Request","reason":"Replication must be stopped before updating config"}

Cause

GetReplicationStatus falls back to the cfg's target state while no replicator has published status, so a replication reports running before anything has started. The wait for running returned on its first poll.

RefreshReplicationCfg reads the cfg, then starts assigned replications while holding activeReplicatorsLock. The test carried on with that start still in flight, stopped the replication, and the start landed afterwards on the pre-stop cfg. The upsert then found a running replicator. The 20.7 ms the failing PUT took is it waiting on activeReplicatorsLock while that start held it.

Fix

Report starting for that stub. The target state stays the answer for every other state, because stopped and error are true when no replicator has reported, and running is not.

Two things already agree with this:

  • PUT _replicationStatus/{id}?action=start answers starting through transitionStateName whenever the current state differs from the target.
  • UpsertReplication gates on stopped, so the rejection of a config change during a start is unchanged.

A wait for running now needs either a local replicator in that state or a status document, which only a running replicator writes. No start can be in flight when the wait returns, because the refresh holds activeReplicatorsLock across Start and the status read needs the read lock.

Behaviour changes

  • GET _replicationStatus and the _cluster response report starting instead of running for a replication that no replicator has published status for.
  • ?activeOnly=true returns starting as well as running, so a replication does not drop out of the listing while it starts. The spec description of the parameter is updated to match.
  • PUT _replicationStatus/{id}?action=start answers starting instead of running in that window.
  • A caller polling for running now waits for a replicator instead of returning at once.

The status document is still the only cross-node channel, so a replication whose status write fails, or whose document expired, reads starting on other nodes until the next successful publish. That trades a silent false running for a visible false starting.

Tests

TestReplicationStatusBeforeReplicatorStarts adds a replication assigned to the local node, never runs a refresh, and asserts the reported status. It fails on the old code with expected: "starting", actual: "running" and has no timing dependence.

Verified by delaying RefreshReplicationCfg between its cfg read and the start, which widens the window deterministically: the endpoint reported running 3/3 before the change and starting 3/3 after, and TestRequireReplicatorStoppedBeforeUpsert passes 5/5 under that delay at 8.1 s per run. Without the delay it passes 100/100 under -race. The full rest/replicatortest package, the db replication tests and the rest replication tests pass.

unassigned is added to the ISGRReplicationState enum in the API spec. The code already returned it.

🤖 Generated with Claude Code

@torcolvin torcolvin self-assigned this Sep 8, 2026
/_replicationStatus fell back to the cfg's target state while no replicator had
published status, so a replication reported running before anything started.

- If this is assigned to this node, return starting if it hasn't get made it to running yet.
- add starting to activeOnly=true filter
- Add unassigned to the openapi docs
@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Redocly previews

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The empty target state is treated as running during startup but remains uncovered and incorrectly reported by the new fallback.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Updates ISGR status reporting so pending replications report starting instead of prematurely reporting running.

Changes:

  • Reports assigned, not-yet-started replications as starting.
  • Includes starting replications in activeOnly results.
  • Updates tests and OpenAPI documentation.
File Description
docs/​api/​components/​schemas.yaml Documents the unassigned state.
docs/​api/​components/​parameters.yaml Clarifies activeOnly behavior.
db/​sg_replicate_cfg.go Corrects pending replication status handling.
db/​sg_replicate_cfg_test.go Tests pre-start and active-only status behavior.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread db/sg_replicate_cfg_test.go Outdated
require.NoError(t, err)
require.NoError(t, mgr.StartLocalNode(localNodeUUID, nil))

for _, targetState := range []string{ReplicationStateRunning, ReplicationStateStopped} {
torcolvin and others added 2 commits September 28, 2026 13:55
- An unset target state starts like running, so report it as starting too.
- activeOnly now includes reconnecting replications.
- Cover replications assigned to another node, live replicator states,
  the activeOnly REST query, and upserts while starting.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Create replications with UpsertReplication and let rebalance assign them,
  with the other node joining first where it owns the replication.
- Move the unset target state case into its own test.
- Run activeOnly against a real passive node, so running and reconnecting
  both come from a replicator, and drop the test that forced states.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@torcolvin
torcolvin marked this pull request as ready for review September 29, 2026 00:47
@torcolvin
torcolvin requested a review from a team September 29, 2026 00:47
@factory-droid

factory-droid Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Droid finished @torcolvin's task —— View job


The status fallback and activeOnly behavior changes look correct and are backed by focused tests for the startup window and reconnecting behavior. One small OpenAPI wording mismatch remains: the activeOnly parameter description should reference the response field status, not state.

Comment thread docs/api/components/parameters.yaml Outdated
@torcolvin torcolvin removed their assignment Sep 29, 2026
bbrks
bbrks previously approved these changes Oct 2, 2026
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@torcolvin
torcolvin merged commit 0ee5664 into main Oct 2, 2026
53 checks passed
@torcolvin
torcolvin deleted the CBG-5838 branch October 2, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants