Skip to content

TC-411: performance optimization - #206

Open
samgbafa wants to merge 5 commits into
mainfrom
skgbafa/tc-411-db-roundtrips
Open

samgbafa wants to merge 5 commits into
mainfrom
skgbafa/tc-411-db-roundtrips

Conversation

@samgbafa

@samgbafa samgbafa commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

TC-411: https://linear.app/tinycloud-labs/issue/TC-411/perfnode-establish-and-enforce-a-per-invoke-db-round-trip-budget

Summary

tinycloud-core/src/auth_graph.rs: added AuthGraphSnapshot::load_guarded(db, roots, guarded_keys), which re-derives the ancestor closure on the connection/transaction obtained after chain guards are held and fails closed (ChainTraversalError::LimitExceeded) if the freshly observed node set does not exactly equal the pre-guard guarded key set. This closes the race where a delegation registration commits (revealing new ancestor edges) while an invocation is waiting to acquire its shared chain guard on the same key, which previously let load_from_closure authorize against a stale, ancestor-incomplete view. Updated the now-inaccurate doc comments on load_from_closure and the low-level reuse-cost test, and added a new deterministic test snapshot_load_guarded_fails_closed_on_concurrent_registration asserting fail-closed behavior on a stale guarded-key set and correct success once the guarded set matches. tinycloud-core/src/db.rs: both branches of invoke_with_options_mode (read-only and mutation) now call AuthGraphSnapshot::load_guarded(&self.conn/&tx, &roots, &lock_keys) instead of load_from_closure(..., lock_keys, closure_edges), so authorization always runs against a snapshot verified complete on the guarded connection; revocations are still read after guard acquisition and, for mutations, inside the transaction, unchanged. Verified (no code change needed) that routes/mod.rs's handle_sql_invoke derives its SQL constrained-statement caveat candidates from auth_result.sql_constrained_statement_candidates -- populated by the same validated request-scoped snapshot verify_auth_admitted already builds via KvInvokeOptions{derive_sql_constrained_statement_caveat:true} -- and resolves ambiguity via resolve_constrained_statement_caveat (tightest-caveat containment, fail-closed on incomparable candidates) entirely after authorization completes, with zero added statements; the old pre-authorization derive_chain_constrained_caveat call and its separate-connection query have already been removed, and derive_chain_constrained_caveat_with_conn is retained only as a directly-tested helper.

Acceptance Criteria

  1. Check in an exact budget table for KV get, metadata/head, list, put, delete, homogeneous get/head and put/delete batches, and warm/cold SQL read/write at delegation depths 0, 1, and 4. Define depth as delegation edges and record batch size. Separate foreground capability-DB acquisitions, transactions, and statements from replay, grouped read audit, embedded SQL execution, and artifact persistence.
  2. Replace the loose closure result with an immutable request-scoped snapshot. The pre-guard closure query may be reused only after auditing every parent_delegations writer and proving persisted ancestor edges cannot be changed or backfilled. Missing roots or a concurrent registration not represented by the snapshot must fail closed. Revocations must still be loaded after chain-guard acquisition and, for mutations, inside the transaction.
  3. Read authorization receives the already-loaded snapshot; validation must not issue a second closure or full graph load. Graph batch-query counts must be independent of delegation depth.
  4. SQL authorization derives the effective persisted constrained-statement caveat from the validated snapshot and adds zero statements. Multiple roots and incomparable constraints fail closed; selection cannot depend on row order.
  5. event_spaces performs no revoked-events query when the invocation contains no Revocation event, without changing revocation-event behavior.
  6. Automated assertions enforce every checked-in budget. Depth 4 must equal depth 1 for equivalent authorization and SQL-caveat work. Batch budgets are documented and enforced but batching implementation is not changed in this PR.
  7. Existing bounded telemetry stages are verified, not redesigned. No SQL values, paths, DIDs, CIDs, authorization headers, or other high-cardinality secrets are recorded.
  8. Durable replay insertion, grouped read-audit acknowledgement, history, hook behavior, conditional writes, and storage transaction boundaries remain semantically unchanged.
  9. Production PostgreSQL TLS traces, deployed topology, rollout evidence, S3 atomicity redesign, KV batching, hook re-query removal, and any new production statement metric are explicit follow-ups and do not block this locally provable PR.

Test Plan

Use SeaORM set_metric_callback in a focused route-level query-budget suite. Normalize statements by bounded operation category without retaining bind values. Delimit foreground invocation, replay, and read-audit phases so grouped background commits cannot make counts nondeterministic. Assert logical statement counts on SQLite and the repository's existing PostgreSQL test target; record backend-specific transaction-control behavior separately.

Run the full operation/depth matrix from the budget document with fixtures that assert their exact delegation shape. Cover warm and cold SQL paths separately and use explicit batch sizes 1 and 100. Verify one closure CTE per invoke, no validation reload on reads, depth-independent graph batches, zero incremental SQL-caveat queries, and absence of the empty revoked-events query.

