fix(flex): five-minute rows must not leave stale seconds behind - #1699
Merged
Conversation
The flex balance is stored twice: decimal hours and seconds. One-minute rows write all four columns; five-minute rows write only the decimals and leave SumFlexStartInSeconds/SumFlexEndInSeconds as they were. That is deliberate, and harmless while those columns are 0. It is not harmless when they hold a stale non-zero value. The chain seed is `seconds != 0 ? seconds : Round(hours * 3600)`, which treats non-zero as trustworthy, so a five-minute row carrying a leftover poisons its successor's seed. Seen in production on tenant 994 site 21445: predecessor dated 2026-08-27 with SumFlexEnd -3.97 and SumFlexEndInSeconds -290456; the next row opened at -80.68 h instead of -3.97 h. A 76.71-hour error on a live site. Note that predecessor's DATE is after the site's effective date -- it resolves five-minute via its write-time marker. Any fix keyed on dates alone misses it. Two halves, both needed: - ApplyNettoFlexChainDecimal writes the decimals and clears the seconds in one call, so a call site cannot do one without the other -- the same trick the 2-arg ApplyNettoFlexChainSecondPrecision overload uses on the one-minute side. ClearSumFlexSeconds covers legs that must keep their own formula. - The seed ignores a predecessor's seconds column entirely when that predecessor resolves five-minute, using the full precedence (marker, then effective date, then the audit timeline). This matters beyond our own writes: the background service writes decimals and never touches the seconds columns at all, so rows from that path stay exposed no matter what this plugin does. The clear is mode-gated everywhere, including the two legacy bulk paths (GoogleSheet pull, Excel import). Those legs run unscoped by site name -- the sheet pull on every global settings save -- and compute their decimal with a known inverted sign, so clearing a one-minute row's seconds there would have handed every reader the wrong value. They now build one timeline per site before their row loop and clear only five-minute rows; an unresolvable site leaves the row alone. Deliberately unchanged: NettoHoursOverride handling at five sites that never consulted it, and the inverted sign in the GoogleSheet and Import legs. Both are marked in the code -- grep INTENTIONAL DIVERGENCE and INVERTED-SUMFLEX-SIGN. Neither belongs in a fix about which columns get written. No decimal SumFlexStart/SumFlexEnd/Flex value changes, at any call site, for any input -- verified per site including floating-point association -- except where a five-minute predecessor's stale seconds were previously used as the seed. That change is the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k
There was a problem hiding this comment.
🟡 Changes recommended
The GoogleSheet and Excel import mode-gated clearing can still clear seconds when AssignedSite is missing (mode effectively unknown), which risks corrupting one-minute rows in the known inverted-sign legs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a flex-chain boundary bug where five-minute (decimal) rows could retain stale SumFlex*InSeconds values, causing subsequent one-minute rows to seed from incorrect seconds and display/compute large balance errors.
Changes:
- Introduces
PlanRegistrationHelper.ApplyNettoFlexChainDecimal+ClearSumFlexSecondsand routes five-minute write paths through it to ensure seconds columns are cleared on decimal-mode writes. - Makes seeding mode-aware via
SumFlexEndSecondsWithFallback(pre, preIsOneMinute)and addsOneMinuteModeTimeline.WasOneMinuteForRow/WasOneMinuteForhelpers to centralize marker→effective-date→timeline precedence. - Updates multiple services (working hours, planning, flex, import/GoogleSheet) to use the new helpers and extends tests to cover the production incident and the new invariants.
File summaries
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs | Uses mode-aware seeding for DTOs, clears seconds for five-minute DTO rows, threads timelines into update paths, and gates import clearing by mode. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningPlanningService/TimePlanningPlanningService.cs | Replaces ad-hoc five-minute flex-chain math with shared decimal helper and adds timeline reuse for predecessor/forward cascade. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningFlexService/TimePlanningFlexService.cs | Avoids reading stale seconds on five-minute rows and clears SumFlex seconds when adjusting paid-out flex in five-minute mode. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PlanRegistrationHelper.cs | Adds mode-aware seed fallback, ClearSumFlexSeconds, and ApplyNettoFlexChainDecimal; threads mode hints into second-precision chaining. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/OneMinuteModeTimeline.cs | Adds in-memory row-mode resolution helpers and a nullable async resolver for optional predecessor rows. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/GoogleSheetHelper.cs | Builds per-site timelines once and gates seconds clearing to five-minute rows (and should avoid clearing when mode is not resolvable). |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/RunningFlexChainModeBoundaryTests.cs | Updates expectations to “five-minute clears seconds” and adds a regression reproducing the stale-seconds incident. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanRegistrationHelperTests.cs | Adds unit coverage for the new decimal helper, seconds-clearing invariant, and mixed-mode boundary behavior. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/OneMinuteIntervalsEffectiveDateTests.cs | Extends effective-date/timeline tests to cover mode-aware seed fallback and marker precedence scenarios. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+252
to
+257
| var mappedAssignedSite = await dbContext.AssignedSites | ||
| .AsNoTracking() | ||
| .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) | ||
| .FirstOrDefaultAsync(x => x.SiteId == mappedSiteUid); | ||
| oneMinuteTimelines[mappedSiteUid] = | ||
| await OneMinuteModeTimeline.BuildAsync(dbContext, mappedAssignedSite); |
Comment on lines
+4080
to
+4083
| if (!importTimeline.WasOneMinuteForRow(planRegistration)) | ||
| { | ||
| PlanRegistrationHelper.ClearSumFlexSeconds(planRegistration); | ||
| } |
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.
The bug
The flex balance is stored twice — decimal hours and seconds. One-minute rows write all four columns. Five-minute rows write only the decimals and leave
SumFlexStartInSeconds/SumFlexEndInSecondsas they were. Deliberate, and harmless while those columns are0.Not harmless when they hold a stale non-zero value. The chain seed is
seconds != 0 ? seconds : Round(hours * 3600)— it treats non-zero as trustworthy. So a five-minute row carrying a leftover poisons its successor's seed.Production, tenant 994 site 21445:
A 76.71-hour error on a live, non-resigned site. Note the predecessor's date is after the site's effective date — it resolves five-minute via its marker, so a fix keyed on dates alone misses it.
The fix — two halves, both needed
ApplyNettoFlexChainDecimalwrites the decimals and clears the seconds in one call, so no call site can do one without the other. Same trick the 2-argApplyNettoFlexChainSecondPrecisionoverload already uses on the one-minute side.ClearSumFlexSecondscovers legs that must keep their own formula.The seed ignores a predecessor's seconds column entirely when that predecessor resolves five-minute, under the full precedence: write-time marker → effective date → audit timeline. This matters beyond our own writes — the background service writes decimals and never touches the seconds columns at all, so rows from that path stay exposed regardless.
The gate that a review round caught
Two legacy legs — GoogleSheet pull and Excel import — initially cleared unconditionally. Both run unscoped by site name, the sheet pull on every global settings save, and both compute their decimal with a known inverted sign. Clearing a one-minute row's seconds there would have handed every reader the wrong value: the same corruption class this PR fixes, self-inflicted, estate-wide.
Now both build one timeline per site before their row loop (no N+1) and clear only five-minute rows. An unresolvable site leaves the row alone — unresolvable ⇒ no clear.
Deliberately unchanged
NettoHoursOverridehandling at five sites that never consulted itBoth marked in code:
grep INTENTIONAL DIVERGENCEandgrep INVERTED-SUMFLEX-SIGN. Neither belongs in a fix about which columns get written.Equivalence
No decimal
SumFlexStart/SumFlexEnd/Flexvalue changes, at any call site, for any input — verified per site including floating-point association, not just formula — except where a five-minute predecessor's stale seconds were previously used as the seed. That change is the fix.Tests
Three existing fixtures extended, no new class (shard filters already correct). Includes the site-21445 incident reproduced verbatim, and
ClearSumFlexSeconds_ZeroesBothColumnsAndTouchesNothingElsepinning thatPaiedOutFlexInSecondsis not collateral — it is maintained on five-minute rows.One assertion inverted:
RunningFlexChainModeBoundaryTests.UniformlyFiveMinute_…previously asserted a sentinel in the seconds columns was preserved. That assertion pinned the bug. Its six decimal assertions are unchanged and still pass, which is the evidence that only the seconds semantics moved.🤖 Generated with Claude Code
https://claude.ai/code/session_01TgEyDcnAEBcCF63RX2vm1k