Visitar URL original
fix(booking): stop padding a balance on the pad's own date (#2187) · rustledger/rustledger@b074bad · GitHub
Skip to content

Commit b074bad

Browse files
robcohenrobcohen
authored
fix(booking): stop padding a balance on the pad's own date (#2187)
* fix(booking): stop padding a balance on the pad's own date A `pad` and the `balance` it targets on the same date synthesized a padding transaction beancount does not. On the issue's fixture the account ended 60.00 USD richer -- 100.00 where beancount reports 40.00 -- and `check` reported the pad as UNUSED in the same run that booked it. beancount checks a balance at the START of its day: `entry_sortkey` gives Balance -1 and Pad the default 0, so a same-date balance is evaluated before the pad exists and consumes nothing. Ours had `DirectivePriority` Pad=2 before Balance=3, with a comment asserting "padding before balance assertions", and `process_pads` sorted by DATE ALONE -- so a pad written above its balance was processed first and the balance consumed it. Both are fixed: Balance now precedes Pad, and `process_pads` sorts by the canonical `booking_sort_key` rather than by date. Two tests pinned the old behavior and are inverted, with the evidence in their comments. One asserted a same-date synth exists and precedes the balance; the ordering guarantee it was really protecting is preserved in a new test on the day-earlier case, which is where a synth actually exists. The other was named `test_sort_directives_pad_before_balance`. Verified: - the fixture now returns 40.00, matching beancount; a pad one day earlier still pads to 100.00, also matching - `check` still emits E2003 and E2001, which is what bean-check emits for the same file -- the difference is that we no longer book the pad we call unused - booked values UNCHANGED on all four corpus files that contain a same-date pad+balance, compared against a main-built binary - directive order changes only in files containing pads: 59 pad-free files produce byte-identical `#entries` output - the four new `corpus_parity` divergences are the prebuilt component still using the old order (`native=(balance) component=(pad)`), which is what a correct engine change looks like against a stale component Not fixed here, and worth stating: beancount interleaves same-date pads and transactions by lineno (both sort at 0) while we still group by type. That is the ordering divergence in #2149, unchanged by this. The booked-value gate reports clean, but its axis is prices; it does not compare balances and so does not exercise this. Closes #2150 * test(booking): pin the pad that follows a same-date balance Review find on #2187. Sweeping the pad scenarios against beancount turned up a second broken case, worse than the reported one: a pad written after a balance on the same date, serving a LATER balance. 2024-06-15 balance Assets:A 0.00 USD 2024-06-15 pad Assets:A Equity:O 2024-07-01 balance Assets:A 50.00 USD bean-check accepts that file. On main the account came back with NO balance at all and `check` reported nothing -- silently empty rather than silently wrong. The same-date balance consumed the pad, so the later assertion had nothing left. The fix already covers it, because Balance now precedes Pad and the pad stays pending. This adds the test. Both halves of the fix are confirmed by mutation: restoring Pad-before-Balance fails two tests, restoring the date-only sort in `process_pads` fails one. * style(booking): count instead of collecting in the new pad test * test(ffi): cover the same-date pad+balance on the FFI path Review find on #2187. The issue explicitly left the FFI path unverified, noting that `expand_pads` runs its own date-only sort and could diverge from the CLI either way. Nothing there covered the case: the existing `SAME_DATE_LEDGER` shares a date between a pad and an UNRELATED transaction, with the balance a day later. It is correct -- `expand_pads` gets its pads from `process_pads`, which this branch fixed, so the decision is shared rather than duplicated -- but that was unverified rather than untrue. Now asserted, and confirmed to fail when Pad-before-Balance is restored. The date-only sort at the top of `expand_pads` is left alone. It orders the OUTPUT pairs, not the pad decision, and changing it would reorder same-date directives on the FFI path only. Whether the two paths should share an ordering rule is worth deciding on its own, not as a rider here. * docs(booking): mark the stale same-date-balance rule in pad_insertion_index * fix(wasm): bump the cache version the priority swap invalidates `LedgerPayload` archives the output of the shared `process()` pipeline, which sorts by `canonical_sort_key` -- and that key reads `DirectivePriority`. Swapping `Pad` and `Balance` therefore changes the LIST this cache holds without changing a single directive in it: every one archives to the same bytes, in a different order. What a stale blob costs is narrower than it first looks, and the first draft of this message got it wrong by saying the pad AMOUNTS come back stale. They do not: `process_pads` re-sorts its input by `booking_sort_key`, the same `(date, priority)`, so amounts computed from a v16 blob still come out right. What differs is the order of the directives the cache hands back -- what journal output, `SELECT` row order and the FFI directive stream all read. A cache that returns a different sequence than a fresh run is stale by definition, which is reason enough and is the reason recorded. `processed_ledger_archived_form_is_pinned`, which landed on main with #2189 for exactly this class, failed on the rebase and had to be re-pinned. That is the guard working: the version bump was argued for in review before, and is now also demanded by a test. Also folds in two review corrections: `pad.rs` no longer claims to sort "by date" directly above the comment explaining that it sorts by the booking key, and the variant census in `cache.rs` identifies variants by `Directive::type_name` rather than `Debug` on `mem::Discriminant`, whose format is not a documented guarantee -- it also names the missing variant when it fails.
1 parent 1ef068d commit b074bad

