Skip to content

fix(session): re-derive from scratch on steps without a delta callback - #2089

Open
justinjoy wants to merge 1 commit into
mainfrom
claude/1994-idle-replay
Open

justinjoy wants to merge 1 commit into
mainfrom
claude/1994-idle-replay

Conversation

@justinjoy

Copy link
Copy Markdown
Collaborator

Fixes #1994.

Problem

wirelog_easy_step() on a session with no delta callback re-derived every rule on every step, on top of the rows it had already derived:

  • Each step appended another copy of every derived row (3 rows became 6, 9, then 12).
  • A removal never retracted the rows it had derived.
  • A step with nothing pending re-ran every rule, so a value-position @call ran once per derived row on each idle step.
  • A snapshot taken afterwards emitted that state. This is the "step then snapshot duplicates rows" pattern that wirelog-easy.h warned about.

Fix (col_session_step_impl)

  • An idle step returns early with or without a callback.
  • A plain step clears the derived relations and resets every stratum frontier before re-deriving everything, under the same has_evaluated condition the snapshot path uses. Without the frontier reset, a recursive closure lost (1,4). The full mask is forced even when the pending input recorded only some strata.
  • The step sets pending_full_input_eval before clearing, so a failed step leaves a full re-evaluation pending.
  • A plain step no longer seeds $r$ retraction deltas left by a removal made with a callback installed.

Rule-head input

Clearing derived rows also cleared input: inline facts (reach(1). beside a rule for reach) and host rows inserted into a rule head. The snapshot path already had this defect on main: a second snapshot after an insert returned reach empty, and so did four-worker recursive steps.

A rule head now keeps its input in a private $in$<name> shadow:

  • Created on the first insert into the rule head. Inline facts are loaded through that same insert.
  • Inserts and removals keep the shadow in step. A removal planned before the relation is touched is applied after the commit; it is applied even when the relation no longer holds the row, and in that case it schedules nothing.
  • Restored after both resets: col_session_clear_idb_rows and the multi-worker recursive reset in eval.c.

Placement and cost:

  • Creating shadows at session creation would raise the creation memory floor that test_session pins. Creating them at the first evaluation would leave a reservation behind after a refused step.
  • A direct-mapped identity cache keeps inserts into plain input relations at main's speed. 1M single-row inserts: 0.51 s vs 0.51 s; alternating two relations: 0.53 s vs 0.52 s. Without the cache it was 0.71–0.74 s.

Tests

  • New test_easy_plain_step (15 cases):
    • idle steps;
    • two-strata inserts and removals;
    • a recursive extend and cut;
    • insert and removal transitions when the callback is cleared;
    • inline facts and host rows in rule heads, on 1 and 4 workers, and via snapshots alone;
    • removals with a callback installed, and after it is cleared;
    • a row inserted twice and removed twice.
  • test_extension_replay: no-callback addon counts, a failing plain step followed by a snapshot, and an input-only removal that invokes nothing in another stratum.
  • Mutation checks: 14 mutants, each fails a named check.

Docs

  • wirelog-easy.h documents both step modes and drops the step-then-snapshot warning.
  • docs/SEMANTICS.md specifies the no-callback invocation count and updates its state map.
  • Example 12's comment and README are corrected. Checked on that program, step then snapshot returns the same five rows.
  • CHANGELOG has two Fixed entries.

Validation

  • meson test -C build: 433 OK, 0 failed (before rebase).
  • ASAN/UBSAN on both focused tests, and TSan on test_easy_plain_step including the 4-worker cases.
  • After rebasing onto 84487d59, the abi suite has one failure: python_encoding flags scripts/perf/hosted_perf_calibration.py, which 84487d59 added. This PR does not touch it.

Not changed

These behave the same on main:

  • With a delta callback:
    • the first step publishes an inline fact as a retraction and leaves the relation empty;
    • a host row inserted into a rule head is also published as a retraction.
  • If a session's very first evaluation fails partway, the next evaluation does not clear, because nothing has been evaluated yet.

Peer review: an independent reviewer did four rounds. Rounds 1–3 raised blocking findings, which are fixed here: failure recovery, staged removals, rule-head input loss, doc wording, and an over-eager re-evaluation on input-only removals. The final tree had no blocking findings.

A session driven by wirelog_easy_step() with no delta callback
installed re-derived every rule on every step, on top of the rows the
previous evaluation had already derived. Every step appended another
copy of each derived row. A removal never retracted the rows it had
derived. A step with nothing pending re-ran every rule, so a
value-position @call ran once per derived row on each idle step. With
three `ready` rows, the derived relation held 3, 6, 9 and 12 rows after
one to four steps. A snapshot taken afterwards emitted that state as
the stable model, which is the "step then snapshot duplicates rows"
pattern wirelog-easy.h warned about.

