perf(changes): resolve diff sides once, skip commit lookups for branch names, use full CLI pages

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

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<usize>,
         /// 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<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<usize>,
         /// Fetch exactly one page starting at this opaque token
@@ -1113,7 +1115,8 @@ pub(crate) enum CommitCommand {
         #[arg(long)]
         uri: Option<String>,
         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<usize>,
         /// Fetch exactly one page starting at this opaque token
@@ -1187,7 +1190,8 @@ pub(crate) enum ChangesCommand {
         /// Start mode: now | beginning | after:<commit_id> (default now)
         #[arg(long, conflicts_with = "cursor")]
         start: Option<String>,
-        /// 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<usize>,
         /// Filter by entity kind (repeatable): node | edge

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<usize>) -> Option<usize> {
+    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)?;

crates/omnigraph-server/src/handlers.rs

@@ -4086,38 +4086,6 @@ fn required_diff_param(value: Option<String>, 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 <from> to <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 <from> to <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<String>,
     max_changes: Option<usize>,
 ) -> std::result::Result<omnigraph::changes::GraphDiffPage, ApiError> {
-    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.

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 [

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,