fix(replay): match replayed calls by position, not by value (audit theme 8) by passcod · Pull Request #144 · beyondessential/seedling

passcod · 2026-08-02T02:00:44Z

Closes cross-cutting theme 8 from the logic bug audit: restart/replay correctness around persisted state. See the pattern analysis.

The class

Seedling persists state so a crash can be recovered from. In every affected finding the write path and the restart read path were built and tested separately, and the bug lives exactly at their seam — none of them had a test that severs in-memory state between the write and the read.

Positional replay — H7

The action log is positional: call_index walks committed in the order the closure makes its calls. do_exec always understood that. do_signal did not — it scanned the whole log for any entry with the same (resources, signal). Both halves of that are wrong:

A second, identical rt.signal later in the same closure matched the first entry and was swallowed, so the signal was never delivered. "At most once" is per call site, not per value.
When the resolved instance set differed between passes — a replica added or retired — nothing matched, so a signal already delivered before the crash was delivered again.

ReplayContext::replay_step(expect) is the way to ask now: it returns the committed entry at this position or None for a first run, verifies the call kind, and advances the index as part of consuming the entry — so a caller cannot check without advancing or advance without checking. A log that does not correspond to the calls being made fails rather than guessing.

`INSERT OR REPLACE` on rows with independently-owned columns

INSERT OR REPLACE is delete-then-insert: every column the statement does not name reverts to its default. That is safe only while one writer owns the whole row — and it stops being safe the moment a migration adds a column another writer owns. save_current_operation predated cancel_requested (added by v34) and listed columns explicitly, so it reverted the flag to 0. The replay path re-saves the row immediately after the daemon has read the cancel flag, silently dropping a cancellation while the operation it cancelled was still in flight.

The failure arrived with the migration, not with the statement, so reviewing the statement alone could never have caught it. Every site is now ON CONFLICT ... DO UPDATE naming the columns that writer owns — including the five that list every column today and so have no live bug, because that is exactly the state current_operation was in before v34.

Stamping an effect that does not exist yet

check_due_schedules called upsert_schedule_fired for both Accepted and Queued. A queued fire exists only in the scheduler's in-memory VecDeque, so a restart lost the operation while the database said it had fired — and r[schedule.catch-up] then had nothing to catch up on. Only accepted fires are stamped now. While a fire stays queued the next tick's re-fire is rejected as SameAppAlreadyQueued and silently dropped, so leaving it unstamped cannot double-fire; after a restart the unstamped schedule fires again, which is the point.

Findings closed

Finding	Severity
H7 — `rt.signal` replay dedup is value-based, not positional	high
`save_current_operation` silently resets a persisted cancel request	medium
Queued schedule fire stamped as fired but lost on restart	low

Enforcement

Spec: r[barrier.replay.positional] (a replayed call is matched to the entry at its own position, never to a similar-looking one elsewhere; at-most-once is per call site; a diverged log fails rather than guesses) and r[history.persist.partial-update] (a writer must not reset columns it does not own, and must continue not to as columns are added).
Tests: two identical calls consume two entries and a third runs; a changed instance set is still the same call; a diverged log errors with the position and both kinds. The cancel-preservation test was checked against the old statement and fails there.
CI: etc/ci/check-insert-or-replace.sh, allowlisting only action_log — whose positional overwrite keyed on (operation_id, call_index) is the replay contract — and exempting migrations, which run once against a known schema.

Not in scope, and the honest gap

The theme's fourth finding — dynamic resources preserved for a replay that is then abandoned — is not fixed here. Startup orphan cleanup deliberately skips dynamic_resources rows matching the persisted operation, expecting replay_interrupted_operation to adopt them; but every abort branch in that function just clears the row and returns, so nobody stops the preserved units or deletes their rows, and the reconciler ignores them by design. Fixing it means extracting those abort branches from crates/daemon/src/main.rs into core behind an injected teardown, which is a real refactor of daemon startup rather than part of this one.

That is also the finding that most wants the RestartWorld harness the analysis describes — a harness that rebuilds all in-memory state between passes, keeping only the DB and the world. The existing run_operation tests exercise suspension, not restart: they reuse the same engine, scope, App and registry across passes, so no in-memory state is ever dropped. The tests here are unit-level on replay_step and the DB, which pin the two mechanical rules but not the operation-level restart behaviour. The harness is the element that generalises to the next persisted-state feature and is the natural follow-up.

