Skip to content

fix(correlation): key versioned 1:1 correlation id on source_guid, not position#773

Merged
Muizzkolapo merged 5 commits into
mainfrom
fix/version-correlation-source-guid-keying
Jul 8, 2026
Merged

fix(correlation): key versioned 1:1 correlation id on source_guid, not position#773
Muizzkolapo merged 5 commits into
mainfrom
fix/version-correlation-source-guid-keying

Conversation

@Muizzkolapo

Copy link
Copy Markdown
Owner

Summary

Parallel versions of the same source record intermittently received different version_correlation_ids, so a version_consumption: merge consumer would abort with Missing required field: version_correlation_id on fresh runs.

Root cause: VersionIdEnricher.enrich always passes record_index, so add_version_correlation_id always took the position-based keying branch. Under versions: {mode: parallel}, each version filters/orders records independently, so the same source record lands at a different record_index per version -> different registry key -> different id.

Fix (keying decision lives in version_id.py, where the discriminators are available):

  • Versioned 1:1 (is_versioned_agent and not force and source_guid present) -> key on source_guid, so all N parallel versions share one id regardless of per-version position.
  • Expansions (force=True) and no-source_guid records keep position keying (unique per item / per position) — expansion uniqueness is preserved.
  • The merge abort now names the misaligned source record and version agent instead of a bare field-name message.

No behavior change for expansions; the enricher and the correlator grouping logic are untouched (only the abort's message string).

Verification

  • New tests/unit/utils/correlation/test_version_id_keying.py (6 tests): shared-id-across-positions (the RED repro), distinct-guid, expansion-uniqueness, versioned-expansion-uniqueness, no-guid position fallback, and a pin that the shared id equals the GUID helper's output (kills inline-hash / clamp-to-0 shortcuts).
  • Extended tests/core/graph/test_version_correlator.py: asserts the abort message (not just context dict) names the source_guid and version agent.
  • RED->GREEN confirmed at each step (T1 failed pre-fix with position ids; message test failed pre-fix with the bare string).
  • Full suite: 7855 passed; ruff check + ruff format --check clean across agent_actions and tests.

@Muizzkolapo

Copy link
Copy Markdown
Owner Author

Staff review — approve. Verified the root cause, the fix, and the no-regression surface by fetching the branch and running the affected suites (87 passed: correlator + enrichment + core/utils; ruff check + ruff format --check clean).

Root cause — confirmed

VersionIdEnricher.enrich (processing/enrichment.py:255) is the only production caller and always passes record_index=context.record_index + i. Under versions: {mode: parallel}, each version filters/orders independently, so the same source record lands at a different per-version record_index → different position-based registry key {session}:{base}:position_{idx}:{guid} → different id → merge abort. The old elif source_guid (guid-keying) branch was dead code in the enricher path because record_index was never None.

Fix — correct

Keying versioned 1:1 on source_guid (when present, not forced) converges across versions because version_base_name is shared across all parallel variants — confirmed in workflow/parallel/action_executor.py:_build_version_base_name_map (multiple action_names grouped under one base_name). So {session}:{base}:{guid} is identical across the N versions. Expansions (force=True) and no-source_guid records correctly keep position keying.

Behavior change is real but safe

The id value for versioned 1:1 records changes from position-based to guid-based. This is a silent change to the version_correlation_id written to outputs/store. It is safe because:

  • ids are session-scoped (hash includes workflow_session_id), so cross-run merges were already impossible;
  • within a run all versions use the new scheme consistently;
  • no existing test asserts the old id value — every affected test asserts uniqueness/count/determinism, which hold under both schemes. I ran test_multi_batch_correlation_consistency, test_processor_utils_thread_safety, test_processor_utils_integration::test_realistic_workflow_simulation, and test_version_id_enricher — all green on the PR branch.

Tests — high quality

The 6 new tests include anti-cheat pins (shared-id == guid-helper output; versioned+force keeps uniqueness; no-guid falls back to position) that anticipate inline-hash / clamp-to-0 / drop-not force shortcuts. RED→GREEN visible in the commit sequence. The abort-message test asserts the string, not just the context dict.

Non-blocking notes

  1. Known limitation (acceptable): no-source_guid versioned records under parallel merge still diverge (position keying is per-version, and there is no stable cross-version identity to key on). The PR scopes to the fixable, guid-bearing case — reasonable, since a parallel-merge workflow without source_guid is arguably misconfigured. The improved abort message now names the record + version, so failure is loud rather than cryptic.
  2. Latent (pre-existing, unreachable): the elif source_guid branch (record_index=None + force=True + guid) would assign a shared guid id to expansion items — wrong for 1→N. Unreachable today because the enricher always passes record_index >= 0 (negative guarded at enrichment.py:247). Exists in the old code too; not introduced here. A defensive logger.debug/assert there would be a nice follow-up but is out of scope.
  3. Manifest: utils/correlation/_MANIFEST.md describes "GUID-based and position-based IDs" generically and does not encode the dispatch rule, so it is not stale after this change. Optionally a one-line note on when each keying is chosen would help future readers, but no symbol read/write surface changed.

No merge-blocking issues. Ship it.

Drop the 'records without a source_guid' clause: for versioned 1:1
records source_guid is present (stamped at ingestion), and expansion
children get one unconditionally from LineageEnricher before this runs.
The no-source_guid position fallback is defensive, not a real path
worth advertising in a minimal docstring.
@Muizzkolapo Muizzkolapo merged commit 52c2fdd into main Jul 8, 2026
5 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant