fix(virtual-tags): resolve High code-review finding (Core.VirtualTags-001)
OnScriptSetVirtualTag updated the value cache, notified observers, and recorded history for the written path but never scheduled a cascade for tags depending on that path. This contradicts docs/VirtualTags.md, which states ctx.SetVirtualTag writes "still participate in change-trigger cascades": a change-triggered virtual tag reading a script-written tag went stale until an unrelated trigger fired. OnScriptSetVirtualTag now launches a fire-and-forget CascadeAsync for the written path, mirroring OnUpstreamChange. The cascade is scheduled rather than invoked inline because the callback runs inside EvaluateInternalAsync while the non-reentrant _evalGate semaphore is held. Added regression test SetVirtualTag_within_script_cascades_to_dependents_of_the_written_tag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 13 |
|
||||
| Open findings | 12 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank.
|
||||
| Severity | High |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:306` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `OnScriptSetVirtualTag` updates `_valueCache`, notifies observers, and
|
||||
records history for the written path, but it does not schedule a cascade for tags that
|
||||
@@ -58,7 +58,7 @@ held), or (b) if cascading from a script write is intentionally unsupported, cor
|
||||
documentation and `VirtualTagContext` XML doc to say so. Decide deliberately and make
|
||||
code and docs agree.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-22 — `OnScriptSetVirtualTag` now launches a fire-and-forget `CascadeAsync(path, ...)` after updating the cache, mirroring `OnUpstreamChange`, so change-triggered dependents of a script-written tag are re-evaluated; added a regression test.
|
||||
|
||||
### Core.VirtualTags-002
|
||||
|
||||
|
||||
@@ -315,6 +315,14 @@ public sealed class VirtualTagEngine : IDisposable
|
||||
_valueCache[path] = snap;
|
||||
NotifyObservers(path, snap);
|
||||
if (_tags[path].Definition.Historize) _history.Record(path, snap);
|
||||
|
||||
// A cross-tag write must participate in the change-trigger cascade, exactly
|
||||
// like an upstream delta — any change-triggered tag that reads this path
|
||||
// would otherwise go stale until an unrelated trigger fires (see
|
||||
// docs/VirtualTags.md, VirtualTagContext section). Fire-and-forget: this
|
||||
// callback runs inside EvaluateInternalAsync with the non-reentrant
|
||||
// _evalGate held, so the cascade must be scheduled, not invoked inline.
|
||||
_ = CascadeAsync(path, CancellationToken.None);
|
||||
}
|
||||
|
||||
private void NotifyObservers(string path, DataValueSnapshot value)
|
||||
|
||||
@@ -275,6 +275,38 @@ public sealed class VirtualTagEngineTests
|
||||
engine.Read("Driver").Value.ShouldBe(5);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SetVirtualTag_within_script_cascades_to_dependents_of_the_written_tag()
|
||||
{
|
||||
// Regression for Core.VirtualTags-001: a ctx.SetVirtualTag write must schedule
|
||||
// the change-trigger cascade for tags depending on the written path, exactly
|
||||
// like an upstream delta. "Dependent" reads "Target" and is ChangeTriggered, so
|
||||
// a write to Target via ctx.SetVirtualTag must re-evaluate Dependent.
|
||||
var up = new FakeUpstream();
|
||||
up.Set("In", 5);
|
||||
using var engine = Build(up);
|
||||
|
||||
engine.Load([
|
||||
new VirtualTagDefinition("Target", DriverDataType.Int32,
|
||||
"""return 0;""", ChangeTriggered: false), // operator-written via SetVirtualTag
|
||||
new VirtualTagDefinition("Dependent", DriverDataType.Int32,
|
||||
"""return (int)ctx.GetTag("Target").Value + 1;""", ChangeTriggered: true),
|
||||
new VirtualTagDefinition("Writer", DriverDataType.Int32,
|
||||
"""
|
||||
var v = (int)ctx.GetTag("In").Value;
|
||||
ctx.SetVirtualTag("Target", v * 100);
|
||||
return v;
|
||||
"""),
|
||||
]);
|
||||
await engine.EvaluateAllAsync(TestContext.Current.CancellationToken);
|
||||
|
||||
// Writer set Target = 500; the cascade must have re-evaluated Dependent to 501.
|
||||
engine.Read("Target").Value.ShouldBe(500);
|
||||
await WaitForConditionAsync(() => Equals(engine.Read("Dependent").Value, 501));
|
||||
engine.Read("Dependent").Value.ShouldBe(501,
|
||||
"a ctx.SetVirtualTag write must cascade to change-triggered dependents");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Type_coercion_from_script_double_to_config_int32()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user