[NBS] TStatsServiceActor fails to restore ServiceVolumeCounters after temporary subgroup removal

[nisqatsi]

Summary

TStatsServiceActor removes per-volume metrics from the TDynamicCounters tree when a volume has neither clients nor checkpoints, but leaves volume.ServiceVolumeCounters non-null.

If the same volume later gets a client or checkpoint, RegisterServiceVolumeCounters() sees the non-null pointer and returns without attaching the subgroup back to the monitoring tree. PerfCounters.Publish() then continues updating detached counters that are no longer reachable from the monitoring root.

As a result, metrics with component=service_volume, host=cluster may disappear after a temporary period without clients and never return until the corresponding TVolumeStatsInfo is destroyed or the process/actor is restarted.

Affected code

cloud/blockstore/libs/storage/stats_service/stats_service_actor_solomon.cpp:

void TStatsServiceActor::RegisterServiceVolumeCounters(
    const NMonitoring::TDynamicCounterPtr& counters,
    TVolumeStatsInfo& volume)
{
    // ...
    if (volume.ServiceVolumeCounters) {
        return;
    }

    auto head =
        counters->GetSubgroup("counters", "blockstore")
            ->GetSubgroup("component", "service_volume")
            ->GetSubgroup("host", "cluster");

    volume.ServiceVolumeCounters =
        RegisterChain(head, BuildVolumeChain(volume.VolumeInfo));

    volume.PerfCounters.Register(volume.ServiceVolumeCounters);
    // NUserCounter::RegisterServiceVolume(...)
}

void TStatsServiceActor::UnregisterServiceVolumeCounters(
    const NMonitoring::TDynamicCounterPtr& counters,
    TVolumeStatsInfo& volume)
{
    // ...
    auto head =
        counters->GetSubgroup("counters", "blockstore")
            ->GetSubgroup("component", "service_volume")
            ->GetSubgroup("host", "cluster");

    head->RemoveSubgroupChain(BuildVolumeChain(volume.VolumeInfo));

    NUserCounter::UnregisterServiceVolume(/* ... */);

    // volume.ServiceVolumeCounters remains non-null.
}

The publication condition in UpdateVolumeSelfCounters() is:

const bool shouldPublishServiceVolumeCounters =
    vol.PerfCounters.HasCheckpoint || vol.PerfCounters.HasClients;

if (shouldPublishServiceVolumeCounters) {
    RegisterServiceVolumeCounters(AppData(ctx)->Counters, vol);
    vol.PerfCounters.Publish(ctx.Now());
} else {
    UnregisterServiceVolumeCounters(AppData(ctx)->Counters, vol);
}

Steps to reproduce

1. Register a volume.

2. Submit statistics with HasClients = true.

3. Run a counters update.

4. Verify that the following branch exists:

   counters=blockstore
     / component=service_volume
     / host=cluster
     / volume=<disk-id>
     / cloud=<cloud-id>
     / folder=<folder-id>
   

5. Submit statistics with HasClients = false and HasCheckpoint = false.

6. Run a counters update. The volume branch is removed from the tree.

7. Submit statistics with HasClients = true again.

8. Run another counters update.

9. Check for the branch using FindSubgroup().

The metrics are expected to exist again at step 9. In practice, the branch is still missing.

Regression-test pseudocode for stats_service_ut.cpp:

RegisterVolume(runtime, DefaultDiskId);

BroadcastVolumeCounters(runtime, {0}, VOLUME_HASCLIENTS);
UNIT_ASSERT(VolumeMetricsExists(*runtime.GetAppData(0).Counters));

BroadcastVolumeCounters(runtime, {0}, 0);
UNIT_ASSERT(!VolumeMetricsExists(*runtime.GetAppData(0).Counters));

BroadcastVolumeCounters(runtime, {0}, VOLUME_HASCLIENTS);
UNIT_ASSERT(VolumeMetricsExists(*runtime.GetAppData(0).Counters)); // fails

The check must use FindSubgroup() rather than GetSubgroup(): GetSubgroup() creates a missing branch and may hide the bug.

Actual behavior

After the following transition:

HasClients/HasCheckpoint: true -> false -> true

the lifecycle is:

1. On true, a subgroup is created and assigned to volume.ServiceVolumeCounters.
2. On false, RemoveSubgroupChain() removes the subgroup from its parent TDynamicCounters map.
3. The subgroup object itself remains alive because it is still referenced by:
   • volume.ServiceVolumeCounters;
   • counters registered inside volume.PerfCounters;
   • VolumeBindingCounter and other nested counter holders.
4. On the next true, RegisterServiceVolumeCounters() returns early because volume.ServiceVolumeCounters != nullptr.
5. PerfCounters.Publish() writes values to an object that is alive but detached from the monitoring root.
6. The metrics remain absent from /counters, Solomon, and any other consumer that traverses the tree from its root.

The current implementation therefore incorrectly uses a non-null ServiceVolumeCounters pointer as both:

• an owning reference to the subgroup;
• an indication that the subgroup is attached to the monitoring tree.

These two states diverge after RemoveSubgroupChain().

Additional scenario: cloud/folder update

The same problem occurs in HandleVolumeConfigUpdated():

