feat(storage): delta storage for target_data — 20x reduction#679
Open
Muizzkolapo wants to merge 12 commits into
Open
feat(storage): delta storage for target_data — 20x reduction#679Muizzkolapo wants to merge 12 commits into
Muizzkolapo wants to merge 12 commits into
Conversation
Each action now stores only its own content namespace instead of the full accumulated record. Reconstruction happens transparently inside read_target() by joining upstream deltas. Architecture: - write_target() concrete in StorageBackend base class — extracts delta then delegates to _write_target_raw() (abstract, each backend implements) - read_target() reconstructs full records from upstream deltas via _read_target_raw_batch() — consumers see identical list[dict] - _delta_mode field (delta/first/full) tracks storage mode per record, stripped before return — never leaks to business logic Safety (11 failure modes from spec review): - FM7: reconstruction uses only abstract methods, no backend-specific access - FM8: missing upstream flagged as _reconstruction_incomplete + warning log - FM9: multi-namespace records without _delta_mode tag produce warning - FM10: format version check — old code raises ConfigurationError on delta DB - FM11: raw SQL shows partial records (documented DX tradeoff) Tagged write paths: - Carry-forward records: _delta_mode="full" (processing.py) - Version correlation: _delta_mode="full" (loop.py) - FILE expansion/synthetic: _delta_mode="full" (file_tool.py) New infrastructure: - workflow_metadata table stores execution_order for reconstruction - _read_target_raw_batch() batched IN query in SQLiteBackend - Reconstruction cache (instance-level, cleared on write)
Owner
Author
Code reviewFound 3 issues:
agent-actions/agent_actions/storage/backend.py Lines 112 to 400 in d8f57f9
agent-actions/agent_actions/storage/backend.py Lines 237 to 248 in d8f57f9
agent-actions/agent_actions/processing/strategies/file_tool.py Lines 99 to 108 in d8f57f9 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…delta storage tests
Code review found 3 issues:
1. FM9 multi-namespace warning fired on every normal record at every
action beyond the first — false positive. Removed entirely. The data
bus normally accumulates namespaces; _extract_delta correctly strips
them to deltas. This is the expected behavior, not an anomaly.
2. Guard-skip tombstones not tagged _delta_mode="full" — NOT a bug.
Tombstone delta is {action_name: None}. Upstream content is stored
in upstream action deltas and reconstructed on read. Verified with
test_tombstone_reconstruction_includes_upstream.
3. No tests — FIXED. Added 20 tests across 7 test classes:
- TestDeltaExtraction (5 tests): first action, subsequent, full mode,
no content, auto-detect
- TestDeltaReconstruction (4 tests): full round-trip, delta_mode
stripping, cache invalidation, missing upstream flag
- TestGuardSkipTombstone (2 tests): stored as delta, reconstructed
with upstream
- TestBackwardCompatibility (1 test): legacy records returned as-is
- TestMetadata (4 tests): save/load round-trip, missing key, overwrite,
execution order
- TestFormatVersionCheck (3 tests): version stored, future version
rejected, corrupt version rejected
- TestPreviewReconstruction (1 test): preview shows full content
_delta_mode tags set on FILE-mode expansion/synthetic records were dropped during enrichment because RecordEnvelope.build() only carries _PERSISTENT_FIELDS. Add _delta_mode to RECORD_LIFECYCLE_FIELDS so the tag survives through the pipeline to write_target. Without this fix, FILE tools that produce new records (3→13 flatten, N→M grouping) had their _delta_mode="full" tag dropped, causing delta extraction to strip upstream content from records whose source_guids don't exist in upstream actions.
The _delta_mode="full" tag set in file_tool.py was dropped during enrichment because RecordEnvelope.build() creates new records. Move tagging to enrichment.py where expansion records get fresh source_guids — this is AFTER record creation, so the tag survives to write_target. Also add force_full parameter to write_target for callers that know their data should not be delta-extracted.
Root cause: when a FILE tool expands 3→13 records with new GUIDs, downstream actions tried to find those GUIDs in ALL upstream actions (including pre-expansion ones where the GUIDs don't exist). This produced _reconstruction_incomplete warnings for every record at every action after the expansion. Fix: track which GUIDs have a _delta_mode="full" upstream record. Full records are self-contained — they already embed all upstream content. Reconstruction for a given GUID starts at the full record and merges forward, skipping actions before the expansion boundary. This is the architectural fix: the execution order is a flat list but GUIDs change at expansion boundaries. Reconstruction must respect those boundaries instead of blindly walking the full chain.
The execution order is a flat list where parallel actions are serialized in arbitrary order. Using execution_order[:idx] as "upstream" incorrectly includes parallel peers (e.g., verify_answer_3 treated validate_final_question_1/2/3 as upstream, but they're siblings at the same level). Fix: store the actual dependency DAG in workflow_metadata alongside execution_order. Reconstruction uses _get_upstream_actions() which reads the DAG — only true transitive dependencies are fetched. Falls back to flat execution order for legacy DBs.
…_actions _get_reachable_actions walks action_configs using depends_on names. For versioned actions (extract_raw_qa_1/2/3), the config declares dependencies using base names (extract_raw_qa) but action_configs uses versioned names. The lookup fails silently, producing empty dependency lists for ALL actions. Fix: build the graph from execution levels instead. Actions at level N depend on all actions at levels 0..N-1. The level orchestrator already handles versioned name resolution correctly. This produces the right graph for both versioned and non-versioned actions, and correctly excludes parallel peers.
Owner
Author
Code review (post-fix pass)Found 1 issue:
agent-actions/agent_actions/workflow/coordinator.py Lines 427 to 438 in 86389e0 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Three bugs fixed: 1. save_metadata now clears the in-memory dependency_graph_cache and execution_order_cache. Without this, the cache loaded stale data from a previous run and never refreshed — causing reconstruction to use empty dependency lists even though the DB had correct data. 2. async_run now uses levels-based dep graph (same as sync run). Previously used _get_reachable_actions which fails for versioned actions due to base-name mismatch. 3. save_metadata becomes concrete in base class (cache clearing), delegates to _save_metadata_raw (abstract, each backend implements).
- Extract duplicated metadata-saving logic from async_run and _run_workflow_with_context into _persist_execution_metadata() - Remove inconsistent logging (was in sync only) - Add invariant comment on records[0] check in _reconstruct_from_deltas
… leak, false incomplete 4 fixes from code review: - #6: _get_upstream_actions logs warning when action not in dep graph and falls back to flat order, instead of silently using wrong upstream - #7: Partitioned pipelines no longer get false _reconstruction_incomplete flags — upstream actions with no data for this file are skipped (they process different record partitions) - #8: writer.py strips _delta_mode from records before disk write so filesystem files don't leak the internal storage marker - #9: json.loads on corrupt dependency_graph/execution_order metadata now logs warning and degrades gracefully instead of crashing with opaque JSONDecodeError
Two remaining review findings: - #2: Records without source_guid are now stored as _delta_mode="full" instead of being delta-extracted. Without a join key, upstream content is unrecoverable — store the full record. - #3: Removed _reconstruction_incomplete flag entirely. Partitioned pipelines (where upstream actions process different guid subsets) produced false flags on correct records. Missing-guid cases are now logged at debug level, not flagged on the record. No consumer ever read the flag — removing it is the clean solution.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
read_target()— consumers see identicallist[dict]StorageBackendbase class — future backends (Postgres, Mongo) get it for free_delta_mode: "full": carry-forward, version correlation, FILE expansionArchitecture
write_target()→ concrete in base class, extracts delta, delegates to_write_target_raw()read_target()→ concrete in base class, reconstructs from upstream deltas via_read_target_raw_batch()_delta_modefield (delta/first/full) stripped before returning to consumersStorage reduction
Safety
_reconstruction_incompleteflag + warningConfigurationErroron old code reading delta DBVerification
ruff checkandruff format --checkclean