Repository navigation
fix(consolidation): shorten retry backoff base from 60s to 5s - #1854
Merged
Merged
Conversation
Issue #1842 reports banks sitting idle on transient LLM errors (a 5xx that clears in seconds). The current schedule (60, 120, 240, 480, 960, 1800-cap) treats every failure like a multi-minute outage, so a one-second blip parks a bank for at least 60s before the worker tries again. Drop the base to 5s. New schedule: 5, 10, 20, 40, 80, 160, 320, 640, 1280, 1800-cap. Transient errors clear in seconds; the 1800s cap is preserved so a genuine multi-hour outage still doesn't hammer the upstream. Dedup-by-bank and indefinite-retry semantics are unchanged.
nicoloboschi
added a commit
that referenced
this pull request
May 29, 2026
) (#1853) * feat(consolidation): scope-locked parallel LLM dispatch Adds opt-in HINDSIGHT_API_CONSOLIDATION_LLM_PARALLELISM (default 1, sequential). Parallel groups acquire per-scope asyncio.Locks computed from each memory's observation_scopes setting, so two tag groups whose write-scope sets overlap serialise on the overlapping scope rather than racing on the same observation row. Locks acquired in tuple(sorted(scope)) order across all groups for deadlock-freedom. Covers combined / per_tag / all_combinations / explicit-list scopes uniformly with no operator opt-in. Refactor extracts the per-memory observation_scopes resolver into module-level helpers (_resolve_obs_tags_list, _resolve_write_scopes, _parse_observation_scopes, _scope_sort_key) so the dispatcher and the lock layer share one source of truth. Per-batch stats deltas now return as _BatchDeltas and merge serially after dispatch — no lost-update race on shared counters/tag set. * feat(consolidation): per-batch perf log + default parallelism=4 - Per-batch log uses a batch-local ConsolidationPerfLog so timings, llm_calls, and input_tokens reflect only that batch's work — no delta-from-shared-snapshot bleed under parallelism > 1. Local perf merges into the job-level perf at end-of-batch so the final flush still totals everything. - Restore the cumulative processed=N/total progress indicator. The counter increments + snapshots atomically between awaits in single-threaded asyncio, no lock needed. - Bump DEFAULT_CONSOLIDATION_LLM_PARALLELISM from 1 to 4 to match retain_max_concurrent and let combined-mode banks pick up the throughput win out of the box. Lock-on-overlap makes this safe by construction; per_tag / all_combinations banks degrade to serial automatically. - New regression test test_per_batch_log_line_attributes_only_own_work asserts per-batch log fields are isolated (llm calls / memories / created / timing) and cumulative processed indicator is monotonic. * chore: regenerate docs-skill + merge two alembic heads to unblock CI - skills/hindsight-docs/references/developer/configuration.md: regenerated via ./scripts/generate-docs-skill.sh to pick up the new HINDSIGHT_API_CONSOLIDATION_LLM_PARALLELISM entry from the source configuration.md edited in the previous commit. - alembic/versions/mrgvchgraf01_*: empty merge revision unifying main's two open heads (b5a4c3e2f1d8 add_graph_maintenance_queue and b8c9d0e1f2a3 vchord_cosine_opclass). test_alembic_dag.py::test_single_head catches the divergence and recommends `alembic merge heads`; this is that. Pre-existing on main — only surfaced because this PR touches API code and trips the path-filtered test-api job. * ci: cap every job in test.yml at 30 minutes Adds timeout-minutes: 30 to all 60 jobs. Without it each job inherits GitHub Actions' 6-hour default, so a hung worker or a flaky LLM call can keep the whole suite "running" for hours before someone notices. 30 min is ~2x headroom over the slowest current job (test-api shards ~13 min, test-doc-examples ~14 min, test-python-client-oracle ~13 min). If a specific job legitimately needs more later, bump just that one. * chore: drop redundant alembic merge migration Main shipped its own merge revision c1d2e3f4a5b6 for the same two heads (b5a4c3e2f1d8 and b8c9d0e1f2a3) in #1854/#1857's neighbourhood, so my mrgvchgraf01 became redundant after rebase. Keeping only main's version to avoid a fresh divergent-heads situation. * test: bump pool_max_size from 5 to 30 in memory fixtures The 4 MemoryEngine fixtures in conftest were sized for sequential consolidation; with consolidation_llm_parallelism now defaulting to 4 (and other parallel knobs like retain_max_concurrent=4 already active), a pool of 5 connections can be exhausted when an HTTP integration test triggers multiple async retains that each fan consolidation across several concurrent tag groups. CI surfaced this as test_async_retain_parallel hanging on test-api shard 2 — 5 parallel retains × 4-way intra-op consolidation parallelism + the test's own polling HTTP calls all competed for 5 connections under xdist's worker concurrency. Bumping to 30 keeps tests bounded but matches a more realistic deployment pool size (default prod cap is 100) and removes the head-of-line stall. * test: bump pg0 max_connections to 300, pool to 15, fix configurable counter CI surfaced two real failures from the previous bump: - shard 2: tests/test_hierarchical_config.py::test_hierarchical_fields_categorization hardcoded `assert len(configurable) == 36`. Adding consolidation_llm_parallelism to _CONFIGURABLE_FIELDS made it 37. Bumped and added an explicit membership assertion so a future drop of the flag fails loudly. - shard 3: asyncpg.TooManyConnectionsError. With pool_max_size=30 and 8 xdist workers, peak demand was ~240 connections against postgres's default cap of 100. Two related changes: * EmbeddedPostgres now accepts a ``config: dict[str, str]`` and forwards it to Pg0 (which has been a documented Pg0 kwarg). The pg0_db_url fixture passes ``{"max_connections": "300"}`` so 8 workers × pool=15 fits comfortably. * Pool back to 15 (from 30 in the previous commit). 15 still accommodates default consolidation_llm_parallelism=4 + retain_max_concurrent=4 + the test's own queries without head-of-line stalls, but caps total connections at a sane fraction of the 300 max.
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
Reported in #1842: with a backlog of >50k unconsolidated memories across 17 banks, "active consolidation rises toward the slot limit, then decays back toward 1 over a few minutes" on bursts of transient LLM errors. The current retry schedule treats every failure like a multi-minute outage:
A momentary LLM 503 that clears in a second still parks the bank for at least 60s. With 24 worker slots, a burst of transient errors across 17 banks compounds into the "decay to 1" pattern the issue describes.
This PR drops
_CONSOLIDATION_RETRY_BACKOFF_BASE_SECONDSfrom 60 → 5. New schedule:The 1800s cap is preserved so a genuine multi-hour outage still doesn't hammer the upstream. Dedup-by-bank and the indefinite-retry policy (no attempt cap) are unchanged — only the base interval moves.
What this does NOT address
This is one of multiple symptoms in #1842 — the most directly tunable one. The "ops finish a round and disappear entirely with backlog remaining" symptom is still under investigation (it doesn't reproduce with the new
tests/test_consolidation_reschedule_after_round.pyregression guard, which exercises the round-limit re-queue end-to-end against the real DB and asserts a fresh pending row lands).Issue #1842 stays open pending that investigation; this PR is the safe, isolated piece that helps now.
Test plan
uv run pytest tests/test_consolidation_retry_dedup_by_bank.py tests/test_consolidation_round_limit.py tests/test_consolidation_reschedule_after_round.py -v— all 12 pass./scripts/hooks/lint.shtest_backoff_helper_schedule(5/10/20/40/80/160/320/640/1280, cap at 9th retry)test_retry_is_indefinite(retry_count=100)test_backoff_matches_schedule_by_retry_count[0,1,2,9]New regression guard
tests/test_consolidation_reschedule_after_round.pyexercises the round-limit re-queue end-to-end (the existingtest_consolidation_round_limit.pymockssubmit_async_consolidation, so it doesn't validate the DB row actually lands). Useful regardless of this PR — if the re-queue path ever regresses silently, this catches it.Closes — partial relief for #1842