The other do_* calls (do_write, do_start, do_stop, do_query, record_subaction_entry) and check_barrier's already_satisfied lookup are not yet ported to replay_step, so committed stays public for now — making it private is what would prevent a value-based scan being reintroduced, and it should follow once they are.

Overlap with other themes

Independent — sits on main. It touches INSERT OR REPLACE in oi/handler/apps.rs, which theme 2 (#138) also edits nearby; expect a small merge.

Generated by Claude Code

…ared rows do_signal scanned the whole committed log for an entry with the same resources and signal. Both halves of that are wrong. A second, identical rt.signal later in the same closure matched the first entry and was swallowed, so the signal was never delivered; and when the resolved instance set changed between passes — a replica added or retired — nothing matched, so a signal already delivered before the crash was delivered again. The log is positional, as do_exec already understood. ReplayContext::replay_step makes that the way to ask, and advancing the index is part of consuming the entry, so a caller cannot check without advancing or advance without checking. INSERT OR REPLACE is delete-then-insert: every column the statement does not name reverts to its default. save_current_operation predated the cancel_requested column, so the replay path re-saving the row silently dropped a cancellation the daemon had just read. The failure arrived with the migration, not with the statement. Every site is an ON CONFLICT DO UPDATE naming its own columns now, and a CI guard keeps the exception to action_log, whose positional overwrite is the replay contract. A queued schedule fire was stamped last_fired_at although it existed only in the scheduler's in-memory queue, so a restart lost the operation while the database said it had fired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

github-code-quality · 2026-08-02T02:02:20Z

Code Coverage Overview

Languages: TypeScript, Rust

TypeScript / code-coverage/vitest

The overall coverage in commit 33efdd3 in the claude/pr-115-theme-... branch remains at 66%, unchanged from commit c2f174b in the main branch.

Rust / code-coverage/rust

The overall coverage in commit 33efdd3 in the claude/pr-115-theme-... branch remains at 59%, unchanged from commit c2f174b in the main branch.

Show a code coverage summary of the most impacted files.

File	`main` c2f174b	`claude/pr-115-theme-...` 33efdd3	+/-
`crates/core/src...rier/runtime.rs`	70%	70%	0%
`crates/core/src...time/history.rs`	82%	82%	0%
`crates/core/src...handler/apps.rs`	75%	75%	0%
`crates/core/src/oi/server.rs`	60%	60%	0%
`crates/core/src...runtime/apps.rs`	89%	89%	0%
`crates/core/src...me/schedules.rs`	77%	77%	0%
`crates/core/src...time/desired.rs`	97%	97%	0%
`crates/core/src...ecret_params.rs`	92%	92%	0%
`crates/core/src...rrier/oracle.rs`	80%	81%	+1%
`crates/core/src...time/barrier.rs`	74%	85%	+11%

_{Updated August 02, 2026 05:19 UTC}

Copilot

Pull request overview

This PR closes audit theme 8 findings by tightening restart/replay correctness: replayed runtime calls are consumed positionally (per call site), and SQLite writes avoid INSERT OR REPLACE patterns that can reset independently-owned columns as schemas evolve.

Changes:

Introduces ReplayContext::replay_step(...) and ports rt.signal replay behaviour from value-scanning to positional consumption.
Replaces multiple INSERT OR REPLACE statements with INSERT ... ON CONFLICT ... DO UPDATE to avoid resetting unrelated columns.
Adds spec + tests for the new replay/persistence rules and adds a CI guard (check-insert-or-replace.sh) to prevent regressions; fixes schedule “last fired” stamping to only occur for accepted fires.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`etc/ci/check-insert-or-replace.sh`	New CI guard to block `INSERT OR REPLACE` outside an allowlist.
`docs/spec/runtime.md`	Adds spec requirements for positional replay matching and partial-row updates.
`crates/core/src/runtime/schedules.rs`	Stops stamping `last_fired_at` for queued schedule fires (only stamp accepted).
`crates/core/src/runtime/scaling.rs`	Switches scaling decision persistence to `ON CONFLICT ... DO UPDATE`.
`crates/core/src/runtime/history/tests.rs`	Adds regression test ensuring cancel flags survive operation re-save.
`crates/core/src/runtime/history.rs`	Fixes `save_current_operation` to upsert only owned columns (preserve `cancel_requested`).
`crates/core/src/runtime/desired.rs`	Switches dynamic resource inserts to `ON CONFLICT ... DO UPDATE`.
`crates/core/src/runtime/barrier/runtime.rs`	Updates `rt.signal` replay logic to use positional replay consumption.
`crates/core/src/runtime/barrier/replay/tests.rs`	Adds tests for positional replay semantics and mismatch detection.
`crates/core/src/runtime/barrier.rs`	Adds `ReplayMismatch` and implements `ReplayContext::replay_step`.
`crates/core/src/runtime/apps/secret_params.rs`	Switches secret param upsert to `ON CONFLICT ... DO UPDATE`.
`crates/core/src/runtime/apps/params.rs`	Switches param upsert to `ON CONFLICT ... DO UPDATE`.
`crates/core/src/runtime/apps.rs`	Switches `registered_apps` persistence to `ON CONFLICT ... DO UPDATE`.
`crates/core/src/oi/handler/apps.rs`	Switches OI-side `registered_apps` persistence to `ON CONFLICT ... DO UPDATE`.
`.github/workflows/rust.yml`	Adds CI step to run the new `INSERT OR REPLACE` guard.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+            match g.replay_step(CallKind::Signal) {
+                Ok(Some(_)) => return Ok(()),
+                Ok(None) => {}
+                Err(mismatch) => return Err(Box::<EvalAltResult>::from(mismatch.to_string())),
            }


+        if !self.is_replaying() {
+            return Ok(None);
+        }
+        let entry = self.committed[self.call_index].clone();


Review catch. Positional matching alone treats a script edited from SIGHUP to SIGTERM at the same position as already replayed, so the new signal is never delivered. The signal name is a literal in the script and so is stable across passes, unlike the resolved instance set — which legitimately varies when a replica is added or retired and must not be checked this way. replay_step takes the expected argument where the caller has one that is stable by construction, and ReplayMismatch distinguishes the two divergences. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

passcod · 2026-08-02T02:10:26Z

Both correct. One fixed in 2b1ea92, one I'm deliberately not doing blind.

Signal name not validated — fixed. Right, and it's the mirror of the bug this PR is about: positional matching alone would treat a script edited from SIGHUP to SIGTERM at the same position as already replayed and never deliver the new one. The distinction that matters is which arguments are stable across passes by construction. The signal name is a literal in the script text, so a change means the script changed under the log. The resolved instance set is not — a replica may be added or retired between passes and it is still the same call — which is exactly why value-matching on it was wrong in the first place. replay_step now takes the expected argument where the caller has a stable one, ReplayMismatch distinguishes the two divergences, and there's a test for each direction (a changed name diverges; the recorded name replays cleanly).

Under-consumption — real, but not a change I should make from here. You're right that nothing detects the opposite divergence: a pass that takes a different branch and makes fewer calls leaves committed entries unconsumed and completes without error. The obvious check is call_index == committed.len() at the end of the pass, and if every termination were a clean completion I'd add it.

They aren't. An operation that errors mid-closure, one that is cancelled, and one that suspends on a new barrier before reaching the old log's end all legitimately leave entries unconsumed — and I can't tell from here which of those should be treated as divergence and which are normal. Getting that wrong turns a recoverable operation into a hard failure at exactly the moment recovery matters, which is worse than the gap. It also wants the RestartWorld harness this PR's description already flags as missing: without a test that severs in-memory state between passes, a completeness assertion is being added on faith.

So: worth doing, wants a decision about which termination paths must have consumed the log, and belongs with the harness rather than bolted on here. Happy to take it as a follow-up if you tell me how you'd want the error and cancel paths treated.

Generated by Claude Code

matches! on the error checks the classification; the message is what an operator actually reads when an operation refuses to resume, and nothing exercised it — assert!(..., "{err}") only formats on failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

crates/core/src/runtime/barrier.rs:172

ReplayMismatch::Extra's Display message is grammatically incorrect ("making one of …") and reads oddly for a single expected value. This makes replay divergence errors harder to understand when they occur.

            } => write!(
                f,
                "replay diverged at call {call_index}: the log records a {kind:?} of {found:?} \
                 but the script is making one of {expected:?}"
            ),

