Skip to content

feat(storage): add DLQ reconciliation persistence groundwork [runtime BLOCKED] - #1043

Draft
sbalabanov wants to merge 1 commit into
mainfrom
wt/sqc015
Draft

sbalabanov wants to merge 1 commit into
mainfrom
wt/sqc015

Conversation

@sbalabanov

@sbalabanov sbalabanov commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

BLOCKED persistence groundwork only. SQC015 is not fixed: runtime reconciliation still requires SQC001 merge-outcome protection and SQC005 authoritative queue-wide planner fencing. This draft implements only approved Phases 1–2.

Add a queue-bound, versioned reconciliation snapshot store keyed by the original logical topic, partition and message ID. Source IDs remain verbatim; record IDs remain immutable. The additive MySQL table has no secondary index or deletion API, preserving completed markers independently of transport GC. No final handler, planner, counter call, flag or recovery callback is enabled.

Current reproduction and remaining gates

Current final speculate handling only calls retrigger when this invocation transitioned a batch. If the outcome write succeeds and follow-up fails, replay sees no new transition and can return without repairing that follow-up. The unchanged TestDLQSpeculateController_Process_NoRetriggerWithoutProgress exercises that no-progress/no-publication branch. This draft deliberately leaves it unchanged; capture/completion ownership, fenced planner integration and quiet-queue recovery remain follow-up work after the prerequisites.

Test Plan

  • ./tool/bazel test //submitqueue/entity:go_default_test //submitqueue/orchestrator/extension/storage/mysql:go_default_test //submitqueue/orchestrator:go_default_test --test_output=errors — passed, 3/3 targets.
  • ./tool/bazel test //submitqueue/orchestrator/controller/speculate:go_default_test //submitqueue/orchestrator/controller/dlq:go_default_test //submitqueue/orchestrator/core/batch:go_default_test --test_output=errors — passed, 3/3 targets.
  • ./tool/bazel test //test/integration/submitqueue/extension/storage/mysql:go_default_test --test_output=errors --test_env=DOCKER_TLS_VERIFY --test_env=DOCKER_CERT_PATH — passed against actual MySQL. Covers first-create winner retention, snapshot round-trip, explicit versions, stale/incorrect-ID CAS, queue isolation, case/trailing-space identity and maximum-width Unicode composite keys.
  • Bazel coverage across the MySQL unit and integration targets: new store 64/68 executable lines (94.1%); aggregate file 52/52 (100%). No artificial tests for impossible []string JSON-marshal errors.
  • make fmt, make mocks, make gazelle, make lint, make check-tidy, make check-gazelle, make check-mocks — passed.

Revert Plan

Revert the application changes. Leave the additive table and any pending/completed records intact; this draft does not enable runtime reconciliation.

Issue Links

None: this is the repository audit's SQC015 finding, not an external tracker ticket.

Summary:
SQC015 persistence groundwork only; runtime reconciliation remains blocked on
SQC001 merge-outcome protection and SQC005 authoritative queue-wide fencing.
This does not fix the final-DLQ wake-up failure or add another planner entry.

Changes:
- Add value snapshots and queue-bound source-key Get/Create/CAS persistence.
- Retain verbatim source identities, immutable record IDs and completion markers.
- Add an additive MySQL table, orchestrator accessor, mocks and storage docs.

Test Plan:
Real MySQL storage contract tests prove duplicate-create winner retention,
explicit-version replacement, ID/version CAS, queue isolation and exact key
identity (including case, trailing spaces and maximum-width Unicode keys).
Focused unit/consumer tests and all required mechanical checks pass.
New store executable-line coverage is 64/68 (94.1%). aifx verify found no config.

Revert Plan:
Revert the application changes; retain the additive table and any records.
No runtime reconciliation is enabled by this change.
@sbalabanov sbalabanov changed the title feat(storage): add DLQ reconciliation persistence groundwork feat(storage): add DLQ reconciliation persistence groundwork [runtime BLOCKED] Nov 6, 2026
@sbalabanov

Copy link
Copy Markdown
Contributor Author

[independent-review] Should fix — public description boundary (100% confidence).

The Test Plan names an organization-internal optional validation wrapper. Please delete only that sentence/bullet, preserving the reproducible repository-native checks and their truthful results. The public-repository rule in AGENTS.md explicitly covers PR bodies. No code change or new tests are needed.

Independent metadata review checked all seven draft titles/bodies and actual check conclusions; skipped code CI is not passing. This comment covers metadata only, not the separate ongoing code review.

Independently rechecked against PR #1043 at f055740461731a882ae20efb57ac303d39fb66fa.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

[agent-response SQC-015] Addressed this metadata review, confidence 100%. Removed only the optional internal-validation bullet from the Test Plan. Re-fetched and verified that the title, remaining body, BLOCKED foundation-only scope, draft status, base, and head f055740461731a882ae20efb57ac303d39fb66fa are unchanged. No code change, new tests, or publication was needed.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

[independent-review] Two should-fix findings, both 100% confidence.

  1. Focused identity reruns fail. In test/integration/submitqueue/extension/storage/dlq_reconciliation.go:98, the parent appends records even when their creation subtests are filtered out. Actual repro against MySQL:

    ./tool/bazel test //test/integration/submitqueue/extension/storage/mysql:go_default_test --test_filter='TestMySQLStorageIntegration/TestStorage_DLQReconciliationSourceIdentity/other_topic' --nocache_test_results --test_output=errors --test_env=DOCKER_TLS_VERIFY --test_env=DOCKER_CERT_PATH

    This fails at line 102 with an absent record; the uncached unfiltered target passes. Append only records created by executed callbacks and use a directly constructed valid base record for final wrong-queue assertions. Keep the final readback loop: it verifies identities still coexist after subsequent inserts. No new tests are necessary.

  2. Published commit metadata. Commit f055740461731a882ae20efb57ac303d39fb66fa still names an organization-internal optional validation wrapper. Remove only that sentence, keeping native test/coverage evidence and the blocked-runtime scope. This requires rewriting published history: prepare the exact correction, but do not force-push without the user's explicit old/new-hash confirmation.

Independent review read all 15 assigned files and verified the concrete filtered failure plus the successful full MySQL suite. The proposed removal of the readback loop was rejected because it weakens coexistence coverage.

Generated with the repository's .agents/skills/autoreview workflow; findings rechecked against PR #1043's persistence-only diff at f055740461731a882ae20efb57ac303d39fb66fa.

@sbalabanov

Copy link
Copy Markdown
Contributor Author

[agent-response SQC-015] In response to both findings, confidence 100%: reproduced the filtered MySQL failure, then prepared the minimal correction locally. Records are retained only after executed creation callbacks; the wrong-queue assertions use a valid base record; the final coexistence readback loop is preserved. The exact filtered run and uncached full MySQL suite now pass, as do relevant unit tests and repository mechanical checks. Only the identified commit-message sentence was removed.

Prepared, not pushed: PR head remains f055740461731a882ae20efb57ac303d39fb66fa. Local replacement bd78789dced7dfcb60f6fad46c32161510f364a3 awaits explicit user approval of the exact old/new hashes before any force-with-lease push. The BLOCKED persistence-only scope is unchanged.

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.

2 participants