jared/cc-os

Watch

Fork 0

Make ExpectedConstants unregister the loaders it creates #1420

New issue

Closed

opened 2026-10-27 16:37:32 +00:00 by jared · 3 comments

jared commented

2026-10-27 16:37:32 +00:00

Owner

Problem

ExpectedConstants leaves a Zeitwerk loader registered after every call.
loaded at plugins/os-sdlc/lib/os_sdlc/expected_constants.rb:46 runs load loader_path on the target's loader file.
Line 47 then collects every Zeitwerk::Loader from ObjectSpace whose dirs sit under lib_root.
Each run adds a new loader to Zeitwerk::Registry, and a second run also returns the stale loaders from the first run.
Callers: crc/census/settings.rb:47, crc/score/run.rb:26, and detector/{cycle_edges,error_lineage,holders}/signals.rb.

Reproduce

n/a: call ExpectedConstants.of twice on a fixture project and compare Zeitwerk::Registry loader counts.

Approach

After ExpectedConstants reads all_expected_cpaths and the root constant, it unloads and unregisters each loader it created.
It must not unregister a loader that existed before the call. When the target is os-sdlc itself, OsSdlc.loader may already be set.
Risk: unregister is marked @experimental in zeitwerk 2.8.3 (loader.rb:365).

Required work:

Unregister the loaders that ExpectedConstants created — check: present unregister in plugins/os-sdlc/lib/os_sdlc/expected_constants.rb

Acceptance criteria

Given a fixture project, when ExpectedConstants.of returns, then the Zeitwerk::Registry loader count equals the count before the call.
Given a fixture project, when ExpectedConstants.of runs twice in one process, then both runs return the same cpaths.

The tests use a fixture project and are red before the fix.
AC 3 ("root constant is unchanged") was removed on 2026-10-27: it describes existing behavior, so its test cannot be red.

Owner sketch

class OsSdlc::ExpectedConstants
  def loaded
    before = ObjectSpace.each_object(Zeitwerk::Loader).to_a
    load loader_path
    created = ObjectSpace.each_object(Zeitwerk::Loader).to_a - before
    covering = (registered(before) + created).select { |loader| inside?(loader) }
    dispose(created)
    covering
  end

  def dispose(created)
    created.each do |loader|
      begin
        loader.unload
      rescue Zeitwerk::SetupRequired
        nil
      end
      loader.unregister
    end
  end
end

all_expected_cpaths and dirs(namespaces: true) need no setup, so both still work on a disposed loader. A pre-existing loader (for example OsSdlc.loader when the target is os-sdlc) is selected but never disposed.

Origin

Trigger: comment on #1390.
Improvised this session: none
Chain: stale loaders in Registry ← loaded never unregisters its loaders ← DESIGN (expected_constants.rb:46)
Root candidate: none found; searched ExpectedConstants, unregister, Zeitwerk Registry, loader
Where: plugins/os-sdlc/lib/os_sdlc/expected_constants.rb
Session: c843bd27-b3bf-4e36-9243-df1697457146
Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os/c843bd27-b3bf-4e36-9243-df1697457146.jsonl

References #1390.
VERDICT: CREATE

## Problem `ExpectedConstants` leaves a Zeitwerk loader registered after every call. `loaded` at `plugins/os-sdlc/lib/os_sdlc/expected_constants.rb:46` runs `load loader_path` on the target's loader file. Line 47 then collects every `Zeitwerk::Loader` from `ObjectSpace` whose dirs sit under `lib_root`. Each run adds a new loader to `Zeitwerk::Registry`, and a second run also returns the stale loaders from the first run. Callers: `crc/census/settings.rb:47`, `crc/score/run.rb:26`, and `detector/{cycle_edges,error_lineage,holders}/signals.rb`. ## Reproduce n/a: call `ExpectedConstants.of` twice on a fixture project and compare `Zeitwerk::Registry` loader counts. ## Approach After `ExpectedConstants` reads `all_expected_cpaths` and the root constant, it unloads and unregisters each loader it created. It must not unregister a loader that existed before the call. When the target is os-sdlc itself, `OsSdlc.loader` may already be set. Risk: `unregister` is marked @experimental in zeitwerk 2.8.3 (`loader.rb:365`). Required work: 1. Unregister the loaders that `ExpectedConstants` created — check: present `unregister` in `plugins/os-sdlc/lib/os_sdlc/expected_constants.rb` ## Acceptance criteria 1. Given a fixture project, when `ExpectedConstants.of` returns, then the `Zeitwerk::Registry` loader count equals the count before the call. 2. Given a fixture project, when `ExpectedConstants.of` runs twice in one process, then both runs return the same cpaths. The tests use a fixture project and are red before the fix. AC 3 ("root constant is unchanged") was removed on 2026-10-27: it describes existing behavior, so its test cannot be red. ## Owner sketch ```ruby class OsSdlc::ExpectedConstants def loaded before = ObjectSpace.each_object(Zeitwerk::Loader).to_a load loader_path created = ObjectSpace.each_object(Zeitwerk::Loader).to_a - before covering = (registered(before) + created).select { |loader| inside?(loader) } dispose(created) covering end def dispose(created) created.each do |loader| begin loader.unload rescue Zeitwerk::SetupRequired nil end loader.unregister end end end ``` `all_expected_cpaths` and `dirs(namespaces: true)` need no setup, so both still work on a disposed loader. A pre-existing loader (for example `OsSdlc.loader` when the target is os-sdlc) is selected but never disposed. ## Origin - Trigger: comment on #1390. - Improvised this session: none - Chain: stale loaders in Registry ← `loaded` never unregisters its loaders ← DESIGN (expected_constants.rb:46) - Root candidate: none found; searched ExpectedConstants, unregister, Zeitwerk Registry, loader - Where: plugins/os-sdlc/lib/os_sdlc/expected_constants.rb - Session: c843bd27-b3bf-4e36-9243-df1697457146 - Transcript: /home/jared/.claude/projects/-home-jared-dev-cc-os/c843bd27-b3bf-4e36-9243-df1697457146.jsonl References #1390. VERDICT: CREATE

