Skip to content

fix(tl): serialize ingestion, recover leaves, and derive certificate status - #127

Open
csnitker-godaddy wants to merge 3 commits into
fix/acme-owner-validationfrom
fix/tl-ingestion-status
Open

csnitker-godaddy wants to merge 3 commits into
fix/acme-owner-validationfrom
fix/tl-ingestion-status

Conversation

@csnitker-godaddy

@csnitker-godaddy csnitker-godaddy commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Repeated submissions of the same event return the original leaf. A timestamp-only repeat of the latest agent state is also a duplicate. Conflicting or stale lifecycle changes are rejected, while a renewal with changed certificate evidence can append for the same FQDN/version.

Committed leaves remain recoverable after an interrupted write. Recovery preserves historical duplicate leaves and their signed evidence. Current status cannot be served from an out-of-date event index.

Badges and status tokens use min(max(server notAfter), max(identity notAfter)) for the required certificate families. Expired overlap certificates do not expire their replacements. Expired agents receive no status token; tokens include only unexpired certificate fingerprints and cannot outlive any certificate they authorize.

Contract comparison

The existing AppendResponse in spec/api-spec-tl-v2.yaml remains:

AppendResponse:
  type: object
  required: [leafIndex, leafHashHex, duplicate, treeSize]
  properties:
    logId:       { type: string, format: uuid }
    message:     { type: string }
    success:     { type: boolean }
    leafIndex:   { type: integer, format: int64 }
    leafHashHex: { type: string }
    duplicate:   { type: boolean }
    treeSize:    { type: integer, format: int64 }

No response, event-envelope, checkpoint, receipt, or status-token fields change. The existing CBOR exp claim is bounded by the certificates it authorizes. The OpenAPI changes document duplicate/lifecycle behavior and the AGENT_STATE_CONFLICT / STALE_AGENT_EVENT validation codes; the event-schema edits describe lapsed-family evidence.

Expiry and lapsed-family semantics depend on agentnameservice/ans-registry#68. The additional semantic lifecycle checks go beyond the current ANS-4 content-hash deduplication text and need corresponding prose clarification. This PR does not claim that clarification is already merged.

Validation

make build, make check, and make test-race pass on this branch. Coverage is internal 90.83%, domain 100.00%, crypto 98.45%.

Regressions cover concurrent V1/V2 submissions, repeated events, conflicting lifecycle changes, successive renewals, interrupted writes, restart recovery, historical duplicate leaves, and preservation of signed evidence. Certificate tests cover both family orderings, V1/V2 overlap, expiry boundaries, signed status, and token lifetime.

Third PR in the deployment-fix stack. Back up the log data and stop the existing TL process before upgrading. Only one TL writer may use a log data directory. The upgrade preserves the Merkle log's history.

Fixes #123.

Depends on #126; this PR targets fix/acme-owner-validation to keep its review diff scoped.

AI assistance

Assisted-by: Codex (GPT-6), under Connor Snitker's direction.

…atus

Signed-off-by: Connor Snitker <csnitker@godaddy.com>

@kperry-godaddy kperry-godaddy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ingest gate, startup and pre-read recovery, migration 006, the writer lock and the family-max expiry rule fix real defects, and the tests exercise them against a real Tessera log; that part I would merge. What shapes my read is checkAgentState. It makes the Transparency Log a second lifecycle arbiter, with semantic dedup that ANS-4 §8 does not define, and its rejections are permanent 422s. That interacts badly with two things on the RA side: the inline activation seal rebuilds its leaf with a fresh now on retry instead of replaying persisted bytes, and the outbox has no per-agent ordering at this base. I would split the policy layer out (or hold it) and fix the seal replay in the RA, where the root cause lives.

Four things I'd settle before this ships, all inline. Non-blocking notes below.

