feat(ITL-508): indexed entry_id versioning for concurrency-safe miss-triggered recomputation by kurodo3[bot] · Pull Request #218 · nauticalab/orcapod-python

kurodo3 · 2026-07-08T20:54:18Z

Summary

Introduces compute_base_entry_id (stable, index-free hash) as the in-memory cache key and Phase 1 filter column (_PIPELINE_BASE_ENTRY_ID_COL), separate from the versioned DB primary key produced by compute_pipeline_entry_id(tag, data, recomputation_index).
Redesigns add_pipeline_record to read the max existing recomputation_index for a given base_entry_id and write at max+1 with skip_duplicates=True, making concurrent asyncio coroutines naturally serialise without duplicate pipeline records.
Re-keys Phase 1 internals (get_cached_results, _fetch_joined_records, _load_cached_entries) and Phase 2 cache writes to use base_entry_id, so stale versioned rows drop out of the inner join automatically while valid results are found via the stable key.

Test plan

TestComputeBaseEntryId — stability and uniqueness of the index-free hash
TestVersionedEntryIdDiffersByIndex — determinism and index sensitivity of versioned hash
TestAddPipelineRecordIndexed — first call writes at index 0, second at index 1, _PIPELINE_BASE_ENTRY_ID_COL written, skip_cache_lookup removed
TestConcurrentMissSerialization — two asyncio TaskGroup tasks for the same input produce valid pipeline records; session 2 does not recompute
TestPersistentMissWarning::test_recompute_after_persistent_miss_appends_new_pipeline_record — miss-triggered Phase 2 write at index 1 resolves the stale index 0 entry
TestEphemeralWritePath::test_recompute_after_ephemeral_miss_no_infinite_cycle — ephemeral miss does not loop
Full suite: 4212 passed, 56 skipped, 6 xfailed

Linear

Fixes ITL-508
ITL-507
ITL-515

Covers schema changes (base_entry_id + recomputation_index columns), entry ID computation redesign, Phase 1/2 flow updates, single shared index chain, asyncio atomicity guarantee, and test plan. Files ITL-515 as follow-up for multi-process / thread-safe insert_if_not_exists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ne_entry_id Adds two new module-level constants (_PIPELINE_BASE_ENTRY_ID_COL, _PIPELINE_RECOMPUTATION_INDEX_COL) and two methods on FunctionJobNode: - compute_base_entry_id: stable hash without recomputation index (pre-ITL-508 semantics) - compute_pipeline_entry_id: now accepts recomputation_index (default 0) so each recomputation attempt gets a distinct DB primary key Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…e helper Corrects the inaccurate constant comment and docstring for compute_base_entry_id that referenced future wiring not yet implemented, and eliminates code duplication between compute_base_entry_id and compute_pipeline_entry_id by extracting a shared _build_entry_id_preimage helper method. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… remove skip_cache_lookup Replaces the old skip_cache_lookup-based deduplication guard with an indexed versioning strategy. add_pipeline_record now computes the next recomputation index by querying existing rows sharing the same base_entry_id and writes at max_index+1 with skip_duplicates=True for concurrency safety. Removes skip_cache_lookup from the method signature and from PodNodeProtocol. Replaces TestAddPipelineRecordDeduplication with TestAddPipelineRecordIndexed (4 tests, all passing).

