Skip to content

fix(storage): ask the bucket whether this instance can serve before answering ready - #132

Merged
BryanFRD merged 1 commit into
mainfrom
fix/readiness-asks-the-bucket
Aug 18, 2026
Merged

fix(storage): ask the bucket whether this instance can serve before answering ready#132
BryanFRD merged 1 commit into
mainfrom
fix/readiness-asks-the-bucket

Conversation

@BryanFRD

Copy link
Copy Markdown
Contributor

Closes #126.

/ready answered by writing a probe file under the storage root. With
LFSX_STORAGE=s3 that is the write buffer, not the storage: an instance whose
credentials were rotated, whose bucket was deleted, or that simply cannot reach
the endpoint passed the probe, stayed in rotation, and failed every transfer it
was handed with a 500.

Readiness now asks the backend it actually serves from. The volume probe stays,
because a bucket deployment still stages locally and a broken mount still takes
the instance out. On top of it, a bucket deployment gets one HEAD on the
bucket. Either failing answers 503, and the two are named apart in the response
and the log, because a full disk and a rotated key are not the same afternoon.

The bucket probe reports only the status it got back, never the store's body:
the store names the bucket in there, and /ready answers whoever asks.

One HEAD per probe is affordable at the chart's ten-second interval. The issue
suggested caching the failure rather than the success if it ever isn't; nothing
here caches yet, since there is no evidence it needs to and a cached readiness
answer is the kind that reports an outage late.

Verification

an_instance_that_cannot_reach_its_bucket_is_not_ready covers both directions:
a reachable bucket answers 200, and a bucket that is gone answers 503 while the
staging volume is untouched and perfectly writable. Checked against the old
behaviour before trusting it: with the bucket probe removed it answers 200,
which is the instance staying in rotation the issue describes.

The existing local-mode readiness tests still cover the volume path unchanged.

docs/operations.md said "/ready covers the volume", which stops being true
here, so it is updated in the same diff.

  • cargo fmt --check, cargo clippy --all-targets -- -D warnings
  • full test suite, plus 22 tests against real MinIO

Copilot AI lite review requested due to automatic review settings August 18, 2026 05:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@BryanFRD
BryanFRD enabled auto-merge (squash) August 18, 2026 05:48

@ferrfleet ferrfleet Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed the diff end to end. The change does what the description claims: Store::writable() now checks the staging volume and, for a bucket backend, does a HeadBucket too, and the two failures are named apart in the log and response. S3Store::reachable/Keyspace::reachable report only the status code, not the bucket's response body, so the bucket name isn't leaked through the unauthenticated /ready route. Docs update matches the new behavior, and the new MinIO test (an_instance_that_cannot_reach_its_bucket_is_not_ready) exercises both the happy path and the deleted-bucket path while confirming the staging volume stays untouched.

Nit: ready() now returns error.to_string() as the response body instead of a fixed string. Today that's safe (the staging-volume error is a bare OS error, the bucket error is just a status code), but /ready is unauthenticated, so any future Error::Storage variant that happens to format a path, endpoint, or credential detail into its Display would leak straight out. Worth a comment on ready() flagging that constraint, or keeping the body deliberately terse the way reachable()'s error already is.

No blocking issues.

@BryanFRD
BryanFRD merged commit 847135e into main Aug 18, 2026
23 checks passed
@BryanFRD
BryanFRD deleted the fix/readiness-asks-the-bucket branch August 18, 2026 05:50
@github-actions

Copy link
Copy Markdown

SonarQube — aucune nouvelle issue

Comparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Readiness never asks the bucket, so an instance that cannot serve stays in rotation

2 participants