Fix Lifecycle Supervision Failures

I’m using the improve-codebase-architecture skill because this review spans API shape, duplication, adoption gaps, and behavior preservation across several repositories. I’ll first pin each repo’s actual branch/diff, then trace go-nuts against its callers and run focused validation.

The earlier go-nuts review history is relevant because it identified runtime lifecycle semantics as the main risk area, but I’ll treat those conclusions as stale until the current branches and callers are re-pinned. I’m now mapping each repository’s exact change surface and reading its domain/ADR constraints before judging the refactor.

All four checkouts are clean and currently match their configured origin/main. The refactor is already merged in entiredb and entire-api; go-nuts is at v0.6.0. I’m reconstructing the merged change ranges now, including the earlier mirror-pipeline adoption, so behavior comparison is against the pre-refactor implementations rather than only the final code.

The change has three distinct shapes: mirror-pipeline only moved the original connection/drain helpers from entwine to go-nuts (v0.3.0), while entire-api and entiredb removed substantial local NATS plumbing in favor of later go-nuts packages. I’m reviewing those separately so a safe module-path rename does not mask behavior changes in consumer lifecycle or tracing.

The shared module’s current lifecycle fixes are present on v0.6.0, including the earlier prefetch, concurrent-stop, transient-heartbeat, durable-validation, and multi-value-header fixes. The next check is caller completeness: I’m inventorying direct nats.go lifecycle, consumer, tracing, publish, and redelivery code that remains outside go-nuts, then distinguishing intentional domain logic from duplicated plumbing.

The first architectural finding is now concrete: ShutdownGroup.Go recovers a critical loop panic and leaves the process running, and mirror-worker uses it for both its main consumer and advisory backstop. That can produce a healthy-looking pod with its primary loop dead. I’m validating the rest of the behavior surface now; I’ll separate such runtime risks from adoption/simplification opportunities in the final review.

A second public-interface issue is confirmed: natsmsg.Publisher names spans with the full subject ("publish "+subject). These subjects often contain repo/account identifiers, while every existing publisher uses a stable operation name and keeps the subject as an attribute. That cardinality mismatch, plus its modern-JetStream-only seam, explains why the shared publisher remains unused in all three intended adopters.

I’ve finished the code-path comparison and the focused race suites. I’m doing the last lint/build and clean-worktree checks now, then I’ll give you the prioritized interface and adoption findings with a separate functionality-preservation verdict.

The refactor is directionally strong: go-nuts now has useful depth around connection lifecycle, consumer setup, tracing, heartbeats, and test doubles. I found no unintended change to subjects, durable names, consumer filters, AckWait/MaxDeliver, or Ack/Nak/Term behavior. I did find two important lifecycle risks and several areas where the public interface still limits adoption.

Findings

1. High: ShutdownGroup can leave a healthy-looking but nonfunctional process

   • Files: shutdown.go, mirror worker
   • Problem: Go catches panics and early loop returns, but only logs them. It neither cancels the group nor exposes the failure. Mirror’s readiness check only checks the NATS connection, so its main consumer can die while the pod remains ready.
   • Solution direction: Make the supervision policy explicit: critical loop failure should reach the owning module and normally make the process unready or initiate shutdown.
   • Benefits: Prevents silently dead consumers and gives the supervision interface real leverage beyond goroutine bookkeeping.

2. High: jsconsumer.Run retries permanent failures forever

   • Files: jsconsumer.go validation, Run
   • Problem: Local validation catches only three conditions. Invalid stream names, mutually exclusive filter settings, permissions failures, and other permanent consumer configurations all enter the same retry loop as a temporarily missing stream. The documented “configuration errors return immediately” contract is therefore incomplete.
   • Solution direction: Establish a clear seam between retryable provisioning races and permanent configuration/authorization failures.
   • Benefits: Misconfigured deployments fail visibly instead of running indefinitely without a consumer.

