Repository navigation
api: delete the unused sandbox_ip / access_sandbox_ip substrate surface - #1159
Merged
Merged
Conversation
|
Thanks for opening this pull request! A SPACE maintainer will take a look. CONTRIBUTING.md explains how we review outside contributions. |
ac-pplx
force-pushed
the
ac/substrate-delete-sandbox-ip
branch
from
October 30, 2026 17:32
42921bc to
6f799c4
Compare
Contributor
Author
|
Merge note for open PRs that touch code this PR deletes. @the-gigi-pplx, these are yours: #1068, #1131, #1172, #1175 and #1015 edit bodies of |
ac-pplx
marked this pull request as ready for review
October 30, 2026 21:11
kumarpallav-pplx
self-requested a review
October 30, 2026 21:46
Collaborator
Review — 1 findingLow
Checked and holds:
Not checked: I didn't run cargo, and relied on this head's green CI for compilation and the tests the description lists. Verified against |
the-gigi-pplx
approved these changes
Oct 30, 2026
the-gigi-pplx
left a comment
the-gigi-pplx
left a comment
Collaborator
There was a problem hiding this comment.
Approved. 1 non-blocking finding in the review comment above.
SubstrateAdapter carried two required methods, `sandbox_ip` and `access_sandbox_ip`, with a forwarding arm each, a Kubernetes implementation, no-op Direct implementations, and SandboxController wrappers. Nothing outside tests called the wrappers: every data-plane caller takes a node-api `RuntimeClient` from the substrate (`access_runtime_client`, `runtime_client` and their siblings), and nothing asks it for a bare sandbox IP. Every reader of the trait and every future dispatch change paid for the dead surface. Remove both trait methods, their forwarding arms, the Kubernetes and Direct implementations, the controller wrappers, and the test doubles' panicking stubs. Also remove what existed only for them: the reconnect-outcome helpers the wrapper used and their unit test; the Kubernetes test that drove dormant access through `access_sandbox_ip`; and `SandboxAction::Access`, whose only production constructor was the wrapper. The drain-gate test that passed it now passes `SandboxAction::Spaced`, since the gate uses the action only to label its 409, and the characterization test's internal-label list drops it. Of the deleted Kubernetes test's invariant, two tests still pin parts: `legacy_paused_restore_preserves_handoff_without_route_identity` (a dispatched Wake keeps its operation owner and does not publish the route, through `restore_evacuated_execution`) and `only_a_proven_no_send_wake_failure_may_release_the_operation` (the release predicate). Update comments and the Direct module doc that named the deleted items, including the lock-contention arm of `reconnect_controller_error_outcome`. That arm is now pre-emptive: the function's only production caller, the Wake admission check, passes cluster-admission 429s. Left in place: the `(_, "access") => ReconnectEntrypoint::Dataplane` reconnect-metric mapping, which no production path produces now; the `suspend_restore_storage_uri` parameter of `ensure_execution_ready_for_access`, now `None` at every call site; and the arms of `reconnect_controller_error_outcome` that only tests reach. Lower four file-size caps in architecture-baseline.toml to the counts `cargo arch` reports for the changed files: `sandbox/lifecycle.rs` 1825 -> 1815, `sandbox/mod.rs` 1193 -> 1086, `substrate_adapter.rs` 2467 -> 2344, and `substrate/mod.rs` 2771 -> 2089. The `substrate/mod.rs` drop includes 646 lines of slack that predate this change. `direct.rs` shrank too, but its cap is already below its count and stays. In-process only; no wire, schema or config change. Revert to roll back.
ac-pplx
force-pushed
the
ac/substrate-delete-sandbox-ip
branch
from
October 30, 2026 23:28
6f799c4 to
b7631d0
Compare
ac-pplx
added a commit
that referenced
this pull request
Nov 2, 2026
Every archcheck rule is to be required from the day it lands, with exceptions that only shrink: no new baseline entries, no raised caps, and stale entries removed. A rule that can't be enforced that way is deleted. The README now states this policy under "Configuration and modes". pattern_ratchet.direct_tables, also advisory, stays because open pull requests shrink it (#1202). The advisory file_size rule fails that test. Its baseline was seeded with 99 entries on 2026-10-13, reached 102, and lists 98 production files over 1,000 code lines today; #1156 and #1159 lowered caps. What can't be made required is listed_no_growth: in the 14 days to 2026-10-31, 149 of main's 371 first-parent commits (40%) added more lines than they deleted to one of the 98 listed files (git log --numstat, raw lines). Requiring it would block about that share of pull requests. The change removes the file_size module, its configuration code and its [rules.file_size] tables in ARCHITECTURE.toml, its [file_size] baseline table and base-copy read, its part of the generated header, and --write-baseline=file_size. ProductionFile's code_lines field and count_code_lines go too, since only file_size read them. The baseline was edited by hand, so the other tables keep their comments, and its header matches what --write-baseline now generates. Kept: the shared count ratchet (rules/ratchet.rs), which pattern_ratchet uses, and the code-line definition (context::code_lines). Two small changes follow from [file_size] no longer always being present: - Baseline::write puts one blank line between whichever parts it writes. Without this, a baseline with no table of caps gets a doubled blank line. - When the base baseline is not TOML and no rule compares tables with it, skipped() falls back to each rule's own lines. Without this, the warning ends in "; skipped". crate_names then reports the unreadable copy itself. Tests that used file_size for behaviour the pattern ratchets share now use a pattern_ratchet fixture. These cover renames (merged into pattern_ratchet_renames), --renames from git, the human report, rdjson (document, exact paths, stdout/stderr), base-baseline problems, the skip-line and no-base checks, implicit members, scoped and unscoped --write-baseline, and write errors. The rows of lenient_base_baseline and behaviours not covered elsewhere moved into the pattern_ratchet tables. Tests that only exercised file_size are deleted: its behaviours table, its modes test, its limit digit grouping, write_baseline_output's limit cases, and configuration rows for its keys. New assertions pin Baseline::write's layout, with and without a table of caps, and the one warning that names every pattern_ratchet instance's comparisons for a base copy that is not TOML. The layers tests change only their fixtures and the expectations that follow from them: their [file_size] lines are gone from configurations and baselines, so expected baseline line numbers move up by one, the warning for a base copy that is not TOML no longer names file_size's comparisons, and a scoped write keeps a stale [crate_names] entry where it kept a stale [file_size] one. The CI ordering script's comment no longer says the report lists file_size findings first. Its test fixtures use a pattern_ratchet code and table, with each case's meaning unchanged. Five api test modules' doc comments no longer cite the file-size guidance.
meefs
pushed a commit
to meefs/SPACE
that referenced
this pull request
Nov 4, 2026
…rplexityai#1222) Every archcheck rule is to be required from the day it lands, with exceptions that only shrink: no new baseline entries, no raised caps, and stale entries removed. A rule that can't be enforced that way is deleted. The README now states this policy under "Configuration and modes". pattern_ratchet.direct_tables, also advisory, stays because open pull requests shrink it (perplexityai#1202). The advisory file_size rule fails that test. Its baseline was seeded with 99 entries on 2026-10-13, reached 102, and lists 97 production files over 1,000 code lines today; perplexityai#1156 and perplexityai#1159 lowered caps. What can't be made required is listed_no_growth: in the 14 days to 2026-10-31, 149 of main's 371 first-parent commits (40%) added more lines than they deleted to one of the 97 listed files (git log --numstat, raw lines). Requiring it would block about that share of pull requests. Two sbx-mgr SQLite checks go too, since the required layers rule now covers them: layers.storage_access lets only stores, storage helpers and baselined edges depend on a database driver or a storage helper, and layers.store_edges lets only a store's listed users depend on it. - dependency_fence, the whole rule, with its sbxmgr_rusqlite and sbxmgr_sqlx instances. Every package they fenced (rusqlite, libsqlite3-sys, pplx-space-sqlite, sqlx, sqlx-core, sqlx-sqlite) is a [rules.layers.storage] driver or helper, which storage_access keeps out of the normal and build dependencies of every sbx-mgr crate but its two stores. - pattern_ratchet.sbxmgr_sqlite: twelve regexes for SQLite calls and SQL literals in sbx-mgr's crates, which aliases escaped; storage_access confines SQLite to the stores at the crate level instead. pattern_ratchet.sbxmgr_sqlite_schema stays, because layers does not see what it catches: the storage checks read only dependency edges, skip stores and helpers and tolerate baselined edges, and state.writes scans only stores' and helpers' write statements, so db-migrate, a store with sqlx and rusqlite, could name sbx-mgr's migrator again, and a node service with a baselined driver edge could open SANDBOX_MANAGER_DB. Its comment now says so instead of contrasting it with sbxmgr_sqlite. What this narrows: - pplx-space-sbxmgr-migrations, an l30_storage crate, may now take rusqlite, libsqlite3-sys and pplx-space-sqlite, which sbxmgr_rusqlite allowed only the store. CI's run_sbxmgr_production_checks, which asserts which crates bring rusqlite and pplx-space-sqlite into the server's normal graph, still rejects a normal dependency on either there, but not a build or dev dependency, nor libsqlite3-sys. - The migrations crate's sources are no longer scanned for SQL text; state.writes still reports a write statement there that names another store's table. - Dev-dependencies, which the fences covered and storage_access leaves to tests. - pplx-space-sbx-mgr's baselined storage_access edge to the pplx-space-common-jobs helper, which it uses for job types today and which sbxmgr_sqlite watched for calls into common-jobs' SQL functions. sbxmgr_sqlite's baseline table was empty. The rule's module, configuration and context code, its cli test, sbxmgr_sqlite's repository tests and their helper, and the rule's README section are deleted. The fence test's dev-dependency case was the only test of a finding on a dependency declared in a table of its own ([dependencies.sqlx]), which places every layers finding on such a dependency; layers_storage_checks gains that case. Comments in ARCHITECTURE.toml, run-rust-checks.sh, the sbx-mgr store's AGENTS.md and pplx-space-sqlite's crate docs now name storage_access where they named the fences. scripts/move_crates.py drops its check of dependency_fence member globs, and that check's test. The change removes the file_size module, its configuration code and its [rules.file_size] tables in ARCHITECTURE.toml, its [file_size] baseline table and base-copy read, its part of the generated header, and --write-baseline=file_size. ProductionFile's code_lines field and count_code_lines go too, since only file_size read them. The baseline was edited by hand, so the other tables keep their comments, and its header matches what --write-baseline now generates. Kept: the shared count ratchet (rules/ratchet.rs), which pattern_ratchet uses, and the code-line definition (context::code_lines). Two small changes follow from [file_size] no longer always being present: - Baseline::write puts one blank line between whichever parts it writes. Without this, a baseline with no table of caps gets a doubled blank line. - When the base baseline is not TOML and no rule compares tables with it, skipped() falls back to each rule's own lines. Without this, the warning ends in "; skipped". crate_names then reports the unreadable copy itself. Tests that used file_size for behaviour the pattern ratchets share now use a pattern_ratchet fixture. These cover renames (merged into pattern_ratchet_renames), --renames from git, the human report, rdjson (document, exact paths, stdout/stderr), base-baseline problems, the skip-line and no-base checks, implicit members, scoped and unscoped --write-baseline, and write errors. The rows of lenient_base_baseline and behaviours not covered elsewhere moved into the pattern_ratchet tables. Tests that only exercised file_size are deleted: its behaviours table, its modes test, its limit digit grouping, write_baseline_output's limit cases, and configuration rows for its keys. New assertions pin Baseline::write's layout, with and without a table of caps, and the one warning that names every pattern_ratchet instance's comparisons for a base copy that is not TOML. Apart from the layers_storage_checks case above, the layers tests change only their fixtures and the expectations that follow from them: their [file_size] lines are gone from configurations and baselines, so expected baseline line numbers move up by one, the warning for a base copy that is not TOML no longer names file_size's comparisons, and a scoped write keeps a stale [crate_names] entry where it kept a stale [file_size] one. The CI ordering script's comment no longer says the report lists file_size findings first. Its test fixtures use a pattern_ratchet code and table, with each case's meaning unchanged. Five api test modules' doc comments no longer cite the file-size guidance.
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.
Summary
Deletes
sandbox_ipandaccess_sandbox_ipfromSubstrateAdapter, plus the code that existed only to support them (the leftovers are listed under "Not covered"). Neither method had a production caller. Every data-plane caller takes a node-apiRuntimeClientfrom the substrate (access_runtime_client,runtime_clientand their siblings), and nothing asks it for a bare sandbox IP. Every reader of the trait and every dispatch change was paying for dead surface.Removed:
Both trait methods, their two forwarding arms in
impl SubstrateAdapter for Substrate, and the Kubernetes and Direct implementations.The
SandboxController::sandbox_ip/access_sandbox_ipwrappers.The panicking stubs in the two test-double substrates in
create_postgres_tests.rs.initial_lifecycle_may_need_wakeandsuccessful_access_reconnect_outcome, which only the wrapper used, and the unit test ofsuccessful_access_reconnect_outcome.SandboxAction::Access. The wrapper was the only production code that built it. The drain-gate test that passed it now passesSandboxAction::Spaced; the gate uses the action only to label its 409, and the test asserts only the status. The characterization test's internal-label list drops it.The Kubernetes test that drove dormant access through
access_sandbox_ip(dormant_access_preserves_operation_after_dispatched_wake_failure). Two tests still pin its invariant:legacy_paused_restore_preserves_handoff_without_route_identity: a dispatched Wake does not publish the route and keeps its operation owner, throughrestore_evacuated_execution.only_a_proven_no_send_wake_failure_may_release_the_operation: the release predicate.adapter_characterization_tests/restoring_access.rscoversrestoring, whichis_dormant_lifecycleexcludes, so it does not stand in for the deleted test.Updated the comments and the Direct module doc that named the deleted items. That includes the lock-contention arm of
reconnect_controller_error_outcome, which is now described as pre-emptive: its only production caller, theWake/ImplicitWakeadmission check inSandboxController::action, passes cluster-admission 429s. Helpers the deleted code shared still have other callers and stay:resolve_pod_ip,selected_wake_pod_ip,suspend_restore_plan,charge_implicit_access_activation,ensure_access_path_not_blocked_by_drain, and others.File-size caps in
architecture-baseline.toml, lowered to the new counts:controllers/sandbox/lifecycle.rs: 1825 → 1815controllers/sandbox/mod.rs: 1193 → 1086controllers/substrate/kubernetes/adapter/substrate_adapter.rs: 2467 → 2344controllers/substrate/mod.rs: 2771 → 2089. This drop includes 646 lines of slack that predate this change.controllers/substrate/direct.rsshrank too, but its cap (3907) is already below its count, so it is unchanged.Validation
Before deleting,
rg -n "sandbox_ip|access_sandbox_ip"acrosscrates/,space_smoke/,scripts/,examples/anddocs/found only the definitions, forwarding arms, impls, the wrapper, test-double stubs, one Kubernetes test and comments. The only other hit, inspace_smoke, is an unrelated local variable namedsandbox_ips. After removingSandboxAction::Access, a grep of docs, Python and Go found nothing that names theaccessaction label or its drain message. Theaccesshits there are the auto-lifecycle wake source, a separate vocabulary.At this head, rebased on current main (the rebase resolved #1153's
direct.rsgap table, kept as merged, and aruntime_upgrade: Noneline #1015 added inside the deletedaccess_sandbox_ip; caps recounted withcargo arch):cargo fmt --all -- --check: clean.cargo check -p pplx-space-api(nokubernetesfeature) andcargo check --workspace --all-features --all-targets: clean.cargo clippy --workspace --locked --all-targets --all-features -- -D warnings: clean.cargo arch: 0 required findings; against main's baseline, no raised caps or new entries.cargo nextest run -p pplx-space-api --all-features --no-fail-fastagainst real Postgres and Redis, started by the nextest setup scripts: 2126 passed, 1 skipped by nextest's config, 0 failed, no test-dependency skips. The run includesnon_lifecycle_actions_are_bad_requests_and_write_nothing,drain_gate_blocks_dormant_sandbox_on_draining_target_node,reconnect_tags_classify_cluster_admission_rejection,legacy_paused_restore_preserves_handoff_without_route_identityandonly_a_proven_no_send_wake_failure_may_release_the_operation.No new tests: nothing behavioural changes, and the compiler is the oracle for the deletion. Not run: Kind or smoke tests, since there is no wire, schema or config change.
Severity and risk
Severity: does not apply (dead-code removal). Risk: low. The surface is in-process only, and the compiler proves nothing else in the workspace referenced it. Roll back by reverting the commit.
Rollout
No wire, schema or config change. Trial merges against the current open-PR heads give:
direct.rsmodule doc with api: cap each client's live sandboxes, counted from the pod reflector and a Redis pending set #1172, and on thelifecycle.rs[file_size]line with api: refuse a cold create with a 503 when every eligible node's network slots are held #1061, which also lowers that cap; whichever lands second recounts it.Not covered
(_, "access") => ReconnectEntrypoint::Dataplanereconnect-metric mapping and its"dataplane"tag. The wrapper was their only production producer; existing tests still exercise them. Left for a follow-up.suspend_restore_storage_uriparameter ofensure_execution_ready_for_access, nowNoneat every call site. Left because an open PR edits that line.reconnect_controller_error_outcomethat only tests reach now: its one production caller passes admission 429s. Whether to simplify them is a design call for the follow-up; the code and its tests stay.AI assistance: Claude Code wrote the code change, the commit message and this description.