diff --git a/changelog.d/cli-change-pages-at-the-maximum.performance.md b/changelog.d/cli-change-pages-at-the-maximum.performance.md new file mode 100644 index 000000000..a7393befa --- /dev/null +++ b/changelog.d/cli-change-pages-at-the-maximum.performance.md @@ -0,0 +1,10 @@ +- **Fewer, larger change pages from the CLI.** When `commit changes`, + `changes poll`, `diff` or `branch diff` follows every page itself, it now + asks for 8,192 changes per page instead of the 1,000 the HTTP routes + default to. Every page restarts its comparison after the last change the + previous page returned, so fewer, larger pages cost fewer requests and + less time; the output is unchanged. `--limit` still sets the size, and a + single `--page-token` fetch still defaults to 1,000. See + [changes and change feeds][cli-change-pages]. + +[cli-change-pages]: ../docs/user/branching/changes.md#inspect-one-commit diff --git a/crates/omnigraph-cli/src/cli.rs b/crates/omnigraph-cli/src/cli.rs index dc55d7928..bc6496c8a 100644 --- a/crates/omnigraph-cli/src/cli.rs +++ b/crates/omnigraph-cli/src/cli.rs @@ -267,7 +267,8 @@ pub(crate) enum Command { /// Read target the diff ends at #[arg(long)] to: String, - /// Changes per page (the server clamps at its public ceiling) + /// Changes per page, at most 8192: 8192 when the CLI follows every + /// page, 1000 for one `--page-token` fetch #[arg(long)] limit: Option, /// Fetch exactly one page starting at this opaque token @@ -977,7 +978,8 @@ pub(crate) enum BranchCommand { /// Branch the source is compared against (default main) #[arg(long)] into: Option, - /// Changes per page (the server clamps at its public ceiling) + /// Changes per page, at most 8192: 8192 when the CLI follows every + /// page, 1000 for one `--page-token` fetch #[arg(long)] limit: Option, /// Fetch exactly one page starting at this opaque token @@ -1113,7 +1115,8 @@ pub(crate) enum CommitCommand { #[arg(long)] uri: Option, commit_id: String, - /// Changes per page (the server clamps at its public ceiling) + /// Changes per page, at most 8192: 8192 when the CLI follows every + /// page, 1000 for one `--page-token` fetch #[arg(long)] limit: Option, /// Fetch exactly one page starting at this opaque token @@ -1187,7 +1190,8 @@ pub(crate) enum ChangesCommand { /// Start mode: now | beginning | after: (default now) #[arg(long, conflicts_with = "cursor")] start: Option, - /// Changes per page (the server clamps at its public ceiling) + /// Changes per page, at most 8192 (default 8192; the CLI follows + /// every page) #[arg(long)] limit: Option, /// Filter by entity kind (repeatable): node | edge diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 88631cf66..2c06fa005 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -1074,7 +1074,7 @@ async fn run(cli: Cli) -> Result<()> { .commit_changes_page( &commit_id, next_page_token.as_deref(), - limit, + auto_page_limit(limit), &filter, &settings, ) @@ -1094,7 +1094,7 @@ async fn run(cli: Cli) -> Result<()> { .commit_changes_page( &commit_id, next_page_token.as_deref(), - limit, + auto_page_limit(limit), &filter, &settings, ) @@ -1150,7 +1150,7 @@ async fn run(cli: Cli) -> Result<()> { cursor.as_deref(), start.as_deref(), next_page_token.as_deref(), - limit, + auto_page_limit(limit), &filter, &settings, ) @@ -1172,7 +1172,7 @@ async fn run(cli: Cli) -> Result<()> { cursor.as_deref(), start.as_deref(), next_page_token.as_deref(), - limit, + auto_page_limit(limit), &filter, &settings, ) @@ -2610,6 +2610,14 @@ fn report_deployment_lock(status: &omnigraph_cluster::DeploymentStatus) { } } +/// The page size the CLI asks for when it follows every page itself: the +/// public maximum unless `--limit` sets one. Every page resolves its commits +/// again and restarts its ordered walk after the previous page's last change, +/// so the same changes cost less in fewer, larger pages. +fn auto_page_limit(limit: Option) -> Option { + Some(limit.unwrap_or(omnigraph::changes::COMMIT_CHANGES_MAX_CHANGES)) +} + /// Print a graph diff: one page when `page_token` is given (the raw escape /// hatch), otherwise every page in turn, each emitted before the next is /// fetched. @@ -2648,7 +2656,12 @@ async fn run_diff( let mut output = DiffJsonStream::new(io::BufWriter::new(io::stdout())); loop { let page = client - .diff_page(sides, next_page_token.as_deref(), limit, &filter) + .diff_page( + sides, + next_page_token.as_deref(), + auto_page_limit(limit), + &filter, + ) .await?; next_page_token = page.next_page_token.clone(); output.write_page(&page)?; @@ -2661,7 +2674,12 @@ async fn run_diff( let mut output = DiffHumanStream::new(); loop { let page = client - .diff_page(sides, next_page_token.as_deref(), limit, &filter) + .diff_page( + sides, + next_page_token.as_deref(), + auto_page_limit(limit), + &filter, + ) .await?; next_page_token = page.next_page_token.clone(); output.write_page(&page)?; diff --git a/crates/omnigraph-server/src/handlers.rs b/crates/omnigraph-server/src/handlers.rs index f4375956f..d2c368799 100644 --- a/crates/omnigraph-server/src/handlers.rs +++ b/crates/omnigraph-server/src/handlers.rs @@ -4086,38 +4086,6 @@ fn required_diff_param(value: Option, name: &str) -> std::result::Result } } -/// Authorize `read` on the branch one graph diff side resolves to: a branch -/// name, or the authored branch (a proposal's or snapshot's recorded target) -/// of a commit. The side is resolved before the policy check, so a denial is -/// reported exactly as a side that names nothing: the route is not an -/// existence oracle for branches or commits the actor cannot read. -async fn authorize_diff_side( - db: &Omnigraph, - actor: Option<&AuthenticatedActor>, - policy: Option<&PolicyEngine>, - side: &str, -) -> std::result::Result<(), ApiError> { - let not_found = || ApiError::not_found(format!("'{side}' names no branch and no commit")); - let branch = match db.diff_read_target_branch(side).await { - Ok(branch) => branch, - // A branch deleted between resolution steps is the same fixed 404. - Err(OmniError::BranchNotFound { .. }) => return Err(not_found()), - Err(error) => return Err(change_route_error(error)), - }; - match authorize( - actor, - policy, - PolicyRequest { - action: PolicyAction::Read, - branch: Some(branch), - target_branch: None, - }, - )? { - Authz::Allowed => Ok(()), - Authz::Denied(_) => Err(not_found()), - } -} - #[utoipa::path( get, path = "/diff", @@ -4169,9 +4137,13 @@ pub(crate) async fn server_diff( } /// One page of the net change between two read targets, for `GET /diff` and -/// the `branch diff to ` statement: each side is authorized on the -/// branch it resolves to, and a side the actor cannot read answers the 404 a -/// side naming nothing does. +/// the `branch diff to ` statement. The engine authorizes each side +/// on the branch it resolves to (a branch name, or the authored branch, a +/// proposal's or snapshot's recorded target, of a commit) as soon as it +/// classifies that side, before it looks at the next side or opens a +/// snapshot. A side the actor cannot read is the 404 of a side that names +/// nothing, whichever side it is: the route is not an existence oracle for +/// branches or commits the actor cannot read. pub(crate) async fn two_point_diff_page( handle: &GraphRequest, actor: Option<&AuthenticatedActor>, @@ -4181,19 +4153,42 @@ pub(crate) async fn two_point_diff_page( page_token: Option, max_changes: Option, ) -> std::result::Result { - let db = &handle.engine; - for side in [&from, &to] { - authorize_diff_side(db, actor, handle.policy.as_deref(), side).await?; + let policy = handle.policy.as_deref(); + // A policy failure (a missing bearer, an evaluation error) is not a + // denial: it stops the read, and its own error answers. + let mut failure = None; + let mut can_read = |branch: &str| { + let request = PolicyRequest { + action: PolicyAction::Read, + branch: Some(branch.to_string()), + target_branch: None, + }; + match authorize(actor, policy, request) { + Ok(Authz::Allowed) => true, + Ok(Authz::Denied(_)) => false, + Err(error) => { + failure.get_or_insert(error); + false + } + } + }; + let page = handle + .engine + .diff_page_readable( + &omnigraph::changes::GraphDiffRequest { + sides: omnigraph::changes::GraphDiffSides::Between { from, to }, + scope, + page_token, + max_changes, + max_bytes: None, + }, + &mut can_read, + ) + .await; + if let Some(error) = failure { + return Err(error); } - db.diff_page(&omnigraph::changes::GraphDiffRequest { - sides: omnigraph::changes::GraphDiffSides::Between { from, to }, - scope, - page_token, - max_changes, - max_bytes: None, - }) - .await - .map_err(change_route_error) + page.map_err(change_route_error) } /// Authorize `read` on each named branch of a branch diff or merge base. diff --git a/crates/omnigraph-server/tests/integration/auth_policy.rs b/crates/omnigraph-server/tests/integration/auth_policy.rs index 3d434d972..3c13c682b 100644 --- a/crates/omnigraph-server/tests/integration/auth_policy.rs +++ b/crates/omnigraph-server/tests/integration/auth_policy.rs @@ -1633,6 +1633,21 @@ async fn policy_commit_diff_forbidden_is_indistinguishable_from_unknown() { .await .unwrap(); let feature_commit = receipt.commit.graph_commit_id.clone(); + // A commit whose branch is deleted afterwards. + db.branch_create_from(ReadTarget::branch("main"), "gone") + .await + .unwrap(); + let gone_commit = db + .load_with_receipt( + "gone", + r#"{"type":"Person","data":{"name":"gone-only","age":41}}"#, + omnigraph::loader::LoadMode::Merge, + ) + .await + .unwrap() + .commit + .graph_commit_id; + db.branch_delete("gone").await.unwrap(); drop(db); let policy_path = temp.path().join("policy.yaml"); @@ -1687,9 +1702,9 @@ async fn policy_commit_diff_forbidden_is_indistinguishable_from_unknown() { "forbidden and unknown must share the same error code" ); - // A graph diff resolves each side before it authorizes the branch the side - // resolves to, so a forbidden side, commit or branch, answers exactly as a - // side that names nothing. + // A graph diff classifies each side before it authorizes the branch the + // side resolves to, so a forbidden side, commit or branch, answers exactly + // as a side that names nothing. let get = |uri: String| { Request::builder() .header(HTTP_API_CONTRACT_HEADER, HTTP_API_CONTRACT) @@ -1734,6 +1749,29 @@ async fn policy_commit_diff_forbidden_is_indistinguishable_from_unknown() { ); assert_eq!(forbidden["code"], unknown["code"]); } + // The sides are authorized in order, each before the next is looked at: a + // forbidden `from` answers as an unknown `from` does whatever `to` names, + // so an unknown `to` cannot reveal that `from` exists. A commit whose + // branch was deleted is no longer readable and answers the same way. + let unknown_from_id = "01ARZ3NDEKTSV4RRFFQ69G5FAW"; + for (forbidden_from, unknown_from, to) in [ + ("feature", "no-such-branch-2", "no-such-branch"), + (feature_commit.as_str(), unknown_from_id, unknown_id), + (gone_commit.as_str(), unknown_from_id, "main"), + ] { + for from in [forbidden_from, unknown_from] { + let expected = format!("'{from}' names no branch and no commit"); + let (status, body) = + json_response(&app, get(format!("/diff?from={from}&to={to}"))).await; + assert_eq!(status, StatusCode::NOT_FOUND, "{body}"); + assert_eq!(body["error"], expected, "/diff from {from} to {to}"); + let statement = format!(r#"branch diff "{from}" to "{to}""#); + let (status, body) = + json_response(&app, statement_request("/query", "team-token", &statement)).await; + assert_eq!(status, StatusCode::NOT_FOUND, "{body}"); + assert_eq!(body["error"], expected, "{statement}"); + } + } // The branch routes name their branches outright: a read denial is a 403, // and a dry run is a read of both branches. for uri in [ diff --git a/crates/omnigraph/src/db/mod.rs b/crates/omnigraph/src/db/mod.rs index f4235c14a..4764178c9 100644 --- a/crates/omnigraph/src/db/mod.rs +++ b/crates/omnigraph/src/db/mod.rs @@ -20,7 +20,7 @@ pub use omnigraph::{ CollectorRowSummary, DatasetCleanupStats, DatasetOptimizeStats, DatasetRepairStats, EXPORT_CHUNK_MAX_BYTES, ExportCut, FullTextIndexRebuildResult, GraphCreateReconciliation, InitOptions, MergeOutcome, MergeResult, Omnigraph, OpenMode, PendingIndex, PreparedGraphCreate, - PreparedSchemaApply, PreparedSchemaSettlement, RebuiltFullTextIndex, RepairAction, + PreparedSchemaApply, PreparedSchemaSettlement, ReadCheck, RebuiltFullTextIndex, RepairAction, RepairClassification, RepairOptions, RepairStats, RetainedManifestVersions, SYSTEM_COLUMNS_PREFLIGHT, SchemaApplyReconciliation, SchemaApplyResult, SchemaApplySettlement, SchemaContractDigest, SchemaNonPublicationProof, SkipReason, StagingVerdict, diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index 208d4fbce..65c5b8e93 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -2721,6 +2721,37 @@ impl Omnigraph { &self, request: &crate::changes::GraphDiffRequest, ) -> Result { + let prepared = self.prepare_diff(request, None).await?; + self.run_diff_page(prepared).await + } + + /// [`Self::diff_page`] for a caller that may read only some branches. + /// `can_read` answers for the branch each two-point side reads: a branch + /// itself, or a commit's authored branch (an unpublished proposal's or + /// unattached snapshot's recorded target). Each side is authorized as + /// soon as it is classified, before the next side is looked at and before + /// its snapshot opens, and a side `can_read` refuses is + /// [`OmniError::ReadTargetNotFound`] naming it, exactly as a side that + /// names nothing: the answer is not an existence oracle. A continuation + /// classifies and authorizes its sides again before it reopens the + /// token's commits. The branch form names its branches outright, so its + /// caller authorizes them before the call. + pub async fn diff_page_readable( + &self, + request: &crate::changes::GraphDiffRequest, + can_read: &mut ReadCheck<'_>, + ) -> Result { + let prepared = self.prepare_diff(request, Some(can_read)).await?; + self.run_diff_page(prepared).await + } + + /// Resolve a graph diff page request, reading lineage only: its + /// endpoints and its continuation position. + async fn prepare_diff( + &self, + request: &crate::changes::GraphDiffRequest, + can_read: Option<&mut ReadCheck<'_>>, + ) -> Result { use crate::changes::token; let max_changes = request @@ -2735,7 +2766,11 @@ impl Omnigraph { let filter_digest = token::filter_digest(&request.scope); let sides_digest = token::sides_digest(&request.sides); let (endpoints, resume) = match request.page_token.as_deref() { - None => (self.resolve_diff_endpoints(&request.sides).await?, None), + None => ( + self.resolve_diff_endpoints(&request.sides, can_read) + .await?, + None, + ), Some(encoded) => { let decoded = token::decode_diff_page_token(encoded)?; if decoded.graph_identity != token::hashed_identity(&graph_identity) @@ -2750,6 +2785,14 @@ impl Omnigraph { "diff page token was minted for a different filter scope", )); } + if let Some(can_read) = can_read + && let crate::changes::GraphDiffSides::Between { from, to } = &request.sides + { + for side in [from, to] { + self.authorized_diff_side(side, Some(&mut *can_read)) + .await?; + } + } let endpoints = self .reopen_diff_endpoints( &decoded.from_commit_id, @@ -2766,7 +2809,35 @@ impl Omnigraph { (endpoints, Some(resume)) } }; + Ok(PreparedDiffPage { + scope: request.scope.clone(), + max_changes, + max_bytes, + graph_identity, + filter_digest, + sides_digest, + endpoints, + resume, + }) + } + /// Read one prepared graph diff page. + async fn run_diff_page( + &self, + prepared: PreparedDiffPage, + ) -> Result { + use crate::changes::token; + + let PreparedDiffPage { + scope, + max_changes, + max_bytes, + graph_identity, + filter_digest, + sides_digest, + endpoints, + resume, + } = prepared; // Reclaimed history under either endpoint is a typed gap naming the // diff's `to` commit; a diff has no feed cursor to resume. let to_commit_id = endpoints.to_commit_id.clone(); @@ -2785,7 +2856,7 @@ impl Omnigraph { &endpoints.to_snapshot, &graph_identity, &endpoints.to_commit_id, - &request.scope, + &scope, resume.as_ref(), &mut budget, &mut changes, @@ -2890,14 +2961,21 @@ impl Omnigraph { } /// Resolve a graph diff's sides to exact commits and their snapshots. + /// The two-point form takes its sides in order, each authorized by + /// `can_read` before the next is looked at; the branch form's caller + /// authorizes the two branches it names. async fn resolve_diff_endpoints( &self, sides: &crate::changes::GraphDiffSides, + mut can_read: Option<&mut ReadCheck<'_>>, ) -> Result { match sides { crate::changes::GraphDiffSides::Between { from, to } => { - let (from_commit_id, from_snapshot) = self.resolve_diff_read_target(from).await?; - let (to_commit_id, to_snapshot) = self.resolve_diff_read_target(to).await?; + let (from_commit_id, from_snapshot) = self + .resolve_diff_read_target(from, can_read.as_deref_mut()) + .await?; + let (to_commit_id, to_snapshot) = + self.resolve_diff_read_target(to, can_read).await?; Ok(DiffEndpoints { from_commit_id, to_commit_id, @@ -2918,7 +2996,8 @@ impl Omnigraph { } else if base_commit_id == target_head { target_snapshot } else { - self.commit_snapshot(&base_commit_id).await? + self.diff_endpoint_snapshot(&base_commit_id, &base_commit_id) + .await? }; Ok(DiffEndpoints { from_commit_id: base_commit_id.clone(), @@ -2942,8 +3021,12 @@ impl Omnigraph { from_commit_id: from_commit_id.to_string(), to_commit_id: to_commit_id.to_string(), base_commit_id, - from_snapshot: self.commit_snapshot(from_commit_id).await?, - to_snapshot: self.commit_snapshot(to_commit_id).await?, + from_snapshot: self + .diff_endpoint_snapshot(from_commit_id, from_commit_id) + .await?, + to_snapshot: self + .diff_endpoint_snapshot(to_commit_id, to_commit_id) + .await?, }) } @@ -2958,37 +3041,66 @@ impl Omnigraph { .snapshot) } - /// The branch a transport authorizes `read` on for one graph diff side: - /// the branch itself, or the authored branch of the commit the side names - /// (an unpublished proposal's or unattached snapshot's recorded target). - /// Refuses exactly as the diff would: ambiguity and absence are typed. - pub async fn diff_read_target_branch(&self, target: &str) -> Result { - Ok(match self.classify_diff_read_target(target).await? { - DiffReadTarget::Branch(resolved) => { - resolved.branch.unwrap_or_else(|| "main".to_string()) - } - DiffReadTarget::Commit(commit) => { - commit.graph_branch.unwrap_or_else(|| "main".to_string()) - } - }) + /// The graph as of one diff endpoint commit. A commit whose writing + /// branch is gone, or that a raced branch deletion hides mid-lookup, is + /// no longer readable: it is the not-found of the spelling that named it, + /// never the branch its record names, which the caller may not be allowed + /// to learn. + async fn diff_endpoint_snapshot(&self, commit_id: &str, spelling: &str) -> Result { + self.commit_snapshot(commit_id) + .await + .map_err(|error| match error { + OmniError::BranchNotFound { .. } => OmniError::ReadTargetNotFound { + target: spelling.to_string(), + }, + error => error, + }) } - /// Resolve one diff endpoint to its commit and the graph as of it. - async fn resolve_diff_read_target(&self, target: &str) -> Result<(String, Snapshot)> { - match self.classify_diff_read_target(target).await? { + /// Resolve one authorized diff endpoint to its commit and the graph as + /// of it. + async fn resolve_diff_read_target( + &self, + side: &str, + can_read: Option<&mut ReadCheck<'_>>, + ) -> Result<(String, Snapshot)> { + let classified = self.authorized_diff_side(side, can_read).await?; + match classified { DiffReadTarget::Branch(resolved) => { let commit_id = resolved.graph_commit_id.ok_or_else(|| { - OmniError::manifest(format!("branch '{target}' has no head commit")) + OmniError::manifest(format!("branch '{side}' has no head commit")) })?; Ok((commit_id, resolved.snapshot)) } DiffReadTarget::Commit(commit) => { - let snapshot = self.commit_snapshot(&commit.graph_commit_id).await?; + let snapshot = self + .diff_endpoint_snapshot(&commit.graph_commit_id, side) + .await?; Ok((commit.graph_commit_id, snapshot)) } } } + /// Classify one diff side, then, when the caller may read only some + /// branches, authorize the branch it reads before anything else is + /// resolved: a refused side is the not-found of a side that names + /// nothing. + async fn authorized_diff_side( + &self, + side: &str, + can_read: Option<&mut ReadCheck<'_>>, + ) -> Result { + let classified = self.classify_diff_read_target(side).await?; + if let Some(can_read) = can_read + && !can_read(&classified.read_branch()) + { + return Err(OmniError::ReadTargetNotFound { + target: side.to_string(), + }); + } + Ok(classified) + } + /// Classify one diff endpoint: a live branch (its head now) or a graph /// commit. A string naming both is refused, never resolved by precedence; /// one naming neither is a typed not-found carrying only the caller's @@ -3010,17 +3122,27 @@ impl Omnigraph { Err(error) => return Err(error), }, }; - let as_commit = match self - .coordinator - .read() - .await - .resolve_commit(&SnapshotId::new(target)) - .await - { - Ok(commit) => Some(commit), - Err(OmniError::Manifest(error)) if error.kind == ManifestErrorKind::NotFound => None, - Err(OmniError::BranchNotFound { .. }) => None, - Err(error) => return Err(error), + // Every graph commit id is minted canonical, a ULID or an `hb1` + // address, so any other spelling names no commit and needs no lookup. + // The lookup is not free: on a miss it checks out every live branch's + // `__manifest`, which a branch name like `main` would otherwise pay. + let as_commit = if !omnigraph_core::graph_commit_id::is_valid_graph_commit_id(target) { + None + } else { + match self + .coordinator + .read() + .await + .resolve_commit(&SnapshotId::new(target)) + .await + { + Ok(commit) => Some(commit), + Err(OmniError::Manifest(error)) if error.kind == ManifestErrorKind::NotFound => { + None + } + Err(OmniError::BranchNotFound { .. }) => None, + Err(error) => return Err(error), + } }; match (as_branch, as_commit) { (Some(_), Some(_)) => Err(OmniError::AmbiguousReadTarget { @@ -4466,12 +4588,42 @@ pub(crate) fn schema_for_table_key(catalog: &Catalog, table_key: &str) -> Result ))) } +/// A caller's answer to "may it read this branch?", asked once per two-point +/// graph diff side; see [`Omnigraph::diff_page_readable`]. +pub type ReadCheck<'a> = dyn FnMut(&str) -> bool + Send + 'a; + /// What one graph diff endpoint names. enum DiffReadTarget { Branch(Box), Commit(GraphCommit), } +impl DiffReadTarget { + /// The branch a caller authorizes `read` on: the branch itself, or the + /// authored branch of the commit (an unpublished proposal's or unattached + /// snapshot's recorded target). + fn read_branch(&self) -> String { + match self { + Self::Branch(resolved) => resolved.branch.clone(), + Self::Commit(commit) => commit.graph_branch.clone(), + } + .unwrap_or_else(|| "main".to_string()) + } +} + +/// A graph diff page request resolved to its endpoints and continuation +/// position. +struct PreparedDiffPage { + scope: crate::changes::ChangeFeedScope, + max_changes: usize, + max_bytes: u64, + graph_identity: String, + filter_digest: String, + sides_digest: String, + endpoints: DiffEndpoints, + resume: Option, +} + /// A graph diff's two endpoints, resolved to exact commits and their /// snapshots. struct DiffEndpoints { diff --git a/crates/omnigraph/tests/forbidden_apis.rs b/crates/omnigraph/tests/forbidden_apis.rs index 8e2eada1b..f2ef70357 100644 --- a/crates/omnigraph/tests/forbidden_apis.rs +++ b/crates/omnigraph/tests/forbidden_apis.rs @@ -313,7 +313,7 @@ const READ_ONLY_SURFACES: &[(&str, &str)] = &[ ("db/omnigraph.rs", "resolve_snapshot"), ("db/omnigraph.rs", "diff_page"), ("db/omnigraph.rs", "merge_base"), - ("db/omnigraph.rs", "diff_read_target_branch"), + ("db/omnigraph.rs", "diff_page_readable"), ("exec/merge.rs", "branch_merge_dry_run"), ("session.rs", "branch_read"), ("db/omnigraph.rs", "commit_changes_page"), diff --git a/crates/omnigraph/tests/integration/changes.rs b/crates/omnigraph/tests/integration/changes.rs index ad657870d..25a8b1647 100644 --- a/crates/omnigraph/tests/integration/changes.rs +++ b/crates/omnigraph/tests/integration/changes.rs @@ -4535,16 +4535,66 @@ async fn graph_diff_refuses_an_ambiguous_or_unknown_side() { OmniError::AmbiguousReadTarget { target } => assert_eq!(target, genesis), other => panic!("expected AmbiguousReadTarget, got {other:?}"), } - assert!(matches!( - db.diff_read_target_branch(&genesis).await.unwrap_err(), - OmniError::AmbiguousReadTarget { .. } - )); - match graph_diff_all(&db, between("main", "no-such-side"), None) + // A spelling no commit id has skips the commit lookup: an unknown name, + // and a malformed `hb1` address, are the same typed not-found. + for side in ["no-such-side", "hb1.not.a.commit"] { + match graph_diff_all(&db, between("main", side), None) + .await + .unwrap_err() + { + OmniError::ReadTargetNotFound { target } => assert_eq!(target, side), + other => panic!("expected ReadTargetNotFound for {side}, got {other:?}"), + } + } + + // A caller that may read only some branches is asked, side by side, for + // the branch each side reads (a branch itself, or a commit's authored + // branch) as soon as the side is classified. A refused side is the + // not-found of a side that names nothing, and the next side is never + // looked at, so an unknown `to` cannot reveal that `from` exists. + db.branch_create("feature").await.unwrap(); + let on_feature = write_on(&db, "feature", "insert_person", &person("Fay", 30)).await; + for from in ["feature", on_feature.graph_commit_id.as_str()] { + let mut asked = Vec::new(); + let refused = db + .diff_page_readable( + &diff_request(between(from, "no-such-side"), None), + &mut |branch: &str| { + asked.push(branch.to_string()); + branch == "main" + }, + ) + .await + .unwrap_err(); + match refused { + OmniError::ReadTargetNotFound { target } => assert_eq!(target, from), + other => panic!("expected ReadTargetNotFound for {from}, got {other:?}"), + } + assert_eq!(asked, ["feature"], "a refused side ends the resolution"); + } + let mut asked = Vec::new(); + db.diff_page_readable( + &diff_request(between("main", &on_feature.graph_commit_id), None), + &mut |branch: &str| { + asked.push(branch.to_string()); + true + }, + ) + .await + .unwrap(); + assert_eq!(asked, ["main", "feature"]); + + // A commit whose branch was deleted is no longer readable: it is that + // side's not-found, never the branch its record names. + db.branch_create("gone").await.unwrap(); + let on_gone = write_on(&db, "gone", "insert_person", &person("Gil", 31)).await; + db.branch_delete("gone").await.unwrap(); + match graph_diff_all(&db, between(&on_gone.graph_commit_id, "main"), None) .await .unwrap_err() { - OmniError::ReadTargetNotFound { target } => assert_eq!(target, "no-such-side"), + OmniError::ReadTargetNotFound { target } => assert_eq!(target, on_gone.graph_commit_id), other => panic!("expected ReadTargetNotFound, got {other:?}"), } } diff --git a/crates/omnigraph/tests/integration/changes_cost.rs b/crates/omnigraph/tests/integration/changes_cost.rs index 340d502c3..6f6a9dbe9 100644 --- a/crates/omnigraph/tests/integration/changes_cost.rs +++ b/crates/omnigraph/tests/integration/changes_cost.rs @@ -725,6 +725,73 @@ async fn change_feed_caught_up_poll_manifest_reads_are_flat_in_history() { .await; } +/// Resolving a graph diff's sides reads no `__manifest` per live branch. A +/// side spelled as a branch name cannot be a commit id, so it skips the +/// commit lookup, whose miss path checks out every live branch's +/// `__manifest`. Both sides here are branch names; the sweep adds idle +/// branches, which neither side reads. +#[tokio::test] +async fn graph_diff_side_resolution_is_flat_in_live_branches() { + use omnigraph::changes::{GraphDiffRequest, GraphDiffSides}; + + cost_harness(async { + let mut curve: Vec<(u64, IoCounts)> = Vec::new(); + for idle in [1u64, 9] { + let dir = tempfile::tempdir().unwrap(); + let db = Omnigraph::init( + dir.path().to_str().unwrap(), + "node Person {\n name: String @key\n age: I32?\n}\n", + ) + .await + .unwrap(); + let db = helpers::session(db); + db.load_with_receipt( + "main", + r#"{"type":"Person","data":{"name":"base","age":1}}"#, + LoadMode::Merge, + ) + .await + .unwrap(); + db.branch_create("feature").await.unwrap(); + db.load_with_receipt( + "feature", + r#"{"type":"Person","data":{"name":"feature","age":2}}"#, + LoadMode::Merge, + ) + .await + .unwrap(); + for i in 0..idle { + db.branch_create(&format!("idle{i}")).await.unwrap(); + } + + let (page, io) = measure(db.diff_page(&GraphDiffRequest { + sides: GraphDiffSides::Between { + from: "main".to_string(), + to: "feature".to_string(), + }, + scope: ChangeFeedScope::default(), + page_token: None, + max_changes: None, + max_bytes: None, + })) + .await; + assert_eq!(page.unwrap().changes.len(), 1); + eprintln!( + "DIFF idle_branches={idle}: manifest_reads={} data_open_count={}", + io.manifest_reads, io.data_open_count, + ); + curve.push((idle, io)); + } + assert_flat( + &curve, + |io| io.manifest_reads, + 1, + "a diff between two branch names must not read a __manifest per live branch", + ); + }) + .await; +} + /// A backlog walk reads the records of the commits it examines in one scan of /// `__history`, so its manifest reads are flat in the backlog, and pays at /// most two data opens per effectful commit, a term that grows with it. diff --git a/docs/rfcs/0030-cdc-time-travel.md b/docs/rfcs/0030-cdc-time-travel.md index 443c62102..d88b152e3 100644 --- a/docs/rfcs/0030-cdc-time-travel.md +++ b/docs/rfcs/0030-cdc-time-travel.md @@ -712,8 +712,12 @@ unreadable endpoint maps to the ยง6 typed outcomes. Authorization of the two-point form is `read` on the branch each endpoint resolves to. A commit endpoint authorizes against its authored branch, or against the recorded target of a proposal or snapshot. The two-point route -resolves a side before it authorizes it, so a forbidden side is reported +classifies a side before it authorizes it, so a forbidden side is reported exactly as a side that names nothing (`404`), as the commit diff does. The +engine authorizes the sides in order, each as soon as it is classified and +before the next side is looked at or a snapshot opens, so what the other side +names cannot reveal whether a forbidden side exists. A commit whose writing +branch is gone is no longer readable and is reported the same way. The branch form and the merge base require `read` on both named branches, source and target, on every request, a continuation included, before they read lineage. Their endpoints are the base and the source head, which can both be diff --git a/docs/user/branching/changes.md b/docs/user/branching/changes.md index f87eb9a70..e325d1b9d 100644 --- a/docs/user/branching/changes.md +++ b/docs/user/branching/changes.md @@ -29,13 +29,16 @@ with its first parent. Each response includes: [Blobs](../blobs.md)). Filter with repeatable `--kind node|edge`, `--type `, and -`--op insert|update|delete` options. `--limit` defaults to 1,000 and may be -at most 8,192. Results are deterministic by entity kind, type identity, logical -id, and operation. +`--op insert|update|delete` options. A page holds at most 8,192 changes; the +HTTP routes default to 1,000. Results are deterministic by entity kind, type +identity, logical id, and operation. Large results are paginated. The CLI normally follows every -`next_page_token`; passing `--page-token` fetches exactly one page. A page -token continues that one commit result and is not a change-feed cursor. +`next_page_token` and asks for 8,192 changes per page, because every page +restarts its comparison and fewer, larger pages cost less; passing +`--page-token` fetches exactly one page, of 1,000 changes unless `--limit` +sets the size. A page token continues that one commit result and is not a +change-feed cursor. A parentless commit returns `409` with reason `parentless_commit`. A diff that crosses a schema boundary returns `409` with reason @@ -190,9 +193,10 @@ Baseline kind and type filters select the snapshot contents. An operation filter applies only to the resumed feed. Baselines require the `export` policy action; commit changes, diffs and feed polling require `read`. A diff needs `read` on the branch each side resolves to; a side the actor cannot read -answers `404` exactly as a side that names nothing. `branch diff` and -`branch merge-base` need `read` on both branches, every page included, and -answer `403` otherwise. +answers `404` exactly as a side that names nothing, whatever the other side +names. A commit whose branch was deleted answers the same `404`. +`branch diff` and `branch merge-base` need `read` on both branches, every +page included, and answer `403` otherwise. The CLI's durable `--out` installation is available on POSIX platforms. The file contains only snapshot entity records, not the terminal handshake. The CLI