Skip to content

Toy server: support maintenance-notifications - #3191

Draft
mgravell wants to merge 1 commit into
mainfrom
marc/maint-optin-server
Draft

Toy server: support maintenance-notifications#3191
mgravell wants to merge 1 commit into
mainfrom
marc/maint-optin-server

Conversation

@mgravell

@mgravell mgravell commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

support the maintenance-notification opt-in, and sending events

Deliberately on RedisServer rather than a bespoke test subclass. The opt-in is an ordinary command, so any test should be able to use it, and a server that never sends a notification is the normal case - every OSS, Valkey and Garnet build behaves that way, and so will our own docker topology. The interesting behaviour is all client-side, so the fake should not be a special place.

CLIENT MAINT_NOTIFICATIONS <ON|OFF> [parameter value ...] per the contract: a bare ON is valid and means "server defaults", moving-endpoint-type is the only parameter defined so far, and its five values are validated. Unknown parameters are refused rather than ignored - the client is asking the server to do something specific, and silently not doing it is worse than saying no. State is per connection, including a count, since re-arming after a reconnect is a requirement and a count is what distinguishes that from having opted in once.

MaintenanceNotifications selects how the server answers: accept, reject as an unknown subcommand (what a server that never heard of it does), or reject as disabled (what one with the feature flag off does). A client has to survive all three, so a test has to be able to ask for all three.

Sending covers MOVING with or without an address, the shard-scoped and slot-scoped families, explicit or generated sequence ids - the contract never defines those, so repeating one deliberately is part of what the fake owes us - and a raw-push hook for malformed frames. Notifications go only to connections that opted in; a raw push is not gated, which is the contrast the tests assert.

Two bugs the tests caught: the count returned was clients visited rather than sent to, because ForAllClients' Action overload returns one per client regardless; and an assertion comparing against ClientCount was racy, since under RESP2 the subscription connection can register between the send and the read.

Checklist

  • I fully and freely contribute this code in accordance with the project license (and am legally able to do so)
  • I take responsibility for this contribution's quality and correctness, including any portions produced with AI assistance (see CONTRIBUTING.md).

@mgravell

mgravell commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

@philon-msft my plan is to turn on SCH for AMR (in the options-provider) pre-emptively - i.e. ahead of AMR actually supporting it; this would be in "auto" mode, which means "only try in RESP3 mode; if it gets rejected: meh, no problem" - however, before I do this, I'd want to validate against a real AMR endpoint that this doesn't break anything. Would you be able to lend me something to test this against, or otherwise help me validate this? If we need to do "turn it on in AMR if the server reports version > XXX, otherwise don't because it breaks the proxy/whatever" then that's totally fine too: we'll make it work - I just want to not break your users.

Specifically, this would issue an additional command: CLIENT MAINT_NOTIFICATIONS ON pipelined during handshake for RESP3 connections only.

  • if this causes the connection to become severed today (which seems unlikely), that's a bad thing, and would mean we can't use blind auto - we'd need to version check or use opt-in for now, deferring auto until AMR confirm the fleet is updated
  • if it just reports -ERR something that's today fine, we'll ignore it
    • however, if this would impact your reporting, you might also want to pre-emptively suppress that at your end
  • if it reports success and just never sends us anything: that's fine - it costs us nothing at the client (although it does make it a little confusing for the client, since there's also an "enabled" mode, which effectively demands support; I don't think we'll cry about it, but in this case, "supported" would be indistinguishable from "silently not supported")

…events

Deliberately on RedisServer rather than a bespoke test subclass. The opt-in is an
ordinary command, so any test should be able to use it, and a server that never
sends a notification is the normal case - every OSS, Valkey and Garnet build
behaves that way, and so will our own docker topology. The interesting behaviour
is all client-side, so the fake should not be a special place.

CLIENT MAINT_NOTIFICATIONS <ON|OFF> [parameter value ...] per the contract: a
bare ON is valid and means "server defaults", moving-endpoint-type is the only
parameter defined so far, and its five values are validated. Unknown parameters
are refused rather than ignored - the client is asking the server to do something
specific, and silently not doing it is worse than saying no. State is per
connection, including a count, since re-arming after a reconnect is a requirement
and a count is what distinguishes that from having opted in once.

MaintenanceNotifications selects how the server answers: accept, reject as an
unknown subcommand (what a server that never heard of it does), or reject as
disabled (what one with the feature flag off does). A client has to survive all
three, so a test has to be able to ask for all three.

Sending covers MOVING with or without an address, the shard-scoped and
slot-scoped families, explicit or generated sequence ids - the contract never
defines those, so repeating one deliberately is part of what the fake owes us -
and a raw-push hook for malformed frames. Notifications go only to connections
that opted in; a raw push is not gated, which is the contrast the tests assert.

Two bugs the tests caught: the count returned was clients *visited* rather than
sent to, because ForAllClients' Action overload returns one per client
regardless; and an assertion comparing against ClientCount was racy, since under
RESP2 the subscription connection can register between the send and the read.
@mgravell
mgravell force-pushed the marc/maint-optin-server branch from 88b79ce to 0f3ae13 Compare August 26, 2026 08:20
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