Ensure Hygraph sync background task uses fresh session by shayancoin · Pull Request #89 · shayancoin/paform

shayancoin · 2025-10-16T17:44:09Z

Summary

open a dedicated SessionLocal in the Hygraph webhook background task and close it after sync completes
add a helper to load the expected API write token and update sync metrics tests to reset cached settings
add a regression test that confirms the webhook background task can execute database work without closed session errors

Testing

pytest backend/tests/test_sync_routes_metrics.py

https://chatgpt.com/codex/tasks/task_e_68f12bc067108330a145d8ae8d126867

Summary by CodeRabbit

Chores
- Internal infrastructure and test setup improvements

coderabbitai · 2025-10-16T17:44:35Z

Caution

Review failed

The pull request is closed.

Walkthrough

Adds SessionLocal import to the sync routes module and introduces test fixture setup with cache clearing for settings configuration. No behavioral changes are introduced at this stage.

Changes

Cohort / File(s)	Summary
Backend API imports `backend/api/routes_sync.py`	Adds `SessionLocal` to imports from `api.db` module alongside existing `get_db` import
Test setup and dependencies `backend/tests/test_sync_routes_metrics.py`	Adds imports for `sqlalchemy.text` and `api.config.get_settings`; introduces `get_settings.cache_clear()` call in client fixture to reset cached settings state

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Ensure Hygraph sync background task uses fresh session #89: Adds SessionLocal to the same file (backend/api/routes_sync.py) and uses it to create per-task database sessions, indicating this PR may be laying groundwork for similar database session management patterns.

Poem

🐰 A session is born, a cache now clear,
SessionLocal arrives without fear,
Test fixtures dance with settings anew,
Foundations are set for what's coming through! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description does not follow the required template sections: it omits the PR Type header, uses “Summary” instead of “Short Description,” and lacks a dedicated “Tests Added” section, so it fails to meet the mandated structure.	Update the description to include the “PR Type” field, rename “Summary” to “Short Description,” add a “Tests Added” section with details of new or modified tests, and ensure the template’s headings and format are followed.
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly and accurately summarizes the primary change—ensuring the Hygraph sync background task uses a fresh SQLAlchemy session—without extraneous details or ambiguity.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1401f65 and e2218f8.

📒 Files selected for processing (2)

backend/api/routes_sync.py (1 hunks)
backend/tests/test_sync_routes_metrics.py (1 hunks)

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

backend/api/security.py (1)
38-41: Fix line length violation.

Line 41 exceeds the configured maximum line length (91 characters, limit is likely 88-89).

Apply this diff to fix the line length:
     expected = _load_expected_write_token(settings)
     if not expected or not secrets.compare_digest(token, expected):
-        raise HTTPException(status_code=HTTP_403_FORBIDDEN, detail="Invalid API write token.")
+        raise HTTPException(
+            status_code=HTTP_403_FORBIDDEN,
+            detail="Invalid API write token.",
+        )
     return True
backend/api/routes_sync.py (2)
66-104: Fix function return type annotation.

The function signature declares -> Dict[str, Any] but returns JSONResponse objects at lines 104 and 144. This causes a mypy type error.

Apply this diff to fix the return type:
 async def hygraph_webhook(
     request: Request,
     background: BackgroundTasks,
     db: Session = Depends(get_db),
-) -> Dict[str, Any]:
+) -> JSONResponse:
72-77: Fix line length violations in docstring.

Lines 74-75 exceed the configured line length limit (91 characters).

Apply this diff to wrap the long lines:
     """
     Webhook receiver:
     - HMAC validated (dependency)
-    - Single size guard (2MB) already enforced by dependency; body/raw set on request.state
+    - Single size guard (2MB) already enforced by dependency;
+      body/raw set on request.state
     - DB dedup via SyncEvent(event_id, body_sha256 unique)
     - 202 fast-ack with background processing (pull_all)
     - Structured JSON log line and Prometheus counters
     """

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d2a35c4 and 1401f65.

📒 Files selected for processing (3)

backend/api/routes_sync.py (3 hunks)
backend/api/security.py (1 hunks)
backend/tests/test_sync_routes_metrics.py (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

backend/api/routes_sync.py (2)

backend/api/db.py (1)

get_db (53-59)

backend/services/hygraph_service.py (1)

pull_all (62-64)

backend/api/security.py (1)

backend/api/config.py (1)

Settings (10-62)

backend/tests/test_sync_routes_metrics.py (2)

backend/api/config.py (1)

get_settings (66-69)

backend/services/hygraph_service.py (1)

HygraphService (11-80)

🪛 GitHub Actions: CI

backend/api/routes_sync.py

[error] 110-110: Mypy: Incompatible return value type (got JSONResponse, expected dict[str, Any]).

[error] 74-75: ruff: Line too long (91 > 88) in docstring or comment.

backend/api/security.py

[error] 41-41: E501 Line too long in security token check.

backend/tests/test_sync_routes_metrics.py

[warning] 1-1: pre-commit reported multiple hooks modified files (trailing whitespace, end-of-file, yaml and ruff fixes). Re-run formatting checks.

🔇 Additional comments (3)

backend/api/routes_sync.py (1)

113-140: LGTM! Fresh session lifecycle correctly implemented.

The background task now properly creates and manages its own database session:

Session created at line 113 using SessionLocal()

Session passed to HygraphService.pull_all at line 115

Session closed in the finally block at line 140

This ensures the background task operates independently of the request-scoped session and avoids closed-session errors.

backend/tests/test_sync_routes_metrics.py (2)

24-24: Good test isolation practice.

Clearing the get_settings cache in the test fixture ensures each test starts with fresh configuration, preventing cross-test pollution from cached settings.

62-82: Effective regression test for session management.

This test validates the core fix: the background task can now execute database operations using its own session without encountering closed-session errors. The test:

Patches pull_all to perform a real DB interaction (SELECT 1)

Captures the scalar result to verify the query executed successfully

Confirms the webhook returns 202 Accepted

This provides good coverage for the session lifecycle changes in routes_sync.py.

backend/api/security.py

coderabbitai · 2025-10-16T17:49:50Z

backend/tests/test_sync_routes_metrics.py

@@ -1,12 +1,15 @@
 from __future__ import annotations


⚠️ Potential issue | 🟡 Minor

Run pre-commit hooks to fix formatting issues.

The pipeline reports that pre-commit hooks modified files due to trailing whitespace, end-of-file issues, and other formatting problems.

Run the following command to apply the fixes:

#!/bin/bash # Description: Run pre-commit hooks to fix formatting issues pre-commit run --all-files

🧰 Tools

🪛 GitHub Actions: CI

[warning] 1-1: pre-commit reported multiple hooks modified files (trailing whitespace, end-of-file, yaml and ruff fixes). Re-run formatting checks.

🤖 Prompt for AI Agents

In backend/tests/test_sync_routes_metrics.py around lines 1 to 1, the CI reported pre-commit hooks modified files for trailing whitespace, EOF and formatting issues; run the project's pre-commit checks locally (e.g. execute "pre-commit run --all-files"), review and stage the modified files, and commit the changes so the repository matches pre-commit standards before pushing the branch.

coderabbitai · 2025-10-17T02:07:51Z

Note

Docstrings generation - SUCCESS
Generated docstrings for this pull request at #153

@shayancoin

…session` Docstrings generation was requested by @shayancoin. * #89 (comment) The following files were modified: * `backend/api/routes_sync.py` * `backend/api/security.py` * `backend/tests/test_sync_routes_metrics.py`

…emy-session

@shayancoin

…session` (#153) Docstrings generation was requested by @shayancoin. * #89 (comment) The following files were modified: * `backend/api/routes_sync.py` * `backend/api/security.py` * `backend/tests/test_sync_routes_metrics.py` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Shayan <shayan@coin.link>

Ensure Hygraph sync background task uses fresh session

1401f65

shayancoin added the codex label Oct 16, 2025 — with ChatGPT Codex Connector

coderabbitai bot reviewed Oct 16, 2025

View reviewed changes

coderabbitai bot mentioned this pull request Oct 17, 2025

📝 Add docstrings to codex/refactor-_process-to-use-local-sqlalchemy-session #153

Merged

Merge branch 'main' into codex/refactor-_process-to-use-local-sqlalch…

e2218f8

…emy-session

shayancoin merged commit 5cde972 into main Oct 17, 2025
0 of 5 checks passed

This was referenced Oct 17, 2025

Ensure webhook sync uses independent sessions #90

Merged

Add API write token configuration and tests #91

Merged

Add session tracking assertions to sync webhook metrics test #167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure Hygraph sync background task uses fresh session#89

Ensure Hygraph sync background task uses fresh session#89
shayancoin merged 2 commits intomainfrom
codex/refactor-_process-to-use-local-sqlalchemy-session

shayancoin commented Oct 16, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

Review failed

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot Oct 16, 2025

Uh oh!

coderabbitai bot commented Oct 17, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

shayancoin commented Oct 16, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

shayancoin commented Oct 16, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 16, 2025 •

edited

Loading

coderabbitai bot commented Oct 17, 2025 •

edited

Loading