always increment finishedQueries counter if incrementing allQueries

[jkt-signal]

Storage server processes publish a pair of counters total_queries and finished_queries (as well as a derived special counter query_queue_max) tracking how many read operations have been started and finished (and the high-water-mark for the number of pending queries—those that have been started, but not finished—in a status-fetch interval).

Unfortunately, the counter underlying total_queries was incremented at the beginning of one of several actors, and the matching increment to finished_queries was done at the end, with several intervening waits. If the actor was cancelled (or experienced another non-returnable error), finished_queries was not incremented, resulting in a permanent drift between total_queries and all_queries and apparently endless growth in query_queue_max.

The easiest way to demonstrate this behavior is to have two different clients alternate between setting watches and updating values for the same keys—for example, run two simultaneous instances of this simple Java program against a single database, and monitor its status to see query_queue_max on storage servers go up permanently every time you run the test.

To solve this problem, we add a simple class CountedSection to flow/Stats.h to bump two counters in RAII-enforced balance; once a local CountedSection has bumped the first counter, it will always eventually bump the second one whether it is destroyed as a result of normal exit or exception. (Of course, if you store one in a heap object and leak it, you will still experience drift, so, uh, don't do that.) CountedSection is made move-only so you can use it in actor state, since state variables seem to always be default-constructed and then assigned to their initializers even if the initialization comes before the first wait in an actor.

One of the actors involved actually updates a second counter pair, getMappedRangeQueries and finishedGetMappedRangeQueries, in a way vulnerable to the same problem, so we use CountedSection to address that as well, though I have not seen any instances of these getting out of sync (entirely due to lack of trying).

Some other metrics updated at the ends of the affected actors—mostly latency metrics, but also some tagged size distributions—are not addressed by this change. Arguably they should not be, since including latency and response size of interrupted queries may significantly throw off the statistics in a not-necessarily-useful way.

Leaving this as an RFC because I'm not sufficiently familiar with the FoundationDB testing setup to write a good test; the Java program linked above is a great demonstration but is not trivially adapted to an integration test (and presumably we'd like tests of the fdbserver itself to be C++, not dependent on the Java bindings). Advice for how to test the change greatly appreciated!

This PR is against the 7.3 branch, rather than main, because all the relevant actors have been rewritten to use C++20 standard coroutines in the interim, and while I strongly suspect the same bug applies equally in main, I was not able to confirm this because main does not currently build a usable fdb for me. Porting forward to main should be relatively trivial and I'm happy to do so if requested.

[jkt-signal]

(Unfortunately, I don't appear to have the rights to apply the RFC label as suggested by the commit process document, so I'm marking this PR ready for review instead.)

[gxglass]

Local AI review:

PR 13786 — always increment finishedQueries counter if incrementing allQueries

Intent

The storage-server query actors (getValueQ, getKeyValuesQ, getMappedKeyValuesQ, getKeyValuesStreamQ, getKeyQ) increment counters.allQueries at entry and counters.finishedQueries at exit as separate statements. If the actor throws/returns/cancels between them, finishedQueries is skipped, so the in-flight gauge (allQueries - finishedQueries, e.g. maxQueryQueue) drifts upward permanently. This PR adds an RAII CountedSection (++start in ctor, ++end in dtor) held as a state variable, so finishedQueries is incremented on every exit path. The move-assign-swap idiom neatly matches Flow's default-construct-then-assign state initialization without double-counting.

Correctness

• The counter-balancing intent is sound and the CountedSection design (default ctor + move-assign swap + no move ctor) is correctly tailored to Flow's state-variable init, so allQueries/finishedQueries stay paired on normal completion.
• But it introduces a use-after-free on the SS-teardown-with-in-flight-query path (see Bugs). The old code only touched counters from the actor body (normal completion, while the SS is alive); moving the finishedQueries increment into a destructor makes it also fire on cancellation, including the cancellation that happens while the owning StorageServer is being destroyed.

Bugs

• Use-after-free of a destructed Counter during StorageServer teardown — fdbrpc/include/fdbrpc/Stats.h (CountedSection::~CountedSection) + fdbserver/storageserver.actor.cpp (data->actors.add(getValueQ(...))).
  • Confirmed member ordering on release-7.3 (via GitHub API): ACTOR/ActorCollection actors; is declared at line 1313, the } counters; member at line 1617. Members destruct in reverse declaration order, so in ~StorageServer counters (and its Counter finishedQueries, whose implicit dtor destructs the Int64MetricHandle metric member) is destroyed first, and the actors collection is destroyed last.
  • The query actors are owned by data->actors (e.g. data->actors.add(getValueQ(data, req)), 7.3 line 4108). When actors destructs it cancels any still-in-flight query actor, which destroys its state CountedSection cs, whose destructor runs ++data->counters.finishedQueries → Counter::operator+= → metric += delta, dereferencing the already-destructed Int64MetricHandle. Counter has no trivial destructor (it owns Int64MetricHandle), so this is genuine use-after-destruction, crash-capable in an ASAN build and UB otherwise.
  • Failure scenario: a storage server is removed/excluded/killed (or its storageServer actor is otherwise cancelled) while a getValueQ/getKeyValuesQ/… is in flight — common in simulation (SS attrition with concurrent reads). ~StorageServer destroys counters, then actors cancels the pending query, whose CountedSection destructor increments the freed finishedQueries.
  • This is a regression introduced by the PR: the pre-change code incremented finishedQueries only in the actor body, which never runs on the teardown-cancellation path, so it never touched counters after destruction.

