fix: sleep profile folds every derive pass instead of once per night - #174
Conversation
Real user export had "nights": 1348 against 12 days of data. The EWMA fold runs on every staging pass for a day — algo bumps, BLE drain re-derives, backfill sweeps — so the same handful of nights folded hundreds of times. Two things break. personalWeight saturates at 28 nights so it's pinned at the 0.5 cap from the first sweep, and the EWMA (alpha ~2/15) collapses onto whichever day happened to be re-derived last instead of a representative fortnight. Replayed that profile over the same 11 nights: wake 4.3% -> 36.4%, deep 1.9% -> 0.0% on the worst night. So the personalization layer was re-manufacturing the exact wake over-call cardioStager exists to avoid. Fixes: - fold at most once per day_id, tracked in the profile payload - don't apply the profile at all until 3 nights (van der Aar 2025 — gains need >=3 nights and ~17.5% of subjects get WORSE from personalization, so a 1-2 night profile is downside with no edge) - discard pre-tracking profiles. Can't repair them, there's no record of what went in, so they rebuild from cold start Watch out: gating the profile to null below the floor would make every fold restart from empty and pin nights at 1 forever. The fold uses the loaded profile regardless, only staging is withheld. Logic is in SleepProfilePolicy (pure, 15 tests) rather than inline in the engine. kAlgoVersion 51 -> 52. 1073 tests green.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe sleep-profile lifecycle now validates persisted payloads through ChangesSleep profile lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DerivationEngine
participant SleepProfilePolicy
participant LocalDb
DerivationEngine->>SleepProfilePolicy: Validate profile and check fold eligibility
SleepProfilePolicy-->>DerivationEngine: Return usable profile and fold decision
DerivationEngine->>LocalDb: updateBaseline with fold transformation
LocalDb->>DerivationEngine: Read latest profile inside exclusive transaction
DerivationEngine->>SleepProfilePolicy: Recheck day eligibility
SleepProfilePolicy-->>DerivationEngine: Return updated fold metadata
DerivationEngine->>LocalDb: Persist folded observation and folded day
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
PR Reviewer Guide 🔍(Review updated until commit 7d9c8e4)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 7d9c8e4 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit e029dfd
Suggestions up to commit 365c6f8
Suggestions up to commit 45f0939
Suggestions up to commit d1ccf16
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/compute/derivation_engine.dart (1)
1175-1263: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftSerialize the roll-in of sleep-profile folds.
runWithConcurrencylaunches up tomaxForegroundConcurrencylanes concurrently and allows any lane to read/await/write next, whileprocessDayreadssleep_user_profile, waits for an isolate, thenreplaces that shared baseline key. Under a multi-day sweep, two days can both read the same pre-write profile, decide to fold, and the later write clobbers the earlier fold. The only coverage added here testsSleepProfilePolicyidempotence in isolation, not this shared-key race.Accumulate fold decisions during the concurrent run and merge/persist one
sleep_user_profileupdate after the run completes, or otherwise make the folded-key update atomic and read-modify-write one fold at a time.🤖 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 `@lib/compute/derivation_engine.dart` around lines 1175 - 1263, Serialize updates to the shared sleep_user_profile baseline in the processDay fold path so concurrent days cannot overwrite each other’s folds. Preserve the existing per-day idempotence checks in SleepProfilePolicy, but accumulate fold results across the concurrent run and merge/persist them after the run, or use an atomic read-modify-write mechanism that applies one fold at a time instead of independently replacing the profile after _runIsolateCancellable.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.
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1175-1263: Serialize updates to the shared sleep_user_profile
baseline in the processDay fold path so concurrent days cannot overwrite each
other’s folds. Preserve the existing per-day idempotence checks in
SleepProfilePolicy, but accumulate fold results across the concurrent run and
merge/persist them after the run, or use an atomic read-modify-write mechanism
that applies one fold at a time instead of independently replacing the profile
after _runIsolateCancellable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c3ff2521-456f-44ca-b482-3e2e0cc0123e
📒 Files selected for processing (3)
lib/compute/derivation_engine.dartlib/compute/sleep_profile_policy.darttest/sleep_profile_policy_test.dart
CodeRabbit caught a real one. processDay runs up to _deriveConcurrency days at once in the foreground, and each day read the profile, awaited a staging isolate, then wrote it back. Two days read the same payload, both fold, later write clobbers the earlier — losing the fold AND its day_id, so that day re-folds next sweep and nights drifts up again. Same corruption the PR is fixing, just slower. Worker now returns the raw observation instead of a pre-folded profile, and the fold happens on the main isolate under SleepProfilePolicy.withProfileLock, which re-reads the profile and re-checks shouldFold inside the critical section. Lock is only held across the DB read-modify-write, not the isolate, so day concurrency is unaffected. 3 tests for it including the concurrent-clobber case. 18 policy tests, 1073 total.
|
Checked the one finding. It's real — fixed. the raceVerified the path is genuinely concurrent: So: two days read the same fixThe worker now returns the raw observation instead of a pre-folded profile — folding in the isolate would bake in whatever profile it read before staging started, which is stale by definition. The fold moved to the main isolate inside Went with the atomic read-modify-write rather than accumulate-and-merge-after-the-run: the lock is only held across the DB read/write, never across the isolate, so day-level concurrency is untouched, and it doesn't require restructuring the sweep. 3 tests, including one that models N days folding concurrently and asserts every 18 policy tests, 1073 total, analyze clean. 🤖 Generated with Claude Code |
|
Persistent review updated to latest commit a535b2d |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/compute/derivation_engine.dart (1)
1219-1219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the observation payload so the identifier matches its contents.
foldedJsonandupdatedProfileJsonnow carry a rawSleepNightObservation, not a folded profile. The names describe the previous contract. In a path whose whole purpose is to stop a stale profile from being written back, a name that still says "profile" invites exactly that mistake in a later edit. The parameter at line 1282 is already namedobservationJson, so the producer side is the only inconsistent part.♻️ Proposed rename
- String? foldedJson; + String? observationJson;- foldedJson = jsonEncode({ + observationJson = jsonEncode({- return (jsonEncode(candidate.toJson()), foldedJson); + return (jsonEncode(candidate.toJson()), observationJson);- final (candidateJson, updatedProfileJson) = + final (candidateJson, observationJson) = await _runIsolateCancellable(() {- if (updatedProfileJson != null) { - await _foldObservationIntoProfile(dayId, updatedProfileJson); + if (observationJson != null) { + await _foldObservationIntoProfile(dayId, observationJson); }Also applies to: 1240-1240, 1266-1268
🤖 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 `@lib/compute/derivation_engine.dart` at line 1219, Rename the observation payload variables foldedJson and updatedProfileJson to observationJson consistently throughout the relevant derivation flow, including their declarations and uses near the producer logic. Preserve the raw SleepNightObservation content and align the producer-side naming with the existing observationJson parameter.
🤖 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.
Inline comments:
In `@lib/compute/sleep_profile_policy.dart`:
- Around line 136-143: Replace the static withProfileLock/_lock serialization in
the derivation flow with database-backed coordination that works across
isolates. Ensure the read, fold, and write of the sleep_user_profile row occur
atomically inside a transaction or use a compare-and-set retry path, so
concurrent DerivationEngine passes preserve all folded_days entries.
In `@test/sleep_profile_policy_test.dart`:
- Around line 149-169: Update the test “a day already folded by another lane is
skipped, not double-counted” to start with an empty folded_days profile,
allowing both concurrent foldDay calls to initially observe the day as eligible.
Keep the concurrent Future.wait execution and assert that folds equals 1 after
both complete, so the test verifies withProfileLock prevents double-folding.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Line 1219: Rename the observation payload variables foldedJson and
updatedProfileJson to observationJson consistently throughout the relevant
derivation flow, including their declarations and uses near the producer logic.
Preserve the raw SleepNightObservation content and align the producer-side
naming with the existing observationJson parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 88af8b86-b2b7-4f26-8d35-41d0d00ce278
📒 Files selected for processing (3)
lib/compute/derivation_engine.dartlib/compute/sleep_profile_policy.darttest/sleep_profile_policy_test.dart
Two more from CodeRabbit, both right. 1. The Dart static lock I added doesn't serialize across ISOLATES. Verified: derivationDispatcher is a vm:entry-point WorkManager entry that builds its own DerivationEngine in a background isolate, so a static has one copy per isolate. A background heavy pass and a foreground sweep would each hold "the" lock and still clobber each other. Worse than nothing because it looked sufficient. Replaced with LocalDb.updateBaseline: read-modify-write inside ONE exclusive SQLite transaction. exclusive:true issues BEGIN IMMEDIATE so the write lock is taken up front rather than failing to upgrade under WAL. SQLite's lock is cross-connection, so it holds across isolates and processes. 2. My "already folded by another lane" test didn't test anything. It seeded the day into folded_days first, so both lanes skipped on their first read and folds==0 with or without the lock. Deleting the lock left it green. Replaced with tests that model the transaction body at commit time: two lanes folding the SAME day from an empty profile must commit exactly once, distinct days must all survive, a stale pre-staging read must not resurrect an already-folded day, and a legacy row rebuilds from 0 rather than 1348. Also renamed foldedJson/updatedProfileJson to observationJson — they carry a raw observation now, and a name saying "profile" in the one path built to avoid writing a stale profile is asking for it. 1077 tests, analyze clean.
|
Both right. Second one especially — thanks for catching it. the lock doesn't cross isolatesVerified before changing anything: Worse than nothing, really — it looked sufficient. Removed it and left a comment saying why, so nobody re-adds one. Replaced with the vacuous testYou're right and it's a bit embarrassing. Seeding Rewrote that group to model the transaction body at commit time, which is what the exclusive write lock actually guarantees:
renameDone — PR Agent's "lock not reset on throw"Moot now, the lock is gone. For the record it did release correctly ( 1077 tests, analyze clean. 🤖 Generated with Claude Code |
|
Persistent review updated to latest commit d1ccf16 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1266-1268: Wrap the await of _foldObservationIntoProfile in the
observationJson branch within local error handling so any database or isolate
failure is contained and does not escape _sleepCandidateForDay or mark the day
as failed. Keep the persisted sleep-session candidate and normal day-success
path unchanged; the day should remain eligible for a later fold attempt.
In `@test/sleep_profile_policy_test.dart`:
- Around line 117-126: Add direct regression tests that invoke
LocalDb.updateBaseline using the existing sqflite_common_ffi harness. Verify the
transform receives null for an absent key, null results preserve an existing row
byte-for-byte, and non-null results replace payload_json while advancing
updated_at.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05300a5b-56b6-4495-a87b-d2fb0004894b
📒 Files selected for processing (4)
lib/compute/derivation_engine.dartlib/compute/sleep_profile_policy.dartlib/data/db.darttest/sleep_profile_policy_test.dart
| group('concurrent-fold semantics (DB transaction contract)', () { | ||
| // The real serialization is an exclusive SQLite transaction in | ||
| // LocalDb.updateBaseline — a Dart lock cannot span isolates. What is | ||
| // testable here without a DB is the PURE contract the transaction body | ||
| // relies on: given the payload as it exists at commit time, decide once. | ||
| // | ||
| // These model the transaction body running serially (which is what the | ||
| // exclusive write lock guarantees) and assert the outcome is correct for | ||
| // any interleaving. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find an existing sqflite test harness and any updateBaseline coverage.
set -euo pipefail
echo '--- existing sqflite/ffi test setup ---'
rg -n -C3 --type=dart 'sqfliteFfiInit|databaseFactoryFfi|inMemoryDatabasePath|sqflite_common_ffi' test lib || true
echo '--- ffi dev dependency ---'
rg -n -C2 'sqflite' pubspec.yaml || true
echo '--- existing references to updateBaseline ---'
rg -n -C3 --type=dart '\bupdateBaseline\s*\(' . || trueRepository: OpenStrap/edge
Length of output: 35925
🏁 Script executed:
#!/bin/bash
# Inspect the implementation and call points for updateBaseline without executing repository code.
set -euo pipefail
echo '--- updateBaseline implementation ---'
sed -n '3900,3935p' lib/data/db.dart
echo '--- updateBaseline call site context ---'
sed -n '1300,1330p' lib/compute/derivation_engine.dart
echo '--- tests mentioning updateBaseline ---'
rg -n --type=dart '\bupdateBaseline\s*\(' test || true
echo '--- tests mentioning foldObservationIntoProfile ---'
rg -n --type=dart '_foldObservationIntoProfile|foldObservationIntoProfile|folded_days|fold' test/sleep_profile_policy_test.dart || trueRepository: OpenStrap/edge
Length of output: 6840
Add direct regression tests for LocalDb.updateBaseline.
test/sleep_profile_policy_test.dart covers the policy contract, but LocalDb.updateBaseline is the new cross-isolate synchronization primitive and has no tests calling it. Add tests for the deterministic database contract:
transformreceivesnullwhenkeyis absent.- A
nullreturn leaves the existing row byte-identical. - A non-null return replaces
payload_jsonand advancesupdated_at.
The suite can use the existing sqflite_common_ffi harness that other LocalDb tests already use.
🤖 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 `@test/sleep_profile_policy_test.dart` around lines 117 - 126, Add direct
regression tests that invoke LocalDb.updateBaseline using the existing
sqflite_common_ffi harness. Verify the transform receives null for an absent
key, null results preserve an existing row byte-for-byte, and non-null results
replace payload_json while advancing updated_at.
Source: Coding guidelines
Self-inflicted, from round 2. updateBaseline takes an exclusive SQLite write lock and the entire point is that two derivation isolates contend for it — so SQLITE_BUSY is an EXPECTED outcome. But it escaped _sleepCandidateForDay into processDay's broad catch, which calls _markDaySkipped and bumps failures. So a fully computed day (already persisted) gets thrown away, and the tz hold stays, because a bookkeeping write lost a lock race. _markDaySkipped is the same path that was a P0 for destroying good day_result rows. Wrapped in try/catch + log. Nothing is lost: the day_id never reaches folded_days, so the next pass folds it again. Also added test/db_update_baseline_test.dart — updateBaseline is the new cross-isolate primitive and had no direct coverage. transform sees null for an absent key, a null return leaves the row byte-identical (updated_at included), a non-null return replaces and advances updated_at, sequential accumulate, concurrent accumulate, concurrent set-union, and a throwing transform rolls back without wedging the connection. Checked these actually bite, given last round: swapped updateBaseline for a naive read-then-write and the concurrent tests lose 19 of 20 increments and 19 of 20 day_ids. Restored, all green. 1084 tests, analyze clean.
|
Both valid. The first one is a regression I introduced in the previous round, so thanks for staying on it. fold failure was failing the whole dayVerified the path: What makes it worse is that I caused it. Round 2 introduced an exclusive SQLite write lock specifically so two derivation isolates contend for it — so Wrapped in try/catch with a log line. Nothing is lost: the direct tests for updateBaselineFair — it's the new cross-isolate primitive and had no coverage of its own. Added
And I checked they actually bite, since last round I shipped a test that asserted nothing. Temporarily swapped 19 of 20 increments and 19 of 20 day_ids lost. Restored, all green. 1084 tests, analyze clean. 🤖 Generated with Claude Code |
|
Persistent review updated to latest commit 45f0939 |
Neither turned out to be a bug, but both pointed at genuinely untested corners, so they get tests rather than a reply saying "works on my machine". - concurrent LEGACY discard. The worry was that several lanes all observe the pre-tracking row at once, each treat it as a cold start, and each write a profile containing only its own day. Can't happen through updateBaseline — BEGIN IMMEDIATE serialises them, so only the first lane sees the legacy row and the rest see folded_days already present. 15 concurrent lanes over a nights:1348 row now assert nights==15 and no day_id lost. - override skip must not blacklist the day. Declining to fold an override records nothing, so removing the override later and re-deriving folds it normally. Pinning it because the tempting alternative — marking it folded to "remember we skipped" — would exclude that night forever. The third claim (syntax error in the test helper, "none of the 15 tests run") is wrong: `key: ?value` is null-aware element syntax, valid on our SDK 3.11.4, and the analyzer's own use_null_aware_elements lint is what asked for it. 19 tests in that file run and pass, analyze is clean, and test.yml triggers on pull_request so CI ran them too. 1086 tests, analyze clean.
|
PR Agent's persistent review flagged three things. Checked all three — none is a bug, but two pointed at untested corners so they get tests rather than a "works on my machine". "Syntax error in test helper" — incorrectThe claim is that
Evidence: Also "no CI gate runs on PRs" is wrong — "Legacy discard race" — not reachable, now coveredThe scenario: several lanes all observe the pre-tracking row simultaneously, each treats it as a cold start, each writes a profile containing only its own day, so the rebuild drops folds. It can't happen through Rather than argue it, added "Observation silently dropped on override path" — correct by design, now pinnedThe write-up says "this is correct behavior" three times and is partly self-contradictory (says override days are "permanently excluded", then correctly notes a later non-override re-derive would fold them). The second reading is right: skipping an override records nothing in But "not tested" was fair. Added 1086 tests, analyze clean. Note: CodeRabbit shows 🤖 Generated with Claude Code |
|
Persistent review updated to latest commit 365c6f8 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/compute/derivation_engine.dart (1)
1325-1333: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRe-check override state during the serialized fold.
_foldObservationIntoProfilepasseshasOverride: falseunconditionally. If a user adds an override after_sleepCandidateForDayreadsoverride == nullbut beforeupdateBaselinecommits, the observation is folded into the profile. Read the override row in the same transaction as the profile update, or make both row updates atomic.🤖 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 `@lib/compute/derivation_engine.dart` around lines 1325 - 1333, The serialized fold in _foldObservationIntoProfile must re-check the day’s override state inside the LocalDb.updateBaseline transaction instead of passing hasOverride: false from the earlier read. Read the override row using the transaction’s current state and pass whether an override exists to SleepProfilePolicy.shouldFold, ensuring a concurrently added override prevents folding; alternatively make the override and profile updates atomic.
🤖 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.
Inline comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1266-1281: The bookkeeping catch around
_foldObservationIntoProfile must preserve retryability when folding fails,
including SQLITE_BUSY and after an override is removed. Persist the pending
observation for dayId, or update _sleepCandidateForDay to bypass its
finalized-candidate cache until dayId is present in folded_days, so the next
pass reloads observationJson and retries the fold without changing the
successful day result path.
In `@test/db_update_baseline_test.dart`:
- Around line 1-10: The current contention tests only use concurrent futures
within one isolate and do not exercise cross-isolate database synchronization.
Extend the LocalDb.updateBaseline test suite with a regression case that spawns
a real second Isolate, opens the same sqflite_ffi database, and performs the
competing update alongside the test isolate; alternatively, document an existing
isolate-based test if one already covers this behavior.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 1325-1333: The serialized fold in _foldObservationIntoProfile must
re-check the day’s override state inside the LocalDb.updateBaseline transaction
instead of passing hasOverride: false from the earlier read. Read the override
row using the transaction’s current state and pass whether an override exists to
SleepProfilePolicy.shouldFold, ensuring a concurrently added override prevents
folding; alternatively make the override and profile updates atomic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 113bb88b-b54f-4f0d-bf6d-822b236c7408
📒 Files selected for processing (3)
lib/compute/derivation_engine.darttest/db_update_baseline_test.darttest/sleep_profile_policy_test.dart
| if (observationJson != null) { | ||
| // BEST-EFFORT, and deliberately isolated from the day's success path. | ||
| // The fold is bookkeeping; the day's real result is already persisted | ||
| // above. `updateBaseline` takes an exclusive SQLite write lock, and the | ||
| // whole point of this change is that two derivation isolates contend | ||
| // for it — so SQLITE_BUSY here is an EXPECTED outcome, not an | ||
| // exceptional one. Letting it escape would hit processDay's broad | ||
| // catch, which calls `_markDaySkipped` and increments `failures`, | ||
| // throwing away a fully computed day (and holding the timezone) over a | ||
| // bookkeeping write. Nothing is lost by swallowing it: the day_id never | ||
| // reaches `folded_days`, so the next pass simply folds it again. | ||
| try { | ||
| await _foldObservationIntoProfile(dayId, observationJson); | ||
| } catch (e) { | ||
| _log('sleep profile fold skipped for $dayId (day result kept): $e'); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not lose a fold when bookkeeping contention occurs.
After this catch, a day can be finalized without its day_id in folded_days. On the next pass, _sleepCandidateForDay returns the cached finalized candidate at Lines 1133-1150 before loading the profile or producing observationJson. _foldObservationIntoProfile is then never retried. The same path prevents folding after an override is removed. Persist a pending observation, or bypass the finalized-candidate cache until the day is recorded in folded_days.
🤖 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 `@lib/compute/derivation_engine.dart` around lines 1266 - 1281, The bookkeeping
catch around _foldObservationIntoProfile must preserve retryability when folding
fails, including SQLITE_BUSY and after an override is removed. Persist the
pending observation for dayId, or update _sleepCandidateForDay to bypass its
finalized-candidate cache until dayId is present in folded_days, so the next
pass reloads observationJson and retries the fold without changing the
successful day result path.
| // LocalDb.updateBaseline — the cross-isolate synchronization primitive behind | ||
| // the rolling sleep-profile fold, exercised against the REAL LocalDb over | ||
| // sqflite_ffi. | ||
| // | ||
| // Why this has its own suite: a Dart `static` mutex cannot serialize the fold, | ||
| // because derivation also runs in a background isolate (`derivationDispatcher` | ||
| // is a vm:entry-point WorkManager entry that builds its own DerivationEngine), | ||
| // and a static has one copy per isolate. The read-modify-write therefore has to | ||
| // be atomic in the DATABASE. `sleep_profile_policy_test.dart` covers the pure | ||
| // decision contract; these cover the storage contract it depends on. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how LocalDb opens the database and how the background isolate entry point accesses it.
ast-grep outline lib/data/db.dart --items all
rg -n "openDatabase|singleInstance|databaseFactory" lib/data/db.dart
rg -n -C6 "derivationDispatcher" lib/computeRepository: OpenStrap/edge
Length of output: 5566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'test/db_update_baseline_test.dart' 'lib/data/db.dart' 'lib/compute/background_derivation.dart' 'pubspec.yaml'
echo
echo "== db_update_baseline_test.dart outline and relevant lines =="
ast-grep outline test/db_update_baseline_test.dart --items all || true
wc -l test/testify db_update_baseline_test.dart lib/data/db.dart
sed -n '1,240p' test/db_update_baseline_test.dart
echo
echo "== lib/data/db.dart updateBaseline-related sections =="
rg -n -C8 "updateBaseline|exclusive|LocalDb|openDatabase|Database|singleInstance|sqflite|databaseFactory|sleep_profile" lib/data/db.dart test/db_update_baseline_test.dartRepository: OpenStrap/edge
Length of output: 519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'test/db_update_baseline_test.dart' 'lib/data/db.dart' 'lib/compute/background_derivation.dart' 'pubspec.yaml'
echo
echo "== db_update_baseline_test.dart outline and relevant lines =="
ast-grep outline test/db_update_baseline_test.dart --items all || true
wc -l test/test db_update_baseline_test.dart lib/data/db.dart
sed -n '1,240p' test/db_update_baseline_test.dart
echo
echo "== lib/data/db.dart updateBaseline-related sections =="
rg -n -C8 "updateBaseline|exclusive|LocalDb|openDatabase|Database|singleInstance|sqflite|databaseFactory|sleep_profile" lib/data/db.dart test/db_update_baseline_test.dartRepository: OpenStrap/edge
Length of output: 516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository root =="
pwd
echo "== top-level files =="
find . -maxdepth 3 -type f \( -name "pubspec.yaml" -o -path "./test/db_update_baseline_test.dart" -o -path "./lib/data/db.dart" -o -path "./lib/compute/background_derivation.dart" \) | sort
echo
echo "== db_update_baseline_test.dart lines 1-240 =="
sed -n '1,240p' test/db_update_baseline_test.dart
echo
echo "== updateBaseline and database opening references =="
rg -n -C6 "updateBaseline|exclusive|openDatabase|singleInstance|Database|sqflite|databaseFactory|background_derivation|derivationDispatcher|sleep_profile" lib/data/db.dart test/db_update_baseline_test.dart lib/compute/background_derivation.dart lib/compute/sleep_profile_policy.dart lib/compute/derivation_engine.dart pubspec.yamlRepository: OpenStrap/edge
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== public API and updateBaseline implementation =="
rg -n -C24 "static.*updateBaseline|Future<(.*)?>\s*updateBaseline|baseline\(|putBaseline\(|BEGIN IMMEDIATE|transaction\(|exclusive" lib/data/db.dart
echo
echo "== LocalDb public methods near top =="
sed -n '24,240p' lib/data/db.dart
echo
echo "== updateBaseline call sites in lib =="
rg -n -C8 "updateBaseline\(" lib/compute lib/dataRepository: OpenStrap/edge
Length of output: 50370
Exercise cross-isolate contention in db_update_baseline_test.dart.
The concurrent tests use Future.wait in one test isolate with a single FFI-derived sqflite_ffi Database, while the fix is a cross-isolate problem from derivationDispatcher. Add a regression test that spawns a real second Isolate opening the same DB and contends with the test isolate, or document an already-present isolate test.
🤖 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 `@test/db_update_baseline_test.dart` around lines 1 - 10, The current
contention tests only use concurrent futures within one isolate and do not
exercise cross-isolate database synchronization. Extend the
LocalDb.updateBaseline test suite with a regression case that spawns a real
second Isolate, opens the same sqflite_ffi database, and performs the competing
update alongside the test isolate; alternatively, document an existing
isolate-based test if one already covers this behavior.
Neither of these is a code fix. Both are cases where a comment or a test name promised something the code doesn't do. 1. My round-3 comment said a swallowed fold failure is harmless because "the next pass simply folds it again". That's false. _sleepCandidateForDay short-circuits on a cached finalized candidate before staging runs, so observationJson is never regenerated and the fold never retries. Same for a day whose override is removed if it already had a cached candidate. Documented as a KNOWN LIMITATION with the reasoning for accepting it: the profile is an EWMA over ~14 nights capped at 0.5 blend weight, so a missing night is a small perturbation, while a retry path needs durable pending state plus a way to tell "failed, retry" from "declined permanently" — a <120-epoch nap never folds and would otherwise bypass the candidate cache and re-stage every sweep forever. Naively bypassing the cache is the wrong fix and the comment says so. 2. My test "skipping an override does NOT blacklist the day forever" reads like an end-to-end guarantee. It isn't — it only asserts the policy keeps saying yes. The engine often won't re-fold, for the reason above. Renamed and scoped so nobody trusts it for more than it checks. Also scoped the db_update_baseline header: those concurrent tests run in one isolate on one connection, so they prove atomicity under interleaved async access (they do fail 19/20 against a naive read-then-write) but NOT cross-connection contention. The cross-isolate guarantee rests on SQLite's documented BEGIN IMMEDIATE locking, which they assume rather than verify. 1086 tests, analyze clean.
|
Both correct. Neither gets a code fix from me, and I want to be explicit about why rather than quietly closing them. the fold-retry gap — you're right, and my comment was wrongVerified: I have not built the retry. Reasoning, so it can be argued with:
So: documented as a KNOWN LIMITATION at the call site, with the reasoning and an explicit "do not simply bypass the cache" note for whoever picks it up. Happy to build the retry if you'd rather — say the word. the override test was overclaimingRelated, and my fault. That's twice now in this PR that I've shipped a test whose name promised more than it checked. cross-isolate coverageFair. The concurrent tests run in one isolate on one I didn't add the spawned-isolate test: 1086 tests, analyze clean. 🤖 Generated with Claude Code |
|
Persistent review updated to latest commit e029dfd |
PR Agent point, narrow but fair. appendFoldedDay evicts by LEXICOGRAPHIC order, which only equals chronological order for zero-padded ISO dates. Feed it an epoch string or a UUID and a RECENT day could be evicted while an older one is kept — and an evicted day passes shouldFold again, which is the double-fold this class exists to prevent. Invisible in the payload if it ever happened. Only day_label.dart values reach this today, so it's a precondition rather than a bug. Asserted it and documented why the sort is load-bearing. The assert immediately failed my own cap test, which was building fake labels like '2020-01-00000'. Rewrote it to walk real dates, and added coverage for eviction across a month and year boundary plus the non-date rejection. Two other PR Agent focus areas need no change: - "Fold skipped permanently on SQLITE_BUSY" is the same gap CodeRabbit raised; already documented as a KNOWN LIMITATION at the call site with the reasoning. - "Stale mayFold pre-check" — the substantive half was whether a drained observation accumulator could starve a later day in the same isolate. It can't: _runIsolateCancellable does Isolate.spawn per call with onExit wired, so it's one fresh isolate per day and the globals die with it. The rest is a few serialized doubles occasionally declined by the transaction re-check, which is the correct design — mayFold can go stale during 90s of staging no matter how fresh the pre-check is. 1088 tests, analyze clean.
|
Three focus areas in the updated persistent review. One acted on, two need no change. "Cap evicts by sort order, not insertion age" — fair, assertedNarrow but correct. Only Nice side effect: the assert immediately failed my own cap test, which was generating fake labels like "Fold skipped permanently on SQLITE_BUSY" — already documentedSame gap CodeRabbit raised in the previous round. Confirmed real, deliberately not fixed, documented as a KNOWN LIMITATION at the call site with the reasoning (EWMA over ~14 nights at a 0.5 cap, and the naive fix — bypassing the candidate cache — would make a <120-epoch nap re-stage every sweep forever). Details in my earlier reply. "Stale mayFold pre-check" — not reachableThe substantive half of this was: "if that drain has side effects (clearing the accumulator), a subsequent legitimate fold attempt for a different day in the same isolate lifetime could see an empty observation list. Whether the isolate is reused across days in the same sweep determines whether this matters." Checked — it isn't reused. The remainder is a handful of serialized doubles that the transaction re-check occasionally declines. That's the intended design rather than waste: 1088 tests, analyze clean. 🤖 Generated with Claude Code |
|
Persistent review updated to latest commit 7d9c8e4 |
|
Persistent review is down to two focus areas on On the current code suggestions — nothing actionable, but for the record: "Warm-up gate driven by tracked distinct nights, not EWMA counter" (importance 3). The stated divergence doesn't occur: a skipped fold increments neither "Ensure exclusive write lock is released on transform exception". The suggested diff is a semantic no-op ( "Fix invalid null-aware map value syntax" (importance 7). Repeat of the earlier claim. Still incorrect — "Remove redundant expect that fires before skip takes effect" — this one targets 1088 tests, analyze clean. Nothing further from me unless a human reviewer wants the fold-retry state machine built. 🤖 Generated with Claude Code |
1 participant
User description
A real user export had
"nights": 1348insleep_user_profile. They have 12 days of data.The EWMA fold runs on every staging pass for a day — algo bumps, BLE drain re-derives, backfill sweeps — so the same handful of nights folded hundreds of times. Two things break:
personalWeightsaturates at 28 nights, so it's pinned at the 0.5 cap from the first sweepReplayed that profile over the same 11 nights. Worst night: wake 4.3% -> 36.4%, deep 1.9% -> 0.0%. So the personalization layer was re-creating the exact wake over-call cardioStager exists to avoid. Same bug class as #108, different table.
fix
day_id, tracked in the profile payload (no migration —baselinesis free-form JSON)gotcha
Gating the profile to null below the floor would make every fold restart from empty and pin
nightsat 1 forever. The fold uses the loaded profile regardless; only staging is withheld. Nearly shipped that.Logic went into
SleepProfilePolicy(pure, 15 tests) instead of inline in the engine. One test simulates 12 days x 100 re-derives and asserts exactly 12 folds.kAlgoVersion 51 -> 52 so days re-stage without the corrupt blend.
1073 tests green, analyze clean.
Separate from OpenStrap/analytics#34 which fixes the staging rules themselves — that one'll need another bump when we repin.
🤖 Generated with Claude Code
PR Type
Bug fix, Tests
Description
Fix sleep profile EWMA folding hundreds of times per night instead of once
nights: 1348against only 12 days of dataAdd per-
day_idfold tracking in profile payload; discard legacy profiles lacking itGate profile influence on staging until ≥3 nights (van der Aar 2025 warm-up floor)
kAlgoVersionbumped 51→52 so all days re-stage without the corrupt blendDiagram Walkthrough
flowchart LR A["BLE drain / algo bump\n/ backfill re-derive"] -- "every pass" --> B["_loadSleepUserProfileJson()"] B -- "usableProfileJson()\ndiscards legacy" --> C["SleepProfilePolicy"] C -- "foldedDays()" --> D["Set of already-folded day_ids"] D -- "shouldFold()" --> E{"day_id\nalready folded?"} E -- "yes → skip" --> F["No EWMA step\n(idempotent)"] E -- "no → fold once" --> G["base.fold(obs)\n+ withFoldedDays()"] G --> H["Persist updated profile\nwith folded_days stamp"] C -- "shouldBlend(nights)" --> I{"nights >= 3?"} I -- "no" --> J["cardioUserProfile = null\n(cold-start staging)"] I -- "yes" --> K["cardioUserProfile = loaded\n(personalized staging)"]File Walkthrough
sleep_profile_policy.dart
New pure SleepProfilePolicy enforcing fold idempotency and warm-upgatelib/compute/sleep_profile_policy.dart
SleepProfilePolicywith no Flutter/DBdependencies
shouldFold()gates folding to once perday_idusing afolded_dayssetin the payload
shouldBlend()withholds the profile from staging until ≥3 nights areaccumulated
usableProfileJson()/isLegacy()discard pre-tracking profiles thatlack
folded_daysappendFoldedDay()/withFoldedDays()maintain the sorted, capped(400-entry) fold ledger
derivation_engine.dart
Wire SleepProfilePolicy into derivation engine; bump kAlgoVersion to52lib/compute/derivation_engine.dart
kAlgoVersionbumped from 51 to 52 with detailed changelog entry_loadSleepUserProfileJson()now delegates toSleepProfilePolicy.usableProfileJson(), discarding legacy profilesloadedprofile (used for folding) fromcardioUserProfile(gated byshouldBlend)mayFold(SleepProfilePolicy.shouldFold());folded result stamped with
withFoldedDays()sleep_profile_policy_test.dart
15 unit tests for SleepProfilePolicy covering all policy branchestest/sleep_profile_policy_test.dart
legacy-payload discard, and bookkeeping
exactly 12 folds occur
null/corrupt payload handling
Summary by CodeRabbit