Hacktoberfest 2026: những issue maintainer đã đánh dấu cho tháng Mười, đang mở và phù hợp người mới. Xem issue Hacktoberfest

fix(leaderboard): bulk recompute has no concurrency guard, overlapping all=true runs race on the KV checkpoint

Đang mở
  • Có gì không ổn?
#26 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 1 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
48/100
Loại issue
Lỗi
Độ rõ ràng
Khá rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
typescript
Lĩnh vực
api, backend, distributed-systems

Hướng nghiên cứu

Read apps/web/app/routes/api.admin.recompute.ts and apps/web/app/lib/leaderboard/recompute-all-skills.ts, starting at the cited action, state load, and state write paths. Use the overlapping curl reproduction to verify that a second active run is rejected or coordinated atomically, and that the KV checkpoint remains consistent.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

bug

Summary

POST /api/admin/recompute?all=true has no concurrency guard, so two simultaneous runs share one KV checkpoint. Both requests read and write the same state, offset can be advanced or reset by whichever write lands last, and every skill in the overlap window is scored twice or skipped. Both callers still get HTTP 200.

Current behavior

  • apps/web/app/routes/api.admin.recompute.ts:51 — action() verifies the admin secret and starts batching immediately. There is no in-flight check and no 409 response path anywhere in the file.
  • apps/web/app/lib/leaderboard/recompute-all-skills.ts:45-46 — KV state is read only when resume is true, and only to choose a starting offset: 
    savedState = await loadState(kv);
offset = savedState?.status === "running" ? savedState.offset : 0;
  • apps/web/app/lib/leaderboard/recompute-all-skills.ts:191-194 — every batch then overwrites or clears that same state, with no compare-and-set: 
    if (isComplete) { await clearState(kv); } else { await saveState(kv, state); }
  • apps/web/app/routes/api.admin.recompute.ts:21 caps one request at MAX_BATCHES_PER_REQUEST = 50, so a single call can run for many seconds — long enough for a second call to overlap. batchCount is per-request only, so it never coordinates across requests.

Expected behavior

A second run is rejected while a run is active — for example 409 Conflict when status === "running" and lastBatchAt is recent — or the runs coordinate through an atomic lock so the checkpoint advances monotonically.

Reproduction

  1. Send two overlapping requests with the admin secret in the header: 
    ADMIN_SECRET=... curl -sS -X POST "https://skillx.sh/api/admin/recompute?all=true&batch=100" &
ADMIN_SECRET=... curl -sS -X POST "https://skillx.sh/api/admin/recompute?all=true&batch=100" &
wait
  2. Watch the returned nextOffset and state from each response, then GET /api/admin/recompute to read the surviving checkpoint.
  3. Interleaved responses show both runs advancing the same checkpoint.

Conditions: requires ADMIN_SECRET plus the D1 and KV bindings; admin-only endpoint. The code paths above are read directly from main at 9d0f9e9 — no live concurrent run against production was performed for this report.

Flow

flowchart LR
  A[POST all=true] --> B[verifyAdmin]
  B --> C[recomputeBatch: load KV offset]
  D[POST all=true] --> E[verifyAdmin]
  E --> F[recomputeBatch: load KV offset]
  C --> G{{both write same KV checkpoint}}
  F --> G
  G --> H[offset / totalProcessed corrupted]

Evidence

  • Affected code: apps/web/app/routes/api.admin.recompute.ts:51, apps/web/app/routes/api.admin.recompute.ts:21, apps/web/app/lib/leaderboard/recompute-all-skills.ts:45, apps/web/app/lib/leaderboard/recompute-all-skills.ts:191
  • Introduced by: 88a5bff ("feat: add bulk recompute admin endpoint for leaderboard scores"), shipped on main via 9d0f9e9 (PR #9)
  • Related: plans/reports/code-reviewer-260306-1531-admin-recompute-endpoint.md, section "4. No concurrency guard"

Open questions

  • Preferred mechanism: a KV read-check-write optimistic lock, a Durable Object, or simply rejecting when lastBatchAt is recent (under 5 minutes)?
  • Is this reachable in practice? scripts/run-recompute.sh sleeps DELAY seconds between batches but holds no lock, and it is currently untracked — see the runbook tooling issue.

Notes

  • The other findings from the same review are already fixed on main: the date/millisecond bug, the N+1 aggregation, the all=true batch guard, the secret-in-query-param auth, and the redundant ternary.
  • A KV read-check-write is not atomic on its own, so if that route is chosen the check must tolerate the small race window (accepting a rare double-reject) rather than assume a real lock.
Ngôn ngữ chính
TypeScript
Star
190
Fork
42
Merge trung bình
5 giờ 50 phút
Pull request đã merge (30 ngày)
37

Chuẩn bị môi trường

Dự án này không cung cấp dev container, Dockerfile hay hướng dẫn đóng góp, nên bạn cần tự thiết lập môi trường: hãy bắt đầu từ README và xem hướng dẫn đóng góp lần đầu của chúng tôi để biết các bước chung.

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của nextlevelbuilder/skillx

Tất cả issue của nextlevelbuilder/skillx

Issue tương tự

Thêm issue về TypeScript

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.