…ecord callers Remove the four call sites in _process_data_internal and _async_process_data_internal (ephemeral and persistent branches) that still passed skip_cache_lookup=True after Task 2 removed that parameter from add_pipeline_record, causing TypeError at runtime. Also update test_execute_data_second_call_same_input_deduplicates to reflect the ITL-508 recomputation-index design: two calls for the same logical input now write two versioned pipeline records (index 0 and 1) rather than deduplicating to one. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ocol stub with impl Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…data_internal Switches the in-memory cache key in _process_data_internal and _async_process_data_internal from compute_pipeline_entry_id (versioned) to compute_base_entry_id (stable). The versioned entry ID changes with each recomputation attempt; using it as a cache key would cause cache misses on subsequent recomputation cycles for the same logical input. base_entry_id is stable across all recomputation attempts, enabling proper cache hits during Phase 1 lookups even after miss-triggered Phase 2 rewrites. Also updates Phase 1 cache lookup in execute() and async_execute() to use base_entry_id for consistency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…nc_execute - Fix Fix 1: Update docstring for _process_data_internal to reference base_entry_id instead of stale entry_id - Fix Fix 2: Rename cached_by_entry_id dict and entry_id variable to cached_by_base_entry_id and base_entry_id respectively for consistency with the actual method names used Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…h/load/get_cached - Rename `get_cached_results` parameter from `entry_ids` to `base_entry_ids` - Rename `_fetch_joined_records` parameter from `entry_ids` to `base_entry_ids`; update anti-join and filter to use `_PIPELINE_BASE_ENTRY_ID_COL` - Rename `_load_cached_entries` parameter from `entry_ids` to `base_entry_ids`; update internal `_fetch_joined_records` call and dict key extraction - Update `get_all_records` to also drop `_PIPELINE_BASE_ENTRY_ID_COL` and `_PIPELINE_RECOMPUTATION_INDEX_COL` from user-facing output - Update `execute` caller of `get_cached_results` to use `base_entry_ids=` kwarg - Update test files: `test_function_node_fetch_joined.py` and `test_function_node_get_cached.py` to use `base_entry_id` and `compute_base_entry_id`

…ase 1

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

codecov · 2026-07-08T20:56:07Z

Codecov Report

❌ Patch coverage is 97.61905% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/orcapod/core/nodes/function_node.py	97.61%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…ion-indexed-entry_id-versioning

Copilot

Pull request overview

This PR implements ITL-508’s “base entry id + recomputation index” scheme to make miss-triggered recomputation concurrency-safe by separating a stable base_entry_id (used for Phase 1 filtering + in-memory cache keys) from a versioned entry_id (used as the pipeline DB primary key).

Changes:

Add compute_base_entry_id() and extend compute_pipeline_entry_id(..., recomputation_index=0); re-key Phase 1/2 caching internals to use base_entry_id.
Redesign add_pipeline_record() to append versioned pipeline rows at max(recomputation_index)+1, removing skip_cache_lookup.
Update protocols and tests to reflect the new ID model, filter semantics, and recomputation behavior (including new concurrency-focused tests/spec docs).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/orcapod/core/nodes/function_node.py	Core implementation: new ID methods/columns, redesigned `add_pipeline_record`, Phase 1/2 re-keying to `base_entry_id`.
src/orcapod/protocols/pipeline_protocols.py	Update `add_pipeline_record` protocol signature (remove `skip_cache_lookup`, add `computed` / `is_ephemeral`, use `uuid.UUID`).
tests/test_core/nodes/test_function_node_get_cached.py	Update `get_cached_results` tests to use `base_entry_id` keys.
tests/test_core/nodes/test_function_node_fetch_joined.py	Update `_fetch_joined_records` tests to filter by `base_entry_ids`.
tests/test_core/function_pod/test_function_pod_node.py	Update expectations: repeated same input appends recomputation-indexed rows instead of deduplicating.
tests/test_core/function_pod/test_function_node_caching.py	Add tests for `compute_base_entry_id` and index-sensitive versioned IDs.
tests/test_core/function_pod/test_ephemeral_result.py	Replace dedup tests with indexed-write tests; add concurrent Phase 2 serialization tests.
superpowers/specs/2026-07-08-itl-508-indexed-entry-id-versioning-design.md	New design spec documenting the ITL-508 approach and testing plan.
superpowers/plans/2026-07-08-itl-508-indexed-entry-id-versioning.md	New implementation plan capturing the intended task breakdown and TDD steps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kurodo3 · 2026-07-08T22:07:48Z

