Repository navigation
fix(sd-import): a failed import leaves a half-session that looks complete, and an empty one is deleted at next launch - #200
Conversation
Importing a truncated or garbage .bin from an SD card showed "Import Failed", which was correct, and said nothing about the data it had left behind. The import was never atomic: CreateSession commits the Sessions row before a single sample is parsed, every 1,000-sample batch commits in its own transaction, and the parse loop had no try/catch at all. So a parser that gave up part way through left a committed session plus however many batches had landed -- and because startup reloads whatever has samples, that fragment came back at the next launch as an ordinary, complete-looking session with silently truncated data. Nothing crashed; the database simply said something untrue. Sessions now carry a Status (new column, defaulting every existing row to Complete). The importer writes Importing before it parses anything, and on the way out writes Complete, or ImportFailed if the parse threw or was cancelled after samples had landed. Those samples are KEPT -- the source log is usually damaged in a way a re-import cannot get past, so discarding the prefix would be the one outcome the user cannot undo -- and both session lists show the row with an amber INCOMPLETE IMPORT chip, tooltip and screen-reader summary instead of presenting it as finished. A failure before the first batch landed keeps nothing: there is no data to preserve and no marker worth showing, so the row is removed and the database goes back to how it was. The status column also covers the case no catch block can: a process killed mid-import leaves the row saying Importing, and startup flags it. Second half of the ticket, the zero-sample session. The importer deliberately treated a 0-byte log as a legitimate empty session and reported success, while LoadPersistedLoggingSessions deleted zero-sample rows at the next launch without a word -- two components disagreeing about whether such a session exists. They now agree: a persisted row always either has samples or is being written. An empty log persists nothing and the dialog says "contained no samples, so no session was created" rather than claiming an import that vanishes overnight; Import All records it as a skip instead of counting it. The purge keeps its actual job -- deleting the empty rows an abandoned logging session leaves -- and gains an exemption for rows an import is writing, which is the hazard GetLoggingSessionsSnapshot was added to dodge. Also: the importer allocated MAX(Sessions.ID) + 1, the narrow version LoggingManager's own comment warns against, because it can hand out an ID orphan rows still reference. Both now go through one SessionIdAllocator. Two smaller changes the above needed. The final in-memory SampleCount write now Posts to the UI thread rather than Invoke-ing: nothing waits on it, and blocking the importing thread on a dispatcher loop that may not be running is a hang -- it is what made the import untestable. And ImportFromDeviceAsync no longer deletes the file from the device after a log that produced no samples. Tests: 10 new (267 total, was 257). Seven of the ten fail against the old behaviour, verified by neutering the fix and re-running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoMake SD card imports recoverable and visibly incomplete
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…ort what it actually did All four Qodo round-1 findings. 1. Overwrite loses the existing session (High) — conceded, and the sharpest of the four. CreateSession deleted the session an OverwriteExistingSession import replaces BEFORE parsing anything, so the new "remove the row when the import produced nothing" paths finished the job: an empty or immediately failing overwrite destroyed the user's existing session and put nothing in its place. The replacement is now imported alongside the original, and the original is dropped only once a complete replacement exists — the same "delete only after success" discipline as the neighbouring PRs in this family. Three tests, all failing before the change. 2. Finalization failure reports success (High) — conceded. FinalizeSession swallowed a missing row and every database error, and SessionPersisted was derived from the sample count, so an import whose status write never landed still reported success: the device path would delete the source file and the caller would list a session the next launch flags as incomplete. It is now a single conditional UPDATE whose row count says whether it happened, and SessionPersisted is an explicit result field set from that. 3. Cleanup failure leaves a hidden row (Medium) — conceded. RemoveSessions returns whether the delete committed, and the user is only told no session was created when it did. The result carries OutcomeGuidance, one sentence written where the outcome is known and read by all three import call sites, so an empty log, an uncleanable leftover and an unrecorded session each say the right thing and the three dialogs cannot drift (the pattern DatabaseMigrator.DescribeQuarantineForUser set). 4. Startup relabels live imports (Low) — conceded in part. MarkAbandonedImportsFailed was a read followed by a save of tracked entities, so an import finishing in another instance between the two had its Complete overwritten by a stale ImportFailed. It is now one conditional UPDATE, which closes that window entirely. The remainder of the finding — an ownership token or lease so recovery only claims expired imports — is rebutted on the record set by #181: the app has no cross-process database ownership guard of any kind, introducing one is an app-wide mechanism rather than part of this fix, and Qodo's own relevance rating on that half is Weak. Tests: 13 (was 10); 270 total, was 257 on main. Desktop and iOS Release builds green, no packages.lock.json drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 1 triaged; all four findings addressed in Conceded in part, and the part that holds is now fixed. Rebutted: the ownership token / lease half. The suggestion is to persist an ownership token or lease, or enforce single-process database ownership, so recovery claims only expired imports. That is the same request PR #181 took in its round 3, and the answer has not changed:
Qodo's own relevance rating on this half is /agentic_review |
The rebuttal to Qodo's round-1 finding 4 belongs beside the code it defends, not only in the PR conversation: MarkAbandonedImportsFailed deliberately does not prove an import is dead before flagging it, because the app has no cross-process database ownership guard at all and a lease this had to wait out would leave a crashed import's session invisible until it expired -- the bug this exists to close, with a timer on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit b771263 |
…claim it replaced anything Both Qodo round-2 findings, and both are on the ReplaceOverwrittenSessions I added in round 1 rather than on the original change. 1. Overwrite deletes active imports (High) — conceded. The replacement targets were chosen after the import finished, by session name. Two imports of the same file produce the same name, so that list could include a concurrent import's Importing row, and deleting it destroys the batches that import has already committed. The list is now taken in CreateSession, before this session exists, and excludes rows an import owns at that instant: it can only ever name sessions that predate this import and that nothing is writing. Nothing puts an existing row back into Importing, so a row that is not importing then cannot become so later. 2. Failed overwrite reports success (Medium) — conceded. RemoveSessions returns whether the delete committed and ReplaceOverwrittenSessions discarded it, so a failed cleanup left the superseded session beside the replacement while the dialog said the import succeeded — the user asked for a replacement and got a duplicate, silently. The outcome now reaches SdCardImportResult. OutcomeGuidance, which becomes "anything worth saying beyond it worked" rather than "why it did not work": SessionPersisted stays true, because the new session is committed and correct, and the message adds a line telling them the old one is still there. ImportAllOutcome gains Notices for the batch equivalent — deduped like the skip advice, but for files that DID import, so it is not counted as a skip. Tests: 15 (was 13); 272 total, was 257 on main. Desktop and iOS Release builds green, no packages.lock.json drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 7b317c3 |
…y grey metadata
Caught by rendering it, not by reading it. The desktop chip is unmistakable —
amber text in an amber outline. The mobile one, on the same data, came out in
the same muted grey as the date and SAMPLES chips beside it, with no border at
all: on a phone the only thing marking a session as the fragment of a failed
import read as one more metadata pill.
Two independent causes, both silent, neither visible to the compiler:
- `Border.chip` sets no BorderThickness, and Border's default is 0, so the
chip's `BorderBrush="{DynamicResource StatusAmber}"` painted nothing. It
was a dead property. The mobile views that already do this pair the brush
with an explicit `BorderThickness="1"` (DeviceLogsMobileView,
DevicesMobileView); this one now does too.
- `Border.chip > TextBlock` already sets Foreground to TextSecondary, and the
inline `{DynamicResource StatusAmber}` on the child lost to it, so the text
fell back to grey. The fix is the pattern the neighbouring chipValue style
established for exactly this: override one style with a later one. New
`TextBlock.chipWarn` class, and the reason it has to be a style rather than
an inline value is written down beside it so the next person does not
rediscover it the same way.
Verified by capture, not by inspection: tools/parity-audit/AvaloniaCapture boots
the real app headless against a seeded database. Before, mobile-portrait-3-storage
showed grey-on-grey; after, the chip is amber on both portrait and landscape and
matches desktop-2-loggeddata. Substituting a literal magenta proved the inline
value was the thing being overridden rather than the resource failing to resolve.
The desktop pane is untouched — it never used the chip class.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every existing test of Sessions.Status starts from a database this build migrated from nothing, where the column has existed since the file did. That is the one case a real user never has. Theirs was written before the column existed, and the question a greenfield test cannot ask is what their sessions read as afterwards: Complete, or — if the default landed wrong or the migration never ran — flagged INCOMPLETE IMPORT on a session that imported perfectly well a year ago. A chip appearing on untouched data is a visible regression no test on a fresh database can see, and this repo has been burned twice by database-state bugs (#181, #196). Three tests that start where the user starts. The old schema is built by the app's own migrations stopped at the last one that shipped (IMigrator.Migrate to 20260415000000_AddSessionSampleCount) rather than by a hand-written CREATE TABLE that could drift from it; rows go in by raw SQL naming only the columns that existed then; and only then does the new code get the file, through DatabaseMigrator.PrepareMigration / ApplyMigrations and LoggingManager.LoadPersistedLoggingSessions — what startup actually calls. - a pre-existing session still reads Complete, shows no chip, and keeps its samples and count; - the migration is reported as pending, applied automatically, and then no longer pending; - the migration reverses without taking the sessions with it, which matters because SQLite cannot drop a column without rebuilding the table. The first asserts the fixture is honest before it asserts anything else — that the Status column really is absent — so a mistake leaving the database fully migrated cannot make it pass for the wrong reason. Behaviour confirmed by neutering: changing the migration's defaultValue from 0 to 2 fails exactly the first test (1 failed, 2 passed), the other two being independent of the default. 275 total, was 272. No production change: the upgrade path was already correct. This is the guard that says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The INCOMPLETE IMPORT chip, actually renderedXAML bindings are not compile-checked here, so a green build says nothing about whether a new label reaches the screen. These come from Desktop session row (
Mobile session card (
What this caughtThe mobile chip was reaching the screen but not reading as a warning — same muted grey as the neutral metadata pills next to it, and no border at all. Both causes were silent, and neither is visible to the compiler: Same chip, before and after the fix in
Substituting a literal magenta proved the inline value was the thing being overridden rather than the resource failing to resolve. Landscape ( |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 527c46d |
|
Qodo-clean, CI green — ready for review on Qodo round 4 on this head: 0 bugs, 0 rule violations, 0 unresolved threads (5 total, all resolved), re-checked after a settle interval with no change. CI: all six checks pass, Android and iOS included. Not merged. |
Not merging — for review. Closes #185. Body refreshed against the final diff (
527c46d), after three Qodo rounds and a shepherd pass.What was wrong
Import a truncated or corrupt
.binfrom the SD card — a card pulled mid-write, a device that died during the transfer, a file a filesystem check partly recovered. You see "Import Failed", which is correct. What you are not told is that the import left data behind.The import was never atomic.
CreateSessioncommits theSessionsrow before a single sample is parsed,FlushBatchAsynccommits every 1,000-sample batch in its own transaction, and theawait foreachdriving both had notry/catchat all. A parser that gave up part way through therefore left a committed session plus however many batches had landed — and because startup reloads every session that has samples, that fragment came back at the next launch looking like an ordinary, complete session, with silently truncated data. Nothing crashed. The database just said something untrue.Import a 0-byte log — what an interrupted logging session routinely leaves on a FAT card. The importer treats that as a legitimate empty session on purpose, creates the row, reports success, and shows it in the list. Then
LoadPersistedLoggingSessionsdeletes every zero-sample session at the next launch, with no message. Two components disagreeing about whether such a session exists, and the user only ever sees the disagreement.The two halves pull against each other: flag a failed import and the purge deletes the flag; leave the purge alone and the flag has to survive it.
How it is fixed
The mechanism that decides a session is incomplete is a new persisted
Sessions.Statuscolumn (SessionStatus:Complete,Importing,ImportFailed; migration20260902160000_AddSessionStatusdefaults every existing row toComplete, which is what they are). The importer writesImportingbefore it parses anything, and on the way out writes:Complete+SampleCountImportFailed, samples keptPartial samples are kept rather than rolled back because the source log is usually damaged in a way a re-import cannot get past — discarding the prefix is the one outcome the user cannot undo. Both heads show the chip (desktop session row, mobile session card), with a tooltip explaining that the session holds only the part of the log read before the import stopped, and
AccessibilitySummarysays "incomplete import" so a screen reader announces it too.The column also covers the case no
catchblock can: a process killed mid-import leaves the row sayingImporting, and startup flags it as failed. That is the only reason the state is persisted rather than kept in memory.The zero-sample question is settled by the invariant that makes the whole thing small: a persisted session row always either has samples or is being written right now. Under it —
Import Allrecords it as a skip with its own guidance line rather than counting it among the imported);Samples.Anyfilter never sees it;GetLoggingSessionsSnapshotwas added to dodge, now impossible rather than avoided by which method a caller happens to pick. Its doc comment is updated to say so.Session IDs. The importer allocated
MAX(Sessions.ID) + 1— the narrow versionLoggingManager's own comment warns against, since it can hand out an ID that orphanSamples/SessionDeviceMetadatarows still reference, after which the composite PK rejects the insert. Both callers now go through oneSessionIdAllocator.Overwrite.
ImportOptions.OverwriteExistingSessiondeleted the session it replaces inCreateSession, before anything had been parsed — so an overwrite that then failed or produced nothing destroyed the user's existing session and put nothing in its place. The replacement is now imported alongside the original and the original dropped only once a complete replacement exists; the sessions it is entitled to replace are chosen when the import starts, excluding rows another import owns, so cleanup can never take a concurrent import's row and the batches it has already committed. (No production call site sets this option today — all three passoptions: null— but the code is on the same path as everything above.)Two smaller changes this needed. The final in-memory
SampleCountwrite nowPosts to the UI thread instead ofInvoke-ing it: nothing waits on the result, the caller only adds the session to the list after this returns, and blocking the importing thread on a dispatcher loop that may not be running is a hang — it is what made the import untestable at all. AndImportFromDeviceAsyncno longer deletes the file from the device unless a finished session came of it.The upgrade path
A new persisted column means an existing user's database has to survive meeting it, and this repo has been burned twice by database-state bugs (#181, #196). What happens:
Sessions.Statusis picked up by the existing startup path —DatabaseMigrator.PrepareMigrationseeds__EFMigrationsHistoryfor pre-migration databases and reports the column as pending, andApplyMigrations(which backs the file up first, and rolls back on failure) runs it. The mobile heads reach it throughMigrateWithCorruptionRecovery.Complete, not incomplete. The column isNOT NULL DEFAULT 0andComplete = 0, so every row written before it existed migrates to "finished" — which is what those rows are. A session that imported fine a year ago does not start wearing an INCOMPLETE IMPORT chip after the upgrade.Downdrops the column; SQLite rebuilds the table, and the sessions and their samples come through intact.All three are now tested rather than reasoned about — see below.
Verification
Daqifi.Avalonia.Tests/Loggers/SdCardImportAtomicityTests.cs, 3 inSessionStatusUpgradeTests.cs, 1 inImportAllSummaryTests. Full suite 275 passed / 0 failed (257 onmain, whose head4f96f77is this branch's merge base, so 275 is the merge result rather than a parallel measurement). They run against a real SQLite file migrated by the app's ownDatabaseMigrator, drivingImportSessionAsyncwith a parsed log whose sample stream throws, cancels, or ends empty, and asserting on what is actually committed.SessionStatusUpgradeTestsbuilds the old schema out of the app's own migrations stopped at the last one that shipped (IMigrator.Migrate("20260415000000_AddSessionSampleCount")) rather than a hand-writtenCREATE TABLEthat could drift from it, writes rows by raw SQL naming only the columns that existed then, and only then hands the file toPrepareMigration/ApplyMigrations/LoadPersistedLoggingSessions. It asserts theStatuscolumn really is absent first, so a fixture mistake cannot make it pass for the wrong reason.Complete, no finalisation in thecatch, unconditional purge) →Failed: 7, Passed: 3of the original ten. The three survivors are the successful-import case, the abandoned-empty-session purge (a guard on behaviour that must not change) and the ID allocation, which was not part of what was neutered.Importingrows) → exactlyAn_overwrite_never_replaces_a_session_another_import_is_writingfails, the other three still pass.defaultValuefrom0to2→Failed: 1, Passed: 2, exactly the upgrade test that asserts a pre-existing session still readsComplete; the other two are independent of the default.ImportAllSummaryTestscase is not covered by this exercise: removingRecordNoticestops the suite compiling, so there is nothing to run.Daqifi.Avalonia.Desktop, and iOSRelease/iossimulator-arm64(restore without-r, build with it and--no-restore), re-run on every push. Nopackages.lock.jsondrift. The Android head was not built — the workload is absent on this machine; CI covers it.IsIncompleteImport,IncompleteImportTooltip); nothing was renamed or removed, so no reflection-bound view can break silently.StatusAmberis defined in both ofDesignTokens.axaml's theme dictionaries (DarkandDefault, identical values — the visual system is dark-only), so the lookup cannot come back empty. XAML bindings are not compile-checked here, so the chip was additionally rendered rather than reasoned about — see below.ImportFromDeviceAsync) is unchanged apart from the delete-after-unsuccessful-import guard and a log line; everything tested here is the parse-and-commit half, driven directly.The chip, rendered
A green build cannot tell you a new label appears — a blank cell is the classic silent failure here. So both heads were captured by
tools/parity-audit/AvaloniaCapture, which boots the real app headless against a seeded database (aCompletesession and anImportFailedone, each with samples, in a throwawayDAQIFI_DATA_DIR). Screenshots in the comment below.That is what caught the one real defect the shepherd pass found: the mobile chip rendered as ordinary grey metadata, indistinguishable from the date and SAMPLES pills beside it, for two independent and individually silent reasons —
Border.chipsets noBorderThickness, andBorder's default is0, so the chip'sBorderBrush="{DynamicResource StatusAmber}"painted nothing; andBorder.chip > TextBlockalready setsForeground, and the inline{DynamicResource StatusAmber}on the child lost to it, so the text fell back to the muted grey.Fixed by pairing the brush with an explicit
BorderThickness="1"(as the other mobile views that do this already do) and adding aTextBlock.chipWarnstyle beside thechipValueone that established the pattern — with the reason it must be a style, not an inline value, written down next to it. Substituting a literal magenta proved the inline value was the thing being overridden rather than the resource failing to resolve. The desktop pane never used the chip class and was correct throughout.Qodo
Three rounds during the build, six findings, all triaged; every inline thread answered and resolved.
7086045): Overwrite loses existing session (High), Finalization failure reports success (High), Cleanup failure leaves hidden row (Medium) conceded and fixed in084d1ea. Startup relabels live imports (Low) conceded in part — the read-then-save race inMarkAbandonedImportsFailedis now one conditionalUPDATE; the ownership-token/lease half is rebutted on fix(startup): a damaged DAQiFiDatabase.db made the app permanently unopenable #181's record (the app has no cross-process database ownership guard at all, and a lease this had to wait out would leave a crashed import's session invisible until it expired — the bug being fixed, with a timer on it). Recorded in the code comment, not only in the thread.b771263): two new findings, both on the round-1 fix rather than the original change — Overwrite deletes active imports (High) and Failed overwrite reports success (Medium). Both conceded and fixed in7b317c3.7b317c3): zero findings, zero unresolved threads, re-checked after a settle interval.Deliberately not done
No shared quarantine/relocate helper. This is the fifth member of the "a partial operation reported success" family, and unifying the four hand-written copies is a deferred target blocked on #195 and #196; this PR adds no new copy of that pattern.