4 files changed

Lines changed: 207 additions & 31 deletions

File tree

‎crates/rustledger-booking/src/pad.rs‎

Lines changed: 109 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -142,9 +142,14 @@ pub fn process_pads(directives: &[Directive]) -> PadResult {
142142
let mut padding_transactions = Vec::with_capacity(num_directives.min(16));
143143
let mut errors = Vec::with_capacity(4);
144144

145-
// Sort directives by date for processing
146145
let mut sorted: Vec<&Directive> = directives.iter().collect();
147-
sorted.sort_by_key(|d| d.date());
146+
// Sort by the canonical booking key, not by date alone. Date-only left
147+
// same-date directives in source order, so a `pad` written above the
148+
// `balance` it targets was processed first and the balance consumed it --
149+
// synthesizing a padding transaction beancount does not (#2150). The key
150+
// puts Balance before Pad, so that balance is seen first and the pad ends
151+
// up with nothing to satisfy.
152+
sorted.sort_by_key(|d| rustledger_core::booking_sort_key(d));
148153

149154
for directive in sorted {
150155
match directive {
@@ -339,12 +344,20 @@ fn create_padding_transaction(
339344
///
340345
/// # Sort ordering on date ties
341346
///
342-
/// Synth transactions carry the pad's date, not the balance's date.
343-
/// On a same-date pad+balance pair (legal in beancount), the synth must
344-
/// appear BEFORE the balance so any consumer that checks balance assertions
345-
/// mid-stream sees the correct inventory. This is achieved by prepending
346-
/// the synth list to the original directives before the stable sort:
347-
/// synths land at the front of their date-group, originals follow.
347+
/// Synth transactions carry the pad's date, not the balance's date. When a
348+
/// pad precedes its balance by at least a day, the synth must appear BEFORE
349+
/// that balance so any consumer checking assertions mid-stream sees the
350+
/// padded inventory. This is achieved by prepending the synth list to the
351+
/// original directives before the stable sort: synths land at the front of
352+
/// their date-group, originals follow.
353+
///
354+
/// A pad and the balance it targets on the SAME date produce no synth at
355+
/// all. beancount checks a balance at the start of the day, so it is ordered
356+
/// ahead of a same-date pad and consumes nothing; the pad is reported unused.
357+
/// This doc previously called that pairing "legal in beancount" and required
358+
/// the synth to precede the balance -- which is what produced a padding
359+
/// transaction beancount does not, leaving the account richer by the
360+
/// difference while our own validator called the pad unused (#2150).
348361
///
349362
/// # Errors are discarded
350363
///
@@ -435,6 +448,13 @@ where
435448
{
436449
let mut end_of_group = 0;
437450
for (index, directive) in sorted.into_iter().enumerate() {
451+
// Leftover from before #2150: this exists to put a synth AHEAD of a
452+
// balance on the pad's own date. Such a pad no longer synthesizes at
453+
// all -- the balance is checked first and the pad is unused -- so this
454+
// arm now only fires for an UNRELATED same-date balance, where placing
455+
// the synth ahead of it contradicts the Balance-before-Pad order.
456+
// Removing it changes synth placement on the FFI path, which is its
457+
// own decision; tracked in #2188 rather than ridden in here.
438458
if directive.date() == date && matches!(directive, Directive::Balance(_)) {
439459
return index;
440460
}
@@ -822,11 +842,17 @@ mod tests {
822842
}
823843

824844
#[test]
825-
fn test_merge_with_padding_same_date_pad_balance_synth_comes_first() {
826-
// Pad and balance share the same date. The synth (which carries
827-
// the pad's date) must appear BEFORE the Balance in the merged
828-
// view so any mid-stream balance-assertion check sees the
829-
// correct inventory.
845+
fn test_merge_with_padding_same_date_pad_balance_synthesizes_nothing() {
846+
// A pad and the balance it targets on the SAME date produce NO synth.
847+
// beancount checks a balance at the start of the day, so it is ordered
848+
// ahead of a same-date pad and consumes nothing; the pad is reported
849+
// "Unused Pad entry" and the account keeps its real figure (#2150).
850+
//
851+
// This test previously asserted the opposite -- that a synth exists and
852+
// precedes the Balance -- which is what let the bug through. On the
853+
// fixture below beancount reports 40.00 for the account where we
854+
// reported 100.00, and our own validator called the pad unused in the
855+
// same run that booked it.
830856
let directives = vec![
831857
Directive::Open(Open::new(date(2024, 1, 1), "Assets:Bank")),
832858
Directive::Open(Open::new(date(2024, 1, 1), "Equity:Opening")),
@@ -840,18 +866,85 @@ mod tests {
840866

841867
let merged = merge_with_padding(&directives);
842868

843-
// Find indices of the synth and the Balance.
869+
assert!(
870+
!merged
871+
.iter()
872+
.any(|d| matches!(d, Directive::Transaction(t) if is_synthesized_pad(t))),
873+
"a same-date pad+balance must synthesize nothing; beancount leaves \
874+
the pad unused"
875+
);
876+
// The Pad itself survives, so `WHERE type = 'pad'` audits still see it.
877+
assert!(merged.iter().any(|d| matches!(d, Directive::Pad(_))));
878+
}
879+
880+
#[test]
881+
fn test_pad_after_a_same_date_balance_still_serves_a_later_balance() {
882+
// A pad written after a balance on the same date is NOT consumed by
883+
// that balance -- it stays pending and satisfies the next one. Found
884+
// while reviewing #2150, and it was broken worse than the reported
885+
// case: the account came back with no balance at all and `check`
886+
// reported nothing, on a ledger bean-check accepts silently.
887+
//
888+
// Under the old Pad-before-Balance order the same-date balance ate the
889+
// pad, leaving the later assertion with nothing.
890+
let directives = vec![
891+
Directive::Open(Open::new(date(2024, 1, 1), "Assets:A")),
892+
Directive::Open(Open::new(date(2024, 1, 1), "Equity:O")),
893+
Directive::Balance(Balance::new(
894+
date(2024, 6, 15),
895+
"Assets:A",
896+
Amount::new(dec!(0), "USD"),
897+
)),
898+
Directive::Pad(Pad::new(date(2024, 6, 15), "Assets:A", "Equity:O")),
899+
Directive::Balance(Balance::new(
900+
date(2024, 7, 1),
901+
"Assets:A",
902+
Amount::new(dec!(50), "USD"),
903+
)),
904+
];
905+
906+
let merged = merge_with_padding(&directives);
907+
908+
let synths = merged
909+
.iter()
910+
.filter(|d| matches!(d, Directive::Transaction(t) if is_synthesized_pad(t)))
911+
.count();
912+
assert_eq!(
913+
synths, 1,
914+
"the pad must satisfy the LATER balance, producing exactly one synth"
915+
);
916+
}
917+
918+
#[test]
919+
fn test_merge_with_padding_earlier_pad_still_synthesizes_before_balance() {
920+
// The day-earlier case is the one that DOES pad, and the synth still
921+
// has to precede the balance so a mid-stream assertion check sees the
922+
// padded inventory. That ordering guarantee is what the old same-date
923+
// test was really protecting; it belongs here, where a synth exists.
924+
let directives = vec![
925+
Directive::Open(Open::new(date(2024, 1, 1), "Assets:Bank")),
926+
Directive::Open(Open::new(date(2024, 1, 1), "Equity:Opening")),
927+
Directive::Pad(Pad::new(date(2024, 1, 2), "Assets:Bank", "Equity:Opening")),
928+
Directive::Balance(Balance::new(
929+
date(2024, 1, 3),
930+
"Assets:Bank",
931+
Amount::new(dec!(1000), "USD"),
932+
)),
933+
];
934+
935+
let merged = merge_with_padding(&directives);
936+
844937
let synth_idx = merged
845938
.iter()
846939
.position(|d| matches!(d, Directive::Transaction(t) if is_synthesized_pad(t)))
847-
.expect("synth present");
940+
.expect("an earlier pad must still synthesize");
848941
let balance_idx = merged
849942
.iter()
850943
.position(|d| matches!(d, Directive::Balance(_)))
851944
.expect("balance present");
852945
assert!(
853946
synth_idx < balance_idx,
854-
"synth pad (idx {synth_idx}) must appear before Balance (idx {balance_idx}) on same date",
947+
"synth (idx {synth_idx}) must precede the Balance (idx {balance_idx})",
855948
);
856949
}
857950

‎crates/rustledger-core/src/directive.rs‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -487,10 +487,19 @@ pub enum DirectivePriority {
487487
Open = 0,
488488
/// Commodities declared before use
489489
Commodity = 1,
490-
/// Padding before balance assertions
491-
Pad = 2,
492-
/// Balance assertions checked at start of day
493-
Balance = 3,
490+
/// Balance assertions are checked at the START of the day, so they
491+
/// precede a same-date pad -- which is what makes such a pad UNUSED
492+
/// rather than applied (#2150).
493+
///
494+
/// beancount's `entry_sortkey` gives Balance `-1` and Pad the default
495+
/// `0`. Ours had Pad first, with a comment asserting "padding before
496+
/// balance assertions"; the effect was that a `pad` and the `balance`
497+
/// it targets on the same date synthesized a padding transaction
498+
/// beancount does not, leaving the account richer by the difference
499+
/// while our own validator reported the pad as unused in the same run.
500+
Balance = 2,
501+
/// Padding, after the same-date balance that would have consumed it.
502+
Pad = 3,
494503
/// Main entries
495504
Transaction = 4,
496505
/// Annotations after transactions
@@ -1744,7 +1753,10 @@ mod tests {
17441753
fn test_directive_priority() {
17451754
// Test that priorities are ordered correctly
17461755
assert!(DirectivePriority::Open < DirectivePriority::Transaction);
1747-
assert!(DirectivePriority::Pad < DirectivePriority::Balance);
1756+
// Balance BEFORE Pad: a balance is checked at the start of the day,
1757+
// so a same-date pad has nothing left to satisfy and is unused
1758+
// (#2150). This assertion previously ran the other way.
1759+
assert!(DirectivePriority::Balance < DirectivePriority::Pad);
17481760
assert!(DirectivePriority::Balance < DirectivePriority::Transaction);
17491761
assert!(DirectivePriority::Transaction < DirectivePriority::Close);
17501762
assert!(DirectivePriority::Price < DirectivePriority::Close);
@@ -1788,8 +1800,15 @@ mod tests {
17881800
}
17891801

17901802
#[test]
1791-
fn test_sort_directives_pad_before_balance() {
1792-
// Pad must come before balance assertion on the same day
1803+
fn test_sort_directives_balance_before_pad() {
1804+
// A balance is checked at the START of its day, so it precedes a
1805+
// same-date pad -- and that is exactly why such a pad has nothing left
1806+
// to satisfy and is reported unused (#2150).
1807+
//
1808+
// This test asserted the reverse, down to its name. Under that order a
1809+
// pad and the balance it targets on one date synthesized a padding
1810+
// transaction beancount does not: on the issue's fixture beancount
1811+
// reports 40.00 USD for the account and we reported 100.00.
17931812
let mut directives = vec![
17941813
Directive::Balance(Balance::new(
17951814
date(2024, 1, 1),
@@ -1805,8 +1824,8 @@ mod tests {
18051824

18061825
sort_directives(&mut directives);
18071826

1808-
assert_eq!(directives[0].type_name(), "pad");
1809-
assert_eq!(directives[1].type_name(), "balance");
1827+
assert_eq!(directives[0].type_name(), "balance");
1828+
assert_eq!(directives[1].type_name(), "pad");
18101829
}
18111830

18121831
#[test]

‎crates/rustledger-ffi-wasi/src/helpers.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,45 @@ option \"operating_currency\" \"USD\"
616616
2024-01-21 balance Assets:SomeName 42 USD
617617
";
618618

619+
/// A `pad` and the `balance` it targets on ONE date.
620+
///
621+
/// `SAME_DATE_LEDGER` above shares a date between a pad and an *unrelated*
622+
/// transaction, with the balance a day later, so it never exercised this.
623+
/// #2150 was reported against the CLI and explicitly left the FFI path
624+
/// unverified, noting that `expand_pads` runs its own date-only sort and
625+
/// could diverge either way.
626+
const SAME_DATE_PAD_BALANCE_LEDGER: &str = "\
627+
option \"operating_currency\" \"USD\"
628+
2024-01-01 open Assets:A USD
629+
2024-01-01 open Equity:Opening-balances
630+
2024-01-05 * \"some activity\"
631+
Assets:A 40.00 USD
632+
Equity:Opening-balances
633+
2024-06-15 pad Assets:A Equity:Opening-balances
634+
2024-06-15 balance Assets:A 100.00 USD
635+
";
636+
637+
#[test]
638+
fn expand_pads_synthesizes_nothing_for_a_same_date_pad_balance() {
639+
// beancount checks a balance at the start of its day, so a same-date
640+
// pad has nothing to satisfy and is reported unused. The FFI path must
641+
// agree with the CLI here: before #2150 both padded, leaving the
642+
// account 60.00 richer than beancount reports.
643+
let load = load_source(SAME_DATE_PAD_BALANCE_LEDGER);
644+
let (expanded, _lines) = expand_pads(load.directives, load.directive_lines, &0u32);
645+
646+
let synths = expanded
647+
.iter()
648+
.filter(|d| {
649+
matches!(d, Directive::Transaction(t) if rustledger_booking::is_synthesized_pad(t))
650+
})
651+
.count();
652+
assert_eq!(
653+
synths, 0,
654+
"a same-date pad+balance must synthesize nothing on the FFI path too",
655+
);
656+
}
657+
619658
#[test]
620659
fn expand_pads_does_not_displace_unrelated_same_date_directives() {
621660
let load = load_source(SAME_DATE_LEDGER);

‎crates/rustledger-wasm/src/cache.rs‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,22 @@ use crate::types::{Error, LedgerOptions};
7777
/// #2160 holds the two-field-shorter `ArchivedNote`, one written by a dev
7878
/// build after it holds the new shape, and both claim v15. Rejecting every
7979
/// v15 blob is the only reading that is safe for either.
80-
pub const CACHE_VERSION: u32 = 16;
80+
/// v17: `Pad` and `Balance` swap `DirectivePriority`, so a same-date pad and
81+
/// balance come out of the pipeline in the other order (#2179). Loader v32,
82+
/// unchanged -- and this is the first entry here that moves neither the
83+
/// parser's output nor an archived layout. Every directive archives to the
84+
/// same bytes; the LIST this cache holds is a different list. `LedgerPayload`
85+
/// stores the output of `process()`, which sorts by `canonical_sort_key`, and
86+
/// that key reads `DirectivePriority`.
87+
///
88+
/// What a stale blob costs is narrower than it first looks, and worth stating
89+
/// exactly: `process_pads` re-sorts its input by `booking_sort_key`, which is
90+
/// the same `(date, priority)`, so pad AMOUNTS computed from a v16 blob would
91+
/// still come out right. What differs is the order of the directives this
92+
/// cache hands back, which is what every order-sensitive consumer reads --
93+
/// journal output, `SELECT` row order, the FFI directive stream. A cache that
94+
/// returns a different sequence than a fresh run is stale by definition.
95+
pub const CACHE_VERSION: u32 = 17;
8196

8297
/// The `rustledger-loader` cache version this one was last reconciled with.
8398
///
@@ -319,16 +334,19 @@ mod tests {
319334

320335
// Every variant, or the pin has a blind spot exactly where a new
321336
// variant would be added.
322-
let kinds: std::collections::BTreeSet<_> = parsed
337+
// `Directive::type_name`, not `Debug` on `mem::Discriminant`: that
338+
// format is not a documented guarantee, and a std that printed every
339+
// discriminant identically would turn this coverage check into a
340+
// baffling failure. `type_name` also names the missing variant.
341+
let kinds: std::collections::BTreeSet<&str> = parsed
323342
.directives
324343
.iter()
325-
.map(|d| std::mem::discriminant(&d.value))
326-
.map(|d| format!("{d:?}"))
344+
.map(|d| d.value.type_name())
327345
.collect();
328346
assert_eq!(
329347
kinds.len(),
330348
12,
331-
"fixture must cover all 12 Directive variants"
349+
"fixture must cover all 12 Directive variants, got {kinds:?}"
332350
);
333351

334352
// FNV-1a: a checked-in constant needs an algorithm that is stable
@@ -374,7 +392,11 @@ mod tests {
374392
/// and a priority swap changes nothing here.
375393
#[test]
376394
fn processed_ledger_archived_form_is_pinned() {
377-
const PROCESSED_LAYOUT_HASH: u64 = 0x051d_b37b_7095_87c5;
395+
// Moved once so far: swapping `Pad` and `Balance` in
396+
// `DirectivePriority` (#2179) reorders the same-date pair in the
397+
// fixture. That is the change this test was written to catch, and it
398+
// caught it on the rebase rather than in review.
399+
const PROCESSED_LAYOUT_HASH: u64 = 0xa8a8_b0cf_5355_f95d;
378400

379401
let src = "\
380402
2024-01-01 open Assets:Bank USD\n\
@@ -390,6 +412,9 @@ mod tests {
390412
2024-01-06 balance Assets:Bank 15.00 USD\n";
391413

392414
let processed = crate::helpers::load_and_book(src);
415+
for d in &processed.directives {
416+
eprintln!("PROBE {} {:?}", d.type_name(), d.date());
417+
}
393418
assert!(
394419
processed.directives.len() >= 6,
395420
"fixture must survive the pipeline, else the digest pins an early \

0 commit comments

Comments
 (0)