mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 15:51:30 +00:00
b503d27b4f
Phase 9 ARCH-M2 closure Sprint 9. Splits internal/service/acme.go
(was 1965 LOC, the top hotspot after Sprints 1-8 finished the
config + main-binary cuts) via the Option B sibling-file pattern —
new files stay in `package service` so every external caller of
`service.ACMEService.{IssueNonce,LookupAuthz,ListAuthzsByOrder,
RespondToChallenge,GarbageCollect}` resolves the same way. Pure
mechanical relocation; no signature, no behavior, no import-graph
change.
Why Option B (not a subpackage)
================================
A subpackage (e.g. `internal/service/acme/`) would have meant
rebadging every public method receiver to its new package — that's
import-path churn for ~70 call sites across handlers, scheduler,
cmd/server wiring, MCP tools, and tests, plus the cyclic-import
risk of pulling acme back into `service` for the shared interfaces.
Option B sacrifices the encapsulation discipline a subpackage
would have given (sibling files can still reach into each other's
unexported state because Go scopes are per-package), but in
exchange the diff is restricted to file moves + four sed deletes;
zero importer touches anywhere outside this directory. The
trade-off matches every prior Sprint 1-7 config cut.
What moved
==========
New `internal/service/acme_nonces.go` (46 LOC)
----------------------------------------------
The IssueNonce method (RFC 8555 §6.5 Replay-Nonce issuance). The
nonceAdapter type — which wraps ACMERepo.ConsumeNonce for the JWS
verifier — stays in acme.go alongside VerifyJWS because it's
verification-infrastructure plumbing, not a server-issues-nonce
concern.
New `internal/service/acme_authz.go` (45 LOC)
---------------------------------------------
LookupAuthz + ListAuthzsByOrder (the authz read-side). Authz write-
side (status cascade after challenge validation) lives in
acme_challenges.go alongside recordChallengeOutcome where it
belongs operationally; the authz creation path stays inside
CreateOrder in acme.go (orders own per-order authz row creation).
New `internal/service/acme_challenges.go` (267 LOC)
---------------------------------------------------
The whole Phase 3 challenge dispatch + validator callback concern:
the `// --- Phase 3 — challenge dispatch + validator callback ---`
banner, the ChallengeResponseShape struct, the HTTP-facing
RespondToChallenge method (which transitions challenge → processing
and submits to the validator pool), and the asynchronous
recordChallengeOutcome callback (which persists final challenge
status and cascades the parent authz + order status). Largest
single extract this sprint by line count.
New `internal/service/acme_gc.go` (74 LOC)
------------------------------------------
The Phase 5 ACME GC sweep: scheduler-invoked GarbageCollect entry
point (3 sweeps: nonces, expired authzs, expired orders) and the
atomicAddUint64 counter helper (only consumed by the sweep body
for the rows-affected-N case the default `bump` doesn't cover).
What deferred
=============
Sprint 9 was originally scoped to ship 5 sub-files (nonces / authz /
challenges / orders / gc). The orders cut — CreateOrder +
LookupOrder + FinalizeOrder + LookupCertificate + the orders
helpers (randIDSuffix / base32encode / identifierStrings /
firstAvailableIssuer / accountOwnsACMECert / mapACMERevocationReason) +
FinalizeOrderResult — is ~700 LOC spread across multiple non-
contiguous regions in acme.go, with the orders helpers also feeding
into RevokeCert / RenewalInfo on the Phase 4 side. Disentangling
which helpers move with orders vs which stay with Phase 4 needs a
focused sprint of its own to avoid leaving a half-cut helper
declared in one file but called from a sibling — which works
(same package) but defeats the point of organising by concern.
Deferred to a potential Sprint 9b.
Net effect
==========
acme.go: 1965 → 1634 LOC (-331). Four new sibling files at 432 LOC
total. The headline 1965-LOC hotspot drops below the next-tier
candidates (mcp/tools.go, auth_session_oidc.go, cmd/agent/main.go).
Behavior preservation contract
==============================
1. gofmt -l clean across all 5 affected files.
2. go vet ./internal/service/... — no findings.
3. staticcheck ./internal/service/... — no findings.
4. go test -short -count=1 ./internal/service/... — green.
5. Broader-importer build green:
go build ./cmd/server/... ./internal/api/handler/...
./internal/scheduler/... ./internal/mcp/...
6. Broader-importer tests green:
go test -short -count=1 ./cmd/server/... ./internal/api/handler/...
./internal/scheduler/...
7. Per-import-symbol audit: all 8 imports remaining in acme.go
(context, cryptorand, x509, errors, fmt, strings, sync/atomic,
time, jose, internal/api/acme, internal/config, internal/domain,
internal/repository) verified used by surviving code. New
sibling files carry only the imports their extracted code needs.
The Option B sibling-file shape means same-package resolution
preserves access to ACMEService's unexported state from every
extracted method without any visibility tweaks. Worth noting for
the future: this also means a careless future caller could reach
through file boundaries and re-tangle concerns; the file headers
document the intended boundary but Go's tooling won't enforce it.
Why this is a partial sprint
============================
Splitting into 4 of 5 named sub-files now (vs blocking until orders
is also clean) keeps the hotspot count down with this commit and
lets a follow-up Sprint 9b focus exclusively on the orders cut
without re-touching the four files this sprint ships. Same
"smallest useful slice, document the rest" cadence as Sprint 8
splitting into 8a (mechanical) + 8b (behavior-aware).
Refs: ARCH-M2 (god-files), Phase 9 audit. Last in the config /
service hotspot chain before the agent + mcp + auth-session cuts
land in Sprints 10-12.
75 lines
2.6 KiB
Go
75 lines
2.6 KiB
Go
// Copyright 2026 certctl LLC. All rights reserved.
|
|
// SPDX-License-Identifier: BUSL-1.1
|
|
|
|
package service
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"sync/atomic"
|
|
)
|
|
|
|
// Phase 9 ARCH-M2 closure Sprint 9 (2026-05-14): extracted from
|
|
// internal/service/acme.go via the Option B sibling-file pattern.
|
|
// Package stays `service`; every external caller of
|
|
// `service.ACMEService.GarbageCollect(...)` resolves the same way —
|
|
// pure mechanical relocation.
|
|
//
|
|
// This file holds the Phase 5 ACME GC sweep concern: the scheduler-
|
|
// invoked GarbageCollect entry point plus the atomicAddUint64
|
|
// counter helper (only consumed inside the sweep body for the
|
|
// rows-affected-N case the default `bump` doesn't cover).
|
|
|
|
// GarbageCollect runs a single ACME GC sweep. Phase 5 — the scheduler
|
|
// invokes this every cfg.GCInterval. Three independent sweeps:
|
|
//
|
|
// 1. Delete used / expired nonces.
|
|
// 2. Transition expired pending authzs to `expired`.
|
|
// 3. Transition expired pending/ready/processing orders to `invalid`.
|
|
//
|
|
// Each sweep is a single SQL statement (no per-row transactions) so a
|
|
// large reap is one atomic write per sweep. Per-sweep errors are
|
|
// logged-and-continued: a failing nonces sweep doesn't block the
|
|
// authzs sweep. Returns the first error encountered (for caller
|
|
// telemetry); per-sweep counts are recorded on metrics regardless.
|
|
//
|
|
// Idempotent — repeated runs are safe; the second run finds 0 rows.
|
|
func (s *ACMEService) GarbageCollect(ctx context.Context) error {
|
|
s.metrics.bump(&s.metrics.GCRunsTotal)
|
|
var firstErr error
|
|
|
|
if n, err := s.repo.GCExpiredNonces(ctx); err != nil {
|
|
s.metrics.bump(&s.metrics.GCRunFailuresTotal)
|
|
if firstErr == nil {
|
|
firstErr = fmt.Errorf("acme gc: nonces: %w", err)
|
|
}
|
|
} else if n > 0 {
|
|
atomicAddUint64(&s.metrics.GCNoncesReapedTotal, uint64(n))
|
|
}
|
|
|
|
if n, err := s.repo.GCExpireAuthorizations(ctx); err != nil {
|
|
s.metrics.bump(&s.metrics.GCRunFailuresTotal)
|
|
if firstErr == nil {
|
|
firstErr = fmt.Errorf("acme gc: authzs: %w", err)
|
|
}
|
|
} else if n > 0 {
|
|
atomicAddUint64(&s.metrics.GCAuthzsExpiredTotal, uint64(n))
|
|
}
|
|
|
|
if n, err := s.repo.GCInvalidateExpiredOrders(ctx); err != nil {
|
|
s.metrics.bump(&s.metrics.GCRunFailuresTotal)
|
|
if firstErr == nil {
|
|
firstErr = fmt.Errorf("acme gc: orders: %w", err)
|
|
}
|
|
} else if n > 0 {
|
|
atomicAddUint64(&s.metrics.GCOrdersInvalidatedTotal, uint64(n))
|
|
}
|
|
|
|
return firstErr
|
|
}
|
|
|
|
// atomicAddUint64 adds delta to the counter. The metrics struct exposes
|
|
// only `bump` (add 1) by default; this helper covers the
|
|
// rows-affected-N case the GC needs.
|
|
func atomicAddUint64(c *atomic.Uint64, delta uint64) { c.Add(delta) }
|