Not blocking, worth a look

  • Badge and status token disagree on a malformed notAfter: certExpiresAt falls back to expiresAt (badge ACTIVE) while currentCertFingerprints returns an error (token 500, permanently, for that agent). Ingest never validates notAfter, and CertificateExtended leaves it optional although the arrays may now carry lapsed certificates. One shared parser with one policy, plus RFC3339 validation in Event.Validate, closes it.
  • WithAntispam cannot fire: every leaf carries a fresh UUIDv7 logId and a fresh attestation signature, so no two appends share Tessera's identity hash. If IsDup ever came back true, StoreEvent would be called with the original leaf index and hit UNIQUE(leaf_index). Remove it and the IsDuplicate plumbing, or honour it before the mirror write.
  • The conflict tests assert only err != nil; three of the six scenarios are rejected by the new INVALID_EVENT name checks rather than by the state logic they are named for, and STALE_AGENT_EVENT, the foreign-producer rule and the DEPRECATED transitions have no test at all. A wantCode column and a second producer key in the testbed cover that.
  • Operator steps exist only in the PR body: stop the old writer, back up, one writer per data directory, the one-time tl_events rebuild and its disk cost, recovery before the listener opens, what "index extends beyond the integrated log" means. Compose's health check marks the TL unhealthy after roughly 55s of startup work. An Upgrading section in README or docs/ carries them past the merge.
  • Contract text: state conflicts sit under 422 "Body fails validation" while the handler already maps conflicts to 409; the raId ownership rule is undocumented; the identity route still says "same dedup / same codes" although checkAgentState skips identity envelopes; /v2/internal/agents/event is registered but absent from the spec. LatestAgentState also binds a versioned name to its first agentId forever, across producers and past revocation, and nothing says so.