+        # Determine the next recomputation index by querying all existing rows
+        # for this base_entry_id. No await is used here, so within a single-threaded
+        # asyncio event loop this read-then-write sequence is uninterrupted.
+        existing = self._pipeline_database.get_records_with_column_value(
+            self.node_identity_path,
+            {_PIPELINE_BASE_ENTRY_ID_COL: base_entry_id},
+        )


Added a schema guard at the top of add_pipeline_record that calls get_all_records for the node path and checks whether the returned table (if non-empty) contains both _PIPELINE_BASE_ENTRY_ID_COL and _PIPELINE_RECOMPUTATION_INDEX_COL. If either column is absent, a ValueError is raised immediately with a clear message identifying the missing columns and instructing the user to clear or migrate the pipeline database. This fires before the get_records_with_column_value call, so the user gets an actionable error instead of a cryptic Arrow filter exception.

kurodo3 · 2026-07-08T22:08:06Z

+### Concurrent-miss serialisation (asyncio)
+- Two asyncio tasks both observe a Phase 1 miss for the same (tag, data).
+- Both enter Phase 2 concurrently.
+- After both complete, pipeline DB contains exactly **one** row at
+  `recomputation_index=1` (no duplicate).
+- `call_count` reflects two computations (both ran) but only one pipeline
+  record was written.


Updated the design spec's "Concurrent-miss serialisation (asyncio)" section to reflect actual semantics. The section now explains that add_pipeline_record is fully synchronous (no await), so asyncio cooperative scheduling serialises the two writes: each coroutine reads the current max_index before the other writes, so both rows land (indices 0 and 1). The assertion is documented as >= 1 — the correct lower bound — with a note that skip_duplicates=True only de-dupes when two coroutines accidentally compute the identical versioned entry ID (impossible when max_index advances between reads). The test code already used >= 1; only the spec was wrong.

kurodo3 · 2026-07-08T22:08:16Z

    def test_entry_ids_filter_narrows_rows(self, executed_node):
-        """Passing a single entry_id returns only that row."""
+        """Passing a single base_entry_id returns only that row."""
        node = executed_node


Renamed to test_no_base_entry_ids_returns_all_rows and updated the docstring to say base_entry_ids=None instead of entry_ids=None.

kurodo3 · 2026-07-08T22:08:17Z

    def test_empty_entry_ids_returns_zero_rows(self, executed_node):
        """Passing entry_ids=[] returns a 0-row table, not None."""
-        result = executed_node._fetch_joined_records(entry_ids=[])
+        result = executed_node._fetch_joined_records(base_entry_ids=[])


Renamed to test_empty_base_entry_ids_returns_zero_rows and updated the docstring to say base_entry_ids=[] instead of entry_ids=[].

…, spec update - add_pipeline_record: fail fast with a clear ValueError when the pipeline DB contains pre-ITL-508 records missing _PIPELINE_BASE_ENTRY_ID_COL / _PIPELINE_RECOMPUTATION_INDEX_COL, instead of crashing with a cryptic Arrow error - test_function_node_fetch_joined: rename test_no_entry_ids_returns_all_rows → test_no_base_entry_ids_returns_all_rows and test_empty_entry_ids_returns_zero_rows → test_empty_base_entry_ids_returns_zero_rows; update docstrings to match - design spec: correct concurrent-miss testing plan — actual asyncio semantics yield >= 1 rows (not exactly one), explain why Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

kurodo3 · 2026-07-08T22:08:28Z

Review round addressed

All four Copilot comments resolved in commit f7bad25:

