fix(firmware): close shot-history file/header inheritance race across…#715
Open
FR4NKLN wants to merge 2 commits into
Open
fix(firmware): close shot-history file/header inheritance race across…#715FR4NKLN wants to merge 2 commits into
FR4NKLN wants to merge 2 commits into
Conversation
… rapid brews When a brew ends, ShotHistoryPlugin's cleanup branch in record() only runs on the next 250ms tick AND only if recording=false AND extendedRecording=false. If the user starts a second brew before that tick runs (or before extendedRecording's up-to-3s window closes), the new brew's startRecording() flipped recording=true and left the previous shot's file handle and header in place — the next record() tick saw isFileOpen=true and appended the new brew's samples into the old file under the old header, then upserted the index with the wrong profileId / profileName. The new brew was effectively erased; the old shot's true ending was lost. Two changes: 1. Extract the cleanup branch in record() into finalizeShotFile(), then call it defensively at the top of startRecording() when isFileOpen. The previous shot now gets properly patched, closed, and indexed before the new shot's state is initialized. 2. Populate the shot file header (magic / sample-size / startEpoch / profileId / profileName / phaseTransitionCount) synchronously in startRecording() using the running BrewProcess's per-shot immutable profile copy, rather than re-reading the live selectedProfile in record() on the next tick. Closes a separate narrower race where a profile switch landing between brew:start and the first record() tick would stamp the wrong profile name into the header. Repurposes the existing currentProfileName field (previously set but unread) as a mirror of the header value for downstream telemetry. record()'s file-open path no longer initializes the header — startRecording guarantees it's already populated.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughShot recording lifecycle refactored: header initialization moved to startRecording(), end-of-shot work extracted into finalizeShotFile(), and startRecording() now finalizes any open shot file before starting a new one. ChangesShot Recording and Finalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…tory header
SonarQube flagged the two `strncpy` calls in `startRecording()` as
`cpp:S5816` ("strncpy may not null-terminate") security hotspots —
even though each was followed by an explicit NUL-termination write.
Switched to `snprintf`, which is bounded and always null-terminates
within the destination buffer in one call.
Observable behavior identical; clears the Quality Gate on this PR.
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
Owner
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
FR4NKLN
added a commit
to FR4NKLN/gaggimate
that referenced
this pull request
May 29, 2026
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
2 participants
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.
Summary
When a user starts a second brew before the first brew's shot file has been finalized,
ShotHistoryPluginwrites the new brew's samples into the previous brew's file under the previous brew's header. The user sees their most recent brew either missing from Shot History entirely, or labeled with the previous profile's name.What was wrong
ShotHistoryPlugin::startRecording()did not resetisFileOpenorheader. The cleanup branch inrecord()only runs on its 250 ms tick AND only whenrecording=false AND extendedRecording=false. With a BLE scale connected,extendedRecordingcan staytruefor up to 3 seconds aftercontroller:brew:end.The race:
endRecording()flipsrecording=false,extendedRecording=true. Post-drip sampling continues. File still open with A's header.Controller::activate()firescontroller:brew:clear→endExtendedRecording()flipsextendedRecording=false. Thendelay(200)(shorter than the 250 msrecord()interval, so the cleanup branch usually does NOT run). Thencontroller:brew:start→startRecording()flipsrecording=true.record()tick seesisFileOpen=trueso skips the file-open path. Brew B's samples get appended to A's file under A's header.sampleCount/durationMs. The index entry upsert at lines 622-628 overwrites the early-index entry (which had the correct "B" name) with the merged header.Net effect: B's brew appears in Shot History under A's name and A's timestamp / id; B's separately-named
.slogfile is never created.Compounded by a separate narrower race:
record()readsgetSelectedProfile()live on each tick to populate the header. If the user switches profile betweenbrew:startfiring and the firstrecord()tick, the wrong name lands in the header even without the file-inheritance race.Fix
Two coupled changes:
Extract the cleanup branch into
finalizeShotFile(), then call it defensively at the top ofstartRecording()whenisFileOpen=true. The previous shot gets properly patched, closed, and indexed before the new shot's state is initialized.Populate the shot file header synchronously in
startRecording()— magic / sample-size / startEpoch / profileId / profileName / phaseTransitionCount — using the runningBrewProcess's per-shot immutable profile copy (brewProcess->profile). Closes the live-profile-read race.record()'s file-open path no longer initializes the header;startRecording()guarantees it.The previously-dead
currentProfileNamefield is repurposed as a mirror of the header value for downstream telemetry consistency.User-visible impact
Before: rapid back-to-back brews (within ~3s with BLE scale connected, or ~250 ms without) caused the second brew to be lost or mislabeled in Shot History.
After: each brew gets its own correctly-named entry in Shot History regardless of how quickly the next brew starts.
Test plan
Summary by CodeRabbit