[review] Correctness issues in recent master changes: Dream route 500, full scans in Source management, credential lock failures under concurrency, ownership migration conflicts #2456

These issues come from a review of changes made to master (1190dcbb) since 2026-10-27. The review covered:

• server/, including ownership.py, artifact_plugins.py, and the Unix socket listener
• builtin/persistence/
• builtin/runtime/
• builtin/inference/
• the new source_management/ package

Architecture-level issues are out of scope here; they are tracked in #2085.

Before filing, each item below was checked against #2081–#2085 (including N1–N14 in the #2085 comment) and against the currently open issues and PRs. None of them is reported there. Where an item relates to a closed issue, that is noted in the item.

Items are ordered by severity; none is high severity. Evidence levels:

• Reproduced: confirmed by running it.
• Confirmed: confirmed by reading the code along the call path, without a reproduction test.
• Likely: the code path holds, but triggering it requires specific timing or a specific environment.

1. Blank scope_id on Dream routes returns 500 (medium, reproduced)

• Location: server/app.py:3953/3969/3997 (get_dream_run, create_dream_run, list_dream_runs)
• Problem: These three routes declare scope_id / run_id as bare str, without the Path(min_length=1, max_length=256, pattern=r".*\S.*") that the other routes use. A blank scope reaches validate_scope_id, which raises ValueError, and _map_domain_error has no mapping for it.
• Reproduction: With a SQLite BuiltinRuntime and httpx ASGITransport, GET /v1/scopes/%20/dream/abc returns 500 {"code":"internal_error","message":"The Server failed."}. For comparison, GET /v1/scopes/%20/artifacts/atomic-memory/a returns 422.
• Suggestion: Add the same Path(...) constraints to these three routes, plus a regression test.

2. Source manager reads every target on every sync (medium, confirmed)

• Location: source_management/manager.py:483; see also :367, :460
• Problem:
  • Every call to sync_instance, add, and connect_instance reads and decodes all targets from Store.all("target").
  • sync_instance also sorts the full result.
  • add already has the follow_identity primary-key table, yet deduplication is still a linear scan.
• Impact:
  • Each Runtime's synchronize() polls every 0.1 seconds (runtime.py:234), and every page it turns triggers another full scan.
  • As the number of targets grows, JSON decoding will overwhelm the manager, which runs on a single event loop.
  • This conflicts with the AGENTS.md requirement to avoid whole-collection scans on incremental operations.
• Suggestion:
  • Add instance_id, identity, and target_id columns with indexes to the records.
  • Have sync_instance page by instance_id and target_id > cursor with a LIMIT.
  • Have add look records up through follow_identity.

3. credential() uses a non-blocking lock, so concurrent calls fail outright (medium, confirmed)

• Location: source_management/deployment.py:121-127, source_management/durable.py:47
• Problem: The credential() docstring says it serializes credential creation, but the RuntimeOwner it uses locks with LOCK_EX | LOCK_NB. A caller that does not get the lock immediately receives 409 instance-running instead of waiting.
• Impact:
  • When Server startup (prepare(), factory.py:193) and CLI powercontext source load call it at the same time, one of them fails with a misleading error.
  • If the Server is the one that fails, startup aborts.
• Suggestion: Use a blocking LOCK_EX (optionally with a timeout) for the configuration lock, separate from the Runtime owner lock.

4. Custom Artifact search SQL has no LIMIT (medium, confirmed)

• Location: builtin/runtime/custom_artifact_search.py:403-448
• Problem:
  • Neither the _text nor the _vector query has a LIMIT; candidate_limit is applied in _rank only after every row has been returned to Python.
  • The vector query computes a distance for every row in the scope / build.
  • With a non-builtin authorization provider, it also runs await bound.check once per identity (:293).
• Impact: Memory and latency of a single search grow linearly with the number of custom Artifacts in the scope. R9 in [risk] 性能与并发风险合集（R1–R14）：启动锁内全库工作、O(N²) 迁移、检索全量计算等 #2084 covered the same pattern for Atomic Memory search, and perf(atomic-memory): related recall peak memory grows linearly under complete enumeration #2448 / perf(atomic-memory): bound related recall candidates in SQL to reduce peak memory and recall cost #2449 are bounding related recall; this path was added by feat(artifact): add managed plugin framework and some example #2246 and falls outside both.
• Suggestion: Add ORDER BY ... LIMIT in SQL using max(limit, candidate_limit). If per-item authorization filtering leaves too few results, fetch more in batches.

5. Source Runtime / manager reconnect loops handle only some exceptions (medium, likely)

• Location: source_management/runtime.py:80, 139, 240, source_management/manager.py:575, 652
• Problem: run() and serve_channel() catch only httpx.HTTPError and ManagementError; any other exception escapes the reconnect loop as an ExceptionGroup.
• Impact:
  • Runtime side: When the manager hits an unhandled exception (for example, SQLite raising OperationalError on a full disk), it returns a plain-text 500. response.json() in post() then raises JSONDecodeError and the Runtime process exits. In background mode, no supervisor restarts it.
  • Manager side: An unexpected exception in respond silently ends the serve_channel background task created in the lifespan. The process keeps holding the owner lock but never reconnects, and the Server keeps reporting management unavailable.
• Suggestion:
  • Map non-JSON error responses to ManagementError.
  • Catch Exception (but not cancellation) as a fallback in the reconnect loops, log it through log_safely, and reconnect.
  • Add a done callback to the background task so the process fails and exits when the task ends with an exception.

6. Race between approval and online ownership migration (medium, likely)