#	Comment	Action taken
1	`add_pipeline_record` crashes opaquely on pre-ITL-508 DB records	Added an explicit schema guard: calls `get_all_records` upfront, checks for `_PIPELINE_BASE_ENTRY_ID_COL` / `_PIPELINE_RECOMPUTATION_INDEX_COL`, raises `ValueError` with a clear migration message if either is absent
2	Spec says "exactly one row at `recomputation_index=1`" but test asserts `>= 1`	Updated the design spec's concurrent-miss testing plan to explain actual asyncio serialisation semantics and why `>= 1` is the correct assertion
3	`test_no_entry_ids_returns_all_rows` — stale name/docstring	Renamed → `test_no_base_entry_ids_returns_all_rows`; docstring updated
4	`test_empty_entry_ids_returns_zero_rows` — stale name/docstring	Renamed → `test_empty_base_entry_ids_returns_zero_rows`; docstring updated

Full test suite: 4212 passed, 0 failures after all changes.

kurodo3 · 2026-07-08T22:08:34Z

Review round addressed

All four Copilot comments resolved in commit f7bad25:

#	Comment	Action taken
1	`add_pipeline_record` crashes opaquely on pre-ITL-508 DB records	Added an explicit schema guard: calls `get_all_records` upfront, checks for `_PIPELINE_BASE_ENTRY_ID_COL` / `_PIPELINE_RECOMPUTATION_INDEX_COL`, raises `ValueError` with a clear migration message if either is absent
2	Spec says "exactly one row at recomputation_index=1" but test asserts >= 1	Updated the design spec's concurrent-miss testing plan to explain actual asyncio serialisation semantics and why >= 1 is the correct assertion
3	test_no_entry_ids_returns_all_rows — stale name/docstring	Renamed to test_no_base_entry_ids_returns_all_rows; docstring updated
4	test_empty_entry_ids_returns_zero_rows — stale name/docstring	Renamed to test_empty_base_entry_ids_returns_zero_rows; docstring updated

Full test suite: 4212 passed, 0 failures after all changes.

kurodo3 Bot and others added 12 commits July 8, 2026 08:32

refactor(ITL-508): fix stale skip_cache_lookup docstrings; align prot…

c3940a5

…ocol stub with impl Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(ITL-508): rename entry_ids to base_entry_ids in execute() Ph…

d25a6f7

…ase 1

test(ITL-508): add concurrent-miss asyncio serialisation tests

d133a52

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

docs(ITL-508): add implementation plan

b62065a

eywalker requested review from Copilot and eywalker July 8, 2026 21:01

Merge branch 'main' into eywalker/itl-508-concurrency-safe-recomputat…

35a1fa7

…ion-indexed-entry_id-versioning

Copilot started reviewing on behalf of eywalker July 8, 2026 21:01 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

eywalker approved these changes Jul 8, 2026

View reviewed changes

eywalker merged commit 1418ff8 into main Jul 8, 2026
11 checks passed

eywalker deleted the eywalker/itl-508-concurrency-safe-recomputation-indexed-entry_id-versioning branch July 8, 2026 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(ITL-508): indexed entry_id versioning for concurrency-safe miss-triggered recomputation#218

feat(ITL-508): indexed entry_id versioning for concurrency-safe miss-triggered recomputation#218
eywalker merged 14 commits into
mainfrom
eywalker/itl-508-concurrency-safe-recomputation-indexed-entry_id-versioning

kurodo3 Bot commented Jul 8, 2026

Uh oh!

codecov Bot commented Jul 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

kurodo3 Bot Jul 8, 2026

Uh oh!

kurodo3 Bot Jul 8, 2026

Uh oh!

kurodo3 Bot Jul 8, 2026

Uh oh!

kurodo3 Bot Jul 8, 2026

Uh oh!

kurodo3 Bot commented Jul 8, 2026

Uh oh!

kurodo3 Bot commented Jul 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

kurodo3 Bot commented Jul 8, 2026

Summary

Test plan

Linear

Uh oh!

codecov Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

kurodo3 Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

kurodo3 Bot commented Jul 8, 2026

Review round addressed

Uh oh!

kurodo3 Bot commented Jul 8, 2026

Review round addressed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 8, 2026 •

edited

Loading