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

TL;DR: Adds storage for queue-repair records without enabling runtime recovery.

Summary

Changes can stay stuck when recovery saves a batch's result but fails before waking the queue. This draft adds storage for unfinished repairs and their completion status, but does not yet make recovery run.

Still blocked: SQC015 is not fixed. Enabling recovery first requires SQC001 to protect merge outcomes and SQC005 to ensure only one planner can act on a queue at a time. This draft includes only approved Phases 1–2. Recording all affected batches, completing repairs, connecting the fenced planner and proving quiet-queue recovery remain follow-up work.

Records are separated by queue and keep the original topic, partition and message IDs unchanged. Each record's own ID stays unchanged. The added MySQL table has no extra lookup index or delete operation; finished records remain after old queue messages are cleaned up. No final handler, planner, counter call, flag or recovery callback is enabled.

Repro

  1. Have a final-DLQ speculate message for batch 1 and another live batch 2 in an otherwise quiet queue.
  2. Save batch 1 as Failed, then fail the attempt to send the queue its next wake-up message.
  3. Retry the same failed message. The handler sees that batch 1 is already failed and sends no wake-up, so batch 2 can stay stuck. This runtime bug remains unchanged by this draft.

Test Plan

Previously run checks for the published draft:

  • Retrying record creation keeps the first record; stale updates or updates naming the wrong record are rejected.
  • Real-MySQL checks keep queues isolated and preserve distinct source IDs, including case, trailing spaces and long Unicode values.
  • The existing retry test confirms that an already-failed batch sends no new wake-up. It covers that retry boundary, not the whole failure sequence or a working recovery fix.

No tests were rerun for this description-only edit.

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