[ryuzdev]

Root cause

Round 1 found that a 504 from listAllApps is cached forever (lib/resource.ts keeps failures). Its timed auto-retry is dropped by this round (see Design): the real defect is the stall, and a retry only waits it out while the user watches a 15 s hang.

Flake H (run 39782655475, e2e (multi round 1 shard 1), admin.spec.ts:521). From lane-caddy-access.log (start time and duration):

Start	Request	Duration
13:05:47.536	POST /api/admin/adminOverview	5.29 s
13:05:47.556	GET /api/auth/get-session	5.27 s
13:05:47.556	POST /api/admin/listAllApps	15.08 s → 504
13:05:50.846	POST /api/prs/branchRulesGet	5.05 s
13:05:50.860	POST /api/environments/listEnvironments	5.04 s

Every stalled request touches the control D1 (session/admin reads plus a runner fetch), while D1-free requests in the same window stay fast (/api/streams 0 ms, static assets ~ms, runner deploys proceeding) — so the isolate event loop was not blocked and the runner was not the holder. Durations cluster at one 5 s wait per request, with listAllApps at 5 + 10 (withDb's 10 s bound losing the race to the 15 s middleware bound).

Issues #91/#92 (run 39764004222, multi r1 s2). Two workers loaded /apps/new at 10:37:14.55/14.59; each got every chunk in ms except one /assets/* chunk, which the control worker held for exactly 15.00 s until the timeout middleware returned a JSON 504:

Start	Request	Duration
10:37:14.620	GET /assets/rolldown-runtime-hePW80VL.js	15.0 s → 504
10:37:14.740	GET /assets/dist-CWwpyyv2.js	15.0 s → 504

Assets have no D1 of their own — yet they stalled identically. void.config.ts sets run_worker_first: true, so assets traverse the same middleware chain, and 03-db.ts runs ensureDbPromise() unconditionally on every request. One mechanism covers both incidents: the shared isolate-level D1 setup pass.

The mechanism (already half-documented in SPEC): ensureDb shares one setup pass per D1 binding; a waiter sits out 5 s on a pass another request started, then runs its own. celld drops a request's pending work when that request answers or its client leaves — and the pass was never registered with ctx.waitUntil. So whenever the starter's request finished first (a fast asset on a cold isolate, a navigating page), the pass never settled: every waiter burned the full 5 s, then started its own equally orphanable pass. 5 s, 10 s, 15 s, in cohorts — exactly both incidents. The waiters' herd additionally multiplied the damage: each expired waiter ran a full migration pass, hammering celld's single-threaded D1. Noite-side holder at the statement level is unidentifiable from lane artifacts (worker stdout is not captured); what is proven is the shared unregistered pass plus the herd amplifier, and both are removed here. A concurrent fresh-database DDL race (two passes migrating an empty DB) remains possible at first boot; it is narrowed from N passes to two and noted as residual below.

PR #88 is innocent (lane plumbing only, verified from its diff); issues #91/#92 should close with this PR.

Design

• lib/db.ts + middleware/03-db.ts: the shared pass outlives its starter. startSetupPass registers the pass with the worker lifetime (c.executionCtx.waitUntil, stamped by 03-db.ts, new EnsureDbOptions{ctx, waitMs}); a starter that answers or disconnects no longer orphans the pass. Waiters that outlast their 5 s bound share the single replacement pass instead of each starting their own (single-flight: the loser of the replace race shares the winner's pass outright). At most two concurrent passes; a failure still clears the entry so the next request retries. Rejected alternative: exempting /assets/* from ensureDb — principled (assets need no D1) but a second semantic, and waitUntil already covers all requests uniformly.
• lib/server/admin.server.tsx: one fewer D1 round-trip on every admin action. ensureAdminAccount takes the session's fresh role and returns early when it is already admin (better-auth reads the user row on every session read — verified no cookieCache in auth.ts and disabled by default in the installed better-auth — so the skip cannot resurrect a demoted admin; the demotion shows up on the very next request). The promotion path for non-admin anchored addresses is unchanged.
• lib/resource.ts: failures are evicted, never cached, never retried on a timer. A failed load surfaces on error and drops the cache entry, so a remount refetches;explicit refetch() is unchanged. No setTimeout, no retry budget, no loophole.

Security review

Independent reviewer subagent over the diff (middleware is a sensitive path): sound, no blockers, no concerns. Checked: (a) the promotion skip cannot keep a demoted admin (role is server-allowlisted from the DB-backed session, impersonation refused before it, anchor semantics unchanged); (b) the shared replacement is failAfter-bounded (5 s + 15 s worst case, middleware 504 independent) and failures still evict, so no poisoned-pass DoS; single-flight is a synchronous check-and-set, at most 2 concurrent passes; (c) waitUntil leaks nothing (WeakMap, swallowed rejection, degrades to old behavior without a ctx); (d) EnsureDbOptions carries no identity and ctx-less callers fail safe. Full findings in the thread. security-reviewed still needs the parent (never self-applied).

Verification

• New unit tests (all deterministic, zero wall-clock: injected zero bound, gated D1, loop-turn yields to quiescence): the starter registers its pass with waitUntil (1 assertion); N expired waiters share one replacement (prepare count 2× baseline, not 4×); admin role skips the promotion round-trip (0 statements) while a non-admin still promotes; a failed mount evicts so a remount refetches (calls 1 → 2, error → rows).
• Fail-before proven by stashing the prod files: the waitUntil, herd-count and remount-refetch tests fail on old code; all pass with the fix. The herd test fails on old code because waiters never expire inside the test (5 s bound) — the new code expires and single-flights.
• Gates: bun run check clean, bunx tsc --noEmit clean, bun run test 613 pass / 0 fail, bun run build clean (pre-existing chunk-size warning only). Runner untouched (no cargo gates needed).
• Hygiene: no setTimeout/sleep in tests (docker/check-tests.ts patterns checked); setImmediate loop-turn yields are not wall-clock waits.

CI

Release suite on both lanes (e2e label present). Run: ci 39794442088 (PR #100, SHA 9a1009a).

• e2e (multi round 1 shard 1) pass (5m04s), (multi round 1 shard 2) pass (4m34s), (single round 1 shard 1) pass (3m42s).
• e2e (single round 1 shard 2) FAIL: worker-errors.spec.ts:267 (src/index.ts frame never visible). Server side shows twin 15.01 s middleware 504s at 14:24:49.455/14:24:49.470 (listEnvironments, errorDetail, same app) while D1-backed siblings at the same instant stayed fast (get-session 0.04–0.09 s, adminOverview 0.07 s) — the hang is past D1-auth, in the unbounded runner fetches for that app (withDb's 10 s bound rules out a D1 hang: 15.01 s exceeds it). None of this PR's paths are in that hang (setup healthy per siblings; not admin actions; client cache cannot 504 a server action). D-family recurrence (S7 unlanded); reported, not retried.
• check (all slices), rust, worker, image, types, ui, cli pass. CodeRabbit pass (rate-limited).
• security-gate fail: needs the parent-applied security-reviewed label (security-sensitive tripped by middleware/**); independent review found no blockers/concerns (see above).
• ci-ok fail (aggregate of the two above).

Open risks / residual

• If celld drops even waitUntil-registered work on client disconnect (workers semantics say it survives; celld's exact behavior is unverified from here), the single-flight replacement still bounds the damage to one extra pass per 5 s generation. No lane time was available to probe this directly.
• Concurrent fresh-database migrations can still collide on non-idempotent DDL (pre-existing, boot-time only; narrowed N → 2).
• The exact first-drop trigger mid-lane (cold isolate via recycle/scale-out vs a transient setup failure) is not pinned from artifacts; the fix removes the 5 s-per-generation penalty regardless of trigger.

Round 3 — the 15.01 s pair (run 39794442088 single r1 s2, corrected pin)

Correction to the round-2 note above: the 15.01 s hang is NOT past D1-auth in unbounded runner fetches. findApp swallows every runner error into null → 401, and runnerFetch aborts at 5 s, so a runner stall resolves at ~5 s as 401/422 — it cannot produce a 504 at 15.01 s. The 504 bodies are 29 bytes ({"error":"Request timed out"}, the 01-timeout middleware's exact JSON), so the worker held both requests the full 15 s. withDb provably rejects at 10 s (probed). The only awaits on these paths that can legally pend to 15.01 s are readBoundedText (excluded: sibling POST bodies parsed fine in the same window) and the unbounded better-auth getSession D1 read. Full module-level audit (worker server code, void framework, better-auth/kysely/sql-d1/paranorm internals) found no other unregistered shared promise: the hung pair stalled in getSession's D1 read while a transient control-D1 stall (seeded ~180 ms after the same app's apps/get + errorList + eventsSnapshot were aborted mid-read at 0.29–0.31 s) held it; concurrent identical reads succeeded, so no worker-side promise held them. S7 was already on main, so that explanation is withdrawn.

Design (round 3)

• lib/db.ts: a waiter without a worker lifetime fails fast. The expiry path previously deleted the stored pass and started an unregistered replacement any waiter could then await — an abort of that waiter orphaned it for the next 15 s. Now a lifetime-less waiter throws instead of storing; the middleware (always first, always ctx-carrying) still owns every replacement. Middleware-first ordering makes this unreachable in the healthy path — it only triggers on a genuinely wedged pass.
• lib/server/session.server.ts: the session read is bounded at 5 s. getSessionOrNull races auth.api.getSession against the existing abortable sleep helper; expiry throws (actions fail 500 with cause; browser routes deny as signed-out via their existing .catch(() => null) — deny either way, never proceed, never a 15 s hang). The bound is injectable for tests; production callers all use 5 s.
• Guard: docker/check-worker-rules.ts (+ unit tests, + check.yml step). CI tripwire for the class: module-level in-flight promise/cache/client in worker server code (allowlist: the waitUntil-registered setupPasses), and auth.api.getSession outside the bounded wrapper.
• SPEC.md Worker rules + changelog.d/admin-app-list-timeout.md. Round-2 wording kept; added the fail-fast and bounded-read rules plus the guard reference.

Security review (round 3)

Independent security-reviewer subagent over the round-3 diff: no blockers, no regressions. All auth decisions fail closed (throw → 500 via apiAction, or → null → 401/redirect via existing route catches; no path treats throw as authenticated). The 5 s bound is not attacker-settable in prod and reduces hold time (15 s → 5 s). No timer/listener leaks, no unhandled rejection on the losing branch. The CI scanner executes nothing it scans (read-only regex over text), no ${{ }} interpolation in the new step, no ReDoS shape in the patterns. Two Low findings (both fixed): changelog wording on route-side 401s (F1), guard-scope wording softened to tripwire (F2). security-reviewed still needs the parent (never self-applied).

Verification (round 3)

• New/updated unit tests, all deterministic (injected bounds, gated D1, loop-turn yields; no sleeps): lifetime-less setup waiter fails fast and leaves the stored pass intact (prepare count stays 1× baseline; fails pre-fix with 2× baselines); expired ctx-ful waiters still share one replacement (updated to carry spy lifetimes); wedged session store throws /did not answer/ at the injected 20 ms bound (fails pre-fix: hangs to the test timeout — the incident shape); healthy store resolves; missing session stays null.
• bun run check clean, bunx tsc --noEmit clean, bun run test 617 pass / 0 fail (74 files), bun run build clean (pre-existing chunk-size warning only), bun test docker/ 59 pass, bun docker/check-changelog.ts clean, bun docker/check-worker-rules.ts clean (53 files).
• Runner untouched (no Rust files in the diff; cargo fmt --check clean; CI's rust job is the gate).

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 25 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Source mode: Auto
• Repository: ryuzcorp/noite
• Branch: fix/admin-app-list-fresh
• File: .coderabbit.yaml
• Commit: 58ddbf9
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 8e87abf6-7d82-4bd3-8526-567337473789

📥 Commits

Reviewing files that changed from the base of the PR and between ba452f1 and 58ddbf9.

⛔ Files ignored due to path filters: 2

• changelog.d/admin-app-list-stall.md is excluded by !changelog.d/**
• changelog.d/admin-app-list-timeout.md is excluded by !changelog.d/**

📒 Files selected for processing: 14

• .github/workflows/check.yml
• SPEC.md
• apps/noite/src/lib/db.test.ts
• apps/noite/src/lib/db.ts
• apps/noite/src/lib/resource.test.tsx
• apps/noite/src/lib/resource.ts
• apps/noite/src/lib/server/admin.server.test.ts
• apps/noite/src/lib/server/admin.server.tsx
• apps/noite/src/lib/server/session.server.test.ts
• apps/noite/src/lib/server/session.server.ts
• apps/noite/src/middleware/03-db.ts
• apps/noite/src/testing/d1.ts
• docker/check-worker-rules.test.ts
• docker/check-worker-rules.ts

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ryuzdev]

Parent review across rounds 1–3: accepted.

• Root cause (H, [flake] fleet-oom.spec.ts › a fleet that outgrows its cap restarts with a tenant-visible event #91, [flake] hostile.spec.ts › hostile tenant is contained #92): the ensureDb shared setup pass was never registered with the worker lifetime. celld drops a request's pending work when that request answers or aborts, so waiters sat out 5 s generations. Assets go through 03-db.ts too, because of run_worker_first.
• Fix: waitUntil registration, a single shared replacement pass, a lifetime-less waiter that fails fast, the admin-role short-circuit, failures evicted instead of a timed retry, and a 5 s session-read bound that throws (never logs the caller out over a stall). There's also a check-worker-rules tripwire. Fail-before tests, an independent security review with no blockers, and CI on 58ddbf9 all green, including the previously failing single shard 2.

Residual, owned upstream: the celld D1 store going unresponsive for ~14 s after client-aborted reads is not observable from the lane artifacts. With this PR a recurrence fails at 5 s with cause instead of a 15 s 504. Tracked as a follow-up. Labelled security-reviewed; auto-merge on.

Fixes #91. Fixes #92.