Repository navigation
fix: palace single-writer guard (T-065), 10M default cap (T-068), unstale model-picker test (T-070) — suite now 607/607 - #9
Closed
aaroncoville wants to merge 7 commits into
Closed
aaroncoville wants to merge 7 commits into
aaroncoville wants to merge 7 commits into
Conversation
added 7 commits
August 24, 2026 19:39
…ter guard memory.ts states the invariant at its own mineNow(): "the palace permits a single writer, so mines MUST be serialized". retainWorker called mineAgent directly and never touched `mining`, so the 60s GC sweep and the 10-minute mine loop could open two writers at once — the loser fails with "held by another writer", and on the retain path that is a reaped worker's memory, lost immediately before its scratch dir is deleted. retainWorker now takes the same lock, but WAITS for it rather than early-returning the way mineNow does: a false return defers the scratch deletion to the next sweep, and on a busy floor "the loop is mining" is true often enough that the deletion would never converge. The wait sits inside the existing Promise.race, so the 30s cap bounds lock-wait + mine together; an abandoned wait releases the lock instead of leaking it (`abandoned` flag), because a guard left held silently stops every future mine. Tests assert SERIALIZATION, not completion: the second writer must not BEGIN before the first FINISHES. Verified RED 0/3 before the fix and again by reverting the guard afterwards.
…ueue past a pair
The T-065 single-writer guard is correct on both paths; neither was tested.
Both gaps fail SILENTLY — a leaked lock does not crash, it just makes every
future palace write stop, so nothing on the floor would report it.
(1) RELEASE ON THROW. mineAgent is documented never to reject, but both call
sites release in a finally anyway. Nothing exercised that. Two tests make
mineAgent throw in mineNow and in retainWorker and assert the lock is free
afterwards — asserted through BOTH kinds of consumer, since a leak hides
differently in each: mineNow silently bails, retainWorker silently waits out
its whole timeout and refuses to delete the scratch dir.
(2) THE QUEUE BEYOND A PAIR. All three existing tests use exactly two writers,
so only one waiter is ever parked and releaseMineLock's "one wins the
re-check, the rest queue again" contract is vacuous. Four concurrent
retainWorkers now assert strict start/end pairing: exactly one writer live
at a time, every one of them runs, each exactly once, lock free after.
Break-it, per mutation (new suite / existing serialization suite):
- mineNow drops its finally-release 1 of 3 / n-a
- retainWorker drops its finally-release 1 of 3 / n-a
- release wakes only the head of the queue 2 of 3 / n-a
- acquire re-checks with `if` not `while` 2 of 3 / 3 of 3 GREEN
The last is the one that matters: it serializes two writers correctly and lets
every writer past the second barge in together, and the existing suite cannot
see it.
One mutation deliberately does NOT bite: a release that wakes the queue without
clearing it. The stale resolvers are already settled and settling twice is a
no-op, so that is benign rather than a defect — recorded in the test comment so
nobody later mistakes the green for coverage.
Production lock design untouched. typecheck 0 errors; suite 590 of 591 (the one
failure is the known pre-existing 'model picker options stay provider-specific').
…sert the trap `model picker options stay provider-specific` has been the suite's one known failure for a day. It is a STALE TEST, not a missing model — verdict (a). Evidence: 32b1fe3 (Aaron, 2026-08-23 23:43) deliberately rewrote CODEX_MODELS after querying codex-cli 0.149.0's own `model/list` over `codex app-server`. It added gpt-5.5 and moved gpt-5.6-sol last with an "(API key only)" label. Nothing was dropped: sol is still offered, at index 4 instead of 1. The test was written 2026-08-21 and never updated, so it failed on ORDER and on the added slug. The exact-list assertion is corrected to the shipped catalog. On its own that would only mirror current output, so a second test pins the property 32b1fe3 actually established, which a flat id-list cannot express: - gpt-5.6-sol must stay OFFERED (API-key users pick it) and stay LABELLED as API-key-only, because a ChatGPT login is rejected on it; - the codex preset's recommendedOrchestratorModel must be a slug the picker offers, and must never be sol. Recommending an unoffered slug is exactly how `gpt-5-codex` shipped and 400'd every turn. Verified by breaking it four ways — deleting sol, stripping its label, and setting the recommendation to `gpt-5-codex` and to sol — each turns the intended assertion red with its own message. Both CODEX_MODELS and this test are UPSTREAM files. 32b1fe3 already diverged the catalog; this brings its test back in step. No source changed here. Suite 596/596, typecheck 0 errors.
Aaron, 2026-08-24: "Without a tokenCap I think 10M is a sensible default."
This supersedes the earlier "NO per-worker cap" directive, which was written
when a cap could only arrive on a spawn-request, so 0 meant "not throttling
yet". Running a worker uncapped is now a DECISION (defaultWorkerTokenCap: 0),
not an omission.
The fallback chain already existed; this is a value change plus the tests that
pin the behaviour. Two call sites (the reaper tick and workers:list) each
carried their own copy of `typeof cfg.defaultWorkerTokenCap === 'number' && > 0`
— they now share one exported helper in src/shared/tokenCaps.ts, which is what
makes the behaviour testable without a test re-implementing a call site. A
malformed stored value (negative/NaN/Infinity/string) resolves to 0 rather
than to a nonsense cap.
The stale rationale comment on the constant is replaced: it cited a directive
that no longer holds, and that is how the next person gets this wrong. It also
now records that a config.json already persisting this key keeps its stored
value — readConfig merges `{...DEFAULTS, ...parsed}`, so this default only
reaches installs that never set one.
Tests: test/worker-default-token-cap.test.cjs — explicit cap wins, uncapped
gets 10M, 0 still means unlimited, malformed values sanitise, and both call
sites route through the helper.
Owner
Author
|
Closed per Aaron's decision: behaviour abandoned. Branch remains in git if this is ever revisited. |
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
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.
The suite is fully green for the first time: 607 of 607, typecheck 0 errors.
The "known pre-existing failure" every hire was told to ignore all day is gone — T-070 is the reason.
Three independent tasks, one commit each, so any single one can be reverted without disturbing the others. Bundled because they are all small, all verified, and the green suite only arrives with all three.
T-065 (P0) —
retainWorkermined outside the palace single-writer guardmemory.ts:298-300states the invariant in its own comment: the palace permits a single writer, so mines MUST be serialized.mineNowhonoured it;retainWorkercalledmineAgentdirectly and never touched the flag. The GC sweep runs every 60s against a 10-minute mine loop, so overlap was a matter of time, not luck — and two concurrent writers to a store documented as single-writer risks a corrupted palace, which would silently poison recall for every future agent.retainWorkernow waits for the lock rather than early-returning (a false return there would defer scratch deletion forever on a busy floor), with the wait inside the existingPromise.raceso the 30s timeout bounds lock-wait and mine together.Full lifecycle: dev
kevin-t065→ god review → QAdwight-t065qa.This was the strongest work of the day: my adversarial mutations found nothing. I removed the leak-guard the author claimed prevents a lock leak, and its own test caught it. QA then closed the two paths its tests did not reach — both of which fail silently:
mineAgentthrows — a leaked lock does not crash; it blocks every future mine with no error anywhere, and the palace just quietly stops learningVerified: leaking the release turns both new tests red.
T-068 — uncapped workers now default to 10M
Approved decision. A worker with no
tokenCapin its spawn-request and none in its manifest previously spawned unlimited. The fallback mechanism already existed; the shipped value was simply0.defaultWorkerTokenCap: 0still means unlimited — the change makes unlimited a decision rather than an omission. Verified: reverting to0turns "a worker with no cap of its own inherits the 10M shipped default" red, and the test asserts the literal behaviour rather than sharing the constant with the implementation.This changes live behaviour for existing uncapped spawns, as intended.
T-070 — the long-standing suite failure
Diagnosed as a stale test, not a product bug: the codex model list was reordered and gained
gpt-5.5.It did more than unstale the assertion — it added the trap the flat id-list could never express.
gpt-5.6-solbills against an API key and is rejected under a ChatGPT login, so it must stay offered but must never be recommended. Recommending a slug the picker does not offer is exactly howgpt-5-codexshipped and 400'd every turn.The slugs are spelled out literally on purpose — importing them from the catalog would make the test agree with the catalog by construction and protect nothing.
Verification
Every claim reproduced independently rather than relayed:
retainWorker'sfinallydefaultWorkerTokenCapto0npm run typecheckOne thing worth flagging: merged onto the post-T-061
main, the T-065 branch's diff appeared to deletetest/worktree-deps.test.cjs. That was a stale-base artifact — the branch predates the T-061 merge. I confirmed the file survives the actual merge.