Repository navigation
docs(quest): plan gate admission with headroom - #180
Conversation
Follow-up of the contention measurements in #177: admit at once while the host has room, fix the two-window spacing, a stable claim directory, backward clock steps, and forwarding Ctrl-C once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecd0bf28c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Independent review standing in for Codex, at ecd0bf2. Docs-only, so I checked the plan against src/cli/gate.ts, the README's "Gate expensive commands" section, scripts/gate-bench.sh, the measurements and follow-ups in kixelated/quest#177, the decisions in kixelated/quest#128, and kixelated/quest#178 (quest worktree run). I didn't re-open the recorded choices: headroom admission, the reversal of "no adaptive scheduler", and keeping memory admission.
quest check passes (48 documents). These factual claims hold: about 10 s effective spacing, the split claim directory, the backward clock step allowing one extra adjacent admission, the doubled Ctrl-C, the gate-bench.sh overhead dependence on XDG_RUNTIME_DIR, and 104 s versus 19 s with timeouts only at 8x.
Findings:
- P1: as written, headroom admission bypasses memory admission for a burst, because the memory check counts no recent admissions.
- P2: admission records need to last a minute and be counted atomically, and the current one-claim-per-window design does neither.
- P2: "near the limit" is undefined.
- P2:
quest worktree runexists only in #178, which is still open. - P3: the spacing fix, claim directory rule, monotonic clock, Ctrl-C example, 8x bench expectation, wording, and the
seed.test.tspath. See the inline comments.
(Written by Claude Opus 5.5)
Count recent admissions against free memory as well as load, define the spacing band, require atomic minute-long admission records, and pin down the claim directory, clock-step, and Ctrl-C rules. The Ctrl-C fix covers `quest worktree run` only if #178 has landed. The quest is now [M]. Also fix the seed test path in the inbox-accuracy quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Before Linux 5.14 a scope's cgroup has no cgroup.kill, so leftover descendants were never killed and the supervisor waited on a populated scope forever. run now refuses such hosts up front, and kill() ignores a missing cgroup.kill only once the cgroup itself is gone, without ever creating the file. Retitle the worktree quest [M] and point the Ctrl-C follow-up at #180. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dfc30ea5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Derive the recent-admission count from unexpired records with a record-then-count step instead of slot numbers, which expiry leaves with holes. Rewrite a future-dated record's time to now rather than discarding it, so a large backward clock step still spaces admissions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b2c83a4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Record-then-count prevents over-admission but can livelock when a whole burst withdraws together. Suggest serializing prune, count, and record under a short host-wide lock reclaimed from exited owners. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cally (#178) * chore(quest): claim quest/m0/worktree.md Quest-Claim: 4465a18959f4b93c * feat(worktree): supervise runs in systemd scopes and release automatically `quest worktree run <id> -- <command>` runs a job in a checkout inside a systemd user scope named once per run. A handshake proves the shell is in that scope's v2 cgroup before the supervisor lets the command start, so a dead supervisor can never start it. When the command exits, descendants left in the scope are killed, and once the scope is empty and no other run remains, the scratch declared at `create` is pruned. Runs are recorded before their scope exists. Every registry operation reconciles runs whose owner (PID plus Linux start time) exited and whose scope is empty, so a crashed supervisor's checkout is released only after its orphans stop. A run that cannot be inspected or confirmed stopped stays recorded and the checkout stays reserved, with the reason noted. `release` refuses while runs are live. Non-Linux hosts fail explicitly. The registry lock now names its owner in a file inside the lock directory and is taken by an atomic rename, so the lock of a process that exited on this host is reclaimed by unlinking that exact owner file, never a later owner's lock. Also: scratch is declared at `create` instead of `release`, a scratch symlink to another worktree is unlinked rather than refused, and CI starts a systemd user manager for the real-scope fixtures (orphan cleanup, cancellation, crash reconciliation). The quest now covers only `quest worktree remove`; the runner and agent quests no longer wait on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(worktree): refuse scratch naming the checkout on Windows resolve('/', '.') is a drive root on Windows, so '.' passed the check there and was refused only by the tracked-file check. Test the path's own components instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(worktree): require cgroup.kill instead of waiting forever Before Linux 5.14 a scope's cgroup has no cgroup.kill, so leftover descendants were never killed and the supervisor waited on a populated scope forever. run now refuses such hosts up front, and kill() ignores a missing cgroup.kill only once the cgroup itself is gone, without ever creating the file. Retitle the worktree quest [M] and point the Ctrl-C follow-up at #180. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m0/README.md
#178 landed, so the gate headroom quest fixes the double Ctrl-C in both commands unconditionally, and the worktree quest links to it instead of the planning PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3201727034
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four review rounds kept finding edge cases in the prescribed records, lock, and clock-step mechanisms. Keep the decisions and the three-band rule, state what the admission mechanism must guarantee, and list every review finding as a hazard the implementation must test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8b6cc6ace
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
/run/user/<uid> comes and goes with logind sessions, so choosing by its existence can split callers again. State the requirement and the hazards instead of a directory rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8c6431b61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A SIGHUP sent with kill to Quest alone cannot be told apart from a terminal hangup, so skip forwarding only SIGINT in the foreground group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Plans the follow-ups from the
quest gatecontention measurements in #177, plus small leftovers from #128 and #178.Changes
quest/m0/gate-headroom.md[M], ranked first in m0 where the gate quest was. It covers:SETTLE_MSapart instead of about two windows;quest gateandquest worktree run(landed in kixelated/quest#178).quest/m0/worktree.md: its out-of-scope Ctrl-C bullet links to the new quest instead of this PR.quest/m0/kix/inbox-accuracy.md: slimtest/seed.test.tsrather than raising its timeout as the seed grows (cloudflare/test/seed.test.ts). It was the only test to time out at 8× overload.quest worktree stopand clearing stale inspection failures go intoquest/m0/worktree.mdinside #178, which is editing that quest now.quest agentfrom #176 is dropped for now: m0's runners are Linux and macOS. feat(agent): start, resume, and stop billed headless agents #176 records it.worktree-agents.mdwaits on resource control is left to that quest; the gate quest links it.Decision paper trail
Gate behaviour after the #177 measurements (on a 24-core host, a burst of 8 took 104 s gated versus 19 s ungated; the gate only prevented timeouts at about 8 suites per CPU):
Review round 1 (review): all 11 findings (12 threads, the P1 posted twice) fixed in 5dfc30e, including counting recent admissions against free memory (P1), which keeps "memory admission stays". The fixes made the quest [M].
Codex review rounds 2 and 3 (on 5dfc30e and 83b2c83): fixed in 83b2c83 and 3f6adc2. The count now comes from unexpired records. A future-dated record has its time rewritten to now instead of being dropped. Prune, count, and record run under a host-wide lock, so a burst elects a winner instead of livelocking.
Design checkpoint after Codex round 4 (on 3201727), decided by the orchestrator under the maintainer's standing instruction:
After the checkpoint, Codex found two more issues, both fixed. The claim directory is now a requirement for one location that never switches with environment or login sessions, with
/run/user/<uid>coming and going listed as a hazard (0d96219). The Ctrl-C fix now skips forwarding only SIGINT in the foreground group and always forwards SIGHUP and SIGTERM (d3983e2). Codex found no issues on d3983e2, and the landed-quest guard passes against main.Reviewer while Codex is out of credits:
/code-review --comment.just checkandjust testpass.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code