fix(quota): charge what the backend really caps, and name every limit that refused - #712
Open
HYChou0515 wants to merge 8 commits into
Open
fix(quota): charge what the backend really caps, and name every limit that refused#712HYChou0515 wants to merge 8 commits into
HYChou0515 wants to merge 8 commits into
Conversation
`/my-resources` reported "CPU 0" next to a live environment, and a per-person cpu/memory cap could never bind. The tally read the SPEC. `None` there means "not stated — backend, apply your own ceiling", and every real backend does: a cgroup at `SANDBOX_HOST_CPU_CORES` / `sandbox.isolation.*`. So an App that declared nothing held a core for free, and `_SUMMED_DIMENSIONS` summed zeros. The number now comes from the side that applies it, which is the same rule the workspace quota already follows (the sandbox measures, the facade enforces): - `Sandbox.effective_limits(spec)` — each `None` replaced by what this backend will really enforce. A backend that caps nothing (mock, plain local process) returns `None`s and nothing is charged, which is the honest answer rather than an invented one. - The isolated backend answers from its own `_CgroupManager`, so the number charged is the number written to `cpu.max` — one rule, not a second copy. - The http backend cannot read another service's environment, so the HOST advertises what it applies on `/healthz`, and the client caches it. A host too old to say leaves the request's `None`s in place — today's behaviour, a visible under-count rather than a guess — and `resource-defaults` joins `REQUIRED_CAPABILITIES` so the staleness probe names the stale image. Also: the admin override form picks its target from the company directory (`UserPicker`, the one share/@mention already use) instead of taking a typed id. A mistyped id saved perfectly well, showed up in the exceptions list, and bound to nobody. Deploying this needs sandbox-host rebuilt and rolled out; until then the app charges what it charges today.
The wire doc listed `/healthz` as `{status, version, capabilities}` and §6.5
explained the LIMIT without ever saying how the USAGE beside it is measured —
which is the half that was wrong, and the half whose answer depends on which
sandbox backend is deployed.
`ruff format` at the repo root does not reach `sandbox-host/` — it is a separate package with its own config, and CI lints it as its own job. Same shape as the miss on #709: I verified with a subset of what CI actually runs.
Two rules, written twice, reporting one answer. The check was the same two lines copied into `chat_send.send` and `workflow_exec.drive_turn`, run in a fixed order — disk, then environments — so only the first to fire was ever reported. Someone both out of disk and at their environment limit is told to delete files, deletes them, sends again, and is told something different. Every message in that sequence is true; the sequence is what reads as a bug, because the first one implied that acting on it would let the turn through. `turn_gate.admit_turn` is that check, once. It runs BOTH gates and raises `TurnRefused` when more than one bound — costing one extra measurement on a path that is already failing, and buying an answer the person can act on in a single pass. The environment leads when both bind: closing one is a single click on a page the refusal links to, while freeing disk means going into the item and choosing what to lose. The WORDING was duplicated the same way — a body literal per exception handler, which is how the 507 phrasing drifted across four entry points once already. The three handlers now share one, and `quota_body` builds the body for both them and the combined refusal, so a fourth entry point cannot invent a fourth spelling. On the wire: `detail.also` lists the other codes. A client that only reads `error` behaves exactly as before; the FE appends one clause per extra limit (`resources.also.*`), and those clauses name `/my-resources` so the refusal is still clickable when the environment limit is the one that did not lead.
HYChou0515
changed the title
…hat catch this Writing the acceptance condition P5 never had — "cpu ALONE refuses, for an App that declares nothing" — found the other half of the same defect, still live after P1: the turn was admitted (202), not refused. `admission.check(item_id)` was called with no `incoming` at the one production call site, so the sandbox about to be opened was weighed as free. `cpu` and `memory` could then only refuse someone ALREADY over their limit, never the "one more would put you over" case that is the whole point. The gate's own tests stayed green throughout because they feed the ledger by hand (`store.bump(cpu_milli=1000)`) — they prove the GATE, not the pipeline that fills it. A green unit test says nothing about whether its input is right. `cost_of=registry.would_cost` makes the gate ask for that number itself, from the same source the ledger charges from, so "does one more fit?" and "what is already held?" cannot answer in different units — and no caller can forget to pass it. A rule that can be forgotten is a suggestion. docs/plan-sandbox-resource-quota.md §3.3 records why the plan passed while all of this was broken: every flow-dimension condition was written with `count`, the demo config declared resources (hiding the undeclared case), "pressable" stood in for "legible", and the operator's view was never in scope. Plus the four rules for writing the next plan's conditions, and the conditions P5/P8 should have had.
Four reviewers (defect / conformance / veracity / regression) ran in parallel. The worst finding was in P1's own fix. **The failure was cached for ever.** `HttpSandbox` asked the host once; when that one call failed it remembered `(None, None)` — so a single `/healthz` blip pinned the pod to "every sandbox is free", which IS the defect P1 exists to fix, now silent and curable only by restarting. The staleness probe stays green throughout: the host is fine, it was this side that gave up. Reproduced. Replaced rather than patched, because the mechanism was the problem: cache only ANSWERS, expire them (the host is a separate deployment — rolling it does not restart the app, so "a redeploy is picked up" needed a TTL to be true), and hold a lock (racing cold callers each wrote the cache, so a failing probe could overwrite an answer the host had already given correctly). Timeout 10s → 3s: it sits in front of a turn, before the message is persisted. **The old rule was still being printed to operators.** P1 moved the criterion from "did an App declare a cost" to "does the BACKEND enforce one", and five places still stated the old one — including a boot line telling operators that `per_user.cpu` "never fires" for exactly the configuration in which P5's new test proves it refuses a turn. Two green tests asserting opposite things about one config. `warn_unenforceable_dimensions` now takes what the backend enforces (a REQUIRED argument — the four call sites that forgot it failed to compile, which is the point) and runs after the sandbox exists. **The production number had no test.** Mutating the host's `_CgroupManager` to return garbage left `sandbox-host` at 202 passed: the one new test fed a `MockSandbox` that echoes whatever constant you hand it. Now asserted against the files `create` actually writes to `cpu.max` / `memory.max`, over every stated/unstated combination — a version of the test a wrong `effective()` cannot pass. Verified by mutation: 2 failed, restored, 205 passed. Also, all found by review and each with a test or probe: - The numbers appended to a refusal contradicted its sentence: "you are at your limit for live environments (4 used of 2)" to someone holding ONE — those were cores. The dimension is named now; a number that argues with its own sentence is worse than none, which was the entire reason for adding numbers. - "Names every limit at its cap" was false for the likeliest pair: both disk rules live behind one facade call and the first short-circuited the second. `room_refusals` returns all of them; `ensure_room_for` still raises the first, which is what a write that is stopping anyway wants. - A per-person cap below ONE sandbox's cost refuses everyone's FIRST environment while telling them to close one they do not have. Named at boot. - `cost_of` was optional, so "no caller can forget" was false at the constructor. Required now. - The fourth spelling of the 507 body already existed (`file_routes`); it goes through `quota_body` too. - `/healthz` could 500 — it is the deployment's livenessProbe, so a backend that cannot answer a resource question must degrade, not crashloop. - `resources.also.environment` was a message the backend can never produce (the environment limit always leads), and `QUOTA_REFUSALS` had no callers. Deleted. - A second `formatBytes` rendered the same number differently from the `lib/bytes` one beside it. Merged back. - The admin field's `<label>` pointed at nothing once the input became a picker. - A headless refusal counted the other limits instead of naming them — and that is the one surface with no frontend to read `also`.
The round-1 fixes were reviewed. Two of them had introduced defects, and the reviewer's sharpest output was that both are ONE shape: production wiring that every test erases with a double. **The boot probe poisoned the production connection pool.** P6 added `asyncio.run(sandbox.effective_limits(...))` in `__main__` to ask the backend what it enforces. For `kind: http` that is a real request: the keep-alive connection it leaves in the client's pool belongs to a loop that is then closed, so the FIRST sandbox call on uvicorn's loop dies with "Event loop is closed" — swallowed into "charge nothing" on the accounting path, and unmapped on every other (`RuntimeError` is neither `TimeoutException` nor `TransportError`, so `_request` gives it no retry and no translation: a 500). Invisible to the whole suite, which injects a client and never leaves one loop. It now runs in the LIFESPAN, beside `prewarm_tools` — the existing precedent for "boot work that touches the sandbox" — on the loop that will serve. **Gathering reasons charged people for writes that never happened.** The per-person disk gate is not a predicate: on the allowed path it records the post-write size. `ensure_room_for` never reached it once the workspace rule had refused; P6's `room_refusals` deliberately keeps going, and so reached a RECORDING gate for an operation already refused. A refused folder copy moved the owner's ledger 150 → 300, and they were then refused in a different item against a number that appears nowhere in the product — the file tree still showed 150. The gate's own comment says a refused write is "deliberately NOT recorded". `record` is part of the contract now; the turn gate asks without charging. Both fixes' tests used a double that only DECIDES (or never touches the network), which is exactly why both defects shipped green. The new tests model the side effect. Also from the same round, in the ten lines P6 replaced: - The lock serialised the FAILURE path — a failure is deliberately not cached, so twenty concurrent callers each paid a full 3s timeout in turn, in front of the pre-persist gate. A failure is now remembered briefly: long enough that no queue forms, short enough that recovery stays bounded. - TTL expiry discarded a known-good answer, so a host that answered and THEN went down charged every sandbox zero — worse than one never asked. A stale ceiling now beats no ceiling. And `_RoomAlways`, the turn-resilience double, fell behind `WorkspaceFiles` twice in this branch. Its signature now mirrors the real one.
Nine sentences in this branch say some version of "this cannot happen again". Six of them survived having their enforcement deleted: everything stayed green. The P6 commit message claimed "each with a test or probe" for four of those. Each now has a test that goes red when the mechanism is removed — verified by deleting it, not by reading: - the lock around the host-defaults fetch (the fake host now yields, because a transport that never suspends made the lock unmeasurable: five racing callers serialised themselves by accident and removing it changed nothing) - both disk rules being named, not just the first (the only `also` test paired the environment limit with a disk one — two different gates; the pair that short-circuited lives inside one method) - a combined refusal NAMING the other limits in its text, which is the whole message a scheduled run records (nothing referenced `TurnRefused` anywhere) - the streamed upload's 507 going through `quota_body` rather than a fifth hand-written body - the admin field having an accessible name - a refusal's numbers being formatted by the same function as the usage bar And the boot warning I claimed to have fixed came back through another door. `HttpSandbox` reports an unreachable host as "enforces nothing" — the same value as a host that genuinely caps nothing — so the `enforced=None` branch I added for "we could not ask" was unreachable from the only production caller, and an unreachable host still printed the false line. The dead branch is gone and the wording now carries the ambiguity instead: it says the check cannot tell the two apart and to look again once the host is up. Not knowing is a thing worth saying; pretending to know is what this whole branch keeps being about. Also: `docs/plan-sandbox-resource-quota.md` still stated the retired rule in two places — one of them inside a hunk P6 itself moved. Marked as overturned rather than quietly deleted, so the reversal is legible.
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
1 participant
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.
額度的 cpu/memory 用量恆為 0,而被拒絕時只會被告知一個上限。修這兩件,以及三輪對抗式 review 從修正本身挖出來的東西。
一、用量(分子)恆為 0
registry._bump_activity用spec.cpu_cores or 0計費。App 沒宣告resources時spec.cpu_cores是None——但None的意思是「後端請用你自己的天花板」,而每個真實後端都真的會套(本機sandbox.isolation.*、正式SANDBOX_HOST_*,都是 1 core / 512M)。所以沒宣告的 App 免費佔著一顆核心,而 per-user 的 cpu/memory 上限加總全是 0、永遠不生效。做法:由套用的那一方回報。 跟這個 repo 既有的磁碟規則同形(the sandbox measures, the facade enforces)。
Sandbox.effective_limits(spec)—— 把請求裡的每個None換成這個後端真正會套的數字。完全不設 cgroup 的後端(mock、純 local process)回報None,就真的不計費。_CgroupManager,所以計費的數字就是寫進cpu.max的那個。/healthz公告(同樣是問 sandbox 自己,不是重讀 settings),client 快取。舊 host 沒有這個欄位就維持今天的行為,並由既有的版本探針指出映像過舊——看得見的少算,不是憑空猜。准入閘門也讀同一個來源:
admission.check過去在唯一的正式呼叫點從來沒傳incoming,所以「即將開的這一個」一律當成免費,cpu/memory 只擋得住已經超標的人——擋不住「再開一個就超標」,而那正是准入控制的全部用途。二、被拒絕時只被告知一個上限
檢查是同樣兩行複製在兩個入口點,順序固定。兩個都滿的人只被告知磁碟,刪完重送才知道還卡在環境數——每則訊息都是真的,是那個順序讀起來像 bug。收斂成
turn_gate.admit_turn:兩個 gate 都跑,detail.also一次講完,只讀error的 client 行為不變。措辭也從四份 body literal 收成一個quota_body。三、訊息帶上數字
「這個項目的工作區空間已滿」對讀的人無法否證——錯的那則跟對的那則長得一模一樣。現在接上「(已用 X,上限 Y)」,而且環境上限會講清楚是哪個維度:它是三個限制共用一句話,直接掛「已用 4,上限 2」會讓只開 1 個環境的人以為自己開了 4 個。
三輪 review 從修正本身挖出來的
每一輪最嚴重的一格:①「修正把原缺陷種回去」→ ②「修正引入正式環境才會爆的缺陷」→ ③「保證沒有測試守著」。
round 1 ——
HttpSandbox把失敗永久快取:一次/healthz抖動就把該 pod 釘在「每個 sandbox 免費」,正是本 PR 要修的缺陷,只是變無聲、且要重啟才好。換掉機制:只快取答案、加 TTL(host 是獨立部署,rolling 它不會重啟 app)、加鎖、逾時 10s→3s。同輪還發現:開機警告仍在對維運講已被推翻的舊規則,而兩條互相矛盾的綠測試各自守著一半;以及正式環境真正算出那個數字的 host 程式碼零測試覆蓋(改成回(999, 1)仍 202 passed)。round 2 —— 我在 round 1 的修正裡種了兩個新缺陷,而且是同一個形狀:正式環境才有的接線,被替身抹掉了。
asyncio.run對 host 發真請求,keep-alive 連線留在池裡、綁在隨即關掉的 loop 上 → 伺服 loop 上第一個 sandbox 動作必爆Event loop is closed;落在計費路徑被吞成「算 0」,落在create/exec則不映射不重試,直接 500。已搬到 lifespan(抄既有的prewarm_tools前例)。record現在是契約的一部分。round 3 —— 我 9 句「這件事不可能再發生」裡,6 句刪掉強制力之後全綠(而我的 commit 訊息寫著「each with a test or probe」)。六條都補上會紅的測試,包括讓假 host 真的讓出 event loop,否則測試根本量不到那個鎖。同輪還發現我宣稱修好的「開機說謊」換一扇門回來:
HttpSandbox把「問不到」和「host 說它什麼都不管」壓成同一個值,所以我加的「沒問到就不警告」分支永遠走不到。改成刪掉死分支 + 讓訊息在 host 不可達時也為真(「這行分不出兩者,等它起來再看一次」)。docs/plan-sandbox-resource-quota.md§3.3 記錄了為什麼原計劃全部打勾之後還有這一堆洞:每條流量維度的驗收條件都只驗了count、驗收用的設定剛好遮住缺陷(demo 的 App 有宣告資源,而 repo 裡四個都沒有)、「按得動」被當成「做完了」、整份計劃只從被拒絕的使用者視角寫。以及下一份計劃的四條寫法。驗證(全部重跑取得,非引用)
tests/api + quota + workflow + sandbox + health + files -m "not integration" -n auto→ 2351 passed / 3 xfailedruff check+ruff format --check(root 的 ruff 不涵蓋sandbox-host/)ty check全過;mkdocs build --strictexit 0vitestexit 0,tsc --noEmit乾淨紅→綠都真的紅過,而且紅的理由就是缺陷本身:
test_a_live_environment_is_charged_what_the_backend_really_caps_it_atassert 0.0 == 2.0test_a_refusal_names_every_limit_that_is_at_its_capworkspace_quota_exceeded,而那人同時卡在環境數test_cpu_alone_refuses_a_turn_when_no_app_declared_anythingassert 202 == 507—— 根本沒擋test_what_the_host_publishes_is_what_it_writes_to_the_cgrouptest_concurrent_first_calls_ask_once_and_a_failure_cannot_winassert 5 == 1要重新 build 並推 sandbox-host,否則正式環境的 http backend 拿不到
defaults,計費維持今天的少算(app 不會壞,/health的探針會說映像過舊)。順手發現,不在本 PR 修
sandbox-host的 CI 型別閘 從來沒有檢查過任何東西:uv run ty check src tests(以及./src ./tests、src/sandbox_host)都印WARN No python files found under the given path(s)然後All checks passed!。既有問題,不是本 branch 造成;不帶參數則吐 1215 個診斷,代表設定本身要修——那是獨立的一件事。🤖 Generated with Claude Code
https://claude.ai/code/session_01LJagZZoJ8jKF7QsQAM56tC