Skip to content

fix(web): prime Depot catalog before Library calls - #685

Merged
jmagar merged 5 commits into
mainfrom
fix/library-catalog-502-20260916
Sep 18, 2026
Merged

jmagar merged 5 commits into
mainfrom
fix/library-catalog-502-20260916

Conversation

@jmagar

@jmagar jmagar commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

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/operations intentionally rejects operation dispatch with InvalidCatalog/502 unless the current actor has first loaded the canonical operation catalog. The Library directly dispatched depot.artifacts.list and depot.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

  • centralize catalog preflight in depotCall so Library, Depot Administration, source ingest, and other long-lived operation callers all obey the backend contract
  • share only an in-flight preflight across concurrent calls, preventing duplicate GETs without creating a stale client-side catalog cache
  • reject already-aborted calls before starting/joining preflight
  • bind preflight to the browser session epoch and re-check authority at the operation-dispatch boundary so actor/project transitions cannot reuse another actor's catalog
  • keep mock Library list/detail calls fully local with zero network requests
  • preserve the backend's five-minute actor catalog cache and fail-closed security contract unchanged
  • add Library fail-closed coverage plus client-level ordering, concurrency, cancellation, session-transition, and mock-mode regressions

Adversarial 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

  • Library component suite: 14/14 pass
  • Depot client suite: 51/51 pass
  • mock Depot regression: 1/1 pass
  • full Gateway Admin unit suite: 1066/1066 pass, 0 failures
  • Gateway Admin lint: pass
  • production build: pass, including TypeScript, 64 static routes, route bundle budgets, and static build ID validation
  • final build ID: 2982db3b0354934d67ba73b1064284d655dc6b99
  • git diff --check: pass

No backend authorization or operation-policy behavior is weakened; this aligns every real client operation with the existing actor-scoped catalog requirement.

@github-actions github-actions Bot added the javascript Pull requests that update javascript code label Sep 16, 2026
@jmagar
jmagar requested a lite review from Copilot September 16, 2026 16:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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/operations in NEXT_PUBLIC_MOCK_DATA=true builds 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.

@jmagar

jmagar commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

Adversarial review follow-up is in 079c0a97a.

  • Added an explicit NEXT_PUBLIC_MOCK_DATA=true regression proving Library list/detail never touch the network.
  • Moved the preflight from the Library component into shared depotCall, covering every real Depot operation caller rather than only Library.
  • Concurrent callers now share exactly one in-flight actor-catalog GET, then dispatch after it resolves. We intentionally do not serialize the POSTs because the backend policy is actor-scoped, not request-scoped; both calls are authorized against the same current actor snapshot.
  • Sharing is bound to browser session epoch and never persists beyond the in-flight preflight, so a new operation after completion gets a fresh catalog and cannot rely on a stale client cache.
  • Full Gateway Admin suite is 1064/1064 green, plus lint and production build.

Re-requested Copilot review on the new head.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 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 after await 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 with InvalidCatalog/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

@jmagar

jmagar commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

Second adversarial review is addressed in 2982db3b0:

  • already-aborted callers now fail before they can start or join a catalog preflight
  • the operation captures the browser session epoch, establishes the actor catalog, then checks cancellation and authority again immediately at the dispatch boundary before building the POST
  • session-transition regression proves no operation POST escapes after the actor changes
  • Depot client suite is now 51/51 and the full Gateway Admin suite is 1066/1066, with lint and production build green

Re-requesting review on the new head.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The catalog preflight and regression coverage address the cold-session failure while preserving safety and mock behavior.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@jmagar
jmagar requested a lite review from Copilot September 17, 2026 18:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jmagar jmagar 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.

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:

  1. 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_catalog failure, 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.
  2. 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.
  3. 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.

@jmagar
jmagar merged commit a1019c3 into main Sep 18, 2026
48 checks passed
@jmagar
jmagar deleted the fix/library-catalog-502-20260916 branch September 18, 2026 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants