fix(web): prime Depot catalog before Library calls - #685
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Serialize catalog preflight and artifact calls to prevent concurrent deep-link requests from interleaving.
Pull request overview
Fixes Library 502s by priming Depot’s actor-filtered operation catalog before artifact requests.
Changes:
- Adds catalog preflight before list/detail operations.
- Preserves mock-data behavior.
- Adds ordering and fail-closed regression tests.
File summaries
| File | Description |
|---|---|
apps/gateway-admin/components/depot/library-page-content.tsx |
Adds Depot catalog priming before artifact operations. |
apps/gateway-admin/components/depot/library-page-content.test.tsx |
Tests request ordering and failed-preflight handling. |
Review details
Suppressed comments (2)
apps/gateway-admin/components/depot/library-page-content.tsx:30
- Please add a mock-mode regression for this branch. The component suite imports this module with mock mode disabled and only exercises the real catalog GET path, so it would not catch a future change that performs
/v1/depot/operationsinNEXT_PUBLIC_MOCK_DATA=truebuilds even though both Library operations are supposed to remain local in that mode.
if (!USE_MOCK_DATA) await depotOperations(signal)
apps/gateway-admin/components/depot/library-page-content.tsx:31
- The list and detail effects can enter this helper concurrently on a deep link. Each call starts its own catalog GET, then continues independently, so the request order can be
catalog, catalog, depot.artifacts.list, depot.artifacts.get; the detail POST is therefore not immediately preceded by its own preflight and the added assertion at the deep-link test will fail. Serialize each catalog→operation pair (or otherwise coordinate the calls) so another operation cannot interleave between the preflight and its POST.
if (!USE_MOCK_DATA) await depotOperations(signal)
return depotCall<T>(operation, params, signal)
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Adversarial review follow-up is in
Re-requested Copilot review on the new head. |
There was a problem hiding this comment.
🔵 Needs a closer look
Address the unresolved aborted-signal and session-epoch checks before operation dispatch.
Review details
Suppressed comments (2)
apps/gateway-admin/lib/api/depot-client.ts:270
- Check the caller's signal before starting or joining the shared preflight. With an already-aborted signal, the only
throwIfAborted()is afterawait preflight.promise, so this path still issues a catalog GET for a request that was already cancelled; the previous operation fetch honored the signal immediately. Keep the shared preflight independent of caller cancellation, but return this caller before creating/joining it.
const epoch = getBrowserSessionEpoch()
apps/gateway-admin/lib/api/depot-client.ts:316
- The epoch is checked inside the shared catalog promise, but not again after this
await. If the browser session changes after the GET completes and before the POST is built, this call can send the operation under the new actor even though that actor's catalog was never established; the backend then fails closed withInvalidCatalog/502. Capture the epoch for this call and compare it immediately before dispatch (or move that check into the operation-dispatch boundary) so a session change cannot recreate the cold-session failure.
await ensureDepotOperationCatalog(signal)
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Second adversarial review is addressed in
Re-requesting review on the new head. |
jmagar
left a comment
There was a problem hiding this comment.
Manual adversarial review completed against the current implementation and backend contract. This review was performed directly against the diff, call sites, browser authority/session semantics, Rust Depot dispatcher, and control-plane contract.
Findings identified and fixed in d23293e:
- TOCTOU after catalog preflight: Labby can restart or lose the actor catalog after the GET succeeds but before the operation POST. The old code would recreate the same 502. The client now recognizes only the exact
invalid_depot_catalogfailure, re-primes, and retries once. This is safe because the Rust route raises InvalidCatalog before dispatching anything to Depot; destructive retries preserve the exact same intent/idempotency key. Every other failure remains non-replayable. - Preflight coupled to Administration rendering schema: shared dispatch was using the strict
depotOperations()UI schema. That could reject a backend-valid control catalog merely because an unrelated operation cannot be rendered by Administration. The dispatch preflight now validates only a bounded named operation catalog; the strict schema remains where the Administration renderer actually needs it. - Cancellation semantics for shared preflight: the shared GET must not inherit one caller's AbortSignal, but an aborted caller also must not hang until that shared GET completes. Per-caller waiting is now abort-aware while the underlying single-flight preflight survives for concurrent callers.
Regression coverage now explicitly proves renderer/preflight independence, already-aborted zero-I/O behavior, mid-flight cancellation with a surviving concurrent caller, session/authority transition rejection, single-flight concurrency, exact InvalidCatalog one-retry recovery with a stable destructive intent key, and no replay for any other operation failure.
Verification after fixes: Depot client 55/55, mock Depot 1/1, Library component 14/14, full Gateway Admin unit suite exit 0, ESLint clean, production Next build + TypeScript + static route/bundle checks clean, and git diff --check clean.
No additional blocking/high findings remain from this manual pass.
Summary
Fix the production Library 502 by satisfying Labby's fail-closed actor-filtered Depot catalog contract before every real Depot operation.
Root cause
POST /v1/depot/operationsintentionally rejects operation dispatch withInvalidCatalog/502 unless the current actor has first loaded the canonical operation catalog. The Library directly dispatcheddepot.artifacts.listanddepot.artifacts.get, so a cold Library session had no actor catalog and failed before Depot was contacted. Production logs reproduced authenticated Library POSTs returning 502 while the configured Depot backends remained healthy.Changes
depotCallso Library, Depot Administration, source ingest, and other long-lived operation callers all obey the backend contractAdversarial review fixes
The first review found missing mock-mode coverage and concurrent preflight duplication; the second found an already-aborted request could still issue a catalog GET and a session could change after catalog completion but before POST dispatch. All four issues are addressed in the shared client with regression coverage.
Verification
2982db3b0354934d67ba73b1064284d655dc6b99git diff --check: passNo backend authorization or operation-policy behavior is weakened; this aligns every real client operation with the existing actor-scoped catalog requirement.