Skip to content

[Guardian] Improve S3 log write robustness#792

Open
mskd12 wants to merge 3 commits into
mainfrom
deepakmaram/guardian-limiter-s3-reconcile
Open

[Guardian] Improve S3 log write robustness#792
mskd12 wants to merge 3 commits into
mainfrom
deepakmaram/guardian-limiter-s3-reconcile

Conversation

@mskd12

@mskd12 mskd12 commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

The enclave's committed limiter/seq state can silently lag S3 (the source of truth): an S3 log write can return an error while the record actually persists (a lost ACK), so the enclave reverts its limiter while a durable record exists. Since the seq gate checks only local next_seq, the enclave can then reuse a seq S3 already committed (a collision that corrupts recovery) or under-count the limiter.

Fix

  • Shared fail-stop writer: every non-init log creates one LogRecord and retries that exact key and body every 10 seconds. If no attempt is confirmed within five minutes, the enclave aborts. The writer runs as a detached Tokio task so caller cancellation cannot abandon an in-flight write.
  • Init exception: only log_init skips grace-period retries, providing quick fail-stop for basic S3 write/access issues. A failed init leaves an incomplete enclave that cannot serve and is expected to restart, so S3 being ahead after a lost acknowledgement is acceptable.
  • Verified conditional creates: writes use If-None-Match: *. A 412 triggers a locked point read; matching bytes confirm the intended record is durable, while different bytes indicate an unrecoverable write-once key collision and abort immediately.
  • Withdrawal commit ownership: a successful withdrawal task owns the limiter mutex guard until its success log is confirmed. There is no rollback path after consumption; any unexpected post-consume error aborts, while the guard keeps later withdrawals serialized behind the durable write.
  • Simplified heartbeat: heartbeat now only schedules records and advances its sequence after confirmed durability. Retry and fail-stop behavior comes from the shared writer.
  • Restart recovery: after fail-stop, activation waits for the prior session quiet period and reconstructs limiter state from S3.

Testing

  • Matching and mismatching 412 responses.
  • Application retries beyond the AWS SDK retry budget.
  • Fail-stop after the configured failure interval.
  • Full hashi-guardian library test suite.

@mskd12 mskd12 requested a review from bmwill as a code owner July 9, 2026 22:08
@mskd12 mskd12 marked this pull request as draft July 9, 2026 22:14
@mskd12 mskd12 changed the title [Guardian] Reconcile limiter to S3 before each withdrawal; idempotent log writes [Guardian] Suspect-write reconcile: heal limiter/S3 lag from lost-ack writes Jul 9, 2026
@mskd12 mskd12 force-pushed the deepakmaram/guardian-limiter-s3-reconcile branch 2 times, most recently from 15fb04c to 2663921 Compare July 10, 2026 00:04
@mskd12 mskd12 marked this pull request as ready for review July 10, 2026 00:05
@mskd12 mskd12 force-pushed the deepakmaram/guardian-limiter-s3-reconcile branch from 2663921 to 9de3d20 Compare July 10, 2026 00:09
@mskd12 mskd12 marked this pull request as draft July 10, 2026 16:18
@mskd12 mskd12 changed the title [Guardian] Suspect-write reconcile: heal limiter/S3 lag from lost-ack writes [Guardian] Improve S3 log write robustness Jul 10, 2026
@mskd12 mskd12 force-pushed the deepakmaram/guardian-limiter-s3-reconcile branch from 9de3d20 to 92c4d8e Compare July 10, 2026 22:01
@mskd12 mskd12 removed the request for review from 0xsiddharthks July 10, 2026 22:10
@mskd12 mskd12 force-pushed the deepakmaram/guardian-limiter-s3-reconcile branch 3 times, most recently from ee1f65a to b0be793 Compare July 10, 2026 22:28
@mskd12 mskd12 force-pushed the deepakmaram/guardian-limiter-s3-reconcile branch from b0be793 to b3a1bfc Compare July 10, 2026 22:29
mskd12 and others added 2 commits July 10, 2026 18:33
@mskd12
Merge remote-tracking branch 'origin/main' into deepakmaram/guardian-…
7a2fa67 
…limiter-s3-reconcile

# Conflicts:
#	crates/hashi-guardian/src/enclave.rs
@mskd12 @claude
[Guardian] Fix status-lock stall and retry log spam
c0ed07b 
Two incidental issues from the S3 durable-or-abort refactor:

- limiter_state()/limiter_config() took a bare untimed lock, so a
  get_guardian_info status poll could block for the full 5-min write
  grace behind an in-flight durable write. Bound both reads by
  LIMITER_LOCK_TIMEOUT via a shared helper, returning None on timeout
  (info() already treats None as unavailable).

- write_at_key logged the full serialized record (request data + BTC
  signatures) at info on every attempt; now inside the retry loop, that
  re-emitted the record ~18x during an outage. The record is already
  durably written to S3, so drop the redundant trace-stream dump.

Also fixes a doc_lazy_continuation clippy error in heartbeat.rs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mskd12 mskd12 requested a review from benr-ml July 10, 2026 23:50
@mskd12 mskd12 marked this pull request as ready for review July 10, 2026 23:50
zhouwfang
zhouwfang reviewed Jul 11, 2026
View reviewed changes
Comment thread crates/hashi-guardian/src/s3_client.rs
/// Delay between application-level retries of an immutable S3 log write.
const S3_WRITE_RETRY_INTERVAL: Duration = Duration::from_secs(10);
/// Maximum write failure interval before the enclave aborts.
const MAX_S3_WRITE_FAILURE_INTERVAL: Duration = Duration::from_mins(5);

@zhouwfang zhouwfang Jul 11, 2026

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.

This is now half of the no-double-live invariant: max alive-but-silent \approx HEARTBEAT_INTERVAL + MAX_S3_WRITE_FAILURE_INTERVAL \approx 6min, which must stay below OTHER_SESSION_QUIET_PERIOD (10 min, as in heartbeat_checks.rs), but nothing ties the constants together. Can we document or const-assert the relation so a future bump can't silently open a double-live window?

Comment thread crates/hashi-guardian/src/s3_client.rs
Ok(())
}

async fn verify_existing_write(&self, key: &str, expected_body: &[u8]) -> GuardianResult<()> {

@zhouwfang zhouwfang Jul 11, 2026

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.

It doesn't need the lock-metadata assertion: byte-compare alone decides durable-vs-squat, and going through get_locked_object_bytes means a lock-less squat (or a retention-read perms gap) surfaces as the generic 5-min timeout panic instead of the crisp mismatch one. Consider a plain GET here.

benr-ml
benr-ml approved these changes Jul 12, 2026
View reviewed changes

@benr-ml benr-ml 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.

the new logic looks good to me, but pls address the other questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhouwfang zhouwfang zhouwfang left review comments
@benr-ml benr-ml benr-ml approved these changes
@bmwill bmwill Awaiting requested review from bmwill bmwill is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mskd12 @zhouwfang @benr-ml