Repository navigation
fix(session-load): one sample row whose timestamp is not a date stops the session opening — or plots it 27,000 years wide (#237) - #240
Conversation
…237) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…stead of failing the load (#237) `Samples.TimestampTicks` is a plain SQLite INTEGER with no CHECK, so it can hold anything a long can, while `new DateTime(long)` accepts only 0..DateTime.MaxValue.Ticks. `SessionDataRepository` converted the value at two sites without checking it, and a saved session holding one such row would not open. Running the new tests against the unchanged code turned up a second failure mode the issue had ruled out. Both sites convert only the session MINIMUM, so which failure you get depends on which end the damaged row sits at: a negative row IS the minimum and throws, but an above-range row is not, so nothing throws — it is quietly plotted ~27,000 years from the origin, which stretches the X axis until the real data is a vertical line at zero. Both are fixed by excluding unreadable rows from the queries the session is read through, so the damaged row is gone before anything converts or plots it. That also puts the exclusion ahead of channel discovery, which keys off the first timestamp: a damaged row sorting below the real data would otherwise BE that timestamp, and only its own channel would be discovered. Rows are skipped, not clamped — a clamped origin would shift every delta-time on the plot — and one warning per load names the session and the offending tick bounds. Also folds the file's four repeated ADO.NET parameter blocks into one helper, which is what keeps the change net-negative outside the tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoSkip invalid timestamps when loading saved sessions
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…name the boundary theory for what it covers Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w back as a channel extreme (#237) Qodo review round 1. Skipping unreadable rows from the two loads was not enough on its own: both viewers have a third path. When a session is past INITIAL_LOAD_POINTS and LoadSampledData finds no time range to sample across, they fall back to LoadSingleTickValueSpread, which draws each channel as a MIN..MAX vertical segment aggregated over the whole session — keyed on the session id alone. A damaged row's VALUE was therefore still the segment's extreme: skipped on one path, drawn on another. This change is also what made that path more reachable, which is why the fix belongs here rather than in a follow-up. Excluding unreadable rows from the bounds is exactly what makes a damaged session's readable rows share one timestamp, and therefore what sends the caller down that branch. The regression test exercises the whole chain — over the cap, no time range, then the fallback — rather than calling the fallback directly, since "the fallback is reached" is half the claim. Its 100,001 rows go in with one recursive-CTE insert, so it still runs in about a second. Verified failing with only the new filter reverted: the spread came back with two points, 1.0 and 999999.0, instead of one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd-1 test was added (13/17 -> 14/18) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit bce7240 |
|
Qodo-clean, CI green — ready for review at Shepherd pass. I pushed nothing, so the head is still the SHA both Qodo and CI actually ran on. The verification behind this note is CI's on QodoRound 1 (up to Re-derived rather than taken on trustEvery figure below I measured myself; none is quoted from the description.
The skip is the part worth arguing about, so I checked it rather than the diff
The collapsed graph, rendered instead of asserted"The real data collapses to a line at x=0" is the one claim a reviewer cannot check from the diff, so I staged it. Both heads booted headless through Before — unchanged
After — this head. Same database, same seeded damage; the damaged row is skipped and the session reads normally across 0–2000 ms:
One incidental thing the render turned up, cosmetic and not blocking: the header and session card still read SAMPLES 401 on the fixed head while 400 points are drawn. That number is Not merging. |
Not merging — for review.
closes #237
What a user hits
You open a saved session and it does not open. The session viewer throws
ArgumentOutOfRangeExceptionand the whole load is abandoned — every channel, everysample — because one row in that session holds a timestamp value that is not a date.
Samples.TimestampTicksis a plain SQLiteINTEGERwith noCHECK, so it holdsanything a
longcan, whilenew DateTime(long)accepts only0 .. DateTime.MaxValue.Ticks.SessionDataRepositoryconverted the stored value attwo sites without checking it —
LoadInitialSession(Phase 1) andLoadSampledData(Phase 2).
Reachability is the argument #231 makes for
Samples.Colorand no stronger: nocurrent write path can produce such a value, because every producer starts from a
DateTime. The exposure is a database file that predates the current schema —SQLite does not re-validate rows already sitting in an existing file, and both apps
moved to SQLite in 2025-08, so genuinely older stores exist. This is hardening on the
same footing as #231, not a live crash report.
A_session_database_can_hold_a_tick_value_no_date_can_representbuilds such a file rather than assuming one is possible.
The failure mode the issue ruled out, which is real
Running the new tests against unchanged
mainfirst turned up a second outcome theissue explicitly denied ("there is no silent-wrong-render variant —
new DateTimeeither succeeds or throws"). There is one, and which one you get depends on which
end of the range the damaged row sits at, because both sites convert the session's
MINIMUM tick value and nothing else:
ArgumentOutOfRangeException, session will not openlong.MaxValue)858434381285477.6ms — about 27,000 years — from the originThe second one is worse for being quiet: it stretches the X axis so far that the real
data collapses to a single vertical line at zero. The session "loads", and is
unreadable.
A_sample_above_the_range_is_silently_plotted_27000_years_outpins it, andit is the reason the remedy below is skip rather than clamp.
What happens to the bad row now — please read this bit
It is skipped. The sample is dropped from the graph, from the point lists, from
TotalSampleCount, and from the single-timestamp value spread. That is a data-lossdecision, so it is not silent: one
_appLogger.Warningper load names the session and the offending tick bounds, e.g.Skipping was chosen over clamping deliberately. The plot's X axis is a delta from the
session's first timestamp, so clamping the origin would shift every point on the
graph — trading a load that fails loudly for one that succeeds and is wrong, which is
exactly the second row of the table above. A healthy session is never warned about
(
A_healthy_session_is_not_warned_about).How
One shared range, applied where the rows enter the load rather than where they are
converted — a
readableQueryin Phase 1, two extra SQL predicates in Phase 2, and thesame predicate on the single-timestamp fallback (see the round-1 section below). Two
sites throwing the same way wanted one guard, not two try/catches. Filtering at the
query rather than at the conversion also settles three things a
try/catcharoundnew DateTimewould have left open:sorting below the real data would be that timestamp — and only the channel on that
row would be discovered. Every other channel's samples are then dropped, because a
point list is seeded only for the channels found there. Losing three channels out of
four is a data loss the crash was hiding
(
A_damaged_row_does_not_shrink_the_discovered_channel_set).INITIAL_LOAD_POINTStodecide whether the expensive full-range load runs; rows that will never be drawn must
not drive that decision.
minTicks + i * tickStep, the overflow the issue notes at the end. With bothoperands now bounded by
DateTime.MaxValue.Ticksthe sum cannot leavelong, so itis gone by construction rather than by a second guard.
Detecting the damage costs one extra indexed row-read on a healthy load: the session's
tick bounds are two ordered
LIMIT 1reads offIX_Samples_LoggingSessionID_TimestampTicks(the same "instant via composite index" idiom the first-timestamp read here has always
used), not a second
COUNT, which would have added a whole index pass to every healthyload to detect a row almost no load has. Only a damaged session pays for a third read.
Samples.TimestampTicksis now the only thing filtered — no config knob, no lenientmode, no validation layer.
A third path, found in Qodo round 1
Skipping the row from the two loads was not enough, and the reviewer was right to
push on it. Both viewers have a third path: when a session is past
INITIAL_LOAD_POINTSandLoadSampledDatafinds no time range to sample across, theyfall back to
LoadSingleTickValueSpread, which draws each channel as aMIN..MAXvertical segment aggregated over the whole session — and that aggregation was keyed on
the session id alone. The damaged row's value was therefore still the segment's
extreme: skipped on one path and drawn on another.
This change is also what made that path more reachable, which is why the fix belongs
here rather than in a follow-up — excluding unreadable rows from the bounds is exactly
what makes a damaged session's readable rows share one timestamp, and therefore what
sends the caller down that branch.
The_single_timestamp_fallback_does_not_bring_a_skipped_row_back_as_a_channel_extremewalks the whole chain rather than calling the fallback directly, because "the fallback
is reached" is half the claim: it asserts the count is over the cap, asserts
LoadSampledDatareturns null, and only then checks the spread. Its 100,001 rows go inwith one recursive-CTE insert, so it still runs in about a second. Verified failing with
only the new filter reverted — the spread came back with two points,
1.0and999999.0, instead of one.Relationship to #233 (open, unmerged)
Same convention, deliberately different remedy, and no overlap in files.
#233 introduced
ChannelSeriesColor.ParseOrFallback— one place that normalises adistrusted stored value so a damaged row cannot abort the session load. This follows
that shape: one place that decides what an untrustworthy stored value becomes, applied
at every site on the path, never throwing. Where it differs is the remedy, and the
difference is in the data, not in taste: a fallback colour is cosmetically wrong and
harmless, whereas a fallback timestamp moves the plot origin and shifts every point.
So colour falls back and timestamps are skipped. #233's guard is also silent because it
runs per-sample on the live path; this one warns because it runs once per session load.
git mergeof2204306onto this branch is clean — fix(session): a legacy row's missing channel colour aborts the session load — or silently erases the channel #233 touchesChannelSeriesColor.cs,PlotLogger.cs,PlotModelFactory.cs; this touchesSessionDataRepository.cs, which fix(session): a legacy row's missing channel colour aborts the session load — or silently erases the channel #233 never opens.merged state runs 503 tests, 0 failures (this branch's 485 plus fix(session): a legacy row's missing channel colour aborts the session load — or silently erases the channel #233's 18).
Deliberately out of scope
DataSample.cs:84—IDataSample.Timestamp => new(TimestampTicks), the thirdsite the issue names. It is an explicit interface implementation with no in-repo
caller (
grep IDataSamplefinds only the declaration), and it is not on the sessionload path. Guarding it now would be speculative.
PlotLogger.cs:274—FirstTime ??= new DateTime(dataSample.TimestampTicks)hasthe same shape but sits on the live stream path: both
ILogger.Logcall sites inLoggingManagerare behind aCurrentMode != LoggingMode.Streamguard, so it onlyever sees a sample the device just produced, whose ticks came from a
DateTime. It isalso in a file fix(session): a legacy row's missing channel colour aborts the session load — or silently erases the channel #233 is currently rewriting.
Verification
origin/main— 10 throwingArgumentOutOfRangeExceptionout of the load (7 atSessionDataRepository.cs:184,3 at
:268), and 4 failing on an assertion having thrown nothing at all: the silentvariant. Measured by checking the merge base out, applying only the final test file to
it and running it — re-measured after the round-1 test was added, since adding a
test moves the denominator. The 4 that pass unfixed are meant to: the reachability
demonstration, the no-false-alarm guard, and the two range edges.
SessionDataRepositoryhad no testfile at all before this; that absence is itself part of the finding.
DateTime.MinValue) and so isDateTime.MaxValue.Ticks.Both are kept, which is why the guard is a range test and not a "is it positive?"
test —
The_edges_of_the_representable_range_are_keptcovers each.dotnet build Daqifi.Avalonia.Desktop— green.dotnet restore+dotnet build Daqifi.Avalonia.iOS -c Release -r iossimulator-arm64 --no-restore— green.dotnet test— 485 passed, 0 failed (18 of them new, so 467 pre-existing).than a quoted constant, never against a remembered number: clean
--no-incrementalbuilds of both heads on both branches in the same worktree, warning lines normalised
(
third_party/and SDK-pack warnings excluded, MSBuild node prefixes stripped) and theresulting sets diffed. Desktop 126 → 126 and iOS Release 126 → 126 unique first-party
warnings, with an empty added set and an empty removed set on each. Those are unique
path + code + messagetuples; the raw build lines the two heads print (193 desktop,194 iOS) count the same warning once per build pass. Diffing with line numbers kept
shows exactly one difference across both heads: a pre-existing
CS8604in this filemoves from line 164 to 232 — the nullable
Colorthat Session load throws on a sample row with a null or blank Color #231 is about.packages.lock.jsonunchanged; no-rreached restore.Size
SessionDataRepository.csis+155 / −51, but only +19 of that is code: +79 net iscomment and XML doc, +6 net is blank lines. The four repeated ADO.NET
CreateParameter/ParameterName/Value/Addblocks are folded into oneAddParameterhelper, which is what pays for the guard; the rest of the diff is the 473-line test file
that did not exist.
SessionDataRepositoryhad no test file at all before this.🤖 Generated with Claude Code