jared added the

plugin/os-sdlc

filed-by/user

category/code-change

labels

2026-10-27 16:37:32 +00:00

jared referenced this issue

2026-10-27 16:37:50 +00:00

Run CRC bins in process without a fork (InProcessBin) #1421

jared referenced this issue

2026-10-27 16:37:50 +00:00

Investigate why crc census and score load the target loader in process #1390

jared commented

2026-10-27 16:38:06 +00:00

Author

Owner

Zeitwerk 2.8.3 check (2026-10-27, read-only source review):

unregister is safe to use under these conditions:

Dispose only of loaders that this load call created (snapshot ObjectSpace.each_object(Zeitwerk::Loader) before the load, diff after). Never dispose of a pre-existing loader: when the target is os-sdlc, lib/os_sdlc_loader.rb guards with unless OsSdlc.loader, so the load creates nothing and OsSdlc.loader must stay selected and registered.
all_expected_cpaths and dirs(namespaces: true) need no setup; compute both from the same memoized loader list. Both work after disposal.
unload raises Zeitwerk::SetupRequired on a never-set-up loader: rescue it, then always call unregister.
ObjectSpace still returns unregistered loaders until GC; select from registered loaders plus the created list, never raw ObjectSpace.
unregister is @experimental; the regression test is the guard on each zeitwerk bump.

Extra tests worth having: a second ExpectedConstants on the same fixture project (no "already managed" error), and the os-sdlc-as-target case.

Zeitwerk 2.8.3 check (2026-10-27, read-only source review): `unregister` is safe to use under these conditions: 1. Dispose only of loaders that this `load` call created (snapshot `ObjectSpace.each_object(Zeitwerk::Loader)` before the load, diff after). Never dispose of a pre-existing loader: when the target is os-sdlc, `lib/os_sdlc_loader.rb` guards with `unless OsSdlc.loader`, so the load creates nothing and `OsSdlc.loader` must stay selected and registered. 2. `all_expected_cpaths` and `dirs(namespaces: true)` need no `setup`; compute both from the same memoized loader list. Both work after disposal. 3. `unload` raises `Zeitwerk::SetupRequired` on a never-set-up loader: rescue it, then always call `unregister`. 4. `ObjectSpace` still returns unregistered loaders until GC; select from registered loaders plus the created list, never raw ObjectSpace. 5. `unregister` is `@experimental`; the regression test is the guard on each zeitwerk bump. Extra tests worth having: a second `ExpectedConstants` on the same fixture project (no "already managed" error), and the os-sdlc-as-target case.

jared commented

2026-10-27 16:38:52 +00:00

Author

Owner

Work started on branch ticket-1420 (os-sdlc pipeline, poodr-ticket-implementation).

jared commented

2026-10-27 16:46:30 +00:00

Author

Owner

Resolution

Done: ExpectedConstants#loaded snapshots ObjectSpace loaders before load, disposes (unload, rescue SetupRequired, ensure unregister) each loader the load created, and selects from pre-existing plus created loaders. Pre-existing loaders such as OsSdlc.loader are never disposed.

Evidence: Commit 9e1f144f on ticket-1420, implementation 188: 8 dispatches all pass; suite gate exit 0; new tests/expected_constants_test.rb (Registry loader count unchanged; two runs return same cpaths) red before, green after.

Follow-ups: #1421 (InProcessBin without a fork) is now unblocked.

## Resolution **Done:** ExpectedConstants#loaded snapshots ObjectSpace loaders before load, disposes (unload, rescue SetupRequired, ensure unregister) each loader the load created, and selects from pre-existing plus created loaders. Pre-existing loaders such as OsSdlc.loader are never disposed. **Evidence:** Commit 9e1f144f on ticket-1420, implementation 188: 8 dispatches all pass; suite gate exit 0; new tests/expected_constants_test.rb (Registry loader count unchanged; two runs return same cpaths) red before, green after. **Follow-ups:** #1421 (InProcessBin without a fork) is now unblocked.

jared closed this issue

2026-10-27 16:46:30 +00:00

jared referenced this issue

2026-10-27 16:57:08 +00:00

Investigate why crc census and score load the target loader in process #1390

jared referenced this issue from a commit

2026-10-27 17:10:02 +00:00

feat(os-sdlc): Audited POODR boundaries for ExpectedConstants loader disposal; design and contracts hold. (#1420)