1. When CloudId or FolderId changes, UnregisterServiceVolumeCounters() is called for the old path.
2. volume.VolumeInfo is replaced with the new configuration.
3. RegisterServiceVolumeCounters() is called for the new path.
4. Registration is skipped because the old volume.ServiceVolumeCounters pointer is still non-null.

Consequently:

• the old branch is removed;
• the new branch is not created;
• NUserCounter::UnregisterServiceVolume() has already been called for the old labels;
• NUserCounter::RegisterServiceVolume() is not called for the new labels because of the early return.

Expected behavior

• Whenever HasClients || HasCheckpoint == true, the volume subgroup must be attached to AppData(ctx)->Counters at the current host=cluster/volume/cloud/folder path.
• Metrics must resume publication after a true -> false -> true transition.
• After a CloudId or FolderId update, the old branch must disappear and a new branch with the current labels must be created.
• User-counter registration must remain consistent with service-volume-counter registration.
• Repeated register/unregister operations must be idempotent.

Root cause

TDynamicCounters::RemoveSubgroupChain() removes references to a subgroup from the parent tree but does not invalidate external intrusive pointers. This is valid container behavior, but the current ServiceVolumeCounters lifecycle does not account for it.

UnregisterServiceVolumeCounters() does not change any state that would allow a subsequent registration attempt to determine that the retained subgroup is no longer attached to the monitoring root.

Impact

• Loss of per-volume performance metrics after an unmount/remount cycle that does not destroy TVolumeStatsInfo.
• Loss of metrics after a volume's cloud or folder changes.
• PerfCounters.Publish() continues doing work but updates invisible counters.
• User counters may remain unregistered after clients or a checkpoint reappear.
• The defect is easy to miss in tests that use GetSubgroup(), because that method creates missing groups.

Important note for fix

Simply adding the following assignment is insufficient for expiring scalar counters that have already been materialized:

volume.ServiceVolumeCounters = nullptr;

TDiskPerfData::Register() registers DiskCounters and VolumeSelfCounters against the new subgroup. Permanent TSolomonValueHolder instances are rebound because Init() replaces their SolomonValue. For an expiring holder, however, Init() only updates Parent, CounterName, and Derivative; it does not clear an existing SolomonValue.

For example, IORequestsQueued is a service_volume counter with ECounterExpirationPolicy::Expiring. If it was materialized before the old subgroup was detached, re-registering TDiskPerfData leaves its SolomonValue pointing to the detached counter. Subsequent publications continue updating that invisible counter instead of creating one under the new subgroup. If the value remains positive, LastNonZeroValueTs is refreshed on every publication, so the stale pointer may be retained indefinitely.

The same applies to other expiring service_volume counters, including UsedBlocksMapMemSize, LongRunningReadBlob, and LongRunningWriteBlob.

By contrast, the histogram and request-counter registration paths replace their stored pointers during Register()/ForceRegister(). They should still be covered by regression tests, but the stale-pointer behavior is specifically confirmed for expiring TSolomonValueHolder counters.

[nisqatsi]

Possible fixes

Option 1: track attachment state separately and reattach the existing subgroup

Recommended for consideration:

• keep ServiceVolumeCounters as the owner of the created subgroup;
• add a separate state flag, for example ServiceVolumeCountersRegistered;
• on unregister, detach the subgroup from the tree and clear only the registration flag;
• on register, insert the retained subgroup back into the current host=cluster/volume/cloud/folder branch;
• call NUserCounter::RegisterServiceVolume() again as part of the same transition.

This preserves all pointers held by TDiskPerfData; they continue referring to the same subgroup and do not require bulk rebinding.

The expected semantics of cumulative counters after reattachment must be defined explicitly: either preserve accumulated values or reset them at the beginning of a new publication period.

Option 2: reset the complete registration and create it again

Another approach is to clear volume.ServiceVolumeCounters on unregister and call PerfCounters.Register() again during the next registration.

From issue note - simply adding the following assignment is insufficient for expiring scalar counters that have already been materialized:

volume.ServiceVolumeCounters = nullptr;

Required tests

1. ShouldReregisterServiceVolumeCountersAfterClientsReturn

   • transition HasClients: true -> false -> true;
   • verify that the branch disappears on false and reappears on the next true;
   • verify that published values are accessible from the reattached tree.

2. ShouldReregisterServiceVolumeCountersAfterCheckpointAppears

   • transition HasCheckpoint: false -> true with no clients.

3. ShouldMoveServiceVolumeCountersAfterVolumeConfigUpdate

   • change CloudId and/or FolderId;
   • verify that the old path is absent;
   • verify that the new path exists;
   • verify that values are published under the new labels.

4. ShouldReregisterServiceVolumeUserCounters

   • verify that NUserCounter::UnregisterServiceVolume() is paired with a subsequent RegisterServiceVolume().

5. Verify multiple cycles:

   true -> false -> true -> false -> true
   

   No branches should be missing or duplicated.

6. Verify both expiring and permanent counters after re-registration.

Acceptance criteria

• All regression tests listed above pass.
• After a client or checkpoint reappears, metrics are reachable by traversing from the monitoring root.
• After a label update, metrics exist only under the current path.
• PerfCounters.Publish() does not write exclusively to a detached subtree.
• User counters are re-registered together with service volume counters.
• The solution does not create duplicate subgroups and preserves consistent cumulative/derivative-counter semantics.