feat(upstream): hand the server the history collected before pairing - #115
Conversation
A Desktop that ran standalone for months and was then paired appeared on the fleet page starting from the day of pairing. Every earlier day it had recorded was absent from the total, and there was no way to get it there. The first time a CashPilot server confirms this worker, Desktop now uploads its recorded daily balances to /api/workers/earnings-import. It is a COPY, not a migration. The local rows are read and left exactly where they are, so unlinking leaves this machine still showing precisely what it earned on its own -- which is the behaviour that was asked for, and it falls out of not moving anything rather than being implemented separately. The server files the readings under this client's own source rather than merging them into its own series. That matters because earnings are clamped deltas between consecutive balance readings: interleaving two samplers of one provider account makes every apparent drop clamp to zero and the total comes out systematically understated. Separate series are differenced separately and then summed. Details that are load-bearing: * Only a CONFIRMED worker imports. A client still presenting the shared enrolment key is refused by the server -- every worker holds that key, so it cannot prove who is writing -- and asking anyway would log a 403 every minute. * The marker records WHICH server received the history, not merely that it was sent, so pairing with a different server hands it over too. * A failed or partial upload is not recorded, so it retries on the next heartbeat; the import is idempotent, so a retry costs a round trip. * A server too old to have the endpoint answers 404. That is not transient, so it is asked once per run rather than once a minute -- and not recorded as delivered, so an upgraded server still gets it. * Historical readings carry NO exchange rate. Desktop does not record what a currency was worth on a past day, and stamping today's rate onto a year-old reading would misprice it confidently. Refs: CashPilot-Desktop-xjr
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe desktop client imports up to 400 days of local daily balances after worker confirmation. It authenticates requests with the machine key, omits unknown exchange rates, retries failed transfers, suppresses unsupported servers, and records successful handovers per server. ChangesHistorical earnings synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
+ Coverage 73.74% 74.24% +0.49%
==========================================
Files 17 18 +1
Lines 3634 3747 +113
==========================================
+ Hits 2680 2782 +102
- Misses 761 762 +1
- Partials 193 203 +10
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
upstream_client.go (1)
120-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
historyUnsupportedafter the loop goroutine has stopped.
stopUpstreamclears the flag at Line 127, then cancels and waits at Lines 132-135. AsendUpstreamcall that is already in flight can set the flag back totrueafter the clear.startUpstreamcallsstopUpstreamfirst, so the new loop then starts with a staletrueand skips the import until the next restart. This is the exact case the re-arm exists for: a user who just upgraded the server and re-saved settings.Move the reset after
<-done.🐛 Proposed fix
func (a *App) stopUpstream() { a.upstream.mu.Lock() cancel, done := a.upstream.cancel, a.upstream.done a.upstream.cancel, a.upstream.done = nil, nil - // Re-arm the earnings import. startUpstream calls this first, so every - // restart -- and every unpair -- gives an upgraded server another chance, - // which is exactly where a user who just upgraded theirs would expect it. - a.upstream.historyUnsupported = false a.upstream.mu.Unlock() - if cancel == nil { - return + if cancel != nil { + cancel() + if done != nil { + <-done + } } - cancel() - if done != nil { - <-done - } + // Re-arm the earnings import only once the old loop cannot write the flag + // again. startUpstream calls this first, so every restart -- and every + // unpair -- gives an upgraded server another chance. + a.upstream.mu.Lock() + a.upstream.historyUnsupported = false + a.upstream.mu.Unlock() }🤖 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 `@upstream_client.go` around lines 120 - 136, Move the reset of a.upstream.historyUnsupported in stopUpstream to after the cancellation and completion wait, ensuring the loop goroutine has stopped before clearing the flag. Keep the existing cancellation and done-channel handling unchanged.
🧹 Nitpick comments (1)
internal/upstream/import_test.go (1)
108-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnsynchronized test observation state across three stub handlers. Each site records request details inside an
httptesthandler goroutine and reads them from the test goroutine. The only thing between the write and the read is a real TCP socket, which the race detector does not model as a happens-before edge, sogo test -racecan report a data race at any of these sites.importStubandpairingStubinupstream_history_test.goalready show the intended pattern.
internal/upstream/import_test.go#L108-L130: add async.MutextostubServer, guard thecalls,path,auth, andbodywrites in the handler, and read them through an accessor in every assertion.upstream_history_test.go#L207-L221: guard theauthcapture with a mutex and read it under the same lock at Line 219.upstream_history_test.go#L340-L358: guard thecallscounter with a mutex and read it under the same lock at Line 358.As per coding guidelines: "Run and maintain Go tests with race detection and coverage measurement using
go test -race -coverprofile=coverage.out ./...".🤖 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 `@internal/upstream/import_test.go` around lines 108 - 130, Synchronize test observation state across all three sites: in internal/upstream/import_test.go lines 108-130, add a mutex to stubServer, guard handler writes to calls, path, auth, and body, and expose an accessor that assertions use for reads; in upstream_history_test.go lines 207-221, protect auth capture and read it under the same mutex at line 219; in upstream_history_test.go lines 340-358, protect the calls counter and read it under the same mutex at line 358. Run the Go suite with race detection and coverage using the specified command.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.
Inline comments:
In `@upstream_client.go`:
- Line 183: Update pushHistoryOnce to re-read a fresh configuration snapshot
immediately before the Manager.Save call that persists the upstream marker,
rather than reusing the cfg captured before the earnings import. Preserve all
settings that may have changed through SaveSettings or a.cfg.Save while
retaining the marker update.
---
Outside diff comments:
In `@upstream_client.go`:
- Around line 120-136: Move the reset of a.upstream.historyUnsupported in
stopUpstream to after the cancellation and completion wait, ensuring the loop
goroutine has stopped before clearing the flag. Keep the existing cancellation
and done-channel handling unchanged.
---
Nitpick comments:
In `@internal/upstream/import_test.go`:
- Around line 108-130: Synchronize test observation state across all three
sites: in internal/upstream/import_test.go lines 108-130, add a mutex to
stubServer, guard handler writes to calls, path, auth, and body, and expose an
accessor that assertions use for reads; in upstream_history_test.go lines
207-221, protect auth capture and read it under the same mutex at line 219; in
upstream_history_test.go lines 340-358, protect the calls counter and read it
under the same mutex at line 358. Run the Go suite with race detection and
coverage using the specified command.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15f17e3f-70ba-404c-8ca4-50b55cc64c27
📒 Files selected for processing (7)
CHANGELOG.mdinternal/config/config.gointernal/upstream/import.gointernal/upstream/import_test.gointernal/upstream/upstream.goupstream_client.goupstream_history_test.go
…lobber settings pushHistoryOnce read the config, uploaded up to 400 days of earnings over the network, then saved. Manager.Save writes the WHOLE AppConfig, so anything the user changed on the settings screen while the upload was in flight was silently discarded -- and that upload is the slowest thing the app does unprompted. Manager.Update applies a mutation and persists under the write lock as one read-modify-write, which closes the window rather than narrowing it. Re-reading just before the save would have left a smaller version of the same bug. Save is still right for the settings form, which legitimately writes the whole object; Update is for a single field written by background work. Proven by control: reverting to the whole-config save of the stale snapshot fails the new test with DisplayCurrency back at USD, exactly the reported symptom. Reported by CodeRabbit on PR #115.
1 participant
What
A Desktop that ran standalone for months and was then paired appeared on the fleet page starting from the day of pairing. Every earlier day it had recorded was absent from the total, and there was no way to get it there.
The first time a CashPilot server confirms this worker, Desktop now uploads its recorded daily balances to
POST /api/workers/earnings-import(CashPilot v1.16.0+, GeiserX/CashPilot#256).This is the Desktop half of
CashPilot-Desktop-xjr.It is a copy, not a migration
The local rows are read and left exactly where they are. That is not an implementation detail — it is the "if it unlinks, show only what this machine earned alone" behaviour. Nothing had to be built for it; it falls out of not moving anything. A test asserts the local history survives the push, because a later refactor that "tidied up" by deleting the uploaded rows would be silently destructive.
Why the server files it under a separate source
Both sides may have been reading the same provider account. Earnings are stored as cumulative balance readings, and an earned figure is the clamped delta between consecutive readings — so interleaving two samplers of one account makes every apparent drop clamp to zero, and the total comes out systematically understated. Each client's readings are differenced on their own and the results summed.
The details that are load-bearing
403every minute and teach the user to ignore the log.(platform, source, date)and updates — so a retry costs a round trip and nothing else.stopUpstream, so restarting or re-saving settings re-arms it; persisting it would make one old server a permanent verdict on that URL.0.0would price it at nothing. Absent means unknown, which is the truth.Verification
go build,go vet,gofmtclean;go test -race ./...green across all packages.Every property is confirmed by negative control — each of these mutations fails the test that claims to catch it:
TestHistoryIsHandedOverOnceAndOnlyOnceTestAFailedHandOverIsRetriedRatherThanRecordedTestPairingWithADifferentServerHandsItTheHistoryTooTestATrailingSlashIsNotADifferentServer0.0TestHistoryReadingsConversionTestHistoryReadingsConversionConfirmedalways trueTestAnUnconfirmedWorkerDoesNotTryToImportTestAnUnconfirmedWorkerDoesNotTryToImportTestAnEmptyHistoryIsRecordedWithoutPostingAnythingOne of those controls passed on the first attempt, which meant the test was wrong rather than the code right:
TestAnUnconfirmedWorkerDoesNotTryToImportoriginally covered only first contact, where the import fails anyway for want of a credential — so it would have passed against a build with no gate at all. It now also covers a worker that holds a key the server is still re-delivering, where only the gate itself stops the import. That is the case both mutations now fail.Summary by CodeRabbit