Skip to content

feat(byok): add durable User Runner inference - #712

Open
XuPeng-SH wants to merge 52 commits into
matrixorigin:mainfrom
XuPeng-SH:feat/runner-byok-recovery
Open

feat(byok): add durable User Runner inference#712
XuPeng-SH wants to merge 52 commits into
matrixorigin:mainfrom
XuPeng-SH:feat/runner-byok-recovery

Conversation

@XuPeng-SH

@XuPeng-SH XuPeng-SH commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Implements Runner-owned BYOK inference while retaining Astra's single durable Agent Backbone. Server owns admission, policy, request compilation, continuation, usage, and audit; only the explicitly selected Runner performs provider I/O with local credentials.

  • Adds local model setup and CLI/TUI management without moving private provider credentials or endpoints into Server.
  • Binds requests to durable exact-attempt grants, a pre-I/O journal fence, custodied terminal responses, and canonical acknowledgements.
  • Restores checkpoint-bound custody through the existing Agent Loop, preserves multi-round recovery identity, and constrains recovered tools to the intersection of historical and current policy.
  • Preserves capped-prefix continuation authority; malformed custody, gaps, stale receipts, revoked tools, and missing Work authority fail closed.

Post-review hardening

  • Linux/macOS terminals in the same deployment/account share one private inference-only host and stable journal. Each terminal has an independent credential lease; environment-backed model identities cannot borrow another terminal's key. Detaching a client does not cancel an already-started provider request.
  • Private IPC verifies peer UID, installation/account identity, and compatible proxy/CA settings before accepting credentials. Client, binding, frame, timeout, and idle-drain limits are explicit. Tool Runner registration stays separate.
  • Reconnect reloads the latest token from the same selected CLI profile and revalidates its account. Logout, blank tokens, or an owner change fail closed without a second refresh authority or inherited token override.
  • TUI setup retains saved configuration and its connection after a failed explicit test. Save without test does not call the provider or change the current model. Session/account/selection/attachment-epoch checks fence late setup results. Dead connection metadata no longer prevents retrying setup in the same window.
  • Canonical manifest/head projections carry the exact admitted Offering through resume and fork, not a credential lease or live grant. Explicit --model wins at launch; missing legacy identity requires a fresh selection instead of a friendly-name or Server fallback. Runner credentials are never selected merely because they are first in the catalog.
  • Same-pod live previews use bounded authenticated batches and per-connection immutable-attempt validation caching, not per-token SQL. The hub allows 128 attempts per pod / 16 per user / four 32 KiB slots per attempt. Each decoder caps its combined rendered text/reasoning prefix at 256 KiB.
  • Cancellation and elapsed deadlines take precedence over previews. Sequence gaps, saturation, prefix exhaustion, and late events degrade preview only; terminal custody remains authoritative. Prefix replay is linear and preserves legitimate repeated text.
  • Synchronized current main, including the existing Darwin session execution-lease fixes, on this same PR branch without rewriting published history.

Local verification

No CI wait was used for this verification.

  • make format-check and strict all-target clippy for the seven affected CLI/Edge/runtime/services/types/credentials crates passed.
  • Seven-crate runner matrix: 111 passed; model matrix: 419 passed. These filtered counts overlap other suites below.
  • Complete library suites: credentials 29, Edge 29, turn types 187, Server types 172 passed.
  • Additional CLI resume (87), continuation (25), fresh interactive preflight, services resume, runtime resume, and Server-only/explicit-Edge profile checks passed. Tests marked for optional live lanes were not counted as executed.
  • Live isolated MatrixOne: 10 Runner admission/custody/isolation/readiness tests, three canonical-context tests, and the runtime required-compaction/custody test passed. The new projection test commits, recreates the coordinator, restores exact identity, replaces it at a new causal boundary, and rejects a tampered durable head.
  • Actual astra-edge subprocess integration passed: verifies real WS Authorization, rotates the synthetic profile token and reconnects the same child/journal/boot, observes live output before provider completion, kills the host before terminal ACK, observes failed client liveness, then reopens the same journal and replays custody with exactly one provider request.
  • Edge binary token-rotation/owner/logout tests passed. The broader binary suite was 56 passed / one existing platform failure, described below.

Explicit limitations / release gates

  • Cross-pod live preview still has no trusted ephemeral relay. That placement converges through durable terminal custody and must not be marketed as live streaming.
  • Complete installed CLI/TUI/Server and cross-surface user journeys, and 100/500/1,000-session throughput, tail-latency, memory, fairness, and recovery measurements remain release gates. Functional bounds are not a production SaaS performance claim.
  • Managed local hosting supports Linux/macOS, not Windows named pipes. Optional completion purposes outside the published Runner policy remain explicitly unsupported.
  • Broader local macOS checks expose two unchanged baseline issues: the process-ownership shell fixture's copied sleep receives SIGKILL, and ordinary tool-Runner Bash is rejected because workspace_observation::stable_coordination_root currently supports Linux only. Those source files match main; no sandbox or coordination check was weakened to bypass them. Therefore this is not a claim that the complete repository offline gate or macOS tool-execution journey is green.

Closes #702

Copy link
Copy Markdown
Collaborator Author

Thanks for the substantial work here. The overall architecture is heading in the right direction: the Server retains the single Agent Loop and durable invocation ledger, while the Runner owns credentials, provider transport, dispatch fencing, and terminal custody.

However, I do not think this PR is ready to merge yet. I found three blocking issues:

1. Friendly model names are confused with provider model IDs

InferenceHost::project_bindings() publishes model.model—the provider model ID—as RunnerInferenceBindingDefinition::model_name. The Server catalog then exposes that value as the model name.

After setup, however, the TUI searches the catalog using draft.name, which is the user-defined friendly name:

entry.name.eq_ignore_ascii_case(&model_name)

For a normal configuration such as:

Name: work
Provider model: glm-5

the model can be saved, probed, and published successfully, but the TUI will fail to find it, time out after 12 seconds, and not select it.

Please carry the friendly/display name separately in the binding projection and complete setup using a stable binding or Offering identity—not a name lookup. This is also necessary to avoid selecting the wrong model when two Runners publish the same name.

2. The multi-terminal Runner lifecycle does not satisfy the stated isolation contract

Every interactive CLI invocation starts a new astra-edge child for the same workspace. At the same time, the inference journal permits only one process to acquire host.lock.

Environment credentials are attached only inside the process that successfully owns the inference host; there is no IPC path for another terminal to attach its own credential to that host. Consequently, two terminals using different values for the same environment-variable name may either:

  • make one Runner appear unavailable, or
  • execute through the credential inherited by the first terminal.

That violates UX-07 and may cross provider-account or billing boundaries.

This needs either:

  • one shared local Runner with an explicit attachment IPC protocol and attachment-scoped credential handles, or
  • genuinely independent per-terminal Runner identities and journals.

Simply spawning one child per CLI while sharing the same Runner identity and exclusive journal is not sufficient.

3. The TUI performs a real provider request without an explicit pre-submit choice

The setup form says:

Enter save

but submission automatically calls model check, which sends a real provider request. The user is only told that a bounded stream check is running after submission.

Even though the request is small, it may still incur cost or trigger provider-side audit activity. This also conflicts with UX-03 in the design document.

The form should explicitly offer:

  • Test and use — disclose that one bounded provider request will be made.
  • Save without test — save the model as unverified without provider work.

Additional implementation concerns

  • The CLI starts a Runner for most interactive surfaces even when no local model is configured.
  • Runner stdout and stderr are discarded, and there is no readiness handshake. Authentication failures, lock conflicts, or immediate child exits therefore degrade into a generic catalog timeout.
  • The global model-config revision is copied into every binding. Changing one model republishes unrelated bindings and temporarily invalidates their environment attachments. Configuration CAS revision and per-binding revision should be separate.
  • ProtectedFile is an owner-permission-protected plaintext file on Unix, not encrypted storage, and the system-keychain backend is not implemented. This may be acceptable for an explicitly scoped MVP, but the UI and documentation should describe it accurately.

The durable execution internals are otherwise thoughtfully designed: exact-attempt authority is persisted before provider I/O, the Runner writes a local fence before dispatch, terminal evidence remains in custody until Server ACK, uncertain delivery is represented explicitly, and the queues/artifacts/journals are bounded.

My recommendation is Request changes until the three blocking issues above are fixed. At minimum, please add end-to-end tests covering:

  1. a friendly name different from the provider model ID;
  2. two terminals with different credentials;
  3. Save without test producing no provider request;
  4. duplicate model names on different Runners resolving by stable identity.

XuPeng-SH commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up review after e21c4fa

Thanks for the two follow-up commits. They address several points from the previous review well:

  • the friendly display name is now separate from the provider wire model ID;
  • setup resolves the exact Offering for the current Runner and retains that identity through the picker/thinking flow;
  • Test and use versus Save without test is explicit before submission;
  • model configuration now has a per-model binding revision;
  • early Runner failures have bounded diagnostics.

I re-reviewed the updated implementation beyond those original findings. I still recommend Request changes, primarily for the first two issues below.

P0 — the provider credential is inside the general tool-execution environment

The managed Runner is spawned without env_clear(), intentionally inheriting the attaching terminal's environment so it can resolve an environment-backed provider key (local_runner_lifecycle.rs). The same astra-edge process also constructs DefaultToolExecutor for Server-originated Edge tool calls (main.rs).

Foreground bash inherits the process environment; only the detachable path calls env_clear() (shell_ops.rs). Therefore an allowed tool command can observe the selected BYOK key, as well as unrelated terminal secrets. For example, printing only "$WORK_LLM_KEY" is not reliably protected by the pattern-based output redactor: the configured variable name is arbitrary and the value may have no recognized prefix or assignment syntax.

This violates the central boundary that provider credentials never reach Server/model context, and also conflicts with the design gate that an inference-only Runner must not expose shell access.

Please put inference credentials in an inference-only process/slot that the general tool executor cannot read, and clear/allowlist the environment of every tool subprocess. Add an end-to-end canary test proving that a Server-issued bash call cannot observe the configured provider secret by name or by value.

P1 — a random Runner identity per CLI launch defeats durable recovery and accumulates permanent offline Offerings

