From 74fbd44d244398775f0cce84e4fbb6c486a35a8d Mon Sep 17 00:00:00 2001 From: Abhinav Chordia Date: Tue, 13 Oct 2026 03:54:40 +0000 Subject: [PATCH] api: document the Direct substrate's contract for agents Add a "Direct Substrate Contract" section to the substrate AGENTS.md and a module doc on direct.rs, so agents changing the substrate, or shared code that branches on it, know what Direct is for and where its contract ends. The AGENTS.md section holds the rules: - when the api selects Direct (no `kubernetes` feature, or SPACE_K8S_CONTROL_PLANE not truthy) and that a Kubernetes init failure is an error, not a fallback; - implement new lifecycle operations for Kubernetes; Direct returns Unsupported unless the operation is one of the calls examples/first_sandbox.py makes (create, command session, list/inspect, delete); - make new SubstrateAdapter operations required methods, so the compiler demands each substrate's implementation and the forwarding arm in `impl SubstrateAdapter for Substrate`; only a "nothing to do" hook that shared code calls on every path keeps a default, and it still needs its arm; - where shared code needs a static fact about the substrate, add a SubstrateCapabilities field, set it in DIRECT and KUBERNETES, and read it through capabilities() instead of matching the substrate; where the substrate must act, add a required method (#368); - in new SQL, scope to a substrate's rows by binding the value (`substrate.managed_by()` for the running substrate's rows, or a fixed `SubstrateKind::Kubernetes`/`::Space` for one substrate's rows) instead of a 'k8s'/'space' literal; existing literals are debt (#373); - keep writes to the Direct-only tables in direct.rs and the Direct node registry; - don't cite a cluster-controller reconciler as the retry path for a failed Direct execution retire or repair, and don't change a known gap as a side effect of other work; - existing substrate matches, Direct-table joins in shared code and Unsupported trait defaults are debt, not precedent; - which test fixtures to use: Direct only for tests under `direct/` or substrate-independent behavior, `kubernetes::tests` helpers for shared tests, `scripted_test_doubles.rs` inside `kubernetes/`, and that both compile only with `--features kubernetes`. The direct.rs module doc records what Direct does today, so it is reviewed with the code: how it reaches workers, which lifecycle operations it implements, which it rejects as Unsupported (node-pool placement, forks, cluster pool configs), its no-op trait methods, its Direct-only admin routes, how admin drain/cordon actions fail on a Direct node, and which cluster-controller loops are Kubernetes-only: the drain, orphan, failed-row repair and node-sync loops never run for Direct, while the public-operation dispatcher runs for both substrates and retries transient operation failures. Nothing retries a failed Direct retire. It lists two gaps that succeed silently: one-shot create does not forward requested vCPU or memory (disk size is forwarded), and retire_execution does not enqueue the stopped-disk backup the trait documents. Correct the comment in Direct retire_execution that named the failed-cleanup pass as its retry path: that pass selects only 'k8s' rows, so a failed Direct retire is not retried. Correct the `resolves_cold_create_shape` field doc, which said that without the capability the requested shape reaches the node unchanged: Direct's one-shot create cannot carry vCPU or memory. Make the rules reachable from the code they govern: add the `controllers/substrate/CLAUDE.md` symlink, widen the pointer in crates/libs/api/AGENTS.md, and point the cluster-controller and internal-api AGENTS.md files at the section, with the cluster controller's binding rule for `managed_by` scoping stated there, including `tombstone_retention`'s fixed scope to Direct tombstones. The Direct-table rule names archcheck's `pattern_ratchet.direct_tables` rule (#358) as its enforcement, advisory today. The SQL-scoping rule also covers returning early in Rust when a fixed-scope statement may run under another substrate (#373), and names the shared `routable_execution_or_non_k8s` fragment as remaining debt. The Direct retire wording now says the sweeper's failed-cleanup retry does nothing unless the running substrate is Kubernetes, matching the code after #373. Documentation and comments only; no code, schema, wire or configuration change. --- crates/libs/api/AGENTS.md | 4 +- .../api/src/controllers/substrate/AGENTS.md | 67 +++++++++++++++++++ .../api/src/controllers/substrate/CLAUDE.md | 1 + .../api/src/controllers/substrate/direct.rs | 53 ++++++++++++++- .../libs/api/src/controllers/substrate/mod.rs | 3 +- .../libs/cluster/controller/server/AGENTS.md | 6 ++ crates/libs/internal/api/server/AGENTS.md | 4 ++ 7 files changed, 134 insertions(+), 4 deletions(-) create mode 120000 crates/libs/api/src/controllers/substrate/CLAUDE.md diff --git a/crates/libs/api/AGENTS.md b/crates/libs/api/AGENTS.md index 63f165a4fb..fa4599fed3 100644 --- a/crates/libs/api/AGENTS.md +++ b/crates/libs/api/AGENTS.md @@ -1,6 +1,8 @@ # `pplx-space-api` -Before changing the substrate, read `src/controllers/substrate/AGENTS.md`. +Before changing the substrate, or shared code that branches on the substrate +(`Substrate::Direct`, `managed_by()`) or names a Direct-only table, read +`src/controllers/substrate/AGENTS.md`. ## Changes diff --git a/crates/libs/api/src/controllers/substrate/AGENTS.md b/crates/libs/api/src/controllers/substrate/AGENTS.md index 6865edcbcc..216e0f12bb 100644 --- a/crates/libs/api/src/controllers/substrate/AGENTS.md +++ b/crates/libs/api/src/controllers/substrate/AGENTS.md @@ -26,6 +26,63 @@ view doesn't carry, add it to the view and its conversion; don't add a second cache or fall back to a full-object GET to get it. Annotation and label reads go through `SandboxPodKey`, which is also the set of keys the conversion keeps. +## Direct Substrate Contract + +`DirectSubstrate` (`direct.rs`) exists for single-host deployments without +Kubernetes and for builds without the `kubernetes` feature. +`Substrate::from_env_with_lifecycle` returns Kubernetes only when built with +that feature and `SPACE_K8S_CONTROL_PLANE` is truthy; an unset, false or +unrecognized value selects Direct, and a Kubernetes init failure is an error, +not a fallback. Shipped images and Kind enable the feature; a plain +`cargo build -p pplx-space-api-svc` does not. + +What Direct does today (its `Unsupported` and no-op methods, admin routes, +Kubernetes-only reconcilers and known gaps) is in the `direct.rs` module doc; +update it with any change to Direct. Don't cite a cluster-controller +reconciler as the retry path for a failed Direct execution retire or repair; +the `direct.rs` module doc lists the Kubernetes-only loops. Don't change a +known gap as a side effect of other work: rejecting or implementing one +changes Direct behavior and belongs in its own PR. + +For new work: + +- Implement new lifecycle operations for Kubernetes. On Direct, return + `SubstrateError::Unsupported` unless the operation is one of the calls + `examples/first_sandbox.py` makes (create, command session, list/inspect, + delete). +- Make a new `SubstrateAdapter` operation a required method, so the compiler + demands each substrate's implementation and its arm in + `impl SubstrateAdapter for Substrate`; Direct returns + `SubstrateError::Unsupported` explicitly. A default would let a forgotten + arm give both substrates the default and skip the Kubernetes override. Only + a hook shared code calls on every path may default to "nothing to do", as + `retire_fence_mismatch` does, and it still needs its arm. +- Where shared code needs a static fact about the substrate, add a + `SubstrateCapabilities` field, set it in both `SubstrateCapabilities::DIRECT` + and `::KUBERNETES`, and read it through `capabilities()`; where the + substrate must act, add a required method (above). Don't match + `Substrate::Direct` or branch behavior on `managed_by()`. Matching + `Substrate::Kubernetes(k)` to reach Kubernetes-only state is fine. +- Scoping a query to the rows a substrate owns is required, not debt. In new + SQL, bind the substrate value instead of a `'k8s'`/`'space'` literal: + `managed_by = $n` with `.bind(substrate.managed_by())` for the running + substrate's rows, as the transient-lifecycle sweeper does, or a fixed + `SubstrateKind::Kubernetes`/`::Space` where code targets one substrate's + rows. When a fixed-scope statement may run under another substrate, return + early in Rust instead of also ANDing the running substrate's value, as the + sweeper's failed-active-execution cleanup does. Existing literals, such as + the shared `routable_execution_or_non_k8s` fragment, are debt. Never choose + a runtime endpoint from a row's label. +- `node_direct`, `sandbox_direct_route_identities` and + `sandbox_direct_route_provenance` belong to Direct. Outside tests, write + them only from `direct.rs` and the Direct node registry; read them through + an existing query that already joins them rather than adding a join. + archcheck's `pattern_ratchet.direct_tables` rule enforces this (advisory + today). +- Other existing substrate matches, Direct-table joins in shared code and + `Unsupported` trait defaults (`list_pool_configs`, `upsert_pool_config`, + `disable_pool_config`) are debt, not precedent. + ## Tests Kubernetes pod rendering is owned by `pplx_space_common_core::kubernetes`. @@ -35,3 +92,13 @@ restore, and pooled manifests in one place. Keep narrow tests only for behavior that is hard to express clearly in the broad YAML contract test. + +Use `Substrate::Direct` only to test Direct behavior (tests under `direct/`) or +behavior that genuinely doesn't depend on the substrate. Test shared lifecycle +behavior Kubernetes deployments depend on against a `Substrate::Kubernetes` +built with the `kubernetes::tests` helpers (`substrate_with_store` and its +variants), as `routes/public/sandboxes/admission_test_fixture.rs` does. +Adapter tests inside `kubernetes/` use `scripted_test_doubles.rs`, as +`adapter_characterization_tests/` does. Both compile only with +`--features kubernetes`: CI's `--workspace --all-features` run includes them, +but `cargo test -p pplx-space-api` silently skips them. diff --git a/crates/libs/api/src/controllers/substrate/CLAUDE.md b/crates/libs/api/src/controllers/substrate/CLAUDE.md new file mode 120000 index 0000000000..47dc3e3d86 --- /dev/null +++ b/crates/libs/api/src/controllers/substrate/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/crates/libs/api/src/controllers/substrate/direct.rs b/crates/libs/api/src/controllers/substrate/direct.rs index c2463d9f57..fd4716b9b3 100644 --- a/crates/libs/api/src/controllers/substrate/direct.rs +++ b/crates/libs/api/src/controllers/substrate/direct.rs @@ -1,3 +1,52 @@ +//! The Direct substrate: [`DirectSubstrate`] dials each worker's node-api at +//! `node_direct.node_api_endpoint`, with no sandbox pods. The rules for +//! changing it are in this directory's `AGENTS.md`. Below is what it does +//! today: a selection, not a complete classification. +//! +//! # Operations +//! +//! - Create/delete, start, suspend/restore and wake go through +//! [`SubstrateAdapter`]. Stop, kill, pause/resume and checkpoint go through +//! the shared controller over Direct's runtime clients, which never recreate +//! an execution. +//! - Warm-pool claims adopt node-side; custom vcpu/memory or encrypted claims +//! go cold. Admission-time pool slot claims always miss. +//! - [`SubstrateError::Unsupported`]: node-pool placement, forks and cluster +//! pool configs. +//! - No-ops returning `None` or nothing: `sandbox_ip`, `access_sandbox_ip`, +//! `list_inventory_nodes` (Postgres is the inventory), +//! `retire_fence_mismatch` and `observe_pod_identities`. +//! `retire_execution` ignores its fence, but still stops the sandbox and +//! clears its routing. +//! - Direct-only: the node registry and the template pull and delete routes in +//! `routes/admin/node_maintenance.rs` (501 on Kubernetes). +//! - No drain or evacuation. After drain-state validation, admin `:drain`, +//! `:undrain`, `:cordon` and `:uncordon` return 502 on a Direct node; +//! `:drain` still commits `drain_phase='draining'`, which nothing advances. +//! +//! # Kubernetes-only reconcilers +//! +//! The cluster controller's `run_reconcilers` starts its drain, orphan, +//! failed-row repair and node-sync loops only for Kubernetes. The loops it +//! starts for both substrates include the public-operation dispatcher, which +//! retries transient failures of the create, lifecycle and delete operations +//! it executes. Direct reaches `retire_execution` only through the +//! transient-lifecycle sweeper's best-effort `cleanup_after_sweep`, and the +//! sweeper's failed-cleanup retry does nothing unless the running substrate is +//! Kubernetes, so nothing retries a failed Direct retire. +//! +//! # Known gaps +//! +//! Both succeed silently today: +//! +//! - One-shot create (`use_one_shot_create`) does not forward requested vCPU +//! or memory overrides: sbx-mgr's `CreateRequest` has no fields for them, so +//! autostart uses node-side defaults and memory policy while +//! `public.sandboxes` records the requested values. Disk size is forwarded. +//! - `retire_execution` never enqueues the stopped-disk backup that +//! [`SubstrateAdapter::retire_execution`] documents; the Kubernetes teardown +//! does. + use pplx_space_common_core::sql; use pplx_space_common_svc::sql::contention; use std::fmt; @@ -3768,8 +3817,8 @@ impl SubstrateAdapter for DirectSubstrate { let (_, _, mut client) = self.worker_client_for_sandbox(sandbox_id).await?; // Unlike `delete_execution`, the stop is the whole operation here, so - // its outcome must propagate: a failure keeps the caller's retry loop - // (e.g. the cluster-controller failed-cleanup pass) re-driving it. + // its outcome must propagate to the caller. Nothing retries a failed + // Direct retire: the failed-cleanup pass skips every substrate but Kubernetes. client .stop_sandbox(pplx_space_sbx_mgr_client::proto::StopRequest { sandbox_id: sandbox_id.to_string(), diff --git a/crates/libs/api/src/controllers/substrate/mod.rs b/crates/libs/api/src/controllers/substrate/mod.rs index 7669369fdf..5f1cc8437c 100644 --- a/crates/libs/api/src/controllers/substrate/mod.rs +++ b/crates/libs/api/src/controllers/substrate/mod.rs @@ -2194,7 +2194,8 @@ pub struct SubstrateCapabilities { /// fallthrough create takes a configured or policy pool shape, and /// otherwise vCPU and memory the request left unset come from the /// template's build shape. Without it, the requested shape goes to the - /// node unchanged. + /// node unchanged, except that Direct's one-shot create cannot carry vCPU + /// or memory (see the [`direct`] module doc). pub resolves_cold_create_shape: bool, /// Create requests may carry a Kubernetes node-pool placement pin. pub supports_node_pool_placement: bool, diff --git a/crates/libs/cluster/controller/server/AGENTS.md b/crates/libs/cluster/controller/server/AGENTS.md index 6cf48e63eb..2955f7d0b6 100644 --- a/crates/libs/cluster/controller/server/AGENTS.md +++ b/crates/libs/cluster/controller/server/AGENTS.md @@ -22,6 +22,12 @@ Every table named here is in the control-plane Postgres store. - Read a knob a deployment must change with a production `EnvConfig` reader (`src/env_config.rs`); its `test_*` readers return the default unless `test-hooks` is compiled in. +- Scope SQL to a substrate's rows with `managed_by = $n`, never a + `'k8s'`/`'space'` literal, bound from `substrate.managed_by()` when it scopes + to the running substrate's rows, or a fixed `SubstrateKind` when it targets + one substrate (the Kubernetes arm of `run_reconcilers`; + `tombstone_retention`'s Direct tombstones). For what Direct lacks, see the + [Direct substrate contract](../../../api/src/controllers/substrate/AGENTS.md#direct-substrate-contract). - `stuck_container_creating_gc` reads the `orphaned_sandboxes` and transient-lifecycle sweeper configs and runs only when both would retire the rows of the pods it deletes; changing those readers changes its gate. diff --git a/crates/libs/internal/api/server/AGENTS.md b/crates/libs/internal/api/server/AGENTS.md index 5a98bcc91a..743f2d9cf6 100644 --- a/crates/libs/internal/api/server/AGENTS.md +++ b/crates/libs/internal/api/server/AGENTS.md @@ -2,6 +2,10 @@ Every table named here is in the control-plane Postgres store. +Before changing code that branches on the substrate or names a Direct-only +table (`node_direct`, `sandbox_direct_route_*`), read the +[Direct substrate contract](../../../api/src/controllers/substrate/AGENTS.md#direct-substrate-contract). + ## Changes - New ClusterApi and NodeInfo RPCs rely on the auth layers in `src/run.rs`: