Skip to content

fix: sync s3 metadata writes#382

Merged
alecthomas merged 1 commit into
mainfrom
aat/sync-s3-metadata-writes
Jul 3, 2026
Merged

fix: sync s3 metadata writes#382
alecthomas merged 1 commit into
mainfrom
aat/sync-s3-metadata-writes

Conversation

@alecthomas

Copy link
Copy Markdown
Collaborator

Persist metadata writes before updating local S3 backend state so storage errors are returned to callers instead of being hidden by deferred flushes.

Persist metadata writes before updating local S3 backend state so storage errors are returned to callers instead of being hidden by deferred flushes.
@alecthomas alecthomas requested a review from a team as a code owner July 3, 2026 12:52
@alecthomas alecthomas requested review from worstell and removed request for a team July 3, 2026 12:52
@alecthomas alecthomas enabled auto-merge (squash) July 3, 2026 12:53
@alecthomas alecthomas merged commit cb44752 into main Jul 3, 2026
7 checks passed
@alecthomas alecthomas deleted the aat/sync-s3-metadata-writes branch July 3, 2026 12:55

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: de381755a3

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread internal/metadatadb/s3.go
Comment on lines 86 to 89
backend: s,
name: name,
state: make(map[string]any),
done: make(chan struct{}),
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Restore periodic S3 metadata refreshes

With the old syncLoop no longer started for each namespace, an S3-backed process only sees remote metadata changes after an explicit Namespace.Flush; I checked the production wiring and Flush is not called by the tiered cache or git strategy after config.Load wires the store in. In a multi-replica tiered cache, a replica that only serves reads will therefore keep its local cache-etags map stale indefinitely, so Tiered.invalidateStale can continue serving an out-of-date lower tier even after another replica synchronously persisted a new authoritative ETag.

Useful? React with 👍 / 👎.

Comment on lines +82 to 84
func (n *Namespace) apply(ops ...Op) error {
return errors.Wrap(n.backend.Apply(context.Background(), n.name, ops...), "apply metadata op")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Propagate cancellation into synchronous metadata writes

Now that S3 Apply performs the lock/load/store path synchronously, using context.Background() for every public metadata write discards the caller's request deadline/cancellation. Under an S3 outage or a stuck namespace lock, callers such as tiered cache create/delete can remain blocked in lockNamespace or MinIO calls even after the HTTP request context has been canceled, because this write path has no cancellable context to observe.

Useful? React with 👍 / 👎.

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.

1 participant