3. Medium: one-shot Start leaks its cancellation watcher after explicit Stop

   • File: jsconsumer.go
   • Problem: The watcher waits only for ctx.Done(). Calling the documented Runner.Stop() while the context remains live closes the consume loop but leaves that goroutine behind.
   • Solution direction: Tie the watcher to both context cancellation and runner completion.
   • Benefits: Makes the public one-shot lifecycle self-contained. Run already avoids the leak by cancelling each attempt context.

4. Medium: natsmsg.Publisher is too narrow and produces high-cardinality span names

   • Files: go-nuts publisher, mirror publisher seam, entiredb publisher
   • Problem: It accepts only the modern JetStream implementation, while mirror-pipeline and most entiredb publishers use legacy nats.JetStreamContext. It also names spans publish <subject>; dynamic subject components create unnecessary span-name cardinality. Consequently, none of the three services uses this public interface.
   • Solution direction: Deepen the publishing module around stable operation naming, trace propagation, deduplication, timeout, and acknowledgement, with adapters for both JetStream generations.
   • Benefits: Removes actual duplication while leaving domain-specific serialization, logging, metrics, and error handling local.

Smaller simplifications

• Drain takes both a context and timeout, but context cancellation does not stop the wait and the function returns no outcome. One authoritative cancellation budget plus an observable result would simplify callers and shutdown diagnosis.
• jsconsumer.Process passes an explicit span alongside a context already containing that span. This widens every handler interface without adding much leverage.
• jsconsumer treats zero MaxDeliver as the default of eight, while backoff.Policy treats zero as unlimited. Combining the packages requires callers to understand two different zero-value contracts.
• The earlier lifecycle problems around Runner.Stop, heartbeat transients, empty durables, multi-value baggage, and the fake message’s Nak observability are fixed in the current v0.6.0.

Remaining adoption opportunities

• mirror-pipeline: Replace pkg/otelnats with natsmsg trace propagation after upgrading from go-nuts v0.3.0. Its publishing code is also a candidate once the legacy publishing adapter exists. The custom fetch loops and heartbeat logic have materially different batching and lifetime semantics, so they are not safe drop-in jsconsumer migrations.

• entire-api: The consumers for githubpr, githubmeta, forwarder, and userlifecycle repeat the consumer creation/consume/stop scaffold almost exactly; githubpr is representative. reattribute and analysis could follow after preserving their heartbeat behavior. The ingest pipeline’s batching and worker-pool implementation should remain local. There is also a leftover raw asynchronous drain in server.go.

• entiredb: Consumer lifecycle migration is essentially complete. The next clear target is backoff.Policy: repoops and permswebhook independently implement the same exponential-delay and metadata logic in repoops and permswebhook. Publisher tracing/deduplication is another candidate once the shared publisher supports the legacy seam.

Across all three services, neither go-nuts/backoff nor natsmsg.Publisher currently has a production caller. That is useful feedback: those implementations are reasonable, but their present interfaces do not yet match the estate.

Functionality-preservation verdict

• mirror-pipeline: High confidence. Its refactor was a package/module rename, with no substantive call-path change.
• entire-api: No message-contract regression found. Intentional changes include certificate/CA lifecycle handling, synchronous bounded drains, and correct multi-value trace baggage extraction.
• entiredb: Consumer configuration and dispositions were preserved. Intentional differences include continuing heartbeats after transient failures, bounded joined shutdown, and retry supervision. Startup failures now become repeated warnings rather than the previous error path; that is part of finding 2.
• Validation: Clean worktrees on current main; full race suites passed for go-nuts, mirror-pipeline, and entire-api; focused race tests passed for all migrated entiredb consumers. go-nuts build, lint, module-tidiness, and whitespace checks passed. The relevant GitHub test/lint/integration/smoke/license checks passed; the API and entiredb PR histories still show a separate failed Entire Gates check.

No files were changed. If you want to deepen the design next, I would start with opportunity 1 or 2 because those determine whether the library can safely own process supervision.

I’m using the architecture skill again because the useful answer here is an ordered, low-risk rollout—not just a list of code edits. I’ll map each finding to a small change sequence, validation boundary, and service adoption order.

I’d fix this in three phases: correctness first, then deepen the shared modules, then migrate callers incrementally. Avoid combining library redesign and broad service migration in one PR.

Phase 1: Close lifecycle correctness gaps

1. Fix jsconsumer supervision

   • Expand local validation: required stream, non-nil handler, mutually exclusive filters, valid message limits.
   • Retry only explicitly transient failures: missing declaratively provisioned stream, timeouts, temporary connection failures.
   • Return permanent configuration, authorization, and server validation failures.
   • Make the Start watcher exit when either the context or runner closes.
   • Add tests covering missing-stream recovery, invalid configuration, explicit Stop, cancellation races, and spontaneous consume-loop closure.

   This should be the first PR because it prevents deployments that appear healthy while consuming nothing.

2. Make ShutdownGroup propagate critical-loop failure

   • Treat panic or unexpected loop return as a group failure.
   • Cancel the other tracked loops.
   • Expose the first failure to the owning run module.
   • Update mirror-pipeline so this failure makes the worker unready and causes process termination/restart.
   • Preserve cancellation → handler join → connection drain ordering.
   • Test panic, early return, normal shutdown, concurrent registration, and blocked-loop timeout.

3. Make drain completion observable

   • Respect context cancellation while waiting for CLOSED.
   • Report success, cancellation, immediate drain failure, or timeout.
   • On timeout/cancellation, explicitly close the connection so no ambiguous background drain remains.
   • Replace entire-api’s remaining raw _ = rc.Drain() call.

These three changes are a sensible go-nuts v0.7.0.

Phase 2: Deepen the shared modules

1. Redesign the publishing seam from real callers

   Use mirror’s natspub, entiredb’s gitjobs/repoops publishers, and entire-api’s analysis publisher as requirements.

   The shared module should own:

   • Stable caller-selected operation names.
   • Standard messaging attributes.
   • Trace-context injection.
   • Nats-Msg-Id.
   • Bounded broker acknowledgement.
   • PubAck telemetry and error recording.

   Keep payload encoding, subject construction, domain metrics, logging, and retry response local. Add both legacy nats.JetStreamContext and modern jetstream.JetStream adapters immediately—one adapter would leave the seam hypothetical.

2. Clarify and deepen redelivery policy

   • Remove ambiguous zero-value behavior between jsconsumer and backoff.
   • Separate pure delay/final-delivery calculation from message disposition.
   • Support both legacy and modern message adapters.
   • Preserve current delay sequences exactly before adopting it:
     • repoops: exponential, capped at 30 seconds.
     • permswebhook: exponential, configured cap.
     • entire-api ingest: exponential, capped at 300 seconds.
   • Test the full disposition matrix: first delivery, intermediate retry, final delivery, metadata error, overflow, DLQ success/failure, and Term/Nak errors.

This phase gives the publishing and redelivery modules actual depth, leverage, and locality.

Phase 3: Migrate callers in low-risk slices

1. Entiredb first

   • Replace repoops and permswebhook’s duplicate delay calculations with the shared redelivery module.
   • Migrate trace propagation in legacy publishers through the new adapter.
   • Preserve domain logging, metrics, DLQ publication, and exact Ack/Nak/Term decisions.
   • Clean up stale references to the deleted natsx module.

2. Entire-api second

   Migrate one simple consumer at a time:

   1. githubpr
   2. githubmeta
   3. userlifecycle
   4. forwarder

   Keep ingest’s batching implementation local. Treat reattribute and analysis as separate follow-ups because of their heartbeat behavior. Migrate publishers only after the shared publishing seam has both adapters and stable span naming.

3. Mirror-pipeline last

   • Upgrade from go-nuts v0.3.0.
   • Delete the duplicate pkg/otelnats implementation in favor of natsmsg.
   • Migrate natspub through the legacy publishing adapter.
   • Keep the custom worker and fanout fetch loops local unless jsconsumer first gains their batching, hook, and heartbeat semantics without increasing its interface complexity.

Required preservation gates

For every migration, capture these before changing the implementation and assert them afterward:

• Consumer durable, stream, filters, AckWait, MaxDeliver, MaxAckPending, and inactive threshold.
• Exact subjects and message IDs.
• Ack/Nak/Term/DLQ behavior and retry delays.
• Trace parentage and multi-value baggage.
• Shutdown ordering and in-flight handler completion.
• Metrics and stable span names.
• Embedded-NATS integration tests plus go test -race.
• Clean lint, build, go mod tidy -diff, and git diff --check.

Release a new go-nuts version after each coherent library phase, then bump services individually. Because these are compiled dependencies and the wire contracts remain unchanged, no coordinated cross-service rollout should be necessary.

I recommend starting with step 1: it has the highest operational impact and creates the error contract that ShutdownGroup and service readiness should consume. Which phase would you like me to explore or implement first?

I’ll implement Phase 1 across go-nuts, mirror-pipeline, and entire-api. I’ll start by pinning the current interfaces and tests, then change behavior test-first around consumer supervision, loop failure propagation, and drain outcomes before updating service call sites.

The main sequencing constraint is now clear: go-nuts must land and be released before mirror can consume the new supervision behavior. I’ll keep the library change additive and locally validate mirror against it, while avoiding a permanent replace or an unreleased version in mirror’s go.mod.

I’m locking down the behavior with focused tests first. The key compatibility choice is to keep “missing declarative stream” retryable, while treating local configuration, authorization, and non-transient JetStream responses as fatal.

The new tests exposed a hang in the lifecycle path rather than a compile failure. I’m narrowing it to the exact test now; this is useful because it is likely exercising the same Stop/watcher coordination we’re changing.

The implementation is now complete at source level. One intentional release detail remains: mirror’s new readiness/termination wiring compiles against its current v0.3.0, but only becomes active once the updated go-nuts release is consumed; I’m validating both repositories without committing an unreleased module version.

All three race suites are green. Lint found only the expected fallout from making shutdown/drain errors observable; those call sites are now explicit, and I’m running the final clean build/module checks.

Phase 1 is implemented across the worktrees.

Key changes:

• jsconsumer now validates permanent configuration errors, distinguishes retryable provisioning/transport failures, and releases its watcher after explicit Stop: jsconsumer.go.
• ShutdownGroup now cancels siblings on panic or premature return, exposes the failure through Err, preserves it as the context cause, and returns it from Shutdown: shutdown.go.
• Mirror worker now runs readiness and HTTP lifetime from the group context, returning supervised-loop failures to main: main.go.
• Drain returns failures, force-closes after timeout, and exposes ErrDrainTimeout: drain.go.
• Entire-api’s remaining raw asynchronous drain now uses the bounded helper: server.go.

One deliberate compatibility choice: Drain’s timeout remains authoritative and its context remains logging-only. Existing shutdown callers commonly pass an already-cancelled process context; honoring it would skip graceful flushing.

Validation passed:

• Full race suites: go-nuts, mirror-pipeline, entire-api.
• Lint and builds for all three.
• go mod tidy -diff for go-nuts.
• Compatibility compilation against the modified local go-nuts for mirror-pipeline, entire-api, and focused entiredb consumers.
• Clean git diff --check.
• Entiredb remains unmodified.

No commits were created. Release order should be:

1. Commit and release go-nuts, probably as v0.7.0.
2. Bump entiredb and entire-api, handling newly observable Drain errors.
3. Bump mirror-pipeline and handle the returned Shutdown result; its new supervision wiring then becomes active.

I’ll package this as three ordered PRs: go-nuts first, then mirror-pipeline and entire-api. I’ll re-pin each main, create task branches, commit with the hook-generated checkpoint trailer, verify every trailer, push, and open the PRs with the release dependency called out.

Stopped per AGENTS.md: commit 6acb1a6 was created locally on nodo/cor-929-phase1-lifecycle, but the automatic hook did not add an Entire-Checkpoint trailer.

Nothing was pushed, and no PRs were created. Once the hook is fixed, I can amend without manually inserting the trailer, verify it, and continue all three PRs.

I’ll use entire attach as requested, verify that it rewrites the go-nuts commit with the checkpoint trailer, then resume the remaining commits, pushes, and PR creation.