[manager] add safe asynchronous CopyGA - #307
Conversation
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This is a large, well-structured PR introducing an opt-in asynchronous CopyGA contract with persistent guard fencing across leader changes. The safety model — fail-closed unless the backend proves terminal+safe_to_reuse_dst, guards surviving in-memory state loss, and recovery before leader-only traffic — is sound and consistently applied.
Strengths:
- The two-phase callback split (
AsyncCopyRemoteSubmitCompletionvsAsyncCopyCompletion) with theremote_submit_futuregating recoverability is a clean separation of concerns. - Fail-closed semantics are enforced throughout:
EC_CORRUPTIONon future/malformed guards inMetaDummyBackend::Open,expected_migration_copy_guard_absentCAS guard preventing fence stealing,has_migration_copy_guard()checks in bothCacheReclaimerandCacheGarbageCollector. - The reordering in
RegistryManager::RemoveStorage(drain before delete) and theSetAsyncCopyReferenceCheckerhook are correct safety additions. StartMigrationManager()returningErrorCodeand its promotion to the leader recovery barrier before GC/Reclaimer start is the right ordering.- Test coverage is thorough: CAS guard ownership, round-trip serialization, quarantine list/break-glass validation, persistent-guard fence tests in both Reclaimer and GC.
Issues flagged inline (7 comments): See individual comments for details. The most substantive ones are:
- Potential race on
async_copy_reference_checker_read without the lock inUnRegisterStorage. - The
Demote()re-entrancy question for partially-startedMigrationManager. FindPersistentMigrationSourcePinlinkage — ensure its declaration is in a shared header.
No duplicate findings relative to prior reviews (this is the first review on this PR).
🤖 Generated by Qoder
8c1ef00 to
8fd135c
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c1ef00501
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
8fd135c to
2b3ba61
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b3ba6134c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2b3ba61 to
32143a4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32143a4114
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4e3277435
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
e18ce0f to
2960155
Compare
|
Review-fix history was cleaned up as requested: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2960155a8a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| UpdateAsyncCopyGuard(ctx, | ||
| ExpectedGuardState(ctx), | ||
| MigrationCopyGuardState::MCGS_UNKNOWN, | ||
| ctx.async_backend_task_ids, | ||
| fail_reason); | ||
| MoveAsyncCopyCreditToQuarantine(ctx, fail_reason); |
There was a problem hiding this comment.
When an async submission or completion becomes ambiguous while the metadata CAS or Sync is failing, the return value from UpdateAsyncCopyGuard is ignored and the operation is immediately published in the in-memory quarantine and removed from the active table. The durable guard can therefore remain SUBMITTING, ACTIVE, or CANCELLING; BreakGlassReleaseAsyncCopy then cannot match its required durable UNKNOWN state, and the coordinator is no longer monitored until another leader recovery. Keep the task pending and retry/reconcile the transition, and only publish the quarantine record after kAppliedDurably.
Useful? React with 👍 / 👎.
| const auto result = cleanup.future.get(); | ||
| if (result.status != EC_OK) { |
There was a problem hiding this comment.
When an async target/source cleanup finishes with EC_PARTIAL_OK or EC_ERROR, such as after a transient backend Delete failure, the ready future has already been erased and this branch only logs the result. SchedulePlanExecutor::DoLocationDelTask also proceeds to BatchCADLocationStatus regardless of per-URI deletion failures, so metadata identifying the leaked allocation can disappear and no cleanup reference or retry remains. The fresh evidence beyond the prior reference fix is that ready non-OK futures are still discarded; retain/retry failed physical deletions and avoid removing their metadata until the corresponding URIs are deleted.
Useful? React with 👍 / 👎.
| if (!remote_result.accepted) { | ||
| continue; |
There was a problem hiding this comment.
When graceful demotion occurs after the remote-submit future has become ready with accepted=false and acceptance_unknown=false, this branch discards that authoritative no-side-effect result. The later active-task pass sees a handleless SUBMITTING operation and permanently moves it to UNKNOWN quarantine even though the destination is known safe to delete. The fresh evidence beyond the prior accepted-handle fix is that ready rejected results explicitly take this continue; finalize these as pre-submit failures before processing the remaining detached tasks.
Useful? React with 👍 / 👎.
| if (UpdateAsyncCopyGuard( | ||
| ctx, MigrationCopyGuardState::MCGS_NONE, MigrationCopyGuardState::MCGS_SUBMITTING, {}, "") != | ||
| GuardMutationResult::kAppliedDurably) { |
There was a problem hiding this comment.
The source snapshot is taken before destination allocation, but the persistent guard that pins that source is only installed here after Create, BatchAddLocation, and the mark query. During this potentially long preparing window, Reclaimer and ordinary deletion protect the active target but see no persistent source pin, so they can transition the serving source to DELETING and physically remove it; submission then still launches the remote Copy from the stale URI. The remaining pre-guard window is distinct from the guarded steady-state admission fix: after installing the guard, re-read and require the exact source identity to remain CLS_SERVING before backend handoff.
Useful? React with 👍 / 👎.
2960155 to
f0ed093
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0ed093a1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ctx.dst_location_id.c_str(), | ||
| ctx.async_operation_id.c_str(), | ||
| fail_reason.c_str()); | ||
| SubmitTargetLocationDelete(ctx); |
There was a problem hiding this comment.
When a pre-submit failure occurs after the guarded WRITING -> DELETING CAS succeeds but its Sync times out, this calls ordinary deletion and immediately releases ownership. The delete admission path skips an already CLS_DELETING location unless resume_deleting is set, so it can return EC_OK without physical deletion; if the ambiguous CAS later proves durable, the target is guard-free and recovery cannot discover it, leaving its metadata and allocation stranded. Re-read/retry the finalization and submit prepared/resumed deletion when the transition was applied.
Useful? React with 👍 / 👎.
| // The in-memory cancellation must not outrun its persistent fence. | ||
| // Drop ownership into fail-closed quarantine; a late completion can no | ||
| // longer publish or release the target through this process. | ||
| CompleteCopyTaskAsUnknown(cancelling_ctx, "cancel_guard_persist_failed"); |
There was a problem hiding this comment.
If the ACTIVE/SUBMITTING -> CANCELLING update is definitively not applied, this fallback passes a context already marked kCancelling to CompleteCopyTaskAsUnknown. ExpectedGuardState() therefore attempts CANCELLING -> UNKNOWN, while the durable guard remains ACTIVE or SUBMITTING; every subsequent completion retry mismatches the same state and the operation remains active indefinitely until a leader restart. The fallback must transition from the original expected state or restore/reconcile the in-memory state before retrying.
Useful? React with 👍 / 👎.
| if (!all_physical_deletes_succeeded) { | ||
| // CLS_DELETING metadata is the retry anchor. Never CAD it while any | ||
| // URI may still exist; the owner retains the storage reference and | ||
| // resubmits this exact cleanup through resume_deleting admission. | ||
| if (result.error_message.empty()) { | ||
| result.error_message = "one or more physical storage deletes failed"; | ||
| } | ||
| KVCM_LOG_WARN("retain %zu CLS_DELETING location(s) after physical delete failure for instance %s", | ||
| total_locations_to_delete, | ||
| task.instance_id.c_str()); | ||
| return result; |
There was a problem hiding this comment.
When a physical delete submitted by the ordinary CacheReclaimer or CacheGarbageCollector returns a transient failure, this now preserves the CLS_DELETING metadata and returns a non-OK result, but those callers discard the completed future and release their pending state (CacheReclaimer::HandleDelRes and CacheGarbageCollector::PollInflightDeletes). Their future scans do not select CLS_DELETING locations, and unlike MigrationManager they never resubmit with resume_deleting, so the failed data and metadata remain stranded indefinitely. Add a retry owner for these callers or a general recovery scan for retained deleting locations.
Useful? React with 👍 / 👎.
| expected_items, | ||
| operation_id, | ||
| options, | ||
| [completion, expected_items](AsyncCopyBatchResult batch_result) mutable { |
There was a problem hiding this comment.
When a recovered backend returns the expected number of terminal/safe items but associates them with different, duplicated, or reordered task IDs, this callback discards every AsyncCopyItemResult::backend_task_id and reports success based only on count and flags. MigrationManager can consequently promote the target even though the results do not prove completion of the handles persisted in its guard, potentially serving an incompletely copied cache. Capture the expected handle set/order and treat any task-ID mismatch as unknown rather than successful.
Useful? React with 👍 / 👎.
1 participant
Summary
Safety model
Async execution remains backend opt-in through
SupportsAsyncCopy(). Existing open-source storage backends keep their current synchronous behavior until they provide an implementation.Validation
Validated on the final branch rebased onto the latest
main:instance_group_testmigration_strategy_testDataStorageManagerTestmeta_dummy_backend_testMetaSearcherTestSchedulePlanExecutorTestMigrationManagerTestCacheReclaimerTestCacheGarbageCollectorTestAdminServiceImplTest//kv_cache_manager:kv_cache_manager_binThe downstream PACE backend adapter and final end-to-end smoke validation are tracked separately.