feat(dispatch): add runner registry foundation (pull-dispatch slice 1/6) - #341
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.
取徑（沒有照原建議用 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)
…ting (ADR-0005) RUNNER_REGISTRATION_TOKEN moves from a live env accessor in dispatch/config.py to a field on the centralized Settings (top-level config.py, #345), loaded and validated once at startup. Unset/empty still means registration disabled (fail closed); rotating the shared secret now takes a restart, per-runner revocation stays immediate. dispatch/config.py is renamed to dispatch/params.py and now holds only the spec §13 protocol parameters — the dispatch module no longer reads settings of its own.
| # Compare UTF-8 bytes: compare_digest accepts str only when both sides are | ||
| # ASCII ("str (ASCII only)", hmac docs) and raises TypeError otherwise; | ||
| # `candidate` is attacker-controlled, so str comparison could crash (500) | ||
| # instead of failing closed (401). | ||
| return hmac.compare_digest(expected.encode(), candidate.encode()) |
There was a problem hiding this comment.
Claude: 確認屬實，json.loads 真的會吐 lone surrogate 的 str。f431770 修了：先過 fail-closed 的 _utf8 helper，編不過就直接 False，不會再 500。測試 test_verify_registration_token_lone_surrogate_candidate_rejected。
| stored = _redis().get(redis_keys.runner_token_hash(runner_id)) | ||
| if stored is None: | ||
| return False | ||
| stored_hex = stored.decode() | ||
| return hmac.compare_digest(stored_hex, _token_hash(token)) |
There was a problem hiding this comment.
Claude: f431770 修了：runner_id 和 token 都先過 _utf8 再碰 Redis（runner_id 進 redis-py 也會被 UTF-8 encode，一樣會炸），stored 改成 raw bytes 比較，不再假設它能 UTF-8 decode。測試 test_verify_token_lone_surrogate_inputs_rejected。
|
codex: [P2] hostile JSON string 會讓 registration auth failure 變成 500，而不是 spec 要求的 401 / fail closed。 verify_registration_token() 在確認 candidate 是 str 後直接呼叫 candidate.encode()。Python JSON parser 會接受 lone surrogate，例如 "\ud800"，並解析成 str；但接著 UTF-8 encoding 會拋出 UnicodeEncodeError。因此未驗證的 request 可以讓 endpoint 回 500。 建議捕捉 UnicodeEncodeError 並回傳 False，另加入 lone-surrogate regression test。已在目前 PR head 887c99f 重現。 |
json.loads accepts JSON strings holding lone UTF-16 surrogates, and .encode() on such a str raises UnicodeEncodeError — a hostile register/auth body could turn into a 500 instead of a 401. Encode both comparison sides through a fail-closed _utf8 helper, reject unencodable runner_id/token before touching Redis (redis-py UTF-8 encodes keys), and compare the stored token hash as raw bytes instead of assuming it decodes as UTF-8.
| runners: List[Dict] = [] | ||
| for member, score in members: | ||
| runner_id = member.decode() if isinstance(member, bytes) else member | ||
| raw_meta = client.hgetall(redis_keys.runner_meta(runner_id)) | ||
| meta = { | ||
| (k.decode() if isinstance(k, bytes) else k): | ||
| (v.decode() if isinstance(v, bytes) else v) | ||
| for k, v in raw_meta.items() | ||
| } | ||
| runners.append({ | ||
| 'runner_id': runner_id, | ||
| 'name': meta.get('name'), | ||
| 'last_seen': score, | ||
| 'registered_at': meta.get('registered_at'), | ||
| }) | ||
| return runners |
There was a problem hiding this comment.
Claude: 05d1dad pipeline 化了，meta 一次全撈（transaction=False，讀而已）。cab1efe 另補一個 multi-runner 測試，釘住 zip 位置配對的不變量。
… GC pipelines list_runners fetched each runner's meta hash in its own round trip (N+1); batch them in one non-transactional pipeline. GC's exists probe and sweep need no atomicity (TTL monotonicity, see module docstring), so skip MULTI/EXEC there too. register keeps its transaction: identity creation is all-or-nothing on purpose.
Closes Normal-OJ/Normal-OJ#61
Slice 1/6 of the pull-based sandbox runner rework. Dark PR — no callers; outside
dispatch/and its test module, the only production change is one new Settings field inconfig.py.Spec carriers:
docs/specs/pull-based-job-dispatch.md§5/§7.1/§8/§13,docs/adr/0004-ephemeral-runner-identity.md,docs/adr/0005-registration-token-startup-snapshot.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/params.py— §13 protocol constants only; deployment settings never live indispatch/(ADR-0005)config.py— newRUNNER_REGISTRATION_TOKENfield on the centralized Settings (refactor: centralize deployment settings in top-level config.py #345): startup snapshot per ADR-0005, unset/empty ⇒ registration disabled (fail closed); rotating the shared secret takes a restart, per-runner revocation stays immediatedispatch/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 raw bytes; non-ASCII, non-str, and lone-surrogate hostile inputs fail closed (regression-tested) instead of raisingrunner:<rn>:token_hash→ immediate 401 (ADR-0004)Tests
29 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, revoked-runner observability window.tests/test_config.pycovers the new Settings field. Full suite locally (post-merge with #333 FastAPI + #345 centralized config): 538 passed, 9 skipped, 1 xfailed.