feat(dispatch): add runner registry foundation (pull-dispatch slice 1/6)#341
feat(dispatch): add runner registry foundation (pull-dispatch slice 1/6)#341as535364 wants to merge 5 commits into
Conversation
Slice 1/6 of the pull-based sandbox runner rework (Normal-OJ/Normal-OJ#61). Dark PR: no callers yet. - dispatch/redis_keys.py: centralized spec-§8 key namespace - dispatch/config.py: spec-§13 parameters + fail-closed registration token accessor - dispatch/runner.py: register (rn_/rk_, SHA-256 hash only), constant-time verify_token / verify_registration_token, lazy 7d identity GC, list_runners - tests: 19 fakeredis unit tests incl. revocation and non-ASCII auth inputs
verify_token and verify_registration_token now reject non-str inputs (bytes/int/list/dict) with False instead of raising AttributeError on .encode(). JSON request bodies can legally carry non-str values, so the auth boundary must fail closed (401) rather than crash (500). Add regression tests covering bytes/int/list/dict for both functions.
… evaporated members External review on PR #341 found a TOCTOU in _gc(): it did zrangebyscore then unconditionally deleted meta/token_hash/alive + zrem. A heartbeat (or clock skew) landing between the scan and the delete could get its just-renewed token_hash deleted, turning a live runner into a 401. Close the race structurally instead of with atomicity machinery: TTL is now the ONLY thing that invalidates a live identity. _gc() keeps the >7d score prefilter, then EXISTS-checks each candidate's token_hash and skips any that still exists (its TTL has not fired). Only members whose token_hash already evaporated are swept (zrem + delete meta/alive). Safe without locks because a token_hash can never reappear for the same rn_id: register always mints a fresh ULID and heartbeat requires token auth. Adapt GC tests to simulate the token_hash TTL firing by deleting the key (fakeredis TTLs use real wall-clock, not the monkeypatched _now), and add a regression test asserting a stale-by-score member with a surviving token_hash is spared and still verifies.
|
外部 review 發現 取徑（沒有照原建議用 Lua，而是消除整類競態）：GC 語意改為「TTL 是唯一使活身分失效的機制，GC 只收屍」——score 預篩後，逐一檢查 安全論證：token_hash 一旦消失就不可能為同一 rn_id 重現——register 永遠發新 ULID，heartbeat 續期需先通過 token 驗證。已由獨立驗證以注入探測確認（在 EXISTS 檢查後、刪除前強制重建 token_hash，仍不會被刪）。新增回歸測試 Spec 同步（meta-repo）：§7.1 GC 措辭已更新；§7.2 新增約束「heartbeat 續 TTL 一律 EXPIRE、不得重建已蒸發的 key」——這是未來 heartbeat 切片（HTTP 層）review 時需要盯的不變量。 |
There was a problem hiding this comment.
Pull request overview
Introduces the first slice of the pull-based sandbox runner rework by adding a foundational runner identity/registration layer under dispatch/, including Redis key namespacing, configuration parameters, and a test suite validating token behavior and lazy GC semantics.
Changes:
- Added
dispatch/runner.pyto register runners, verify runner tokens/registration token, list runners, and lazily GC expired identities. - Added
dispatch/redis_keys.pyanddispatch/config.pyto centralize Redis key naming and dispatch parameters / registration-token access. - Added
tests/test_dispatch_runner.pywith fakeredis unit tests for registration, token verification, and GC behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
dispatch/runner.py |
Implements runner identity registration, verification, listing, and lazy GC over Redis. |
dispatch/redis_keys.py |
Centralizes Redis key naming for identity and (future) job dispatch. |
dispatch/config.py |
Defines pull-dispatch timing/TTL parameters and a live env-backed registration token accessor. |
dispatch/__init__.py |
Adds module docstring describing the slice scope. |
tests/test_dispatch_runner.py |
Adds fakeredis tests covering identity registration, verification, and GC. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on #341: - list_runners docstring no longer claims a liveness view; revoked/dead identities stay listed until GC as a deliberate observability window (revocation only guarantees immediate auth failure, ADR-0004) — pinned by test_revoked_runner_stays_listed_until_gc - compare_digest comments now cite the hmac docs' 'str (ASCII only)' contract (same-type non-ASCII str raises TypeError; empirically checked)
There was a problem hiding this comment.
是不是把它合併到 https://github.com/Normal-OJ/Back-End/blob/main/mongo/config.py 比較好？雖然我們目前 config 還挺亂的，或是等 FastAPI 那 pr 合併會更好 🤔
There was a problem hiding this comment.
這樣放在 mongo 底下很怪，然後 fastapi 躺有點久了
如果要改 config 位子也是可以在這個 PR 直接處理完？
There was a problem hiding this comment.
我倒是覺得 #333 幾乎可以 merge 了，剩下一點 format 問題跟或許可以整理一下 commit history，當初沒動是因為我想說學期間不要有這麼大的改動比較好
There was a problem hiding this comment.
那我就把 config 統一拉到 root folder 囉？
| def verify_registration_token(candidate: Optional[str]) -> bool: | ||
| """Constant-time check of a register request's shared secret. Fails closed.""" | ||
| expected = config.registration_token() | ||
| # Fail closed: no configured secret ⇒ registration is disabled, not open. | ||
| # Reject non-str candidates too — a JSON body can carry ints/lists/bytes, | ||
| # and .encode() below would otherwise raise instead of returning False. | ||
| if not expected or not isinstance(candidate, str) or not candidate: | ||
| return False |
There was a problem hiding this comment.
nit: 這是不是應該在 request body 就 parse 好？不應該讓非 str 的資料進來。然後 config 那段應該是 app init 階段就要驗證
There was a problem hiding this comment.
expected = config.registration_token()，我不大確定這邊是否該允許空值
There was a problem hiding this comment.
hmm 有道理，但空值應該要檢查一下
然後把 parse 在 request body 處理好就好？
Closes Normal-OJ/Normal-OJ#61
Slice 1/6 of the pull-based sandbox runner rework. Dark PR — no callers; nothing outside
dispatch/and its test module changes.Spec carriers:
docs/specs/pull-based-job-dispatch.md§5/§7.1/§8/§13,docs/adr/0004-ephemeral-runner-identity.md(meta-repo).What
dispatch/redis_keys.py— centralized §8 key namespace (job keys pre-declared for slices 2–3, unused here by design)dispatch/config.py— §13 parameters + fail-closedRUNNER_REGISTRATION_TOKENaccessor (read live, unset/empty ⇒ registration disabled)dispatch/runner.py— identity layer:register(rn_+ULID / rk_+token_urlsafe(32), stores SHA-256 hex only), constant-timeverify_token/verify_registration_token, lazy 7d identity GC on register/list,list_runnersSecurity notes (auth slice — please review with a security lens)
hmac.compare_digeston UTF-8 bytes; non-ASCII and non-str hostile inputs fail closed (regression-tested) instead of raisingrunner:<rn>:token_hash→ immediate 401 (ADR-0004)Tests
27 fakeredis unit tests (
tests/test_dispatch_runner.py): registration TTLs/ZSET, token verify incl. cross-runner & revocation, GC boundary at exactly 7d, hostile-input matrix. Full suite locally: 523 passed, 7 skipped, 1 xfailed.