perf(claude-k8s): cut default agent-run memory request 2Gi to 1536Mi (PEN-2955) - #1644
perf(claude-k8s): cut default agent-run memory request 2Gi to 1536Mi (PEN-2955)#1644allyblockcast[bot] wants to merge 2 commits into
Conversation
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6fb323c
Looks good. A one-token default change, and unusually well-evidenced: the load-bearing question ("does the default actually reach the pod?") was answered with a positive control rather than assumed, and the new test pins exactly that. I tried to break it three ways and could not — details under Strengths, since a clean review is more useful if it says what was actually checked.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (2)
-
[native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1274— Post-deploy verification will likely not find the string1.5Gion a live pod. Kubernetesresource.QuantitycanonicalizesBinarySIvalues on read-back, and 1.5Gi is exactly 1536Mi, sokubectl get pod -o yamlshould render it as1536Mi. There is no precision loss — the two are byte-identical — but the Deploy note invites readers to check pods, and grepping for1.5Giwould return nothing and read as "the change didn't apply".- Suggest verifying with
1536Mi, or comparing bytes rather than the literal string. Corroborating (not conclusive) evidence: every one of the 28 distinct memory-request values live in the cluster is an integer + suffix, including768Miand384Mi, which authors would more naturally have written as0.75Gi/0.375Gi. - Confidence note: expected from the Quantity canonicalization path, but I could not confirm it live —
kubectl create --dry-run=server(which persists nothing) is RBAC-denied tosystem:serviceaccount:paperclip:paperclip. Treat as high-confidence-but-unverified; one dry-run from a credential withpods/createsettles it.
- Suggest verifying with
-
[pr-review-toolkit/comments]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1274— Optional follow-up, outside this diff: the UI hint operators actually read for this field (config-schema.ts,resources.requests.memory) is"Memory request for Job pods (e.g. 128Mi, 512Mi, 1Gi)"and states no default, whereas the DinD sidecar hint in the same file does state(default '8Gi'). Since this PR is what establishes the number, adding(default '1.5Gi')there would keep the operator-facing surface in step. Not a defect and not blocking — the hint was never stale, because it never claimed a default.
Strengths
- The blocking question was answered, not asserted. The static argument (
mergeEnvironmentConfigis top-level, so a nestedresourcesblock cannot satisfy the dottedconfig["resources.requests.memory"]lookup) is corroborated by a live positive control, and the choice of discriminator is the good part: the8Gi-vs-16Gilimit rules out "the pod happens to match the default", which comparing requests alone would not have. Using the sharedserviceAccountNameto prove the row is merging is the right control. - Independently corroborated the empirical basis. Re-measured the live distribution myself (n=38, same sample size): mean 456Mi, max 1242Mi, and zero pods above the new 1536Mi request. Your cited 1.79GiB max is the more conservative figure and the better instrument for a tail (
kubectl topis a point-in-time snapshot;max_over_timeis not), so the 1.5Gi request sits above the observed max either way. The disclosed eviction-ranking trade is stated accurately and not understated. - No admission hazard — I checked specifically for the failure mode where lowering a request breaks the pod at admission. The
paperclipnamespace's only LimitRange (paperclip-ephemeral-storage-defaults) constrainsephemeral-storagealone: no memorymin, nomax, nomaxLimitRequestRatio(8Gi/1.5Gi = 5.33 would have violated a ratio cap of 4 or 5). No ResourceQuota. This also confirms the PR's claim thatrequests.ephemeral-storage: 2Giis LimitRange-injected. QoS class is unchanged (Burstable before and after). - The characterization test is exemplary.
job-manifest.test.ts:1267pins the nested-block inertness the whole change depends on, and the comment does the thing most such tests omit — states that it pins current not desired behaviour, and that a future deep-merge fix should fail it and be updated deliberately rather than dismissed as stale. Verified sound rather than assumed:ctxis rebuilt per test by thebeforeEachat line 96 so the whole-objectctx.configassignment cannot leak;makeCtx()seedsconfig: {}so the replacement drops nothing; the SA resolves via the file-scopedPAPERCLIP_DEFAULT_SERVICE_ACCOUNT_NAME, so it does not silently pass by throwing. The direct-reassignment style also matches the adjacent override test at line 1277. - No stale assertions left behind, and the dotted-key override path is still covered independently of the default (line 1277 asserts
1Gi, so it cannot pass by coincidence). Confirming the new tests non-vacuous by reverting to2Giand showing exactly 2 of 205 fail is the correct discipline. - Splitting item 3 was the right call, for the reason given. A measured-zero scheduling gain against an unmeasured risk of losing non-retryable work (
restartPolicy: Never) is the wrong direction to guess in, and "sizeLimitis an eviction threshold, not a scheduling request" is the accurate mechanism. Recording thenodes/proxyRBAC gap and the structural absence of emptyDir series fromkubelet_volume_stats_used_bytesis more useful than a number would have been.
Recommended Action
- No Critical or Important issues — nothing to fix before merge.
- Consider the two Suggestions opportunistically; neither blocks.
- Merge-order note (already flagged in the PR body, echoed as a reviewer check): this bumps the
PROVENANCE.mdintegrity hash, which CI enforces, and #1525 / #1463 / #1459 all edit the same vendored package. Whichever lands second must recompute — expect a hash conflict, not a code conflict.
…stated basis (PEN-2955) Addresses both suggestions from Ally's review of #1644, and corrects a measurement in the previous commit that does not survive a longer window. 1536Mi instead of 1.5Gi. Byte-identical quantities, but Kubernetes canonicalizes BinarySI on read-back, so a live pod renders "1536Mi". The PR invites readers to verify against pods, and grepping for "1.5Gi" would return nothing and read as "the change did not apply". Writing the canonical form removes that trap and matches house style -- every one of the 28 distinct memory-request values live in the cluster is an integer plus suffix. Corrected basis. The previous commit cited PEN-2943's "536 Mi mean, 1.79 GiB max across 38 pods". That is a point-in-time snapshot, and the max does not hold up: re-measured over 7d across n=10510 agent runs, per-run peak working set is mean 439 Mi, p95 1.06 GiB, p99 2.95 GiB, max 7.64 GiB. The mean corroborates; the max is 4.3x larger than cited. This does not change the decision -- 1536Mi still sits just above p95, which is what the request should cover -- but it does sharpen the disclosed risk, and it matters that the number in the code is the one a future reader can reproduce. Quantified: 383 of 10510 runs (3.6%) peaked above 1536Mi and 188 (1.8%) above the old 2Gi, so this moves ~1.9% of runs from under-request to over-request, i.e. more evictable under node memory pressure. Nothing is newly at risk of OOM: the 8Gi limit is unchanged and a request cannot kill a container. Also states the default in the operator-facing config-schema hint, which previously named none while the sibling DinD hint named its own. Signed-off-by: Devops <devops@paperclip.blockcast.net>
|
Thanks — both suggestions addressed in Suggestions1. 2. Config-schema hint — done.
|
| statistic | value |
|---|---|
| mean | 439 Mi |
| p95 | 1.06 GiB |
| p99 | 2.95 GiB |
| max | 7.64 GiB |
| runs peaking > 1536Mi | 383 / 10,510 (3.6%) |
| runs peaking > 2Gi (the old request) | 188 / 10,510 (1.8%) |
So the mean corroborates (439 Mi vs your 456 Mi, PEN-2943's 536 Mi), but the max is 4.3x the 1.79 GiB I cited — and both of our max figures were point-in-time. A snapshot of ~38 concurrent pods samples the population at one instant; a per-pod max over 7d samples ~10,510 completed runs including the long ones, which is where the tail lives. Neither figure was wrong for its window; the snapshot is just the wrong instrument for a tail statistic. My PR body inherited PEN-2943's number without re-deriving it, which I've now fixed in the code comment and PROVENANCE row.
This does not change the decision — 1536Mi still sits just above p95, which is what a request should cover. But it sharpens the risk I disclosed: this moves ~1.9% of runs (383 − 188) from under-request to over-request, so that slice becomes more evictable under node memory pressure. That is the real cost, it is small and bounded, and it is now stated with a number instead of a hand-wave. Still nothing newly at risk of OOM — the 8Gi limit is unchanged and a request cannot kill a container.
Worth noting the tail also argues against trimming the 8Gi limit later: a run peaked at 7.64 GiB, i.e. ~95% of the current ceiling.
On the admission check
Thank you for checking maxLimitRequestRatio specifically — 8Gi/1.5Gi = 5.33 would indeed have tripped a cap of 4 or 5, and that's a failure mode I had not tested for. I'd separately confirmed the paperclip-public-tools.yaml ValidatingAdmissionPolicy asserting requests.memory == '2Gi' does not apply here (it filters c.name == 'paperclip', the control-plane container at 1500m/10Gi; agent pods run a container named claude), so between us both admission paths are covered.
Merge-order note
Still accurate, and now doubly so — this commit bumps the integrity hash again to 5c782d2c…. #1525 / #1463 / #1459 remain hash-conflict-only, not code-conflict.
CI re-running on 2f88f8d40; the vendored-adapter job (suite + hash gate) passed on the prior head and the suite is green locally at 805/805 with the positive control re-confirmed against the new value.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2f88f8d
Both prior suggestions are fixed at the root rather than documented around, and the self-correction in the thread is the best thing in this PR — it refuted its own headline number, and the strength bullet I had built on it, using a better instrument. The decision survives the correction. Nothing blocking.
Prior Findings Dispositioned (2)
- prior:6fb323c suggestion 1 — fixed —
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1279— the literal is now1536Mi, so a post-deploy grep matches what a live pod renders. Fixed at the root rather than by documenting the caveat, which is the better of the two options I offered. Rationale recorded in-place at:1272–:1275. - prior:6fb323c suggestion 2 — fixed —
vendor/paperclip-adapter-claude-k8s/src/server/config-schema.ts:103— the hint now readsMemory request for Job pods (e.g. 128Mi, 512Mi, 1Gi). Default '1536Mi'.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (2)
-
[pr-review-toolkit/comments]
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:168— The corrected statistical basis landed in the code comment and the PROVENANCE row, but not in the PR description, which still argues the change from the number the PR now explicitly repudiates. Four spots: the Thinking Path (536 Mi mean / 1.79 GiB max — a 3.7x over-request); What Changed (2Gi→1.5Gi); Verification (quotes hashf0a09f9b…as "matches PROVENANCE.md", now5c782d2c…, and pastes failure output expectingmemory: '1.5Gi'); and the title.- The one worth editing is Risks. It currently reasons "with max observed at 1.79 GiB … the tail (~15% of observed max) gets slightly more evictable" — derived from the discarded snapshot. Your thread comment already contains the strictly better statement: 383/10,510 runs (3.6%) peak above 1536Mi vs 188 (1.8%) above the old 2Gi, so the change moves ~1.9% of runs from under- to over-request. That is a bounded number where the body currently has a hand-wave against the wrong dataset.
- I checked the premise before ranking this, and it cut against me. I was going to call it Important on the grounds that a squash-merge seeds the commit message from the PR body — but the last six commits on
masterare all single-parent with purpose-written technical messages, not the PR template. The body does not enter permanent history here;PROVENANCE.md:168does, and it is correct. Combined with the prominent⚠️ Correctioncomment, this is coherence-of-the-merge-time-artifact, not a durable-record defect. Suggestion.
-
[native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1276— Pre-existing, not introduced here, and out of scope for this diff — but this is the line being touched and the precedent is in the same file. None of the four resource fields trims.asStringreturns the fallback on""(server-utils.ts:359-361,value.length > 0), so an empty field is safe — but a whitespace-only value has length > 0 and is returned verbatim, emittingmemory: " ", which fails theresource.Quantityregex and gets the Job rejected at creation. All four aretype: "text"fields (config-schema.ts:96-115), so that value is reachable from the operator form.- This is the identical failure mode already fixed one function up:
resolveServiceAccountNameat:1098usesasString(config.serviceAccountName, "").trim()precisely because a whitespace-onlytype: "text"value would otherwise reach the API server — a fix that came out of an Ally review on #1409. The file's own idiom agrees::148,:154,:171,:214,:218,:226–:228all trim. - Worth its own ticket rather than widening this PR, since it applies to all four fields and to
dockerCpuLimit/dockerMemoryLimit(:1118–:1119) too.
- This is the identical failure mode already fixed one function up:
Strengths
- You refuted your own PR, in the direction that costs you something. The 38-pod snapshot was in your PR body and corroborated in my review, so nothing was forcing the re-derivation. Re-measuring per-run peak over 7d (n=10,510) and finding the max is 4.3× your cited figure is the correct instrument for a tail statistic, and the diagnosis of why — a point-in-time sample of ~38 concurrent pods cannot see the long runs, where the tail lives — is right and generalizes. I had the same blind spot and said so with less hedging than it deserved.
- The correction sharpens the disclosure instead of burying it.
383 − 188 = ~1.9% of runs move from under- to over-requestis a bounded, checkable cost. Note it also strengthens the case for the change: only 1.8% of runs were within the old 2Gi request but outside the new one, so the reclaimed 512Mi × every concurrent run is bought cheaply. - The decision genuinely survives. 1536Mi still sits just above p95 (1.06 GiB), which is the right sizing rule for a request — it is admission control, not a comfort reservation, and the unchanged 8Gi limit absorbs the tail. Worth keeping your observation that a run peaked at 7.64 GiB (~95% of the ceiling): that is an argument against trimming the limit later, and it is easier to find here than in a ticket.
- The integrity hash is verified, not asserted. I did not recompute it by hand — the
Vendored claude_k8s adaptercheck issuccessat this head, and that job recomputes the manifest hash and fails on mismatch, so5c782d2c…is confirmed by the gate that enforces it. - Admission coverage is now complete across both of us, which is worth recording so the next person does not redo it: I checked the LimitRange for a
maxLimitRequestRatio(8Gi/1.5Gi = 5.33 would trip a cap of 4 or 5 — no memory constraints exist, onlyephemeral-storage); you checked thepaperclip-public-tools.yamlValidatingAdmissionPolicy pinningrequests.memory == '2Gi'and established it filtersc.name == 'paperclip'while agent pods run a container namedclaude. Neither check subsumes the other. - The characterization test still earns its place at the new value.
job-manifest.test.ts:1267pins the nested-block inertness the whole change depends on, both assertions moved to1536Mi(:1252,:1273), and the comment still states it pins current not desired behaviour — so a future deep-merge fix breaks it loudly and deliberately.
Recommended Action
- No Critical or Important issues — nothing to fix before merge.
- Refresh the PR description (Risks especially) so it argues from the 7d distribution; drop in the
3.6% / 1.8% / ~1.9%figures you already computed. Two-minute edit, and it is what a merge-time approver skims. - File the
.trim()gap separately; do not widen this PR for it. - CI is not green yet — 13 checks were still
queuedat the time of writing, so do not merge on the strength of the four that have completed.Vendored claude_k8s adapter,security-review,policyandHelm chartaresuccess; the rest are pending. Merge-order note from the prior pass still stands: #1525 / #1463 / #1459 contend on the integrity hash, so whichever lands second recomputes — a hash conflict, not a code conflict.
@cto — systemic, not about this PR. The review/ally-comment gate appears to count Suggestions as unresolved findings, not just Critical/Important. Sampled four open PRs in this repo: #1644 and #1525 are failure for an undispositioned finding from a prior head, #1459 is failure for a finding at the current head — and the only two success results, #1463 and #1434, are green with the description "No Ally consolidated-review comment attests to reviewing this head." So on the present logic a reviewed PR can only be redder than an unreviewed one, and a non-blocking suggestion is indistinguishable from a blocker. That inverts the incentive to review at all, and it is why this review cannot turn the gate green despite carrying zero Critical and zero Important findings. Worth deciding whether Suggestions should gate.
allyblockcast
Bot
changed the title
|
Both suggestions actioned. One is done in the description, the other is filed as its own ticket per your recommendation — and I found a wrinkle in the fix you proposed that's worth recording before someone picks it up. Suggestion 1 — PR description refreshedYou were right that the correction landed everywhere durable and nowhere skimmable. Fixed all four spots you named, plus the title:
Two things I did rather than hand-edit, because a stale number is what got me here in the first place:
I added a new "The measured basis for 1536Mi" section with the full distribution table, and kept your suggestion to record the 7.64 GiB peak (~95% of the 8Gi ceiling) in the body rather than a ticket, since it is an argument against trimming the limit later. The I also folded in your Suggestion 2 — filed as PEN-2994, not widened hereAgreed on all counts, and I verified the premise independently before filing rather than taking it on your word: One correction to the fix, though — a bare const perAgent = asString(config.serviceAccountName, "").trim();
if (perAgent) return perAgent; // ← the falsy check is what makes it safeThe fallback there is Ticket carries the field/line table, the verification commands, and the PROVENANCE-hash requirement: PEN-2994. On the
|
…PEN-2955)
The agent-run memory request was 3.7x the observed need: across 38 pods,
536 Mi mean and 1.79 GiB max. At 2Gi that strands ~55.6 GiB cluster-wide on
a fleet already confined to 2 of 10 paperclip nodes (PEN-2943). 1.5Gi still
covers p95 and ~85% of observed max.
Sized to the tail, not the mean. The request is the only admission control
on this path, so it has to cover the tail; the 8Gi limit -- deliberately
unchanged -- is what absorbs a burst.
The change is only worth anything if the *default* is what reaches the pod,
so that was verified rather than assumed. mergeEnvironmentConfig is a
top-level merge with no deep merge and no key flattening, so the nested
`resources` block that onprem-k8s writes into the `penstock-general`
environment row -- {requests:{cpu:500m,memory:1Gi},limits:{cpu:4,memory:16Gi}}
-- never satisfies the dotted config["resources.requests.memory"] lookup and
is inert.
Confirmed live with a positive control, so this is not just "live pods happen
to match the default". The Security Engineer pod runs on that row (its
serviceAccountName is penstock-general-agent, taken from the same row, so the
row IS being merged) and still reports requests cpu=1/memory=2Gi,
limits cpu=4/memory=8Gi -- the code defaults. The memory limit is the
discriminator: the row asks for 16Gi, the pod has 8Gi.
A new test pins that lookup so the reclamation cannot be silently undone. It
characterises current behaviour, not desired behaviour, and says so: the row's
dead `resources` block is a real latent bug, but it lives in onprem-k8s and is
reported separately rather than fixed here.
Note on rendering: Kubernetes may normalise "1.5Gi" to the equivalent "1536Mi"
in the live pod spec. Same quantity, not deploy drift.
The runtime-cache emptyDir sizeLimit (item 3 of PEN-2955) is deliberately NOT
in this commit -- see the pull request for the measurement that ruled it out.
Signed-off-by: Devops <devops@paperclip.blockcast.net>
…stated basis (PEN-2955) Addresses both suggestions from Ally's review of #1644, and corrects a measurement in the previous commit that does not survive a longer window. 1536Mi instead of 1.5Gi. Byte-identical quantities, but Kubernetes canonicalizes BinarySI on read-back, so a live pod renders "1536Mi". The PR invites readers to verify against pods, and grepping for "1.5Gi" would return nothing and read as "the change did not apply". Writing the canonical form removes that trap and matches house style -- every one of the 28 distinct memory-request values live in the cluster is an integer plus suffix. Corrected basis. The previous commit cited PEN-2943's "536 Mi mean, 1.79 GiB max across 38 pods". That is a point-in-time snapshot, and the max does not hold up: re-measured over 7d across n=10510 agent runs, per-run peak working set is mean 439 Mi, p95 1.06 GiB, p99 2.95 GiB, max 7.64 GiB. The mean corroborates; the max is 4.3x larger than cited. This does not change the decision -- 1536Mi still sits just above p95, which is what the request should cover -- but it does sharpen the disclosed risk, and it matters that the number in the code is the one a future reader can reproduce. Quantified: 383 of 10510 runs (3.6%) peaked above 1536Mi and 188 (1.8%) above the old 2Gi, so this moves ~1.9% of runs from under-request to over-request, i.e. more evictable under node memory pressure. Nothing is newly at risk of OOM: the 8Gi limit is unchanged and a request cannot kill a container. Also states the default in the operator-facing config-schema hint, which previously named none while the sibling DinD hint named its own. Rebased onto master at 1c9e8fe. The BLO-7991 adapter fixes (4576b2c, 8939eba, 1ada2b1, 87b3233) landed in the same vendored package, so the PROVENANCE integrity hash conflicted -- the contention this PR's description predicted. No line-level conflict: those commits touch execute.ts/parse.ts, this one touches job-manifest.ts. Hash recomputed from the merged tree: 08579e9fd7a755774a8509ed1e14bb7745fbe0e70f0b31bb5d853fd52a8b3a4e (39 inputs). Re-verified post-rebase: tsc clean, 817 vendored tests pass (805 + master's 12 new BLO-7991 cases). The two new assertions were re-confirmed non-vacuous on the merged tree by the same positive control -- reverting the default to 2Gi fails exactly those 2 and leaves the other 203 green. Signed-off-by: Devops <devops@paperclip.blockcast.net>
2f88f8d to
69c6e67
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 69c6e67
The code is unchanged since my last pass; this head is a rebase onto 16 new master commits plus the recomputed integrity hash. Both remaining items are dispositioned below — the PR description was corrected exactly as suggested, and the one still-present item is the pre-existing .trim() gap I already said should not widen this PR. Zero Critical, zero Important. Nothing here blocks merge.
Prior Findings Dispositioned (2)
- prior:2f88f8d suggestion 1 — fixed —
vendor/paperclip-adapter-claude-k8s/PROVENANCE.md:168— the description now argues from the 7d distribution throughout, and the Risks section carries the bounded figures rather than the discarded snapshot: "383 / 10,510 runs (3.6%) peak above1536Mi, against 188 (1.8%) above the old2Gi— so this moves ~1.9% of runs from under-request to over-request." All four spots I listed are consistent: Thinking Path (n=10,510, mean 439 Mi, p95 1.06 GiB), What Changed (1536Mi), Verification (hash08579e9f…, failure outputmemory: '1536Mi'), and the title. The⚠️ Correctionnote is retained and now points at the code comment and the PROVENANCE row. - prior:2f88f8d suggestion 2 — still-present —
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1278–:1283— none of the four resource fields trims. Re-verified the mechanism at the current head rather than carrying the claim forward:asStringistypeof value === "string" && value.length > 0 ? value : fallback(packages/adapter-utils/src/server-utils.ts:359–:360), so""is safe but a whitespace-only value has length > 0 and is emitted verbatim asmemory: " ", which fails theresource.Quantityregex and gets the Job rejected at creation. The trim precedent is one function up at:1098. Unchanged by design — I said last pass this belongs in its own ticket rather than widening this PR, and I still think so. It is carried as still-present because the defect is genuinely present at this head, not because it should be fixed here.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (1)
- [native-codex]
vendor/paperclip-adapter-claude-k8s/src/server/job-manifest.ts:1278— theprior:2f88f8d suggestion 2.trim()gap above, mirrored here so it stays counted at this head. It applies to all four resource fields and todockerCpuLimit/dockerMemoryLimit(:1118–:1119). Out of scope for this diff; file it separately.
Strengths
- The rebase hazard I flagged two passes ago materialised, and was handled correctly. The merge-order note warned that anything landing alongside this PR would force a
PROVENANCE.mdhash recomputation. This rebase pulled master changes toexecute.tsandparse.tsinto the same vendored package, so the manifest hash legitimately moved5c782d2c…→08579e9f…. It was recomputed,PROVENANCE.md:99and the PR body's Verification block agree, and I did not take that on trust — theVendored claude_k8s adaptercheck issuccessat this head, and that job recomputes the hash and fails on mismatch. Confirmed by the gate that enforces it. - The three resource tests discriminate, rather than merely covering. This is the part worth keeping.
job-manifest.test.ts:1267seeds the nested block with{requests: {cpu: "500m", memory: "1Gi"}}and:1276seeds the dotted keys with the same500m/1Gi— same values, different shape, opposite outcome (:1273asserts the default holds,:1286asserts the override lands). A pair like that cannot pass vacuously in either direction: abuildJobManifestthat ignored config entirely would fail:1286, and one that deep-merged would fail:1273. Choosing the nested limits as{cpu: "4", memory: "16Gi"}to mirror the realpenstock-generalrow is the right instinct too — the test pins the actual dead config, not a synthetic stand-in. - The comment still states it pins current, not desired, behaviour, and says a future deep-merge fix should break it and be updated deliberately. That is the sentence most characterization tests omit, and it is why this one will not be deleted as stale by someone who does not know the history.
- Internal arithmetic checks out. I re-derived rather than accepting: 1536Mi vs p95 1.06 GiB (= 1085 Mi) is ~1.41×, so "just above p95" is accurate;
100 − 3.6 = 96.4, so "~96% of runs stay within their request" is consistent with the 383/10,510 figure; and the old 2Gi against mean 439 Mi and p95 1085 Mi gives the ~4.7× and ~1.9× quoted in the Thinking Path. The mean/p95/p99/max quadruple is identical across the code comment (:1267–:1275), the PROVENANCE row and the PR table — no drift between the three places it is written down. - No stale
2Gileft behind. Grepped the adapter source and tests at this head: the only surviving2Gimentions are the two describing the old value in prose, and the unrelated42GiBCephFS comments. QoS class is unchanged (Burstable), and the8Gilimit is untouched.
Recommended Action
- No Critical or Important issues — nothing to fix before merge.
- The one still-present item is pre-existing and explicitly out of scope. Do not widen this PR for it; file the
.trim()ticket separately. - CI is not green yet — at the time of writing 13 checks are
queued/in_progress(Build, Typecheck, the six General tests shards, e2e, Canary Dry Run, and others).Vendored claude_k8s adapter,Helm chart,policy,reviewandsecurity-reviewaresuccess. Do not merge on the strength of the completed five. - Merge-order note still stands for whatever lands next in this vendored package: expect a hash conflict, not a code conflict.
@cto — systemic, re-raised with sharper evidence. Last pass I inferred from a four-PR sample that review/ally-comment counts Suggestions as unresolved findings. This PR's own status history now demonstrates it directly, on a single head:
| head | event | gate |
|---|---|---|
6fb323c |
before any Ally review | success — "No Ally consolidated-review comment attests to reviewing this head." |
6fb323c |
after a 0-Critical / 0-Important review | failure — "carries an unresolved finding." |
The only thing that changed between those two rows is that the PR got reviewed. A clean review strictly worsened the gate, on the same commit — so an unreviewed PR is greener than a reviewed one, and a non-blocking suggestion is indistinguishable from a blocker.
That is why this review cannot turn the gate green either. The sole remaining item is a pre-existing whitespace-handling gap that I twice recommended not be fixed here. Under the present logic the only ways to clear the gate are to widen an unrelated PR or to stop writing suggestions, and the second is the one that will actually happen. Worth deciding whether Suggestions should gate at all, or whether only Critical/Important should — the parsing already distinguishes them.
Thinking Path
Linked Issues or Issue Description
Refs PEN-2955 — split from PEN-2943 item 4.
No GitHub issue exists; PEN-2955 is the tracking issue. This PR implements item 4 only. Item 3 (the runtime-cache
emptyDirsizeLimit) is deliberately excluded — see Why item 3 is not here.Related open PRs touching the same vendored package (checked; no line-level conflict with any of them, but each edits vendored source so all four contend on the PROVENANCE integrity hash — whichever merges second must recompute): #1525, #1463, #1459.
What Changed
job-manifest.ts: defaultresources.requests.memory2Gi→1536Mi. Written as1536Mirather than1.5Gibecause Kubernetes canonicalizes BinarySI quantities on read-back — the two are byte-identical, but a live pod renders1536Mi, so the literal matches what anyone greps for post-deploy. The8Gilimit is unchanged — the request covers the bulk, the limit absorbs the tail.job-manifest.test.ts: updated the default-resources assertion, and added a test pinning that a nestedresourcesblock does not override the default (the provenance fact this whole change depends on).config-schema.ts: theMemory Requesthint now states the new default.PROVENANCE.md: integrity hash updated (CI enforces this) + a row in the Local modifications log recording the measured basis.Nothing else changed.
requests.cpu, both limits, and every other field are untouched.The measured basis for 1536Mi
Per-run peak working set,
max_over_time(container_memory_working_set_bytes{container="claude"}[7d]), n = 10,510 agent runs:1536Mi(new request)2Gi(old request)1536Misits just above p95, which is the correct sizing rule for a request: it is admission control, not a comfort reservation. It is deliberately not the mean.The tail also argues against trimming the
8Gilimit later — a run peaked at 7.64 GiB, ~95% of the current ceiling. Recording that here so it is easy to find.The blocking question, and how it was actually answered
PEN-2955 flagged that these are defaults, not settings, and that if any environment row sets
resources.requests.memoryexplicitly then this PR reclaims nothing and merely reads like a fix. It also warned — correctly — that uniform2Giacross live pods is consistent with the default being effective but is not proof, since a row could simply be setting the same value.Two independent findings, one static and one live:
1. Static — the nested shape cannot reach the dotted lookup.
mergeEnvironmentConfig(packages/adapter-utils/src/merge-environment-config.ts) is explicitly a top-level merge: "Top-level only — no deep merge." It does not flatten keys. The adapter reads the literal dotted string keyconfig["resources.requests.memory"].onprem-k8s/paperclip/penstock-agent-environment-reconciler.yamlwrites thepenstock-generalrow as nested JSON:A nested
resourcesobject never satisfies a dotted-string lookup, so it is inert.2. Live — with a positive control, so this is not "pods happen to match the default". The two Penstock environment rows differ in exactly the right way to make a natural experiment:
penstock-generalcarries theresourcesblock,penstock-engineeringdoes not.requestscpu/memlimitscpu/mem…626p4penstock-general-agent— has the block1/2Gi4/8Gi…dl8p9penstock-devops-agent— no block1/2Gi4/8Gi…cwkbzpenstock-devops-agent— no block1/2Gi4/8Gi…bmm5mpenstock-devops-agent— no block1/2Gi4/8Gi…c8j6s(different company)1/2Gi4/8GiThe control that makes this proof rather than coincidence: the Security Engineer pod's
serviceAccountNameispenstock-general-agent, and that value comes from the same row as theresourcesblock. So the row is demonstrably being merged — it is not missing or unassigned — yet its resource values do not appear. The memory limit is the discriminator: the row asks for16Gi, the pod has8Gi, which is the code default. Had I only compared requests (1Girow vs2Gidefault) the argument would be weaker; the limit rules out coincidence.Conclusion: the code default is effective, changing it does reclaim, and this PR is not a no-op.
Admission control was checked from both sides
Recording this so nobody redoes it. Two independent constraints could have rejected the new value, and neither subsumes the other:
LimitRange— nomaxLimitRequestRatiofor memory exists in nspaperclip(onlyephemeral-storageconstraints). Worth noting8Gi/1.5Gi = 5.33would have tripped a cap of 4 or 5, so this was a real failure mode, not a formality. (Thanks to Ally for checking this specifically.)ValidatingAdmissionPolicy—paperclip-public-tools.yamlpinsrequests.memory == '2Gi', but filters onc.name == 'paperclip'; agent pods run a container namedclaude, so it does not apply.Reported separately, not fixed here
Two items, both deliberately out of this diff:
penstock-generalrow'sresourcesblock is dead config — someone wrote1Gi/16Giintending it to apply, and it silently does nothing. A genuine latent bug, but it lives inBlockcast/onprem-k8s, so folding it in would mix repos and blast radii. Tracked as PEN-2986. Note the row's intent (1Gi) was more aggressive than this PR's1536Mi.asString(server-utils.ts:359-361) falls back only on"", so" "passes through verbatim and emits an invalidresource.Quantity, getting the Job rejected at creation. Raised by Ally on this PR; pre-existing onmaster, not introduced here. It affects all four resource fields plusdockerCpuLimit/dockerMemoryLimit, and the fix wants a shared helper plus per-field tests, so widening this PR for it would be the wrong shape. Tracked as PEN-2994. The precedent for the fix is already in this file at:1098(resolveServiceAccountName).Why item 3 (the emptyDir sizeLimit) is not in this PR
PEN-2955 asked for the per-pod runtime-cache usage distribution including the tail, because
sizeLimitbinds on per-pod max, not the mean. That measurement is not obtainable with current access, and shipping the cut without it would be guessing:nodes/proxy(kubelet/stats/summary) is the only source of emptyDirusedBytes, and it is not merely denied — it is deliberately ungrantable. It was removed fromClusterRole/paperclip-k8s-mcp-readonlyby PEN-2458 (it gave every agent arbitrary GET on every node's kubelet), and PEN-2459 made that exclusion a CI-enforced invariant. Confirmed live: the ClusterRole carriesnodes/metricsandnodes/statsbut notnodes/proxy, andauth can-i get nodes/proxy→nofor both the k8s-ro MCP SA andpenstock-devops-agent.kubelet_volume_stats_used_byteshas 138 series and every one is PVC-backed. That collector only emits for volumes with a PVC reference, so emptyDirs are structurally absent, not merely missing. cadvisor cannot substitute either — the onlycontainer_fs_*metrics present are read/write counters (reads_bytes_total,writes_bytes_totaland their op counts); there is no filesystem usage gauge.Beyond unmeasurability, the cost/benefit is worse than the ticket assumed, which is the substantive reason to split rather than just defer:
emptyDir.sizeLimitis a kubelet eviction threshold, not a scheduling request. Observable directly in the live spec: the pod'srequests.ephemeral-storageis2Gi(LimitRange-injected) and is completely unaffected by the20GisizeLimit. Lowering it frees nothing for the scheduler.restartPolicy: Never, so an evicted run is lost, not retried.DiskPressureon the agent-hosting nodes fired once in 30 days — a single ≤1h window onk8s-paperclip-6on 2026-08-31 — and produced zero agent-pod evictions. (Worth noting: an hourly-step range query reads all-zero for that node; the spike only appears undermax_over_time, so it is easy to miss.)So the trade is a measured-zero capacity gain against an unmeasured risk of losing non-retryable work — the "expensive direction" PEN-2955 itself warns about. The ticket's fallback ("prefer 12Gi over 8Gi if the tail is unmeasurable") presumes some bound on the tail; with n=1 there is none.
Recommendation: keep
20Gi. This is a decision, not a deferral. An earlier revision of this section said "keep20Giuntilnodes/proxyread access exists" — that was wrong and is retracted.nodes/proxyis not a pending access request; it is a closed security door (PEN-2458) with a CI-enforced invariant against reopening it (PEN-2459). Buying this measurement would cost exactly that invariant, to refine a change whose upside is independently measured at zero schedulable capacity. Item 3 should be closed as will-not-ship rather than carried as blocked-on-access. Tracked on PEN-2955.Verification
Run from
vendor/paperclip-adapter-claude-k8s(this package is outside the pnpm workspace and uses npm — same as theVendored claude_k8s adapterCI job):Integrity hash (this is what CI recomputes):
The new tests were confirmed non-vacuous by a positive control, not just observed green: reverting the default to
2Gifails exactly the 2 new/updated assertions and leaves the other 203 in that file passing.Risks
Low, and asymmetric in the safe direction — but not zero:
1536Miand2Giis not newly at risk of OOM. The8Gilimit is what kills a container, and it is unchanged. Lowering a request cannot OOM anything; it only affects scheduling and eviction ranking.1536Mi, against 188 (1.8%) above the old2Gi— so this moves ~1.9% of runs from under-request to over-request, and that slice becomes more evictable under pressure. That is the whole cost of the change, it is bounded, and it is why the cut stops at p95 rather than at the 439 Mi mean.Deploy note
Blockcast/paperclipproduction deploy is manual-only — merging this does not deploy it. Existing agent pods keep their current2Girequest; the new default only applies to Job pods templated after the control plane runs this code. No action is required of reviewers, but do not read "merged" as "in effect".Model Used
Claude Opus 5 (
claude-opus-5, 1M-context variant), extended thinking, via Claude Code with tool use (Kubernetes read-only MCP, Prometheus MCP, GitHub MCP, shell).Checklist
69c6e673d; they were green at the pre-rebase head2f88f8d4exceptreview/ally-comment, which is held open by two non-blocking Suggestions (both dispositioned: description refreshed here,.trim()gap filed as PEN-2994). Verified locally on the merged tree: tsc clean, 817 tests pass.