Each CLI invocation now creates edge-session-<random UUID> (local_runner_lifecycle.rs). This avoids the previous cross-terminal journal lock collision, but it changes three durable identities together:

  • the inference host/journal root is scoped by server_url + user_id + edge_id;
  • the Edge tool invocation journal path is scoped by edge_id + workspace (main.rs);
  • the Offering ID includes Runner ID and journal ID (runner_model_bindings.rs).

Consequences:

  1. After a CLI/Runner crash, relaunching cannot reopen the old inference journal, so it cannot reconcile a fenced attempt or replay a terminal awaiting ACK. The old exact Offering remains offline and the new process publishes a different one. This is inconsistent with the PR's durable resume/recovery goal and the design statement that journal identity survives ordinary restart.
  2. Every normal launch with a configured local model publishes another enabled binding under a new Runner/journal identity. Disconnect removes the registry row but not the immutable publication. The catalog intentionally includes enabled offline publications and fails when more than 256 are present (runner_model_bindings.rs). With one model, a regular user can therefore eventually break their personal model catalog after roughly 257 launches.
  3. astra model remove can disable only the current random Runner's projection; prior offline session Offerings remain enabled.

This needs a stable recoverable host/journal identity plus attachment-scoped credentials, or an explicit persisted attachment identity with a real retirement/GC protocol. A new random durable identity on every process start should not be the lock/isolation mechanism. Shutdown should also drain/reconcile outstanding inference custody rather than relying only on kill_on_drop.

P1 — direct /model <name> still bypasses exact Offering identity

The picker path now preserves offering_id, but the TUI shorthand still accepts an arbitrary name, updates the footer, and explicitly clears the active Offering ID without consulting the catalog (slash_dispatch.rs). The line-mode implementation does query the catalog, but its name lookup takes the first match.

Duplicate display names are now expected because every terminal publishes its own Runner Offering. Thus the same command can either defer/fail later or silently select the first opaque Runner, depending on surface. Please make direct selection catalog-backed, preserve the exact Offering, and reject ambiguous names with actionable candidates. The picker should identify duplicates by a meaningful Runner/device/attachment label rather than only an eight-character Offering suffix (slash_dispatch.rs).

P2 — “Save without test” does not preserve an unverified state

The UI says the model was saved as unverified and tells the user to use /model “to test and select it.” However, LocalModelDefinition stores no probe status/evidence, the Runner publishes the model immediately as an active Offering, and the normal model picker neither tests it nor marks it unverified. After reopening the picker, the user cannot distinguish “available, not tested” from a tested model.

Please persist probe status/freshness per binding generation and expose it in catalog/repair UX, or change the wording and behavior so the product does not claim a state it immediately loses.

All current GitHub checks are green, but these behaviors need system-level coverage. In particular, I would add tests for:

  1. crash/relaunch with a fenced attempt and with a terminal awaiting ACK;
  2. repeated launch/exit without unbounded offline catalog growth;
  3. a BYOK credential canary that is invisible to all Edge tool subprocesses and outputs;
  4. duplicate-name selection through both TUI and line mode;
  5. SaveWithoutTest remaining visibly unverified without any provider request.

@XuPeng-SH

Copy link
Copy Markdown
Collaborator Author

Follow-up pushed on the existing PR branch: 01e1023.

This is a verified hardening slice, not a claim that the full release/SaaS acceptance gates are complete.

  • Exact Offering selection is session-local and survives next-turn metadata refresh without namesake fallback. Local configuration/secrets are deployment/account-scoped; the managed Runner pins the selected profile and verifies the authenticated account before loading model credentials.
  • An explicit inference-only Runner does not construct a tool executor. Bash inherits the canonical restricted environment, not arbitrary terminal material. This is not an OS sandbox against other programs running as the same local user.
  • Fixed the production logical-admission path that previously sent Runner calls through Server-only admission. Exact binding, grant, custody, settlement and optional-purpose policy remain authoritative. Current optional completions explicitly return runner_inference_purpose_unsupported; they do not silently use personal credentials.
  • Reserve bounded result-observation capacity before issuing a grant (1,024 total / 128 per user). Readiness uses one database-bound observer, batches of 128, local notifications and cross-pod fallback, without per-request polling transactions or session locks.
  • Native TUI setup now has private paste routing, masked keys, canonical pre-save validation, narrow-terminal/cursor handling, semantic theme colors, explicit cost/selection wording, and account/session/selection fences against stale background completion. Failed setup does not silently overwrite the active selection.
  • Canonical accepted/terminal trace and usage facts remain tenant/session-scoped and idempotent under terminal replay. Structured Runner spans correlate identities and phases without dumping request/response bodies or credentials.

Local verification: 151 targeted tests passed, including 10 MatrixOne service integration tests and the real ledger-to-Runner-custody required-compaction test. Relevant library/binary clippy with -D warnings, compile checks, make format-check, and diff checks passed. CI was not awaited.

Still open before merge/release: stable shared local-host/journal recovery across CLI restart and attachment lifetime; true live Runner preview/streaming; exact Offering persistence in the causal resume projection; and measured 100/500/1,000-session throughput, tail-latency, memory and noisy-tenant fairness. These limits are recorded explicitly in docs/design/runner-inference.md. The branch is also behind main; no default-branch changes or force-push were made.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review of exact commit f1058fb5144a50cf89e2925928778d9e7745be14. Recommendation: address the two P2 findings below before merging. Submitting COMMENT because the connected account is the PR author.

