Repository navigation
Cite Ray's threshold memory monitor at the revision the fleet executes - #10484
Conversation
Third item in the operator's order. It was parked because I had the measured BEHAVIOUR of the incident but not the upstream constants, and writing env spellings and defaults from compacted memory into an extdeps citation is the fabrication section 5 forbids. A peer session read them from Ray's own source and from inside the serving containers, so they are grounded now. NAMED FOR THE IMPLEMENTATION, NOT THE FACTORY. The obvious name, extdeps.ray.memory_monitor, would have been a meaning fork: upstream has no single memory_monitor any more, but three implementations -- threshold, pressure, event -- behind memory_monitor_factory.h. One name over three materially different behaviours is what section 3 forbids, and a reader could not tell which one a cited default belonged to. The second reason is stronger: PressureMemoryMonitor is never returned by Create at all, so a module named for the directory would have implied coverage of a path upstream does not run. THE REVISION PIN IS LOAD-BEARING AND IT ALREADY CAUGHT AN ERROR. The defaults were first read from master, and master carries count_swap_in_memory_monitor -- a field that DOES NOT EXIST at 2.58.0. Written into a row pinned here it would have cited a knob absent from the version we run, in a shape no later reader could detect: real repository, real file, real revision, wrong about one field. The version is established from the running deployment (ray.__version__ inside the serving container), not assumed. The absence is now a ROW rather than an omission, because a field merely missing from a list is indistinguishable from one nobody looked for. It also matters here specifically: the Spark hosts carry 16 GiB of swap and are USING it, so whether the monitor counts swap is live rather than hypothetical -- and my own prior assumption that these hosts had no swap was wrong. THE ENV SPELLING IS DERIVED, NOT LISTED. Ray's pattern is RAY_ prefixed to the config field name verbatim, which is why the variables are lowercase rather than the SCREAMING_CASE an author would guess. Deriving it means a config row cannot drift from its own environment variable, and the documented disable -- a zero refresh interval -- is computed from the same row rather than authored beside it. THE PRINTED PERCENTAGE IS A TRAP AND IS RECORDED AS ONE. The monitor derives the displayed fraction as threshold_bytes / total_bytes, so it is a re-derivation and not the configured fraction echoed back; the two agree only when the byte threshold came from that fraction against the same total, which is exactly what a cgroup limit or an isolated slice changes. THE UNCAPTURABLE LINE IS NOT A MEASUREMENT, and that is the arm that matters for reading our own incidents: a reader who scans for the exceeded-threshold line and finds none has NOT established that memory was below threshold. The monitor may have been announcing it could not see. Absence of an alarm and evidence of safety are different facts. WHAT IS DELIBERATELY NOT MODELLED: where resource_isolation_enabled originates. It is an argument to Create, not a spelling, so its provenance is the caller's fact rather than this citation's, and guessing would put a gunbc-layer decision inside an extdeps row. Witnesses 11/11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 21c6da33d098603e6c73312aae09a9668d6e3511.
The narrow implementation name is right, and the explicit count_swap_in_memory_monitor absence at ray-2.58.0 is real. But the 11/11 battery currently proves the authored model, not fidelity to the pinned source. Five blockers:
-
The revision pin is violated by the trigger literal. At
ray-2.58.0,threshold_memory_monitor.ccemitsMemory usage %dB exceeded threshold of %dB (%.1f%% of %dB total). This branch records... %dB limit), which is the currentmasterspelling introduced with the swap-aware accounting. So the exact master→2.58.0 contamination this PR says it prevents survived in a different field. Correct the literal and bind the log declarations to the exact tagged source file. -
ray_selected_monitorsis not a total observer ofMemoryMonitorFactory::Create. The Linux implementation also branches onmemory_monitor_refresh_ms: with zero refresh and isolation off it returnsNoop; with zero refresh and isolation on it returnsEvent + Noop. The current function accepts only platform and isolation, andRaySelectedMonitorscannot representEventAndNoop, despite this same module exporting the zero-refresh disable binding. Add the interval/enabled dimension and all outcomes, or structurally restrict the function to positive refresh and name that narrower subject. Enrol the zero cases. -
Empty
RAY_*environment is not evidence that compiled defaults are the effective fleet configuration.RayConfig::initialize(config_list)reads environment values and then applies JSONconfig_listoverrides. Either rename this record to compiled-in defaults and remove the claims that these values govern the fleet, or capture the effective config/config-list through a deployment observation in thegunbc.sparklayer. Also resolve the partial defaults surface:MinMemoryFreeBytesis enumerated as a threshold-monitor config and is passed directly intoGetMemoryThreshold, but its-1default is absent fromRayThresholdMonitorDefaults. Narrow the config population or model the sentinel; do not silently omit it. -
The unmeasured state is partial across the very isolation fork this module models.
NodeMemoryUncapturableis the non-isolated host path. The isolated path instead emitsFailed to take user and system slice memory snapshot due to: %s. The threshold memory monitor will not be able to provide resource isolation protection.and also returns no measurement.ray_monitor_line_reports_measurementtherefore cannot represent the isolation-side observation failure. Add that false arm, or narrow the type/function explicitly to host-memory lines. Likewise,RayMonitorLogLine/ “WHAT THE MONITOR PRINTS” overclaims unless it is exhaustive; “selected incident lines” would be an honest narrower subject. -
The source authority does not cover most of the modeled facts, and one negative is duplicated as a disconnected verdict. The sole
ExternalModelScopeanchors onlyRayThresholdMonitorDefaultstoray_config_def.h; the environment macro, factory selection, log formats, and master/tag contrast live in other files and have no exact-source scope here. Add exact-tag authorities/scopes (and a master authority only if the master contrast remains). Keep mutable Spark swap/environment observations downstream rather than inside extdeps ground text. Finally,ray_pressure_monitor_is_factory_selectable() -> falseis a second authored assertion beside the closed factory result type; its witness proves the assertion returns false, not that the factory cannot select pressure. Delete it or derive the negative from the completed exhaustive factory model—the same vocabulary-versus-binding distinction that mattered on #10440.
briansrls
left a comment
There was a problem hiding this comment.
Addendum to blocker 2: the zero-refresh binding disables ThresholdMemoryMonitor, not Ray memory monitoring as a whole. With resource isolation enabled, EventMemoryMonitor remains selected and the factory returns Event + Noop. Please narrow ray_memory_monitor_disable_env and its heading/body to ray_threshold_memory_monitor_disable_env; otherwise the exported name itself recreates the factory/implementation meaning fork this PR correctly avoided at module scope.
…, not memory enforcement Three corrections, all from a verbatim read of multi_memory_monitor_factory.cc at ray-2.58.0. Two are defects in what I opened an hour ago, and the second overstates in the direction a refusal must never overstate. 1. SELECTION DEPENDS ON CONFIG THE SIGNATURE DOES NOT NAME. I modelled selection from Create's parameter list -- kill_workers_callback, resource_isolation_enabled, cgroup_manager -- which omits the refresh interval entirely. The interval is read from the RayConfig GLOBAL inside the body: uint64_t monitor_interval_ms = RayConfig::instance().memory_monitor_refresh_ms(); so the function gave a wrong answer for the disable case, the case that matters most. A header's parameter list is not the set of inputs a function reads, and the module now cites the body. 2. A ZERO INTERVAL DOES NOT DISABLE MEMORY ENFORCEMENT. It swaps the THRESHOLD monitor for a NoopMemoryMonitor. The EVENT monitor is pushed BEFORE the interval is read, guarded only by resource_isolation_enabled, so isolation ON plus a zero interval builds Event and Noop together -- a live cgroup-driven monitor still killing workers. I had called that argv "disable the monitor", which would tell a caller a host is unenforced while something is enforcing on it. Renamed to ray_threshold_monitor_disable_env, and ray_built_monitors now returns the POPULATION so the two isolation settings give visibly different answers. 3. THE PLATFORM ARM IS DELETED, AND ITS DELETION IS THE RULE THIS MODULE ALREADY RECORDS. I carried a non-Linux Noop arm from a comment on MASTER's header; the 2.58.0 body contains no platform branch at all. Asserting it would have been a citation to a revision it was not read at -- exactly the failure the count_swap_in_memory_monitor absence row exists to record this module coming close to once. If the behaviour is real it can land later with a reading behind it. THE THREE-CAUSES PROBLEM DISSOLVES RATHER THAN NEEDING A MODEL. I had feared a constructed-but-never- scheduled state, indistinguishable from a live monitor seeing nothing. It does not exist: the zero interval swaps the OBJECT and logs on construction, naming the variable. So a silent threshold log separates into three DECIDABLE causes -- disabled line present, uncapturable line present, or neither -- and the disabled line is now a modelled arm. A constant-valued silence_is_evidence_of_safety predicate written on the way to this is deleted: every arm answered false, which no input can discriminate and a witness could only assert back at itself. Witnesses 15/15. The discriminating pair is the two isolation settings at a zero interval: ON still enforces, OFF enforces nothing. A model answering "disabled" for both fails the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact-head re-review of c67cc15.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head c67cc15c686d14de8d4492605c90a133426a48bb.
The dangerous factory defect is fixed: the observer now exposes the hidden RayConfig interval read, returns a population, represents [Event, Noop], narrows the zero-refresh binding to the threshold arm, and has a discriminating isolation-on/off pair. Those changes are accepted. Exact-head CI is green.
The other fidelity blockers from review 5118505154 remain, plus one scope consequence of deleting the platform arm:
-
The pinned trigger literal is still from master.
ThresholdExceededstill recordsMemory usage %dB exceeded threshold of %dB (%.1f%% of %dB limit). Atray-2.58.0,threshold_memory_monitor.ccsays... %dB total). Correct the literal and bind it to that exact tagged file. -
Compiled defaults are still presented as effective fleet configuration. The source still says an empty
RAY_*environment means these values are “in force” and that there is no deployment-layer choice.RayConfig::initialize(config_list)applies JSON config-list overrides after reading the environment. Keep this row as compiled-in defaults, or add a downstreamgunbc.sparkeffective-config observation. Also modelmin_memory_free_bytes = -1semantically or removeMinMemoryFreeBytesfrom the claimed defaults population; the factory consumes it and the current defaults carrier omits it. -
The observation population remains partial. The isolated failure path emits
Failed to take user and system slice memory snapshot ...and returns no measurement, butRayMonitorLogLinehas only the non-isolatedNodeMemoryUncapturablearm. Add the isolation-side false arm, or narrow the type/function to selected host-path diagnostics. The new three-cause prose also exceeds the graph: “ran and saw below threshold” is absence of a line and is not an inhabitant ofray_startup_line_explains_threshold_silence; that function currently classifies two explicit texts, not a three-way observation outcome. -
The source authority still covers only
RayThresholdMonitorDefaultsinray_config_def.h.further_citationsremains empty while the module asserts the environment macro, config-list override semantics, factory construction, diagnostic strings, and master/tag contrast from other files. Add exact-tag authority/scope for those declarations. Keep mutable Spark swap use and container-environment observations downstream rather than inside the extdeps absence ground. -
ray_built_monitorsis a Linux-body observer carrying a total name. Deleting an unverified platform arm was honest, but the exact tag's build graph selects a separate noop-only implementation off Linux. Either name/scope thisray_linux_built_monitors, or model the platform selection from the tagged BUILD/noop implementation. Also deleteray_pressure_monitor_is_factory_selectable() -> falseand its witness: the closedRayBuiltMonitorresult already excludes pressure, while the Boolean merely asserts its own authored answer. -
The canonical PR body is stale. It still reports 11/11, says the battery covers three platform/isolation combinations, and presents the compiled defaults as effective fleet values. The current source is 14 property rows plus aggregate = 15/15, has no platform arm, and has the interval/isolation matrix instead.
No objection remains to the threshold-specific module name, the verified count_swap_in_memory_monitor absence at 2.58.0, the derived RAY_ spelling, or the interval/factory rework.
…s effective configuration Four findings from exact-head review 5119115399, all correct, and the fourth explains the first. 1. A master LITERAL SAT UNDER A 2.58.0 PIN. The ThresholdExceeded format string read "(%.1f%% of %dB limit)"; at the pinned tag it is "(%.1f%% of %dB total)". `limit` is master's spelling, associated with the newer swap-aware implementation. So the pin caught count_swap_in_memory_monitor contamination and then admitted the same class through a different field. 2. THE CITATION DID NOT COVER THE MODULE, WHICH IS WHY (1) SURVIVED FIFTEEN GREEN WITNESSES. One ExternalModelScope pointed at ray_config_def.h with empty further_citations, while the module asserted facts from the factory body and the monitor implementation as well. The witnesses compare authored values against authored expectations; nothing in them establishes external-source fidelity. Only a citation naming the file each fact came from can. The authority is now partitioned: ray_config_def.h for the defaults, multi_memory_monitor_factory.cc for ray_built_monitors, threshold_memory_monitor.cc for ray_monitor_log_format. 3. COMPILED DEFAULTS WERE PRESENTED AS THIS FLEET'S EFFECTIVE CONFIGURATION. I wrote that an empty RAY_ environment established the recorded values were "in force on this fleet" and that there was no deployment-layer choice. That entailment does not hold: Ray reads the environment AND THEN applies a JSON config list through RayConfig::initialize, so an empty environment rules out one input and not the other. The observation was real and the conclusion was larger than it -- the same shape as the other defects here. The fleet reading is removed from this module entirely; which defaults survive into an effective configuration is a join over an observed environment and an observed config list, and both are receipts about our deployment rather than facts about Ray, so they belong in gunbc.spark where an unobserved config list is a refusable coverage obligation rather than a silence that reads as zero. Also in (3): min_memory_free_bytes was enumerated as a config and consumed by the factory's threshold calculation while its default was absent from the defaults record -- a partial surface presented as a complete one. Its -1 is upstream's DISABLED marker, not a quantity, so it is an ARM: carried as a signed byte count, "no floor configured" and "a floor of minus one byte" would be the same value. 4. THE THREE-CAUSES CLAIM WAS STRUCTURALLY UNREPRESENTABLE, not merely overstated. The function takes a LINE while the safe case is the ABSENCE of any line, so a function over present texts can never represent "nothing was printed". Renamed to ray_line_explains_absent_threshold_alarm and narrowed to what it can decide: which lines, WHEN PRESENT, explain an absent alarm. Deciding BelowThreshold against ObservationFailed against Disabled needs a carrier for an observation OUTCOME -- a reader's fact about a captured log, not a property of a line -- so it belongs to whatever consumes the capture, and this module does not fabricate it. The isolated path's own diagnostic (UserSliceSnapshotFailed) is added, because the type claimed to enumerate the ways of seeing nothing and was missing one. Witnesses 17/17. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact-head re-review at f0cf2cb.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head f0cf2cbe3649e17f36ae0e246444bcfc9038e6aa.
The four repairs described in the latest commit are accepted: the tagged trigger literal now says total; compiled defaults are no longer presented as effective fleet configuration; min_memory_free_bytes = -1 is a semantic disabled arm; and the line-classifier is narrowed to present diagnostic lines with the isolated snapshot-failure state represented. Partitioning the factory and log-format claims onto their actual source files is also the right direction.
Five blockers remain:
-
The new isolated-path format is not the tagged format.
UserSliceSnapshotFailedrecordsFailed to take user and system slice memory snapshot. The threshold memory monitor...; atray-2.58.0the format isFailed to take user and system slice memory snapshot due to: %s. The threshold memory monitor.... Because the function isray_monitor_log_format, preserve the%sformat slot, or deliberately rename/model a prefix-pattern rather than claiming the format. -
The citation partition is still incomplete. The new scopes cover defaults, the Linux factory body, and selected log formats, but
ray_config_env_spellingremains an upstream claim fromray_config.hwith no scope, andray_config_absencesstill asserts the master/tag contrast without a master authority or a scope for that row. The mutable statements that the Spark hosts have 16 GiB of active swap, that Ray was read from the running container, and that the hosts lack Ray also remain in extdeps comments/ground. Keep the pin, but move fleet observations downstream and cite each surviving external declaration at the file/revision that governs it. -
ray_built_monitorsis still a Linux implementation observer with a total-looking name. Its sole authority ismulti_memory_monitor_factory.cc; the tagged BUILD selects a different noop-only implementation off Linux. Rename/scope it asray_linux_built_monitors, or model the exact tagged platform dispatch from BUILD plusnoop_memory_monitor_factory.cc. -
The disconnected pressure verdict remains.
ray_pressure_monitor_is_factory_selectable() -> falseand its witness still prove only an authored Boolean. The closedRayBuiltMonitorconstruction already excludes pressure. Delete the function and witness, or derive the negative from a modeled exhaustive platform/factory construction rather than asserting it beside that construction. -
The canonical body is still on the superseded design. It reports 11/11, says the factory witness covers three platform/isolation combinations, and presents the empty environment as evidence of effective defaults. The current head reports 16 property rows plus aggregate = 17/17, has no platform arm, and separates compiled defaults from deployment configuration. Rewrite the body before merge.
Exact-head CI is still nonterminal. No objection remains to the threshold-specific module boundary, the zero-refresh [Event, Noop] construction, the tagged count_swap_in_memory_monitor absence, or the new default/silence carriers.
Two things, both anticipating findings rather than waiting for them, and the second is a rule this module has now had to apply three times. THE SCOPE WAS LEFT IMPLICIT WHEN THE PLATFORM ARM WENT. Deleting the guessed non-Linux arm was right -- it came from a comment on master's header and the 2.58.0 body has no platform branch, so asserting it would have been a citation to a revision it was not read at. But the deletion left a reader of ray_built_monitors taking it for the whole story on every platform, when it is the LINUX story. What the factory does elsewhere is UNREAD, not known-to-be-the-same, and that distinction is the whole difference between a citation and a guess. Now said in the module. THE PRESSURE PREDICATE WAS A CHANGE DETECTOR AND IS DELETED. ray_pressure_monitor_is_factory_selectable took no arguments and returned false; its witness asserted the negation. No input discriminates a zero-argument constant, so that witness could only go red if someone edited the constant it reads. Worse than absent, because it would have been cited as coverage for a fact the TYPE already guarantees: RayBuiltMonitor has no pressure constructor, so a pressure monitor cannot be written into a built population at all. The state is unrepresentable, not merely refused -- construction over validation, and the check was the weaker restatement of the stronger guarantee. That is the third constant-shaped decoration removed from this module. The first was silence_is_evidence_of_safety, which answered false on every arm. The second was the disconnected vllm_observed_kv_pool_line in the neighbouring PR, which returned the enum value it was written to return. The rule that catches all three is one question asked before writing a check: is the check's RED authorable? For a constant it never is. Witnesses 16/16 -- one fewer than before, because a row was deleted rather than fixed, which is the correct direction when the row asserted something construction already holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
…trast, evict the fleet facts Three residuals from exact-head review 5119153361. 1. THE ISOLATED-PATH STRING WAS NOT THE FORMAT. It read "Failed to take user and system slice memory snapshot." where the tagged source has "...snapshot due to: %s." The function is named ray_monitor_log_format, so it must carry the FORMAT; dropping the slot silently turned it into a prefix while keeping the name that promises otherwise. Restored rather than renamed, because the format is what a consumer parsing captured stdout needs. 2. THE CITATION PARTITION WAS STILL SHORT BY TWO. ray_config_env_spelling asserts an upstream proposition -- that the environment name is RAY_ prefixed to the config field name -- which comes from the macro in ray_config.h and had no scope at all. And ray_config_absences asserts a claim about TWO revisions: present on master, absent at the tag. Citing only the tag leaves half of it ungrounded, which is a poor state for the one row whose entire job is to record a master-versus-tag contamination this module has now suffered twice. Both ends are named: the absence row cites the tagged header first and a master authority as a further citation. 3. FLEET FACTS ARE OUT OF EXTDEPS. Three claims about OUR deployment were still sitting in the citation -- that the Spark hosts carry 16 GiB of swap and are using it, that the version was read inside a serving container, and that the hosts have no Ray. Every one is true and none belongs here. An extdeps row states which revision it cites; it does not state who observed it, where, or what that observer's hardware looks like. Those are receipts about our fleet and their home is gunbc.spark, where an unobserved input is a refusable coverage obligation rather than a sentence in a comment. The absence row now says why the distinction between "checked and missing" and "nobody looked" matters, and leaves why it matters TO US to the consumer. Witnesses 16/16. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at the requested exact head 886f0b66c8c4461e67032d01a7963e31a326b103.
I read the complete five-part census in review 5119153361, the source and witness file at this SHA, and the relevant ray-2.58.0 upstream implementation. The PR has since moved to d7d92b74294b10d8063aefb3698b558a70360ad4 by integrating main; the compare contains no changes to either of this PR's two Ray files. This review is deliberately anchored to the requested SHA, not an approval of an unreviewed integration tree.
ACCEPTED:
- The isolated-path format now preserves
due to: %s.and matches the tagged implementation. ray_config_env_spelling_scopenow names the tagged ray_config.h macro.ray_config_absences_scopenow names both source ends; the new floating-master issue below is distinct from having no second authority at all.- The concrete fleet swap-use, container-version-read and host-installation observations are removed from the extdeps source/absence ground.
- The disconnected pressure Boolean and its witness are deleted. The source has fifteen individual test functions plus the aggregate, sixteen test functions total; I have not independently executed them.
- The previously accepted interval/isolation population, disabled sentinel and present-line classifier repairs remain intact.
REMAINING CENSUS: two source issues and one documentation/evidence issue.
-
LINUX SCOPE IS STILL ONLY PROSE.
ray_built_monitorsand the declaration named byray_built_monitors_scoperemain unqualified. The new end-of-file comment expressly says that a reader would take this as the story on every platform, but changes neither declaration nor input. At ray-2.58.0, src/ray/common/monitors/BUILD.bazel selects multi_memory_monitor_factory.cc for Linux and noop_memory_monitor_factory.cc otherwise; the latter returns only Noop. Rename the function and all binding references toray_linux_built_monitors, or add the actually cited platform dispatch. A further Linux-only sentence does not discharge this item. -
THE NEW MASTER ENDPOINT IS A MOVING BRANCH, NOT THE READ REVISION.
ray_config_master_authoritypoints to blob/master while the absence row records a source-version contrast. The missing second-source link is fixed, but the newly introduced authority still cannot identify which master content was compared. Pin the commit actually inspected and make the contrast refer to that observation, or remove the positive master contrast and retain the exact-tag absence. Do not guess a commit or silently recast a later master read as the earlier one. -
CANONICAL EVIDENCE STILL DESCRIBES THE PREVIOUS SHAPE. The live PR body still says 17/17 and 'Three scopes now'; the current source contains sixteen test functions and five ExternalModelScope rows. Its factory prose still uses the unqualified observer. Synchronize these claims, and remove the residual witness comment saying silent logs separate into 'three decidable causes': the repaired function classifies present lines only. The body may discuss a dated fleet incident as motivation, but must not promote that historical observation to current-fleet authority or imply the extdeps model proves live enforcement.
The latest-head workflow was still in progress when checked; neither the author's 16/16 report nor this source review is a terminal CI receipt. No merge, squash, deployment, or wet-execution authorization is given. GB10 and downstream fleet-carrier rulings are separate work and are not added to this PR's blocker census.
Superseded by exact-head review 5119371584 at requested SHA 886f0b6. The full five-part census was read and dispositions recorded: accepted format/macro/fleet/pressure repairs; remaining Linux declaration scope, reproducible master endpoint, and body/witness-comment currency. This dismissal is supersession, not approval; the replacement REQUEST_CHANGES remains active.
…ith no subject CI refused with 21 parse errors and every one is mine, in this file, lines 376-400: "source annotation names no subject: no module item follows". WHEN I DELETED ray_pressure_monitor_is_factory_selectable I LEFT ITS ANNOTATION BLOCK STRANDED AT END OF FILE. Section 4c admits a standalone leading // block only where it attaches to a module-scope declaration; at scope end there is nothing to attach to, and the parser refuses rather than discarding it. So the deletion that removed a decoration created an unattached block, which is the same class I repaired in someone else's file in #10425. The block now sits ABOVE the factory section it is about, so it attaches to the declaration it describes rather than trailing the module. WHY MY LOCAL RUN DID NOT CATCH IT, WHICH IS THE PART WORTH KEEPING. I verified with claim_batch on the witness entry, which resolves that entry's CLOSURE. The parse phase reads the WHOLE CORPUS. A green closure run is not evidence the corpus parses, and I treated it as though it were -- so my evidence was true and did not cover the claim I made with it. The instrument that does cover it is v1_src_dag_parse, which now reports 4887 file(s) parse-clean, and which I should have run before pushing a module whose diff is mostly annotations. Witnesses 16/16, unchanged: this moves prose and touches no declaration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact-head review at 5f99e49 after main integration and the annotation-attachment repair.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 5f99e4999c51d78b1808fabead82b429f34f03b7.
The new parse repair is accepted. Deleting the zero-argument pressure predicate left its 25-line annotation block at scope end; moving that block ahead of the factory declaration gives it a module item to attach to. Exact-head run 33939472158 is terminally green in all four jobs, so the whole-corpus parse failure is discharged. The distinction recorded in the commit is also correct: a green claim-entry closure did not establish whole-corpus parse, while v1_src_dag_parse did.
The substantive repairs already accepted at 886f0b66c8c remain intact: tagged format slot, five source-specific scope rows, compiled/default split, structural min-free sentinel, isolated non-measurement arm, derived env spelling, interval/isolation population, and deletion of the self-attesting pressure Boolean.
The remaining census from review 5119371584 is unchanged. The annotation move fixes attachment; it does not fix any of these three items:
-
Linux scope remains prose-only. The moved block now attaches to
RayBuiltMonitor, but the exported function and itsExternalModelScopesubject remain unqualifiedray_built_monitors. The exact tagged source cited by that scope is the Linuxmulti_memory_monitor_factory.cc; the tagged build selects a different noop-only implementation off Linux. Rename the function, scope subject, imports and witnesses toray_linux_built_monitors, or model the tagged platform dispatch. A declaration annotation saying the function is Linux-only does not make a non-Linux call unrepresentable. -
The master side of the contrast is still a moving branch.
ray_config_master_authoritystill points toblob/master, whileray_config_absencesrecords a source-version contrast. Pin the exact master commit that was inspected and make the contrast refer to it, or remove the positivepresent on masterclaim and retain the exact-tag absence. Do not substitute today's master for the earlier read. -
Canonical evidence is still stale. The PR body says
17/17and “Three scopes now”; the exact source has fifteen property tests plus the aggregate =16/16, and fiveExternalModelScoperows. The witness annotation abovew_the_lines_that_explain_an_absent_alarm_are_exactly_the_non_measurementsstill says a silent log separates into “three decidable causes”, despite the repaired function classifying present lines only. Synchronize those claims. The present-tense Spark swap-use statement in the body should be dated/identified as fleet motivation or removed as current authority; that mutable observation is no longer in extdeps source for the right reason.
No implementation objection is added beyond this established three-item census. Merge/squash authorization is withheld until those source and body repairs land on a freshly reviewed exact head.
…e misreading Exact-head review 5119153361 held four residuals. Three were already repaired at this head -- the snapshot log line carries "due to: %s.", ray_config_env_spelling and ray_config_absences have their own ExternalModelScope rows, and the self-attesting pressure-monitor constant and its witness are deleted. The fourth stands and is fixed here. ray_built_monitors WAS A TOTAL NAME OVER A BODY READ ONLY FROM THE LINUX FACTORY. I had already noticed this and answered it with a paragraph saying "It is the Linux story". That is the same defect I argued against on the GB10 nameplate one hour earlier: a comment cannot forbid a use that the name invites, because the name is what every consumer spells and the comment is what no consumer reads. The declaration is now ray_linux_built_monitors and its scope row's decl_name follows, so the restriction is carried by the thing a caller cannot avoid typing. The paragraph is rewritten rather than deleted, because after the rename its old sentence was FALSE -- it warned that a reader would take the name for every platform, which the name no longer says. It now records why the scope moved from prose into the name, which is the part a later reader needs. ray_built_monitor_enforces keeps its total name deliberately: it is a property of the three monitor kinds, not of the platform-specific selection among them. Witnesses 16/16. v1_src_dag_parse: 4887 file(s) parse-clean -- run before pushing this time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by head 3fa9df5; replacing with an exact-head review.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 3fa9df5fdefaff3383c476bb33dfc29e384f1df5.
The Linux-scope repair is accepted. ray_linux_built_monitors now carries the platform restriction in the declaration every caller must spell; the ExternalModelScope subject, imports, and all three witnesses follow it. Keeping ray_built_monitor_enforces unqualified is correct because it classifies monitor kinds rather than selecting a platform implementation.
The previously accepted repairs remain intact: due to: %s. is preserved in the isolated-path format; five source-specific ExternalModelScope rows exist; the self-attesting pressure predicate/witness are gone; compiled defaults are separated from effective fleet configuration; and the extdeps source no longer embeds the mutable swap/container observations.
Three items remain:
-
The master side of the absence contrast still floats.
ray_config_master_authoritypoints toblob/master, while the source and PR body assert thatcount_swap_in_memory_monitorexists on master. That URI identifies whatever master contains when read, not the revision actually inspected. Pin the exact commit read, or remove the positive master claim and retain only the exact-tag absence. Do not reconstruct a commit after the fact. -
The relocated source annotation now contains a false deictic claim. The block immediately before
RayBuiltMonitorbeginsEVERYTHING ABOVE IS READ FROM THE LINUX FACTORY PATH. Everything above includes the config defaults, env spelling, and threshold log formats, which are sourced from other files. The block is now attached to the following factory section, so rewrite this as the factory construction below being read from the Linux path (or equivalent). The attachment is fixed; the pointer inside the attached annotation is not. -
One witness annotation and one body sentence remain stale. The annotation above
w_the_lines_that_explain_an_absent_alarm_are_exactly_the_non_measurementsstill says a silent log separates intothree decidable causes, although the repaired function classifies only present diagnostic lines and cannot represent the no-line case. Narrow that annotation to the explicit-line claim. The PR body now correctly reports five scopes and 16 witnesses, but it still says in present tense that the Spark hosts have 16 GiB of active swap while later claimingNo fleet facts; date/identify that sentence as motivation or remove it as current authority.
Exact-head workflow 33941013405 is currently in progress. No merge, squash, deployment, or wet-execution authorization is given until the source/body repairs land and the resulting exact-head run is terminally successful.
…location falsified Three blockers from exact-head review 5119521858, all correct. 1. THE MASTER CONTRAST HAD NO IMMUTABLE SUBJECT. ray_config_absences asserted a field is present on master AND absent at the tag, with a second authority pointing at blob/master/... to ground the positive half. But master is not a revision -- it is whichever commit occupies the branch when a reader follows the link -- so the cited end could move while this authored row stayed fixed. That is the same undetectable wrongness the tag pin exists to prevent, re-entering through the CITATION instead of through the defaults, in the one module that had already been bitten by it twice. I took the delete arm rather than pinning a master commit, because the commit I actually read is not recoverable now and reconstructing a plausible one would be a fabricated citation, not a repair. The row keeps only the half grounded at a pinned revision -- absent from ray_config_def.h at ray-2.58.0 -- which is also the only half a consumer needs. further_citations is now empty. 2. MOVING THE ANNOTATION FIXED ATTACHMENT AND BROKE DEIXIS. The block I relocated to repair the parse failure opens "EVERYTHING ABOVE IS READ FROM THE LINUX FACTORY PATH". That was true where it was written and FALSE where I put it: above it now sit the ray_config_def.h defaults, the ray_config.h env spelling, and the threshold_memory_monitor.cc diagnostics, none of them factory material. So my parse repair traded a refused program for a silently wrong sentence. It now names the two declarations it governs instead of a region, which is the DESIGN section 3 rule about citing symbols and not positions -- a region pointer is invalidated by any edit that moves either end, and that is exactly what my own edit did. 3. THE WITNESS ANNOTATION CLAIMED MORE THAN THE FUNCTION DECIDES. It said a silent threshold log "separates into three decidable causes". The function consumes a RayMonitorLogLine, so it classifies only lines that are PRESENT; the no-line case is not one of its inputs and cannot be one of its verdicts. Narrowed to the executable claim: among the modeled lines, exactly the three non-measurements explain an absent alarm. Deciding what total silence means needs evidence the monitor was consulted at all, which is a fleet receipt and stays downstream. Witnesses 16/16. v1_src_dag_parse: 4887 file(s) parse-clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact-head review of 6d86cf6 after all three requested repairs moved the branch.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 6d86cf64facf807c1139407d6c5ae59dea8af00e.
All three blockers from review 5119521858 are substantively repaired:
ray_config_master_authorityis deleted;ray_config_absences_scope.further_citationsis empty; the modeled row now asserts only absence fromray_config_def.hatray-2.58.0. Choosing deletion rather than reconstructing an unread commit is correct.- The relocated factory annotation now names
RayBuiltMonitorandray_linux_built_monitorsinstead of sayingEVERYTHING ABOVE; the pointer is symbol-bound rather than region-bound. - The absent-alarm witness annotation now states that the function classifies present lines and cannot decide total silence.
The canonical PR body is also current: five scopes, sixteen tests including the aggregate, no floating-master proposition, and no present-tense Spark swap assertion. My prior claim that the body at 3fa9df5fdef still carried that swap assertion was based on stale evidence and is withdrawn.
Two source-annotation defects remain in the witness file:
-
The annotation above
w_the_swap_counting_knob_is_recorded_as_absent_at_this_revisionstill says ``count_swap_in_memory_monitorexists on master and not at this revision. That restores, in witness prose, the exact positive master proposition this head deliberately deleted for lack of an immutable subject. Narrow it to the grounded fact—absent at `ray-2.58.0`—and, if useful, describe only that an earlier unpinned read prompted the check without asserting what `master` contains. -
The repaired annotation above
w_the_lines_that_explain_an_absent_alarm_are_exactly_the_non_measurementssays the three non-measurements explain an absent alarm andthe two measurement lines do not. The modeled type has three measurement arms, and the witness checks all three:ThresholdExceeded,NodeMemoryAboveThreshold, andUserSliceMemoryAboveThreshold. Changetwotothree(or avoid the count).
These are annotation-only repairs; no model, body, scope, or witness-logic redesign is requested. Because source annotations participate in whole-corpus parsing, rerun the exact-head corpus/required CI after the edit. Workflow 33941839680 currently names this head and is pending, so it is not yet a terminal merge receipt.
…d its own arms Both from exact-head review 5119561850, both annotation-only, both real. 1. THE DELETED MASTER PROPOSITION SURVIVED IN WITNESS PROSE. The previous commit removed ray_config_master_authority and the "present on master" half of the contrast from the model, on the ground that master has no immutable subject. The witness annotation above w_the_swap_counting_knob_is_recorded_as_absent_at_this_revision went on asserting exactly that sentence. So the deletion moved the fact rather than removing it, and left it in the layer with the weakest citation discipline -- which is the failure the deletion was meant to close, reappearing one file over. Section 4c is explicit that an annotation is never evidence a machine claim holds, and the converse bites here: prose the compiler cannot read is also prose no gate will catch drifting. The annotation is now tag-only, matching the row it sits above: verified absent from ray_config_def.h at ray-2.58.0, prompted by an earlier unpinned read, claiming nothing about what master carries. 2. THE NARROWED ANNOTATION MISCOUNTED THE ARMS. I wrote "the two measurement lines do not" while ray_line_explains_absent_threshold_alarm returns false on THREE: ThresholdExceeded, NodeMemoryAboveThreshold, UserSliceMemoryAboveThreshold. The witness negates all three. Corrected to three. Worth noting the shape: I introduced this number in the same commit that narrowed the annotation for overclaiming, which is a transcribed count sitting next to the function that derives it -- the thing DESIGN section 6 says to name rather than copy. Neither change touches the model, the citation partition, the factory construction, the default carriers, or any witness body. Witnesses 16/16. v1_src_dag_parse: 4887 file(s) parse-clean -- re-run because these comments participate in parsing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact head 5e1271d. Both annotation-only blockers from review 5119561850 are repaired and will be re-reviewed on the current head.
briansrls
left a comment
There was a problem hiding this comment.
APPROVED at exact head 5e1271d1a3771e8b66a209e5b9ec9186fc80fef6.
Both annotation-only blockers from review 5119561850 are closed:
- The verified-absence witness is now tag-only. It asserts absence from
ray_config_def.hatray-2.58.0, identifies an earlier unpinned read only as motivation, and makes no proposition about current or historicalmastercontents. - The absent-alarm annotation now matches the executable partition: three non-measurement arms explain an absent alarm when present, and all three measurement arms are rejected by the predicate.
No model, citation partition, factory construction, carrier, or witness-body regression was introduced. The previously accepted ray_linux_built_monitors boundary, five source-specific scopes, tagged formats, compiled-default/effective-config split, structural min-free sentinel, and deletion of the self-attesting pressure predicate remain intact. The canonical PR body is current at five scopes and sixteen tests including the aggregate.
Content blockers: none. Squash authorization is conditional on terminal success of exact-head workflow 33942567634 with the head unmoved at 5e1271d1a37; its present pending state is not a negative verdict, but it is not yet a merge receipt.
…te the flag that disagreed with it
The operator asked what consumes this module. Nothing did. The only importer of
extdeps.ray.threshold_memory_monitor was its own witness test; every other hit was the module citing
itself. That is specification-without-execution: the witnesses execute, but they exercise the module
against itself, so no serving decision changed if the citation was wrong and none broke if it were
deleted. The brief said "extdeps/ray/memory_monitor WITH THE REFUSAL" and I had landed only the
citation, then spent five review rounds making it more correct without making it load-bearing.
IT WAS ALSO A MEANING FORK THAT DISAGREED. serving_engine decided whether Ray contributes an enforcer
from ray_monitor_disabled: Bool -- a parameter produced by nobody -- while the cited module answers
the same question from pinned ray-2.58.0 source. The two agreed only by accident, and on one
configuration they disagreed in the direction that ADMITS: "disabled" is spelled as a zero refresh
interval, and at a zero interval the factory swaps only the THRESHOLD monitor for a Noop while the
EVENT monitor, pushed before the interval is ever read, keeps enforcing whenever resource isolation
is on. The Bool answered "no enforcer" for a host that was being enforced, so a unified-pool Ray host
with the monitor apparently off admitted silently -- the exact class this refusal exists to stop.
WHAT CHANGED. ray_implied_enforcers and spark_memory_enforcer_admits no longer take a Bool asserting
Ray's state; they take the two arguments Ray's factory actually selects on, resource_isolation_enabled
and monitor_refresh, and derive the implied enforcer from ray_linux_built_monitors filtered by
ray_built_monitor_enforces. A caller can no longer assert whether Ray is enforcing; it supplies the
configuration and the citation decides.
THE JOIN IS any, NOT A MAP, AND THAT IS LOAD-BEARING. With isolation on and a live interval the
factory builds TWO enforcing monitors, event and threshold. One member per monitor would hand
MoreThanOneEnforcer { count: 2 } to every ordinary Ray host, turning a correct citation into a
refusal of the normal case. The population this module counts is ENFORCING SUBSYSTEMS over one pool,
and Ray is one of those however many objects it builds internally.
EVIDENCE, AND THE FALSIFICATION. New RED row
w_a_zero_refresh_with_isolation_on_is_still_a_live_ray_monitor covers the configuration the old shape
could not express. I falsified it rather than trusting a green: reverting the derivation to the old
"zero refresh means no monitor" semantics fails that row AND NOTHING ELSE, so it discriminates
exactly the behavior that changed and the other rows are insensitive to it by construction.
serving_engine 15/15, ray monitor 16/16, v1_src_dag_parse 4887 file(s) parse-clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 3bc0b07ffb6d40b5fd20e7fea5c11111a0f1c043.
The prior approval on 5e1271d1a37 is dismissed. Voiding it was correct: this head adds a load-bearing gunbc.spark.serving_engine integration that the extdeps-only review did not cover.
ACCEPTED DIRECTION:
- The operator's consumption question exposed a real gap. A self-tested extdeps module with no production importer could be wrong without changing a serving decision.
- Deleting the unproduced
ray_monitor_disabled: Boolis correct. The Ray factory actually selects fromresource_isolation_enabledplus the globalmemory_monitor_refresh_ms, and the zero-refresh/isolation-on case really is[Event, Noop], not no enforcement. - The new mutation row is a valid discriminator for the old
zero refresh => no Ray enforcersemantics. - Collapsing multiple Ray monitor objects to one top-level Ray subsystem can be the right cardinality grain; mapping every Ray object directly into the existing enforcer list would make ordinary
[Event, Threshold]look like two independent subsystems. But the current output identity and cause do not yet model that grain honestly.
THREE BLOCKERS:
- The event-only state is retyped as
RayThresholdMonitor, then inherits a threshold-specific accounting claim. For(ExecutorRay, resource_isolation_enabled=true, monitor_refresh=0), the cited factory builds[EventMonitorBuilt, NoopMonitorBuilt]; no threshold monitor exists.ray_implied_enforcersreduces that population withany(...)and returns[RayThresholdMonitor].spark_memory_enforcer_admitsthen emitsDeviceReservationCountingEnforcerOnUnifiedPool { kind: RayThresholdMonitor }becausememory_enforcer_counts_device_reservation(RayThresholdMonitor)is true.
That is a subject mismatch: an Event monitor watching cgroup memory.events has been relabelled as a Threshold monitor that polls memory snapshots, and the measured device-reservation-counting property of the latter is silently assigned to the former. The new witness does not catch this because enforcer_refused_with matches DeviceReservationCountingEnforcerOnUnifiedPool { kind: _ } and discards the exact kind.
Preserve the one-Ray-subsystem cardinality if that is the intended policy, but preserve its active composition too—for example a RayMemoryEnforcement { active_monitors: List<RayBuiltMonitor> } top-level member, or another shape that cannot call an event-only population a threshold monitor. Then derive the refusal from a property actually established for that composition. If the intended policy is broader—any live Ray memory enforcement on a unified pool refuses—name and ground that broader rule rather than routing it through counts_device_reservation(RayThresholdMonitor). Add a witness for the exact zero-refresh/isolation-on composition and exact refusal cause; do not wildcard the kind.
ray_built_monitor_enforcesis now load-bearing and has no external scope. The fiveExternalModelScoperows cover defaults, factory construction, env spelling, the tagged absence, and log formats. None namesray_built_monitor_enforces, yet serving admission now depends on its three authored arms: Threshold=true, Event=true, Noop=false. Its local witness compares that authored mapping to authored expectations; it does not establish upstream fidelity.
Ground this declaration at the tagged implementation sources (threshold monitor, event monitor, and no-op monitor), or change the carrier so enforcement/non-enforcement is structural and externally scoped at construction. This distinction is genuinely semantic: Ray's NoopMemoryMonitor::IsEnabled() returns true even though the class performs no monitoring, so enforces cannot be inferred mechanically from the method name alone.
- The refusal is still not on an actuating/chooser path, and its new inputs are still unproduced scalars. Before this PR, repository search showed
spark_memory_enforcer_admitsonly at its definition and witness. This PR's only new calls are again inserving_engine_witness_test; none of the changed production files invokes the verdict, andserving_converge_slice_wetdoes not consume it. The extdeps citation now reaches one production function, but that function's result still reaches no serving decision. Specification-without-execution moved one hop inward.
Wire EnforcerVerdict into the actual serving chooser/admission/convergence gate so a refusal prevents actuation. At that boundary, do not replace the old unproduced Boolean with independently authorable Bool + Millisecond: bind backend, isolation setting, and effective refresh in a typed desired/observed Ray configuration receipt for the same host/runtime, and refuse absent or ambiguous effective configuration. The extdeps module correctly says compiled defaults alone are not effective fleet configuration; the production consumer must preserve that boundary.
No regression is found in the previously approved tagged defaults/log formats, five existing citation scopes, Linux factory name, structural min-free sentinel, or tag-only absence row. Exact-head workflow 33945507926 is currently in progress, so there is also no terminal CI receipt yet.
No merge, squash, deployment, or wet-execution authorization is given.
…ccounting we never established
Exact-head review 5119848537, blocker 1. My previous commit fixed a wrong answer and introduced a
fabricated cause.
WHAT WAS WRONG. ray_implied_enforcers answered [RayThresholdMonitor] whenever anything enforced. At a
zero refresh with isolation on the factory builds [Event, Noop] -- THERE IS NO THRESHOLD MONITOR ON
THAT HOST. So the derivation relabelled the event monitor as the threshold monitor, and the next
function inherited the threshold monitor's device-reservation accounting for it. The refusal was
right about the host and invented its mechanism. That is worse than not refusing, because a named
cause gets read as a finding.
MY WITNESS COULD NOT SEE IT. enforcer_refused_with matched
DeviceReservationCountingEnforcerOnUnifiedPool { kind: _ } and discarded the kind, so it proved that
SOME counting refusal fired and never that the refusal named the mechanism present. A wildcard in an
oracle is the oracle declining to check the field. The helper now compares cause AND kind.
THE THIRD ANSWER IS NOT A false. Ray's event monitor fires off a cgroup event counter rather than a
byte comparison, and nothing read for this citation settles whether that accounting includes a device
reservation on a unified pool. A Bool has nowhere to put that: false asserts safety on the strength
of not having checked -- the fabricated-plausible-answer failure -- and true invents the measurement.
memory_enforcer_counts_device_reservation is replaced by
memory_enforcer_device_reservation_accounting returning
CountsReservation | ExcludesReservation | AccountingUnestablished, and on a unified pool the third
arm REFUSES under its own cause, UnestablishedReservationAccountingOnUnifiedPool. An enforcer whose
accounting is unknown is exactly as unsafe to admit as one known to double-count; the difference
belongs in the cause, not in the verdict.
MemoryEnforcerKind gains RayEventMonitor, because two mechanisms that measure different things are
two kinds. Whether anything enforces is still read from ray_built_monitor_enforces; WHICH enforcer it
is now comes from the composition, so the cardinality question and the identity question stay
separate. Ray still contributes at most one member.
FALSIFIED AGAIN: reintroducing the relabelling -- always answering [RayThresholdMonitor] -- fails
w_a_zero_refresh_with_isolation_on_is_still_a_live_ray_monitor AND NOTHING ELSE, so the repaired
oracle detects exactly the defect it was blind to.
serving_engine 15/15, v1_src_dag_parse 4887 file(s) parse-clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact-head re-review at 294b284 after the event-monitor accounting and exact-cause repair.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 294b284f17ca250e8a3aa40e47e966111d6c54bb.
The event/threshold subject repair is accepted:
MemoryEnforcerKindnow distinguishesRayThresholdMonitorfromRayEventMonitor.[Event, Noop]at zero refresh with isolation enabled becomes the event kind rather than a fabricated threshold kind.- device-reservation accounting is three-valued:
CountsReservation | ExcludesReservation | AccountingUnestablished. - the event-only unified-pool case refuses under
UnestablishedReservationAccountingOnUnifiedPool { kind: RayEventMonitor }, while the threshold case retains the distinct known-counting cause. - the new oracle checks both cause and exact kind, and the reported relabelling mutant is therefore a valid discriminator.
Keeping Ray as at most one top-level enforcement subsystem is acceptable for the present policy. When both Event and Threshold are active, choosing the actually-present Threshold mechanism as the sufficient accounting refusal is not a fabricated cause. This output is not a complete composition receipt, so a later consumer needing the full internal population must carry it then.
RULING ON THE TWO DEFERRED ITEMS:
Blocker 2 must land before this PR can merge. ray_built_monitor_enforces now controls whether [Noop] yields no implied enforcer and whether [Event, Noop] yields a live Ray subsystem. It is therefore a load-bearing external proposition, but none of the five ExternalModelScope rows names it. The local witness only compares the authored mapping with authored expectations. Perform the first-hand exact-tag read you described and ground the declaration in the Threshold, Event, and Noop implementation sources. State the semantic criterion precisely—e.g. capability to perform monitoring/invoke the kill callback—not IsEnabled(), because Ray's Noop returns true from IsEnabled() while doing no monitoring. The serving-engine prose that says the Event monitor fires from a cgroup event counter should be grounded by that same read or removed.
Blocker 3 may be a separate PR, but it remains a hard wet-execution gate. Forcing the effective-configuration carrier and chooser wiring into this PR before their observations exist would encourage another fabricated producer. I accept this PR as a separately landable policy-kernel construction once its remaining content/evidence blockers close. It must not claim that actuation is already protected: spark_memory_enforcer_admits still has no production caller, and its Bool + Millisecond inputs are not yet bound to a same-host effective configuration receipt. The next cut must supply the observed environment/config-list join, refuse absent or ambiguous effective configuration, and put EnforcerVerdict on the chooser/admission/convergence path before any wet run.
THREE CURRENT BLOCKERS:
-
External grounding for
ray_built_monitor_enforces, as above. This is not safely deferrable now that it decides admission inside the policy kernel. -
The wildcard-oracle defect survives on
MoreThanOneEnforcer.count.enforcer_refused_withnow correctly checks kind for accounting refusals, but still matchesMoreThanOneEnforcer { count: _ }and accepts any count whenwanted == "many". The row above says two enforcers refuse and the count is carried. A mutant returning 3 or 99 would remain green. Compare the exact count—both current two-enforcer rows should require2—rather than applying the new no-wildcard rule to one cause family only. -
Source/body evidence still describes the preceding shape. In
serving_engine.dag, the annotation still saysA Ray-backed executor brings the threshold monitor with it whether or not anyone declared it; that is false for zero refresh, where the factory yields either[Noop]or[Event, Noop]. The non-Ray control also still refers to amonitor flagthat no longer exists. Narrow those annotations to the factory-derived population.
The canonical PR body still opens as One extdeps module, omits RayEventMonitor, AccountingUnestablished, and the new exact refusal cause, and says the consuming bug is fixed and a caller can no longer assert the state. Update it to distinguish:
- what this head establishes: a cited factory model and a non-actuating serving policy kernel;
- what it deliberately does not yet establish: effective per-host Ray configuration and an actuating chooser consumer;
- the exact event-only refusal and its unestablished-accounting cause;
- the still-required downstream wet-path cut.
No regression is found in the previously approved tag pin, five existing scopes, log formats, Linux factory name, compiled-default/effective-config split, min-free sentinel, or tag-only absence row.
Exact-head workflow 33946488090 is pending, so there is no terminal CI receipt. No merge, squash, deployment, or wet-execution authorization is given.
…r the threshold one
Two findings from the in-flight review, both mine.
THE SECOND WILDCARD. Having just fixed an oracle that discarded the refusal's `kind`, I left the one
that discards its `count`. `enforcer_refused_with` matched MoreThanOneEnforcer { count: _ } and
answered on the cause alone, so it would have accepted a refusal reporting ANY number -- including
one, which is not "more than one" and would mean the population arithmetic had broken while the row
stayed green. The count is the entire content of that cause. Rows that mean two enforcers now say two,
through enforcer_refused_as_many, and the cause arm in the string oracle answers false so the count
cannot be asserted without reading it.
Falsified: reporting a wrong count fails exactly the two rows that assert it, and nothing else.
STALE THRESHOLD-SPECIFIC PROSE. Three sentences still said "the threshold monitor" where the model
now admits two Ray kinds -- most load-bearingly "a Ray-backed executor brings the threshold monitor
with it", which is FALSE: it brings a monitor, and which one is read from the citation rather than
assumed. Left standing, that prose is a second authority disagreeing with the code beside it, and it
is the half a reader trusts.
serving_engine 15/15, v1_src_dag_parse 4887 file(s) parse-clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
…mechanism story I had borrowed
BLOCKER 2, CLOSED WITH A FIRST-HAND READ. ray_built_monitor_enforces selects whether a serving host
counts as having a live memory enforcer, so once serving_engine consumed it the mapping sat on the
decisive branch while being verified only against expectations this repository also authored. It now
has its own ExternalModelScope citing the three implementation headers at ray-2.58.0.
THE CRITERION IS STATED BECAUSE THE OBVIOUS METHOD IS WRONG. It is CAN IT INVOKE THE KILL-WORKERS
CALLBACK, not IsEnabled(). NoopMemoryMonitor overrides IsEnabled() to `return true` while its Enable
and Disable bodies are empty and its own doc says it "does not perform any monitoring" -- so a
classifier keyed on IsEnabled would have reported the disabled configuration as enforcing, which is
the direction that admits a host nothing is watching. Read at the tag:
ThresholdMemoryMonitor "checks the memory usage periodically and invokes the callback"
EventMemoryMonitor monitors events "when the memory.high is reached or exceeded and trigger
the kill workers callback"
NoopMemoryMonitor "does not perform any monitoring"
I READ THESE MYSELF RATHER THAN RELAYING THEM. The same read confirmed, first-hand, three things this
module had asserted on weaker evidence: the factory pushes the event monitor BEFORE reading the
interval and then selects threshold-versus-noop on it; the disabled log line matches the modeled text
byte for byte; and no pressure monitor is constructible from this factory, which is what makes
RayBuiltMonitor's closed set closed.
AND I DELETED THE PART I COULD NOT SOURCE. My previous commit explained the unestablished accounting
by saying Ray's event monitor "fires off a cgroup event counter rather than a byte comparison". I got
that from a peer's report, not from the source, and asserting it in an extdeps citation is precisely
the failure this module exists to prevent -- so it is gone from both files. The honest fact needs no
mechanism story: THIS REPOSITORY HAS NOT ESTABLISHED whether that accounting includes a device
reservation on a unified pool. AccountingUnestablished stands on that alone.
Also stale-prose repairs the review named: the non-Ray control no longer speaks of a "monitor flag"
that no longer exists.
serving_engine 15/15, ray monitor 16/16, v1_src_dag_parse 4887 file(s) parse-clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact-head review of 539c94e.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 539c94e9903d343a77dbe4f5af66077247427807.
The four previously required merge items are closed:
ray_built_monitor_enforcesnow has its own sixthExternalModelScope, grounded at the exactray-2.58.0Threshold, Event, and Noop headers. The criterion is correctly stated as capability to perform monitoring and invoke the kill-workers callback, notIsEnabled(): the tagged Noop header says it performs no monitoring while returning true fromIsEnabled().- both
MoreThanOneEnforcerwitnesses now inspect and require the exact count2; the generic cause oracle no longer accepts that arm without reading its payload. - the stale threshold-only and deleted-monitor-flag prose is repaired.
- the canonical body now accurately distinguishes the cited factory model and non-actuating policy kernel from the still-missing effective-configuration receipt, production consumer, and wet gate.
The first-hand source partition is accepted. Removing the borrowed Event-accounting mechanism story was also correct: AccountingUnestablished needs only the honest fact that this repository has not established the relevant accounting.
The effective-configuration carrier and chooser/fence integration remain separately landable, exactly as previously ruled, but they remain a hard precondition for any wet run. They are not re-added as a merge blocker for this policy-kernel PR.
One new substantive blocker appears now that the factory mode and accounting function are read together:
The resource-isolation fork is discarded before threshold-monitor accounting is decided. The exact tagged Threshold header says that when resource_isolation_enabled is true, the Threshold monitor changes mode and monitors only user-application memory; the implementation selects the user-slice path instead of the host-memory path. This PR passes resource_isolation_enabled into ray_linux_built_monitors, but after selecting RayThresholdMonitor it drops that dimension. memory_enforcer_device_reservation_accounting(kind) then maps every RayThresholdMonitor to CountsReservation.
The witness makes the unsupported extension executable: w_a_live_ray_monitor_on_a_unified_pool_refuses supplies resource_isolation_enabled: true, positive refresh, and requires counts:ray_threshold. The measurement prose in serving_engine says the node memory reading included the GPU reservation; it does not establish that the resource-isolated user-slice accounting does too. Thus the branch applies a host-path observation across a source-defined change of subject and turns an unmeasured arm into CountsReservation—the same unknown-as-known shape that the new Event arm correctly refuses.
Preserve the mode through the accounting decision: split the threshold kind by monitoring locus, or pass the resource-isolation state into the accounting classifier. The non-isolated node path may retain CountsReservation if that is the measured subject; the isolated threshold path must be AccountingUnestablished unless independently grounded. Add a positive-refresh/isolation-on witness that checks the exact resulting cause. Keeping Event + Threshold as one top-level Ray subsystem remains acceptable; this finding is about the accounting attached to the selected sufficient mechanism, not about subsystem cardinality.
Two evidence identities should be repaired in the same source pass:
w_only_rays_monitor_counts_the_device_reservationno longer names what its body proves: one Ray kind counts whileRayEventMonitoris explicitly unestablished. Rename it to the actual four-way accounting partition.w_the_same_configuration_admits_once_the_monitor_is_offis not the same configuration as the adjacent refusal—it changes the isolation setting and carries a declared guard—and its annotation saysthe row aboveeven though the immediately preceding row is now the accounting-partition witness. Keep the useful control, but name the exact Noop-only Ray configuration and remove the positional pointer.
Exact-head workflow 33947335131 is still pending with no conclusion. No merge, squash, deployment, or wet-execution authorization is given.
…ng dropped between them NEW BLOCKER FROM REVIEW 5119954985, AND IT IS THE DEFECT I JUST FIXED FOR EVENT, ONE KIND OVER. The tagged header says it plainly: with resource isolation enabled the threshold monitor "will only monitor user application memory usage"; with it disabled it reads node memory. Those observe different populations. The measurement this whole module was built around -- a node-memory reading that includes the GPU reservation -- is about the NON-isolated path, and nothing established that a resource-isolated user slice charges that reservation the same way. WHAT WAS WRONG. resource_isolation_enabled was consumed only to decide WHICH monitors the factory builds and then discarded, so every threshold monitor received CountsReservation whichever population it was actually watching. That extends a measured fact to an unmeasured subject -- exactly what AccountingUnestablished exists to refuse -- and my witness made the unsupported extension executable by asserting counts for the isolated configuration. REPAIRED BY SPLITTING THE KIND rather than adding a parameter to the accounting function. MemoryEnforcerKind now carries RayHostThresholdMonitor and RayUserSliceThresholdMonitor, so accounting stays a TOTAL FUNCTION OF THE KIND -- the property this module already argued for when it deleted the declared counts_device_reservation field -- and the refusal names which of the two threshold subjects is live. The isolation flag is threaded through the selection to the member instead of dying at it. The non-isolated node path keeps CountsReservation because that is the measured subject; the isolated path returns AccountingUnestablished until separately grounded. Two rows now, where there was one: isolation off with a live interval is the counting refusal, isolation on with a live interval is the unknown-accounting refusal. Falsified by collapsing the fork back to a single threshold kind, which fails the isolated row AND NOTHING ELSE. TWO STALE EVIDENCE IDENTITIES, both real. w_only_rays_monitor_counts_the_device_reservation had stopped describing its body, which is now a partition over five kinds and three accounting answers rather than a claim that one monitor counts. It is w_the_accounting_partition_over_every_enforcer_kind. w_the_same_configuration_admits_once_the_monitor_is_off was not the same configuration as any refusing row -- it changes the isolation setting AND supplies a declared guard -- and its annotation pointed at "the row above", which moved when a row was inserted. It is now named for what it proves, that a declared guard admits when the factory yields only a Noop, and the positional pointer is gone. A positional pointer in evidence decays exactly like a positional citation in source, which is the same lesson this branch already learned once when a relocated annotation falsified its own deixis. serving_engine 16/16, v1_src_dag_parse 4887 file(s) parse-clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact head 6d66af8; replacement review follows.
briansrls
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES at exact head 6d66af865b4e30af0aaf4b881564614376b5c9e9.
The substantive threshold-mode repair is accepted:
MemoryEnforcerKindnow distinguishesRayHostThresholdMonitor,RayUserSliceThresholdMonitor, andRayEventMonitor.resource_isolation_enabledsurvives factory selection into the threshold member identity instead of being discarded.- the measured non-isolated node subject retains
CountsReservation; the isolated user-slice subject and Event subject areAccountingUnestablished. - the positive-refresh/isolation-on row requires the exact
UnestablishedReservationAccountingOnUnifiedPool { kind: RayUserSliceThresholdMonitor }cause, while the non-isolated row requires the exact host-threshold counting cause. w_the_accounting_partition_over_every_enforcer_kindandw_a_declared_guard_admits_when_the_factory_yields_only_noopnow describe what they execute, and the positional evidence pointer is gone.
Keeping accounting total over the split kind is the right choice; adding the isolation flag back as an independent accounting argument would have partially recreated the caller-supplied authority this lane removed.
Two prose/evidence blockers remain:
-
The source annotation immediately above the accounting function still describes the pre-split partition. It says
The threshold monitor polls a byte total that includes the reservationand thenRAY'S EVENT MONITOR IS THE ONE WE HAVE NOT ESTABLISHED. On this head onlyRayHostThresholdMonitorcarries the established counting result; bothRayUserSliceThresholdMonitorandRayEventMonitorare unestablished. Those sentences re-fuse the threshold subjects in prose and falsely make Event the sole unknown. Narrow the measured statement to the non-isolated host-threshold kind and state both unestablished kinds. No model change is requested. -
The canonical PR body still names the deleted constructor as current. Under
An exact threshold/event distinctionit saysMemoryEnforcerKind carries RayThresholdMonitor and RayEventMonitor separately.RayThresholdMonitorno longer exists on this head. Replace that current-state sentence with the three exact identities—host threshold, user-slice threshold, and Event—and their accounting boundary. The historical paragraph about the earlier singleRayThresholdMonitormay remain historical.
The policy-kernel / effective-configuration / wet-gate split remains accepted: the downstream observed-effective-configuration carrier and chooser/fence integration are separately landable but still mandatory before any wet run.
Exact-head workflow 33948251516 is currently pending with no conclusion. No merge, squash, deployment, or wet-execution authorization is given.
…split into Both items from exact-head review 5120004532, annotation and body only. No type, function, refusal or witness body changes. THE ANNOTATION DESCRIBED THE PRE-SPLIT PARTITION. It spoke of "the threshold monitor" as one counting subject and then named the EVENT monitor as the sole unknown. After the split both sentences are false at the current grain: only the non-isolated host threshold monitor has an established counting result, and the isolated user-slice threshold monitor is unknown alongside the event monitor. So the prose re-fused in words exactly the two subjects the type had just been separated into, and the annotation disagreed with the executable partition about the very distinction the split existed to make. It now states the established side as one kind and the unestablished side as two, and records that naming only one unknown was itself the defect. THE BODY NAMED A DELETED CONSTRUCTOR. It described the split correctly in one section and then went on, in the present tense, to say MemoryEnforcerKind carries RayThresholdMonitor -- a constructor this head removed. The historical paragraph about the earlier single-kind version stays, because that is narration of what changed; the present-tense sentence now names the three current Ray identities and which of them has an established accounting answer. This is the third time on this branch that prose outlived the model it described: EVERYTHING ABOVE after a relocation, a witness annotation asserting a deleted master proposition, and now a partition sentence surviving the partition. The common shape is that annotations are invisible to every instrument -- the parse checks that they ATTACH, never that they are TRUE -- so each one is only ever caught by a reader. serving_engine 16/16, v1_src_dag_parse 4887 file(s) parse-clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G
Superseded by exact head 3bf1a4e; both prose/body blockers were repaired without changing the accepted model or witnesses.
briansrls
left a comment
There was a problem hiding this comment.
APPROVED at exact head 3bf1a4eb7c96badbf16264ddd158d4b550d8ec3c.
Both prose/evidence blockers from review 5120004532 are closed:
- The source annotation now matches the executable accounting partition: only
RayHostThresholdMonitorcarries the establishedCountsReservationresult; bothRayUserSliceThresholdMonitorandRayEventMonitorare explicitlyAccountingUnestablished. - The canonical body now names the three current Ray identities in the present tense. The deleted
RayThresholdMonitorspelling survives only in historical narration of the defect.
The head movement is limited to the accounting annotation; no type, function, refusal, or witness body changed. The previously accepted split-kind accounting, exact causes, six source-specific scopes, exact kind/count oracles, policy-kernel boundary, and explicit absence of wet-path protection remain intact.
Content blockers: none.
Exact-head workflow 33948624751 is currently pending with zero materialized jobs. That is queue/control-plane state, not a negative branch verdict, but it is not a merge receipt. Squash authorization is conditional on terminal success of that run with the head unmoved at 3bf1a4eb7c9.
This approval does not authorize wet execution. The observed-effective-configuration carrier and chooser/admission/convergence consumption remain mandatory before the wet run.
What this is
A cited model of Ray's memory-monitor factory at
ray-2.58.0, and the serving-policy kernel that consumes it. It started as an extdeps citation alone; that was specification-without-execution, and the consumer is what makes it load-bearing.What this PR establishes
A cited Ray factory model.
extdeps.ray.threshold_memory_monitor, pinned atray-2.58.0and read from source at that tag, with sixExternalModelScoperows naming the file each proposition came from:RayThresholdMonitorDefaultsray_config_def.hray_linux_built_monitorsmulti_memory_monitor_factory.ccray_built_monitor_enforcesthreshold_/event_/noop_memory_monitor.hray_monitor_log_formatthreshold_memory_monitor.cc+ factoryray_config_env_spellingray_config.hray_config_absencesray_config_def.hat the tag (verified negative)A bug fix in the serving decision.
serving_enginedecided Ray's enforcer contribution fromray_monitor_disabled: Bool— produced by nobody, and a second authority for what the citation answers. They disagreed in the direction that admits: "disabled" is a zero refresh, and at a zero interval the factory swaps only the threshold monitor for a Noop while the event monitor — pushed before the interval is ever read — keeps enforcing. So a unified-pool Ray host with the monitor apparently off admitted silently. It now refuses.The threshold monitor split into its two real subjects. The tagged header says that with resource isolation enabled the threshold monitor "will only monitor user application memory usage"; with it disabled it reads node memory. The measurement this module was built around — a node-memory reading including the GPU reservation — is about the non-isolated path.
MemoryEnforcerKindtherefore carriesRayHostThresholdMonitor(CountsReservation, the measured subject) andRayUserSliceThresholdMonitor(AccountingUnestablished) separately, so the isolation flag survives into the accounting decision instead of dying at monitor selection.An exact per-mechanism distinction. Ray contributes one of three identities —
RayHostThresholdMonitor,RayUserSliceThresholdMonitor,RayEventMonitor— and only the first has an established counting result; the other two areAccountingUnestablished. A first version of the fix labelled the[Event, Noop]population as a threshold monitor and inherited the threshold monitor's device-reservation accounting for it — right host, invented mechanism. The refusal now names what was actually built.A fail-closed unknown.
memory_enforcer_device_reservation_accountingreturnsCountsReservation | ExcludesReservation | AccountingUnestablished. Whether Ray's event monitor's accounting includes a device reservation on a unified pool is not established by this repository, and on a unified pool that refuses under its own cause. ABoolhad nowhere to put it:falseasserts safety on the strength of not having checked;trueinvents a measurement.The classifier's criterion is "invokes the kill-workers callback", not
IsEnabled().NoopMemoryMonitorreturnstruefromIsEnabled()while itsEnable/Disablebodies are empty and its own doc says it "does not perform any monitoring" — a classifier keyed on that method would report the disabled configuration as enforcing.What this PR does NOT establish
resource_isolation_enabledandmonitor_refreshare still freely authored. There is no observed-environment / observed-config carrier yet, and an unobserved config list should be a refusable coverage obligation.spark_memory_enforcer_admitshas no caller outside its witnesses.EnforcerVerdictis not wired into the chooser, admission fence, or convergence plan.ray_linux_built_monitorsis read from the Linux factory path; the name carries the restriction because a comment cannot forbid a use the name invites.Evidence
16 witnesses in
ray_threshold_memory_monitor_witness_test.dag(one aggregate), 16 inspark/serving_engine_witness_test.dag. Whole-corpusv1_src_dag_parse: 4887 file(s) parse-clean.Four claims were falsified rather than assumed — each mutant fails exactly the rows that assert it and nothing else:
w_a_zero_refresh_with_isolation_on_is_still_a_live_ray_monitorw_an_isolated_threshold_monitor_has_unestablished_accountingMoreThanOneEnforcer.countTwo oracle repairs came out of this: the helper wildcarded the refusal's
kind, and then itscount. A wildcard in an oracle is the oracle declining to check a field the claim says matters.🤖 Generated with Claude Code
https://claude.ai/code/session_01U397y4s3dSBof7vGPAX87G