Repository navigation
bug(config): nats mq defaults duplicate mq.Default* untested, no grammar check, sweeper role is a silent no-op #691
Description
Area: config — external NATS mq backend
Three related gaps in how mq.nats's config is validated and kept in sync with the code it configures, all in internal/config/mq_nats.go and its interaction with internal/mq/nats_topology.go / internal/app/app.go.
1. defaultMQNATS() duplicates mq.Default* with nothing pinning them equal.
defaultMQNATS() (internal/config/mq_nats.go:60) hard-codes SubjectPrefix: "wh", Partitions: 1, Shards: 32, IngestConsumer: "wh-ingest", which are meant to mirror mq.DefaultNATSSubjectPrefix, mq.DefaultNATSPartitions, mq.DefaultNATSShards and mq.DefaultNATSIngestConsumer (internal/mq/nats_topology.go:61-67). Only one of the four, MaxNATSShards, is pinned by a test (internal/app/mq_nats_test.go:126, assert.Equal(t, mq.MaxNATSShards, config.MaxNATSShards)) — and that constant isn't even one of the four defaultMQNATS() duplicates. Nothing checks that defaultMQNATS()'s literals still match mq.Default*.
- Scenario: someone changes
mq.DefaultNATSShards(e.g. while retuning the shipped topology) without noticingdefaultMQNATS()needs the same change.wavehouse mq manifests(which readsmq.Default*) then generates a different shard count than a default-config boot assumes, and a default-configured process fails its own topology check at boot with a metadata mismatch finding. - Evidence: measured —
grepfor a test assertingdefaultMQNATS()'s literals againstmq.Default*finds none beyond the oneMaxNATSShardscase.
2. ingest_consumer and history_stream have no grammar check; other name-like fields do.
MQNATSConfig.validate() (internal/config/mq_nats.go, around line 88) checks SubjectPrefix against natsSubjectPrefix (a [a-z0-9_-] token regex) and rejects an empty IngestConsumer — but IngestConsumer's only check is non-empty, not the same token grammar, and HistoryStream has no validation at all at the config layer (contrast CoordNATSConfig.validate(), mq_nats.go:194, which checks Bucket against natsBucketName, [a-zA-Z0-9_-]).
- Scenario:
mq.nats.ingest_consumerormq.nats.history_streamis set to a value containing.or a space (invalid in a NATS subject/stream-name token). Config load succeeds; the malformed name surfaces later, if at all, as a topology-verifier finding at boot rather than a clear config error at load time — still fail-closed, but a worse diagnostic. - Evidence: inferred, from reading
validate()and comparing it against the other name-like fields it does check.
3. A sweeper role paired with another role under mq.backend: nats is a silent no-op.
Config.validateNATSTopology() (internal/config/mq_nats.go, around line 166) only refuses roles: [sweeper] alone: if c.MQ.Backend == MQNATS && len(c.Roles) == 1 && c.Roles[0] == RoleSweeper. A config like roles: [api, sweeper], or the default roles: [api, ingest, sweeper], passes validation under mq.backend: nats — but App.wire (internal/app/app.go:207) only wires the sweeper when MQ.Backend == config.MQEmbedded:
if a.cfg.Has(config.RoleSweeper) && a.cfg.MQ.Backend == config.MQEmbedded {
a.wireSweeper()
}So under nats, a process configured with sweeper alongside another role boots successfully and simply never runs a sweeper — nothing errors, and nothing warns. This is consistent with the documented design (retention under nats is the operator's stream policy, so there is genuinely nothing for the sweeper to do there), but an operator who leaves sweeper in a multi-role process's config under nats — perhaps left over from switching backends — gets no signal that the role is being ignored.
- Evidence: measured, by reading
validateNATSTopologyandapp.go's wiring together; the sweeper-only case is confirmed refused, the multi-role case is confirmed accepted with no warning.
Reproduce/confirm: for (1), diff defaultMQNATS()'s literals against mq.Default* after changing one; for (2), load a config with mq.nats.ingest_consumer: "bad name" and check it passes Load; for (3), boot a process with roles: [api, sweeper] and mq.backend: nats and check that no sweeper runs and no warning is logged.
Found in review of #624.
Metadata
Metadata
Assignees
Labels
Type
Projects
- StatusShow more project fieldsBacklog
Activity
feat(mq): external nats broker with sharded, pinned ingest workers (#624