[connector/manager] refactor connector write protocol & add spec group lookup in instanceInfo

[Tyndalllll]

Summary

Unify v1/v2 write protocol in the SGLang connector and speed up spec group lookup on the manager side.

Commits

1. [manager] enhance instance info and cache manager with spec group lookup

• Add InstanceInfo::FindSpecGroup — lookup via std::lower_bound.
• Refactor IsSpecNameInSpecGroup to take InstanceInfo& and reuse the new lookup.
• Restructure StartWriteCache / FilterWriteCache with fail-fast branches keyed on whether location_spec_group_names is provided.
• Replace all_of/any_of with std::includes (pre-sorted) in select_location_policy.cc.

2. [connector] refactor sglang connector for modularity and v2 cross-rank safety

Shared write-protocol helpers (used by both v1 and v2):

• _start_write: rank 0 issues the request and broadcasts (result, hash, extras); non-rank-0 validates the hash → skip_transfer; MLA non-rank-0 short-circuits.
• _finish_write: rank 0 commits; returns False on failure so callers can fall back.
• _sync_per_block_flags: per-block MIN all_reduce; skipped for MLA / single rank.

Align v2 cross-rank semantics with v1:

• scalar flag → per-block MIN (a block succeeds only if all ranks wrote it).
• Add valid_save_mask so each rank writes only blocks it actually has local data for.
• Handle prefix_write_count and prepend Falses to finish_mask, matching v1.

Other:

• Split _init_kvcm_client into _register_pool_specs / _register_instance / _init_transfer_client.
• Push MLA branches down into helpers, keeping call sites clean.
• Preserve backward compatibility for pure full-attention deployments (empty location_spec_groups not sent to older managers).

Risk

• Wire format unchanged.
• location_spec_groups only sent when extra pools exist, so older managers are unaffected.

[qoderai]

👋 Review Summary

Nice refactor -- centralizing the binary-search lookup into FindSpecGroup, splitting the init path into focused helpers, and unifying the v1/v2 cross-rank write protocol makes the codebase meaningfully easier to follow. The fail-fast validation in StartWriteCache and the explicit EC_INSTANCE_NOT_EXIST error for missing instances are both welcome improvements over the previous silent-fallthrough behavior.

🛡️ Key Risks & Issues

_start_write divergence hash is incomplete (inline comment on connector.py:834)
The shared _start_write helper hashes block_keys and extra_fields to detect cross-rank input divergence, but omits spec_group_names. This is safe for all current call sites (groups are deterministic from uniform class state), but the helper is documented as reusable -- a future caller with per-rank differentiated groups would silently pass divergence detection. Low urgency, but worth closing now while the code is fresh.

🧪 Verification Advice

• The std::includes change in select_location_policy.cc relies on requested_spec_names being sorted. This invariant holds because all production callers pass group->spec_names(), which LocationSpecGroup::SortSpecNames() guarantees. Consider adding a debug-mode assert(std::is_sorted(...)) at the top of ExistsForWrite to catch any future caller that breaks this precondition.
• The v2 write path (batch_set_v2) now mirrors v1's per-block MIN all_reduce and valid_save_mask semantics. A targeted integration test that simulates a multi-rank v2 write where one rank has fewer local blocks than rank 0 would exercise the valid_save_mask / prefix_write_count interaction in finish_mask.

💡 Thoughts & Suggestions

• The FilterWriteCache refactor to branch once on location_spec_group_names.empty() and then assign two different lambdas is a clean pattern -- it avoids the per-block branch the old code had. Good call capturing instance_info by value in the lambda (keeps the shared_ptr alive) while letting the rest capture by reference.
• FindSpecGroup correctly mirrors the SortLocationSpecGroups comparator (a.name() < b.name() vs g.name() < n). The const LocationSpecGroup* return type is a natural fit for the "nullable lookup" pattern.
• Minor note: IsSpecNameInSpecGroup still passes group_name.data() to PREFIX_LOG with %s (line 100). Since the parameter is std::string_view, data() is not guaranteed null-terminated by the standard (even though it will be in practice from std::string-sourced views). The StartWriteCache log on line 485 already uses the safer %.*s pattern -- it might be worth aligning IsSpecNameInSpecGroup to match for consistency.

---

🤖 Generated by Qoder • View workflow run

[qoderai]

The divergence-detection hash here covers block_keys and extra_fields, but the spec_group_names argument is excluded from the hash. Since _start_write is designed as the shared cross-rank primitive for both v1 and v2, a future caller that passes per-rank differentiated spec groups would silently pass divergence detection -- ranks would proceed with mismatched manager state, potentially leading to storage corruption or cross-rank gloo hangs.

For the current v1/v2 call sites this is harmless because spec_group_names is derived deterministically from class state that is uniform across all ranks. But since _start_write is explicitly documented as a reusable helper, including tuple(spec_group_names) in the hash would close this latent gap:

local_hash = hash(tuple(block_keys) + tuple(spec_group_names) + tuple(v for _, v in extra_fields))

Not blocking -- just a defensive improvement for when this helper gets reused.

---

🤖 Generated by Qoder • Fix in Qoder