Pull from upstream - #1
Open
deepakverma wants to merge 523 commits into
Open
Conversation
-Fix spelling
#2659) * add new AddLibraryNameSuffix API for annotating connections with usage * fixup test * new partial for lib-name bits * use hashing rather than array shenanigans * move to shipped; fix comment typo * comment example * reverse comment owner
* Provide new LoggingTunnel API; this * words * fix PR number * fix file location * save the sln * identify smessage as out-of-band * add .ForAwait() throughout LoggingTunnel * clarify meaning of path parameter
This issue was brought to my attention last night (thanks to Badrish Chandramouli): dotnet/dotnet-api-docs#6660 This changeset ensures that we do not honor self-signed certs or partial/broken chains as a result of `X509VerificationFlags.AllowUnknownCertificateAuthority` downstream and adds a few tests and utilities to generate test certificates (currently valid for ~9000 days). Instead we are checking that the certificate we're being told to trust is explicitly in the chain, given that the result of `.Build()` cannot be trusted for this case. This also resolves an issue where `TrustIssuer` could be called but we'd error when _no errors_ were detected (due to requiring chain errors in our validator), this means users couldn't temporarily trust a cert while getting it installed on the machine for instance and migrating between the 2 setups was difficult. This needs careful eyes, please scrutinize heavily. It's possible this breaks an existing user, but...it should be broken if so unless there's a case I'm not seeing.
Further hardening following #2665. This is an additional check to match the .NET implementation for TLS cert checks so that we don't treat a cert flagged as non-TLS-server effectively. This ensures that a certificate either doesn't have OIDs here (valid, backwards compatible) or has the server-certificate OID indicating it's valid for consumption over TLS for us. Cheers @bartonjs for the report and info here.
Also fix a couple typos
…ss ambiguous alternatives (#2702) Closes #2697. All of the replacements were empirically tested to be correct via simple programs in combination with a local redis instance. Notably, there is one worrying nit; in testing it turns out that the `IDatabase.List{Left,Right}Pop(RedisKey, long, CommandFlags)` overload which I talked about in the issue _can_ actually return null, contrary to its nullability annotations. This occurs on missing key; in that case redis replies Nil reply: if the key does not exist. as per https://redis.io/docs/latest/commands/lpop/, which then at https://github.com/StackExchange/StackExchange.Redis/blob/cb8b20df0e2975717bde97ce95ac20e8e8353572/src/StackExchange.Redis/ResultProcessor.cs#L1546-L1547 and later at https://github.com/StackExchange/StackExchange.Redis/blob/cb8b20df0e2975717bde97ce95ac20e8e8353572/src/StackExchange.Redis/ExtensionMethods.cs#L339-L341 turns into a `null`. I briefly attempted to rectify this, but the `RedisValueArrayProcessor` poses a problem here, as changing it to derive `ResultProcessor<RedisValue[]?>` causes the solution to light up in red, and I'd rather not mess with that as a first contribution without at least prior discussion concerning direction there.
* support reading from last message from stream with xread * recover previous formatting * add redisversion and use it in integration tests
* initial implementation of #2706 * build: net8 compat * Docs: add HighIntegrity initial docs * PR fixups * Options fixes * Update tests/StackExchange.Redis.Tests/TestBase.cs * add tests that result boxes / continuations work correctly for high-integrity-mode * - switch to counter rather than entropy - add transaction work to basic tests, to ensure handled * naming is hard * - add explicit connection failure type - burn the connection on failure - add initial metrics * benchmark impact of high performance mode * be more flexible in HeartbeatConsistencyCheckPingsAsync * add config tests --------- Co-authored-by: Nick Craver <nickcraver@microsoft.com> Co-authored-by: Nick Craver <nrcraver@gmail.com>
Closes/Fixes #2726 added two overloads and use a [filter object](https://github.com/StackExchange/StackExchange.Redis/compare/main...atakavci:StackExchange.Redis:ali/maxage?expand=1#diff-6c39ce347d72497c65974f0d78ab9a986be3198e7b2555975ff06410a9831bd8) to avoid a breaking change. Co-authored-by: Nick Craver <nickcraver@microsoft.com>
Co-authored-by: Willem van Ketwich <REPLACE_EMAIL>
docker-compose is not available by default anymore on `ubuntu-latest` [details here]( actions/runner-images#9692)
…ing forward (#2757) * Project: Enable StyleCop and fix existing rules to make PRs easier going forward This adopts StyleCop and fixes most issues (and disables things we wouldn't want) to make PRs more consistent in an automated way and prevent formatting problems affecting git history, etc. It also does all the documentation enforcement. Note: since I had to fix up many anyway, I finally did the `<inheritdoc />` minimization on `IDatabase`/`IDatabaseAsync` to remove 10% of duplicate documentation. That should also make PRs and maintenance less monotonous. * Fix stylecop issues
Closes/Fixes #2715 This PR add support for a set of new commands related to expiration of individual members of hash: > **_HashFieldExpire_** exposes the functionality of commands HPEXPIRE/HPEXPIREAT, for each specified field, it gets the value and sets the field's remaining time to live or expireation timestamp > **_HashFieldExpireTime_** exposes the functionality of command HPEXPIRETIME, for specified field, it gets the remaining time to live in milliseconds or expiration timestamp > **_HashFieldPersist_** exposes the functionality of command HPERSIST, for each specified field, it removes the expiration time > **_HashFieldTimeToLive_** expoes the functionality of command HPTTL, for specified field, it gets the remaining time to live in milliseconds or expiration timestamp --------- Co-authored-by: Nick Craver <nickcraver@microsoft.com> Co-authored-by: Nick Craver <nrcraver@gmail.com>
It seems the Envoy apts have gone missing breaking out build, so instead of relying on the install let's docker compose their image as a proxy against the Redis supervisor instance as a simpler and faster-to-start setup that also works. This rearranged some things to simplify the Docker story overall. A move to AzDO or just GitHub builds would simplify everything further, but we need to figure out Windows testing against a Docker setup in CI. Note: we still can't use Linux containers on a Windows GitHub Actions host (actions/runner#904), so this remains much more complicated and not-really-testing-the-real-thing in the Windows front.
Closes/Fixes #2721 Brings new functions to the API ; - IDatabase.HashScanNoValues - IDatabase.HashScanNoValues - IDatabaseAsync.HashScanNoValuesAsync ...to enable the return type consisting of keys in the hash. Added some unit and integration tests in paralleled to what is there for `HashScan` and `HashScanAsnyc`. Co-authored-by: Nick Craver <nrcraver@gmail.com>
## Issue #2763 ## Solution Simply added a lock around `_handlers` in `ConnectionMultiplexer.Subscription`, like I was suggesting in the issue. ## Unit Test I added one that does exactly what the example code in #2763 was doing & testing for. I used the other tests as template/guide, let me know if something isn't up to spec. --------- Co-authored-by: Nick Craver <nrcraver@gmail.com>
…cks (#2784) * Fix #2778: Run CheckInfoReplication even with HeartbeatConsistencyChecks This is an issue identified in ##2778 and #2779 where we're not updating replication topology if heartbeat consistency checks are enabled because we exit the `if` structure early. This still runs that check if it's due in both cases, without changing the behavior of the result processor. * Add release notes
3.0 is GA now
* investigate and fix #3123 * F+F the test setup * nit, nuke a using direction * in debug: make all string -> RedisValue *explicit* (incomplete, needs a fix to the ship-file) * fix shipfiles for DEBUG hack * docs * fix exp link * I knew we had a Write...(RedisKey) method somewhere! * move NO ONE to RedisLiterals
* propose agents guidelines and skills * add command source notes * ensure `SORT_RO` parse correctly; impact: Execute would not apply command-map rename correctly otherwise * Update AGENTS.md
* Throw a clear error when an Execute command contains whitespace
Execute("ACL SETUSER x") passes a whole command line as the single command
token, which gets sent as one unknown command and comes back as an opaque
server error. Since a redis command token never contains internal whitespace,
this is always a caller mistake, so the ExecuteMessage constructor now fails
fast with an ArgumentException-style RedisCommandException that names the
offending command and shows the correct token-per-argument form. Resolves #2689.
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
* test for simple space only
limit to simple space; if people are being *that* creative, that's on them
* Remove test case for 'echo\thello' command
Removed test case for command with tab character.
---------
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Co-authored-by: Marc Gravell <marc.gravell@gmail.com>
* initial cut * transactions * iteration/evolution * docs * refactored API based on internal conversations * more docs * WIP * clarify MULTI/EXEC logic * avoid value-tuple
* fix: Queue batch commands to backlog during MOVED reconnection When a MOVED-to-same-endpoint triggers reconnection (PR #3003), batch commands issued during the ~50-100ms reconnection window fail immediately with NoConnectionAvailable. This is because: 1. RedisBatch.Execute() calls SelectServer() which returns null for disconnected/reconnecting servers. 2. PhysicalBridge.TryEnqueue() returns false when !IsConnected, unlike TryWriteAsync/TryWriteSync which queue to the backlog. This creates an inconsistency where individual async commands succeed (queued via BacklogPolicy) but batch commands throw during the same reconnection window. Fix: - RedisBatch.Execute(): When SelectServer returns null and BacklogPolicy.QueueWhileDisconnected is enabled, retry with allowDisconnected=true to find the server endpoint. - PhysicalBridge.TryEnqueue(): When disconnected/reconnecting and BacklogPolicy.QueueWhileDisconnected is enabled, queue messages to the backlog instead of returning false. This matches the existing behavior of individual command paths and ensures batch commands are transparently queued and sent after reconnection completes. Tested with proxy redirect scenario (MOVED-to-same-endpoint + disconnect): - Before: 1-3 NoConnectionAvailable errors per MOVED redirect - After: 0 errors, batch waits for reconnection via backlog * test: Add batch backlog tests for MOVED-triggered reconnection Add two integration tests verifying batch commands are queued to the backlog during MOVED-to-same-endpoint reconnection: - MovedToSameEndpoint_BatchCommands_QueuedDuringReconnect: Verifies that a batch containing a MOVED-triggering command completes successfully after reconnection (commands queued to backlog). - MovedToSameEndpoint_SubsequentBatch_QueuedDuringReconnect: Verifies that a second batch issued during the reconnection window is queued rather than throwing NoConnectionAvailable (fire-and-forget scenario). --------- Co-authored-by: advaMosh <advaMosh@users.noreply.github.com>
…es (#3130) SwitchPrimary only reconfigured when the new primary was an unknown endpoint. On an in-place failover (old master demoted, replica promoted), the new master is already in `servers`, so it no-op'd and the client kept writing to the demoted node until restart. Reconfigure with reconfigureAll:true when a known primary's cached role is stale, so connected nodes re-read their role before re-election. Also move the switch reconfigure out of the sentinelConnectionChildren lock. Refs #1891
* LMOVEM * test CI image available * SUNIONCARD/SDIFFCARD * XREAD/XREADGROUP MAXCOUNT/MAXSIZE * cleanup unshipped * fixup test versions
We hand-start every server instance the tests need, so drop a policy-rc.d that tells invoke-rc.d/deb-systemd-invoke not to start services at all. The packaged redis-server unit would otherwise race our own primary for 127.0.0.1:6379 on any run where WSL came up with systemd. That also sidesteps an install hang we saw with the 8.10.0 package: its redis.conf lost "supervised auto", so redis never sd_notify()s, the Type=notify unit never leaves "activating", and the postinst's "systemctl start" blocks (indefinitely, since redis then can't save its RDB on SIGTERM and the unit disables the stop timeout). That is an upstream packaging slip which they are already fixing, so this isn't a permanent change in behaviour from 8.10 - but not starting the service is the right thing for this job regardless. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR is best summarised by docs/Failover.md
Point directly to GitHub releases to save a click.
* Add RESPite.Transports.DuplexTransport + Tunnel.ConnectTransportAsync (SER009, experimental) The [Experimental] transport seam decided with Marc: a duplex byte-transport abstraction that is deliberately NOT a Stream and NOT a pipe, plus a null-default virtual on Tunnel that supplies the ENTIRE transport for a connection -- the same hijack as BeforeAuthenticateAsync one level deeper (no socket is created at all), returning null for every existing tunnel. The shape is derived from measured transport work rather than taste, and each member's derivation is in its doc comment: any-thread copying writes with an explicit Flush (batching at the caller's natural boundaries was the largest single lever measured), PUSH inbound with transport-owned spans (pull adapters over a push transport measured 24-40%), and an OnBatchEnd notification (coalescing burst responses into one flush eliminated a measured 3x send amplification). Abstract classes by design so members can be added with safe defaults while experimental; receive-into-caller-buffer is deliberately deferred to exactly that mechanism. House ceremony: SER009 registered in Experiments, docs/exp/SER009.md, PublicAPI entries in both projects with the [SER009] experimental prefix. A working SocketSet implementation of this shape already exists and is gated across plaintext/TLS/@abstract surfaces (SocketSet repo, src/SocketSet.StackExchange.Redis + bench/tunnel-selftest); it retargets to these types next. * SER009: the transport IS the IBufferWriter -- Output deleted, writer members abstract on the transport With Flush on the transport, a separate .Output object never truly described output; the split was inherited from IDuplexPipe, whose 3-object shape exists for independent Input/Output completion that this contract does not have. GetMemory/GetSpan/Advance are now abstract members of DuplexTransport itself, and passing the transport AS IBufferWriter<byte> grants stage-only access: the holder composes, the owner flushes at its batch boundary. TransportReceiver deliberately stays separate: it is the CONSUMER's half, and it must remain abstract-class-evolvable on the net461/netstandard2.0 targets, where interfaces cannot grow. Experimental (SER009), so this is a free change; PublicAPI entries updated to match. Implementation side re-gated ALL PASS across plaintext/TLS/@abstract before this landed. * SER009: GetSpan is a virtual convenience over GetMemory, not a second abstract GetMemory is the one true abstract of the writer face; GetSpan defaults to GetMemory(sizeHint).Span, so an implementer owes exactly one buffer-acquisition member. Transports with a genuinely cheaper span path (the SocketSet implementation forwards to its connection's own GetSpan) override it -- nice if they do, correct if they don't. * SER009 doc: state plainly that this is not for external use at this time The seam is public only because a seam must be public to be implemented; the doc now says so, spells out that no stability of any kind is implied (members, semantics, name, home, existence -- any version, no ceremony), and frames suppressing the diagnostic as accepting exactly that. If a generally useful transport seam emerges it gets stabilised deliberately, not by this surface quietly hardening.
…the single copy (#3153) The trio existed twice: here (redis-coupled factory) and drifted lib-generic copies on the proxy spike branch. Duplicated code is asking for trouble, so this is the unification, replace/move not duplicate: the SE.Redis files are GONE, the one copy lives in RESPite.Streams (it already leaned on RESPite.Buffers -- the move is with the grain), and the proxy branch deletes its copies on next rebase. What decoupled: the factory loses ConnectionType/ConfigurationOptions and takes (WriteMode, Stream, MemoryPool<byte>?, CancellationToken); WriteMode.Default maps to Async as the lib-safe default. The redis POLICY (pub/sub never wants sync latency mode) moves to the one call site in PhysicalConnection.InitOutput, where it reads as policy instead of hiding in a lib factory. RESPite gains the same conditional System.IO.Pipelines reference SE.Redis already carries (in-box from net10.0) so PipeStreamWriter -- the comparison/troubleshooting implementation -- stays with its family rather than being split back out via cross-assembly inheritance. Everything stays internal; RESPite already grants IVT to StackExchange.Redis and the test projects. Solution builds 0 errors on all TFMs; BufferedStreamWriter + RoundTrip + InProcess/WriteMode test batteries all pass (129 tests).
investigate and fix #3157
* Fix KeyType handling of "none" fix #3156 * fix generator handling of explicit "" * additional tests to pin the new generator "" behavior
* first stab at transaction analyzer * intermediate * server version logic * packaging; verifier arg validation; CI packing test * redundant conditions and compound operations * build cleanup * final category * update transaction docs * upgrading to "warning"; "information" is too invisible * advertise Foo[Async] instead of just Foo; handle ITransactionAsync * Marc/tran analyzer review fixes (#3163) * Identify the transaction terminator by symbol, not by method name ITransaction inherits IDatabaseAsync.ExecuteAsync(string command, params object[] args) - a raw command, very much queued, and the one people reach for exactly when the library has no wrapper for what they want. The switch discarded it by name alongside the transaction's own ExecuteAsync() terminator, so it was invisible to the counts every rule below depends on: _ = tran.StringGetAsync(key); _ = tran.ExecuteAsync("PERSIST", key); // invisible _ = tran.KeyDeleteAsync(key); reported SER303 "use StringGetDelete[Async](key)", and taking that advice drops the PERSIST. The guarded families were wrong the same way: a raw command beside a conditional write still made the transaction a one-command transaction as far as the analyzer could see. Execute/ExecuteAsync now count as the terminator only when declared on ITransaction/ITransactionAsync, which is exactly where the terminator lives (ITransaction.cs) and is not where the raw command lives. A raw ExecuteAsync is now recorded as an ordinary queued operation; it matches nothing in the mapping tables, so its only effect is to suppress, which is the whole point. The third test is the control: both spellings of the terminator must still be recognised, or the rules would go quiet everywhere and the two negatives above would pass for the wrong reason. * Require queued commands to share a branch, and treat lambdas like loops The loop check was the only thing standing between "one call site" and "one queued command", and it only covered the repetition half of that gap. Branching was not covered at all, so mutually exclusive code read as a pair: if (flag) { _ = tran.SetAddAsync(a, member); } else { _ = tran.SetRemoveAsync(b, member); } reported SER303 "use SetMove[Async](source, destination, value)" - which queues a removal the code deliberately did not. The asymmetric form was the same: a StringGet followed by a KeyDelete queued only under an if became GETDEL, and a condition guarding a command in a branch it is not itself in became a conditional argument. The fix is branch *matching* rather than a blanket "anything conditional is out", because two commands in the same if body do always queue together and a compound command really does replace them - and because a whole transaction inside an if or a try is ordinary code that must not go silent. Each call records the innermost enclosing branch (the arm, not the if: the two arms of one if/else have to compare unequal), and a transaction whose calls do not agree is disqualified. The terminator is exempt - "queue it all, then commit inside an if" says nothing about whether the queued commands belong together. Repetition also had a second, larger hole: a lambda or local function is one call site whose invocation count is not visible at all, so _ = tran.KeyDeleteAsync(key); Queue(); Queue(); void Queue() => _ = tran.KeyDeleteAsync(other); reported SER304 over "these 2 queued KeyDeleteAsync calls" when three are queued. Those bodies now disqualify exactly as a loop body does. OperationsInTheSameBranch_AreStillFlagged is the control: without it, matching could be tightened to "never fire under a conditional" and every negative here would still pass. * Decline where the suggested command cannot carry an argument the caller wrote Only arguments 0 and 1 were ever looked at, so everything after them was invisible - and several suggestions cannot express what was there. The rewrites were silently lossy: - N x StringSet(key, value, expiry) -> MSET. MSET takes one expiry for the whole batch, not one per key, so following SER304 here makes both keys permanent. This one outlives the build. - N x HashSet(key, field, value, When.NotExists) -> HSET variadic, which has no NX. - StringGet + KeyExpire(key, expiry, ExpireWhen.HasNoExpiry) -> GETEX, which has no NX/XX. - Condition.KeyNotExists + StringSet(key, value, when: When.Exists) -> "use When.NotExists", turning code that never writes into code that writes when absent. Each mapping now states which parameters its suggestion still carries, and an argument the caller wrote that is not among them declines the rewrite. Per-mapping rather than a blanket "any extra argument is out", because SET *does* take an expiry alongside NX and that is the commonest shape the rule exists for - GuardedOperationWithExpiryAndFlags_IsStillFlagged is the control that keeps it. Family C carries a null coverage set meaning "everything": it keeps the command exactly as written and only deletes the condition, so no argument of it can go missing however exotic. CommandFlags is exempt throughout - it is on every command, no suggestion mentions it, and the help pages already say to carry it over verbatim. Omitted optional arguments do not count either: they carry no intent, and are what the suggested form would default to anyway. Coverage is stated as names the suggestion keeps rather than names it drops so that the fail-safe direction is the default: a parameter added to an overload later reads as uncovered and goes quiet, instead of being silently dropped by a rewrite that has never heard of it. Also fixes a related sharp edge in ArgumentText: an omitted optional argument reports the *invocation* as its syntax, so "the second argument" of tran.KeyDeleteAsync(key) was the text of the whole call. It could only ever collide with another omission from the same call site, but it was never a member the caller wrote, and RequiresMember was asserting nothing as a result. * SER303: StringSet then KeyExpire is SET ... EX Requested as a backlog item during review, and it fits family D exactly: two queued commands, no condition, one command that does both. No version clause. Setting a value and its lifetime in one command is as old as SET''s options (2.6.12), so naming a version would be the noise the other Any entries avoid. An absolute expiry is covered too - Expiration converts implicitly from DateTime as well as TimeSpan - though the EXAT form underneath that one does want 6.2; that caveat goes on the help page rather than into the version clause, because putting 6.2 in the mapping would hide the ordinary relative case from everyone who has declared a floor below it. Order is load-bearing, in the opposite direction to the reads already here: SET *clears* any TTL, so an EXPIRE followed by a SET leaves no expiry at all. Only (StringSet, KeyExpire) maps, and KeyExpireThenStringSet_IsNotFlagged pins that. A StringSet that already carries an expiry, followed by an EXPIRE that overrides it, stays quiet: which of the two lifetimes the single command should carry is a guess. That falls out of the coverage machinery from the previous commit rather than needing its own check - "expiry" is simply absent from the first operation''s coverage set - which is what turns MapPair''s coverage from one set into one per operation. The same split lets GETEX keep rejecting an ExpireWhen while accepting the expiry beside it, where a single shared set had to spell "when" in both senses at once. Not covered, and worth its own item if anyone wants it: Condition.KeyNotExists + StringSet + KeyExpire is StringSet(key, value, expiry, When.NotExists), but that is a condition *and* a pair, which is neither family as they are drawn today. * Do not walk every block twice to answer a question most blocks never ask The reassigned-locals scan ran over every operation block before anything knew whether the block held a transaction at all, so every method body in every project that references the package paid two full Descendants() walks where one would do. The compilation-level short-circuit does not help here: it only skips compilations that have never heard of StackExchange.Redis, which is precisely not the population this analyzer ships to. Moved below the "no transaction locals here" bail-out. Same answer, and the class comment''s claim of one pass is now true for the blocks that take it. Also, while here: parse the declared server version with the invariant culture, since it comes from a config file rather than from someone typing in a locale; and stop re-trimming a method name that QueuedOperation already trimmed, which read as though Map expected raw input. * Document the shapes the analyzer now declines, and the new SET ... EX pair The per-rule "deliberately not flagged" lists were repeating each other and had gone out of date in the same places, so the family-wide cases move to one section in the rules index and each page points at it. Three of them are new behaviour from this branch (a third queued command including a raw ExecuteAsync, commands in different branches, arguments the single command cannot carry) and one - SER300 declining a caller''s own when: - is worth stating next to the expiry case that is still flagged, since "extra arguments suppress" is not the rule and would be a fair thing to conclude otherwise. SER302 gets the opposite note: it keeps the command exactly as written and deletes only the condition, so the argument caveat is the one family-wide case that does not apply to it. SER303 gains the StringSet + KeyExpire row, the second (and opposite) reason order matters on this rule - SET clears the TTL, so only one direction is SET ... EX - and the absolute-expiry caveat that the version column cannot carry. * implement-resp-command: consider whether the new command replaces a transaction A surprising share of new commands are atomic compositions - GETDEL, GETEX, HGETDEL, SMOVE, SET NX/GET/IFEQ, SMISMEMBER, every variadic form - and every one of them exists because people were writing a transaction to get the same effect. TransactionAnalyzer is what tells those people to stop, and a command that is not added to its tables is invisible there: the analyzer stays quiet about exactly the code the command was written to replace. Cheap at the time, and nobody comes back for it later. So the skill now asks the question as a step, and the new section says what a mapping costs: which table by the shape of transaction it replaces, the server version the *suggestion* needs (not the flagged code), the coverage set that stops a rewrite silently dropping an argument, and the same-member / order / key-direction constraints that each table has a column for. Two things it insists on because they are what the review of that analyzer turned up. Coverage is stated as names kept rather than names dropped, so a parameter added to an overload later fails safe. And near-misses get written down where the next person will hit them - LMOVE and LMPOP are both wrong for reasons that are only obvious once you have had them explained. * Say plainly that the usage rules are guidance, and where to report a bad one These five read source text and cannot see keys, servers, or intent, so they are heuristics however carefully drawn - and they arrive as warnings, in a package the consumer did not opt into an analyzer from. Saying "this is a suggestion, not a defect report" costs a paragraph and sets the expectation the severity otherwise sets wrongly. The part that matters is the second half: a rule firing on correct code is a bug in the rule, not something for the consumer to work around, and it reaches everyone. Suppression is documented right below and is easy to reach for silently; the issue link is there so that reporting it is just as easy, and the wording pushes that way first. SER350 deliberately does not get this. It reports an actual build problem - code that was not generated - rather than offering an opinion about working code, and there is nothing heuristic about it. * Hedge the usage diagnostics: they are suggestions, and they are heuristics "can be replaced", "are one command", "is redundant" are findings of fact, and these rules are not in a position to state one - they read source text and cannot see keys, servers, or intent. Combined with a warning severity, which already overstates the case, the wording claimed more certainty than the analysis has. Titles now say "may be replaceable" / "may be redundant" / "may suit", and messages lead with "Consider" or "looks like ... - consider". Message arguments and their order are untouched, so this is wording only; the tests format through the same descriptors and are unaffected. SER350 deliberately keeps its plain phrasing. It reports that generated code was not emitted, which is a fact about the build rather than an opinion about working code. The help pages, the index list and the release-tracking notes move with the titles, since a message that hedges next to a doc heading that does not is worse than either alone. * Recognise CommandFlags by name as well as by type, and gate it with tests that fail without it Flags never bear on any of the argument audit: they are on every command, no suggestion mentions them, and the rewrite carries them over verbatim. The exclusion was by type alone, which is the check that can come back null - and the failure mode is not a missed exclusion but silence everywhere, because every command takes flags, so one unrecognised spelling would suppress every rule for anyone who passes them. Name and type both, which costs a string comparison on a path that only runs for calls on a transaction. The behaviour was technically already covered, but only by GuardedOperationWithExpiryAndFlags, which carries an expiry alongside the flags and is really about the expiry - and only for SER300, where the audit runs in three separate places. Three tests now pass flags as the *only* extra argument, one per family. Checked rather than assumed: with the exclusion disabled, exactly those three and the expiry one fail, and nothing else does. A test that would pass either way is not a gate.
…ries (#3161) * Sub-command retry categorization, tranche 1 (#3148) Categorize commands whose side-effect profile depends on their arguments rather than just the command name, via the (previously unused) WithCategory hook at the message factories. Fixes an oversight in WithCategory itself: CommandServerSpecific is orthogonal to the severity ladder, but was being masked off - and since setting a ladder bit suppresses WithDefaultCategory, a conditional category would also have dropped the server-specific bit the default would have supplied. Corrects three classifications that were unsafe (retried by the default policy when they should not be): - SORT ... STORE was read-only; it writes the destination key - GETEX/HGETEX with EX/PX/EXAT/PXAT/PERSIST were read-only; they mutate TTL - SortedSetIncrement(When.Exists) emits ZADD ... XX INCR but inherited ZADD's last-wins, so a replay could double-increment Demotions: XREADGROUP with an explicit id (re-reads own PEL) never -> read-only; GeoRadius[ByMember] -> read-only (the typed API never emits STORE); SCAN from cursor 0 is no longer node-affine. Promotions to checked: SET with NX/XX or any ValueCondition (IFEQ/IFNE/ IFDEQ/IFDNE); ZADD with NX/XX/GT/LT; ZADD ... NX INCR; EXPIRE/HEXPIRE with NX/XX/GT/LT; XADD with an explicit id or IDMP/IDMPAUTO; XCLAIM/XAUTOCLAIM with JUSTID. COPY ... REPLACE -> last-wins. Also implements CONFIG GET -> CommandRetryConnection, which the docs already described but the code did not do. * Sub-command retry categorization, tranche 2: server commands (#3148) CLIENT, CLUSTER, CONFIG, SCRIPT, SLOWLOG, LATENCY, MEMORY and SENTINEL are each a single RedisCommand spanning very different verbs, so the whole-command default has to assume the most side-effecting one. The IServer methods know which subcommand they issue, so categorize accordingly. Two needed *raising*, because their parent command's default was the permissive end: - MEMORY PURGE: MEMORY defaults to read-only, so a purge was being treated as a harmless read - CLIENT KILL: CLIENT defaults to connection-level Demotions, all also flagged node-affine since the answer (or effect) belongs to the server that was asked: CLUSTER NODES, SCRIPT EXISTS, SCRIPT LOAD (idempotent - same SHA - and on the normal EVALSHA path), SLOWLOG GET, LATENCY DOCTOR/HISTORY/LATEST, MEMORY DOCTOR/STATS/ MALLOC-STATS, and the SENTINEL introspection verbs (MASTER, MASTERS, REPLICAS/SLAVES, SENTINELS, GET-MASTER-ADDR-BY-NAME). SENTINEL FAILOVER, CONFIG SET/REWRITE/RESETSTAT, SCRIPT FLUSH, SLOWLOG RESET and LATENCY RESET stay at the server-admin default. Also de-duplicates a few sync/async message-construction pairs behind shared GetXxxMessage factories, matching the convention used elsewhere, and refreshes the now-stale "ignoring X" / "can be considered more safe" comments in the per-command default table to point at where each case is actually handled. * be less verbose in the docs update * words * clarify transaction rules in failover.md * Marc/sub command category fixes (#3164) * Address cold-eyes review findings 1-4 on #3161 Two of these are correctness fixes to the new arg-specific rules; one is a consistency gap; the last is the coverage that would have caught the first two. 1. XADD <ms>-* was categorized as an explicit id, and so retried by default. GetStreamAddCategory tested the id against the bare "*". Redis 7.0+ also accepts <ms>-*, where the server picks the *sequence* - so a replay appends 5-1 after 5-0 rather than being rejected as "equal or smaller". messageId is an unvalidated RedisValue on the public StreamAdd overloads, so this is reachable, and WriteChecked (12) is <= the default MaxCommandRetryCategory (WriteLastWins, 16): it *is* retried. That made it a regression against the coarse rule, where XADD's whole-command WriteAccumulating (20) blocked it. Now keyed on "does the id end in *", which is exactly Redis's spelling for a server-assigned part, and covers both "*" and "<ms>-*". An id we cannot read (empty, or implausibly long) is treated as caller-specified: the server then rejects it deterministically on every attempt, so a replay is harmless. 2. The XREADGROUP demotion ignored CLAIM. The category keyed purely on the position, but WriteImpl emits CLAIM <ms> whenever claimMinIdleTime is set, independent of the position - and the public API lets you pass both. So StreamReadGroup(position: "0-0", claimMinIdleTime: 30s) was categorized CommandRetryReadOnly while the command may take entries from *other* consumers' pending lists and bump their delivery counts. That is the same side-effect that keeps XCLAIM off the read rung in WithJustIdCategory. CLAIM now suppresses the demotion outright, for both the single- and multi-stream forms. 3. HashFieldExpire did not get the ExpireWhen rule that KeyExpire does. Safe direction - it stayed at the WriteLastWins default - but it meant the NX/XX/GT/LT reasoning applied to keys and not to hash fields. The message build is split out of HashFieldExpireExecute into GetHashFieldExpireMessage so the rule sits with the other factories, and so it is testable. 4. Coverage for the rules that had none. The new rules were about half tested, and the untested half was disproportionately *demotions* - the direction where a mistake loosens safety rather than tightening it. Added: GETEX/HGETEX TTL variants, COPY REPLACE, EXPIRE/EXPIREAT and HEXPIRE conditions, XCLAIM/XAUTOCLAIM JUSTID, XREADGROUP positions, GEORADIUS, SCRIPT LOAD. Verified the three fixes are load-bearing by reverting each in turn against the new tests: exactly one test fails per fix (Accumulating->Checked for 1, Never->ReadOnly for 2, Checked->LastWins for 3), and the 12 pure-coverage assertions stay green either way. A test that passes against the unfixed code would not have distinguished these from deliberate choices. Not addressed here, both judgement calls rather than defects: SCRIPT LOAD is marked CommandServerSpecific, which blocks the failover retry that is arguably the most useful one for it; and CLUSTER NODES is spelled out by hand in ServerEndPoint and ConnectionMultiplexer rather than going through the new RedisServer.GetClusterNodesMessage. Test run: 15/15 in CommandRetryCategoryUnitTests. The wider server-free unit suites are 1746 passed / 5 failed, and those 5 fail identically on the unmodified PR head (they need a live Redis on 6379). * Loosen SCRIPT LOAD, and give CLUSTER NODES one spelling (review findings 5-6) Both were flagged as judgement calls rather than defects in the review of #3161; this takes the calls. SCRIPT LOAD is no longer node-scoped. Everything else under SCRIPT is server-admin and node-scoped, and LOAD had inherited the second half of that. It should not: the SHA is a pure function of the script, so a replay on a *different* node returns the same answer, and nothing about it touches the keyspace, so there is no data outcome to get wrong. The bookkeeping already follows the wire rather than our intent - ResultProcessor.ScriptLoad records the hash via connection.BridgeCouldBeNull.ServerEndPoint.AddScript, i.e. against whichever endpoint actually replied, not the one we aimed at. So a load that lands elsewhere records correctly rather than corrupting a cache. Worst case the node we *meant* to warm is still cold, and a later EVALSHA there gets NOSCRIPT and falls back to EVAL - an already-handled path (IsScriptUnavailable). Being straight about the scope: this is stated intent rather than a live behaviour change. The only caller that could be retried across endpoints is IServer.ScriptLoad, and WithRetry wraps IDatabaseAsync only; the internal load-then-EVALSHA pairing in ScriptEvalMessage.GetMessages is written to one connection as a unit and so is never independently re-routed. It matters if either of those ever changes, and it costs nothing to be right now. The test asserts the *absence* of the node-scoped bit against the SCRIPT whole-command default as a control, because the category alone is identical either way - checking only the category would not have seen this change at all. CLUSTER NODES now has one spelling. The PR added RedisServer.GetClusterNodesMessage but left ServerEndPoint .AutoConfigureAsync and ConnectionMultiplexer.GetEndpointsFromClusterNodes building the same message by hand - and in two different styles, one applying WithCategory and one passing the literal flags straight through. Three copies of one decision is how the three drift apart. Both now route through the factory, and the justification comment moves onto it. Behaviour-preserving by construction: NodeLocalRead is exactly the CommandRetryReadOnly | CommandServerSpecific pair both sites spelled out, and the multiplexer's site passed CommandFlags.None, so WithCategory yields the same flags the literal did. No new test - the factory's output is already pinned by ServerSubCommands_AreCategorizedBySubCommand, and the change is a reduction in call sites rather than in behaviour. Test run: 15/15 in CommandRetryCategoryUnitTests; 1728/1728 across the server-free unit suites (the 5 that need a live Redis on 6379 are excluded here rather than failing). All target frameworks build with 0 warnings. * RedisValue.StartsWith: stop measuring a UTF-16 string against a UTF-8 prefix Two bugs, both from treating char count and byte count as the same number. "Not enough characters to match" gave up whenever the string had fewer chars than the prefix had bytes, which is wrong the moment anything is not ASCII: "e-acute, euro" is 2 chars but 5 UTF-8 bytes, so every prefix of 3 bytes or more was rejected out of hand. A shorter string is not grounds to give up at all - 2 chars can be 8 bytes - so the early-out goes rather than being adjusted. The cut that follows was right in principle: each char is at least one UTF-8 byte, so a prefix of N bytes never needs more than N chars. But cutting at exactly N chars can land between the halves of a surrogate pair, and the encoder then emits U+FFFD (EF BF BD) rather than the character being asked about - so "a" + U+1F600 did not start with 61 F0, which it plainly does. The cut now takes the low half with it. Both were false negatives only, and only for non-ASCII, which is presumably why they lasted: the ASCII path that everything else exercises has char count and byte count agreeing by definition. Tests first, and both failed at the predicted assertion before the fix. * RedisValue.EndsWithAscii, and use it for the stream server-assigned-id test IsServerAssignedId wanted one byte and paid for the whole value to get it: Length() on a string-backed id is a full UTF-8 GetByteCount scan, and then the id was encoded again into a 64-byte stack buffer so that the last byte could be read. A literal "*" or "5-*" is the overwhelmingly common case, and it is exactly the case that path handles worst. EndsWithAscii asks each storage kind directly, in the shape StartsWith(ReadOnlySpan<byte>) already uses: - blob kinds (short blob, byte[], memory manager): index the last byte of the raw span - sequence: TryGetLast walks to the final segment, skipping empties the way the rest of ReadOnlySequenceExtensions does; still no copy - string: compare the last *char*. ASCII-only is what makes this valid rather than merely fast - UTF-8 never uses a byte below 0x80 as a continuation byte, so "the encoding ends with this byte" is exactly "the last char is this char". No encode, no length, and correct for non-ASCII rather than undefined. - integers: the final digit is value % 10. The sign is a non-issue if you negate the *remainder* rather than the value - that is safe even for long.MinValue, where negating the value overflows. - double: format and look. Nobody should be asking a double this, and going through Format is what keeps the answer agreeing with CopyTo and so with the wire, +inf/-inf included. The call site loses MaxStreamIdBytes entirely: the 64-byte cap only ever existed to bound the stackalloc. That is a deliberate behaviour change at the edges - a >64-byte value ending in "*" was classified caller-specified and is now server-assigned - but every such value is an invalid id that the server rejects deterministically on each attempt, so which retry category it carries cannot be observed. Tested per storage kind, each asserted against the kind it actually landed in, because a different route through the switch is the entire point; plus a value run through every kind that can hold it, since the answer must not depend on how it happens to be stored. MemoryManager is fabricated via CreateForeign - no ordinary conversion produces one, and "awkward to construct" is not a reason for a kind to go untested. * EndsWithAscii: name the UnsafeRawSpan scratch local On the TFMs without MemoryMarshal.CreateReadOnlySpan, a short blob''s span is built over a raw pointer to the out parameter''s slot, so that slot has to stay put for as long as the span is read through. StartsWith gets away with "out _" because it consumes the span in the same expression; this reads it across two statements, so it names the local, as BlobSequenceEqual already does and says why. No behaviour change on net10.0. Verified on both test TFMs - net481 is the one that actually takes the pointer path.
…md '@' convention) (#3165) '!name' has always meant a Unix domain socket, but '!@foo' silently produced a PATHNAME socket literally named '@foo' -- the wrong socket, found by nothing. The parse now maps '@' after '!' to the kernel's leading-NUL spelling, Linux-gated (no other platform has the namespace; elsewhere '@' stays a literal filename, matching redis-cli). ToString round-trips for free: UnixDomainSocketEndPoint renders abstract names back as '@name'. Tests: FormatTests gains UDS parse/round-trip coverage (there was NONE, even for pathnames) -- pathname and abstract cells, the latter Linux-gated. Verified live end-to-end besides: a ConnectionMultiplexer built from ConfigurationOptions.Parse("!@se-abs-test,abortConnect=true") against a Garnet listening on the same abstract name, SET/GET round-trip clean, no filesystem footprint.
…queue more than once" (#3166) TryGetBranch lumped IAnonymousFunctionOperation and ILocalFunctionOperation in with ILoopOperation and returned false, which the caller reads as "this call can queue N times" and uses to disqualify the whole transaction. Roslyn hands out no separate operation block for a lambda or local function - the body arrives as part of the containing method - so every transaction written inside one was silently invisible. That includes top-level statements, where the entire program body is one synthesised method, so the analyzer said nothing at all about the commonest way a small repro gets written. The repeat risk belongs to the *captured* transaction, not to the boundary itself. A transaction that is a local of the function being walked out of is created afresh on each invocation, so one invocation holds one whole transaction and the counts within it are exact; whatever encloses the function governs how many transactions there are, not what goes into each. The two boundary cases now stop the walk and accept when the transaction local belongs to that function, and keep returning false when it was captured from outside. A loop inside such a function is still hit first, as it must be. Tests: the two existing negatives were already the captured shape, so they are unchanged and still pass. Four added - transaction declared in a local function (the reported shape), in a lambda, in a local function called in a loop (N transactions, not one with N commands), and a loop inside such a function (still suppressed). docs/rules/index.md stated the old blanket behaviour as an intentional limitation; corrected. No AnalyzerReleases change: detection coverage only, no new or altered diagnostic IDs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.