Feature/offline sync - #11
Merged
Merged
Conversation
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.
Phase 8 — Offline sync (build plan steps 41–43b, + a follow-up pass)
Five commits: the durable op log and change cursor (41), the push engine that replays a
technician's queue against the domain (42), the pull engine that tells a device what it missed
(43), the server side of photo/signature capture (43b), and a pass closing decisions nothing in
the remaining plan would ever force.
720 tests pass (625 unit, 95 integration); zero warnings in Debug and Release under nullable +
analyzers + warnings-as-errors;
make check-contractsclean; both migrations apply to a databasethat already has rows. No endpoint exists yet for any of this — steps 50 and 50b.
Job.Notes/RecordNotes/CanRecordNotes(LWW),Job.Lines/RecordLine/JobLine(field-recorded work),Job.Unschedule()+JobUnscheduled,TimeWindow.LatenessOfAttachmentaggregate,AttachmentKind,AttachmentId,StorageKeySync/SyncOpRecord/SyncOpId/SyncCursor,Sync/PushOps/,Sync/PullChanges/,Sync/FieldOps,Sync/SyncErrors,Sync/SyncRemovalAbstractions/ISyncOpStore,ISyncCursorSource,ISyncChangeReader,IAttachmentStorage,IAttachmentRepository,SyncJobState/SyncStopState/SyncScopeChangesPersistence/ChangeStamps(trigger-based change_seq on every table),SyncCursorSource,SyncOpStore,SyncChangeReader,AssignmentRepository.Remove(writes tombstone)Attachments/LocalDiskAttachmentStorage,JobConfiguration(lines as jsonb),AppDbContext.StampVersions(owned-child changes bump root version)SyncConflictReason.Unsupported; regeneratedjobTransitions(Unschedule)SyncOpLog,SyncRemovals,Attachments— three tables, one trigger function shared by allDecisions a reviewer should weigh in on
The change stamp is a database trigger — the first logic this project has put in the database.
A cursor is only correct if everything a device hasn't seen is at or after it. An application-side
counter (interceptor or per-tenant sequence) has a hole: the number is allocated before the row
becomes visible, so a device can be handed a cursor above rows still invisible to it, and they're
then below its cursor forever. Fix: stamp with
pg_current_xact_id()(the writing transaction'sown id) via
BEFORE INSERT OR UPDATE, and takepg_snapshot_xmin(pg_current_snapshot())as thecursor — the oldest transaction that could still be in flight. Holds by construction; an
interceptor would only be correct while every save happens to sit in an explicit transaction.
Cost: a new table's trigger must be hand-written (
SchemaTestscatches a missing one); thishappened for real three times (SyncOpLog → SyncRemovals → Attachments) and the test caught it each
time I checked by removing one.
"Changed since" is inclusive (
>=), so pull can resend a change. The cursor is the oldestpossibly-in-flight transaction, so missing the exclusive boundary is the only safe direction.
Correct side to fail on: every pulled change is a whole state, so resending writes the same thing
twice; an exclusive cursor could skip a late-committing write and never send it.
The watermark is cluster-wide, not per-tenant — one org's writes advance every org's cursor.
Costs an occasional empty pull response; the alternative is a per-tenant sequence with a lock held
to commit, i.e. serialized writes per org. Wrong trade for a ten-technician shop.
A stop leaves a technician's day two ways, and only one can report itself. Handed to another
technician: the row survives, its own stamp reports it. Deleted outright (re-optimization dropping
unplaceable work): nothing left to carry a stamp, so
AssignmentRepository.Removewrites async_removalstombstone in the same save. Rejected alternative: soft-deleting assignments, whichwould make the unique index on
assignments(job_id)partial and put aRemovedAt is nullfilteron every existing query. A tombstone is additive; a soft delete changes everything that reads a
stop. The tombstone write lives in the repository (not the handler) so a future second
plan-dropping handler can't forget it — the alternative, a
SaveChangesinterceptor, is fragilewhile EF is still building its change set.
Pull scope = "a job is in scope because a stop for it is mine." Needs no per-device state,
which is what keeps sync retriable from a cursor alone. Consequence: a job whose stop moved away
leaves the device's world silently via the stop's removal, with no second "this isn't yours"
event. This incidentally fixed step 37's old wart (a dropped job kept showing
Scheduledwithnothing planned) on the phone's side;
Job.Unschedule()(below) fixes the dispatcher's side.Removals are deliberately over-emitted — named to a technician whether or not they ever held that
stop, because the server can't know what a device holds without per-device state.
The op log holds only applied operations; refusals aren't recorded.
AppliedAtis notnullable. A resent refusal is re-judged against current state rather than answered from memory —
correct while every refusal is re-derivable (it is, today).
A pushed batch never fails; conflicts ride inside a successful result.
TransactionBehaviorrolls back a failed result, so reporting one bad op as failure would discardevery op that succeeded ahead of it — a technician's whole morning lost to one stale tap.
Mutation-verified: forcing a failure when conflicts exist fails exactly the two integration tests
asserting the batch still committed.
The build plan's domain didn't exist yet, so most of step 42's diff builds it. "Notes, line
items" as field ops assumed
Jobalready had somewhere to put them; it didn't. Notes are oneoverwritable field (LWW needs something to beat — a list can never conflict); recorded lines are
an append-only
JobLinecollection, deliberately not the invoicingLineItem(one is a record ofwhat happened, the other a statement of what's owed — merging them now would prejudge whether
invoicing derives from field work, which is Documents 6–7's call). Neither is refused by any job
status: a technician who drove out to a cancelled job has still spent the hour.
Recorded lines are one
jsonbcolumn on the job, not a table — never read without the job,never queried alone, and being part of the job's own row is what makes a recorded part a change
to the job, moving both the version stamp and the sync change stamp. Cost: EF leaves the owner's
change-tracker entry
Unchangedwhen only a JSON child changes (confirmed with a throwawaydiagnostic), so
StampVersionsnow walks owned children explicitly — which incidentally fixed thesame latent bug for customer locations and invoice lines, neither of which had ever bumped their
root's version either. Limit: a removed child is invisible to this walk (nothing removes one
today; step 47 will).
Contractsgained a thirdSyncConflictReasonmember,Unsupported, in a step whose textnamed only an Application folder. The alternative — reject the whole batch when it contains an
operation type the server doesn't know — would let a device one release ahead poison its own
queue forever, since
Entity/Typeare strings specifically so unknown ops can be named. Addingthe member and refusing per-operation was the only option that doesn't strand a technician.
A
StorageKeyvalue object is derived, never given. It becomes a filesystem path today and abucket key later; a key built from anything a device sends (filename, path) is directory
traversal. It can only be constructed from a tenant + attachment id, both already-vouched values.
Cost: a fourth value object/converter for what could've been a string — worth it because it moves
the guarantee from "the adapter remembers to sanitize" to "the dangerous call can't be written."
Bytes are written before the metadata row (in
UploadAttachmentCommand, step 50b's problem tocall): a mid-request death leaves an orphaned file rather than a row pointing at nothing — orphans
are inert litter, dangling rows are reachable and broken. Nothing sweeps orphans yet (no owner,
same class as unpruned tombstones).
Local-disk attachment storage is the shipped answer, not a placeholder — Document 1's
self-hosting promise needs nothing more for one machine. Its stated limit: two instances without
shared storage will write on one and 404 on the other; the bucket adapter is one class behind the
same port whenever that's needed.
The follow-up pass
DECISIONS.local.mdhad accumulated 19 entries with no owner in the remaining plan (steps44–56 won't force them). Two get materially more expensive after step 52 publishes
@opendispatch/contractsto the client repos, so this was the moment to act, not later.Fixed, three things:
Clock skew. LWW notes trusted the device's claimed
ClientTsoutright, so a phone with aclock a day fast would win every conflict until that date passed, undetected.
PushOpsHandlernow clamps every observed instant to
min(ClientTs, now)— including the payload's owncompletedAt— before the domain sees it, while the rawClientTsstill goes to the oplog (the log is evidence; clamping it would hide the broken clock). Clamped, not rejected: the
op itself isn't wrong. Mutation-verified: removing the clamp fails exactly three push tests.
Job.Unschedule(). A job the optimizer drops loses its stop but keptScheduledstatus —the board's one visibly false state. Fixed now rather than later because the transition table
is exported to
@opendispatch/contracts; today the cost is onemake gen-contractsand atwo-line diff, after step 52 it's a release across three repos. First backward edge in an
otherwise linear machine: a plan can be withdrawn, work cannot (nothing returns from
EnRouteonward).JobIntents.Drivableis unchanged — no request drives this, only theoptimizer. Mutation-verified: skipping the withdrawal call fails exactly two optimizer tests.
Lateness stated once. Lived only inside
ObjectiveEvaluator; the web repo was next toneed it and would've invented a second definition. Now
TimeWindow.LatenessOf(start)is therule (measured at work start, not end), used by the evaluator for both lateness and overtime,
and sent to the board as
BoardStop.LateByrather than left to be re-derived client-side —arithmetic can't be exported safely the way the transition table is (data vs. logic).
Mutation-verified: forcing it to always say "on time" fails the domain tests, two objective
tests, three search tests, and the board test — five independent readers of one rule.
Decided, not coded: MediatR stays pinned at 12.5.0 (reviewed, not inherited — trades upstream
fixes for Document 1's no-licence-bill promise; reasoning now lives permanently in the csproj, not
this file). Unassigned-job reasons are committed to step 48 rather than left unowned (the
expensive half is the engine —
Solution.Unassignedwould need a reason per job, touching theconstructor invariants,
ObjectiveEvaluator,Insertion, and both schedulers). Retention isstill open but is now one question instead of three, naming the gap that was missing:
sync_ops— not the tombstones or blobs — is the table that actually grows fastest and can't bepruned freely, since dropping rows un-dedupes any device offline longer than the window.
Checked and left alone: the payment reference (zero risk pre-processor), the duplicated
window-overlap predicate (still two sites, both boundary-tested), the local-disk storage limit (a
deployment decision behind an existing port), the
Microsoft.OpenApipin (dependabot owns it),the missing outbox (step 51 is the first real subscriber to need one), and everything whose answer
belongs to the client repos (
Dispatched's schedulability, the skill-match override, pull'sover-emission).
What is deliberately not here
/sync/pushor/sync/pullendpoint — step 50, including the technician-from-principalresolution and the
SyncConflictReasoncode mapping.50 owns it — can't be paging, since the cursor is a transaction watermark with no position
inside a transaction.
GET /export— step 50b.GenerateInvoicestill ignoresjob.Lines— a product decision (what's billed isn't alwayswhat the visit took), left to Documents 6–7 or step 55.
unowned questions rather than silently deferred.