Omissions

• No test exercises the SS-teardown-with-in-flight-query cancellation path (which is exactly where the RAII change diverges from the old behavior).

Alternatives / Suggested fix

• Ensure counters outlives any actor whose destructor may touch it: declare the counters member before actors in StorageServer (so actors destructs first, cancelling the query actors while counters is still alive). Alternatively, drain/cancel data->actors explicitly before counters is torn down, or have CountedSection capture something that guarantees the counter's lifetime. Note main has the same actors-before-counters ordering (actors ~L1185, counters member ~L1410), so any future forward-port of this pattern needs the same reordering to be safe.

Verdict

NEEDS-CHANGES — The RAII counter-balancing is the right idea, but as written it moves the finishedQueries increment into a destructor that runs on query-actor cancellation, and on release-7.3 (confirmed member order: actors before counters) that cancellation happens during ~StorageServer after counters is already destroyed — a use-after-free of the Counter's Int64MetricHandle when a storage server is torn down with an in-flight read. Fix by making counters outlive actors (member reorder) or otherwise guaranteeing the counter's lifetime across the destructor increment.

[jkt-signal]

Yep, I think the bug noted in the review is real.

I don't see any way in which methods of any member of counters can reference any member of actors, so I think it's safe to move counters before actors in StorageServer to solve this.

[jkt-signal]

Yep, I think the bug noted in the review is real.

I don't see any way in which methods of any member of counters can reference any member of actors, so I think it's safe to move counters before actors in StorageServer to solve this.

and done.

[foundationdb-ci]

Result of foundationdb-pr-73 on Linux CentOS 7

• Commit ID: aead434
• Duration 1:11:15
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang-73 on Linux CentOS 7

• Commit ID: aead434
• Duration 1:19:39
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-clang on Linux RHEL 9

• Commit ID: aead434
• Duration 1:23:00
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr on Linux RHEL 9

• Commit ID: aead434
• Duration 1:25:35
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-cluster-tests-73 on Linux CentOS 7

• Commit ID: aead434
• Duration 1:39:36
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)
• Cluster Test Logs zip file of the test logs (available for 30 days)

[gxglass]

AI review attached below.

A comment mentions that main doesn't generate a usable fdbserver. Can you describe that in more detail? In general we rely on main not to broken for day to day development.

review-13786.md

[foundationdb-ci]

Result of foundationdb-pr-macos on macOS 14.x

• Commit ID: aead434
• Duration 3:39:55
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[foundationdb-ci]

Result of foundationdb-pr-macos-m1 on macOS 14.x

• Commit ID: aead434
• Duration 4:01:44
• Result: ✅ SUCCEEDED
• Error: N/A
• Build Log terminal output (available for 30 days)
• Build Workspace zip file of the working directory (available for 30 days)

[jkt-signal]

AI review attached below.

A comment mentions that main doesn't generate a usable fdbserver. Can you describe that in more detail? In general we rely on main not to broken for day to day development.

I'll build and try again now to confirm, but last time I tried, fdbcli status against a newly started container (itself built by substituting the fdbserver binary in the published foundationdb/foundationdb:7.3.68 container with a freshly-built fdbserver from main) hung indefinitely and I was not able to create a database. It's entirely possible that I need to be building the container differently, or was using the wrong client, or something.

review-13786.md

This link 404s for me; perhaps it is not public?

[jkt-signal]

review-13786.md

This link 404s for me; perhaps it is not public?

The link works now 🤷 Yep, good catch, will fix the missed ++data->counters.finishedQueries.

I don't really agree that std::exchange(other.end, nullptr) would be better; in my opinion the invariant of a CountedSection should hold even if you assign over the top of it (that is, if you do { CountedSection cs(a, b); cs = CountedSection(c, d); } then all of a, b, c, and d should get incremented). However, this is largely academic because I think it's highly unlikely that anyone will ever move into a non-default-constructed CountedSection. I also can't replace the Counter *s with Counter &s because we need the struct to be default-constructible.

I will supply a couple unit tests for CountedSection. I'm also happy to write more useful tests for the counters but I need some pointers on how to build such a test (even a link to a vaguely-related test would be good; I'm struggling to find anything relevant.)

[gxglass]

review-13786.md

This link 404s for me; perhaps it is not public?

The link works now 🤷 Yep, good catch, will fix the missed ++data->counters.finishedQueries.

I don't really agree that std::exchange(other.end, nullptr) would be better; in my opinion the invariant of a CountedSection should hold even if you assign over the top of it (that is, if you do { CountedSection cs(a, b); cs = CountedSection(c, d); } then all of a, b, c, and d should get incremented). However, this is largely academic because I think it's highly unlikely that anyone will ever move into a non-default-constructed CountedSection. I also can't replace the Counter *s with Counter &s because we need the struct to be default-constructible.

I will supply a couple unit tests for CountedSection. I'm also happy to write more useful tests for the counters but I need some pointers on how to build such a test (even a link to a vaguely-related test would be good; I'm struggling to find anything relevant.)

On the tests the agent likes to just say "no new tests" if it doesn't see any. If there is no prior art in the area and it is not obvious how to write a test case for the code then I do not think that nit would block LGTM. (This is a purely generic answer because agent reviews very often nit about "no new tests" - I haven't looked at specifics in this area - use your judgement.)

Based on "will supply a couple unit tests" I'm anticipating 1+ more commits. Can you tag me for re-review on this PR when it's all ready.