Keep blocking work off the event loop - #729
Open
fangge518 wants to merge 109 commits into
Open
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
2 tasks
fangge518
marked this pull request as draft
August 24, 2026 21:44
fangge518
marked this pull request as ready for review
August 26, 2026 18:55
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
This was referenced Aug 27, 2026
Claude Code only scans .claude/skills/<name>/SKILL.md one level deep, so the 12 project skills synced to .claude/skills/project/<name>/ have been invisible to native skill routing since they were created — only the three un-nested skills (next-best-practices, next-upgrade, web-design-guidelines) ever loaded. sync_claude() now strips source-tree grouping and writes flat, with a duplicate-name guard. Verified live: all 12 learn-* skills appear in the session skill listing immediately after the flat sync. Also fixes sync_cursor()'s reference-inlining dir check to references/ (plural, the convention bundled reference files actually use) — the singular form matched nothing.
Synthesizes Anthropic's official skill docs (Claude Code reference, Agent Skills engineering post, platform best-practices guide, all read 2026-08-27) with this repo's own principles: skills individuated by concern rather than file surface, declarative bodies over DO/DON'T lists for knowledge skills, official procedural patterns (checklists, feedback loops, solve-don't-defer scripts) for runbooks. Divergences from official examples are stated with grounds. Governs the skill rewrites that follow.
Wraps pre-commit/ts/build/pytest into one script that scopes the expensive gates to whichever side of the repo the diff touches, and runs build+pytest concurrently since they live in separate containers with no contention to serialize for.
Merges the two skills into one concern (server endpoints + data model) since AGENTS.md's own selection rule always fires them together. Preserves the shattered-parent contract and UDF policy as grounded, dated content; moves DB/migration/UDF depth into references/db-patterns.md, which now also carries the ParentChildEdges departition/drop/revert history (PR #625, PR #721, and the reverted 2026-07-17 drop attempt) mined from git and PR history.
Documents the actual seven-stage CLI sequence (backend + pipelines) to take a GeoPackage to a working, shattered, contiguity-checked map module, with a verification step after each stage. No script: the sequence requires judgment calls (shatterable vs not, batch vs one-off) that a mechanical wrapper would hide rather than solve.
…orkers into learn-map-frontend These four skills always co-fired on the same concern (how the interactive map renders and responds) per AGENTS.md's guide-selection rules — evidence the split was per-surface, not per-concern. Rewritten per AUTHORING.md: grounded invariants with their why, a territory map, causal history mined from PRs #492, #634/07af68f6, and #550, and sub-topic depth pushed into references/ (layers-and-styling, runtime-events, workers).
…migrations Procedure is generate -> review for dangerous ops (drops, large-table type changes, anything touching the partitioned parentchildedges table, lock- taking rewrites) -> fix downgrade -> upgrade/downgrade/upgrade loop. Uses commit 291cc45 as a worked example of the review habits stated in prose.
…erence Declarative rewrite grounded in fetchDocument.ts, useDocumentWithSync, idb.ts, and the four resolveConflict resolvers in assignmentsStore.ts — works through the updated_at/clientLastUpdated comparison that decides conflict vs. no-conflict and mirrors the server-side 409 check in update_assignments (learn-backend). Adds references/debugging.md (absorbing issue #525's Sync Conflict Debugger): inspecting IDB state, reading the three relevant timestamps, reproducing a conflict on purpose, and replaying each resolution path.
Mines PR #721 (graph-representation refactor, measured benchmarks), PR #540 (graph LRU cache), PR #625 (departitioning under lock contention), and PR #550 (moving unassigned-geometry computation off PostGIS onto a cached graph) into a dated, PR-anchored causal history — the concern spans backend, workers, and infra with no dedicated surface, matching AUTHORING.md's cross-cutting-concern case. Includes EXPLAIN ANALYZE guidance for query-cost triage, kept as method only since this worker cannot run it against a live DB.
…gis-validation and export-testing references One concern — how a map module comes to exist — split across a backend lifecycle skill and a pipelines skill that never fired together despite covering adjacent halves of the same pipeline-to-DB handoff. Rewritten per AUTHORING.md: the onboarding order is grounded in load_data.py's own enforcing docstring, and the pipeline/backend graph-generation split is grounded in transforms/graph.py's module docstring. Adds gis-validation.md and export-testing.md (issue #525), each written against real code paths (EPSG:4326 handling, the `path` column contract, ST_CoverageUnion topology requirements, the four DocumentExportType formats) rather than generic guidance.
Small and hard-rule-dense by design (AUTHORING.md: security is the legitimate home for invariants). Each invariant carries its ground in the actual code: VerifyToken's scope check, the shared-secret/different-aud distinction between session and share JWTs, the two separate Turnstile secrets, and the password-protected edit-access grant in save_share/main.py.
…and FE types audit_contracts.py does name-matched field-set diffing between backend Pydantic/SQLModel classes (via ast, chasing local base-class inheritance) and app/src/app/utils/api/apiHandlers/types.ts (via a brace-depth-aware regex parse). Static analysis only -- no import or execution of repo code. v1 heuristic; limitations are printed with every run.
Thin by design: delegates correctness/simplification/security review to /code-review and security-review, and adds only the repo-specific checks (concern-skill compliance, migration safety, contract drift, gate status, auth surface) that those built-ins have no way to know about.
Grounds the draft/published-column publish model (not a status enum), the TipTap node round-trip pattern via BoilerplateNode as a worked example (attrs + parseHTML/renderHTML + SSR-guarded NodeView), and the moderation-score-vs-review-status relationship in the actual code.
Bun's built-in `bun outdated`/`bun audit` for the frontend; `pip list --outdated` (container) and `uvx pip-audit -r requirements.txt` (host, no env needed) for the backend's uv-compiled lockfile. Survey-and-triage only -- findings get filed, not auto-upgraded.
… reference Quality-gate commands move out (a separate runbook owns them now); memory/lock history moves to learn-performance, cross-referenced rather than duplicated. Adds references/troubleshooting.md (absorbing issue #525's Docker Environment Doctor) as causal symptom -> root cause -> fix entries, each checked against the current docker-compose.yml rather than carried over unverified from the old skill's failure-mode list. Notes the CI/local PostGIS version mismatch (16-3.5 in test-backend.yml vs 15-3.3 in compose) found while verifying.
AGENTS.md's guide list and selection rules described the retired 12-skill surface-sliced layout; replaced with the 14-skill inventory (8 concern-individuated knowledge skills, 6 runbooks) and a pointer to AUTHORING.md, noting that Claude Code routes by description while the listing serves humans and non-routing agents. CLAUDE.md's count line updated to match. audit_contracts.py's first live run paired every suffix-stripped name collision (Assignments x AssignmentsCreate x AssignmentsCreateResponse), reporting 55 pairs of mostly cross-product noise. Exact-name matches now pair first; suffix-stripped matching only resolves leftovers with a single candidate per side. Re-run yields 18 pairs, all same-name, with findings that read as genuine drift.
The original framed get_document vs get_protected_document as the choice governing response safety — wrong axis. get_document gates the write path (requires the true UUID) and never enters a response-shape decision at all. The actual choice a route serving public_id callers faces is get_protected_document (guard-only, never return) vs get_document_public (the function that safely builds a response including the document). Caught in review; rewritten to name the real contrast.
The prior wording ("this, not get_document, is the choice...",
"a different axis entirely... never enters this choice at all")
was written as a rebuttal to a wrong prior draft, presupposing context
a cold reader never has. Restated as a flat, positive description of
what get_protected_document and get_document_public each do — no
reference to a rejected alternative framing.
The prior text framed the split as a hot-path/write-frequency
optimization ("assignments are the hot path... while document
metadata changes rarely") — doesn't hold up: metadata is small enough
that including it in assignment writes would add negligible overhead.
The real reason is cardinality: a document has one metadata row but
many assignment rows (one per geo_id, potentially tens of thousands),
which has no single-row representation on Document regardless of
write frequency. Restated as a flat structural fact.
…able paragraph Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s imitation/structure-carried (per Ge) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… troubleshooting and duplicate close protocol cut Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cating the commands Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ew as newcomer-oriented The counts drift; AUTHORING.md sits in the directory a skill-editing session already opens (its own choke point). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ap is newcomer orientation, not agent instruction Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssion start; the lists were drifting paraphrases Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in the script's --help, mechanics in AUTHORING Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…md (base on dev) and AUTHORING (canonical source rule); everything else duplicated the eager file or the skills Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s now purely the skills source; fix stale Fly.io section, decisions section defers to decisions.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…isible, self-describing source dir (per Ge) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…metrics removal + this branch's WAF and access-logs rows); graph.py references updated to graph_loader.py
Convert 33 async route handlers to plain def so FastAPI runs them in its anyio threadpool (80 threads) instead of on the event loop. The event loop was serialized by sync SQLAlchemy/boto3 calls inside async handlers; a single slow request could stall all concurrent requests on the same ECS task, including ALB health checks. Three parts: 1. Handler conversion. Audit across main.py, cms/main.py, comments/main.py, exports/main.py, save_share/main.py, thumbnails/main.py. Plain def for all DB-only bodies. Genuine-await handlers (Turnstile, request.body(), graph threadpool calls) stay async; sync DB portions move behind run_in_threadpool. update_assignments extracts _sync_update_assignments so request.body() stays awaited while the 300-line DB body runs in the threadpool. Three graph-loading handlers (get_unassigned_geoids, check_document_contiguity, get_connected_component_bboxes) untouched — PR #721 replaces that subsystem wholesale. 2. VerifyToken.verify → sync def (core/security.py). JWKS fetch + jwt.decode were blocking the event loop on every authenticated request; as a sync FastAPI dependency it now runs in the threadpool automatically. 3. Capacity check: threadpool 80 threads vs. pool 60 connections per task (40 base + 20 overflow). The 20-thread headroom queues at pool-acquire rather than spawning unbounded connections — correct, no config change. db.py comment arithmetic ("in step with the limiter") remains accurate. Convention comment added at the route-handler block documenting the rule for future endpoints. Supersedes PR #556 (120 commits behind dev, 15 conflict hunks; six of its change groups audited — only three survived: this conversion, VerifyToken, and comments threadpool). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
get_unassigned_geoids, check_document_contiguity, get_connected_component_bboxes: async def → def; get_graph(...) called directly instead of via run_in_threadpool. get_graph is a plain sync function — the prior await was not a genuine async operation, just a bridge pattern that is redundant once the handler is def. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nments extraction - Snapshot comment: explains why we capture pre-existing assignments (selective district_unions eviction, not full cache wipe) - NULL-zone comment: explains why zone IS NULL is always included in the DELETE (unassigned row totals depend on all assigned zones) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Records why graph handlers are def (not async def + run_in_threadpool), why Turnstile handlers stay async def, threadpool sizing rationale, measured evidence, and the misleading third-party skill anti-pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…segments inside async handlers stay in run_in_threadpool
fangge518
force-pushed
the
fix/async-blocking-cleanup
branch
from
September 4, 2026 14:45
c4de985 to
2ba17df
Compare
… def, add dev stress-test runbook - Restore the five invariant comment blocks dropped by the _sync_update_assignments extraction (no-op updated_at rationale, full-replacement-set semantics, valid_community_ids bootstrap path, comments None-vs-[] semantics, assignments_updated_at staleness contract) - Convert root() to def, matching the convention comment above it - stress_test/README.md: "Dev stack run" section — env overrides, capacity sizing, seed-slug precheck
…dpool sites inside def handlers The rebase resolved textual conflicts only; #740's base (post-#721 dev) carried graph pre-warm/load awaits inside handlers this branch had converted to def — a SyntaxError caught by pytest. All three become direct get_graph calls; the handlers already run in the threadpool. Full suite: 531 passed.
…pushed, CI-tested SHA Guards: clean backend/ worktree, local tip == remote tip (ls-remote), and a completed test-backend.yml run for that exact SHA (one gh call inside the script — no extra agent round-trips). CI success skips the ~4-min local run; CI failure still runs locally for a fresh log and prints a pointer to the CI record; any doubt falls through to a normal local run.
# Conflicts: # AGENTS.md # agent-skills/AUTHORING.md # agent-skills/backend-endpoints/SKILL.md # agent-skills/deploy-authority/SKILL.md # agent-skills/map-edit-sync/SKILL.md # agent-skills/map-edit-sync/references/debugging.md # agent-skills/map-rendering/SKILL.md # agent-skills/performance-memory/SKILL.md # agent-skills/run-api-contract-audit/SKILL.md # agent-skills/run-dependency-audit/SKILL.md # agent-skills/run-map-onboarding/SKILL.md # agent-skills/run-map-onboarding/references/export-testing.md # agent-skills/run-map-onboarding/references/gis-validation.md # agent-skills/run-migration/SKILL.md # agent-skills/run-quality-gate/SKILL.md # docs/architecture.md # docs/decisions.md # docs/overview.md
One Nygard-format record per decision (Date/Status/Context/Decision/ Consequences), numbered in decision order, with the customary 0001 meta-ADR and an index README. Content preserved from decisions.md; the two undated design entries are marked retrospective. All references updated to point at docs/adr/ (specific records where the referent was specific).
…decisions.md dispatch entry (both land in the ADR-adoption branch)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every
async defroute handler that does blocking SQLAlchemy or JWKS work runs that work on the event loop. On AWS, each ECS task is a single uvicorn process with one event loop — one slow request stalls all concurrent requests on the same task, including ALB health checks. The anyio threadpool (80 threads, set in the lifespan) and connection pool (60/task,core/db.py) were sized in PR #623 for exactly this conversion; this is the code-level half of that fix.Fix: Convert 38
async defroute handlers to plaindefacrossmain.py,cms/main.py,comments/main.py,exports/main.py,save_share/main.py,thumbnails/main.py— FastAPI runs plain-def handlers in the anyio threadpool. Handlers with genuine awaits (Turnstile verification,request.body()) stayasync; their sync DB portions now run throughrun_in_threadpool.update_assignmentsextracts a_sync_update_assignmentshelper sorequest.body()keeps its await while the 300-line DB body runs off the loop. The three graph handlers (get_unassigned_geoids,check_document_contiguity,get_connected_component_bboxes) are converted todefand callget_graph(...)directly —get_graphis a plain sync function (S3 fetch + unpickle); the priorawait run_in_threadpool(get_graph, ...)was scaffolding around blocking code, not a genuine async operation.VerifyToken.verifyconverts to syncdef(core/security.py) — JWKS fetch andjwt.decodenow run in the threadpool as a FastAPI dependency. A convention comment at the top of the handler block documents the rule for future endpoints, and the dispatch decision (with the fullasync defendpoint inventory) is recorded indocs/decisions.md. Review follow-ups: the invariant comments dropped by the_sync_update_assignmentsextraction are restored, and the two zero-work handlers the review flagged (root,/_debug/cache) are converted todeffor consistency with the convention.Capacity: threadpool 80 threads vs. pool 60 connections — the 20-thread headroom queues at pool-acquire, a natural backpressure mechanism; the
db.pycomment arithmetic stays accurate.Supersedes PR #556 ("API Blocking work async cleanup," June 10). That branch was 120 commits behind and had 15 conflict hunks; of its six change groups, three survived (this handler conversion,
VerifyToken.verify, and the comments Turnstile threading). The rest was already fixed (share-endpoint password prints, PR #619), absorbed (graph call sites), or obsolete (fly.toml/WEB_CONCURRENCY— AWS runs single-process uvicorn per ECS task, not Fly's multi-process model).Tested
feat/agent-skills-rebuildbase (was 337 passed / 117 skipped on thedevbase; the graph PR's tests joined the suite). The run also caught three rebase semantic conflicts —await run_in_threadpoolsites the base added inside handlers this branch had converted todef— fixed as directget_graphcalls.GET /,GET /db_is_alive,GET /api/gerrydb/views,GET /api/documents/list,GET /api/cms/content/page/list,GET /api/cms/site_settings— all respond correctly after conversionGET /db_is_alive) stalled 63s ondevand held 3–11 ms on this branchDev stress-test plan
Paired runs of the existing Locust harness (
backend/stress_test/) against the dev stack, comparing dev as deployed today vs. dev with this PR (and PR #740's base) merged:SCALE=0.25(~3,200 users, sized to dev's 1–2 task /db.t4g.smallcapacity),BASE_URL=https://api.dev.districtr.org.SCALE, window, and RNG seed; compare per-route p50/p95/failure-rate CSVs, expecting the largest deltas on mixed traffic (slow geometry work concurrent with fast reads).stress-test-cleanup.The harness already parameterizes everything needed (
STACK_PREFIX,CLUSTER,BASE_URL,RESULTS_BUCKET); this PR adds a "Dev stack run" section tostress_test/README.mddocumenting the overrides, the capacity sizing, and the seed-slug precheck against the dev DB.