Comment on lines +134 to +147
if rec.EventType == string(event.TypeAgentRevoked) ||
env.EventType() == string(event.TypeAgentRegistered) ||
(rec.EventType == env.EventType() && env.EventType() != string(event.TypeAgentRenewed)) ||
(rec.EventType == string(event.TypeAgentDeprecated) && env.EventType() != string(event.TypeAgentRevoked)) {
return nil, domain.NewValidationError("AGENT_STATE_CONFLICT", "event duplicates or reverses an existing lifecycle state")
}
oldTime, oldErr := time.Parse(time.RFC3339, prior.Timestamp)
newTime, newErr := time.Parse(time.RFC3339, env.Timestamp())
if oldErr != nil || newErr != nil {
return nil, domain.NewValidationError("INVALID_EVENT", "event timestamp is invalid")
}
if newTime.Before(oldTime) {
return nil, domain.NewValidationError("STALE_AGENT_EVENT", "event predates the current agent state")
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These rejections assume per-agent in-order delivery and apply to AGENT_REVOKED. STALE_AGENT_EVENT compares producer-supplied timestamps with no skew window and no exemption for revocation, so an RA clock step backwards between two events makes the revocation unappendable and the agent stays ACTIVE here. At this base the RA outbox has no per-agent ordering (a row in backoff is overtaken by later rows) and the TL client treats every non-429 4xx as permanent, so once #128 emits AGENT_RENEWED through the outbox, one transient error followed by a later event turns a signed, RA-committed statement into a permanent absence from the log, with the row retrying at max backoff forever.

Worth settling before this ships, because it decides what the log is: a witness that seals every verified statement, or an arbiter. Revocation should never be rejected on ordering or time; the identity lane already treats it as terminal by existence (HasIdentityRevoked). Either seal every verified same-producer statement and derive current state at read time (any AGENT_REVOKED on the stream wins, otherwise the latest producer timestamp with leaf_index as tiebreak), or, if rejection stays, return 409 with a distinct code so the producer can retry after its predecessor, add a bounded skew window, and land per-agent FIFO plus a dead-letter state in the RA outbox first. The minimum change if nothing else moves:

	if env.EventType() != string(event.TypeAgentRevoked) {
		oldTime, oldErr := time.Parse(time.RFC3339, prior.Timestamp)
		newTime, newErr := time.Parse(time.RFC3339, env.Timestamp())
		if oldErr != nil || newErr != nil {
			return nil, domain.NewValidationError("INVALID_EVENT", "event timestamp is invalid")
		}
		if newTime.Before(oldTime) {
			return nil, domain.NewValidationError("STALE_AGENT_EVENT", "event predates the current agent state")
		}
	}

Comment on lines +72 to +81
// checkAgentState checks the database before any append. An unchanged state
// retried with fresh timestamps maps to its original leaf. Renewal is repeatable
// when its certificate/attestation state changes; a UNIQUE(FQDN,version,status)
// constraint would incorrectly suppress every renewal after the first.
//
//nolint:nilnil // A nil record and nil error mean the new state is eligible for append.
func (s *LogService) checkAgentState(ctx context.Context, env event.Signable, canonical []byte) (*sqlitetl.EventRecord, error) {
if _, ok := env.(*identityevent.Envelope); ok {
return nil, nil
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkAgentState strips timestamp and issuedAt to decide that two differently-signed events are the same state, re-encodes domain.ValidTransitions as a four-clause predicate, and pins each agent to the raId of its first event, all inside the I/O orchestrator and testable only through the Tessera testbed. ANS-4 §4 and §8 define dedup as byte-identical only, and none of these rules has a spec home yet. The semantic dedup exists for one producer behaviour: sealActivationEvent rebuilds the activation leaf with a fresh now on retry instead of persisting and replaying the signed bytes, which is the rule the outbox-replay invariant already imposes on worker rows. When a retry after a seal-then-crash differs in anything beyond the timestamps (a DNSSEC AD bit, an optional record), it gets a permanent AGENT_STATE_CONFLICT: the agent stays PENDING_DNS, the RA refuses the name, this log holds it, and registration.go still calls the orphaned leaf benign residue.

Worth settling before this ships. Fix the root cause in the RA (persist {innerEventCanonical, producerSignature} for the activation leaf before calling SealAgentEvent and replay it verbatim on retry) and reduce this function to what only the log can check: canonical name and version (which belong in Event.Validate) plus the binding guard that a versioned FQDN is not bound to a different agentId. eventStateBytes, the raId rule, the transition predicate and both new codes can then go, or be held until the ANS-4 amendment lands and the transition rule is a pure table-driven function shared with the domain. That also lets migration 006, the gate and the expiry work merge on their own.

Comment on lines +26 to +59
func (s *LogService) RecoverIndex(ctx context.Context) error {
select {
case s.ingestGate <- struct{}{}:
defer func() { <-s.ingestGate }()
case <-ctx.Done():
return ctx.Err()
}
s.indexReady = false
if err := s.recoverIndex(ctx); err != nil {
return err
}
s.indexReady = true
return nil
}

// lockIndexedRead linearizes current-state reads with append/index writes.
// After a failed mirror write, a healthy SQLite read must not mint an ACTIVE
// token from an older row while a revocation is already committed in Tessera.
func (s *LogService) lockIndexedRead(ctx context.Context) (func(), error) {
select {
case s.ingestGate <- struct{}{}:
case <-ctx.Done():
return nil, ctx.Err()
}
unlock := func() { <-s.ingestGate }
if !s.indexReady {
if err := s.recoverIndex(ctx); err != nil {
unlock()
return nil, fmt.Errorf("log: recover event index before read: %w", err)
}
s.indexReady = true
}
return unlock, nil
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

internal/tl has no logger, and this change adds the paths that most need one: startup and in-request recovery, the fail-closed indexReady flips in log.go, the lifecycle rejections, and the writer lock. After a failed mirror write every read and write returns 500 while the TL's own log shows only "listening"; a restored leaf, which is evidence of an earlier failed write, leaves no trace; a cross-producer conflict is visible only in the RA's outbox log. The startup failure is logged once, generically, by main.

Worth fixing before this ships since CLAUDE.md treats a silent component as a defect, and the wiring is small: a logger zerolog.Logger field on LogService (default zerolog.Nop()) with a WithLogger option that tags component=tl-log, applied from cmd/ans-tl/main.go. Then INFO on recovery start and end with firstUnindexedLeaf, indexedSize, logSize, the restored count and duration; WARN or ERROR with leafIndex, logId, agentId where indexReady flips; WARN with agentId, ansName, eventType, existingLeafIndex before each AGENT_STATE_CONFLICT and STALE_AGENT_EVENT; INFO with the path on writer-lock acquire. For this function:

	s.indexReady = false
	s.logger.Info().Msg("recovering event index")
	if err := s.recoverIndex(ctx); err != nil {
		s.logger.Error().Err(err).Msg("event index recovery failed; reads and ingest stay fail-closed")
		return err
	}
	s.indexReady = true
	s.logger.Info().Msg("event index recovered")
	return nil

Comment on lines +195 to +206
select {
case s.ingestGate <- struct{}{}:
defer func() { <-s.ingestGate }()
case <-ctx.Done():
return nil, ctx.Err()
}
if !s.indexReady {
if err := s.recoverIndex(ctx); err != nil {
return nil, fmt.Errorf("log: recover event index: %w", err)
}
s.indexReady = true
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gate acquired here is held across KMS signing, s.log.Append (whose Tessera future has no context and, with BatchSize: 1, fsyncs entry bundle, tiles and tree state per leaf) and StoreEvent, and every current-state read takes the same single slot through lockIndexedRead. Reads therefore exclude each other and wait on write-path disk I/O; a stalled tiles filesystem wedges badges, status tokens, receipts and audit behind the 30s timeout while /v2/admin/health and /ready keep returning 200, because indexReady is unexported. Before this change reads served from SQLite through such a stall. The property the gate needs is narrower: no read while the mirror may lag Tessera.

Worth fixing before this ships. A writer mutex serializes check-then-append; an RWMutex (or weighted semaphore) covers only the mirror-lag window, that is recovery and the gap between a resolved append and its StoreEvent, and is never held across future(); readers take the shared side and upgrade only to run recovery. Bound the Tessera wait with the request context and treat an abandoned append as an uncertain write that forces one more recovery. Export a Ready(ctx) error (index ready plus a 2s-bounded IntegratedSize) and have /v2/admin/ready return 503 while it errors, so an orchestrator can see the state instead of routing to an instance that answers 500 on every real route.

Signed-off-by: Connor Snitker <csnitker@godaddy.com>
Signed-off-by: Connor Snitker <csnitker@godaddy.com>
Copilot AI lite review requested due to automatic review settings September 21, 2026 21:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical recovery, lifecycle-state, and status-token correctness findings remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 High severity

Open (4)
What changed in this PR

This PR hardens TL ingestion and recovery, aligns certificate-derived status behavior, and makes RA activation replay durable without changing public wire shapes.

Changes:

  • Adds serialized ingestion, recovery, readiness checks, antispam, and writer locking.
  • Adds lifecycle validation and durable activation seals.
  • Updates certificate status handling, schemas, migrations, documentation, and tests.
File Summary
spec/​api-spec-v2.yaml Documents activation conflicts.
spec/​api-spec-tl-v2.yaml Documents lifecycle deduplication and validation codes.
internal/​tl/​service/​statustoken.go Filters expired certificates and bounds token lifetime. Critical finding (1 vote): duplicate fingerprints can bypass expiry filtering.
internal/​tl/​service/​statustoken_internal_test.go Updates status-token tests.
internal/​tl/​service/​schemas/​V2.json Documents lapsed certificate evidence.
internal/​tl/​service/​schemas/​V1.json Documents legacy certificate evidence.
internal/​tl/​service/​receipt_test.go Extends receipt fixtures.
internal/​tl/​service/​readiness.go Adds index readiness checks. Moderate finding (1 vote): readiness can remain healthy while the writer is stalled.
internal/​tl/​service/​readiness_test.go Tests readiness locking.
internal/​tl/​service/​log.go Serializes appends and fences indexed reads. Critical finding (1 vote): latest-state reads can report an active state after a terminal event; nit (1 vote): successful appends lack structured INFO logging.
internal/​tl/​service/​ingest.go Adds lifecycle validation and leaf recovery. Critical findings (1 vote each): V0 recovery is unsupported, and duplicate historical leaves can resurrect terminal states. Moderate finding (1 vote): noncanonical host spellings can cause state conflicts.
internal/​tl/​service/​ingest_test.go Tests ingestion, recovery, and lifecycle behavior.
internal/​tl/​service/​envelope_wrapper.go Derives family-based certificate expiry. Moderate finding (1 vote): malformed V1 singleton expiry values can make badge and token paths disagree.
internal/​tl/​service/​envelope_wrapper_test.go Tests certificate overlap handling.
internal/​tl/​service/​certificate_status_test.go Tests badge and token expiry consistency.
internal/​tl/​service/​badge.go Shares lifecycle status derivation.
internal/​tl/​receipt/​statustoken.go Caps token expiration by certificate validity.
internal/​tl/​logstore/​writerlock.go Adds process-level writer locking.
internal/​tl/​logstore/​writerlock_test.go Tests writer-lock exclusivity.
internal/​tl/​logstore/​log.go Integrates locking and antispam.
internal/​tl/​logstore/​log_test.go Tests duplicate suppression.
internal/​tl/​event/​v1/​event.go Validates V1 expiry evidence.
internal/​tl/​event/​event.go Validates V2 expiry evidence.
internal/​ra/​service/​registration.go Persists and replays activation evidence.
internal/​ra/​service/​registration_test.go Updates outbox test doubles.
internal/​ra/​service/​lifecycle.go Supports durable activation replay.
internal/​ra/​service/​activation_replay_test.go Tests exact activation replay.
internal/​domain/​attested_expiry.go Adds shared expiry parsing.
internal/​domain/​attested_expiry_test.go Tests expiry parsing.
internal/​adapter/​store/​sqlitetl/​migrations/​006_recover_duplicate_leaves.sql Preserves duplicate leaves for recovery.
internal/​adapter/​store/​sqlitetl/​migrations/​005_agent_state_index.sql Adds lifecycle indexing.
internal/​adapter/​store/​sqlitetl/​migration_test.go Tests migration preservation.
internal/​adapter/​store/​sqlitetl/​events.go Adds gap recovery and lifecycle queries.
internal/​adapter/​store/​sqlite/​migrations/​014_activation_seals.sql Adds activation-seal storage.
internal/​adapter/​store/​sqlite/​activation_seals.go Implements activation-seal persistence.
internal/​adapter/​store/​sqlite/​activation_seals_test.go Tests seal persistence across restart.
internal/​adapter/​docsui/​openapi/​tl.yaml Mirrors TL contract documentation.
internal/​adapter/​docsui/​openapi/​ra.yaml Mirrors RA contract documentation.
go.mod Adds synchronization and system dependencies.
docs/​operations/​tl-recovery-upgrade.md Documents upgrade and recovery procedures.
cmd/​ans-tl/​main.go Wires recovery, locking, and readiness.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +143 to +144
oldTime, oldErr := time.Parse(time.RFC3339, prior.Timestamp)
newTime, newErr := time.Parse(time.RFC3339, env.Timestamp())
case header.SchemaVersion == eventv1.SchemaVersion:
env = &eventv1.Envelope{}
default:
return nil, fmt.Errorf("unsupported stored schema %q", header.SchemaVersion)
Comment on lines +369 to +373
unlock, err := s.lockIndexedRead(ctx)
if err != nil {
return nil, err
}
defer unlock()
Comment on lines +181 to +185
if fp == "" || seen[fp] {
continue
}
if raw, ok := m["notAfter"]; ok {
value, _ := raw.(string)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

3 participants