feat(EN-1827): freeze idempotency expiry per outcome (PR3) - #1907
feat(EN-1827): freeze idempotency expiry per outcome (PR3)#1907Azorlogh wants to merge 6 commits into
Conversation
✅ Approve — automated reviewNo actionable correctness defects remain in the current diff. The previously reported preload freshness, eviction cutoff, and Pebble deletion lifecycle issues are addressed at HEAD. No findings. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1907 (comment)
Final review — PR #1907 "feat(EN-1827): freeze idempotency expiry per outcome (PR3)"This PR moves the idempotency retention deadline from node-local config into the data: each apply freezes an absolute Standards1. The
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1907 +/- ##
================================================
+ Coverage 83.12% 83.16% +0.04%
================================================
Files 458 458
Lines 42329 42348 +19
================================================
+ Hits 35184 35219 +35
+ Misses 7140 7124 -16
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…utoff The stale-preload guard added earlier compared expires_at against LastAppliedTimestamp, but an IdempotencyEviction is a technical-only proposal that returns before AdvanceHLC, so it never advances the HLC. On an idle cluster the HLC lags the eviction's wall-clock cutoff, and a proposal whose plan predates the eviction would re-inject the removed outcome into the cache (map ahead of Pebble; a later eviction could then double-SingleDelete the main key). Raised by NumaryBot and shipfox-ai on PR #1907. Track a monotonic high-water cutoff of every applied eviction in FSMState.LastIdempotencyEvictionCutoff, advanced (and persisted at ZoneGlobal/SubGlobLastIdempotencyEvictionCutoff, alongside the eviction's deletions) by applyIdempotencyEviction, and re-read in RecoverState so replay/snapshot restore is deterministic. The preload gate now skips a value whose expires_at is at or below that cutoff — identifying the eviction that removed it rather than inferring expiry from the order HLC. Also add the missing coverage: a load -> evict -> stale-preload regression asserting map/Pebble parity and no double-delete, an incremental-restore parity test for the rebuilt expires_at + time index (invariant #11), and fix a deployment-profiles.md section still describing the removed persisted-TTL validation.
|
@shipfox-ai thanks — all three addressed in 1554467:
|
|
This PR moves the idempotency TTL out of node-local persisted config into the Raft-replicated cluster policy: each apply freezes an absolute Recommendation: request changes — one test-only change required; no code defect found. Standards
Spec
Both models' remaining candidates were rejected on verification: the discarded-error Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM. |
|
@shipfox-ai — both items from the re-review addressed in
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).
Summary: #1907 (comment)
|
This PR freezes each idempotency outcome's absolute Standards1. [Medium-low] Invariant #11 proof is incomplete for the checkpoint-seeded half of the new
Concrete impact: a future regression that drops or corrupts No other documented-standard violations found: deterministic-FSM invariant #2 is improved (no node-local TTL or wall clock in apply; the preload gate reads replicated committed state), invariant #8 is satisfied (checker verification + tamper test), the protobuf field removal follows the AGENTS.md delete-and-realign rule off the hand-decoded paths, and documentation was updated with the behavior change. Judgement-call smells from the sub-reviews (the SpecSpec source: the PR body ("feat(EN-1827): freeze idempotency expiry per outcome (PR3)"). The implementation is faithful: every enumerated requirement — freeze from committed policy before the proposal, no node-local TTL read in apply, 1. [Low] The PR body's impact enumeration omits new replicated state the preload gate actually reads. "Architecture / behavior impact → FSM/Raft" says "The preload gate and eviction read only the stored Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM. |
| v := ik.GetValue() | ||
| if v != nil && (v.GetFirstLogSequence() > 0 || v.GetFailure() != nil) { | ||
| if v != nil && (v.GetFirstLogSequence() > 0 || v.GetFailure() != nil) && | ||
| !IdempotencyEvicted(v.GetExpiresAt(), fsm.State.LastIdempotencyEvictionCutoff) { |
There was a problem hiding this comment.
🔴 [blocker] A stale expired preload can overwrite a newer live outcome
The eviction cutoff proves that this plan-carried value was not already evicted; it does not prove that it is still the newest value for the key. A and B can both build while Pebble contains the same expired outcome V0 and the cutoff is still below V0.expires_at. A then applies, treats V0 as expired, executes, and installs a live V1. When B applies, this Put overwrites V1 with its stale V0. The idempotency gate then sees V0 as expired and executes again: with identical content I observed a second created log (sequences 3 then 4); with different content I observed no idempotency conflict and another created log. Recovery first restored V1 correctly, but the stale preload overwrote it again, so restart does not close the race.
The existing load → eviction → stale-preload regression covers the case fixed by LastIdempotencyEvictionCutoff. Please also protect the distinct expired-but-not-yet-evicted replacement interleaving; otherwise at-most-once and conflict semantics remain bypassable.
There was a problem hiding this comment.
Fixed in ee2434f — the preload re-injection now also gates on freshness: it re-injects only when the plan value is not older than the map's current value (created_at, the strictly-monotonic apply HLC), so a stale plan value can't clobber a newer live outcome. Regression TestPreload_DoesNotOverwriteNewerOutcomeWithStalePlan fails on the old unguarded code.
| if value.GetCreatedAt() > cutoffMicros { | ||
| // Defensive: the leader scan returned this hash with a stale | ||
| // cutoff. Don't evict a still-live entry. | ||
| if value.GetExpiresAt() == 0 || value.GetExpiresAt() > cutoffMicros { |
There was a problem hiding this comment.
🔴 [blocker] Expired-key reuse violates the claimed SingleDelete lifecycle
The production path still permits Set(K,A) → Set(K,B) → SingleDelete(K). Once A is logically expired, apply treats the key as reusable and SaveIdempotencyKey writes B with Set to the same main key while adding a second expiry-index row. When B later reaches this cutoff, Evict emits SingleDelete on that twice-written main key. The in-memory map gate prevents a duplicate SingleDelete for the repeated hash, but it does not undo the two Sets.
A deterministic DAL/Pebble fixture confirmed the reachability: Set(A), flush, Set(B), flush produced two scanned time-index rows and one production Evict SingleDelete. This particular run remained absent after delete, CompactAll, close, and reopen, so I did not observe a resurrection; however, that does not establish safety for a lifecycle Pebble documents as undefined. The write-once/delete-once guarantee in the comment below is therefore not actually enforced.
There was a problem hiding this comment.
Fixed in ee2434f — Evict now uses a plain Delete instead of SingleDelete. The main key is legitimately re-Set on reuse (an expired key reused by a fresh proposal), so it is NOT write-once; a plain Delete tombstones every prior Set and is correct — matching backup_jobs.go's existing DeleteKey pattern for rewritten keys. Updated the now-false write-once comments in Evict and the scheduler. Regression TestEviction_ReusedKeyDeletesCleanly.
|
@shipfox-ai — both re-review items addressed:
Separately, gfyrag found two further blockers in the preload/eviction path (a stale-preload overwrite of a newer outcome, and |
|
This PR freezes each idempotency outcome's absolute Standards1. [High] Invariant #11 / incremental-restore contract — the Preserved No other confirmed findings. Candidate concerns investigated and closed against the code: the persisted-config protobuf field renumbering (no SpecNo confirmed material finding. Requirements were verified against the PR body as spec source at HEAD (7d18106): the single per-apply freeze from the policy committed before the proposal (verified through Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM. |
The FSM read a node-local idempotency TTL when deciding whether a committed outcome had expired (IsExpired in apply), so two nodes configured with different TTLs could diverge on whether the same committed outcome was still live — a deterministic-FSM boundary violation (EN-1797). Each apply now freezes the outcome's absolute expires_at once, from the TTL in the cluster policy committed before that proposal (created_at + IdempotencyTtlMicros; 0 = never; overflow saturates), and stores it on the outcome. Every later expiry decision (IsExpired, preload re-injection, the leader eviction scan) reads only that stored value, so no node-local TTL can move a committed outcome's lifetime. The expiry is chain-bound in the audit header, so restore and the checker re-derive and verify it from the chain. - proto: add expires_at to IdempotencyKeyValue and Idempotency; drop PersistedConfig.idempotency_ttl_seconds and its boot backfill/mismatch check (the flag now only seeds the desired cluster policy). - store: key the eviction time index by expires_at; the scheduler cutoff is wall-clock now. - checker: verify the stored expires_at against the chain-derived value.
…utoff The stale-preload guard added earlier compared expires_at against LastAppliedTimestamp, but an IdempotencyEviction is a technical-only proposal that returns before AdvanceHLC, so it never advances the HLC. On an idle cluster the HLC lags the eviction's wall-clock cutoff, and a proposal whose plan predates the eviction would re-inject the removed outcome into the cache (map ahead of Pebble; a later eviction could then double-SingleDelete the main key). Raised by NumaryBot and shipfox-ai on PR #1907. Track a monotonic high-water cutoff of every applied eviction in FSMState.LastIdempotencyEvictionCutoff, advanced (and persisted at ZoneGlobal/SubGlobLastIdempotencyEvictionCutoff, alongside the eviction's deletions) by applyIdempotencyEviction, and re-read in RecoverState so replay/snapshot restore is deterministic. The preload gate now skips a value whose expires_at is at or below that cutoff — identifying the eviction that removed it rather than inferring expiry from the order HLC. Also add the missing coverage: a load -> evict -> stale-preload regression asserting map/Pebble parity and no double-delete, an incremental-restore parity test for the rebuilt expires_at + time index (invariant #11), and fix a deployment-profiles.md section still describing the removed persisted-TTL validation.
Addresses shipfox's re-review of the per-outcome expires_at freeze. Restore parity (invariant #11): a RebuildDelta unit test alone does not meet the incremental-restore contract's cross-lifecycle requirement. Extend the e2e restore suite (restore_idempotency_test.go) with a CheckStore assertion on the restored node — the checker re-derives and verifies expires_at from the audit chain, so a clean result proves the frozen retention deadline (and its eviction time-index entry) survived restore. Reword the RebuildDelta unit test's comment to name itself the lowest-level check and point at the e2e proof, and record the Rebuilt classification in rebuildIdempotency's doc. Doc: IdempotencyExpiresAt is stamped on every keyed proposal, including conflict/non-freezable ones that freeze nothing (the checker derives no expectation from those); tighten audit-chain.md accordingly.
… plain Delete Addresses two correctness blockers gfyrag found in the idempotency path. Blocker 1 — stale preload overwriting a newer outcome: the eviction-cutoff gate proves a plan-carried value was not evicted, but not that it is still the newest value for the key. Two proposals can carry the same expired-but-not-yet-evicted value; if the first supersedes it with a fresh outcome, the second's Preload re-injected the stale copy over the live one, letting the duplicate re-execute (at-most-once break). Preload now re-injects only when the plan value is not older than the map's current value (created_at is the strictly-monotonic HLC). Blocker 2 — SingleDelete lifecycle: the main key is legitimately re-Set on reuse (a fresh proposal reusing an expired key writes a new outcome over the old), so it is NOT write-once. Evict used SingleDelete, which is undefined over multiple Sets and can resurrect a stale outcome at compaction. Switch to a plain Delete (matching backup_jobs.go's existing pattern for rewritten keys). Regressions: TestPreload_DoesNotOverwriteNewerOutcomeWithStalePlan (fails on the old unguarded code) and TestEviction_ReusedKeyDeletesCleanly.
Addresses shipfox's re-review: the e2e restore suite checkpoints an empty store, so only the Rebuilt (delta) path was covered. Add TestRebuildDelta_PreservesCheckpointExpiresAt — a checkpoint-carried outcome with a finite expires_at, folded past an unrelated delta key, must keep its expires_at and eviction time-index entry, asserted against a literal expected value (an oracle independent of the shared audit decoder the restore writer uses).
|
Follow-up on Methodology concernSeveral test comments describe a production lifecycle that the fixture itself does not execute. A white-box assertion immediately around the changed helper is useful as a unit test, but it must not be presented as proof of the cross-component race, restart behavior, or storage lifecycle that motivated the fix. The regression should fail for the original externally observable reason, not merely because the new conditional or API call is absent. 1. Exercise the complete stale-preload interleaving
Please add a deterministic FSM-level regression which:
This should test the actual at-most-once and conflict contracts. Avoid inserting an unrelated replay between A and B unless the reserved 2. Finish the storage-lifecycle proof
Please assert the exact precondition (two index rows for the reused hash and one logical eviction), then verify absence immediately after delete, after 3. The Preserved restore path is still not cross-lifecycleI agree with the current Shipfox finding. Please put a finite-expiry keyed outcome before a real full checkpoint, add a meaningful non-empty post-checkpoint delta, restore through the production path, compare the literal Green CI cannot close these gaps because the current fixtures do not traverse the failure surfaces above. I am not approving at this point. |
shipfox's [High]: the RebuildDelta unit test alone does not satisfy the incremental-restore contract's cross-lifecycle requirement for the Preserved (checkpoint-carried) half of the expires_at projection, and the rebuild_test.go comment overstated the e2e coverage. Extend the e2e restore suite to freeze a keyed outcome BEFORE the full checkpoint (Preserved via the raw-SST copy) alongside the existing post-checkpoint delta keys (Rebuilt), and assert both dedup after the real restore + CheckStore. Correct the rebuild_test.go comment to scope its claim and point at both unit tests plus the e2e for the full cross-lifecycle proof.
7d18106 to
35f68bd
Compare
|
@shipfox-ai — the [High] Preserved-path finding is addressed in
Also rebased onto the current |
Final review — PR #1907 (feat/idempotency-expiresat, EN-1827 PR3)I verified every retained candidate finding from the GLM and Codex reports against Resolution of contradictions between the two reports
Standards
Otherwise no confirmed material Standards finding: protobuf rules are followed (sequential renumber, no Spec
Otherwise the Spec axis has no confirmed material finding: every requirement in the PR body maps to a verified hunk (apply-time freeze including batch-freeze semantics, chain binding with golden test, checker verification with a non-circular tamper test, Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM. |
3 participants
What changed
The FSM no longer reads a node-local idempotency TTL when deciding whether a committed outcome has expired. Each apply freezes the outcome's absolute
expires_atonce, from the TTL in the cluster policy committed before that proposal, stores it onIdempotencyKeyValue, and chain-binds it in the audit header.PersistedConfig.idempotency_ttl_secondsand its boot backfill/mismatch check are removed;--idempotency-ttlnow only seeds the desired cluster policy.Why
Reading the node-local
--idempotency-ttlinside the apply-time expiry check (IsExpired) let two nodes configured with different TTLs diverge on whether the same committed outcome was still live — a deterministic-FSM boundary violation (EN-1797). This is PR3 of the EN-1827 replicated-cluster-policy umbrella: PR1 made the TTL replicated; this closes the remaining node-local read.Product / operational motivation
docs/technical/architecture/subsystems/fsm/deterministic-fsm.md§3.5.deterministic-fsm.md,audit-vs-technical-state.md,subsystems/admission/idempotency.md,subsystems/checker/audit-chain.md.Technical decision
expires_at = created_at + committed-policy TTLper outcome at apply time (0= never; overflow saturates), store it on the outcome, and bind it in the audit header so restore and the checker read it back from the chain. Eviction keys its time index byexpires_at; the leader scan takes a wall-clock cutoff.Risk
MEDIUM — touches the audit hash chain (a new bound field) and a persisted projection. v3 is unreleased so there is no compatibility burden (fields deleted + renumbered), and the checker verifies the new
expires_atagainst the chain-derived value.Validation
bash scripts/agent-checkinternal/infra/state/...,internal/application/check/...,internal/bootstrap/...,internal/adapter/grpc/...,internal/infra/node/...New tests: per-outcome freeze from committed policy (+ non-retroactivity + audit-chain binding), TTL=0 never-expires,
expires_atround-trip through recovery,IdempotencyExpiresAt/IdempotencyExpiredunit tests, and a checkerexpires_at-tamper case.Architecture / behavior impact
Idempotency.expires_atis now bound into the header pre-image (tamper-evident; golden test updated).IdempotencyKeyValue.expires_atadded;PersistedConfig.idempotency_ttl_secondsremoved and field numbers realigned (v3 unreleased, per RULE/CLAUDE.md). New replicatedFSMState.LastIdempotencyEvictionCutoff(persisted underSubGlobLastIdempotencyEvictionCutoff0x14, advanced inapplyIdempotencyEviction, reloaded inRecoverState), the eviction high-water mark the preload gate consults.expires_at, the replicatedLastIdempotencyEvictionCutoff(an eviction never advances the HLC, so the gate cannot rely on it), and the current outcome'screated_at(freshness); eviction reads only the storedexpires_at.expires_at.Review focus
machine.go: the singleidempotencyExpiresAtfreeze point feeding the audit stamp, both success/failure freeze paths, and the preload gate — confirm it always reads the pre-apply committed policy.audit_envelope.go+ golden test: the new chain-bound field.checker.goidempotencyMismatch: theexpires_atverification.Known concerns
None. This branches off the chapters-removed base (EN-1945): the checker's archived idempotency re-derivation is already gone, so PR3's checker delta reduces to the
expires_atverification.