refactor(remediation): close phase-3 shared-core + phase-5 dispatcher blockers - #64
Closed
grubmeshi wants to merge 7 commits into
Closed
Conversation
… seam (BLOCKER 2) PLAN_DETAIL_03 §5.6 promised phase 4 an injectable metrics constructor so the run_controller_* collector could be built against a caller-chosen registry instead of the process-global singleton; the phase-3 run left only the singleton NewMetricsCollector(). Add NewMetricsCollectorWithRegistry(reg prometheus.Registerer) as the injectable seam and re-express the singleton NewMetricsCollector() as a thin default-registry wrapper over it, so every existing call site (and its duplicate-registration protection) keeps working unchanged. Wire the run-controller management listener (cmd/bbrunner) to construct one collector against a dedicated mgmt.NewRegistry and serve that registry, dropping the reliance on prometheus.DefaultRegisterer/DefaultGatherer. Metric names, labels and help strings are byte-identical (D12 scrape surface); only the registry object changed. Add a test proving two injected registries stay isolated and that the frozen series actually registers on the injected registry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cryptor (BLOCKER 2)
PLAN_DETAIL_03 step 8 intended the run-JSON decryption (Decryptor + DecryptRunDetails)
to live in the shared meshapi package; the phase-3 run left DecryptRunDetails as an
unexported, crypto-concrete function in internal/k8sjob and left tf carrying a duplicate
Decryptor interface.
- Add meshapi.DecryptRunDetails(runJsonBase64, dec Decryptor): the former
internal/controller/decryption.go logic, moved verbatim (five impl-type branches,
empty-value guards, unsupported-type error) but decoupled from the concrete crypto via
the Decryptor seam. Add meshapi.NewCertDecryptorFromCrypto to adapt an already-built
key pair. k8sjob's KubernetesJobDispatcher now calls the shared function; the k8sjob
decryption source + its full test suite move to meshapi (step 8 "tests move"), keeping
meshapi's own coverage of the new code ≥90.
- Alias tf.Decryptor to meshapi.Decryptor and tf.NoopDecryptor to meshapi.NoopDecryptor,
removing the duplicate declarations with zero assertion changes. tf keeps its own
non-empty-guarded certDecryptor so its polling decrypt behavior stays byte-identical
(meshapi.CertDecryptor's Kotlin decrypt("")=="" guard is port-persona semantics).
The crypto package's forcetypeassert paths were already made fail-safe (checked
assertions) in phase 7; no change needed there. Coverage: meshapi 91.5%, k8sjob 98.4%,
tf 90.2% -- all gates green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…m requester option (BLOCKER 3)
Shared dispatcher infrastructure the tf in-process cutover needs, all additive:
- mgmt.RunMetrics gains the two additive standalone counters the plan names
(PLAN_DETAIL_05 §16): runner_runs_unhandled_total{runner_uuid,type} and
runner_at_capacity_skips_total{runner_uuid}, with RunUnhandled/AtCapacitySkip methods.
Unhandled is deliberately NOT counted as runner_runs_failed_total.
- dispatch.Loop gains an optional StandaloneMetrics dep (structurally satisfied by
*mgmt.RunMetrics) fired at the at-capacity-skip and unhandled-type sites. nil for the
run-controller persona (its run_controller_* series already covers these), so controller
behavior is byte-identical.
- dispatch.NewRunClaimClient gains a WithRequester functional option so the tf persona can
stamp its frozen "<uuid>-worker-1" node-id (D9), which is uuid-first and does not fit the
default prefix-first "<prefix>-<uuid>" shape. Existing callers unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ns/registration config (BLOCKER 3) The dispatch framework + InProcess landed in phase 5 as dormant library code; the tf persona never got a dispatch.RunHandler, so phase 6's ports build against an interface with no real reference. Provide it: - internal/tf.NewHandler implements dispatch.RunHandler. Execute maps the claimed DTO to an internal Run with the cert Decryptor (polling decrypt semantics -- pins intact), builds a per-run RunApi authenticated with that run's own runToken (H5: never shared across concurrent runs), and drives the exact Worker.tfExecution machinery so register/ PATCH/artifact/metering wire behavior is byte-identical to the polling Worker. A run- scoped RunApi factory is injectable (tested over a fake RoundTripper). ctx shutdown cancellation is intentionally NOT propagated (equivalent to today's Manager; the plan's ABORTED-on-shutdown divergence is recorded, not adopted here). - internal/tf.NewClaimClassifier reproduces Worker.handleFetchRunError as a dispatch.ClaimClassifier (404/409 => no-run; chunked-transfer glitch => no-run; else => backoff + poll-error meter). - tf config gains additive maxConcurrentRuns (default 3, env RUNNER_MAX_CONCURRENT_RUNS; =1 reproduces the serial cadence) and an opt-in registration: section (displayName, ownedByWorkspace, publicKey, capability; nil => never self-registers, as today). Handler scenario tests reuse the polling suite's hermetic fixtures (MockedTfFacade, local git repo, fake transport) to prove APPLY-succeeded, tf-failure-reported, and mapping- failure-silent behavior + metering. tf coverage 90.4%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cher auto-detect (BLOCKER 3) Make the tf persona actually able to run on the dispatch framework, and give the run-controller/superset a dispatcher auto-detect, without disturbing the proven paths: - internal/tf.NewDispatchRunner assembles the tf dispatch stack (claim client with the frozen "<uuid>-worker-1" node-id, TERRAFORM handler, InProcess, Loop with maxConcurrentRuns + the tf claim classifier + the runner_* StandaloneMetrics) and runs the opt-in registration PUT. It returns the loop+dispatcher; the mains own the Start/signal/drain lifecycle (the ungated persona-wiring seam), keeping the assembly hermetically testable. - cmd/tf and cmd/bbrunner tf select the dispatcher via RUNNER_DISPATCHER=inprocess (opt-in); the legacy Manager/Worker loop stays the DEFAULT (equivalence not yet fully proven through the full characterization suite -- run-log addendum). The run_controller_* loop metrics register on the tf persona's existing dedicated registry via the §5.6 injectable seam. - cmd/bbrunner gains detectDispatcherKind (in-cluster via KUBERNETES_SERVICE_HOST => k8sjob; else => inprocess; RUNNER_DISPATCHER overrides), unit-tested. The controller still runs k8sjob for both in-cluster and out-of-cluster-via-kubeconfig (byte-identical, no regression); only an explicit RUNNER_DISPATCHER=inprocess fails fast, since the InProcess superset handler wiring (all five persona configs) is deferred (run-log addendum). tf 90.3%; all gates green; -race clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
controller.UseTestClient (PLAN_DETAIL_03 §12.2, moved to cmd/bbrunner in phase 4) is a dead switch: declared once, read in two places, written nowhere in the repo and bound to no env/config key, so it was always false. Removing it and the always-false branches it guarded (the registration short-circuit and the testMode log attribute) eliminates one of the remaining package-level mutable globals with zero behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e knobs - PLAN_IMPL_RUN_LOG_ADDENDUM.md: dated "Follow-up remediation (phase-3/5)" section closing BLOCKER 2 & 3, listing exactly what was closed vs. deferred (with reasons): kept the tf Manager as default (equivalence not fully proven through the whole characterization suite), deferred full tf.AppConfig de-global + tf->shared-report, and the InProcess superset controller wiring. (The real PLAN_IMPL_RUN_LOG.md is not tracked on this base branch, hence a companion file.) - CROSS_REPO_TODO.md: meshfed-release awareness for the additive tf dispatch knobs and the still-owed multiplexing-block-runner retirement. - containers/tf-block-runner/runner-config.yml: document maxConcurrentRuns and the opt-in registration: section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the two known BLOCKER gaps left by the autonomous refactor run (see
PLAN_IMPL_RUN_LOG.mdBLOCKERs 2 and 3). Stacked on top of #63 (base =refactor/single-go-binary/phase-7-cleanup). Every commit is green:task test(with-race),task lint, and all 11 coverage gates (tf 90.3, config 97.3, meshapi 91.5,report 98.8, mgmt 96.7, dispatch 95.7, k8sjob 98.4, manual 97.7, gitlab 92.2, azdevops
93.2, github 91.2).
A full "Follow-up remediation (phase-3/5)" section is in
PLAN_IMPL_RUN_LOG_ADDENDUM.md(the tracked run log is not on this base branch).BLOCKER 2 — phase-3 shared-core
Closed
dispatch.NewMetricsCollectorWithRegistry(reg)seam (§5.6); singletonNewMetricsCollector()kept as a default-registry wrapper. Controller mgmt listener +tf
/metricsuse the injectable seam; metric names/labels byte-identical.DecryptRunDetails+Decryptormoved intomeshapi(step 8), decoupled from concretecrypto;
k8sjobdelegates; decryption tests moved to meshapi.tf.Decryptor/NoopDecryptornow alias the shared types. Crypto
forcetypeassertalready fail-safe (phase 7).UseTestClientglobal removed (was never written). ControllerAppConfig/DiscoveredOidcIssuerwere already threaded by phase 4/7 — verified gone.Deferred (reasons in the addendum)
tf.AppConfigde-globalization (~180 sites, touches frozen wire pins + a largecharacterization suite) — new code here threads config explicitly, not via the global.
config/reportpackages into tf (PLAN_DETAIL_03's own "riskieststep 9" — rewrites tf's status model + every PATCH-body assertion).
BLOCKER 3 — phase-5 dispatcher (tf in-process cutover)
Closed
tf.NewHandlerimplementsdispatch.RunHandler(reusesWorker.tfExecution, per-runrunToken-only
RunApifor H5; byte-identical wire behavior). Scenario tests reuse thepolling suite's hermetic fixtures.
maxConcurrentRunsconfig +RUNNER_MAX_CONCURRENT_RUNS(default 3, documented).registration:section + WIF-less startup PUT (tf.Register).runner_runs_unhandled_total{runner_uuid,type}+runner_at_capacity_skips_total{runner_uuid}, driven by an optionaldispatch.LoopStandaloneMetricshook (nil for the controller => byte-identical).cmd/bbrunnerdispatcher auto-detect (KUBERNETES_SERVICE_HOST=> k8sjob; else =>inprocess;
RUNNER_DISPATCHERoverrides), unit-tested.tf.NewDispatchRunner, frozen<uuid>-worker-1node-id, tfclaim classifier), selectable via
RUNNER_DISPATCHER=inprocessincmd/tf+bbrunner tf.Deferred (kept safe, not deleted on faith — reasons in the addendum)
tf.NewManager+SetRunToken/ClearRunTokenNOT deleted: the Manager stays the DEFAULT,the new path is opt-in, because full equivalence (whole characterization suite driven
through the loop + N-concurrent acceptance smoke) is not yet proven.
RUNNER_DISPATCHER=inprocessoncmd/bbrunner)fails fast for now — it needs all five persona configs loaded; auto-detect mechanism +
the k8sjob paths are unchanged/byte-identical.
New CROSS_REPO_TODO entries
RUNNER_DISPATCHER,RUNNER_MAX_CONCURRENT_RUNS,registration:); no edits required (Manager stays default).is wired.
🤖 Generated with Claude Code