A race the comment named and then reasoned past - #191
Merged
Conversation
Four-agent review of the whole project. Most of it came back clean; this fixes
what did not, and the first one is real.
g_coredump WAS UNGUARDED ACROSS THREE TASKS. The erase action clears it from the
AsyncTCP task; bridgeInfo() copies its std::string taskName and its backtrace from
loop(), rs485Task and the AsyncTCP task, about four times a second. Assigning over
a std::string while another core copies it is not a stale read, it is a
use-after-free waiting for the two to line up.
What makes it worth writing down: the declaration's own comment NAMED the
mutation -- "Cleared in place when the erase action succeeds, which is the one
thing that CAN change it" -- and then spent three lines arguing the cached copy is
accurate. Thirty lines above sits g_configMutex, written for exactly this hazard,
with a comment describing exactly this shape. The reasoning stopped one step short
of its own conclusion. Now guarded by the same pattern.
SunSpec readModel() narrowed kHeaderRegisters + entry.length to uint16. entry.length
comes off the bus unbounded, so 65534 wraps the sum to zero and the read silently
becomes a no-op. Not exploitable -- loop and decoders agree on the wrapped value
and refuse it -- but walkChain() does the same arithmetic in uint32 with a bounds
check thirty lines up. Now consistent.
Five documentation claims that were false, all verified against code before
changing:
- adding-a-device.md said enum mode setpoints "cannot" be expressed. They can;
write-path.md documents how and the profile driver implements it. Both texts
landed in the same commit and survived a dedicated doc-review pass
- prometheus.md was missing heliograph_grid_power_watts, added yesterday, and
docs/README.md still said 52 metrics where there are 53
- README.md said 442 host tests where there are 1022, and twenty-nine documents
where there are thirty-three
- README.md said a deliberate write path "is not enabled". The path is built,
wired to REST and MQTT and tested end to end; what is missing is a verified
register, not the path. The safety claim it was making is still true, but it
was making it about the wrong thing
- schema.md's example output still showed one profile valid, where ten are
Everything else audited came back clean and is worth saying: the write path has no
bypass (every mutating REST route authorises, Modbus TCP refuses writes at three
independent points, three differently-defaulted gates stand between a build and a
moved inverter), no exploitable out-of-bounds was found in any parser, and the
"one activity per bus-task iteration" invariant holds.
Two follow-ups from the review round, both dormant today and both the kind that only bite once somebody else changes something. THE BUS LOCK WAS MISSING FROM THE ENTIRE MODBUS FAMILY. PMU and MaxTalk drivers take Transport::lock; the profile driver, the SunSpec driver and both of their discovery probes never did, because they all route through modbus_client's runTransaction() and it did not. Meanwhile CommandDispatcher's own comment states that "execute() on a real driver is an RS485 transaction that waits up to 2 s for the bus lock" -- false for every profile-driven inverter, which is most of them. Harmless right now: only rs485Task ever reaches a Transport, and its one-activity-per-iteration rule already serialises everything. That is exactly why it was worth closing rather than documenting away. The protection was an invariant stated nowhere near the call site, and the first second bus user would have found half the drivers guarded and half not -- with the class comment pointing at the wrong half. Taken in runTransaction() because every Modbus path funnels through it, and verified free of nesting: none of the four existing TransportLock sites routes through this function. COMMAND OUTCOMES WERE KEYED ON THE REQUEST ID ALONE. A request id is the CALLER's to choose -- REST and MQTT both accept one -- so two devices can legitimately carry the same one. Polling device A could then return device B's result: the right answer to somebody else's question, which is worse than no answer because it looks like one. Now keyed on (deviceId, requestId), threaded through both the REST context and the MQTT outcome provider. Dispatch was never affected. That is scoped by deviceId in Request, so the correct device always received the correct command; only the report an operator reads was wrong. Worth being precise about, because "commands could go to the wrong inverter" is what this sounds like and is not what it was. The new test is mutation-proven: dropping the device comparison fails it.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens correctness across concurrency, command-result reporting, Modbus bus access, and SunSpec parsing, and aligns documentation with the codebase’s actual behavior (including write-path gating).
Changes:
- Make coredump summary access thread-safe and scope command outcomes by
(deviceId, requestId)end-to-end (REST/MQTT/tests). - Add a Transport-level bus lock to shared Modbus transactions and fix SunSpec
readModel()span narrowing. - Correct several documentation claims (metrics counts, write-path wording, schema/examples).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_commands/test_main.cpp | Updates CommandQueue outcome tests and adds coverage for cross-device request id collisions. |
| src/protocols/modbus/modbus_client.cpp | Adds a TransportLock around Modbus transactions with a fixed timeout. |
| src/outputs/rest/rest_api.h | Updates RestContext commandOutcome callback signature to include deviceId. |
| src/outputs/rest/rest_api.cpp | Passes (deviceId, requestId) when querying command outcomes. |
| src/outputs/mqtt/mqtt_output.h | Updates MQTT command outcome provider signature to include deviceId. |
| src/outputs/mqtt/mqtt_output.cpp | Passes (channel.id, requestId) when polling for command outcomes. |
| src/main.cpp | Adds g_coredumpMutex guarding reads/writes; wires new CommandQueue outcome API through MQTT/REST; scopes outcome recording by device. |
| src/drivers/sunspec/sunspec_driver.cpp | Prevents uint16_t wrap in readModel() by using uint32_t and bounding. |
| src/commands/command_queue.h | Changes outcome API to be scoped by deviceId and stores last outcome device id. |
| src/commands/command_queue.cpp | Implements device-scoped outcome storage and lookup. |
| README.md | Updates documentation counts/test count and adjusts write-path wording (still needs one correction). |
| docs/README.md | Updates Prometheus metric count from 52 to 53. |
| docs/prometheus.md | Documents heliograph_grid_power_watts. |
| docs/device-profiles/schema.md | Updates gen_profiles.py --check example output. |
| docs/adding-a-device.md | Fixes incorrect “cannot” claims about what the write path supports. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+200
to
+202
| if (span > 0xFFFF) { | ||
| return false; | ||
| } |
Comment on lines
490
to
+492
| All inverter drivers are **read-only**, and that is not in tension with the curtailment above: | ||
| no driver ever writes to your inverter over its protocol. Sending a setpoint would need a | ||
| hardware-verified register map and a deliberate write path, and neither is enabled today (see | ||
| hardware-verified register map and a write row marked verified, and no shipped profile has one (see |
Comment on lines
+51
to
+52
| // No nesting risk -- no caller of this function holds the lock (the four TransportLock sites | ||
| // are all on PMU, MaxTalk or capture paths, none of which route through here). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four-agent review of the whole project. Most came back clean; this fixes what did not.
g_coredumpwas unguarded across three tasks — the real oneThe erase action clears it from the AsyncTCP task;
bridgeInfo()copies itsstd::string taskNameand its backtrace fromloop(),rs485Taskand the AsyncTCP task, about four timesa second. Assigning over a
std::stringwhile another core copies it is not a stale read — it isa use-after-free waiting for the two to line up.
What makes it worth recording: the declaration's own comment named the mutation — "Cleared in
place when the erase action succeeds, which is the one thing that CAN change it" — and then spent
three lines arguing the cached copy is accurate. Thirty lines above sits
g_configMutex, writtenfor exactly this hazard, with a comment describing exactly this shape. The reasoning stopped one
step short of its own conclusion.
SunSpec
readModel()narrowingkHeaderRegisters + entry.lengthwas cast touint16.entry.lengthcomes off the bus unbounded,so 65534 wraps the sum to zero and the read silently becomes a no-op. Not exploitable — but
walkChain()does the same arithmetic inuint32with a bounds check thirty lines up.Five false documentation claims, each verified against code first
adding-a-device.mdsaid enum mode setpoints "cannot" be expressed. They can;write-path.mddocuments how and the profile driver implements it. Both texts landed in the same commit and
survived a dedicated doc-review pass.
prometheus.mdwas missingheliograph_grid_power_watts;docs/README.mdsaid 52 metrics wherethere are 53.
README.mdsaid 442 host tests where there are 1022, and twenty-nine documents wherethere are thirty-three.
README.mdsaid a deliberate write path "is not enabled". The path is built, wired to REST andMQTT, tested end to end; what is missing is a verified register, not the path. The safety
claim was still true — it was just about the wrong thing.
schema.md's example output showed one profile valid, where ten are.What came back clean, and is worth stating
No bypass in the write path — every mutating REST route authorises, Modbus TCP refuses writes at
three independent points, and three differently-defaulted gates stand between a build and a moved
inverter. No exploitable out-of-bounds in any parser. The "one activity per bus-task iteration"
invariant holds.
Verification
1022 host tests, all gates, board build clean.