api: filter the admin sandbox list by node and assignment, and tighten its parameter checks by ac-pplx · Pull Request #1385 · perplexityai/SPACE

ac-pplx · 2026-11-03T05:55:00Z

Summary

GET /admin/api/v1/sandboxes gains back the node_id and assigned filters
that #1321 dropped in review, and its parameter checks get stricter where they
answered 500 and looser where they broke callers of the unpaged list.

Node filters. For sandbox rows, the filters match the node the same
response reports as active_node_id. A sandbox with a current Direct route is
on its route's node, whatever its row's active_node_id says. Any other
sandbox, Kubernetes-managed or Direct without a route, is on its row's node.
assigned=true keeps the sandboxes reported with a node, assigned=false the
ones without, and node_id with assigned=false answers 400. Like every
filter, they list sandbox rows only. The node-reported rows with no sandbox row
(pool slots and observed-local sandboxes) still come only with the unfiltered
list, so assigned=false leaves them out although their responses report no
active_node_id. The OpenAPI description of assigned says so.

Each request is one statement, with no route read, no transaction and no
second round trip. A read of the disagreeing routes would not stay small. When
a sandbox is written off from a gone node, when a restore is reverted, and when
an activation is rolled back, its row's active_node_id is cleared but its
route is kept until clear_direct_route_identity runs or the row goes. So
controllers/substrate/direct/reported_node.rs exports two whole boolean
predicates, and the admin statements interpolate them. The Direct tables stay
behind Direct's module, so there is no new pattern_ratchet.direct_tables
finding:

reported_on_node(s, $n) first narrows to the sandboxes whose row or route
names the node. The planner finds those through idx_sandboxes_active_node
and the primary key. It then tests
COALESCE(route.current_node_id, s.active_node_id) IS NOT DISTINCT FROM $n
exactly, so it is never NULL: negated, it keeps a sandbox on no node. Both
the page and the unpaged statement use it.
reported_on_a_node(s) is s.active_node_id IS NOT NULL or an EXISTS on
the route's primary key. The page statement uses it. The unpaged statement
already joins the route through {sandbox_from}, so assigned tests the
response's own expression there.

Parameter checks.

Every empty filter parameter except org_id (lifecycle=, template=,
node_id=, assigned=, user_id=, created_after=, created_before=) is
unset, as prefix= already was. Those used to answer 400 on the unpaged
list, which ignored the names before they were filters. org_id, which that
list already parsed, still refuses an empty value. An empty item inside a
lifecycle list is still refused.
A created_after/created_before pair is compared as given, before
created_before is rounded up to its microsecond. A range inverted inside
one microsecond now answers 400 instead of being accepted.
A timestamp outside what Postgres's timestamptz holds (before
4714-11-24 BC) answers 400 instead of 500. That covers the parameters and a
hand-built cursor.
The OpenAPI description of prefix, and its doc comment, say that it
searches sandbox rows only. A node-reported row never matches, even by its
own ID.

Comments. The projection arm's comment, which moved with the handler, said
the projection reads erase the SQLSTATE. They keep it through
db_contention_error, and support::controller_read_error answers contention
as a retryable 503. The comment and its pointer on get_sandbox now say that.
The spaced-version count's doc records why stored deleted rows are filtered
at its FULL JOIN rather than before it (see Validation).

With this, the console can filter its sandbox list by node on the server,
instead of fetching every page and filtering client-side.

Validation

