Browser-002/-003/-004 fixed and -005 resolved as a documented constants change
(eddf5f57); Contracts-004/-005/-006/-007 fixed (93f70504). Both module headers
drop to 0 open findings. Each resolution records the disposition, the RED
evidence, and the deviations from the recommendation where one was taken.
Driver.Mqtt.Contracts-004/-005/-006/-007.
-004: retainSeed was the last behavioural key read leniently, so "retainSeed":"false"
— what a CSV import produces — silently became the default TRUE, the INVERSE of the
authored intent. Both mappers now read it through TryReadBoolStrict (the qos
absent/valid/present-but-invalid split) and DescribeRejection names it. The authoring
side needed no hand-mirrored rule: MqttTagConfigModel.Validate already consults this
same parse through TagConfigParseGate, so the save is blocked with the inspector's
wording — pinned by a new AdminUI parity theory pair.
-005: the seven [Range] attributes on MqttDriverOptions/MqttPlainOptions never ran —
the options are bound by JsonSerializer, and nothing in the four MQTT projects calls
Validator.TryValidateObject. Two of them CONTRADICTED the real authority (KeepAlive's
[Range(1,…)] forbade the zero Normalize admits as legal MQTT; MaxPayloadBytes' forbade
the non-positive value its own remarks document as falling back to 1 MiB), so wiring
them up would have broken deliberate behaviour. Deleted, with each property naming
where its bound actually lives. The one escape they papered over is closed: an
out-of-range defaultQos rode into the SUBSCRIBE verbatim, and MqttSubscriptionManager's
constructor now clamps it exactly as the AdminUI driver form already persists.
-006: the DateTime write arm cast a negative long to ulong, so 1969-12-31 published a
timestamp ~584 million years in the future; it is now refused as ValueNotRepresentable.
An Unspecified kind was handed to ToUniversalTime(), which reads it as LOCAL — the same
authored instant encoded differently on the two nodes of a redundant pair; it is now
read as UTC.
-007: Format/FormatState enforced only null/empty, and nothing at all on deviceId, so
the type was not round-trip-closed. They now apply the parser's IsValidSegment (plus
'/', which only the format side can see). MqttWriteEncoder's own guards stay — TryEncode
returns an error rather than throwing, and that contract does not change.
Resolutions record the red-first evidence and the two decisions a later reader would
otherwise re-litigate: why the write block is emitted in both states, and why -007's
race budget (8 racers x 3000 rounds) is part of the fix rather than incidental.
Open findings 5 -> 1. The remaining one is Driver.MTConnect-003 (High, browse-committed
MTConnect tags coerced to String by RawBrowseCommitMapper), which was not in this batch.
001 aligned keyValue onto the same scalar read as whereValue (678aa3aa); 002 fixed the
numeric-enum-spelling accept in Core.Abstractions.TagConfigJson.ClassifyEnumText,
fleet-wide (8d4ca548).
IWritable shipped behind write.enabled and docs/drivers/MTConnect.md told the operator to
set it, but MTConnectDriverForm — the only editor DriverFormMaps maps for this type, with
no raw-JSON escape hatch and no driver-instance API — exposed no write section, so the
documented enablement path was a hand edit of DriverInstance.DriverConfig in the config DB.
- Write-back section: switch + form text stating it is observation injection, that the
Agent's AllowPut is an independent second gate, and that it does not by itself make a
node writable over OPC UA (Tag.AccessLevel still composes CurrentWrite).
- Write added to the mirrored MTConnectConfigDto and to the form model, written in BOTH
states — an omitted block would leave a previously-authored true standing, so the switch
could never turn writing off again.
- The false 'MTConnect is read-only' header comment is gone; the doc names the new section.
No bUnit here, so the model/serialization seam is unit-tested both sides: the AdminUI test
pins the emitted bytes, and MTConnectFactoryTests parses that same document and asserts
MTConnectDriverOptions.Write.Enabled. Live page verification remains a rig step.
The first form compared the publish's managed thread id with the caller's, which flaked
(1 in 6): the evaluation pump may reuse the pooled thread the test released while awaiting
the drain, so an id match proves nothing. It now sets a ThreadLocal flag for exactly the
OnDependencyValue window, so 'published inline' is answered on the stack rather than by
thread identity. Re-falsified by making SchedulePump run the pump synchronously (red
immediately, reverted); 5 consecutive clean suite runs after.
The two value-bearing fields were read asymmetrically and nothing said why.
whereValue went through ReadScalar (string/number/boolean — the design doc's own
wide-row example authors it as the number 7); keyValue went through the string-only
ReadString, so a numerically-authored key refused the tag outright. Nothing downstream
justified the split: both are BOUND as DbParameters and both are matched back as text —
SqlPollReader.IndexByKey stringifies the key cell precisely so an authored key can face
an INTEGER key column.
The refusal was also mis-explained. Inspect branched on ReadString(root, "keyValue") is
null, so a numerically-authored key drew "Sql KeyValue tag has no 'keyValue'" — telling
the operator the field is missing when it is right there in the blob, the #549 shape of
an authoring surface disagreeing with the runtime about WHY.
keyValue now uses ReadScalar; Inspect mirrors it with HasScalar and says what a usable
key is; SqlTagConfigModel loads it with the shared TagConfigJson.GetScalarText (which
also replaces SqlRowSelectorModel's private copy), so the editor is not left stricter
than the parser. An object/array/null key is still refused. docs/drivers/Sql.md updated.
RED first: 4 failing in Driver.Sql.Tests.
-003 is a documented Won't Fix: the mitigating control is the ConfigEditor page
policy on the only two surfaces that reach a browse session (/raw and /uns),
there is no HTTP endpoint for browse, and PageAuthorizationGuardTests pins both
pages to that exact policy. Recorded in docs/security.md under Control-Plane
Authorization and pointed at from SqlDriverBrowser's class summary.
Dialect header 3 -> 0 open. The Driver.Sql header count is left alone for the
orchestrator to reconcile — sibling agents are editing that file in parallel.
Both modules go to 0 open findings. -005 records the concurrency guarantee the fix rests
on (at most one evaluation at a time: single pump + _evalGate, lock order _evalGate ->
_lock), the RED evidence, why the dependency snapshot is taken at trigger time, why the
timer path was left on its own scheduler, and the unbounded-queue trade. -001 records that
the equivalence is exact (Inspect's pre-checks already establish TryReadTagConfig's) and
that its new pin was verified against the pre-refactor code.
Driver.Mqtt.Browser-002/-003/-004/-005.
-002: ObservePlainTopic's pre-pass judged segment LENGTH only, so a plain topic
segment carrying a newline or an ANSI escape became the picker's displayed label —
a rule ObserveSparkplugBirth already applied via IsUsableLabel. The control-character
half is now single-sourced (HasControlCharacter) and the plain marker text converges
with the Sparkplug one. An EMPTY topic level stays accepted on purpose: it is legal
MQTT and its terminal leaf is a real committable address.
-003: GetOrCreate read _nodeCount and incremented it later, so N observers could each
see cap-1 and all insert. The slot is now reserved with one Interlocked.Increment and
given back on both non-consuming paths.
-004: Project ordered a level by Segment alone, but the level is keyed by Path
precisely because two Sparkplug siblings CAN share a Segment — leaving the tie to
ConcurrentDictionary enumeration order, which .NET's randomised string hashing makes
differ per process. .ThenBy(Path) makes the sort total.
-005: no behaviour change — DefaultNodeCap's doc now states the real per-session
ceiling and records that the aggregate across sessions is bounded by the reaper's
idle TTL, not by any session cap.
ListSchemasSql has two consumers and the remarks only justified the divergence
from the catalog gate's viewpoint. SqlBrowseSession.RootAsync renders every row
as an expandable schema, so an Oracle browse offers every database user. The
narrower ALL_TABLES/ALL_VIEWS query is named, along with why it is not taken
here: it narrows the gate too, and that is a poll-path behaviour change only a
real Oracle server can gate.
Inspect parsed the TagConfig blob once itself (root kind + the present-but-discarded
changeTriggered/timerIntervalMs checks) and then called TryReadTagConfig(reference, out _),
which parsed the same string from scratch again for the scriptId rule. The field-by-field
read is now TryReadRoot(JsonElement), which both entry points share, so Inspect consults
the one authority over the root it already has. The public string-based TryReadTagConfig
signature other modules consume is unchanged.
Pure refactor: added CalculationTagDefinitionFactoryInspectTests (17 cases pinning the
exact wording, the rejection-vs-advisory split and their order, and the
never-stricter-than-the-runtime-read invariant). They pass unchanged against the
pre-refactor code — verified by stashing the production change.
ISqlDialect gains MaxBindParameters — the per-command ceiling the seam was
missing while modelling everything else that differs per backend: 2098 usable
of SQL Server's 2100 (sp_executesql spends two), 1000 for Oracle's IN-list
limit (ORA-01795, tighter than its 64k bind count), 65535 for the Postgres
Bind message.
SqlGroupPlanner.AddKeyValuePlans now emits several plans for one group when
its distinct keys exceed that ceiling, instead of one statement the provider
refuses after the connection opened — which left every tag in an EAV group
publishing BadCommunicationError on every poll, permanently, and reported the
database endpoint as down. Chunks share the group key; members follow their
own key so a shared keyValue never splits. SqlPollReader is unchanged: it runs
one command per plan and slices each plan's own Members back, proven by a
fixture read at a ceiling of 1.
OnDependencyValue ran every change-triggered dependent's script INLINE, under _lock,
via TimedScriptEvaluator.RunAsync(...).GetAwaiter().GetResult() — on the Akka dispatcher
thread of Runtime's DependencyConsumerMuxAdapter, for up to RunTimeout (2 s default) per
arrival, with the state lock held for the whole run.
The callback now records the value, PREPARES the affected tags under the lock (definition
+ warm-up gate + dependency snapshot taken at trigger time) and queues them for a single
serialized evaluation pump. _lock is a short-held state lock again; a new _evalGate
serializes the pump against the timer ticks and is taken only by evaluation workers, so
no host thread can wait on a script. Lock order is always _evalGate -> _lock.
Semantics preserved: per-tag ordering (one FIFO pump), the warm-up gate BEFORE the
evaluator runs, dedup/Bad-transition/recovery publishes, and — because the dependency
snapshot is taken when the trigger fires rather than when the pump catches up — a burst
of arrivals still evaluates against the values that triggered it.
TagConfigJson.ClassifyEnumText classified via Enum.TryParse(text, ignoreCase: true),
which accepts a numeric STRING for any enum: "1" returns the member at that ordinal
and an out-of-range "99" parses just as happily. Every driver contract's strict enum
read inherits that, so a Sql tag authored "type":"99" was ACCEPTED, carried
(DriverDataType)99, and SqlPollReader.TryCoerce fell through its switch to the String
arm — a stringified cell published Good under a type nothing can map.
Refuse a digit- or sign-leading spelling before the parse (judged on the TRIMMED text,
because Enum.TryParse trims), and require Enum.IsDefined — which covers what the
leading-character check cannot, since TryParse also ORs a comma-separated list for a
non-[Flags] enum. Mirrors RawBrowseCommitMapper.ParseNativeType and
MTConnectTagDefinitionFactory.TryParseDataType, which already guard their own parses.
Three AdminUI restatements of the same rule now ask ClassifyEnumText instead of
Enum.TryParse (TagConfigJson.GetEnum, SqlTagConfigModel.ReadEnumStrict,
MqttTagConfigModel.DescribeInvalidEnum/GetEnumNullable) — leaving them would have opened
the #549 direction the moment the runtime tightened: a blob the modal accepts and the
driver skips.
RED first: 10 failing in Core.Abstractions.Tests, 4 in Driver.Sql.Tests, and 10 of the
11 new fleet-wide parity cases (re-confirmed by reverting the Core fix).
Driver.MTConnect-005/-006/-007, each TDD'd red-first.
- -005: _options was the one field at the StartCoreAsync commit point published by a
plain write and read lock-free from ReadAsync/WriteAsync/DiscoverAsync/HostName/health.
Now Volatile.Write/Volatile.Read like _session/_binding/_announcedInstanceId, pinned by
a source scan (MTConnectOptionsPublicationTests) that fails on any unfenced touch.
- -006: TryFormatValue guarded null and Array, but Convert.ToString renders ANY
non-IConvertible value as its type name under a Good status. Guard is now the property
the remark relies on; the accepted set (string/bool/numeric/DateTime) is unchanged.
- -007: MTConnectAgentClient.Dispose was a check-then-set on a non-volatile bool, so two
racing disposals both passed and the loser cancelled an already-disposed CTS. Now an
Interlocked.Exchange claim over an int, with the request seams reading it volatile.
(1) DisposeAsync disposed _gate unconditionally. SemaphoreSlim.Dispose does not
complete pending async waiters, and the in-flight call's Release() then threw
ObjectDisposedException from inside the semaphore (swallowed by the finally), so
a caller parked in WaitAsync was never released and its call never returned. The
gate is now left alive - the pattern SqlPollReader documents - so the waiter is
admitted and meets the session's own guarded ThrowIf(_disposed, this).
(2) LastUsedUtc advanced only on completion, so a slow catalog call (the AdminUI
allows 20 s) was invisible to the TTL reaper for its whole duration and a session
at the edge of its TTL could be disposed mid-call. It is now stamped on entry
too.
PreferredTimestampColumn nominated the first column mapping to DriverDataType
.DateTime, and that set includes date-only types (SQL Server / PostgreSQL
'date'). A table whose first date/time column is a 'date' committed
rowSelector.topByTimestamp = <date>, making the wide-row read ORDER BY <date>
DESC LIMIT 1 - which ties for every row written that day, so 'the newest row'
became storage order among the ties.
Time-bearing candidates now outrank date-only ones; a date-only column is still
stated when it is all the table has, so such a table stays bindable. Oracle's
DATE carries a time of day and is exempt - pinned by a control dialect that
differs from the SQLite one only in the Provider it reports.
Records the disposition and rationale for each: the primary-host gate is no
longer disposed (matching MqttConnection's documented rule), a reconnect that
resolves zero filters for live topics now throws like the zero-granted case it
is indistinguishable from, and a password configured without a username is
warned about rather than silently dropped. Both module headers drop to 0 open.
Driver.Mqtt-006 (Low, concurrency). DisposeAsync disposed _gate while StopAsync
cancels the reconcile loop WITHOUT awaiting it, so a ReconcileAsync already in
flight could be inside _gate.WaitAsync, or on its way to the finally-release,
when the semaphore went away. The release then threw ObjectDisposedException
naming SemaphoreSlim, logged as a reconcile failure — telling an operator
nothing about what happened and hiding whatever the reconcile was really
failing on.
Left undisposed, matching MqttConnection's documented rule for its own lifecycle
primitives and its stated rationale: a late callback racing teardown should be a
benign no-op rather than an ObjectDisposedException naming the wrong type. Two
lifecycle types in one module disagreeing about this is what gets copied into
the third. Nothing is leaked — the gate holds no timer and no registration — and
ClaimAsync still re-checks _disposed under it, so no claim can be created after
dispose either way.
Test is deterministic rather than timing-dependent: the gate is taken before the
connect starts, so a MiniBroker in blackhole mode (TCP accepted, CONNECT never
answered) accepting the socket proves the claim is inside the gate when dispose
runs. RED: log.Captured contained "ObjectDisposedException".
Driver.Mqtt-007 (Low, error handling). OnReconnectedAsync throws when a
re-subscribe is granted zero filters — MqttConnection's contract is that a
throwing Reconnected subscriber tears the fresh session down and retries under
backoff, because a connected-but-deaf driver is the one outcome this path must
never serve. But when the live subscribed topics resolved to zero filters in the
first place it logged a warning and returned, reaching the same "nothing was
re-established" state by a different route and giving it the opposite answer.
Throw there too, and say so in the remarks so the type states ONE rule. It costs
no other tag — every live topic is in the set that resolved to nothing, so there
is nothing left flowing to protect, which is the whole reason the partial-
rejection case is exempt — and it is not permanent: the retry runs against
whatever Register has published by then.
Test reaches the state the only way production can: Register prunes
_subscribedTopics of the topics its new table no longer carries, but a SUBSCRIBE
already in flight records its own topic AFTER that prune ran, so a deploy landing
inside the round-trip leaves a live topic no authored tag resolves to. Paired
with the control that a reconnect with no live subscriptions at all stays a
no-op — throwing there would tear down the session of every driver whose tags
have not been subscribed yet.
Driver.Mqtt.Transport-002 (Low, correctness). Credentials were attached only
when Username was non-empty, so a config carrying a Password and no Username
connected anonymously with the secret silently discarded — the deployment reads
as authenticated and is not, which is the direction an operator cannot see from
the config.
Warned rather than sent. MQTT 3.1.1 may not carry a password without a
username, so the password-only CONNECT is not available on every protocol
version this factory serves, and synthesizing one under V500 only would change
the wire behaviour of deployments that connect happily today. The configured
values stay authoritative; the drop just stops being silent. The secret is
never formatted into the message.
Tests: the warning fires and does NOT contain the password, plus the falsifying
control that an ordinary anonymous config (neither credential set) stays silent
— without it the fix could warn on every connect and still pass.
NAMEDATALEN-1 (63) and Oracle 12.2's 128 are byte limits in the database
encoding, not character counts, so a 32-character Cyrillic name passed
ident.Length and was then silently truncated (Postgres) or rejected by the
server (Oracle). Both dialects now measure Encoding.UTF8.GetByteCount and say
'bytes' in the rejection. SqlServerDialect deliberately keeps ident.Length —
sysname is nvarchar(128), a real character limit — pinned by a control test.
SqlDriverProbe surfaced 'Config JSON is invalid: ' + ex.Message, where
SqlDriverBrowser.ParseDocument deliberately withholds the identical message
because the malformed text may itself carry a connection string. The two read
the same blob shape - the AdminUI Sql picker composes a document with a
top-level connectionString literal - so a malformed paste reaches both. The
probe now reports the failure class only, matching the browser.
The comments and docs that described Driver.MTConnect-003 as live at HEAD would
have been wrong the moment 64be41d8 landed:
- code-reviews/Driver.MTConnect/findings.md — Driver.MTConnect-003 Resolved,
with the fixing commit; module open-finding count 5 -> 4.
- code-reviews/Driver.MTConnect.Contracts/findings.md — the one clause inside
Contracts-001's prose that asserted 003 was Open.
- docs/drivers/MTConnect.md — the coercion-precedence warning now states that a
browse-committed tag resolves at step 1 CARRYING the step-3 answer, and that
step 3 remains the rescue for a hand-authored / CSV / API blob with no type.
- MTConnectDriver.BuildBinding, MTConnectDataTypeInference and
MTConnectDriverOptions.RawTags remarks — same correction, each keeping a short
historical note of what the defect was so a reader meeting the old blobs still
understands them.
Comment-only in src; the driver suite (538) and AdminUI suite (1474) re-run green.
RawBrowseCommitMapper.MapLeaf typed a browse-committed raw tag's OPC UA node
from the leaf's probe-inferred DriverDataType, but its BuildTagConfig MTConnect
branch emitted `new MTConnectTagConfigModel { FullName = address }` — and that
model's DataType defaults to String with ToJson() unconditionally writing
`dataType`. A non-blank authored type wins the driver's coercion precedence, so
the Agent-probe inference step was never consulted and every browsed SAMPLE
published text into a Double/Int64 node. Silent: the blob is perfectly valid, so
neither the authoring gate nor the deploy gate had anything to say.
Same defect class as #536, which is why the AbCip / TwinCAT / FOCAS branches
forward the browsed native type via NativeType(addressFields, …). MapLeaf now
passes driverDataType into BuildTagConfig, whose MTConnect branch uses it as the
NativeType fallback — a stated native `dataType` address field still wins, so
the branch reads like its siblings; the difference is only that MTConnect's
fallback is the leaf's own inference rather than a fixed model default. Its
coercion type IS a DriverDataType, the very enum the leaf carries, so nothing is
re-derived. NativeType's numeric-string and unparseable-name guards are shared
via the extracted ParseNativeType, so an unusable name still keeps String.
Tests assert the PAIR (row OPC UA type and blob coercion type agree with the
inference) — the blob alone would pass against a mapper that stopped typing the
row — plus the parity control that a leaf inferred (or reported) as String still
commits `"dataType":"String"`, so the forward cannot be implemented as a
deletion. Observed red first: expected "Float64" but was "String".
Joseph Doherty