Skip to content

Remove legacy top-level Configuration.Concurrency and dead set command - #1093

Open
evanphx wants to merge 2 commits into
mainfrom
evan/mir-796-remove-legacy-configuration-concurrency
Open

Remove legacy top-level Configuration.Concurrency and dead set command#1093
evanphx wants to merge 2 commits into
mainfrom
evan/mir-796-remove-legacy-configuration-concurrency

Conversation

@evanphx

@evanphx evanphx commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Vestigial code from before concurrency became per-service, noticed while reviewing #667.

  • cli/commands/set.go is gone. It defined m set -c <n>, writing the top-level Configuration.Concurrency. It was never registered in the dispatch table, so there was no way to invoke it — confirmed by grepping for any reference to commands.Set.
  • The HasConcurrency() checks in app_status.go are gone, in both the human and JSON paths. GetConfiguration never populated that field, so both checks were always false: the "Concurrency:" line could not print, and the concurrency key could not appear in --format json. The JSON field was omitempty, so nothing consuming that output sees a change.
  • concurrency and auto_concurrency are out of Configuration in api/app/rpc.yml, along with the now-unreferenced AutoConcurrency type. Indices 1 and 2 are left unused with a comment rather than reassigned, so an old client cannot read a new field through an old index.

Per-service concurrency (services.*.concurrency, SetConcurrencyMode / SetNumInstances) is untouched — that is the live path and this removes none of it.

go build ./..., go test ./cli/commands/... ./appconfig/... and go generate ./api/app/... are clean.

Closes MIR-796

@evanphx
evanphx requested a review from a team as a code owner August 27, 2026 21:05
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fd76c378-eede-462c-b150-b6182ce8350e

📥 Commits

Reviewing files that changed from the base of the PR and between 3d33bc5 and 3dd2b65.

📒 Files selected for processing (4)
  • api/app/app_v1alpha/rpc.gen.go
  • api/app/rpc.yml
  • cli/commands/app_status.go
  • cli/commands/set.go
💤 Files with no reviewable changes (1)
  • cli/commands/set.go

Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour.


📝 Walkthrough

Walkthrough

The RPC configuration contract removes AutoConcurrency, concurrency, and auto_concurrency. Legacy field positions remain reserved. Generated accessors and serialization support are removed. App status no longer displays concurrency in human-readable or JSON output. The Set command implementation is removed. JSON configuration output now contains only environment variable keys when present.

Merge Risk: ⚪ Minimal · up to 3dd2b

This PR removes unreachable legacy concurrency configuration and preserves per-service concurrency behavior; no actionable merge-blocking risk remains after normal checks and review.


Comment @coderabbitai help to get the list of available commands.

@miren-code-agent miren-code-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🍪 biscuit: ✅ ready to merge — auto-review, non-blocking

This is a clean, focused removal of two dead pieces: the top-level Configuration.Concurrency/AutoConcurrency fields (superseded by per-service concurrency) and the set CLI command that was the only consumer of those fields.

What I checked:

  • No surviving callers of HasConcurrency, SetConcurrency, AutoConcurrency, or commands.Set anywhere in the codebase — the search came back empty across all 400 files scanned.
  • The CBOR/JSON index reservation in rpc.yml is handled correctly: indices 1 and 2 are left as a gap (with an explanatory comment) rather than recycled, so old clients decoding a new payload won't misread a future field through a stale index. That's the right call for a binary-serialised protocol.
  • The generated rpc.gen.go matches the schema change exactly — the AutoConcurrency type and the two configurationData fields drop together cleanly.
  • app_status.go removes both the human-readable and JSON output paths for the old concurrency field, and correctly tightens the output.Configuration guard from cfg.Concurrency > 0 || len(cfg.EnvVars) > 0 to just len(cfg.EnvVars) > 0.
  • The remaining concurrency machinery (ServiceConfig.ConcurrencyMode, ServiceConcurrency in core_v1alpha, the appconfig per-service concurrency block) is untouched — this change is precisely scoped to the legacy top-level fields.

One minor cosmetic note: removing the three concurrency lines from app_status.go around line 106 left a blank line immediately after ctx.Printf("\n%s\n", ...). It's harmless but mildly untidy — not worth blocking on.

Overall the change is safe, well-scoped, and the protocol-safety reasoning in the comment is sound.


🍪 full review note · comment /biscuit review to run biscuit again.

@phinze phinze self-assigned this Aug 28, 2026

@phinze phinze left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 I chased the three claims and they all hold. Set never made it into RegisterAll, GetConfiguration never populated the field, and regenerating the schema gives back exactly what's committed.

The one thing I'd note for later is that the app.toml side of this outlives the wire side. appconfig.go:285 still parses a top-level concurrency that nothing reads, and it's fairly stuck there, because DisallowUnknownFields() means dropping the field would turn any app.toml still setting the key into a parse error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants