Fix required promotion memory follow-ups#8813
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7812be9d6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "result_status": LifecycleState.active.value, | ||
| }, | ||
| control=control, | ||
| source_packet_id=f"promotion_merge_{run_id}", |
There was a problem hiding this comment.
Use a stable operation id for duplicate merges
If the survivor merge commits but the subsequent source-supersede apply fails or the worker crashes before it commits, the short-term source remains promotable. The next maintenance run has a different run_id, so this source_packet_id produces a new operation_id; Firestore apply idempotency is per operation document, so the survivor merge is applied again and increments corroboration_count / advances the ledger a second time before retrying the supersede. Make this merge operation identity deterministic from the source and target memory ids rather than the run id.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 3 files
Confidence score: 3/5
- In
backend/utils/memory/short_term_promotion.py, required-duplicate merges in the same run can collapse into a single operation due to shared run-level source packet state, so later source items may never incrementcorroboration_count; this can undercount evidence and skew promotion decisions — isolate deduplication keys per source item (or equivalent) and add a multi-duplicate same-run test before merging. - In
backend/utils/memory/short_term_promotion.py, derivingsource_packet_idfromrun_idmakesoperation_idnon-deterministic across maintenance runs, so a partial failure (survivor merge committed, source-supersede failed) may not replay cleanly and can leave inconsistent state — make operation IDs deterministic across reruns and verify recovery/idempotency with a failure-in-the-middle test before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed the review comments in 110eeea:\n\n- Changed required-promotion duplicate merge and supersede operation source_packet_id values to stable source/target-derived idempotency keys instead of run_id-derived values.\n- Added same-run multi-duplicate coverage to verify each source increments corroboration independently.\n- Added retry coverage where survivor merge commits, supersede fails, and a later run_id retries without double-incrementing the survivor.\n\nVerification: pre-push selected backend checks passed, including WS-B lifecycle regressions, async blocker scan, OpenAPI check, and black. |
Summary
Fixes #8677
Testing