StackResource port readiness gate + branded Traefik error pages - #52
Merged
Conversation
Add Type field to LastFailureDetail struct with enum validation for runtime_crash and readiness_failure. This allows downstream systems to distinguish between container crashes and port readiness failures. - Add FailureTypeRuntimeCrash and FailureTypeReadinessFailure constants - Empty Type defaults to runtime_crash for backward compatibility - Regenerate CRDs and update chart CRD copies
…ng workloads Service and StatefulService workloads that declare ports but no explicit readiness check previously got no readiness probe at all, so kubelet marked the pod Ready the instant the process started -- a container not yet listening on its port would be published to Service endpoints and return 502s while every status signal read healthy. buildProbes now synthesizes a TCP readiness probe on the primary port (public port if declared, else the first port) with a generous grace window (5 min) to tolerate slow JVM/Rails boots. No liveness or startup probe is ever synthesized, so a slow starter is held out of endpoints rather than restart-looped. An explicit spec.healthChecks.readiness always wins.
- Add PortResult and Result types for port check outcomes - Implement Dial() for concurrent TCP connection testing to declared ports only - Implement Result methods: AllOpen(), ClosedPorts(), Message() - Add comprehensive spec suite with real port listening tests - All specs pass with race detector enabled
This implements the Verifier type which runs port checks on a bounded worker pool and stores results for reconcilers to read without blocking. Key features: - Non-blocking Enqueue that drops duplicate/saturated requests - Get returns stored results (caller retries if not ready) - Forget removes cached results to force redial - Bounded worker pool prevents file descriptor starvation - Race-safe with RWMutex for results/pending maps
… generation tracking Add per-key generation counter to invalidate in-flight jobs when Forget is called. When a stale job (dequeued before Forget) completes, it compares its stamped generation with the current generation before writing results; if they differ, the write is dropped, preventing a slow dial to an old IP from clobbering a fast dial to a new IP. Also adds spec that verifies the generation mechanism works by checking that re-enqueueing after Forget produces a fresh result (not stale). Fixes issue where Forget+Enqueue could allow both the old and new jobs to write to results[key], with whichever finishes last winning regardless of which was enqueued last.
Two improvements to the generation-based stale-job protection: 1. Prune generations map on write-discard: when a worker detects a stale job (generation mismatch), delete the generation entry after pending is cleared. This prevents unbounded map growth across rollouts. Safe because the generation check already happened and next Enqueue will reinitialize if needed. 2. Rewrite regression test to actually exercise the race: previous spec waited for the first job to fully complete before calling Forget, so no in-flight job existed; the generation-mismatch code path was never exercised. New test keeps job A in-flight (dial to unroutable IP on 3s timeout) while calling Forget and enqueueing job B (dial to listening port completes in ms). Asserts that job B's result persists via Consistently check window longer than job A's timeout, proving the stale write was discarded. Verified: test fails identically in broken version (without generation check), confirming it catches the actual race.
…ate corruption Critical fix for a bug introduced in the previous round: stale job completion was unconditionally clearing pending and generations, corrupting state that an in-flight newer job still needed. This allowed duplicate Enqueue during in-flight jobs and caused legitimate results to be discarded as stale. Changes: 1. Make cleanup generation-gated: only touch pending/generations if job generation is current (matches v.generations[key]). Stale jobs do nothing, leaving state intact for the newer in-flight job. 2. Never prune generations map during run(). Keep entries indefinitely. Justification comment: bounded by distinct Keys ever Forgotten. 3. Replace unroutable IP 10.255.255.1 with RFC 5737 TEST-NET 192.0.2.1 (10.x is routinely routable in k8s/VPC CI). 4. Add regression test for the critical interleaving: A (slow, completes first) and B (slow, completes second). Verifies that A's stale completion doesn't clear state belonging to B, and B's legitimate result is stored. The fix ensures the never-block guarantee and generation-based invalidation remain sound across all job interleavings.
…b cleanup The spec covering "stale job completes before the fresh one" could not tell fixed code from broken code. A and B shared one verifier timeout and both dialed TEST-NET, so B finished before the sleep meant to catch it mid-dial; job C was then rejected as already-answered rather than as pending, and the final assertions held for C's result just as well as B's because the only discriminating field, Ports[0].Port, was never asserted. Rewrite it with a single worker and a blocker job so the queue can be staged in exact FIFO order: stale A dials a closed local port (microseconds), B dials TEST-NET (holds the worker for the full timeout), and the assertion pins the stored result to B's port. Verified to fail against the pre-gate unconditional cleanup and pass against the current generation-gated path. Also: - Enqueue no longer materializes generations[key]=0 for every key, so the map retains only keys ever Forgotten and the bounded-growth comment is true. - The queue-full fallback only clears the pending marker while the generation is still the one this Enqueue stamped, so it cannot delete a newer job's marker after an interleaved Forget+Enqueue. - TEST-NET assumptions are named in assertion annotations so a network that answers 192.0.2.1 with ICMP unreachable fails loudly instead of silently voiding the setup.
…ning Wire the async port verifier into the workload status paths. Both the serving and not-serving branches of the Deployment and StatefulSet evaluators now consult the verifier: a proven-dead port records a typed readiness_failure detail and flips the resource off Available. The kubelet probe added earlier guards only the primary port, so a workload whose secondary port is dead reaches the serving branch. Running the verifier there is what removes the silently-green case. The check never dials inside Reconcile. A cached result is consulted before any API call, and only a cache miss pays for the pod lookup; an unanswered result is not a failure — the check is scheduled and the reconcile proceeds. Keys are scoped to the workload revision so a rollout invalidates the previous answer. Adds --port-check-grace (default 3m), which bounds how long an unverified resource keeps being requeued to read its result.
reportStackResourceNotReady updated Available and Converged but left the Stalled condition untouched, so a once-ready resource kept the success reason/message written by reportStackResourceReady while visibly failing. Stalled stays False (not-ready is retriable) but now carries the current reason and message.
…ints Set traefik.ports.web/websecure.middlewares in the agent umbrella chart so every router gets the branded error-page middleware from one place, instead of annotating each generated Ingress. The namespace prefix is literal because Helm does not template values.yaml and the agent namespace comes from .Release.Namespace; noted inline.
…erve Five nginx-backed fixtures declared ports nginx never listens on. They only passed because the port-readiness gate did not exist yet. - MultiResourceStack backend, StackWithThreeResources api/worker: single-port cases now declare 80. - StackWithEnvAndPorts (8080+9090) and StackWithPublicPorts (80+9090) keep both ports to exercise multi-port Service/Ingress shape; nginx is now told to listen on them via a command override.
New internal/controller/errorpages package exposing Assets, a filename to content map built with go:embed, for the agent's error page handler to serve. Each page is a complete standalone document with an inline style block and no external requests of any kind - the pages are served in clusters with no guaranteed internet egress, where a blocked font or script would leave the page unstyled. The duplication across the four files is deliberate: the agent serves each directly with no template step, and a shared stylesheet would mean a second request that could fail independently. Markup follows docs/superpowers/plans/2026-07-30-error-page-assets.md verbatim, with one exception: the colon in the favicon data URI's xmlns is percent-encoded so the source carries no literal http:// substring. Data URI content is percent-decoded before parsing, so the SVG the browser sees is unchanged.
Add a port-mismatch fixture (crccheck/hello-world listens on :8000 while declaring :80) and integration specs asserting the gate keeps such a resource unavailable, records a readiness_failure naming the port, and still lets a correctly configured resource converge. Also declare port 80 on the inline nginx StackResource in the mutation test, which listened on :80 but declared :8080 and would falsely fail under the new gate.
…Traefik The agent now serves its compiled-in error pages over HTTP on :8082 and ensures the three Traefik-facing singletons at start-up: the stackdome-error-pages Service, the stackdome-errors Middleware for 500-599, and the lowest-priority stackdome-catch-all IngressRoute. Traefik has no file backend, so pointing it at the agent removes the ConfigMap, nginx Deployment, and drift repair a static-file approach would need. Ensure failures are logged and start-up continues: a cluster without Traefik CRDs must still run the agent.
…ll booting Review round 1. A pod gets an IP the moment it starts, so the first dial can land while a slow-booting app is still binding. Enqueue short-circuits on an existing result, so that premature refusal became the resource's permanent truth for the life of the revision: the app would bind at t=20s, pass its kubelet probe, reach the serving branch, and be pinned NotReady on a verdict taken before it was listening. A closed verdict read on a path where the kubelet probe has already proven the primary port is now retried once — Forget plus a re-enqueue against the booted workload — before it is believed. The retry is claimed at most once per key, so a genuinely dead port is confirmed by the second verdict and there is no Forget/re-enqueue loop. Still no dialing in Reconcile, and the existing bounded requeue carries the resource across the three passes. Also guards the serving branch against overwriting a captured crash detail with a port verdict — once-per-revision stamping made that overwrite permanent — and drives Converged False alongside Available when a port is proven dead, so a resource nobody can reach cannot read "fully converged".
… spec Available goes True before the port verifier answers: the first serving reconcile only enqueues the dial and the result is read on a later 10s requeue. Asserting no readiness_failure the instant Available flips therefore checked before the guarded behaviour ran, hiding the very regression the spec exists to catch. Hold across the stability window instead, re-fetching the StackResource and asserting Available stays True and no readiness_failure appears.
Controller-runtime puts a runnable that does not implement LeaderElectionRunnable in the leader-election group, so the error page server started only on the leader. The Service selects every agent pod and readiness is probed on the health port, so non-leader replicas were Ready endpoints refusing connections on the error page port. Ensure now runs under a 30s timeout: it executes before mgr.Start, so an unreachable API server would have held up every controller.
…ed error pages Final-review fixes for the port-readiness and branded error-page work. Error pages were fail-open in creation and fail-closed in effect. The umbrella chart applies the stackdome-errors middleware to every router on both Traefik entrypoints, and Traefik fails a router whose middleware does not resolve, so a single transient API error during the one-shot pre-Start Ensure turned into a permanent cluster-wide ingress outage announced by one log line. Ensure now runs as a leader-elected manager.Runnable that retries with jittered backoff until it lands, and the umbrella chart refuses to render when the middleware reference does not match the release namespace. Port verdicts were condemnation by counter: one retry per key, second closed verdict final for the revision. An app whose secondary port bound seconds after its primary probe passed stayed Available=False until the next rollout. Condemnation is now a deadline — a closed verdict is re-verified for as long as the port-check grace budget lasts and only believed once it is spent. The same budget replaces rollout-settledness and convergence as the anchor for "come back and read the result". Those anchors live on the object and survive a process restart while the verifier cache does not, so a workload re-discovered by a freshly started operator was born with its budget spent and its freshly scheduled check went unread until the 10h resync. Also: Ensure refuses an empty pod selector (a non-nil empty Service selector matches every pod in the namespace, not none) and main logs the raw flag value when --error-pages-selector parses to nothing; --port-check-grace defaults to workload.DefaultPortCheckGrace instead of a duplicated literal.
…ten the helm guard Re-review of the previous fix wave found two ways the fixes fell short. The port check budget was stamped on any cache miss, including from the not-serving branches where no dialable pod was found and nothing was enqueued. A revision that took longer than the budget to become Ready — an image pull plus a slow boot, ordinary during the fleet-wide rolling restart this release triggers — reached the serving branch with the budget already spent, so its first premature closed verdict was believed on sight. That is worse than the once-per-key retry this replaced. The budget now opens only when a dial is actually in flight (using the bool schedulePortCheck already returned and both call sites ignored), and is re-based once onto the first probe-passed sighting. The two rules cover different states: the first a pod that does not exist yet, the second a pod that is dialable while its app is still booting. The re-anchor latches, so it cannot extend the budget indefinitely. Restart-discovery semantics are unchanged — a re-discovered serving workload still opens a budget on the spot and still requeues. The helm guard matched on "<namespace>-", which let a namespace that is a prefix of another through: --namespace stackdome accepted a reference naming stackdome-control-plane, while the agent would create stackdome-stackdome-errors@kubernetescrd — an unresolvable reference and the exact cluster-wide 500 the guard exists to prevent. It now requires "<namespace>-stackdome-errors". Also: each Ensure attempt is bounded by a 30s context, so a hung API call fails and is retried instead of parking the retry loop. Two restart specs were passing for the wrong reason — representativePodIP dispatches on workload type, so the StatefulSet fixtures were resolved through the Deployment ReplicaSet path and found nothing. Tightening the stamp exposed it.
…erve The synthesized TCP readiness probe guards the primary declared port, so three inline specs that declared a port nothing listened on could never reach Available: - stack_failure_test: the LastFailureDetails recovery spec swapped in nginx:1.25-alpine but kept the crash fixture's :8080 declaration; it now declares :80 alongside the image switch. - stack_convergence_test: the partial-failure context runs nginx-unprivileged (serves :8080) on NewResource's default :80; both resources now declare :8080 explicitly. - stack_convergence_test: the address-persistence marker script now starts busybox httpd on :8080 during its healthy phase so the first rollout converges. The crash branch is unchanged, so crash semantics and every assertion are preserved. Also bump the ginkgo integration timeout to 90m — the last run hit the 1h cap with 39 specs skipped.
…erifier Per the design owner's decision, the port-check Verifier no longer arbitrates between concurrent jobs for one Key. Instead it guarantees there is never more than one: Enqueue refuses while pending[key] is set, and Forget clears only the stored result, never the pending marker. With no two jobs contending for a key, the generations map, the per-job generation stamp, and both generation equality checks (the completion write and the queue-full rollback) are dead weight and are deleted. Both paths are now unconditional. Accepted and documented tradeoff, not an oversight: a job in flight across a Forget still lands its answer in the cache. It is not discarded. Correction waits for the caller's next reconcile (~10s) to Forget-and-redial, which is admitted because the marker is gone by then. Known accepted edge, called out in Forget's doc comment: a stale write landing just as the grace budget expires can condemn a healthy resource. Specs asserting the generation mechanism (stale-write discard and the reverse-interleaving counterfactual, with its queue-staging machinery) are replaced by two that assert the new contract: an Enqueue is refused while a dial for the key is in flight, and the in-flight answer lands after a Forget and is corrected a cycle later. The RFC 5737 TEST-NET-1 slow-dial technique and its environment guard are retained. The workload readiness retry path is unchanged; its comments describing the old discard guarantee are corrected to say the Enqueue silently no-ops while a dial is in flight and the 10s requeue is the retry. Also repoints the manager at NewVerifierWithDefaults, which the constructor rename had left dangling.
…chart-owned error-page objects Port readiness: replace the reconciler's in-memory budget map (mutex, probe-anchored re-basing latch, per-key lifecycle) with status.portCheck — the revision and the time a closed-port verdict was first seen while serving. The window survives operator restarts, latches per revision, and clears on an open verdict. The two capture entry points collapse into verifyServingPorts (grace-bounded condemnation) and capturePortDiagnosis (provisional not-serving detail). Condemnation no longer stops the sub-reconciler chain, and stays terminal per revision so a dead port cannot become a permanent dial/requeue loop. Verifier gains Pending() so callers only requeue when an answer can arrive. Error pages: the Service, Middleware, and catch-all IngressRoute are static at install time, so the standalone chart creates them (selector from the same selectorLabels helper the Deployment uses; release namespace; gated on errorPages.enabled for Traefik-less clusters). Deletes the runtime Ensure, its retrying Runnable, the selector/namespace flags, and the agent's ingressroutes RBAC; the package moves to internal/errorpages since what remains — embedded pages and the HTTP server — is not a controller. Helm now owns the objects' lifecycle, so uninstall cleans them up. Also waits for the ScheduledBackup in the postgres integration spec instead of asserting immediately.
A dial could land between Get and Enqueue: Enqueue then refused the job, Pending was already clear, and the fresh verdict sat cached but unread until an unrelated event or the resync. verifyServingPorts now re-reads after a refused schedule with nothing pending and processes the landed verdict in the same pass. Results were keyed by revision, so every rollout orphaned the previous entry and the map grew with deploy count for the life of the process. Store per (namespace, name) with the revision alongside the answer instead: Get and Forget match on revision, a new revision's dial overwrites the old entry, and the map is bounded by resource count. Forget's never-read bool return is dropped. Also corrects the --port-check-grace help text (it gates believing a closed verdict, not requeue duration) and removes the dead POD_NAMESPACE env from the config/deploy manifests.
This was referenced Jul 31, 2026
Open
ashishmax31
added a commit
that referenced
this pull request
Aug 2, 2026
…#60) The Traefik error-page middleware added in #52 is applied at the entrypoint level, so it wraps every router on both entrypoints — including the router for the Stackdome API server that the hub installer exposes as a StackResource. Any 5xx from the API therefore reached the dashboard as an HTML page, which axios cannot parse: the UI lost both the status and the message. Traefik copies the original request's headers onto the request it makes to the error-page service, so Accept still identifies the real caller. Serve the page to a browser navigation, and the API error envelope the dashboard already parses to everything else. The status code is unchanged either way. The match is on an explicit text/html rather than */*, because axios sends "application/json, text/plain, */*" — treating */* as a browser would reintroduce the bug for every API client. Note this cannot recover the backend's own error text: Traefik's errors middleware discards the caught response body before the error-page service is reached. Carrying the real message through needs Accept-split routers, which is a larger change.
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.
What & why
Stackdome users declare the ports their app serves, but the kubelet probe only guards the primary port. Two silent failure modes existed: an app listening on the wrong port sat "not ready" forever with no actionable diagnosis, and an app with a dead secondary port went fully Available while serving traffic nobody answered — with visitors seeing raw Traefik 500s.
Changes
Async port verifier (
pkg/portcheck) — bounded worker pool TCP-dials every declared port of a running pod, caches a per-revision verdict. Single-flight per key; reconcilers never dial inline or block.Readiness gate (
workload/readiness.go) — a serving workload whose port stays proven-closed past a 3-minute grace window is drivenAvailable=False / Converged=Falsewith a typedPortNotListeningfailure detail. The window is anchored onstatus.portCheck.failingSince(first closed verdict seen while serving, per revision), so it survives operator restarts and latches by construction. Inside the window closed verdicts are discarded and redialed, so a refusal cached during boot never condemns a healthy app and slow secondary ports get time to bind. Condemnation is terminal per revision (rollout resets); not-serving workloads get an immediate provisional diagnosis naming the dead port.Branded error pages (
internal/errorpages+ charts) — the agent embeds 404/500/502/503 pages and serves them from every replica on :8082. The standalone chart installs the Service, thestackdome-errorsTraefik middleware (5xx → branded page, real status preserved), and a lowest-priority catch-all IngressRoute (unclaimed URLs → 404). The umbrella chart attaches the middleware to every router on both entrypoints, with an install-time guard that fails fast on a namespace mismatch — an unresolvable middleware reference would 500 every route in the cluster.Rollout notes
status.portCheckfield on StackResource.errorPages.enabled=falseon the standalone chart.config/deployunchanged); add static manifests if that path ever ships.Testing
helm lint+helm templateverified: objects render in the release namespace with the Deployment's selector; nothing renders when disabled.make test-integration, Kind, ~11 min) includesport_readiness_test.gocovering dead-port and healthy-path lifecycles — not yet re-run after the final status-window + Helm refactors; should run once before merge.