Skip to content

Feat/scenarios/bounded persist - #10

Open
bakasura980 wants to merge 11 commits into
feat/bisonfi-supportfrom
feat/scenarios/bounded-persist
Open

Feat/scenarios/bounded persist#10
bakasura980 wants to merge 11 commits into
feat/bisonfi-supportfrom
feat/scenarios/bounded-persist

Conversation

@bakasura980

@bakasura980 bakasura980 commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Greptile Summary

The PR adds bounded scenario persistence while retaining the legacy boolean form and updates scheduling so persistence survives clock jumps without overwriting authored timeline transitions.

  • Adds persist: { slots: N } with validation and countdown behavior.
  • Distinguishes scheduler-generated continuations from operator-authored timeline entries.
  • Updates the generated Node binding, RPC documentation, protocol guidance, and regression tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/core/src/surfnet/svm.rs Implements bounded continuation countdown, overdue-slot claiming, authored-entry precedence, and cancellation cleanup; the previously reported timeline and cancellation failures are covered by the current guards and regression tests.
crates/types/src/scenarios.rs Introduces the backward-compatible untagged Persist contract and tests its accepted forms and countdown semantics.
crates/sdk-node/surfpool-sdk/kit/generated/OverrideInstance.ts Updates the generated Node contract to expose bounded persistence consistently with Rust serialization.
crates/types/src/rpc_endpoints.json Documents the bounded persistence shape, zero-slot rejection, cancellation behavior, and recommended usage.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Register override] --> B[Schedule target slot]
  B --> C[Materialize due override]
  C --> D{Persistence}
  D -->|false or window exhausted| E[Stop]
  D -->|true or slots remain| F{Authored entry in next slot?}
  F -->|Yes| E
  F -->|No| G[Queue re-armed continuation]
  G --> C
Loading

Reviews (10): Last reviewed commit: "fix: suppress a continuation the same ba..." | Re-trigger Greptile

Context used (3)

@github-actions

Copy link
Copy Markdown
  • High — “Stopping” an active persisted override does not work. svm.rs:4300 only replaces overrides queued for the newly registered base slot. After an override has run, its persisted copy is queued at base_slot + 1; re-registering with persist: false schedules/materializes a one-shot at the current base slot but leaves that future copy intact. It resumes next slot. Cancellation should also locate/remove or replace future queued copies. The test only cancels before initial materialization, so it misses this case.

  • Medium — { "slots": 0 } still applies once. scenarios.rs:518 treats every Slots value as enabled. Although zero does not re-arm, the initially scheduled override is still applied, contradicting “applies in N slots total.” Reject zero during deserialization/construction or define/document it as one application.

Comment thread crates/core/src/surfnet/svm.rs
Comment thread crates/types/src/scenarios.rs
@github-actions

Copy link
Copy Markdown
  • High — cancellation misses re-armed copies (svm.rs:4300): registration replaces matches only at the newly calculated slot. A persisted copy already queued in another future slot survives and continues re-arming. Cancellation must remove/replace matching queued copies across relevant future slots.

  • Medium — generated Node contract is stale (OverrideInstance.ts:41): persist remains boolean, so typed clients cannot use { slots: number }. Regenerate and commit the bindings.

  • Low — zero-slot windows are accepted (scenarios.rs:504): { "slots": 0 } is enabled and still applies once, contradicting the stated total-slot semantics. Reject zero during deserialization or define/document it explicitly.

@github-actions

Copy link
Copy Markdown
  • [P1] Cancellation only replaces the bucket computed during re-registration (svm.rs:4310). If the override already re-armed into the next slot, registering persist: false for the current slot leaves that future copy alive. The cancellation test only cancels before initial materialization, so it misses this case. Remove matching queued copies across future slots or explicitly target the queued slot.

  • [P1] The Node SDK contract remains persist?: boolean (OverrideInstance.ts:41), so TypeScript consumers cannot pass { slots: N }. Regenerate and commit the bindings.

  • [P2] { "slots": 0 } is accepted and considered enabled (scenarios.rs:515), causing one application despite requesting zero slots. Reject zero during deserialization/schema validation or define it as disabled.

@failfmi