etc/ci/check-insert-or-replace.sh:35

The CI check can miss real INSERT OR REPLACE usages if the SQL string is split across lines (e.g. a line containing "INSERT OR REPLACE without INTO will currently be treated as prose because it contains "). That creates a bypass/false-negative in the enforcement script.

    # Prose mentioning the statement (comments, test assertions) is not a use.
    line="${hit#*:}"
    line="${line#*:}"
    [[ "$line" =~ (//|--|\") ]] && [[ ! "$line" =~ INSERT\ OR\ REPLACE\ INTO ]] && continue
    skip=false

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (1)

etc/ci/check-insert-or-replace.sh:34

The prose-skip heuristic can create false negatives: a real SQL string can contain INSERT OR REPLACE on one line and INTO on the next (or have -- in the SQL), which makes this line get skipped and the guard won’t catch a new unsafe statement. It’s safer to only skip obvious Rust comments and known test-assertion strings, rather than relying on INSERT OR REPLACE INTO being on the same line.

    # Prose mentioning the statement (comments, test assertions) is not a use.
    line="${hit#*:}"
    line="${line#*:}"
    [[ "$line" =~ (//|--|\") ]] && [[ ! "$line" =~ INSERT\ OR\ REPLACE\ INTO ]] && continue

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (2)

etc/ci/check-insert-or-replace.sh:34

The prose-skipping filter treats any line containing a double quote ("") as non-code unless it also contains INSERT OR REPLACE INTO. That means a real use can evade the check if the SQL is constructed in parts (e.g. push_str("INSERT OR REPLACE")) or otherwise doesn’t include INTO on the same line. Tighten the skip to only ignore actual comment lines and known test-assertion strings, so legitimate code hits are always flagged.

    [[ "$line" =~ (//|--|\") ]] && [[ ! "$line" =~ INSERT\ OR\ REPLACE\ INTO ]] && continue

crates/core/src/runtime/barrier.rs:172

The ReplayMismatch::Extra display message is slightly confusing for operators: it prints the recorded value as Some("...")/None and says “making one of …”. Since this message is the primary explanation for why an operation refused to resume, it should read clearly and show the recorded argument without the Option wrapper.

            } => write!(
                f,
                "replay diverged at call {call_index}: the log records a {kind:?} of {found:?} \
                 but the script is making one of {expected:?}"
            ),

Skipping any line with a quote unless INTO was on that same line meant a statement whose INTO wrapped to the next line went unseen — a guard that only catches the formatting it was written against. Skip genuine comment lines only; the one assertion message that names the statement is now an explicit allowlist entry rather than an accident of the filter. Also print the recorded argument in a replay divergence rather than its Option wrapper: that message is the whole account an operator gets of why an operation refused to resume. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

Both add a guard step to the lint job, so the two land side by side. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

Copilot AI review requested due to automatic review settings August 2, 2026 02:00

Copilot started reviewing on behalf of passcod August 2, 2026 02:01 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 2, 2026 02:10

Copilot started reviewing on behalf of passcod August 2, 2026 02:10 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 2, 2026 02:15

Copilot started reviewing on behalf of passcod August 2, 2026 02:15 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

passcod mentioned this pull request Aug 2, 2026

docs: rules to stop the audit's failure modes recurring #145

Draft

merge: bring theme up to main (v0.8.0)

b213f72

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

Copilot AI review requested due to automatic review settings August 2, 2026 03:00

Copilot started reviewing on behalf of passcod August 2, 2026 03:01 View session

Copilot AI reviewed Aug 2, 2026

View reviewed changes

claude added 2 commits August 2, 2026 03:28

merge: bring theme 8 up to main (theme 1)

33efdd3

Both add a guard step to the lint job, so the two land side by side. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HtxQdsF6YhLzz9RrDmHEDv

Navigation Menu

Uh oh!

fix(replay): match replayed calls by position, not by value (audit theme 8) - #144

fix(replay): match replayed calls by position, not by value (audit theme 8)#144
passcod wants to merge 6 commits into
mainfrom
claude/pr-115-theme-8-restart-replay

passcod commented Aug 2, 2026

Uh oh!

github-code-quality Bot commented Aug 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

passcod commented Aug 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

passcod commented Aug 2, 2026

The class

Positional replay — H7

INSERT OR REPLACE on rows with independently-owned columns

Stamping an effect that does not exist yet

Findings closed

Enforcement

Not in scope, and the honest gap

Overlap with other themes

Uh oh!

github-code-quality Bot commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/vitest

Rust / code-coverage/rust

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

passcod commented Aug 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`INSERT OR REPLACE` on rows with independently-owned columns

github-code-quality Bot commented Aug 2, 2026 •

edited

Loading