Repository navigation
Conversation
… snapshots A fence-driven demotion now records the step-down in membership (atomic StepDown, bound to the epoch that raised it), so a lease the instance still holds renews as a real transition back to ACTIVE and the suspended pipeline resumes instead of sitting without checkpointing until a restart. Checkpoint snapshots carry a sequence allocated under the pipeline lock and never rewound by Recover. Within a term the write predicate orders on it, so a canceled writer's late update of an older snapshot is dropped as a no-op instead of overwriting a newer one. Adapts the PCSM-389 tests to the single-writer design: the demotion test drives onFenced, the ordering test is deterministic around the sequence.
A lease renewal interleaving between StepDown's unlock and its gauge update could leave the exported role one transition behind the recorded one. SetRole had the same gap. Also replaces a bare t.Fatal in the bootstrap collision test with the require helper.
… fence A fenced write while this instance still owns the lease means the lease document was reset with recovery data in place: the lease renews at a term the checkpoint rejects and nothing moves it forward, so the fence-driven demotion would repeat on every renewal. onFenced now learns the fencing term from the rejected write and raises the lease term past it through the owner only Membership.AdvanceTerm, so the next renewal is a promotion at a term the checkpoint accepts and the pipeline restores from that checkpoint. Covered end to end with a synctest and a mocked driver, so no tick is waited for, plus an owner only AdvanceTerm test against MongoDB.
The lease loop takes no server lock. A renewal at the stale term that reconciled ACTIVE between the step-down and the term advance left membership ACTIVE, the renewal at the advanced term then only updated the term, and nothing promoted the instance again, so the queued stale promotion opened an epoch without a checkpoint writer. After the repair, a membership that still advertises ACTIVE steps down at that term so the next renewal is a real transition. The synctest models the racing renewal. fencedError unwraps to the sentinel instead of comparing in Is, and the wrapcheck suppression in AdvanceTerm states its reason.
A fence-driven demotion that steps down after a renewal raced it leaves the renewal's ACTIVE transition queued. Consuming it resumed the pipeline for an instance advertising STANDBY until the next renewal replaced the epoch. onPromote now checks membership's role before it launches anything; the next renewal emits a fresh transition. The synctest consumes the stale transition and asserts no epoch opens.
Rely on term and snapshot sequence to reject late checkpoint writes. Drop the promotion join and call onFenced directly from the checkpoint loop; keep epoch cancellation and pipeline suspension.
Check the membership term before opening an epoch or resuming the pipeline. Cover a stale promotion that reaches the server lock after renewal at the repaired term.
A renewal response at the stale term that lands after onFenced has finished reconciles ACTIVE and queues that promotion. When the renewal at the repaired term returns before the queue is consumed, only the term changes; same-role renewals never emitted, so the queued promotion was rejected as stale and the instance stayed ACTIVE at the repaired term with no epoch and no checkpoint writer. An ACTIVE renewed at a new term re-acquired the lease, so it is a transition now, and the promotion at the new term replaces the stale one. The synctest drives both renewals through the real lease tick against the mocked driver and asserts an epoch and a checkpoint at the repaired term; it failed before the change.
Only checkpoint writes are fenced by term. Cancellation contains a deposed instance's data writes; a command already on the wire may still apply, as the root AGENTS.md already says. Also record that an ACTIVE keeps its role through renewal errors within the local lease deadline.
…ease ownership The lease loop reports the ownership each attempt established, as a level, to one lifecycle controller instead of emitting coalesced role transitions. The controller owns the role, the term and the ACTIVE tenure: every tenure builds a fresh pipeline and restores it from the checkpoint, and any loss of ownership or a fenced checkpoint write ends the tenure as a whole, canceling, joining and discarding its pipeline. No two tenures of a process write under one term: a tenure opens only at or above minTerm, and ownership reported below it is raised through the owner-only AdvanceTerm. Removes the same-term in-memory resume (Suspend's settle semantics, suspendedOnDemote, activeTerm, promotionPlan), the RoleChange channel, StepDown and the stale-promotion guards, and the in-memory clone resume with its cross-attempt inventory. Restore refuses a checkpoint owned by a newer term before recovering anything, and requests are admitted once the tenure's restore has finished.
…re the join A request that reaches a tenure still restoring its pipeline now waits for the restore, within its own context, instead of a 409: the HA harness picks an ACTIVE by its advertised role and issues a strict /start right away. The deferred --start is applied before requests are admitted, so a /start cannot pre-empt the configured options. Ending a tenure advertises STANDBY before joining its work, and the join honors the caller's context; the startup Status call runs on the tenure's context, so cancellation reaches its source request. A report that ended a tenure opens nothing itself: the next report, a fresh level, does. A clone ended by cancellation logs as canceled, not failed. The test helper that awaits a tenure no longer races its own readiness against the opener's return, and comments that still described the previous lifecycle are corrected.
A request that expired while waiting for the tenure's restore was still admitted when the restore finished first, and its lifecycle operation could launch for a caller that had gone away. The request's context is checked after the wait, whatever the tenure's state by then.
Claim the checkpoint before restoring it and stop the tenure after a checkpoint error. Document recovery invariants and diagnostic metadata, and cover retry, timeout and lost-response behavior.
Collaborator
Author
|
/agentic_review |
Code Review by Qodo
🔴 High 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - Events for one document route to the same worker by document-key hash to preserve order. | ||
| - Each worker builds bulks in its main loop and executes sealed bulks in a separate writer goroutine. | ||
| - `lastReplicatedOpTime` is the reported frontier; `checkpointOpTime` is the inclusive resume floor, not proof the event at that timestamp was applied. Replay detection keeps strict `<` and applies on every topology: `watchWithRetry` reopens inclusively from the checkpoint on replica sets too, and a redelivered DDL behind newer committed writes must be dropped, not re-applied. | ||
| - `lastReplicatedOpTime` is the reported frontier; `checkpointOpTime` is the inclusive resume floor, not proof the event at that timestamp was applied. An optional checkpoint `startAfter` token identifies the accounted predecessor of a protected index event. Resume/reconnect uses that exclusive token when present, otherwise the inclusive timestamp. Replay detection remains strict `<` on every topology. |
Contributor
There was a problem hiding this comment.
Be careful with remains, it’s profane in some cases remains retext-profanities
| }() | ||
|
|
||
| _, err = sourceColl.InsertOne(ctx, bson.D{ | ||
| {Key: "_id", Value: "deleted"}, {Key: "a", Value: bson.D{{Key: "b", Value: bson.A{}}}}, |
Contributor
There was a problem hiding this comment.
[gofmt] reported by reviewdog 🐶
Suggested change
| {Key: "_id", Value: "deleted"}, {Key: "a", Value: bson.D{{Key: "b", Value: bson.A{}}}}, | |
| {Key: "_id", Value: "deleted"}, | |
| {Key: "a", Value: bson.D{{Key: "b", Value: bson.A{}}}}, |
| awaitSignal(ctx, t, resumedDone, "recovered run did not stop") | ||
| }() | ||
| _, err = sourceColl.InsertOne(ctx, bson.D{ | ||
| {Key: "_id", Value: "after-recovery"}, {Key: "a", Value: bson.D{{Key: "b", Value: 1}}}, |
Contributor
There was a problem hiding this comment.
[gofmt] reported by reviewdog 🐶
Suggested change
| {Key: "_id", Value: "after-recovery"}, {Key: "a", Value: bson.D{{Key: "b", Value: 1}}}, | |
| {Key: "_id", Value: "after-recovery"}, | |
| {Key: "a", Value: bson.D{{Key: "b", Value: 1}}}, |
Collaborator
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit fdcf430 |
Collaborator
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit fdcf430 |
Collaborator
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 3ee8e2b |
Collaborator
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 3ee8e2b |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Persists a drained checkpoint before creating or dropping target indexes, so recovery doesn't replay older writes against a newer index definition. Uses exact
startAfterpredecessors and acknowledged requests through the existing tenure writer. Finalization releases the pipeline lock while draining and retains exclusive ownership.Stacked on #283 for checkpoint write ordering. Existing checkpoints remain readable, but this doesn't repair an already-affected target.
Verified locally: lint, race-enabled unit tests, MongoDB 7 integration tests, 207 replica-set E2E tests, and the actual insert/delete/text-index/restart sequence. The prerequisite baseline reproduces the reported error. The fixed binary preserves documents and indexes and continues replication after restart.
Go CI passes, including MongoDB 6, 7, and 8 integration jobs. The complete E2E Actions matrix passes at
3ee8e2b. Its MongoDB 8 sharded job passed on rerun after target majority-write timeouts caused lease loss and an HTTP 409 before the failed test started cloning. No assertion was changed for that rerun. Qodo's findings are resolved at3ee8e2b; the last commit only changes test timeout assertions.Functional CI used QA main rather than the prerequisite's QA branch and failed on obsolete interrupted-clone and unproven oplog-eviction expectations. The interrupted-clone failure also reproduces on the unchanged prerequisite. Its QA branch already corrects both expectations.
A local companion QA fixture creates the sharding index before cloning so index checkpointing does not invalidate its deliberate stale-checkpoint window. All assertions remain. Five targeted cases pass without skips: sharding replay, oplog eviction, clone interruption, and PCSM-128 recovery on both topologies. Publication of that companion QA branch awaits approval.
Draft pending functional CI and the full Jenkins suite against the matching QA revision. Jenkins #278 was intentionally aborted because it used QA main; it is not a test verdict.