Add deterministic concurrency tests in which a revocation commits between closure preload and guard acquisition and must deny, and a root or child registration races the preload and must either use a complete guarded snapshot or fail as missing parents. Retain or add semantic coverage for revoked leaves and ancestors, ambiguous or incomparable caveats, duplicate and missing batch keys, replay uniqueness, signed-time validity, conditional-write isolation, and immutable acknowledged audit records.

Required commands are cargo fmt --all -- --check; cargo clippy -p tinycloud-node -p tinycloud-core --all-targets -- -D warnings; and cargo test -p tinycloud-node -p tinycloud-core. Merge requires the exact-budget suite and all existing security tests to pass.

Benchmark Plan

Use .context/benchmarks/profiles/TC-411.json and the pinned driver revision 9d4866f. Target scenarios are sdk.kv.get, sdk.kv.list, sdk.kv.put, sdk.sql.execute, and sdk.sql.query because they traverse the changed invoke authorization path. Any other scenario emitted by the harness is non-target and serves as a regression sentinel.

Build both recorded main baseline and candidate from clean source in release mode with identical features, toolchain, database setup, driver revision, dataset, concurrency, and host. Record both SHAs and binary identities; do not compare a debug or stale cached binary with a release binary. Run five paired rounds, preferably alternating baseline and candidate order.

For target p50, reject a regression exceeding both 5% and 0.15 ms. For non-target p50, use the same limits. For all scenarios, reject p95 above both 7.5% and 0.25 ms or p99 above both 15% and 0.5 ms when the direction appears in at least four consistent rounds. Latency improvement is not required when savings fall below the noise floor, but the exact statement reductions are required.

Before merge, attach the profile, commands, SHAs, environment summary, raw five-round results, p50/p95/p99 comparison, consistency classification, exact query-budget table, and passing test output. Confirm the driver reaches the real invoke route rather than a synthetic benchmark route. Local PostgreSQL results are useful supporting evidence; production TLS/topology latency is a non-blocking follow-up.

@samgbafa
samgbafa force-pushed the skgbafa/tc-411-db-roundtrips branch from dc331f8 to 89f6556 Compare August 4, 2026 02:56
@samgbafa

samgbafa commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

TC-411 ship-node evidence: rebased head 89f65560d2a1d9ce1502f3b2a99341c5834b6eb5 now contains current main TC-405 authorization behavior plus the reviewed round-trip optimizations. The superseded remote and local tips are retained in refs/backup/tc-411-before-reconcile-*-20260803; update used a verified expected-tip --force-with-lease.

Public happy path passed with Node SDK 9d4866fbb8415373737522698b388c950d70c1ee against local tinycloud-node: sign-in, KV put/get/list, and SQL execute/query completed via 9 observed signed POST /invoke requests. Focused exact query-budget tests passed; PR CI is fully green (workspace fmt/clippy/test plus Docker build). No merge or deployment was performed. Next: external benchmark gate.

…t docs

Ability/caveat rows must be batch-loaded for every node in the bounded
closure, not just cited proof roots, so an ancestor-only caveat (one
carried by a delegation's ancestor rather than the delegation itself)
stays visible to constrained_statement_caveat_candidates. This keeps the
change to a single bounded IN(...) query since nodes is already capped at
MAX_CHAIN_TRAVERSAL_NODES.

Also checks in docs/invoke-query-budget.md documenting the exact
per-request statement budgets this optimization establishes.
…ounts

Addresses the prior Sol review's blockers/majors:
- Load the guarded authorization snapshot and persist put blobs only after
  it accepts the request, before opening the mutation transaction, so an
  unauthorized/revoked/malformed-proof put can never create an orphan blob.
- Defer kv/get object-store reads until after the transaction commits so no
  explicit DB transaction spans object-store I/O, including mixed
  get+put/delete invocations.
- Batch current_kv index lookups for get/head into one statement regardless
  of batch size (batch_get_kv_entities).
- Batch multipart put history/projection persistence into exactly two
  statements independent of item count; delete reuses the already-loaded
  current_kv row/invocation id instead of re-querying kv_write.
- Bound closure-edge decoding by distinct node count before recursive cycle
  detection so oversized-but-sparse graphs reject before traversal.
- Expand docs/invoke-query-budget.md with the exact batch/warm-SQL budget
  matrix and cite the corresponding regression tests.
@samgbafa
samgbafa force-pushed the skgbafa/tc-411-db-roundtrips branch from 89f6556 to ed86ad9 Compare August 5, 2026 03:54
@samgbafa
samgbafa force-pushed the skgbafa/tc-411-db-roundtrips branch from ed86ad9 to 47e06a0 Compare August 5, 2026 04:15
@samgbafa
samgbafa force-pushed the skgbafa/tc-411-db-roundtrips branch from 979db12 to 8a3d69f Compare August 5, 2026 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant