fix(ha): harden failover runtime and controller - #347
Conversation
…-seed-artifacts # Conflicts: # zig/lib/httpx/src/client/client.zig
…-failover-runtime-hardening
…time-hardening # Conflicts: # zig/pkg/antfly/src/common/http/std_http_listener.zig
bpopadiuk
left a comment
bpopadiuk
left a comment
There was a problem hiding this comment.
Codex (GPT-5): This is an independent review written and posted by Codex, not by Boris.
Blocking review — approval should be withheld. GitHub does not allow the PR author's account to submit a formal “Request changes” event on its own PR, but I do not consider the current b5c5881 head production-ready. The overall direction is strong—typed receipts, fail-closed identity checks, narrow RBAC, generated client coverage, and the operator/runbook work are valuable—but the current implementation has multiple availability and correctness blockers on the core failover path.
Blocking findings
-
Slow seed capture can self-fence the active primary.
go/pkg/operator/controllers/antfly/antflycluster_controller.go:5711performs capture synchronously in the same default single-worker reconcile that renews the fencing Lease. The watchdog only extends authority after a newerrenewTime, and its default grace matches the HTTP client's 10-second timeout. A capture exceeding that window can block renewal long enough to latchapi_unreachableand reject writes; canceling the client does not cancel the Zig capture, and a retry can start another attempt. Make capture asynchronous or independent from Lease renewal, add single-flight semantics, and test a capture longer than watchdog grace without fencing. -
The mutation barrier can deadlock permanently. It is writer-preferring but not reentrant. At
zig/pkg/antfly/src/storage/db/db.zig:11614,setSchemaJsontakes a shared guard and callssetSchema, which takes another. If capture has takenreader_gatewhile waiting for the outer guard'sresource_mutex, the nested acquisition waits onreader_gateforever while capture waits on the outer guard. Other guarded call chains nest similarly. Acquire once at the public boundary/use internal “guard already held” helpers (or implement provably safe execution-context reentrancy), and add a deterministic interleaving test. -
Real portable-artifact Job receipts are rejected. The nested decoder at
go/pkg/operator/controllers/antfly/antflycluster_controller.go:7003declares onlypathwhile usingDisallowUnknownFields. Zig emitspath,size_bytes,crc32,sha256, andchunks; the first realsize_bytestherefore makes a successful Job undecodable, so no typed evidence is recorded and dependent actions remain blocked. Existing tests use truncated path-only fixtures. Share the complete wire schema and contract-test an actual Zig CLI receipt. -
Every seed generation leaks a full prepared snapshot on the primary PVC. The return at
zig/pkg/antfly/src/data/runtime.zig:4019transfers a path whosedeinitfrees only the string. Nothing removes<capture_root>.runtime-snapshots/<generation>; existing capture GC prunes a different<capture_root>/generationstree. Repeated reseeds will exhaust disk. Define ownership and remove the directory after canonical capture with crash recovery, or add paired generation GC, plus a multi-generation disk-bound test. -
Restore publishes false readiness. The checks at
zig/pkg/antfly/src/storage/db/db.zig:11566do not prove the restored index has an active/queryable generation. Currentzig-basereaches “restore runtime repair marked complete” and the immediately following query fails withIndexRebuilding. Gate completion on an index-manager invariant that guarantees queryability and retain an immediate-query regression test. -
Admission accepts portable-seed CRs the planner/executor cannot run. At
go/pkg/operator/api/antfly/v1/antflycluster_webhook.go:1510, the topology tuple may be entirely absent and PVC bindings are optional. The planner then creates no executable chain without source/target PVC data, while the executor requires exact topology, target PVC name, and target PVC UID. Require the complete execution tuple/PVC bindings at admission, or implement and document a genuinely supported unbound path.
Production, UX, performance, and design gates
- Authentication: HA admin auth needs one coherent boundary. On the shared public listener, native API-key authentication can reject a dedicated HA bearer token before the inner handler sees it; with native auth disabled and no HA token configured, the inner handler authorizes destructive routes. Put these routes on a private/admin listener or define one explicit auth path, require its secret at admission, and exercise the real handler end to end.
- Write availability and scale:
zig/pkg/antfly/src/data/runtime.zig:4284holds the global exclusive mutation barrier through snapshot preparation and full recursive copy/link work, making write unavailability proportional to database size. Establish a point-in-time/checkpoint view under a bounded critical section, then copy/hash/upload outside it. Sequential chunk upload, a 600-second Job deadline, BestEffort pods, advertised 8 GiB/file and 64 GiB/artifact limits, and quadratic validation scans at high file counts also need measured bounds and production-sized load tests. - Current verification state: the checked head is merge-conflicting,
zig-baseande2e-baseare red, and at least the restore-readiness failure is directly in this PR's changed behavior. Those failures cannot be treated as unrelated. - Missing live proof: the PR says the live KinD/full matrix was not run. Before approval, run an exact Antfly/Colony-pair matrix covering normal failover, API/network partition, old-primary rejoin, controller restart, large seed/load, object-store outage, backup/restore, and upgrade/rollback, with bounded write-unavailability evidence.
- Security-maintenance surface: the 1,759-line Zig TLS client fork is imported globally by httpx for every HTTPS caller. Isolate the compatibility change to the Lease transport, or document and pin the exact upstream delta and add interoperability/fuzz coverage.
- Code shape: the controller, planner/status, and webhook files are now very large. Split artifact lifecycle, fencing/renewal, startup gates, and planner/executor state machines so their invariants can be tested independently and rollback risk is smaller.
Please fix these blockers, make the head conflict-free and green, and attach the live exact-pair evidence before treating this as a production-ready HA foundation.
# Conflicts: # zig/pkg/antfly/src/api/table_writes.zig # zig/pkg/antfly/src/storage/db/db.zig
|
Codex (GPT-5): This response was written and posted by Codex, not Boris. I re-evaluated the blocking review against final Antfly head The six concrete blockers are addressed:
Authentication is now one explicit fail-closed boundary: the dedicated HA bearer is required and is aligned across the outer listener and inner handler. The real handler path is exercised end to end. The Lease transport remains Zig-native. The OpenSSL C transport, Final deterministic evidence includes operator I agree with these remaining review concerns and am deferring them explicitly rather than claiming closure:
The initial final-head The PR is current with |
| restore: RestoreSource, | ||
| options: RestoreOptions, | ||
| ) !?db_mod.generation_lifecycle.StagedGeneration { | ||
| if (try restoreSnapshotAlreadyApplied(alloc, path, group_id, restore, options)) return null; |
There was a problem hiding this comment.
(not your fault but maybe you could look into) Not sure why this code is in raft/ seems a little bit of a smell? Does it get used for HA?
There was a problem hiding this comment.
nice catch this is a weird + unused merge conflict resolution artifact. i am reverting it to the state of main
| const httpx = @import("httpx"); | ||
| const common = @import("../common/http/http_common.zig"); | ||
|
|
||
| pub const ZigLeaseExecutor = struct { |
There was a problem hiding this comment.
Should this be in this package? Should it just be LeaseExecutor?
There was a problem hiding this comment.
yeah think you're right about the name - going to change it
| //! portable storage artifacts. Keep this module below both layers so decoding | ||
| //! a seed never imports the metadata control loop into storage-only binaries. | ||
|
|
||
| pub const TableRecord = struct { |
There was a problem hiding this comment.
Should this be in this package?
There was a problem hiding this comment.
codex seems to like this particular layout/separation of concerns, what do you make of this?
Codex (GPT-5): This placement is intentional. These topology wire records are consumed by both metadata/table_manager.zig and storage-only HA seed materialization. Defining them under metadata would force the storage layer to import the metadata control-plane dependency tree. Keeping the types in common provides a lower-level shared boundary, while table_manager.zig re-exports them for metadata callers. I think the current package is therefore the right dependency direction.
3 participants
Codex (GPT-5): Delivers the Antfly runtime and operator foundation for production-ready hot-standby HA, paired with the Colony control-plane work in https://github.com/antflydb/colony/pull/346.
Current Antfly revision:
7bd93ae4b1e1d98db09731c81b19d3f2f971042c.Production-readiness scope
Swarmresources to theStandaloneruntime without changing StatefulSet, Service, PVC, selector, mount, or storage identity.Safety properties
Dependency-free runtime transport
The Lease transport remains dependency-free Zig through Antfly's existing
httpxpath. It preserves projected Kubernetes CA validation, DNS hostname verification and SNI, service-account bearer authentication, one absolute monotonic request deadline, bounded response handling, and fail-closed watchdog fencing.The checked-in Zig-native TLS compatibility fix handles an optional TLS 1.3
CertificateRequestby sending an empty client certificate while preserving transcript boundaries. The abandoned OpenSSL Lease transport, system links, package additions, and host-glibc workaround are absent.Deterministic evidence
make test.CertificateRequest.git diff --checkpassed.Review and certification boundary
backup_lsn-bound source. Safely shortening that outage requires an immutable storage-engine snapshot/checkpoint handle or equivalent two-phase boundary.zig-basegate passed. The isolatede2e-basererun reproduced only two unrelated timing failures: managed-embedding retry sequencing and automatic shard-split finalization. Neither exercises HA or the changed paths.Colony PR346 pins this exact Antfly gitlink and operator pseudo-version/checksum.