Time out an abandoned admin edit transaction - #11254
Merged
Merged
Conversation
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (31)
Build artifacts expire on 2026-08-26. Updated for |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
ChangesEdit transaction expiration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/modules/AdminModule.cpp (1)
1884-1892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim these header comments to the 1-2 line limit. Both blocks exceed the project's comment-length guideline, with the
.cppblock forming a two-paragraph explanatory block.
src/modules/AdminModule.cpp#L1884-L1892: condense the 8-line, two-paragraph rationale into 1-2 lines (the bug context can live in the PR/commit message rather than the source).src/modules/AdminModule.h#L43-L45: condense the 3-line comment abovekEditTransactionIdleMsto 1-2 lines.As per coding guidelines, "Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious... do not restate straightforward code or add multi-paragraph explanatory blocks."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/AdminModule.cpp` around lines 1884 - 1892, Condense the explanatory comment above the unattended edit-transaction timeout in src/modules/AdminModule.cpp lines 1884-1892 to one or two lines, retaining only the timeout’s purpose. Also shorten the comment above kEditTransactionIdleMs in src/modules/AdminModule.h lines 43-45 to one or two lines without changing code behavior.Source: Coding guidelines
src/modules/AdminModule.h (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstant naming doesn't follow the project's
UPPER_SNAKE_CASEconvention.
kEditTransactionIdleMsuses ak-prefixed camelCase style; the rest of the block (fields, method) is fine.♻️ Suggested rename
- static constexpr uint32_t kEditTransactionIdleMs = 300 * 1000; + static constexpr uint32_t EDIT_TRANSACTION_IDLE_MS = 300 * 1000;Remember to update the usages in
src/modules/AdminModule.cppandtest/support/AdminModuleTestShim.h.As per coding guidelines, "Use PascalCase for classes, camelCase for functions and member variables, and UPPER_SNAKE_CASE for constants and defines."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/AdminModule.h` around lines 46 - 50, Rename the constant kEditTransactionIdleMs to the project-standard UPPER_SNAKE_CASE name EDIT_TRANSACTION_IDLE_MS, and update every usage in AdminModule.cpp and AdminModuleTestShim.h while leaving the surrounding fields and method unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/modules/AdminModule.cpp`:
- Around line 1884-1892: Condense the explanatory comment above the unattended
edit-transaction timeout in src/modules/AdminModule.cpp lines 1884-1892 to one
or two lines, retaining only the timeout’s purpose. Also shorten the comment
above kEditTransactionIdleMs in src/modules/AdminModule.h lines 43-45 to one or
two lines without changing code behavior.
In `@src/modules/AdminModule.h`:
- Around line 46-50: Rename the constant kEditTransactionIdleMs to the
project-standard UPPER_SNAKE_CASE name EDIT_TRANSACTION_IDLE_MS, and update
every usage in AdminModule.cpp and AdminModuleTestShim.h while leaving the
surrounding fields and method unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0389b622-c377-404f-8be1-d3a873979eea
📒 Files selected for processing (4)
src/modules/AdminModule.cppsrc/modules/AdminModule.htest/support/AdminModuleTestShim.htest/test_admin_radio/test_main.cpp
begin_edit_settings sets a bool that only the matching commit ever clears. If the commit never arrives -- the client dropped its link partway through a bulk config import, or went away entirely -- the transaction stays open indefinitely, and every later config write from any client is applied to RAM, acknowledged, and then never saved. The writes look successful and are visible in get_config, but the node reverts all of them at the next boot, and only a reboot clears it. Give the transaction a one minute idle timeout. Each deferred write restarts the clock, so the window bounds the gap between writes rather than the length of the edit; a bulk import sends them milliseconds apart. The next admin message after it lapses retires the transaction, persisting the segments it had deferred and flushing the warnings it was holding. The check runs after the auth gates and before the switch, so every case sees consistent state and the recovery's flash write happens in the main loop rather than a disconnect callback. It saves rather than rolls back because there is nothing to roll back to: each write already took effect in RAM and was acknowledged, so the transaction only ever deferred the save. That also makes an early expiry cheap -- the worst case for a client that really was still going is that its remaining writes are saved individually instead of batched. Closing on disconnect instead was tempting, but the reporter's captures show iOS dropping and reconnecting within half a second several times per session, which would abort imports that currently survive the blip. Also drops the file-scope hasOpenEditTransaction, shadowed by the class member at every use site and referenced nowhere else in the tree. Reported in #11245
thebentern
force-pushed
the
fix/admin-edit-transaction-timeout
branch
from
July 27, 2026 12:16
5e3dabb to
02094d4
Compare
8 tasks
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
1 participant
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.
Reported alongside #11245, but a separate defect with its own cause.
The problem
begin_edit_settings/commit_edit_settingsbrackets a bulk config write so the node saves once at the end instead of after every field. The "transaction" is a singleboolon the AdminModule singleton, and nothing ever closes it except the matching commit — no timeout, no binding to the client that opened it, and no hook on disconnect.So if the commit never arrives, the flag stays set forever. From that moment on, every config write — from any client, not just the one that opened it — is applied to RAM, acknowledged, and then quietly not saved:
The writes look successful to the app and are visible in
get_config, but the node reverts all of them at the next boot. Only a reboot clears it.@bruschill hit exactly this: an iOS device-profile import dropped its link after 16 of 20 writes. Their capture shows the 16 deferred saves, no commit, and the transaction still open when the log ends.
The fix
Give the transaction an idle timeout. Each write it defers refreshes the clock; if it then sits untouched for a minute, the next admin message retires it — persisting exactly the segments it had deferred and flushing the warnings it was holding.
Because every deferred write restarts the clock, the minute bounds the gap between writes rather than the length of the edit — a bulk import sends them milliseconds apart, so it has no bearing on how long an import may take. And expiring early is cheap: the transaction saves what it applied, so the worst case for a client that really was still going is that its remaining writes get saved individually instead of batched. Nothing is lost either way, which is what makes it safe to keep the window short and get the node back to normal quickly.
The check runs at the top of
handleReceivedProtobuf(), after the auth gates and before the switch, so every case below sees consistent transaction state. That's a plain main-loop context, which matters: the recovery does a flash write, and doing that from a BLE disconnect callback is how you get the stack overflows and disconnect deadlocks we've been fixing lately (#11190, #11202).The expiry saves rather than rolls back. There's nothing to roll back to — each write already took effect in RAM and was acknowledged, so the transaction only ever deferred the save, not the change. Persisting what's already live is the outcome with no surprises in it. It saves only the segments actually touched rather than the blanket set that commit uses, to avoid gratuitous nodeDB writes.
Why not close it on disconnect
That was my first instinct, and the reporter's logs argue against it: their session shows four disconnect/reconnect cycles, with iOS reconnecting within half a second. Aborting the transaction on the first BLE blip would break imports that currently survive one. A timeout tolerates the blip and still bounds the damage.
Also here
Removed the file-scope
bool hasOpenEditTransaction;in AdminModule.cpp. It's shadowed by the class member of the same name at every use site and referenced nowhere else in the tree — dead since the member was introduced, and actively confusing to read next to the real one.Testing
Three cases added to
test_admin_radio, which already covers transaction-scoped warning coalescing:The test shim backdates the activity clock rather than waiting out the real window. Native suite passes.
Summary by CodeRabbit