[atchernych]

Summary

Adds lib/global-common (dynamo-global-common), the crate the Global Router coding standards ask for: the types that more than one Global Router component needs, each defined once. The name follows #17354, which lists it among the Global Router crates. It also moves the Global Discovery Service client from #17208 onto these types.

• A pool's identity and location: PoolKey (cluster_id, namespace, dgd_name), PoolId and its deriver, and PoolLocation. Taken from the shared crate on the global-router-v2 test branch (Sachal's code, credited in the commit).
• The Global Discovery Service's JSON: the POST /pools body and its lease reply, and the pool and router lists.
• How a router names a PoolRelay on a trusted network (address_identity). The service writes one for each address of the PoolRelay's pod into each pool's relay_identities (two on a dual-stack cluster), and a router compares a connecting PoolRelay against them.
• Global Router replicas (RouterReplica), and list-and-watch events and snapshots (WatchEvent, Snapshot, Completeness).
• The client from feat(router): add a Global Discovery Service client crate #17208 uses these types. lib/global-discovery-client drops its own copies and re-exports the types its calls use, so one PoolKey exists on main, as Neelay and Jacob asked on feat(router): add a Global Discovery Service client crate #17208. Its calls and errors do not change.

Compared with the JSON that client sent before this PR:

• The key's first field is cluster_id, not site_id. It is the cluster ID the pool's PoolRelay runs with (--cluster-id, DYN_CLUSTER_ID), so a pool key and its relay use one name. Where MCS also reports the pool, that ID must be the cluster's name in the ClusterSet. Pool IDs do not change: the deriver hashes the values, not the field names.
• PoolLocation has no cluster, since the key carries it.
• A capacity entry counts workers, not count.
• A pool names its PoolRelay by every address of its pod, in relay_identities.

