You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
template-manager: ack node dirty refs by revision so a re-enqueue during a send survives - #1235
template-manager's node_dirty_refs outbox worker (crates/l40_api/template/mgr/server/src/node_state_sync.rs) lists a batch, sends MarkFacetDirty to node-ctl without holding the database lock, then deletes the listed refs by key. A re-enqueue of the same ref in between updates that row in place, so the delete dropped it until the ref was marked again or reconciliation caught up. Now it stays pending for the next pass.
Migration db/template-manager/20261031050126_node_dirty_ref_revisions (additive, sandbox-manager's outbox shape): node_dirty_refs.revision INTEGER NOT NULL DEFAULT 0, a singleton counter node_dirty_ref_revision, and insert and update triggers that stamp each enqueue with the next value. The update trigger fires on UPDATE OF requested_at, updated_at, which every upsert sets, so a same-second re-enqueue gets a new revision.
pplx-space-template-mgr-store (template-mgr-store: add a store that owns template-manager's node_dirty_ref_revision #1429):node_dirty_refs::list_node_dirty_refs reads revision. node_dirty_refs::delete_node_dirty_refs_at_revision deletes only rows still at the listed revision; template-manager's delete_node_dirty_refs runs it in its IMMEDIATE transaction. Enqueues and MarkFacetDirty are unchanged.
Tests, template-manager: the regression test; a stale ack against a recreated key; a same-timestamp write (recursive_triggers off and on); revision exhaustion.
The round trip starts from that release's database: this catalog applies the migration and keeps it on restart, then it is reversed, re-applied and reversed again.
a_bundled_schema_is_left_untouched checks recorded revisions survive a restart.
Retired-migration tests derive history from the bundled catalog; HISTORICAL covers upgrading from the release that recorded it.
Store
pplx-space-template-mgr-store (#1429) declares the migration's node_dirty_ref_revision counter. This PR moves the outbox's list and revision-guarded delete from template-manager's db module into that store, as node_dirty_refs::list_node_dirty_refs and node_dirty_refs::delete_node_dirty_refs_at_revision, with sql! locators. template-manager's delete_node_dirty_refs keeps its IMMEDIATE transaction and runs the store's delete inside it. template-manager now depends on the store, and [rules.layers.store_users] lists it. archcheck reports that entry as the advisory layers.policy_changed, for review.
Stack
#1277 and #1429 have merged; this PR is on main. It must ship in a release after one that contains #1277, which reverses 20261031050126 when a database records it without the catalog bundling it.
nextest for template-mgr-store, template-mgr, sqlite, db-migrate: 520 of 523 and the duration check pass; both NoCoW tests pass under with-nocow-test-filesystem.sh; db-migrate's views_hide_other_substrates_rows_but_their_unique_indexes_still_arbitrate fails on main too (host postgres:16-alpine).
space-db-migrator, main and this head, one file: this head records [20260930000000, 20261026220935, 20261031050126]; main reverses only 20261031050126, keeping both outbox rows; this head re-applies it, and a re-enqueue's revision survives another up.
Kind, fresh, otel: all four template-manager databases record those versions with the column, counter and triggers. Passed with cold receipts: local.template_startup_recovery_decision_table, local.garbage_collectors_reclaim_independent_resources (was …template_cache_lru_eviction_reclaims), local.unanswered_template_deleted_after_wait, local.ecw_template_build_from_oci_boots, local.private_template_build_isolation, local.node_ctl_fence_refuses_and_reconciles, local.btrfs_send_restore_pulls_parent_template, ecw.background_process_control, ecw.cold_create_environment, ecw.warm_pool_environment. The outbox stayed empty, counters advanced, dirty marks were all ok, and template-manager logged no errors.
Not validated: a fleet rollout; environment-partition rows or a mid-send re-enqueue on Kind (the regression test covers the latter).
Severity and risk
Severity: medium. A lost ack delays a node's copy of a template-manager change until the reference is marked again or reconciliation catches up.
Risk: two small trigger updates per enqueue, in its existing transaction. A stale ack leaves the row to be resent, at-least-once delivery node-ctl already handles.
Rollout
Node-local SQLite; template-manager migrates at startup. The previous release works on the migrated schema (explicit columns; the triggers stamp its writes). Existing rows keep revision 0.
Rollback to a release containing #1277 is image-only: its startup (or migrator up) reverses 20261031050126 in one transaction, keeps every outbox row and acks by key again; 20261026220935 stays where bundled (main since #1075). A release without #1277 refuses the database (unbundled migration).
AI assistance: Claude Code (an AI agent) wrote the code, the migration, the tests and this description, and ran the validation above.
No actionable comments were generated in the recent review.
Walkthrough
The migration adds a node-wide revision counter and a revision field for dirty references. The template manager store now provides shared functions to list references and delete them only when their keys and revisions match. The server uses these functions for synchronization and acknowledgement. Migration and server tests cover migration histories, row preservation, stale acknowledgements, and revision-counter exhaustion.
Possibly related PRs
perplexityai/SPACE#1075: Tests reversal of the cache-partition migration alongside the dirty-reference revision migration.
Merge Risk: Low · up to 45324
The change makes dirty-ref acknowledgement revision-aware so a re-enqueue during a send is no longer lost. It is covered by targeted tests. The remaining risk is an unvalidated multi-node rollout and a rollback that works only to an image containing #1277, so owners should be aware before merging.
Security architecture risk: Low · up to 45324
The change prevents stale acknowledgements from clearing newer reconciliation work without adding a remote acknowledgement capability. Residual uncertainty concerns migration ordering during rollback and re-upgrade, rather than a demonstrated new attack path.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The directly affected state is the template-manager database and its pending references sent to node-ctl. The revision mechanism is node-local bookkeeping; broader tenant or environment propagation was not established by the reviewed evidence.
Trust Boundaries and Controls
inferred — The exported store API requires an existing local SQLite connection. The production worker obtains revision witnesses from that database rather than from RPC input, and the remote request omits them. The reviewed change therefore does not establish a new attacker-controlled acknowledgement or privileged reconciliation entrypoint.
Resilience and Maintainability Implications
inferred — Interruption before acknowledgement leaves durable work pending, and failed acknowledgement can cause repeat delivery rather than silent removal. Revision matching prevents that repetition from clearing newer work within a continuous counter lifetime, strengthening reconciliation failure containment.
Hardening Proposals
proposed — Validate that deployment migrators cannot carry an old acknowledgement batch across revision-schema reversal and reapplication. If migrations can occur outside service ownership, exercise that sequence with a paused sender and establish explicit quiescence or fencing.
Pre-merge checks: 5 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Bug Fixes Name A Regression Test
This PR fixes an application defect under crates/. The outbox worker in node_state_sync.rs deletes listed node_dirty_refs rows by key after the send. A re-enqueue made during the send is therefore…
In the Validation section, name the regression test by its full path. Test: acking_a_sent_dirty_ref_keeps_a_re_enqueue_made_during_the_send in crates/l40_api/template/mgr/server/src/db/tests.rs. State that it fails on the pre-fix code (…
Passed checks: 5 passed
Check name
Status
Explanation
Title check
The title uses the lowercase area prefix "template-manager:" followed by a lowercase summary word "ack", which matches the required format. It describes the main change: node dirty-ref acknowledgement…
Description Follows The Pr Template
The PR description contains all four required items, each with real content. Checked against the contributor's text in the provided description; no repository inspection was needed. 1. **## Summary*…
Weakened Safeguards Are Justified
No safeguard is removed, disabled, or weakened without a stated reason. Checks run against the reviewed range (refs/coderabbit/pre-merge-check/base..head): - Lint and attribute suppressions: No `#…
Agent Tooling Runs No New Commands
The check applies to agent plugin manifests (plugin.json, plugin.yaml), SKILL.md files, command or skill files, and guideline files. The changed-file inventory for refs/coderabbit/pre-merge-check/base…
Public-Safe Content
The diff, title, and description contain none of the content this check forbids. - I scanned all 397 added lines in the reviewed range. The scan covered URLs, personal absolute paths, AWS ARNs and 12-…
Full details: Title check
Explanation
The title uses the lowercase area prefix "template-manager:" followed by a lowercase summary word "ack", which matches the required format. It describes the main change: node dirty-ref acknowledgement now deletes by revision so a re-enqueue made during a send survives. No trailing period, ticket ID, or internal link is present.
Full details: Description Follows The Pr Template
Explanation
The PR description contains all four required items, each with real content. Checked against the contributor's text in the provided description; no repository inspection was needed. 1. ## Summary is present. It states that merging the change makes the outbox worker acknowledge dirty refs by revision, so a re-enqueue during a send stays pending. 2. ## Validation is present. It names the runs (fmt, clippy -D warnings, check-migration-versions.sh, archcheck, nextest for four crates, space-db-migrator, and Kind cold-receipt scenarios) and gives results (520 of 523 passing, one failure attributed to main, plus the listed Kind receipts). 3. ## Severity and risk is present. It states "Severity: medium" and gives a risk: two small trigger updates per enqueue, and a stale ack leading to at-least-once resend. 4. AI assistance: line is present. It names Claude Code and lists the parts it drafted: the code, the migration, the tests, and the description. The check requires presence only, so the item-by-item result is a pass.
Full details: Weakened Safeguards Are Justified
Explanation
No safeguard is removed, disabled, or weakened without a stated reason. Checks run against the reviewed range (refs/coderabbit/pre-merge-check/base..head): - Lint and attribute suppressions: No #[allow], #[expect], #![allow], or #![expect] is added in the diff. - Skipped or ignored tests: No skip, xfail, or #[ignore] is added. - CI and test configuration: The changed-file list has no .config/nextest.toml, deny.toml, clippy.toml, .gitleaks.toml, workflow, ci/kind-* file, [lints] table, AGENTS.md, CLAUDE.md, CONTRIBUTING.md, .agents/, or docs/rust/ change. - Deleted tests: No #[test] or #[tokio::test] function is removed. New test functions are added. - Assertions: The removed assertions are all in crates/l10_foundation/sqlite/src/template_manager/tests.rs. Several literal expectations, such as history == [PUBLIC_SQLITE_BASELINE], are replaced by bundled_versions(catalog). This is a derived expectation, not a deletion. The rewritten tests also add new checks, including revisions() counter and row values, outbox preservation, and refusal and atomic-rollback cases. The PR description states this design ("derive expected migration histories from each catalog's up migrations"). The catalog contents stay pinned by the existing checksum and every_listed_migration_matches_the_catalog_that_bundles_it tests. - Allowlist-style entry:ARCHITECTURE.toml adds pplx-space-template-mgr-store = ["pplx-space-template-mgr"] under [rules.layers.store_users]. This permits a new store-user edge. The PR description gives the reason: template-manager now depends on the store. It also states that archcheck reports this as advisory layers.policy_changed for review. No [rules.layers.unmoved] or allowed_edges entry is added. - Review-guidance text: No changed file is an AGENTS.md, CLAUDE.md, skill, or contributing doc, so no review-guidance text changes. Other edits are doc-comment changes in template_manager.rs. They describe a release-sequencing rule and the bundled checksum. They do not loosen a guard. The startup_recovery/tests.rs changes only switch the function path to the store module and keep the same assertions. Failing items: none.
Full details: Agent Tooling Runs No New Commands
Explanation
The check applies to agent plugin manifests (plugin.json, plugin.yaml), SKILL.md files, command or skill files, and guideline files. The changed-file inventory for refs/coderabbit/pre-merge-check/base..head contains 14 paths. None are agent manifests, skills, commands, or guideline files. They are ARCHITECTURE.toml, Cargo.lock, Rust sources and Cargo.toml files under crates/, two SQL migrations under db/template-manager/, and one new Rust module (node_dirty_refs.rs). A case-insensitive path search for plugin, SKILL.md, skills, commands, .claude, AGENTS.md, CLAUDE.md, mcp, lsp, hooks, and guidelines matched nothing. A scan of added diff lines for inline shell substitutions (a ! followed by a backtick command), hooks, mcpServers, lspServers, allowed-tools, and command entries also matched nothing. The sql! locators in node_dirty_refs.rs are Rust macros, which the check excludes. No failure condition is met.
Full details: Public-Safe Content
Explanation
The diff, title, and description contain none of the content this check forbids. - I scanned all 397 added lines in the reviewed range. The scan covered URLs, personal absolute paths, AWS ARNs and 12-digit account IDs, private-key headers, credential keywords, private IP ranges, and incident, outage, and production terms. The only hit was crates/l30_storage/template/mgr/store/Cargo.toml line 19, a # internal comment label. It names no host or URL. - The added migration SQL, Rust code, and Cargo changes contain no credentials, tokens, or hostnames. - The description references only PR numbers in the same repository (#1277, #1429, #1075) and public tooling (postgres:16-alpine, Kind). It contains no private tracker, chat, or document links. - Validation names such as local.*, ecw.*, and the Kind, fresh, and otel labels read as test scenario names. I found no evidence that they identify real clusters or internal environments. Treat this as a judgement call. - The description covers a data-retention reliability defect (a lost acknowledgement). It does not describe an unfixed security vulnerability. - The validation results are test outcomes, not raw production logs, query output, or production measurements.
Full details: Bug Fixes Name A Regression Test
Explanation
This PR fixes an application defect under crates/. The outbox worker in node_state_sync.rs deletes listed node_dirty_refs rows by key after the send. A re-enqueue made during the send is therefore lost. The fix adds revision-guarded deletes (delete_node_dirty_refs_at_revision in crates/l30_storage/template/mgr/store/src/node_dirty_refs.rs) and a migration that adds a revision column and triggers. The regression test exists in the diff. It is acking_a_sent_dirty_ref_keeps_a_re_enqueue_made_during_the_send in crates/l40_api/template/mgr/server/src/db/tests.rs. The Validation section does not meet the check: - It does not name this test. It refers only to "the regression test covers the latter" (a mid-send re-enqueue). - It does not state that the test fails without the fix and passes with it. No before/after run is reported. - It does not explain why such a test cannot be written, and it does not cite an existing test that already covers the behavior. The other tests named in the description (stale ack, same-timestamp write, revision exhaustion) are not in the Validation section. The validation runs listed (nextest counts, Kind receipts, migrator runs) do not show a fail-before/pass-after result for the bug.
Resolution
In the Validation section, name the regression test by its full path. Test: acking_a_sent_dirty_ref_keeps_a_re_enqueue_made_during_the_send in crates/l40_api/template/mgr/server/src/db/tests.rs. State that it fails on the pre-fix code (the key-based delete drops the mid-send re-enqueue) and passes with this change. If that run was not done, run it and report the result. Optionally, name the stale-ack test a_stale_dirty_ref_ack_cannot_delete_a_recreated_key with the same before/after result.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
…ing a send survives
template-manager's node_dirty_refs outbox worker lists rows, calls node-ctl
without holding the database lock, then deletes the listed refs by key alone.
An enqueue of the same ref between the list and the delete updates the row
in place, and the delete then removed that newer entry as well. The ref was
sent again only when it was marked again or node-ctl's periodic
reconciliation ran.
Add an additive template-manager SQLite migration that gives node_dirty_refs
a revision column, stamped from a node-wide counter by insert and update
triggers (the same shape sandbox-manager's outbox uses), and make the ack
delete only rows whose revision still matches the one listed. Existing rows
keep revision 0 and are acked as before. The MarkFacetDirty request sent to
node-ctl is unchanged.
The migration's node_dirty_ref_revision counter belongs to
pplx-space-template-mgr-store, which declares the table. The outbox's
list and its revision-guarded delete, the statements that read and
acknowledge the revisions, move from template-manager's db module into the
store as node_dirty_refs::list_node_dirty_refs and
node_dirty_refs::delete_node_dirty_refs_at_revision, with sql! locators.
Both take a rusqlite Connection; template-manager's delete_node_dirty_refs
keeps its IMMEDIATE transaction and runs the store's delete inside it.
template-manager now depends on the store, and [rules.layers.store_users]
lists it as the store's user.
This ships in a release after one that lists 20261031050126 among the
template-manager migrations its runner can reverse. That release returns a
database recording the migration to its own schema at startup and keeps
every outbox row, so rolling back from this release to that one is
image-only; an older release refuses such a database. The bundled up SQL
must stay byte-identical to that release's history copy, because that
release compares the recorded checksum with its copy. The
pplx-space-sqlite module doc now says that this release bundles both
migrations it lists, 20261026220935 and 20261031050126, and that a migration
ships only in a release after one that lists it, so the image a rollback
returns to can reverse it.
Tests: template-manager covers a re-enqueue during a send, a stale ack
against a deleted and recreated key, a same-timestamp write with
recursive_triggers off and on, and revision exhaustion leaving the pending
row unchanged. In pplx-space-sqlite, this catalog bundles both listed
migrations. The dirty-ref reversal tests roll back to the previous release's
catalog (this one without 20261031050126, so 20261026220935 stays applied),
and the round trip starts from that release's database so this catalog
applies the migration and keeps it on a second startup. The tests that
reverse both migrations together roll back to a catalog that bundles
neither. The bundled-schema test runs this catalog and each catalog without
one of the two, and checks that recorded revisions and an
environment-partition row survive a restart, which they would not if either
migration were reversed. The retired-migration tests derive the expected
history from the catalog they migrate with, and a new test upgrades a
database from the release that recorded the retired migration (baseline plus
that migration only) to a catalog without it, reversing it and applying
20261031050126 in one startup.
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
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.
ac-pplx
changed the title
Summary
template-manager's
node_dirty_refsoutbox worker (crates/l40_api/template/mgr/server/src/node_state_sync.rs) lists a batch, sendsMarkFacetDirtyto node-ctl without holding the database lock, then deletes the listed refs by key. A re-enqueue of the same ref in between updates that row in place, so the delete dropped it until the ref was marked again or reconciliation caught up. Now it stays pending for the next pass.db/template-manager/20261031050126_node_dirty_ref_revisions(additive, sandbox-manager's outbox shape):node_dirty_refs.revision INTEGER NOT NULL DEFAULT 0, a singleton counternode_dirty_ref_revision, and insert and update triggers that stamp each enqueue with the next value. The update trigger fires onUPDATE OF requested_at, updated_at, which every upsert sets, so a same-second re-enqueue gets a new revision.pplx-space-template-mgr-store(template-mgr-store: add a store that owns template-manager's node_dirty_ref_revision #1429):node_dirty_refs::list_node_dirty_refsreadsrevision.node_dirty_refs::delete_node_dirty_refs_at_revisiondeletes only rows still at the listed revision; template-manager'sdelete_node_dirty_refsruns it in its IMMEDIATE transaction. Enqueues andMarkFacetDirtyare unchanged.sqlite/src/template_manager.rs: the module doc now says a migration ships only in a release after one that lists it, so the image a rollback returns to can reverse it;20261031050126's check never refuses, so that rollback is image-only. template-manager: reverse the next release's node_dirty_ref_revisions migration so its rollback stays image-only #1277'sDIRTY_REF_REVISIONSentry stays, documented as bundled from here on; the bundled up SQL is byte-identical to its history copy, which template-manager: reverse the next release's node_dirty_ref_revisions migration so its rollback stays image-only #1277's checksum tests pin.recursive_triggersoff and on); revision exhaustion.pplx-space-sqlite:UNREVISIONED(this catalog without20261031050126: the previous release).a_bundled_schema_is_left_untouchedchecks recorded revisions survive a restart.HISTORICALcovers upgrading from the release that recorded it.Store
pplx-space-template-mgr-store(#1429) declares the migration'snode_dirty_ref_revisioncounter. This PR moves the outbox's list and revision-guarded delete from template-manager'sdbmodule into that store, asnode_dirty_refs::list_node_dirty_refsandnode_dirty_refs::delete_node_dirty_refs_at_revision, withsql!locators. template-manager'sdelete_node_dirty_refskeeps its IMMEDIATE transaction and runs the store's delete inside it. template-manager now depends on the store, and[rules.layers.store_users]lists it. archcheck reports that entry as the advisorylayers.policy_changed, for review.Stack
#1277 and #1429 have merged; this PR is on
main. It must ship in a release after one that contains #1277, which reverses20261031050126when a database records it without the catalog bundling it.Validation
At this head, after #1075:
-D warnings,check-migration-versions.shpass; archcheck: 0 required, 1 advisory (store_users).with-nocow-test-filesystem.sh; db-migrate'sviews_hide_other_substrates_rows_but_their_unique_indexes_still_arbitratefails on main too (hostpostgres:16-alpine).space-db-migrator, main and this head, one file: this head records[20260930000000, 20261026220935, 20261031050126]; main reverses only20261031050126, keeping both outbox rows; this head re-applies it, and a re-enqueue's revision survives anotherup.local.template_startup_recovery_decision_table,local.garbage_collectors_reclaim_independent_resources(was…template_cache_lru_eviction_reclaims),local.unanswered_template_deleted_after_wait,local.ecw_template_build_from_oci_boots,local.private_template_build_isolation,local.node_ctl_fence_refuses_and_reconciles,local.btrfs_send_restore_pulls_parent_template,ecw.background_process_control,ecw.cold_create_environment,ecw.warm_pool_environment. The outbox stayed empty, counters advanced, dirty marks were allok, and template-manager logged no errors.Not validated: a fleet rollout; environment-partition rows or a mid-send re-enqueue on Kind (the regression test covers the latter).
Severity and risk
Severity: medium. A lost ack delays a node's copy of a template-manager change until the reference is marked again or reconciliation catches up.
Risk: two small trigger updates per enqueue, in its existing transaction. A stale ack leaves the row to be resent, at-least-once delivery node-ctl already handles.
Rollout
Node-local SQLite; template-manager migrates at startup. The previous release works on the migrated schema (explicit columns; the triggers stamp its writes). Existing rows keep revision 0.
Rollback to a release containing #1277 is image-only: its startup (or migrator
up) reverses20261031050126in one transaction, keeps every outbox row and acks by key again;20261026220935stays where bundled (main since #1075). A release without #1277 refuses the database (unbundled migration).AI assistance: Claude Code (an AI agent) wrote the code, the migration, the tests and this description, and ran the validation above.