• Location: server/app.py:4856-4881 (approve_candidate), server/ownership.py:236-278, server/authz/repository.py:449-457
• Problem:
  • For a Candidate with target=None, approve_candidate commits the approval first and only then writes the Artifact owner in a separate transaction (key candidate-artifact-owner:<scope>:<cid>).
  • That write is neither inside the approval transaction nor routed through the owner recovery outbox.
  • Online POST /v1/access/ownership/apply is blocked only by pending recovery evidence, so it does not cover this window.
  • R4 in [risk] 性能与并发风险合集（R1–R14）：启动锁内全库工作、O(N²) 迁移、检索全量计算等 #2084 listed create_artifact and propose_experience. fix(authz): commit local write ownership atomically #2164 made same-database writes and owner establishment atomic for those paths, and feat(authz): recover external owner writes from durable intents #2187 added recovery for external stores. The approval path's owner write is outside both fixes and still runs after review.approve() returns.
• Impact:
  1. With access control enforced, user U approves an experience Candidate that U proposed.
  2. After the approval transaction commits but before the owner is written, an administrator runs apply, which writes the owner as the migration's Principal under experience-owner:....
  3. U's establish_artifact_owner finds a different owner, raises AccessConflictError, and the approval returns 409.
  4. The Artifact's owner is permanently the migration's Principal.
• Suggestion: Any one of the following:
  • When migration encounters such a Candidate result, use the recorded proposed_owner and the same key as the approval path.
  • Move the post-approval owner write into the approval transaction (on_commit).
  • Route this step through the recovery outbox.

7. Migration uses a different owner idempotency key from the original write paths (low, confirmed)

• Location: server/ownership.py:505-518 (_owner_identity)
• Problem:
  • Migration writes with bounded_idempotency_key(f"{family}-owner", scope, artifact_id).
  • The original write paths each use a different key: base-artifact-owner:<sha> at app.py:4613, candidate-artifact-owner: at app.py:4879, and background-custom-owner: at builtin/runtime/artifact_plugins.py:321.
  • establish_artifact_owners treats a write as idempotent only when both the owner and the key match (repository.py:454).
• Impact: Suppose the owner write fails after an approval commits (for example, the process restarts), and an administrator then runs apply --owner user:U with the same owner as the original proposer. The subsequent approval retry, and create_artifact retries against an external Access store, consistently return 409. This contradicts the documented "no conflict when the owner is the same".
• Suggestion: Have migration reuse each family's original key derivation, or have retry paths treat an identical owner as success.

8. All authentication fails when the generated identities.json is unavailable (low, confirmed)

• Location: source_management/deployment.py:84
• Problem: DeploymentAuthentication falls back to the user-configured authentication provider only when the generated identity file returns Rejected.
• Impact:
  • When the file is unreadable, corrupted, or over the 1000-entry limit, it raises AuthenticationUnavailableError, and authentication fails for every user token.
  • The file is re-read and re-parsed on every request.
• Suggestion:
  • When the generated file is unavailable, fall back to the user-configured provider only and warn through log_safely.
  • Cache the parsed result by mtime.

9. Inference settings recorded in receipts may differ from the request actually sent (low, likely)

• Location: builtin/inference/pydantic_ai.py:121-137
• Problem:
  • _receipt_model_settings emulates only OpenAI's reasoning rules.
  • When an Anthropic profile sets anthropic_disallows_sampling_settings=True, pydantic_ai drops temperature, top_p, and top_k.
  • The thinking check looks only at supports_thinking and ignores thinking_always_enabled.
• Impact: The receipt's effective_settings records parameters that were never sent. This is the kind of mismatch 32631c1 / bae5d71 set out to remove.
• Suggestion: Derive the receipt settings from the request pydantic_ai actually prepares instead of re-implementing the rules.

10. 18 test failures on macOS (low, reproduced)

• Observed: uv run pytest -n 6 on macOS gives 9221 passed, 18 failed, 911 skipped.
• Cause:
  • 14 failures are in tests/test_source_management.py, tests/e2e/test_source_collect.py, and tests/e2e/test_source_deployment.py. RuntimeOwner.identity() at source_management/durable.py:59 unconditionally reads /proc/<pid>/stat, /proc/sys/kernel/random/boot_id, and /etc/machine-id, which raises FileNotFoundError on macOS.
  • The documentation says local collect deployment requires Linux, but these tests are not skipped by platform.
  • Running a Runtime on macOS shows deployment-failed: FileNotFoundError; check configuration and private credential files rather than a clear background-unsupported.
  • The other 4 failures are in tests/test_ci_installation_roles.py: Python in the newly created virtual environment exits with SIGABRT. This may be a local environment problem and is not yet confirmed.
• Suggestion:
  • Mark tests that depend on Linux process identity with skipif(sys.platform != "linux").
  • Have RuntimeOwner.identity() raise a clear unsupported-platform error on non-Linux platforms.

Other

Operational logs in source_management/runtime.py and source_management/write.py call logger.* directly instead of going through log_safely as AGENTS.md requires.

Reviewed with no issues found

• Inference privacy boundary: privacy.py, protected_transport.py, context_check.py
• Plugin loading and scope authorization
• Unix socket listener: directory and socket permissions, stale socket reclamation, cleanup on shutdown
• Database admission: subprocess isolation
• Source evidence declarations (feat(memory): persist versioned Source evidence and neutral Atomic Memory projection #2004) and historical migrations
• Exact-reference outcome receipt reads (perf(work): read outcome receipts by exact reference #2401)

mysql_compatibility.py and sqlite_compatibility.py were only sampled for consistency between interrupted-DDL recovery and the schema snapshots; they were not fully reviewed.

Environment

• Reviewed: master, commit 1190dcbb
• Test environment: macOS (Darwin, arm64), Python 3.13
• ruff check, ruff format --check, and ty check all pass