Inserts and removals made without a callback already force a full
evaluation, so a plain step is meant to re-derive everything. It now
does so correctly:

- The early return for a step with nothing pending no longer requires
  a delta callback. Every write entry point that changes input sets
  pending_input_change, and a new session starts with it set.
- A plain step evaluates every stratum. Once the session has evaluated,
  it first clears the derived relations with col_session_clear_idb_rows
  and resets every stratum frontier, under the same has_evaluated
  condition the snapshot path uses before a full re-evaluation. Without
  the stratum reset, the transitive closure in the new test lost (1,4)
  after an edge was added. A resumed step keeps what its first attempt
  already merged.
- The full mask is forced even when the pending input recorded only
  some strata, as an insert staged with a callback that is then removed
  does. Clearing alone would leave the other strata empty.
- Before clearing, the step sets pending_full_input_eval, which only
  its commit resets. If the step then fails, the next snapshot
  evaluates in full instead of re-deriving only the strata the pending
  input reaches over the emptied relations.
- A plain step no longer seeds retraction deltas ($r$<name>) left by a
  removal staged with a callback. Seeded, the first iteration read the
  removed rows in place of the relation and re-derived paths through
  them.

Clearing derived rows also cleared input. A rule head can also hold
input: an inline fact such as `reach(1).` beside
`reach(y) :- reach(x), edge(x, y).`, or a host row inserted into it.
It lost that input whenever a full re-evaluation reset it. On main a
second snapshot after an insert already returned `reach` empty, and
four-worker recursive steps did too. A rule head now keeps its input
rows in a private `$in$<name>` shadow:
- The first insert into a rule head registers its shadow; inline facts
  are loaded through that same insert. Creating shadows at session
  creation would raise the creation memory floor that test_session
  pins, and creating them when the first evaluation starts would leave
  a reservation behind after a refused step.
- A direct-mapped cache of relation identities answers "not a rule
  head" for plain input relations. Without it, a million single-row
  inserts took 0.71 s against main's 0.51 s; with it the two match
  within noise, also when two relations alternate.
- Inserts write the shadow first and truncate it if the relation's own
  append fails.
- Removals plan the shadow change before the relation is touched and
  apply it once the removal has committed. The shadow records input, so
  a removal applies to it even when the relation no longer holds the
  row; a callback-mode step can already have dropped it. Such a removal
  leaves the model as it is, so it schedules no re-evaluation.
- wl_columnar_session_restore_seed appends the shadow back after both
  resets: col_session_clear_idb_rows and the multi-worker recursive
  reset in eval.c.

Tests:
- New test_easy_plain_step:
  - idle steps leave one copy of each row;
  - inserts and removals in two strata hold exactly the model;
  - a recursive closure is extended and then cut;
  - a step after removing the callback re-derives everything, after a
    staged insert and after a staged removal;
  - inline facts survive plain steps on one and four workers, and
    snapshots alone;
  - host rows in a rule head without inline facts survive the same
    three modes;
  - host rows and an inline fact removed with a callback installed, or
    after it was removed, stay removed;
  - a host row inserted twice is gone after two removals.
- test_extension_replay:
  - a no-callback case pins five invocations to load, zero per idle
    step, five for an insert into an unread relation and four after a
    retraction. Snapshots in that case leave the invocation counter
    unchanged, so they read the stable model rather than repairing it.
  - a plain step whose addon fails after the clear is followed by a
    snapshot that returns the full model.
  - removing input the model already lacks, with a callback installed,
    invokes no addon in another stratum.
- Mutation checks: each of fourteen mutants fails a named check:
  - restore the callback requirement;
  - drop the clear, the stratum reset, the forced mask, or
    pending_full_input_eval;
  - re-enable the retraction seed;
  - skip the shadow on insert, on its creation, on plain removal, on
    the incremental removal hit or miss, on restore, or in the eval.c
    reset;
  - schedule a re-evaluation for an input-only removal.

Docs:
- wirelog-easy.h documents both step modes and drops the warning
  against a snapshot after a step.
- docs/SEMANTICS.md specifies the no-callback invocation count. Its
  publication-cutoff state map now lists col_session_step_impl as a
  reader of has_evaluated, and session_seed_shadow_truncate as a reader
  and writer of base_nrows. A selftest anchor that the first edit made
  ambiguous now names the one row it edits.
- Example 12's comment and README no longer claim that step followed
  by snapshot double-counts; checked on that program, it returns the
  same five rows.
- CHANGELOG.

Not changed: with a delta callback, a rule head's own input is
mishandled. The first step publishes an inline fact as a retraction and
leaves the relation empty, and a host row inserted into a rule head is
also published as a retraction. Main does the same.

Fixes #1994

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scalar @call replays on every step when no delta callback is installed

1 participant