Skip to content

fix(hit_testing): use flush() instead of commit() to preserve transaction context - #39221

Open
sergioperezcheco wants to merge 1 commit into
langgenius:mainfrom
sergioperezcheco:fix/hit-testing-closed-transaction
Open

fix(hit_testing): use flush() instead of commit() to preserve transaction context#39221
sergioperezcheco wants to merge 1 commit into
langgenius:mainfrom
sergioperezcheco:fix/hit-testing-closed-transaction

Conversation

@sergioperezcheco

Copy link
Copy Markdown
Contributor

HitTestingService.retrieve() called session.commit() after adding the DatasetQuery row, then passed the same session to compact_retrieve_response() which issued another query. Because the controller wraps the whole call in a transaction context manager, the explicit commit closed the transaction before the document lookup ran, raising:

sqlalchemy.exc.InvalidRequestError: Can't operate on closed transaction inside context manager

Replace commit() with flush() so the new row is materialised (gets a primary key, visible to subsequent reads within the same transaction) while leaving the outer context to manage the final commit.

external_retrieve() also calls commit() but its follow-up compact_external_retrieve_response() does not touch the session, so it is left unchanged.

Fixes #38998

@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 19, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Pyrefly Type Coverage

Metric Base PR Delta
Type coverage 54.01% 54.01% 0.00%
Strict coverage 53.50% 53.50% 0.00%
Typed symbols 34,407 34,407 0
Untyped symbols 29,577 29,577 0
Modules 3029 3029 0

…tion context

HitTestingService.retrieve() called session.commit() after adding the
DatasetQuery row, then passed the same session to compact_retrieve_response()
which issued another query. Because the controller wraps the whole call in a
transaction context manager, the explicit commit closed the transaction
before the document lookup ran, raising:

  sqlalchemy.exc.InvalidRequestError: Can't operate on closed transaction
  inside context manager

Replace commit() with flush() so the new row is materialised (gets a primary
key, visible to subsequent reads within the same transaction) while leaving
the outer context to manage the final commit. external_retrieve() also calls
commit() but its follow-up (compact_external_retrieve_response) does not touch
the session, so it is left unchanged.

Fixes langgenius#38998

Signed-off-by: sergioperezcheco <checo520@outlook.com>
@sergioperezcheco
sergioperezcheco force-pushed the fix/hit-testing-closed-transaction branch from f199023 to e1402d4 Compare July 19, 2026 14:49
@Dcsky777

Copy link
Copy Markdown

I modified the corresponding code in the file, but it’s still throwing errors.
屏幕截图 2026-07-20 130657

@sergioperezcheco

Copy link
Copy Markdown
Contributor Author

Hey @Dcsky777 — could you paste the exact error text you're seeing after the change? The screenshot is hard to read on my end.

One common gotcha: if you're running Dify in Docker, editing the .py file on disk doesn't update the running api container — the worker process has the old bytecode cached. You'll need to restart the api worker (e.g. docker compose restart api and docker compose restart worker) so the flush() change actually takes effect. If you mounted the source as a volume, a restart is still needed since the Python process won't hot-reload in production mode.

If the error message changed (or if it's now a different traceback), drop the text here and I'll take a look. The original "Can't operate on closed transaction inside context manager" should be fully resolved by the commit→flush swap, so if you're still seeing that exact message it almost certainly means the running process hasn't picked up the edit yet.

@Dcsky777

Dcsky777 commented Jul 20, 2026 via email

Copy link
Copy Markdown

@sergioperezcheco

Copy link
Copy Markdown
Contributor Author

Thanks for confirming the restart @Dcsky777. Since the error message is identical after the change, the running process almost certainly isn't executing the patched line. Can you verify by grepping the file inside the api container itself?

Run this on the host that runs Docker:

docker compose exec api grep -n "session.commit\|session.flush" /app/api/services/hit_testing_service.py

If you still see session.commit() on line ~188, the image was built before the patch and the on-disk edit was overwritten by the container's baked-in copy (this happens when the source isn't bind-mounted — the image layer wins). In that case the reliable fix is to rebuild: docker compose build api worker && docker compose up -d api worker.

If instead you see session.flush(), then we're chasing a second commit site and I'll need the full traceback (the line number in the stack trace will tell us exactly which call is re-closing the transaction). Either way, paste the grep output and I'll know which path we're on.

@Dcsky777

Dcsky777 commented Jul 20, 2026

Copy link
Copy Markdown

output:
186: session.commit()
230: session.commit()
The modified code was indeed not executed.

@sergioperezcheco

Copy link
Copy Markdown
Contributor Author

Thanks for the grep output @Dcsky777 — that confirms it. Line 186 still shows session.commit() in your container, which means the running image was built before the patch. The on-disk edit you made earlier didn't make it into the image either (likely the container runs a baked-in copy of the code, not a live-mounted volume).

To pick up the fix you have two options:

  1. Apply the one-line change directly in the running container and restart:
docker compose exec api sed -i 's/^        session.commit()$/        session.flush()/' /app/api/services/hit_testing_service.py
docker compose restart api worker
  1. Or rebuild the image from this branch so the change is baked in:
git fetch origin pull/39221/head:pr-39221
git checkout pr-39221
docker compose build api worker
docker compose up -d api worker

After either step, re-run docker compose exec api grep -n 'session.flush\|session.commit' /app/api/services/hit_testing_service.py — you should see session.flush() at line ~186. Let me know if the error persists after that.

@Dcsky777

Copy link
Copy Markdown

You're really awesome.
That’s exactly the issue.
It’s fixed now, thanks for your help.

@sergioperezcheco

Copy link
Copy Markdown
Contributor Author

Glad it helped! If a maintainer has a minute to take a look I'd appreciate the review — happy to adjust anything if needed.

@sergioperezcheco

Copy link
Copy Markdown
Contributor Author

Glad it fixed the issue! The change replaces two session.commit() calls (lines 186 and 230) with session.flush() so the pending objects are written without prematurely closing the transaction context. The original commit was closing the session before the context manager finished, which caused the Can't operate on a closed transaction error on subsequent queries. Bumping for maintainer review when convenient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hit testing fails with "Can't operate on closed transaction inside context manager"

2 participants