failfmi commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Let's double-check what happens with persisting stuff and then timeTravel

@failfmi
failfmi self-requested a review August 25, 2026 13:33
Comment thread crates/core/src/surfnet/svm.rs Outdated
@github-actions

Copy link
Copy Markdown
  • High — Future cancellation can be overwritten (svm.rs:4528): cancellation removes matching copies only after the cancellation slot. An earlier persistent copy can later re-arm into that slot, replace the persist: false entry, and continue indefinitely. Preserve the cancellation as a tombstone or cap/remove earlier matching persistent copies.

  • Medium — Clock jumps can apply one override multiple times (svm.rs:2806): overdue buckets are concatenated without deduplicating (id, account, template_id). Matching copies from different slots therefore execute multiple times at the reached slot, contrary to the documented one-application-per-slot behavior. Deduplicate with the latest scheduled entry winning before materialization.

@github-actions

Copy link
Copy Markdown
  • P1 – Future cancellation stops persistence immediately (svm.rs:4542): scheduling persist: false at relative slot N removes the armed copy for the intervening slots. The override therefore disappears until slot N, rather than persisting through N-1. Preserve/re-arm copies before the cancellation slot.

  • P2 – Failed registration can partially apply (svm.rs:4520): validation occurs while mutating storage. If a later override has slots: 0, earlier overrides remain scheduled although the RPC returns an error. Validate all overrides and absolute-slot calculations before making changes.

  • P2 – TypeScript bounded slots lose u64 precision (OverrideInstance.ts:33): Rust accepts u64, but the generated contract uses number, unlike scenarioRelativeSlot’s number | bigint. Large windows can be silently rounded. Use a bigint-safe representation or validate against Number.MAX_SAFE_INTEGER.

Comment thread crates/core/src/surfnet/svm.rs
@github-actions

Copy link
Copy Markdown
  • svm.rs:4558 remove_queued_copies_elsewhere still deletes intentional matching timeline entries created by earlier register_scenario calls. scenario_slots protects only the current registration, so registering a later same-identity entry can silently remove an earlier scheduled action.

  • svm.rs:4558 A future-dated cancellation removes the already-armed next-slot copy immediately. Consequently, persistence stops at registration time rather than continuing until the cancellation’s scenario_relative_slot; the existing test only checks that nothing remains after cancellation and misses the skipped intermediate applications.

  • svm.rs:2825 Forward-jump deduplication collapses intentional same-identity timeline steps from multiple overdue slots. If those steps modify different fields, dropping all but the latest produces a different final account state than applying the timeline in order.

@github-actions

Copy link
Copy Markdown
  • P1 — Intentional timeline entry is still overwritten (svm.rs:3225): Re-arming matches any same-identity entry in the next slot, including re_armed == false, then replaces it. A persistent step immediately before a deliberate same-ID transition deletes that transition. Only replace an existing entry when queued.re_armed; otherwise append the continuation.

  • P2 — Internal reArmed flag is client-controlled (scenarios.rs:580): The field is hidden from schemas/serialization but still deserializes from RPC input. A caller can submit "reArmed": true, causing deliberate entries to be treated as scheduler continuations and removed/deduplicated. Add skip_deserializing or reset it to false during registration.

Comment thread crates/core/src/surfnet/svm.rs Outdated
Comment thread crates/core/src/surfnet/svm.rs
@github-actions

Copy link
Copy Markdown
  • svm.rs:3007 Forward jumps bypass future cancellation/transition. All overdue buckets are removed before processing. An earlier persistent entry therefore re-arms into target_slot + 1 while its later intentional one-shot is no longer queued for collision detection. The one-shot then runs but leaves that continuation behind, so the old value returns next slot. Suppress re-arming when a later claimed intentional entry has the same identity, or remove the generated continuation when processing that entry. Add a jump-over-cancellation regression test.

@github-actions

Copy link
Copy Markdown
  • P1 – Registration is not atomic on storage errors (svm.rs): each override removes continuations and writes immediately. If a later get/store fails, earlier mutations remain despite the RPC returning an error. Stage all affected buckets and commit transactionally, or roll back on failure.

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.

2 participants