Skip to content

feat(cfg): add DISABLE_STARTUP_RECLAIM flag #3081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wj-e2b merged 1 commit into from
Jun 25, 2026
Merged

feat(cfg): add DISABLE_STARTUP_RECLAIM flag #3081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/orchestrator/pkg/cfg/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@

ClickhouseConnectionString string `env:"CLICKHOUSE_CONNECTION_STRING"`
ClickhouseConnectionStrings []string `env:"CLICKHOUSE_CONNECTION_STRINGS" envSeparator:";"`
DisableStartupReclaim bool `env:"DISABLE_STARTUP_RECLAIM"`

Check failure on line 84 in packages/orchestrator/pkg/cfg/model.go

View check run for this annotation

Claude / Claude Code Review

DisableStartupReclaim flag is never consumed 

The new `DisableStartupReclaim` field (env `DISABLE_STARTUP_RECLAIM`) is declared at `packages/orchestrator/pkg/cfg/model.go:84` but never read anywhere in the repo — a grep for `DisableStartupReclaim` / `DISABLE_STARTUP_RECLAIM` matches only the declaration and the new env-parsing test. The PR description says the field "is used to gate the startup resource-reclaim routine," but no startup-reclaim routine exists and no code consults `Config.DisableStartupReclaim`, so setting `DISABLE_STARTUP_RE

@claude claude Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The new DisableStartupReclaim field (env DISABLE_STARTUP_RECLAIM) is declared at packages/orchestrator/pkg/cfg/model.go:84 but never read anywhere in the repo — a grep for DisableStartupReclaim / DISABLE_STARTUP_RECLAIM matches only the declaration and the new env-parsing test. The PR description says the field "is used to gate the startup resource-reclaim routine," but no startup-reclaim routine exists and no code consults Config.DisableStartupReclaim, so setting DISABLE_STARTUP_RECLAIM=true is a silent no-op. Either land the consumer in this PR or note in the description that the flag is currently inert pending a follow-up.

Extended reasoning...

What the bug is\n\nThis PR adds Config.DisableStartupReclaim bound to DISABLE_STARTUP_RECLAIM at packages/orchestrator/pkg/cfg/model.go:84, plus a test at packages/orchestrator/pkg/cfg/model_test.go:83-89 that only verifies the env tag parses. The PR description claims the field "is used to gate the startup resource-reclaim routine" (present tense), but no consumer of the field is present in this PR or in the post-merge tree.\n\nEvidence (repo-wide grep)\n\nA repo-wide search for both identifiers returns only three lines — all in this PR:\n\n\npackages/orchestrator/pkg/cfg/model.go:84: DisableStartupReclaim bool `env:"DISABLE_STARTUP_RECLAIM"`\npackages/orchestrator/pkg/cfg/model_test.go:84: t.Setenv("DISABLE_STARTUP_RECLAIM", "true")\npackages/orchestrator/pkg/cfg/model_test.go:88: assert.True(t, config.DisableStartupReclaim)\n\n\nNo c.DisableStartupReclaim read exists anywhere. Searching for any "startup reclaim" routine also turns up nothing. The only reclaim machinery in packages/orchestrator is sandbox.bestEffortReclaim (pkg/sandbox/reclaim.go:83), invoked per-sandbox during pause/snapshot from pkg/sandbox/sandbox.go:1195, and it is gated by a LaunchDarkly feature flag (featureflags.GetReclaimConfig) — not by anything in Config. The cmd/resume-build CLI exposes its own --reclaim flag.Bool, again unrelated to this Config field. There is no startup-time reclaim path that this knob could plausibly gate.\n\nContrast with sibling field\n\nThe adjacent Config.ForceStop field is consumed in packages/orchestrator/pkg/factories/run.go (and surfaced in the job template HCL). DisableStartupReclaim has no analogous consumer in either code or IaC.\n\nStep-by-step proof of the no-op\n\n1. Operator sets DISABLE_STARTUP_RECLAIM=true expecting to skip a startup reclaim pass, as the PR title and description advertise.\n2. Orchestrator boots. cfg.Parse() runs and config.DisableStartupReclaim is correctly parsed to true — the new test confirms only this.\n3. The orchestrator proceeds with the rest of startup. No code path reads config.DisableStartupReclaim, so nothing is skipped, suppressed, or otherwise altered.\n4. Net effect: behavior is identical to the DISABLE_STARTUP_RECLAIM unset case. The operator gets no warning that their flag did nothing.\n\nImpact\n\nShipping an advertised operator-facing switch that silently no-ops is a footgun: anyone who flips it expecting the documented behavior will be misled. The test also gives a false sense of coverage — it only checks the tautology that env parsing works, not that any routine is actually gated.\n\nHow to fix\n\nEither (a) include the consumer that reads config.DisableStartupReclaim (and the startup reclaim routine it gates) in this PR, or (b) explicitly note in the PR description / a code comment that the flag is currently inert and is being landed ahead of a follow-up. Option (b) is acceptable as prep, but the present-tense claim in the description should be softened to match reality.\n\nAddressing the duplicate-refutation\n\nOne verifier refuted bug_004 as a duplicate of bug_001. That refutation is correct in the sense that the two reports describe the same defect — and the synthesis layer has already merged them into a single finding (this one). The merged report carries forward the strongest framing from both: the dead-config observation, the absence of any startup-reclaim routine to gate, and the ForceStop counter-example from bug_004. No double-reporting occurs — this is a single comment about a single field.

ForceStop bool `env:"FORCE_STOP"`
GRPCPort uint16 `env:"GRPC_PORT" envDefault:"5008"`
LaunchDarklyAPIKey string `env:"LAUNCH_DARKLY_API_KEY"`
Expand Down
8 changes: 8 additions & 0 deletions packages/orchestrator/pkg/cfg/model_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ func TestParse(t *testing.T) {
require.NoError(t, err)
assert.False(t, config.NFSProxyMetrics)
})

t.Run("startup reclaim can be disabled", func(t *testing.T) {
t.Setenv("DISABLE_STARTUP_RECLAIM", "true")

config, err := Parse()
require.NoError(t, err)
assert.True(t, config.DisableStartupReclaim)
})
}

func TestAdditionalClickhouseEndpoints(t *testing.T) {
Expand Down
Loading