MOO-72 Commit 8: production hardening, feature flags, concurrency limits, version provenance - #18
Conversation
…its, version provenance, rollback runbook) Adds feature flags (FILE_LAYER_ENABLED, FUNCTION_LAYER_ENABLED, DEGRADED_ANALYSIS_ENABLED, EXPERIMENTAL_INTERACTIONS_ENABLED) surfaced via GET /api/capabilities so the UI can hide/disable a disabled layer instead of leaving a still-clickable control that just 503s. Adds a server-wide concurrency limiter shared across all five analysis routes, acquired only around the actual expensive work (creator-only for the file layer's in-flight-deduplicated pyan3 runs), with a 503+Retry-After response the client now parses correctly (previously only 429 was). Makes GitHub fetch concurrency and pyan3's subprocess buffer size configurable instead of hardcoded. Adds a single shared version/commit-provenance source (scripts/generate-build-info.mjs) consumed by both the server (/healthz) and the UI (a small corner badge), since the server runs outside Vite and a define-only injection would never reach it. Documents the confirmed current state of auth/allowlist/rate-limiting, two distinct rollback controls (release vs. domain), and the DNS/Hub cutover runbook in docs/deployment.md -- the live infrastructure steps themselves are explicitly out of this commit's scope, to be proposed individually later. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MAoVCjfAo9e7jWCRrMwyvq
OwenTanzer
left a comment
OwenTanzer
left a comment
There was a problem hiding this comment.
Three production-hardening issues to address before merge.
| const readsRetryAfter = status === 429 || status === 503; | ||
| const retryAfterHeader = readsRetryAfter && headers && typeof headers.get === 'function' ? headers.get('Retry-After') : null; | ||
| const headerRetryAfterMs = retryAfterHeader && !Number.isNaN(Number(retryAfterHeader)) ? Number(retryAfterHeader) * 1000 : null; | ||
| const bodyRetryAfterMs = typeof safeBody.retryAfterMs === 'number' ? safeBody.retryAfterMs : null; |
There was a problem hiding this comment.
[P2] Honor the response body's explicit retryability contract
The new disabled-layer routes return 503 { retryable: false }, but this mapper only considers diagnostics[0].retryable before defaulting every 5xx to true. As a result, a direct disabled-layer response is exposed to the UI as retryable despite the server explicitly saying otherwise (confirmed with mapServerJsonResponse(false, 503, {retryable:false}, ...), which yields retryable === true). Please let a top-level boolean safeBody.retryable take precedence before the status fallback, and add the disabled-503 case to server-request.test.mjs.
There was a problem hiding this comment.
Fixed — mapServerJsonResponse now checks typeof safeBody.retryable === 'boolean' after the diagnostic-level check but before the status-based 429/5xx defaults, so an explicit top-level retryable: false (the disabled-layer 503s) is honored instead of being overridden by the "5xx defaults retryable" fallback. Added 3 new cases to server-request.test.mjs: explicit false on a 503 is honored, explicit true still works as before, and a diagnostic-level retryable still takes precedence over a top-level one (diagnostic is more specific). Verified with the full suite (716/717, 1 expected skip), pushed as d8c2189.
| * @returns {Promise<{fileLayerEnabled: boolean, functionLayerEnabled: boolean, degradedAnalysisEnabled: boolean, experimentalInteractionsEnabled: boolean} | null>} | ||
| */ | ||
| export async function fetchCapabilities(serverAuthToken) { | ||
| if (cached && cachedForToken === serverAuthToken) return cached; |
There was a problem hiding this comment.
[P2] Do not cache feature flags forever by auth token
The auth token is not a configuration version: an operator can toggle these flags and redeploy without changing the token. This permanent cache means an existing tab never observes that rollout/rollback; most notably, after a layer is re-enabled, a page that cached false keeps refusing to call it until a full reload. Please revalidate capabilities (for example with a short TTL or explicit invalidation tied to deployment/version) so these production rollback controls take effect for long-lived sessions.
There was a problem hiding this comment.
Fixed — added a 30s TTL (CAPABILITIES_CACHE_TTL_MS) to the module-level cache, so a long-lived tab revalidates within a bounded time instead of caching by token forever. Chose 30s as a balance: cheap endpoint, doesn't need real-time push, but a rollback/re-enable becomes visible in-session soon rather than requiring a full reload. Added 5 new tests in capabilities-client.test.mjs (injected now() for determinism): serves from cache within the TTL, refetches once it elapses, always refetches on a different token, and the two existing failure-tolerance behaviors (non-ok response, thrown fetch error) still return null without poisoning the cache. Pushed as d8c2189.
|
|
||
| const info = computeBuildInfo({ | ||
| version: pkg.version, | ||
| envCommitSha: process.env.BUILD_COMMIT_SHA, |
There was a problem hiding this comment.
[P2] Consume Railway's provided deployment SHA
Railway provides RAILWAY_GIT_COMMIT_SHA to builds/deployments originating from GitHub, but this Railway-targeted provenance path only reads the custom BUILD_COMMIT_SHA. Whenever git metadata is absent from the remote build context and that custom alias was not configured, /healthz and the UI report unknown even though Railway supplied the exact SHA. Please fall back to process.env.RAILWAY_GIT_COMMIT_SHA before invoking git (while retaining BUILD_COMMIT_SHA as the provider-neutral override), and cover that precedence in the build-info tests. Reference: https://docs.railway.com/variables/reference
There was a problem hiding this comment.
Fixed — precedence is now explicit BUILD_COMMIT_SHA override > Railway's own RAILWAY_GIT_COMMIT_SHA > git rev-parse HEAD > "unknown". The explicit override still wins over Railway's variable deliberately (an operator setting it directly is a conscious choice that shouldn't be silently outranked), but it's no longer required just to get real provenance out of a standard GitHub-connected Railway deployment. Added 3 tests to generate-build-info.test.mjs covering the precedence: RAILWAY_GIT_COMMIT_SHA used when no override is set, BUILD_COMMIT_SHA still wins when both are set, and the git-rev-parse fallback when neither env var is set. Also updated docs/deployment.md's provenance section to describe the corrected precedence and cited the Railway variable reference doc. Pushed as d8c2189.
…che, read RAILWAY_GIT_COMMIT_SHA - mapServerJsonResponse now honors an explicit top-level `retryable` boolean (e.g. the disabled-layer 503s) before falling back to the status-based default, so a server that explicitly says "don't retry" is no longer overridden. - fetchCapabilities now expires its cache after 30s instead of caching forever by token -- the auth token isn't a configuration version, so a long-lived tab could never observe an operator's flag toggle/redeploy. - generate-build-info.mjs now reads RAILWAY_GIT_COMMIT_SHA as a fallback between the explicit BUILD_COMMIT_SHA override and git rev-parse, so a Railway build with no git metadata in the remote build context still gets real provenance instead of "unknown". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MAoVCjfAo9e7jWCRrMwyvq
1 participant
Summary
Code-and-docs portion of MOO-72 Commit 8 ("Production hardening, final domain cutover, and rollback controls"). Per the plan, this PR deliberately excludes the live infrastructure steps (promoting Railway, DNS cutover, Moopertonic Hub link, reaffirming
ALLOWED_OWNERS=*) — those are real production/external-system changes that get proposed and confirmed individually after this merges, not bundled into a single approval.FILE_LAYER_ENABLED,FUNCTION_LAYER_ENABLED,DEGRADED_ANALYSIS_ENABLED(gates the file layer's real pyan3→tree-sitter degrade path specifically — there's no separate "renderer fallback" subsystem to gate),EXPERIMENTAL_INTERACTIONS_ENABLED(currently gates no behavior — none exists yet, shipped as a documented no-op hook, defaultfalse). Surfaced via a new authenticatedGET /api/capabilitiesso the UI can hide/disable a disabled layer's affordance instead of leaving it clickable behind a 503, and in/readyz's authenticated detail for operators.server/lib/concurrency-limiter.js): a server-wide cap (MAX_CONCURRENT_ANALYSES, default4, explicitly documented as not load-tested — a conservative starting point, not evidence-based) shared across all five analysis routes, acquired only around the actual expensive work — after the cache-miss check, and for the file layer specifically only by the in-flight registry's operation creator, never a subscriber joining someone else's already-running work. A 503 at capacity carries both aRetry-Afterheader and aretryAfterMsbody field; the client (src/state/serverRequest.js) previously only readRetry-Afterfor a 429, silently dropping this delay for a 503 — fixed.GITHUB_FETCH_CONCURRENCY(was a hardcoded default param) andPYAN3_MAX_BUFFER_BYTES(same).scripts/generate-build-info.mjs, run as part ofnpm run build) consumed by both the server (/healthz) and the UI (a small corner badge) — the server runs directly vianode server/index.js, never processed by Vite, so adefine-only injection would never have reached it. Env-override →git rev-parse HEAD(caught) →"unknown", never an uncaught build failure.docs/deployment.md(new): confirmed current state of auth/allowlist/rate-limiting/secrets, two distinct rollback controls (release rollback via a temporary git worktree vs. domain rollback), and the DNS/Hub cutover runbook as a recorded procedure for later.Test plan
InFlightRegistry), feature-flag config validation,/api/capabilities//readyzfeatureFlags check,readBuildInfo/generate-build-info.mjs's purecomputeBuildInfo(env-override, git-fallback, uncaught-failure-proof, dirty detection), client 503-retry parsing.FILE_LAYER_ENABLED=false→/api/graph/filereturns a real503,/api/capabilitiesreflects it,/healthzreports realversion/commitSha/dirty.MAX_CONCURRENT_ANALYSES=1with two genuinely concurrent real repository-analysis requests (psf/requests,octocat/Hello-World) → one succeeds with a real graph, the other gets a real503withRetry-After: 2andretryAfterMs: 2000.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
https://claude.ai/code/session_01MAoVCjfAo9e7jWCRrMwyvq