This revision materially improves the earlier architecture. The managed host has a persisted installation identity and journal, uses inference-only dispatch, and receives environment credentials through per-terminal IPC leases after owner/installation checks. Startup clears the inherited environment; the inference-only connection rejects tool requests before entering the tool-executor path. Acknowledged environment-binding disables also remove the ephemeral local publication entries. Direct model selection now resolves exact catalog identities and rejects ambiguous names, and Save without test accurately says that neither testing nor selection occurred.

I traced the shared-host attach/detach/reconnect lifecycle, journal publication/fence/custody handling, model selection/setup, causal Offering restoration, and bounded preview/continuation coordinator. The architecture retains one durable execution owner. The new subprocess regression meaningfully covers token rotation in the same child, output before provider completion, crash before terminal ACK, and same-journal custody replay with one provider request. I inspected that test; I did not execute it locally.

Two setup-to-execution mismatches remain:

  • P2: an accepted full Chat Completions endpoint is probed correctly but has /chat/completions appended again during real Runner dispatch.
  • P2: the explicit local probe constructs a different token-limit wire contract from admitted inference, rejecting otherwise valid OpenAI o-series configurations.

Both are detailed inline. Please test the actual setup/check-to-dispatch boundary against a strict local provider, including path/query preservation and model-specific request validation. Separate passing tests of the probe and executor do not catch these disagreements.

Validation and limits:

  • Locally executed python3 -m unittest discover -s scripts/schema -p 'test_*.py': 47 passed; the CI-script suite: 60 passed.
  • Downloaded the current services/turn-core/plan CI job log: 5,931 tests passed, including Runner waiter bounds and the test preventing implicit selection of Runner credentials. These are CI results, not local Rust executions.
  • At the latest check, runtime, CLI non-edge, core/bridge and both online lanes were still running; Static Checks was also in progress. No claim that complete CI is green.
  • Cargo/Rust and a disposable MatrixOne service are unavailable locally. The findings are source-traced; the o-series restriction was checked against OpenAI's official generated API contract. No paid-provider request, installed CLI/TUI/Server journey, cross-pod deployment, or performance benchmark was run. The PR's documented cross-pod preview, platform and performance release gates remain separate from this review.

Comment thread crates/astra-edge/src/inference_host.rs Outdated
let mut endpoint = reqwest::Url::parse(&model.base_url)
.map_err(|_| InferenceHostError::InvalidRequest)?;
endpoint.set_path(&format!(
"{}/chat/completions",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Use the same endpoint normalization for checking and dispatch

A saved base_url ending in /v1/chat/completions is accepted by LocalModelDefinition::validate. The CLI deliberately supports this form: chat_completions_endpoint leaves it unchanged, and provider_endpoint_is_derived_without_rewriting_query_or_duplicating_path explicitly asserts that behavior (including a query string).

This dispatch path always appends the suffix. For https://provider.example/v1/chat/completions?api-version=1, Test and use probes the correct endpoint, but the first admitted inference sends to /v1/chat/completions/chat/completions?api-version=1. A normal provider returns 404 despite setup having reported success.

Extract one local endpoint-construction owner shared by probe and dispatch, or reject the full-endpoint form consistently before saving. Add a test that saves the full endpoint, runs the real check, then dispatches an admitted request against the same strict fixture; assert identical path/query and exactly one provider call per explicit action.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 5bfd47e. Probe and InferenceHost dispatch now use one endpoint normalizer in astra-inference-adapter; full endpoints are not appended again and query parameters are preserved. The new saved_endpoint_probe_and_granted_dispatch_share_strict_wire_contract test saves through the actual local setup path, runs the real check, and dispatches through the real InferenceHost with a fixture grant to the same strict local HTTP provider. It covers both base and full endpoints, exact encoded query preservation, successful terminal custody, and exactly one HTTP request per explicit action. Additional endpoint cases cover trailing slashes, encoded path segments, and malformed URLs. All 10 local model tests passed. This is a local setup-to-host regression with synthetic grant/provider, not a claim of a full installed Server journey.

&serde_json::json!({
"model": definition.model,
"messages": [{"role": "user", "content": "Reply with OK."}],
"max_tokens": definition.max_output_tokens.min(4),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Align the explicit probe with the admitted request's token contract

The probe always sends max_tokens, while prepare_runner_request uses the canonical OpenAI request builder and emits max_completion_tokens. OpenAI's generated Chat Completions API contract explicitly says that max_tokens is incompatible with o-series models. Thus configuring an o-series model with a valid endpoint/key makes astra model check and TUI Test and use fail at request validation, even though the normal inference builder uses the accepted field.

Use a shared declared wire contract for the probe and admitted execution while retaining a small explicit budget and the no-retry behavior. Do not generalize one provider's dialect to every compatible endpoint. The existing stream-probe fixture matches method/path but does not validate these body fields, so it cannot detect this failure. Add a strict o-series case that rejects max_tokens and accepts the correct bounded request, plus invalid-key/model cases. This finding is based on source and the official contract; no live paid-provider request was made.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 5bfd47e. The probe and canonical OpenAI request builder now share the declared max_completion_tokens field setter. The probe remains bounded to min(configured limit, 4), including reasoning, with no retry or dialect fallback. The strict setup/check-to-host fixture rejects max_tokens, an invalid model, an invalid key, and an incorrect budget; error cases verify exactly one request and unchanged saved configuration. A separate regression exercises actual prepare_runner_request for o3 and verifies the same four-token field without max_tokens or temperature. Documentation explicitly scopes this local profile: legacy-only max_tokens endpoints are not certified, and a short stream check does not certify answer quality or tools. Verification: 10 model tests plus 26 Runner-related tests passed; 1 live-MatrixOne test was skipped. Four affected crates passed all-target clippy with -D warnings; format and diff checks passed. No paid-provider calls and no CI wait.

XuPeng-SH and others added 25 commits September 8, 2026 13:18
@XuPeng-SH

Copy link
Copy Markdown
Collaborator Author

Rebased onto latest main a6406dc and updated this same PR branch using an exact force-with-lease against the previously published 5bfd47e. New head: 0060e2f. The feature commits were replayed, with a final integration commit for semantic conflicts.

Design decisions:

  • Preserve main Memoria identity, revocation, scoped memory, and deployment-model eligibility rules alongside Runner owner isolation and durable recovery.
  • Root/completion admission uses one owner-aware revalidate_model_execution entrypoint for Cloud BYOK and Runner material. Cloud credentials remain Server-only material; memory authorization does not grant permission to spend deployment credentials.
  • Preserve main provider-specific token rules in astra_core::model_wire and remove the duplicate helper introduced by this PR. The local probe uses the same declared compatible dialect.
  • Integrate the validated Cloud BYOK DNS/proxy path with ProviderTransport while retaining the tunnel guard throughout the response. No unvalidated client injection, redirect, ambient-proxy bypass, or automatic retry was added. New strict transport tests cover HTTP, HTTPS, SOCKS5 and SOCKS5h proxies.
  • Keep main astra model add/probe/delete for Cloud BYOK. Device configuration now has an explicit astra model local add/check/show/remove namespace; TUI /model add remains local. Commands never switch credential locations because an alias matches. Help, repair hints and documentation were updated.

Local verification passed:

  • All-target compile checks and strict all-target clippy with -D warnings for affected crates; formatting and diff checks.
  • 406 model tests, 21 BYOK endpoint/proxy tests, and 99 Runner-filtered tests (groups overlap).
  • Full Edge library: 29; inference adapter: 50; CLI auth flow: 14.
  • 22 isolated MatrixOne/HTTP integration tests, including both Cloud and Runner cross-owner rejection, Memoria revocation and memory spending restrictions, and Runner custody/recovery.
  • 7 Cloud BYOK HTTP tests and the real managed-host subprocess test covering token rotation, crash before ACK, same-journal recovery and no provider redispatch.
  • 47 schema tests and 60 CI-script tests.

No paid-provider request or CI wait. This does not replace the documented installed-journey, cross-pod live-preview or production performance acceptance gates.

@XuPeng-SH

Copy link
Copy Markdown
Collaborator Author

Black-box / white-box challenge — findings only

Reviewed PR #712 at head 78440e3 against user behavior, multi-user/multi-session load, recovery, and failure handling. I did not modify the worktree or change the PR state.

P1

  1. TUI model update is destructive when validation fails. save_definition replaces the definition and removes the previous protected secret before Test and use probes the new configuration. A failed probe still reports “Current model unchanged”, but the bad endpoint/credential is persisted and the old secret is gone. The same ordering affects save-without-test, Runner startup failure, and catalog wait timeout.
    crates/astra-cli/src/cli/local_model_command.rs:152-201; crates/astra-cli/src/tui/event_loop.rs:8282-8305

  2. Continuation recovery misclassifies a valid consumed-prefix plus pending-suffix state as a logical-attempt gap. The recovery loop skips consumed receipts without advancing expected_attempt; with consumed attempt 0 and pending attempt 1, it compares attempt 1 against stale expected 0 and aborts recovery.
    crates/services/src/inference_execution/runner.rs:386-490

  3. Initial enrollment Unavailable is sticky for a live connection. A transient DB/persistence failure returns InferenceHelloAck::Unavailable; the client clears its generation, but the WebSocket and worker remain alive and do not retry or reconnect. Subsequent polls stay empty until an external restart/reconnect, while the Runner can appear connected.
    crates/runtime/src/server/edge/runner_inference.rs:344-373; crates/astra-edge/src/inference_connection.rs:284-290

P2

  1. Binding-publication rejection is not retried on the same connection. InferenceBindingRejected does not clear publication_sent. A transient StorageUnavailable or conflict leaves the journal entry pending but suppresses every subsequent send for that operation on the current socket.
    crates/astra-edge/src/inference_connection.rs:320-329,546-560

  2. Server and Edge have incompatible transfer concurrency limits. The server may schedule up to 8 transfers per connection, while Edge rejects dispatch number 5 once incoming.len() >= 4. The worker treats that capacity error as connection failure, causing churn and delayed replay under concurrent sessions.
    crates/runtime/src/server/edge/runner_inference.rs:21-30,618-681; crates/astra-edge/src/inference_connection.rs:344-352,710-727

  3. A host-wide mutex is held across slow I/O and request preparation. The state lock covers journal lookup/fence, config lease, secret retrieval, and transport preparation; cancellation also waits for it. One slow request therefore blocks unrelated sessions and delays cancellation, creating head-of-line blocking despite the advertised multi-client capacity.
    crates/astra-edge/src/inference_host.rs:562-681,787-811

  4. Standalone environment refresh does not converge on credential removal. --inference-only ignores missing/empty environment variables without detaching the old value. If a model is deleted and recreated with the same name/revision, an old credential can still match and be sent. The same missing-credential model is still advertised as a candidate, so users can select an offering that always fails with CredentialUnavailable.
    crates/astra-edge/src/main.rs:264-301; crates/astra-edge/src/inference_host.rs:360-381,644-652

  5. Credential refresh can exceed the IPC aggregate frame limit even when every item is within contract. The sender packs all credentials into one frame capped at 64 KiB, while each value may be about 8 KiB and up to 256 values are accepted. Eight near-limit values are already enough to exceed the frame. The refresh loop then breaks and detaches all credentials for that client.
    crates/astra-edge/src/local_host.rs:25,379-397,498-503,528-569,594-614

  6. There is no per-session fairness; one session can monopolize all active slots. Capacity is checked only against the host-wide limit of 4. Four slow requests from session A cause session B to receive capacity/no-start rather than queueing, and the resulting attempt is terminal instead of waiting.
    crates/astra-edge/src/inference_host.rs:588-592,688-712; crates/services/src/inference_execution/runner.rs:1436-1458

  7. Headless --print starts a local Runner for an unrelated cloud request. If any local model is configured, print mode enters the Runner lifecycle even when the selected model is cloud-hosted, adding up to roughly 35 seconds of startup/wait and possible local-binary error noise.
    crates/astra-cli/src/entrypoint.rs:387-441; crates/astra-cli/src/cli/local_runner_lifecycle.rs:170-295

These are the high-confidence issues found in this pass. I did not count lower-confidence protocol/design observations as findings, and I did not run a real external-provider load test.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review of exact commit cb2dd30d980bc7afcdd88ff55141b68671248221. COMMENT because the authenticated account is the PR author.

I have not confirmed a new blocking defect in the changes reviewed. This is a substantive improvement over the previously reviewed f1058fb, but it is not a claim that the current full test matrix or production release gates have passed.

Both previous P2 findings are addressed in source. Probe and granted dispatch now use the same Chat Completions endpoint helper, preserving an already-complete path and query parameters. The local probe uses the shared output-limit serializer. The new saved_endpoint_probe_and_granted_dispatch_share_strict_wire_contract test crosses setup, probe and real InferenceHost dispatch against a strict local server for both base/full URLs, and checks invalid credentials/model failures without modifying saved configuration. This directly targets the earlier mismatch rather than merely duplicating helper assertions.

I also reviewed the new integration and recovery boundaries:

  • Cloud credential setup remains astra model add; device-only setup is explicitly astra model local add. Runtime Runner revalidation now shares the canonical owner-scoped resolver across foreground/background/delegated callers and retains typed errors instead of stale fallback.
  • The Cloud compatible transport retains its request-owned proxy guard and validated-address routing while using the exact-wire adapter.
  • TUI Test and use stages a candidate before applying it; a failed test preserves the previous configuration/secret, and revision checks reject concurrent updates.
  • Local model generations advance across deletion/recreation. Revisioned IPC snapshots are staged in bounded chunks and activated together, with stale snapshots rejected.
  • Dispatch preparation uses per-attempt ordering, shared configuration/attachment leases and cancellation signalling independent of slow configuration maintenance. The fence handoff is owned by a detached task so dropping a connection does not abandon a filesystem fence operation that is still completing.
  • Server delivery capacity retains reservations across claim expiry; definitive no-start/terminal evidence releases capacity. Continuation recovery advances over authenticated consumed prefixes even when their pending bit has cleared.

The new tests meaningfully target these boundaries: cancellation during paused preparation, cancellation during blocked maintenance, worker removal during a blocking fence, forged cancellation, stale chunked snapshots, candidate preservation, and consumed-prefix recovery. I inspected these tests; I did not execute them locally.

Non-blocking testing follow-up: runner_delivery_preserves_host_capacity_across_claim_expiry currently claims requests sequentially. Add a real-database barrier test with competing claims from different sessions, asserting that no more than four acquire slots, and a same-session case asserting at most one. The registry lock is the intended serialization point; this would verify the new SQL aggregate and lock behavior under actual contention rather than only sequential transitions.

Validation:

  • Locally executed the current schema suite: 47 tests passed.
  • No Cargo/Rust toolchain was available on PATH, and no disposable MatrixOne or paid provider was exercised. Rust/DB/subprocess results recorded in the PR description were not independently rerun here.
  • Immediately before submission, the head remained cb2dd30; PR Title passed, while Test Suite (34224603988) and Static Checks (34224604033) remained in progress. I am not carrying forward green results from an older head.
  • Installed cross-surface journeys, cross-pod preview, Windows hosting and performance measurements remain outside this review's executed validation. The documented release gates still apply.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review of exact commit 799325413155067941fc8b8fd4dba44894dbfd2e. COMMENT because the connected account, XuPeng-SH, is the PR author. Please address the two P2 findings inline before merging.

This review covered the 12 commits since cb2dd30, including the new local readiness projection, removal of superseded Runner paths, lease handoff/settlement and Linux/Darwin identity authorities. The earlier endpoint and token-field findings remain fixed. Positive changes include rejecting probe success without semantic/terminal stream evidence, fencing stale probe persistence against model replacement, retaining leases through cancellation-sensitive handoff, and adding a logical owner/session identity alongside lexical/canonical paths. The new Linux identity-race tests have passing CI evidence; that does not establish Darwin correctness.

New findings:

  • Local provider verification is shared by model definition, while environment credentials are terminal-local. A check using one terminal's credential is incorrectly projected as verification of another terminal's credential; a failure in one terminal also marks the other terminal's model as needing repair.
  • The macOS-only HashSet use has an unconditional import, causing the current Linux strict lint gate to fail. This is observed in the current head's CI, not inferred from an earlier run.

Validation:

  • Downloaded the current services/turn-core/plan log (job 102232164946): 5,969 passed, 377 skipped. Passing tests include execution_lease_lock_inode_replacement_cannot_create_a_second_executor, session_execution_identity_preserves_symlink_parent_dir_semantics, and session_execution_lease_fails_closed_if_identity_retargets_before_open.
  • Downloaded the current Static Checks log (job 102231971563): make lint failed with unused import HashSet at session_journal.rs:19 under -D warnings.
  • The readiness finding is source-traced through check_definition, persist_probe_state, credential_availability and local_model_status; no live paid-provider or installed two-terminal reproduction was executed.
  • No local Cargo/Rust toolchain was available on PATH. I did not rerun unaffected Python suites, a MatrixOne deployment, macOS device-range/kqueue tests, or performance benchmarks. The broader Test Suite was still running at the latest check. Author-reported verification in the PR body is not presented as independently executed evidence.
  • Rechecked the head immediately before submission; it remained 7993254.

}
match &definition.probe {
LocalModelProbeState::NotRun => "ready_for_check",
LocalModelProbeState::Passed { .. } => "ready",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Bind readiness evidence to the credential actually tested

For an Environment credential, credential_availability only checks that this process has a nonempty variable. The Passed/Failed probe is persisted in the shared owner-scoped model definition and checked only against binding_revision, which does not change when different terminals export different values for the same variable. Thus terminal A can run KEY=valid astra model local check work, then terminal B running with KEY=invalid gets status=ready, provider_probe=stream_verified, and select it with /model without its credential ever being checked. B's failed check also overwrites A's readiness. This contradicts the per-terminal credential boundary and the documented material-bound probe evidence; it is not a provider-I/O credential leak.

Scope verification to the actual terminal/material identity, or treat persisted environment-backed results as historical diagnostics and require an explicit check before presenting the current material as verified. Do not persist raw credential material or silently trigger another billable probe. Add public check/list tests with two processes sharing one configuration and different environment keys, plus rotation without editing the model definition.

Comment thread crates/services/src/session_journal.rs Outdated
@@ -16,14 +16,17 @@ use serde::{Deserialize, Serialize};
use sha2::{Digest, Sha256};
use std::borrow::Cow;
use std::cell::RefCell;
use std::collections::{HashMap, VecDeque};
use std::collections::{HashMap, HashSet, VecDeque};

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Gate the Darwin-only HashSet import

HashSet is used by ACTIVE_DARWIN_SESSION_EXECUTION_KEYS behind cfg(target_os = "macos"), but this import is unconditional. The current Linux Static Checks job fails here with unused import: HashSet under -D warnings, and make lint exits 101. Move HashSet into a matching cfg-gated import (or qualify its Darwin-only use) and rerun the strict Linux lint gate.

@XuPeng-SH

Copy link
Copy Markdown
Collaborator Author

Follow-up at d9068d4d9 — adopted the latest review findings.

  • persist_probe_state now re-reads the current credential and owner-local probe key before every CAS attempt. A delayed success or failure from an older key/credential identity returns ProbeIdentityChanged and cannot overwrite newer evidence; same-generation multi-terminal failure isolation remains intact.
  • Added an end-to-end delayed old-generation regression covering both late failure and late success, including key replacement before the newer check.
  • TUI now distinguishes protected-storage/persistence failures and credential/key-identity changes from an actual binding/configuration change, with a projection regression test.
  • Documentation records the generation identity and pre-CAS revalidation contract.

Local verification passed: 34 credential tests, 21 local-model tests, the TUI projection test, affected clippy with -D warnings, make lint, format and diff checks. Both GPT-6 final reviewers independently APPROVE this SHA with no high-confidence P0/P1/P2 findings. A new remote CI run was triggered by this push and is intentionally not awaited.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review of exact commit d9068d4d9d66ac997cdf3c3533b5cc767f3be81f, covering the four commits since 799325413155067941fc8b8fd4dba44894dbfd2e and their surrounding readiness, credential-store and TUI paths. COMMENT because the connected account, XuPeng-SH, is the PR author.

Both previous P2 findings are addressed. I have not confirmed a new blocking defect in this revision.

  • Readiness evidence is now bound to the credential material used by the actual probe. Status recomputes that identity for the current terminal, so another terminal's successful check cannot certify a different environment credential. Legacy authenticated evidence without a fingerprint becomes stale.
  • The persisted fingerprint uses a private local HMAC key rather than a plain hash of potentially low-entropy credentials. Key generation is part of the evidence identity; losing or rotating that key invalidates old observations and permits recovery instead of permanently preserving unverifiable results.
  • Before persisting a result, the implementation checks the current model binding and credential/key identity. This rejects delayed observations after replacement, removal or key rotation. Configuration revision checks protect the write against concurrent configuration changes.
  • A failed check for different credential material preserves another terminal's known observation within the same key generation. The failing terminal remains stale/ready-for-check rather than borrowing the successful state. This is a deliberate single-observation tradeoff: the immediate check reports the error, while the shared listing does not retain a separate failure history for each terminal.
  • Linux now conditionally imports the macOS-only HashSet; the current Static Checks workflow passes.

The new unhappy-path tests exercise materially different credentials, an HTTP 401, missing fingerprint-key recovery, and a delayed old-generation response after a newer successful check. I also checked how these persistence outcomes reach CLI/TUI messages and how candidate testing continues to preserve saved configuration on failure.

Validation:

  • Downloaded the current CLI non-edge job log: 4,097 passed, 992 skipped in its main filtered run. It explicitly records PASS for environment_probe_evidence_is_scoped_to_current_terminal_material, delayed_probe_from_old_key_cannot_overwrite_new_generation, and the existing binding replacement/removal tests.
  • The current Test Suite (34289750593), Static Checks (34289750569), and PR Title (34289750598) workflows report success. These are current-head GitHub CI results, not local Rust executions or a claim that skipped tests ran.
  • The terminal-material regression simulates terminal environments within one process; it is not an installed two-process integration test. The delayed old-generation failure traverses the mock HTTP request, while the old-success rejection also uses a direct persistence assertion. A real two-process test with independent environments would strengthen this boundary.
  • No local Cargo/Rust execution, paid-provider call, installed CLI/TUI journey, macOS device/lease test, cross-pod deployment or performance benchmark was run in this re-review. The broader release gates remain separate.
  • Immediately before submission, the live head remained d9068d4 and no review of this head was present in the retrieved review history.

@XuPeng-SH

Copy link
Copy Markdown
Collaborator Author

Follow-up at 93a80245e — adopted the latest review's non-blocking concerns without adding another runtime state machine.

  • Added a real two-process regression: separate child processes share only the owner-scoped config, use different values for the same environment variable, and verify that the invalid process sees stale/ready_for_check while its failure cannot overwrite the valid process's success.
  • Documented the intentional single-observation model: one canonical probe observation per binding, terminal-local material matching for readiness, and no durable per-terminal failure ledger. This keeps the owner-scoped file bounded and avoids retaining a growing history of credential identities while failing closed for every unverified terminal.

The existing key-rotation/late-result and TUI error-projection fixes remain unchanged. Local verification now includes 34 credential tests, 23 local-model tests (including the child-process regression), affected clippy, format and diff checks.

@XuPeng-SH

XuPeng-SH commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Incremental design and size audit (head a0b2162)

I reviewed the complete diff against main with an explicit check for duplicated legacy implementation.

  • The PR is 147 files, +34,120/-2,411 lines. A path-based estimate is about 5,099 test additions and 2,369 documentation additions; the remaining code spans CLI/TUI, owner-scoped credentials, the shared provider adapter, Edge/local host, the typed Runner protocol, Server admission/custody/recovery, and existing session/lease owners.
  • git diff --find-copies=80% finds no copied legacy implementation. It finds one intentional 88% SSE move into astra-inference-adapter; the old astra-turn-core/src/sse/blocks.rs was deleted. The duplicate runtime provider enum/global HTTP client were also removed, and 0c4d1c0 removed 668 lines of superseded compatibility paths.
  • The new modules are not parallel authorities: the adapter owns exact provider bytes and framing, astra-turn-types owns the wire contract, Edge owns local credential and journal execution, Services owns durable admission and custody, and the existing runtime owns context, Agent Loop, and continuation. The feature reuses inference_* ledger owners rather than adding a byok_runs status store.
  • Tests and unhappy-path assertions are part of the release contract: credential isolation, stale evidence fencing, cancellation, recovery, custody, and cross-owner rejection. Removing them only to reduce line count would weaken the safety guarantee.

One safe duplication was found and consolidated in this head: TUI and prepare_from_tui now share one canonical local-model definition builder and validator. The follow-up also explicitly cleans a protected secret when validation fails before a candidate can own it. Local verification: 24 local-model tests, 8 TUI setup tests, strict CLI clippy, formatting, and diff checks passed. CI was not awaited.

Possible follow-ups, intentionally not mixed into this PR: split very large test and design files; consider a shared private-file primitive only after confirming platform and error semantics; and revisit small CLI/TUI status-projection helpers. I did not find a safe deletion of a production authority or compatibility path in this review.

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.

[Feature]: Execute BYOK inference through personal and enterprise Runners

1 participant