PoolKey checks each field while reading. A registration's model gets the same check, so a PoolRelay with an empty model gets a 422 instead of registering a pool that no request can match. The lease reply keeps the check from the client in #17208 (Jacob's, credited in the commit): a zero interval, or one not below the lease, does not parse. As in that client, there is no upper bound: the service bounds the lease it grants (#17371). Lease is #[must_use], as the client's copy was.

Renamed in Rust only, after Neelay's review: RouterReplica, RouterReplicaId and RouterReplicaEvent (were ReplicaEndpoint, ReplicaId and ReplicaEvent), WatchEvent::Upserted (was Added) and Lease::duration() (was lease_duration()).

The Global Discovery Service (#17371) uses these types. The PRs stacked on this one pick up the new names when they merge it.

Overlap with #15295: that draft defines the same pool identity types (PoolKey, PoolId, V1PoolIdDeriver, PoolLocation) in kv_router::global_view. Whichever lands second should use the other's, so there is one definition. The derived IDs are the same, and a test here pins one.

#17355 is on main: it added an empty lib/global-common for #17354, and a lib/global-*/** code-owner rule that covers this crate. This PR fills that crate, so it adds no workspace member and no code-owner line.

Validation

• cargo test -p dynamo-global-common: 10 passed. They cover pool identity (with one ID pinned), the key's JSON and its checks, the API's JSON (and a blank model), that a reader skips unknown fields and fills missing lists (Dmitry's test), that a reply without its list does not parse, the relay identity, and the lease checks.
• cargo test -p dynamo-global-discovery-client: 7 passed, including the exact JSON the client sends with register_pool.
• cargo clippy --all-targets -- -D warnings, cargo doc with -D warnings and cargo fmt --check for both crates: clean, with --locked.
• An adversarial review of this update ran before the push. It changed the cluster_id doc to state the MCS requirement, trimmed the client's re-exports to what its calls use (adding LeaseError), and added the test that a reply without its list does not parse.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added shared data formats for pool identities, locations, capacity, discovery listings, router endpoints, and state-change updates.
  • Added validation for pool identity fields and discovery service leases, including limits on lease duration and heartbeat intervals.
  • Discovery listings can indicate whether pool data is complete, and snapshots can distinguish exact from estimated data.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: ai-dynamo/dynamo
• Branch: main
• File: .coderabbit.yaml
• Commit: 4b487d8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 8162ab8a-30f6-4b80-8b3c-031da54faf7c

Commits

Reviewing files that changed from the base of the PR and between 4b487d8 and 9e368cf.

Files ignored due to path filters: 1

• Cargo.lock is excluded by !**/*.lock

Files selected for processing: 9

• .github/codeowners/areas.yaml
• CODEOWNERS
• Cargo.toml
• lib/global-common/Cargo.toml
• lib/global-common/src/discovery.rs
• lib/global-common/src/lib.rs
• lib/global-common/src/pool.rs
• lib/global-common/src/replicas.rs
• lib/global-common/src/watch.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

The pull request adds the dynamo-global-common workspace crate. It provides shared pool identity, watch, replica, and Global Discovery Service API types, with serialization and validation tests.

Changes

Global common types

Layer / File(s)	Summary
Crate setup and pool identity .github/codeowners/areas.yaml, CODEOWNERS, Cargo.toml, lib/global-common/Cargo.toml, lib/global-common/src/pool.rs, lib/global-common/src/lib.rs	The workspace and ownership rules include the new crate. The crate defines validated pool coordinates, pool locations, and deterministic pool IDs, and re-exports its shared types.
Watch and replica types lib/global-common/src/watch.rs, lib/global-common/src/replicas.rs	Adds watch events, snapshots, completeness values, replica IDs and endpoints, and a replica event alias.
Discovery API types and validation lib/global-common/src/discovery.rs	Adds pool registration and listing types, lease validation, address identity formatting, router responses, and tests for serialization and validation.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: Minimal · up to 9e368

No actionable issue currently blocks merging. Confirm the discovery API’s wire format when its service implementation is available.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely identifies the primary change: adding a shared crate for Global Router component types.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description is thorough and follows the template's intent. It has a Summary (Overview), a Key Changes list covering the new crate, the moved client types, JSON changes, and renames, and a Validati…

Full details: Description check

Explanation

The description is thorough and follows the template's intent. It has a Summary (Overview), a Key Changes list covering the new crate, the moved client types, JSON changes, and renames, and a Validation section listing the cargo test, clippy, doc, and fmt runs. It is not vague or off-topic. It has no 'Issues and Ongoing Work' section with a 'Closes #XXXX' issue link, and the template's Pull Request Checklist item ('I used Dynamo's /pr-checklist skill') is not shown as checked. The description also does not use the template's section headings: it uses 'Summary' and 'Validation' rather than 'Overview', 'Key Changes', and 'Issues and Ongoing Work'. The PR links related work (#17354, #17208, #17371, #15295, #17355) but does not give a closing issue reference. The template notes the PR Issue Link check is advisory until release/1.7.0 is cut, so the missing link is not a hard blocker.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[devin-ai-integration]

✅ Dynamo PR CI passed — run 40614797175 (attempt 1) on c6efecbb90

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

_{Posted automatically by Devin for run 40614797175. Updated on every full-CI run of this PR.}

[dynamo-review-agent]

Previously reported defects still present:

• Original discussion: The redundant LeaseError doc comment remains at lib/global-common/src/discovery.rs:94; it only restates that Lease::new returns this error, while the validation and detailed error message already preserve the constraints.

[nnshah1]

Naming points, since this crate is where the shared types will live. Checked against head c7ee161. None of these change the JSON unless noted, and the first is the only one I would hold the PR on.

1. PoolKey.site_id versus the relay's cluster_id. The pool key says site_id. PoolLocation has cluster, region, availability_zone and datacenter. The PoolRelay just renamed in #17215 identifies itself with cluster_id and derives a stable hash from it. If site_id is the deployment cluster the relay serves, it is the relay's cluster id in string form and should carry that name, so a pool key and a relay identity line up without a translation table. If it is something else, the doc comment should say what. PoolLocation.cluster then either goes away or is documented as the physical Kubernetes cluster name. Same question I left on #17208; the answer belongs here.

2. ReplicaId / ReplicaEndpoint are Global Router replicas, but "replica" is also what we call relay replicas and worker replicas. RoutersResponse.routers: Vec<ReplicaEndpoint> has the field and the type disagreeing. RouterReplica with routers: Vec<RouterReplica> fixes both. #17208 uses the same names, so change in both or neither.

Smaller, take or leave:

• WatchEvent::Added means new or changed per its doc. Upserted stops readers assuming the kube Added/Modified split.
• Lease::lease_duration() repeats the type name. duration() or ttl().
• Capacity.count is a count of workers per the doc. workers.
• capabilities holding "aggregated" is a serving mode, not a capability. Fine for v1; the field will grow a second meaning.

🤖 Generated with Claude Code, reviewed and posted by @nnshah1.

[dmitry-tokarev-nv]

No P0 or P1 found. This review read head c6efecb, which merges main at 2f296b5.

Finding from this review:

• [P3] lib/global-common/src/discovery.rs:4-6: no test holds the promise that a reader skips unknown fields. It is inline, with a tested test.

Open items from other reviewers:

• discovery.rs:140 (dynamo-review-agent): relay_identities replaces the singular relay_identity of the global-router-v2 service and client. A reply on that thread has measurements.
• discovery.rs:214 and pool.rs:159 (dynamo-review-agent): two duplicate test assertions.
• nnshah1 holds the PR on the name PoolKey.site_id against the relay's cluster_id (issue comment of 2026-10-27).

What this review measured.

• Scope: 8 files, +550/-0 against main. The root Cargo.toml line adds dynamo-global-common to [workspace.dependencies]. lib/global-common was already a workspace member. Cargo.lock adds no package. It adds 5 edges from dynamo-global-common to crates that main already locks: blake3 1.8.7, serde 1.0.228, serde_json 1.0.150, thiserror 2.0.18 and url 2.5.8.
• Tests: at c6efecb and on a merge with main at 457fba9, cargo test --locked -p dynamo-global-common passes 8 tests. The merge has no conflict. Clippy with -D warnings, cargo fmt --check and rustdoc with -D warnings are clean, with cargo 1.96.1. On the merge, the tests of dynamo-global-discovery-client from #17208 also pass. A merge with main at 9be9538 also has no conflict. The 3 newer commits on main change no Cargo.toml, Cargo.lock or lib/global-* file.
• Lease reply and router list: this crate, the #17208 client and the global-router-v2 client accept and reject the same 16 lease bodies and 7 router bodies. This crate and the global-router-v2 service write the same JSON for both.
• Registration body: the differences are the ones that the description names. This crate rejects an empty or blank model and a bad key field. This review ran the #17208 client against a local axum service that takes these types. The client gets the lease for a valid body and a 422 for a bad model or key. This crate also accepts a body without capabilities, frontends or capacity, which the #17208 type rejects. The #17208 client always sends all three.
• Pool list: apart from the relay identity field, one pool with a bad key makes this crate reject the whole list. The global-router-v2 client accepts that list, and its router skips that one pool.
• Pool ID: the derivers of this PR, of global-router-v2 and of #15295 give the same ID for 7 inputs. The pinned ID is one of them.
• Checks: each of 10 deliberate breaks of the parse checks fails an existing test. The breaks remove each validate clause, the key check, the model check and each lease check. They also add back a one-day lease cap and drop to_canonical.
• Release staging: .github/scripts/stage_crates.py selects the same 29 crates in the same order at base and head. dynamo-global-common was in that set before this PR.
• Description: the claims that this review measured hold. These are the 8 tests, clippy, the pinned ID, the 422, the lease checks and the one-day bound in #17371.
• CI on c6efecb: PR run 40614797175 passed. Pre Merge run 40614792813 failed only rust-tests (.), on the dynamo-mocker test generalized_live::tests::cancellation_does_not_end_a_pass_with_unrelated_pending_output. This crate's 8 tests passed in that job. The diff does not reach that test. The dependency tree of dynamo-mocker is the same at base and head. The same test failed in Pre Merge run 40635050407 of pull request 17074, which changes only lib/llm/src/preprocessor.rs. Each skipped job needs a filter that this diff does not match. Those filters are operator, deploy tests, snapshots, DGDR, dev and EFA images, power agent, planner gym, docs and examples. The one exception is allure-report, which is off for every pull request (if: false).

[atchernych]

@nnshah1 thanks, all of it is in 4323f2a and 5aadafa:

1. site_id is the relay's cluster ID in string form, so it is now cluster_id, in Rust and in JSON. Its doc says it is the cluster ID the pool's PoolRelay runs with (--cluster-id, DYN_CLUSTER_ID), and that where MCS also reports the pool, that ID must be the cluster's name in the ClusterSet, so a registration and the MCS view share one key. PoolLocation.cluster is gone, since the key carries the cluster. Pool IDs do not change: the deriver hashes the values, not the field names, and the pinned ID still passes.
2. RouterReplica, RouterReplicaId and RouterReplicaEvent, so it reads routers: Vec<RouterReplica>. No JSON change.
3. The smaller ones too: WatchEvent::Upserted, Lease::duration(), and Capacity.workers (also in JSON). capabilities stays as is for v1, as you suggested.

As you and @jioffe502 asked on #17208, the client moves onto these types in this PR: lib/global-discovery-client drops its own copies and re-exports the shared types its calls use, so one PoolKey exists on main. Its calls and errors do not change; what it sends follows the renames.

@dmitry-tokarev-nv your unknown-field test is in, moved to the new names, and both duplicate asserts are gone. The relay identity thread has a reply.