feat(yield): persist a daily snapshot ledger per active plan - #1210
Open
bilhokista wants to merge 4 commits into
Open
feat(yield): persist a daily snapshot ledger per active plan#1210bilhokista wants to merge 4 commits into
bilhokista wants to merge 4 commits into
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.
Closes #1130.
What this adds
A
daily_yield_snapshotstable and a worker that writes one row per active, yield-earning plan each day. Accrued yield was only ever computed on read fromplans.last_ping, so there was no record of what a plan was worth on any past day — and none could be reconstructed after a rate change.The snapshot copies
principalandyield_rate_bpsas they stood on the day, rather than referencing the live columns, so a later rate change cannot silently rewrite history.The constraint that matters most
CONSTRAINT daily_yield_snapshots_plan_date_unique UNIQUE (plan_id, snapshot_date)This is the ledger's integrity guarantee, not a nicety. A daily worker runs twice more often than one might expect: a restart, a redeploy, or a second replica. Without the constraint each of those silently doubles a plan's history, and an audit ledger that double-counts is worse than none.
Writes use
ON CONFLICT ... DO NOTHING, so the first observation of a day stands and a repeat run is a no-op.snapshot_allreturns the number of rows actually inserted, which is how a re-run makes itself visible in the logs rather than looking identical to a first run.I chose
DO NOTHINGoverDO UPDATEdeliberately — for an audit record, first-write-wins is easier to reason about than a value that can change after the fact. Say the word if you would rather the latest run win.Two edge cases handled explicitly
seconds_until_next_runnever returns zero. Landing exactly on the configured hour returns a full day, not0. A zero delay would spin the worker in a tight loop, re-running the snapshot as fast as the database could answer it. There is a test for exactly that, plus a sweep asserting the wait is positive and at most a day across all 24 hours.elapsed_since_pingis saturating.last_pingis stored as epoch seconds and can sit in the future after a clock correction. A plain subtraction cast tou64would wrap into an enormous positive number and write an absurd figure into the ledger. Tested withi64::MAX.Also: one plan failing to insert logs a warning and the run continues, rather than abandoning the rest of the ledger; and a failed run logs and keeps the worker alive, which is safe precisely because the unique constraint makes the retry idempotent.
Configuration
YIELD_SNAPSHOT_HOUR_UTCpicks the run hour, defaulting to midnight UTC. Values outside 0–23 fall back to the default rather than being trusted into the schedule calculation.Verification
I could execute the scheduling and elapsed-time logic. Both helpers and all 7 tests were lifted into a scratch crate with real chrono and run with
cargo test— all pass, including the 96-combination sweep.Honest note: I could not run the crate's own
cargo test(it needs the full dependency graph and sqlx's database or offline metadata), sosnapshot_allitself — the SQL, theDecimalconversions and therows_affectedaccounting — is unverified beyond review and needs CI with a database.One conversion worth a reviewer's eye:
principalgoes throughDecimal → String → f64to reach the existingcalculate_yield, which takesf64. ForNUMERIC(78, 0)values that is lossy at the extreme end. I kept it rather than changingcalculate_yield's signature, since that function is used elsewhere and widening it is a separate decision — but if you would prefer the whole path inDecimal, I am happy to do that here.Overlap note:
main.rsis also touched by my open PR #1209 (graceful shutdown), which changes how background workers are started. This PR deliberately follows the currentmasterpattern so it stands alone. If #1209 lands first, this worker should return itsJoinHandletoo — a one-line follow-up I am glad to make.🤖 Generated with Claude Code
https://claude.ai/code/session_01CrfEY1tvXrbeMDAUzxfuk7