Repository navigation
Conversation
`dispatch_crdt_apply_admitted_outcome` compared the plan's collection --
`QualifiedCollection::new(database_id, collection)`, so `{database_id}/{name}`
on any database but the default -- against the bare name its caller typed, and
`CrdtAdmissionInvalidPlan` came back as XX000 before any policy was consulted.
A `CRDT MERGE` in a non-default database therefore never reached the RLS
decision its own policy store already carried; in `default` the two forms
coincide, which is why nothing surfaced.
Reduce the request to the canonical form before comparing, and hand the same
string to the preview that fences the apply: the CRDT engine is keyed by that
name, so a preview built from the bare name read a different (empty) document
than the apply it was fencing.
Two places need the bare form and keep getting it: the catalog lookup
(`get_collection` qualifies internally) and the vShard route. Routing stays on
the form each caller passed, because every entry point derives its task vShard
from that same string -- re-deriving it here would move work between cores on a
path this change is not about.
Callers disagree on the form by design: the SQL, HTTP, and sync entry points
pass the name the client typed, while the native raw dispatch passes the
stored, already-qualified one. De-qualification goes through
`target_identity::bare_collection_name`, which strips the prefix only when it is
really there, so an already-bare name and a collection whose own name contains
`/` both survive untouched.
…rors Two surfaces still flattened a classified failure while the routed pgwire paths already carried its class. - CRDT MERGE: the admission and apply failures now map through `error_to_sqlstate`, so a policy denial — `ExternalCrdtPostImagePolicy::deny` returns `RejectedAuthz` — reaches the client as `42501` (INSUFFICIENT_PRIVILEGE) instead of `XX000`. The "authorization returned no capability" site keeps `XX000`: that one is an internal invariant by decision, not a class a client can act on. - HTTP stream: the in-band error lines carry the numeric NodeDB code (shape and malformed-batch failures) and the status the gateway map already computed. Consumer trace for the class change: the two wire assertions that pinned the placeholder move with it — `nodedb/tests/wire/cases/crdt_write_rls_database_scope.rs` asserted `XX000` and now asserts `42501`, and the comment above the first one no longer describes the old wrapper. No other test, doc, or path keys on the CRDT merge SQLSTATE (`grep XX000` over `tests/wire/cases` finds no other crdt or merge site). The pgwire stream and DDL-dispatch sites are not touched here; the HTTP shaping surface above is the whole second half. Also refreshes the `response_shape/schema.rs` module comment, which still claimed nothing consumes the module — the session caches the schema with the physical tasks, and shaping receives it as `projection`. Verification: `cargo nextest run -p nodedb --test wire --all-features --cargo-profile ci --profile ci -E 'test(~crdt_write_rls_database_scope)'` fails on the pre-change tree (the denial surfaces as `XX000`) and passes with this change (`42501`).
…rrors The catalog gate reads in `crdt_gate`, `update_delete::shared` and `implicit_edges::catalog` still passed the collection the caller routed on, which is database-qualified, while the catalog keys collections by the bare name. In a non-default database every one of them missed, and each miss silently answered "no": a CRDT collection read as plain, so a predicate UPDATE/DELETE and an `ON CONFLICT DO UPDATE` skipped the refusal that keeps CRDT convergence authoritative; an edge-bearing collection read as plain, so a primary-key-equality UPDATE/DELETE skipped the mirrored-edge cleanup; and the edge-bearing marker was never set, so that cleanup could not find the collection at all. Reduce all three through `target_identity::bare_collection_name`, as the sibling gate reads already do; it strips the prefix only when it is really there and is identity for `DatabaseId::DEFAULT`. `crdt_state` and `crdt_apply` flattened every classified failure to `XX000` at three sites. Map them through `error_to_sqlstate`, so the policy denial an `ExternalCrdtPostImagePolicy::deny` produces reaches the client as `42501`, the code `CRDT MERGE` already reports, instead of a class a client cannot act on. Covered by four non-default-database variants mirroring the existing default-database gate tests (CRDT predicate UPDATE, CRDT predicate DELETE, CRDT upsert, and the reserved-edge-field expression UPDATE) and a wire assertion that `crdt_apply` reports `42501` for a post-image its policy forbids.
`plan_needs_implicit_edge_recon` fed the database-qualified collection from the plan straight to a catalog keyed by the bare name, so in every non-default database the read missed, `has_implicit_edges` read false, this gate returned `None`, and the OLLP/Calvin dependent-edge reconnaissance never routed. The mirrored edges of a PK-equality UPDATE/DELETE were therefore never cleaned up — and the plan lowers exactly those writes to `Bulk*` so this gate picks them up, so the lowering had no effect outside the default database. This is the same defect the sibling planner gates had; the bare name is what the catalog stores. The returned collection still comes from the plan, because that is the routing key, and both call sites take only the `database_id`. The restore path's comment claimed its bare collection form was intended. It is not: the registry qualifies it before the engine is keyed, so restore still addresses a different document than an ordinary apply in a non-default database. That is pre-existing and left alone, but the comment now says so instead of asserting the opposite.
The fix had no test: `plan_needs_implicit_edge_recon` had zero call sites anywhere in the suite, and the existing non-default-database graph test sends an expression update that the planner rejects at plan time, so it never reaches this gate. Reverting the fix left every test green. Two tests now drive the gate directly: seed a catalog with an edge-bearing collection in a non-default database, hand the gate a `BulkUpdate` carrying the database-qualified collection the planner builds, and assert it fires and returns that qualified key. The second covers `DatabaseId::DEFAULT` as the identity case. Reverting the bare-name lookup fails the first and leaves the second passing, which is the red arm this needed. The restore-path comment also claimed the collection registry qualifies its bare string before the engine is keyed. It does not: the string reaches `from_stored` and the tenant engine's collection map verbatim, which is what makes restore disagree with an ordinary apply. The comment now says that.
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.
Problem
Two routing and classification defects in non-default databases, plus the HTTP stream's error lines.
The catalog reads were qualified where the catalog is keyed bare. A plan routes on the database-qualified collection name, but every catalog read underneath it is keyed by the bare name. Two reads still passed the qualified form:
planner/implicit_edges/catalog.rs— an edge-bearing collection in a non-default database missed the read, sohas_implicit_edgeswas never set and the implicit-edge UPDATE/DELETE cleanup was silently skipped, leaking stale mirrored edges.planner/calvin/dependent_recon.rs— the same miss on the Calvin side, so the OLLP recon that cleans up mirrored edges never routed for a PK-equality UPDATE/DELETE.Both now reduce the routed name through the existing
target_identity::bare_collection_name, which strips the prefix only when it is really there and is identity forDatabaseId::DEFAULT. The default-database path is byte-for-byte unchanged.The HTTP stream's in-band error lines carried no code. A client consuming the NDJSON stream got
{"error": "..."}and nothing else, so it could not classify a failure. The malformed-batch and shaping paths now carry the numeric NodeDB code fromerror_classify::classify, and the mid-stream dispatch-error path carries the HTTP status the gateway map already computed.CRDT admission keys the engine on the canonical collection. Admission and the plan have to address the same document;
engine_key()reduces the request throughQualifiedCollection::newso the preview that fences a write and the plan it fences are keyed identically.What this does not change
The
CRDT MERGEandCRDTop handlers already classify their refusals onmain. They reach42501for a policy denial throughDdlError::from_error, which carries the typed class, rather than through a localerror_to_sqlstatecall. This branch originally replaced those sites; after the rebase ontoaa6e91bbdthey are left asmainhas them, and the classification is not revisited here.Evidence
The new tests fail on
mainwithout this change.crdt_write_rls_database_scope,engine_surface_crdt_document,engine_surface_graph— 29/29 pass on this branch.predicate_update_on_crdt_rejected_in_non_default_database,predicate_delete_on_crdt_rejected_in_non_default_database,upsert_on_conflict_do_update_on_crdt_rejected_in_non_default_database,edge_field_expression_update_rejected_in_non_default_database, andcrdt_apply_in_default_database_reports_rls_denial_as_42501for the default path.cargo check -p nodedbexits 0.Rebase note
Rebased from a base 71 commits behind onto
aa6e91bbd. Eight files conflicted:crdt_merge.rs,crdt_ops.rs,crdt_admission.rs,balanced_gate.rs,crdt_gate.rs,update_delete/shared.rs,insert/identity.rs, andcrdt_write_rls_database_scope.rs.Every one resolved to the
mainside, becausemainreached the same place by a newer route:DdlError::from_errorfor the class,CollectionKey::from_qualified_strfor the admission key, andtarget_identity::naming::bare_collection_namefor the catalog reads. The two files that carried a defectmainhad not fixed —implicit_edges/catalog.rsanddependent_recon.rs— were not in conflict and keep their change.The diff against the new base is 8 files, +386/−14. The original head lived in the upstream repository, where this account has no push access, so this pull request replaces #368.