Conversation
|
@xhd0728 This focused LightResearch concurrency fix is ready for review. It scopes citation registries by pipeline-run ID, carries that ID through generated tool metadata, and releases the registry on completion; the regression interleaves two runs and verifies independent counters. The focused pytest, pipeline build, MCP metadata smoke, and targeted Ruff checks pass. Since you maintain and review |
|
Thanks for the fix! The registry isolation works in our interleaved-request tests, but further testing found two issues:
Could you move cleanup into a guaranteed lifecycle/finally path that preserves the generated result and handles failures and cancellation? Please also add regression tests for these cases before merging. |
|
Chiming in from #408 (the thread-local attempt this PR supersedes) — the two issues @xhd0728 found both trace to the same design choice: the registry state lives in a class-level global ( Two ways to remove the global-lifecycle problem entirely: Option A — carry the state in the pipeline context (no global at all). The registry state is just Option B — keep the registry-id design, replace the trailing step with allocation-time GC. Add a Either way, the regression tests that would pin this down: (1) interleaved runs keep independent counters (already present), (2) a pipeline that raises after Happy to test either approach against the interleaved MCP-client scenario from the maintainer testing on this thread, or to send a patch if useful — we have the failing repro from #394 wired up locally. |
|
Thanks for the suggestions. I traced this change through the current UI demo execution and citation-rendering paths. There is an additional UI regression beyond the pipeline return-value issue. Citation data remains compatibleThe registry isolation itself looks sound. The pipeline executor extracts those IDs and emits So the registry isolation does not inherently break citation rendering. The problem is the trailing cleanup step and its effect on answer delivery. The cleanup step changes how the UI handles the final answerThis PR changes the end of LightResearch to: In In This means the final answer—including its citation markers—no longer streams into the normal answer body. The final event does not reliably recover the answerThe cleanup tool returns The UI backend then chooses the answer using: There are two relevant cases:
The frontend subsequently replaces the answer body with the answer from the Focused reproductionI reproduced the relevant execution path using the repository’s actual executor function and this PR’s citation functions, with external generation and MCP calls replaced by test doubles.
This is a focused code-path reproduction, not a full browser end-to-end test with live retrieval and generation services. Suggested directionOf the proposed alternatives, I would prefer Option A: carry citation state in the pipeline context. The state can remain scoped to each execution, while the tool continues returning numbered passages in the existing format. This removes the shared registry lifecycle problem and allows A few compatibility details would need to be preserved:
A lifecycle cleanup solution could also work, provided cleanup is outside the normal answer-producing step sequence and runs on success, failure, and cancellation without replacing the answer. I would be more cautious about Option B: TTL-based cleanup. Age-based eviction could remove a registry that a long-running pipeline is still using, and allocation-triggered cleanup only runs when another allocation occurs. It would need explicit handling of active registries and resource bounds rather than relying on TTL alone. Validation before mergingCould we add coverage for:
The isolation fix is valuable, but I would hold off on merging the current version. Preserving |
Summary
Root cause
init_citation_registry()reset a class-level dictionary shared by every request. Starting a second pipeline therefore erased the first pipeline's in-progress citation mappings.Validation
pytest -p no:cacheprovider tests/test_citation_registry.py -qultrarag build examples/demos/LightResearch.yamlruff check --select I,F,E9 tests/test_citation_registry.pyruff check --select I servers/custom/src/custom.pyFixes #394