cargo nextest run -p pplx-space-api --all-features: 2196 passed and 1
skipped, with Postgres from the nextest setup script.
cargo clippy -p pplx-space-api --all-targets --all-features -- -D warnings
and cargo fmt --all -- --check pass.
cargo arch against main's baseline and ARCHITECTURE.toml: 112 advisory
findings, the same as main. The predicates are under the ratchet's allowed
controllers/substrate/direct/**. The only changed line is the existing
routes/admin/docs.rs size finding (already over the limit on main),
which grows by 2 lines.
the_node_filters_match_the_node_the_response_reports covers Kubernetes
rows on a node and unassigned, and Direct rows with and without a route. It
includes routes whose node differs from the row's in both directions, and a
route on a row with no node, which is the written-off shape. For each of
node_id (two nodes), assigned=true and assigned=false, it checks the
page, a cursor walk one row a page, and the unpaged statement. For node_id,
it also checks against the response's own active_node_id.
Every filter goes through both statements. each_filter_returns_exactly_its_rows
and the node test above compare the page and the unpaged list with the same
expected rows, so the duplicated filter WHERE cannot drift unnoticed.
an_empty_filter_parameter_is_unset compares each empty filter with the
same list without it, paged and unpaged. It fails without the change.
malformed_list_parameters_answer_400 adds the inverted-microsecond range,
a pre-4714 BC created_after/created_before, and an out-of-range cursor;
the cursor answered 500 before the change. The list's route test pins that
prefix leaves out the node-reported row.

Measured in Postgres 16 on 500,000 synthetic sandboxes: 300,000 written off,
each with no row node and a route on one of 1,000 gone nodes (about 300
each); 100,000 live on 40 nodes, with agreeing routes; 100,000 unassigned
with no route. Times are warm psql round trips of the built statements.
"Before" is the route read plus the statement with its arrays bound.

filter	before	after
`assigned`, page	270 ms read (300,000 IDs) + 355 ms	4 ms
`assigned`, unpaged, `limit=1000`	270 ms read + 359 ms	10 ms
`node_id`, live node (2,500 rows), page	26 ms + 7 ms	22 ms
`node_id`, gone node (291 rows), page	12 ms + 5 ms	13 ms
`node_id`, no rows, page	11 ms + 2 ms	13 ms
`node_id`, unpaged, `limit=1000`	not measured	12-37 ms

Every case was repeated 8 times with no change in plan. Old and new return
identical rows for each filter on this table. Testing only the COALESCE
scans the list's order: 690 ms for a node with no rows, which is why
reported_on_node narrows by index first. The unpaged statement's joined
COALESCE cost 190-420 ms per node, which is why it takes the predicate too.

The count's stored-deleted rows were measured on 1,500,000 rows
(1,000,000 stored deleted, 102,500 observed). Postgres already applies the
deleted filter at the FULL JOIN, below the inventory LATERAL. Dropping
those rows from sandboxes first measured 1,180 ms as an EXISTS, against
1,058 ms. A hashed IN ran in 980 ms, but it becomes a per-row subplan once
the observed rows outgrow hash_mem. The statement keeps its shape, and its
doc says why. the_count_is_the_tally_of_the_full_list still covers stored
deleted rows with and without a node reporting them.
Kind was not run at this head. A route that disagrees with its row cannot be
produced through the API, so the Postgres tests above carry the filters, and
the route tests in routes/admin/sandbox_list/tests.rs carry parsing and the
400s.

Severity and risk

Severity: does not apply (feature restore, plus parameter-check fixes).

Risk: low. The node filters are new optional parameters. Without them, each
statement's node conditions are $n IS NULL tests that Postgres folds away;
the unfiltered page measured 3.6 ms on the table above. reported_on_node scans the route
table once per statement for the node's routes, inside the database, because
current_node_id has no index: about 10 ms at 400,000 routes. More requests
now succeed: an empty filter is unset instead of 400. Fewer fail as 500: an
out-of-range timestamp is 400. Roll back by reverting.

Rollout

While replicas on current main still serve, a paged request (cursor) that
names node_id or assigned answers 400 there, and an unpaged one ignores
them. Send the filters from the console once every API replica runs this
release.

Not covered

An index on sandbox_direct_route_identities (current_node_id) would let
reported_on_node skip its scan of the route table. It is left out because
it needs a migration on a table the activation path writes.

AI assistance: Claude Code drafted the code, tests and this description.

github-actions · 2026-11-03T05:55:10Z

Thanks for opening this pull request! A SPACE maintainer will take a look.

CONTRIBUTING.md explains how we review outside contributions.

the-gigi-pplx · 2026-11-03T06:56:00Z

Review — 2 findings

Medium

crates/libs/api/src/controllers/substrate/direct/route_overrides.rs:62-68 — the assigned read returns every Direct route whose row has no node, and that set isn't small. It grows with every failed or written-off Direct sandbox.
The module doc and the PR body rest on disagreeing routes being "a few, outside a route's move". But three writers clear sandboxes.active_node_id without touching sandbox_direct_route_identities:
- release_lost_placement (crates/libs/sandbox/store/src/lost.rs:86-92), called by the gone-node write-off at cluster/controller/server/src/gone_node_write_off.rs:122;
- unplace_reverted_restore (sandbox/store/src/sweeper.rs);
- the activation-rollback converge in internal/api/server/src/sandbox_quarantine_store.rs:515-520.
Only direct.rs's clear_direct_route_identity deletes a route, and none of those paths calls it. So every Direct sandbox failed by a node write-off keeps its route, with its row's node null, until the sandbox row itself is deleted. Each assigned=true or assigned=false request reads all of those IDs into the API and binds them back as $11, on every page of a cursor walk. The benchmark's 500 rows-with-no-node doesn't model that population. The filter stays correct, since those rows' responses report the route's node too. It's the cost and the doc's premise that don't hold.
The unpaged statement doesn't need the extra read at all: ADMIN_LIST_SQL goes through {sandbox_from}, which already LEFT JOINs direct_route (controllers/sandbox/responses.rs:74). So COALESCE(direct_route.current_node_id, s.active_node_id) = $10, and … IS NOT NULL = $13, can filter on the response's own expression in one statement, as internal/api/server/src/published_state.rs:2076-2078 already does. The page statement's candidate rows have no join, which is the direct_tables ratchet you're working around. A predicate fragment exported from controllers/substrate/direct/ would keep the Direct table's SQL in its owner, and it reads closer to substrate/AGENTS.md:118-122 ("read them through an existing query that joins them") than a second query does. It also drops the transaction and the second round trip. If you keep the read instead, scoping it so it can't scale with failed rows would keep the arrays small. One option is to exclude rows in a terminal lifecycle, though only if their responses' node doesn't matter to the filter.

Low

crates/libs/api/src/routes/admin/sandbox_list.rs (node_id: Option<NodeId>, assigned: Option<bool>) — on the unpaged path, an empty node_id= or assigned= now answers 400, where the list used to ignore both names.
It's the same class as the empty lifecycle= and template= values on PR api: page and filter the admin sandbox list, count spaced versions, and share one sandbox ID parser #1321, and the same fix of treating an empty value as unset would cover it.

Verified against 78818d0ccdce29a7175f1c237e2778d3dee5e6d7, reviewed as its three commits on top of PR #1321's head 411c536b.

What I traced:

Both filter predicates against the response's projection (COALESCE(direct_route.current_node_id, s.active_node_id), responses.rs:30). sandbox_direct_route_identities is keyed by sandbox_id, with current_node_id NOT NULL, so a sandbox has at most one route. The node_id and assigned arms select exactly the sandboxes the response would show on or off that node. An empty array is safe in both ANY and <> ALL.
The writers in finding 1, and that the Direct path writes the row's node in the same transaction as the route on placement (direct.rs:1128-1145).

I didn't run anything beyond reading; CI's Postgres tests at this head are green. Not done: a Kind Direct run.

the-gigi-pplx

Approved. 2 non-blocking findings in the review comment above.

node_id and assigned come back, matching the node the response reports as active_node_id. The Direct module gains current_routes, the owning read of the routed sandboxes; the admin statements take its IDs as bound arrays, so a routed sandbox matches by its route's node and any other by its row's.

…in the page's snapshot The admin list's node filters read every current Direct route and bound all their IDs into the page statement, so each node-filtered page moved the whole route table, whatever its org, cursor or limit. A route matters to these filters only where its node differs from its row's active_node_id, including a row with no node; everywhere else the row answers alone. The Direct module's read (now route_overrides) returns only those routes: for node_id, the ones onto that node from elsewhere and away from a row on it; for assigned, the ones of rows with no node. The admin statements are unchanged. The route read and the page statement were also separate snapshots, so a route moving between them could list a sandbox under one node while the same response reported another. With a node filter set they now run in one REPEATABLE READ, READ ONLY transaction; without one the list runs as before. Tests: the unpaged statement is checked in every node and filter case; the combined cases now depend on their node filter; and a new test commits a route move or removal between the two reads and checks that the page agrees with the node its responses report.

Like every filter, node_id and assigned list sandbox rows only, so assigned=false leaves out the node-reported pool slots and observed-local sandboxes that the unfiltered list appends, although their responses report no active_node_id. The OpenAPI description of assigned, the NodeAssignment doc, and is_unfiltered's doc now say so; is_unfiltered no longer claims no filter could describe those rows.

…o route read The node filters read the Direct routes that disagree with their row and bound them back into the page statement, in one REPEATABLE READ transaction. That read is not small: writing off a sandbox from a gone node, reverting a restore and rolling back an activation clear the row's active_node_id and leave its route, so every written-off Direct sandbox kept its route in the assigned read until its row went. Direct now exports two whole boolean predicates for the node a response reports, and both admin list statements interpolate them in place of the read: reported_on_node narrows by idx_sandboxes_active_node and the primary key before its exact COALESCE test, and reported_on_a_node probes the route by key. The unpaged statement already joins the route, so its assigned filter tests the response's own expression. Each request is one statement, with no transaction and no second round trip. On 500,000 synthetic sandboxes with 300,000 written-off routes, an assigned page went from a 270 ms route read and a 355 ms statement to 4 ms (unpaged: 10 ms), and node pages stay at 13-22 ms; both statements return the old rows for every case measured.

Before these were filters the unpaged list ignored the names, so a caller sending node_id= or assigned= now got a 400. An empty value is unset, paged or not.

created_before was rounded up to its microsecond before it was compared with created_after, so created_after=...0.0000006Z with created_before=...0.0000003Z passed the check. The range is now compared as given and rounded afterwards.

A created_after or created_before before 4714-11-24 BC is valid RFC 3339, and a hand-built cursor can carry any instant, but Postgres's timestamptz cannot hold them, so the statement failed and the list answered 500. The parameters and the cursor are now checked against that range when parsed. The malformed-parameter test labels its cases by position, so a failure prints no cursor or body.

The comment moved with the list handler still said the projection reads erased the SQLSTATE and that the arm flattened every error to a 500. Both reads go through db_contention_error, and controller_read_error answers contention as a retryable 503, so the comment and its pointer on get_sandbox now say that.

lifecycle=, template=, user_id=, created_after= and created_before= answered 400 on the unpaged list, which ignored those names before they were filters. Like node_id, assigned and prefix, an empty value is now unset, paged or not; an empty item in a lifecycle list is still refused. org_id, which the unpaged list already parsed, keeps refusing an empty value. The OpenAPI description says so once for the endpoint.

prefix searches sandbox rows only, like every filter, so a pool slot or observed-local sandbox never matches, even by its own ID. The field's doc comment and the OpenAPI parameter now say so, and the route test pins it.

The count could drop the stored deleted rows no node reports before its FULL JOIN. Postgres already applies the deleted filter at the join, below the inventory lookup, so each such row costs a hash probe. On 1.5 million synthetic rows (1 million tombstones, 102,500 observed) an EXISTS prefilter measured 1,180 ms against 1,058 ms, and a hashed IN, 980 ms, becomes a per-row subplan once the observed rows outgrow hash_mem. The doc comment records that so the statement keeps its shape.

…he clock `reported_on_node` was NULL, not FALSE, for a sandbox with no Direct route and no `active_node_id`: both conjuncts compared NULL with the node. The list's `$10 IS NULL OR {on_node}` dropped such a row either way, but a negation would drop it too. The exact test is now `IS NOT DISTINCT FROM`, so the predicate is never NULL, and the node-filter test checks that `NOT` of it keeps the unassigned sandboxes. `an_empty_filter_parameter_is_unset` compared whole bodies from two requests, and each body's `age` counts whole seconds on the wall clock, so a second ticking between them failed it. It now compares the listed IDs and `next_cursor`, which also keeps the bodies out of its failure message.

ac-pplx · 2026-11-03T18:00:58Z

Thanks. Both findings are addressed; the head is now 905e6a49, rebased onto main after #1321 merged.

(Medium) The assigned read. route_overrides.rs is gone. Each request is one statement, with no route read, no transaction and no second round trip.
- controllers/substrate/direct/reported_node.rs exports two whole predicates, and the admin statements interpolate them, so Direct's tables stay behind Direct's module.
- reported_on_node narrows through idx_sandboxes_active_node and the route primary key, then tests the response's own COALESCE(route node, row node).
- reported_on_a_node is the row's node or an EXISTS on the route.
- The unpaged statement tests assigned on the route it already joins, as you suggested.
- The module doc now says the route table keeps the routes of written-off, reverted and rolled-back sandboxes, naming the three writers, and the body no longer says those are few.
(Low) Empty values. An empty node_id= or assigned= is unset again, and so is every other empty filter except org_id=, which the unpaged list already parsed.

The PR also takes #1321's lows:

the inverted sub-microsecond range;
out-of-range timestamps and cursors answering 400;
the contention comments;
the prefix scope in the docs.

A test checks that the page and unpaged statements return the same rows for each filter and combination.

Could you take another look?

the-gigi-pplx · 2026-11-03T19:24:42Z

Re-review — 5 findings, none blocking

The head is now 905e6a492b. Here's where each earlier point stands:

Earlier point	Status at this head
The `assigned` route read grows with every failed or written-off Direct sandbox (this PR)	Fixed. The node filters are now predicates evaluated in the statement (`controllers/substrate/direct/reported_node.rs`, used at `list_page/admin.rs:194-212` and `:239-254`). There's no route read, no ID arrays and no transaction.
An empty `node_id=` or `assigned=` answered 400 on the unpaged path (this PR)	Fixed (`routes/admin/sandbox_list.rs:233-245`, `empty_as_unset`).
Created range checked after rounding (#1321)	Fixed. The bounds are compared as given (`sandbox_list.rs:172-180`).
Out-of-range timestamps answered 500 (#1321)	Fixed for both the filters and the cursor (`sandbox_list.rs:51-57`, `:257-266`, `:302`).
The moved contention comment contradicted the code (#1321)	Fixed (`sandbox_list.rs:377-381`).
`prefix` leaves out the node-reported rows (#1321)	Documented, in the field doc and the OpenAPI text (`docs.rs:805`).
Empty `lifecycle=` / `template=` answered 400 (#1321)	Fixed (`empty_as_unset`).
Duplicated filter `WHERE` and the count's tombstone cost (#1321)	The tombstone cost is now documented on the count. The filter `WHERE` is still written out in both statements, and now carries the node predicate twice too (finding 3).

I also checked two things in the new code:

reported_on_node against the response's own expression, COALESCE(route node, row node). The narrowing conjunct is complete, since every sandbox reported on N has either its route or, without one, its row on N. The exact conjunct decides.
That the switch to IS NOT DISTINCT FROM doesn't change which rows match. It only turns a NULL into FALSE, and the predicate is only used positively, inside ($10 IS NULL OR …).

Both POSTGRES_TIMESTAMP_NANOS bounds equal Postgres's MIN_TIMESTAMP and END_TIMESTAMP converted to the Unix epoch.

Low

crates/libs/api/src/routes/admin/sandbox_list/tests.rs:342 — the assigned case of an_empty_filter_parameter_is_unset can't fail.
Every seeded sandbox has no node, so assigned= misparsed as false would list the same IDs as no filter. Seeding one assigned sandbox would let the comparison see the difference.
crates/libs/api/src/controllers/sandbox/list_page/admin/tests.rs:533-556 — the reworked combined cases dropped created_after, which the old combined case set. No case combines it with the other filters any more. That's the case that catches a placeholder shift like this PR's $10→$12 renumbering.
list_page/admin.rs:239-254 — "reported on a node" is now written two ways. The paged statement uses reported_on_a_node (an EXISTS on the route), and the unpaged one uses COALESCE(direct_route.current_node_id, s.active_node_id) IS NOT NULL. Both copy SANDBOX_COLUMNS's expression.
In the unpaged statement, reported_on_node's exact conjunct also looks the route up again, although {sandbox_from} has already joined it. If the response's active_node_id ever changes, these copies have to change together, and only node_id has a test that compares against the response itself.
controllers/substrate/direct/reported_node.rs:20-24 — the doc says the planner reaches a node's few sandboxes through idx_sandboxes_active_node and the primary key, but nothing pins that plan.
Every filter combination shares one prepared statement, guarded by ($10::UUID IS NULL OR …). Once Postgres switches that statement to a generic plan, a node_id request may walk the list order instead. I haven't measured whether it does. An EXPLAIN test under plan_cache_mode = force_generic_plan, as sandbox_quota/plan_tests.rs does, would settle it, or the comment could claim less.
sandbox_list.rs:75-78 — on the unpaged path, a non-empty but malformed node_id or assigned, such as a node name instead of a UUID, or assigned=1, still answers 400 where the list used to ignore those names. Empty values are now covered, so this is the remaining edge.

Verified against 905e6a492b4777abd53dd5dcffd2a02d976c16b3. I ran the admin list, ID parser and node-filter tests at this head on Postgres (cargo nextest run -p pplx-space-api --features kubernetes, filtered to sandbox_list::, list_page::admin::, reported_node and sandbox_id::): 17 run, 17 passed. I checked the timestamp bounds against Postgres's definitions by hand and traced the predicates by reading. Finding 4 is unmeasured.

github-actions Bot added the triage:pending Waiting for a maintainer to triage label Nov 3, 2026

ac-pplx marked this pull request as ready for review November 3, 2026 06:43

kumarpallav-pplx requested review from shehan-pplx and the-gigi-pplx November 3, 2026 06:43

the-gigi-pplx approved these changes Nov 3, 2026

View reviewed changes

ac-pplx mentioned this pull request Nov 3, 2026

api: page and filter the admin sandbox list, count spaced versions, and share one sandbox ID parser #1321

Merged

Base automatically changed from ac/admin-api-sandbox-list to main November 3, 2026 15:22

ac-pplx marked this pull request as draft November 3, 2026 15:22

ac-pplx changed the title ~~api: filter the admin sandbox list by node and assignment through the Direct route's owning read~~ api: filter the admin sandbox list by node and assignment, and tighten its parameter checks Nov 3, 2026

ac-pplx added 11 commits November 3, 2026 11:18

api: count an empty node_id or assigned as unset on the admin list

821ecb8

Before these were filters the unpaged list ignored the names, so a caller sending node_id= or assigned= now got a 400. An empty value is unset, paged or not.

ac-pplx force-pushed the ac/admin-api-sandbox-list-node-filters branch from 78818d0 to 14a4dbc Compare November 3, 2026 16:18

ac-pplx marked this pull request as ready for review November 3, 2026 17:33

ac-pplx requested a review from the-gigi-pplx November 3, 2026 18:00

ac-pplx added this pull request to the merge queue Nov 3, 2026

Merged via the queue into main with commit 7a72df0 Nov 3, 2026
28 of 43 checks passed

ac-pplx deleted the ac/admin-api-sandbox-list-node-filters branch November 3, 2026 19:50

This was referenced Nov 3, 2026

api: follow up the admin sandbox list's node filters #1405

Merged

[cr-test] api: follow up the admin sandbox list's node filters #1441

Closed

[cr-test] api: follow up the admin sandbox list's node filters #1456

Closed

Navigation Menu

Repository navigation

api: filter the admin sandbox list by node and assignment, and tighten its parameter checks - #1385

api: filter the admin sandbox list by node and assignment, and tighten its parameter checks#1385
ac-pplx merged 12 commits into
mainfrom
ac/admin-api-sandbox-list-node-filters

ac-pplx commented Nov 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Nov 3, 2026

Uh oh!

the-gigi-pplx commented Nov 3, 2026

Uh oh!

the-gigi-pplx left a comment

Uh oh!

ac-pplx commented Nov 3, 2026

Uh oh!

the-gigi-pplx commented Nov 3, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ac-pplx commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

Severity and risk

Rollout

Not covered

Uh oh!

github-actions Bot commented Nov 3, 2026

Uh oh!

the-gigi-pplx commented Nov 3, 2026

Review — 2 findings

Medium

Low

Uh oh!

the-gigi-pplx left a comment

Choose a reason for hiding this comment

Uh oh!

ac-pplx commented Nov 3, 2026

Uh oh!

the-gigi-pplx commented Nov 3, 2026

Re-review — 5 findings, none blocking

Low

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ac-pplx commented Nov 3, 2026 •

edited

Loading