Switch to Valkey, drop Celery

[martinhoyer]

• Replaced Celery task queue with FastAPI's BackgroundTasks for async processing
• Implemented TaskManager class for background task lifecycle management

[codiumai-pr-agent-free]

Title

Switch to Valkey, drop Celery.

---

User description

• Replaced Celery task queue with FastAPI's BackgroundTasks for async processing
  • Implemented TaskManager class for background task lifecycle management

---

PR Type

Enhancement, Tests, Configuration changes

---

Description

• Replaced Celery with FastAPI BackgroundTasks and Valkey for task management.

• Introduced TaskManager for handling task lifecycle with Valkey.

• Updated tests to reflect the new task management system.

• Removed Celery-related configurations, dependencies, and documentation.

---

Changes walkthrough 📝

	Relevant files
Enhancement	4 files api.py Refactored API to use FastAPI BackgroundTasks and Valkey  +110/-132 html_generator.py Updated HTML generator for Valkey-based task handling        +15/-6    service.py Replaced Celery with Valkey in service layer                          +54/-12  task_manager.py Added TaskManager for Valkey-based task lifecycle management +146/-0
Configuration changes	5 files settings.py Updated settings to use Valkey instead of Redis                    +1/-1      entrypoint.sh Removed Celery-related commands from entrypoint                    +0/-3      test.yml Updated CI workflow to use Valkey instead of Redis and Celery +4/-11    .pre-commit-config.yaml Removed Celery types from pre-commit configuration              +0/-1      compose.yaml Replaced Redis and Celery with Valkey in compose configuration +6/-20
Tests	3 files test_api.py Updated integration tests for Valkey-based task handling  +156/-95 test_html_generator.py Updated unit tests for HTML generator with Valkey                +20/-11  test_service.py Updated service unit tests for Valkey integration                +76/-50
Documentation	1 files README.md Updated documentation to reflect Valkey usage                        +11/-7
Dependencies	1 files pyproject.toml Removed Celery dependency and added Valkey                              +17/-3

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[codiumai-pr-agent-free]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error handling in _handle_task_result has inconsistent behavior. When task_info["status"] == FAILURE, it raises an HTTPException, but when deserializing the result fails, it also raises an HTTPException with a different error message. This could lead to confusing error responses. task_info = task_manager.get_task_info(task_id) if task_info["status"] == FAILURE: error_message = task_info.get("error", "Unknown error") if error_message and "not found" in error_message.lower(): raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=error_message, ) raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=error_message or "Task failed", ) if task_info["status"] == SUCCESS and task_info["result"]: try: logger.debug(f"Task result: {task_info['result']}") result_str = str(task_info["result"]) # Check if result is already formatted or needs formatting try: # Try to deserialize - if it works, the result needs formatting data = deserialize_data(result_str) formatted_result = format_data(data, out_format, logger) except Exception: # If deserialization fails, assume the result is already formatted formatted_result = result_str Task Expiration Tasks are stored with a fixed expiration time (24 hours). For long-running tasks or tasks that need to be preserved for auditing purposes, this might be insufficient. Consider making the expiration configurable or implementing a task cleanup strategy. DEFAULT_TASK_EXPIRY = 60 * 60 * 24 Error Handling The task_wrapper function catches all exceptions generically and stores only the error message. This loses the stack trace and exception type, which could be valuable for debugging. Consider storing more detailed error information. except Exception as exc: # Update task with error information error_message = str(exc) self.logger.fail(f"Task {task_id} failed: {error_message}") self.update_task(task_id, FAILURE, error=error_message)

[codiumai-pr-agent-free]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Pass task status to callback Suggestion Impact: The commit implemented the suggestion by passing the task status and result to the generate_status_callback function, and also added additional logic to handle error messages for FAILURE status. code diff: + # For FAILURE status, use the error message if available + result = task_info.get("result") + if task_info["status"] == FAILURE and task_info.get("error"): + result = task_info["error"] + return HTMLResponse( - content=html_generator.generate_status_callback(task_id, status_callback_url, logger), + content=html_generator.generate_status_callback( + task_id, + status_callback_url, + logger, + status=task_info["status"], + result=result, + ), The function is not passing the task status and result to the generate_status_callback function. This means the status page will always show "PENDING" status regardless of the actual task status, as the function uses default parameter values. src/tmt_web/api.py [369-384] @app.get("/status/html", response_class=HTMLResponse) def get_task_status_html( request: Request, task_id: Annotated[ str | None, Query( description="Task ID to check status for", examples=["123e4567-e89b-12d3-a456-426614174000"], ), ] = None, ) -> HTMLResponse | RedirectResponse: """Get task status in HTML format.""" logger.debug(f"HTML status check for task {task_id}") if not task_id: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail="task-id is required", ) task_info = task_manager.get_task_info(task_id) if task_info["status"] == SUCCESS and task_info["result"]: # For successful tasks, redirect to root endpoint return RedirectResponse( url=f"{settings.API_HOSTNAME}/?task-id={task_id}", status_code=303, # Use 303 See Other for GET redirects ) status_callback_url = f"{settings.API_HOSTNAME}/status/html?task-id={task_id}" return HTMLResponse( - content=html_generator.generate_status_callback(task_id, status_callback_url, logger), + content=html_generator.generate_status_callback( + task_id, + status_callback_url, + logger, + status=task_info["status"], + result=task_info.get("result"), + ), ) [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical functional issue where the status page would always show "PENDING" regardless of actual task status, by correctly passing the task status and result to the generate_status_callback function.	High
	✅ Handle JSON decode errors The function is decoding JSON data without handling potential JSON decoding errors. If the task data in Valkey is corrupted or not valid JSON, the function will raise a json.JSONDecodeError and crash the application. src/tmt_web/utils/task_manager.py [61-75] def get_task_info(self, task_id: str) -> dict[str, Any]: """Get information about a task.""" task_key = self._get_task_key(task_id) task_data = self.client.get(task_key) if not task_data: self.logger.fail(f"Task {task_id} not found") return { "id": task_id, "status": FAILURE, "error": "Task not found", "result": None, } - return json.loads(task_data) + try: + return json.loads(task_data) + except json.JSONDecodeError: + self.logger.fail(f"Corrupted task data for {task_id}") + return { + "id": task_id, + "status": FAILURE, + "error": "Corrupted task data", + "result": None, + } [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: This suggestion addresses an important error handling issue where corrupted JSON data could crash the application. Adding try-except for JSONDecodeError provides proper error recovery and maintains system stability.	Medium
	✅ Inconsistent image tag reference Suggestion Impact: The commit addressed the inconsistency in the Valkey image reference, but instead of changing to 'valkey:alpine' as suggested, it changed to 'valkey/valkey:latest' which adds the proper namespace to the image reference while keeping the 'latest' tag code diff: - podman run -d --pod tmt-web-pod --name valkey valkey:latest + podman run -d --pod tmt-web-pod --name valkey valkey/valkey:latest The image reference for Valkey appears to be incorrect. In the compose.yaml file, the image is specified as 'valkey:alpine', but in the workflow it's 'valkey:latest'. This inconsistency could cause the workflow to fail if the 'latest' tag doesn't exist or behaves differently. .github/workflows/test.yml [49] -podman run -d --pod tmt-web-pod --name valkey valkey:latest +podman run -d --pod tmt-web-pod --name valkey valkey:alpine [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies an inconsistency between the Valkey image tags used in compose.yaml ('valkey:alpine') versus the workflow file ('valkey:latest'). This inconsistency could cause deployment failures in CI/CD if the tags reference different images or if one tag doesn't exist.	Medium
	✅ Handle JSON serialization errors The function is missing error handling for JSON decoding of task_data. If the task data is corrupted, the function will raise a json.JSONDecodeError and crash the application. Additionally, there's no validation that the result can be properly serialized to JSON before storing it. src/tmt_web/utils/task_manager.py [77-101] def update_task( self, task_id: str, status: str, result: Any = None, error: str | None = None ) -> None: """Update task status and result.""" task_key = self._get_task_key(task_id) task_data = self.client.get(task_key) if not task_data: self.logger.warning(f"Trying to update non-existent task {task_id}") return - task_info = json.loads(task_data) - task_info["status"] = status + try: + task_info = json.loads(task_data) + task_info["status"] = status - if result is not None: - task_info["result"] = result + if result is not None: + task_info["result"] = result - if error is not None: - task_info["error"] = error + if error is not None: + task_info["error"] = error - # Update timestamp - task_info["updated_at"] = datetime.now(tz=UTC).isoformat() + # Update timestamp + task_info["updated_at"] = datetime.now(tz=UTC).isoformat() - # Store updated info - self.client.set(task_key, json.dumps(task_info), ex=DEFAULT_TASK_EXPIRY) + # Store updated info + self.client.set(task_key, json.dumps(task_info), ex=DEFAULT_TASK_EXPIRY) + except json.JSONDecodeError: + self.logger.fail(f"Failed to decode task data for {task_id}") + except (TypeError, ValueError) as e: + self.logger.fail(f"Failed to encode task data for {task_id}: {e}") [Suggestion has been applied] Suggestion importance[1-10]: 7 __ Why: The suggestion adds critical error handling for both JSON decoding and encoding operations, preventing potential application crashes when dealing with corrupted data or non-serializable objects.	Medium
Update

[martinhoyer]

@coderabbitai full review

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request transitions the application from Redis and Celery to Valkey and FastAPI’s background tasks. The changes update CI workflows, configurations, documentation, and service definitions by replacing Redis containers and environment variables with those for Valkey. Celery-specific code in the entrypoint, API, service, and tests has been removed and refactored to use FastAPI background processing. A new TaskManager class has been introduced to manage task states in Valkey. Related tests have also been updated to assert task status rather than Celery responses.

Changes

Files	Change Summary
.github/workflows/test.yml, .pre-commit-config.yaml, pyproject.toml	Updated CI configurations and dependency definitions to replace Redis and Celery with Valkey; adjusted environment variables accordingly.
README.md	Revised documentation and instructions: removed Celery references and REDIS_URL/USE_CELERY, added VALKEY_URL and explained the new FastAPI background task architecture.
compose.yaml	Removed Redis and Celery services; added a new Valkey service with updated image, environment variable (VALKEY_URL), dependency, and healthcheck command.
entrypoint.sh, src/tmt_web/settings.py	Removed Celery startup logic in the entrypoint and replaced configuration from REDIS_URL to VALKEY_URL in settings.
src/tmt_web/api.py, src/tmt_web/generators/html_generator.py, src/tmt_web/service.py	Refactored task handling and API logic to use FastAPI background tasks and a new task management system; updated endpoints and response formatting; removed Celery-specific code.
src/tmt_web/utils/task_manager.py	New file added: Implements TaskManager to create, update, and retrieve background tasks using Valkey for state storage.
tests/integration/test_api.py, tests/unit/test_html_generator.py, tests/unit/test_service.py	Updated test suite to reflect the migration from Celery to background tasks, adjusting fixtures, assertions, and test class names accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant C as Client
    participant A as API (FastAPI)
    participant S as Service (process_request)
    participant TM as TaskManager
    participant V as Valkey

    C->>A: Send request
    A->>S: Call process_request with background task
    S->>TM: Create new task (status: PENDING)
    TM->>V: Store task information
    S->>TM: Execute task worker (_process_request_worker)
    TM->>V: Update task state (STARTED → SUCCESS/FAILURE)
    C->>A: Poll for task status using task ID
    A->>TM: Retrieve task info
    TM->>C: Return task status and result

Loading

Note

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback.
Learn more here.

---

Note

⚡️ Faster reviews with caching

CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.
Enjoy the performance boost—your workflow just got faster.

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 13

🔭 Outside diff range comments (1)

src/tmt_web/api.py (1)

158-259: 🧹 Nitpick (assertive)

Clean separation of concerns in root endpoint.
The route handles both retrieving existing tasks and creating new ones. This is convenient, but can grow complex over time. Consider splitting into two endpoints if the logic expands to maintain clarity and single responsibility.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4394861 and 4ccc5a1.

📒 Files selected for processing (14)

• .github/workflows/test.yml (1 hunks)
• .pre-commit-config.yaml (0 hunks)
• README.md (3 hunks)
• compose.yaml (2 hunks)
• entrypoint.sh (0 hunks)
• pyproject.toml (3 hunks)
• src/tmt_web/api.py (11 hunks)
• src/tmt_web/generators/html_generator.py (2 hunks)
• src/tmt_web/service.py (4 hunks)
• src/tmt_web/settings.py (1 hunks)
• src/tmt_web/utils/task_manager.py (1 hunks)
• tests/integration/test_api.py (14 hunks)
• tests/unit/test_html_generator.py (4 hunks)
• tests/unit/test_service.py (3 hunks)

💤 Files with no reviewable changes (2)

• .pre-commit-config.yaml
• entrypoint.sh

🧰 Additional context used

🧬 Code Graph Analysis (3)

tests/unit/test_html_generator.py (1)

src/tmt_web/generators/html_generator.py (1)

• generate_status_callback (42-67)

src/tmt_web/service.py (3)

src/tmt_web/formatters.py (1)

• format_data (35-46)

src/tmt_web/models.py (3)

• PlanData (45-69)
• TestData (17-42)
• TestPlanData (72-76)

src/tmt_web/utils/task_manager.py (1)

• execute_task (117-156)

src/tmt_web/api.py (4)

src/tmt_web/utils/task_manager.py (1)

• get_task_info (61-84)

src/tmt_web/formatters.py (1)

• deserialize_data (54-61)

src/tmt_web/service.py (1)

• process_request (166-212)

src/tmt_web/generators/html_generator.py (1)

• generate_status_callback (42-67)

🔇 Additional comments (69)

src/tmt_web/settings.py (1)

4-4: Appropriate transition from Redis to Valkey in configuration.

Replacing REDIS_URL with VALKEY_URL and updating the default URL to use the valkey protocol aligns with the PR objective of switching from Redis to Valkey for task state storage.

.github/workflows/test.yml (3)

45-45: Comment update is appropriate for the new architecture.

The comment update correctly reflects the new focus on background tasks rather than Redis-specific functionality.

---

47-47: Job name updated to reflect the service change.

The job name update accurately reflects the transition from Redis to Valkey.

---

54-54: Environment variable updated to use Valkey URL.

The environment variable change is consistent with the changes in settings.py, replacing REDIS_URL with VALKEY_URL.

pyproject.toml (3)

137-139: Line length exception added for HTML generator.

Adding a line length exception for the HTML generator file is reasonable, as HTML-related code often contains long lines that are difficult to break without affecting functionality.

---

162-172: Added pytest warning filters for cleaner test output.

The pytest warning filters help suppress expected warnings to maintain cleaner test output:

1. Ignoring warnings for test classes with names starting with "Test" that aren't actual test classes
2. Ignoring deprecation warnings from dependencies (pint and docutils) which come from tmt

This is a good practice for improving the signal-to-noise ratio in test results.

---

30-30:

Details

✅ Verification successful

Dependency updated from Celery to Valkey.

The addition of Valkey dependency and removal of the Celery dependency aligns with the PR objective of dropping Celery in favor of FastAPI's background tasks with Valkey for storage.

---

🌐 Web query:

What is Valkey and how does it compare to Redis?

💡 Result:

Valkey is an open-source, high-performance key-value store developed as a community-driven alternative to Redis, following Redis's shift to a more restrictive licensing model. Backed by major tech companies like AWS, Google Cloud, and Oracle, Valkey prioritizes scalability, multi-threading, and true open-source governance under the BSD 3-clause license. Below is a detailed comparison:

Key Differences

Feature	Valkey	Redis
Licensing	Open-source (BSD 3-clause) [2][10]	Source-available (RSALv2/SSPLv1) [3][8]
Threading	Multi-threaded I/O and command execution [2][6][10]	Single-threaded [2][6][8]
Performance	Up to 1.19M requests/sec [2][6]	High but limited by single-threaded model [6][8]
Scalability	Automatic cluster failover, 1,000+ nodes [2][6]	Supports clustering with manual tuning [2][8]
Memory Efficiency	20% improvement via optimized structures [6][8][10]	Efficient but less optimized [8][10]
Community	Linux Foundation, AWS, Google, Oracle [3][10]	Commercial backing by Redis Inc. [8]

Licensing and Governance

• Valkey: Fully open-source with no usage restrictions, ensuring long-term accessibility [2][10].
• Redis: Transitioned to restrictive licenses (RSALv2/SSPLv1), limiting commercialization and prompting the Valkey fork [3][8].

Performance and Architecture

• Multi-threading: Valkey’s enhanced architecture leverages modern multi-core CPUs for 3x higher throughput than Redis in some benchmarks [2][6].
• RDMA Support: Valkey offers experimental Remote Direct Memory Access for low-latency networks, absent in Redis [2][8].
• Latency: Both deliver sub-millisecond response times, but Valkey’s multi-threading reduces contention under high concurrency [6][8].

Feature Comparison

• Shared: Strings, lists, sets, hashes, streams, and geospatial indexes [2][6][8].
• Redis Exclusives: Native JSON support, time-series data, and vector search for AI [6][8].
• Valkey Advantages: Per-slot metrics for granular monitoring and Rust-based memory safety [2][6][10].

Use Cases

• Valkey: Ideal for scalable, high-throughput workloads (e.g., real-time analytics, caching) and environments requiring open-source compliance [2][4][8].
• Redis: Suited for applications needing specialized data structures (e.g., session management, AI/ML) and commercial support [6][8].

Ecosystem and Adoption

• Major cloud providers (AWS, Google Cloud, Oracle) now offer Valkey-managed services with cost reductions up to 30% [4][10].
• Redis retains popularity but faces declining interest due to licensing; Valkey has rapidly gained traction since its 2024 launch [3][4][10].

In summary, Valkey excels in scalability, openness, and modern architectures, while Redis remains strong for niche data structures and enterprise support. The choice hinges on licensing preferences, performance needs, and ecosystem requirements.

Citations:

• 1: https://study.com/academy/lesson/valley-definition-examples-quiz.html
• 2: https://www.dragonflydb.io/guides/valkey-vs-redis
• 3: https://devops.com/valkey-is-rapidly-overtaking-redis-2/
• 4: https://www.kedos.co.uk/p/news/valkey-vs-redis-aws
• 5: https://eschooltoday.com/learn/valley/
• 6: https://betterstack.com/community/comparisons/redis-vs-valkey/
• 7: https://www.vedantu.com/evs/the-valley
• 8: https://redisson.pro/blog/valkey-vs-redis-comparision.html
• 9: https://www.britannica.com/science/valley
• 10: https://experience.percona.com/valkey-vs-redis-choosing-the-right-in-memory-data-store/comparison-table

---

Approved: Dependency Update Verified

The removal of Celery and addition of the "valkey" dependency (as seen at line 30 in pyproject.toml) aligns with the PR objective to replace Celery with FastAPI’s native background tasks using Valkey for storage. Our verification confirms that Valkey is an open-source, high-performance key-value store offering multi-threading and scalability advantages—qualities that support its adoption over legacy systems like Redis. This dependency update is in line with the intended improvements.

compose.yaml (3)

9-9: Environment variable updated to use Valkey URL.

The environment variable has been appropriately changed from REDIS_URL to VALKEY_URL with the correct valkey protocol.

---

21-22: Service dependency updated from Redis to Valkey.

The service dependency has been correctly updated to depend on the valkey service instead of redis.

---

30-30: Healthcheck command updated for Valkey.

The healthcheck command has been correctly updated to use valkey-cli instead of redis-cli.

src/tmt_web/generators/html_generator.py (7)

4-4: Importing Any is appropriate for flexible type handling.
This aligns well with Python typing for result in the new function signature.

---

42-48: Refactor to use direct task parameters instead of Celery AsyncResult.
Switching to task_id, status, and result clarifies the function's responsibilities and removes dependencies on Celery.
Please ensure all callers pass the expected arguments.

---

52-52: Docstring for task_id is updated correctly.
Thanks for keeping the docstring in sync with the new signature.

---

55-56: Docstring changes for status and result match the refactor.
Accurate parameter documentation is essential for maintainability.

---

60-60: Enhanced debug log includes the task_id.
This extra context is helpful for tracking task state in logs.

---

62-63: data dict keys align well with the updated parameters.
task_id and status are clearly labeled, improving clarity.

---

65-65: Including result in data captures the task output.
This is consistent with the function’s new responsibilities.

tests/unit/test_html_generator.py (5)

125-125: Replacing the Celery mock with a string task_id.
This simplifies test setup and aligns with the new generate_status_callback signature.

Also applies to: 128-130

---

141-141: Retrying test uses direct task_id and status="RETRYING".
The test remains straightforward and accurately reflects the new parameter usage.

Also applies to: 144-146

---

155-155: SUCCESS test now passes task_id and HTML result directly.
Good approach to validate the final rendered result without relying on Celery mocks.

Also applies to: 160-162

---

170-170: FAILURE test with direct task_id and error message.
This is consistent with the updated function’s handling of error states.

Also applies to: 174-176

---

186-186: UNKNOWN status test transitions smoothly to the new parameter style.
Ensures robust coverage for unrecognized statuses.

Also applies to: 189-191

src/tmt_web/service.py (8)

1-1: Module docstring updated to reflect the FastAPI + Valkey transition.
Clear statement of the new approach improves discoverability.

---

7-7: Imported BackgroundTasks from FastAPI for async processing.
This is a necessary step in removing Celery dependencies.

---

16-16: Import task_manager from tmt_web.utils.
Centralizing task execution logic is a solid architectural choice.

---

166-168: New process_request signature accommodates background_tasks.
Good design for queuing tasks without blocking.

---

181-181: Docstring accurately describes the new async workflow.
The explanation of returning a task ID is helpful for clients.

Also applies to: 183-193

---

199-212: Refactored to utilize task_manager.execute_task.
Removing Celery references simplifies the pipeline and shows a concise approach to scheduling.

---

263-264: Immediate formatting of data when out_format is specified.
Users can choose the format on the fly, providing a flexible solution.

---

268-268: Fall back to serialized data if no format is specified.
Maintains backward compatibility and future extensibility.

README.md (4)

15-15: Docs updated to remove Celery-specific references.
The new phrasing "For quick development without container setup" aligns well with the transition away from Celery.

---

32-33: Environment variable reference is clear.
Defining VALKEY_URL with a default of valkey://localhost:6379 is consistent, though it reuses the default Redis port.

---

57-59: Correct reflection of the new asynchronous workflow.
Now returning the ID of a created background task fits the updated architecture.

---

18-18:

Details

❓ Verification inconclusive

Validate the default path.
Ensure that CLONE_DIR_PATH=/var/tmp/test does not conflict with other usage or permissions.

---

🏁 Script executed:

#!/bin/bash
# Confirm no conflicting usage in the codebase.
rg 'CLONE_DIR_PATH' -A 5

Length of output: 2318

---

Action Required: Validate the Clone Directory Setting

The README example sets CLONE_DIR_PATH=/var/tmp/test, but in the code (specifically in src/tmt_web/settings.py) the default is defined as ./.repos/. Please verify that using /var/tmp/test does not lead to permission issues or unexpected behavior in other parts of the system (e.g., as used in tests within tests/unit/test_git_handler.py). If /var/tmp/test is intended solely for demo purposes, consider clarifying its usage or aligning the README example with the documented default path.

• Locations to review:
  • README.md (Line 18): Example CLI command using /var/tmp/test
  • src/tmt_web/settings.py: Default value of CLONE_DIR_PATH is set to ./.repos/
  • tests/unit/test_git_handler.py: Usage of settings.CLONE_DIR_PATH in path resolution

tests/unit/test_service.py (10)

3-3: New JSON import usage.
No issues identified. The import is pertinent for encoding/decoding task manager data.

---

16-16: Additional import for process_request.
Change is aligned with the new approach, removing Celery references in favor of direct processing.

---

20-20: Task manager import.
No issues—just be mindful of test coverage for concurrency edge cases in the future.

---

385-408: Comprehensive parameter validation tests.
Good job testing _process_request_worker with missing parameters.

---

438-464: Thorough background task test coverage.
Mocked calls and assertions are accurate; confirms task_manager.execute_task is invoked.

---

464-483: Proper logic for no output format scenario.
Excellent check ensuring serialize_data is used when format is None.

---

485-506: Validation for non-existent task updates.
Great approach: verifying no set call is made and logging a warning.

---

508-527: Corrupted data handling.
Properly ensures tasks with invalid JSON are set to FAILURE.

---

529-539: Update flow for decoding errors.
Robust check ensures we log decode failures.

---

544-568: Handling JSON encoding failures.
Good coverage for unserializable objects to confirm error logging.

tests/integration/test_api.py (19)

35-37: Corrected HTML status page validation.
Ensures the “Processing…” and “Status: PENDING” text is present.

---

51-53: Similar HTML status check for test path.
Maintains consistent UI feedback across varied requests.

---

64-66: HTML format check.
The “Processing…” state for tasks is verified properly.

---

76-80: JSON response structure.
Verifies that "id", "status", and "status_callback_url" are returned as expected.

---

87-93: YAML format still returns JSON for status.
Good clarification that status remains JSON.

---

103-106: HTML plan request checks.
Ensures consistent status messaging for plan-based queries.

---

117-120: Combined test/plan request check.
Properly verifies pending status with a multi-parameter request.

---

133-135: New approach to not found error handling.
Now returning appropriate 404 instead of a Celery-driven response.

---

146-154: Refined not-found error tests.
Divides “test not found” vs. “plan not found,” ensuring correct 404 responses.

---

155-182: Generic error task check.
Properly simulates a generic FAILURE scenario returning 500.

---

183-218: Pending state test coverage.
Great approach: verifying JSON vs. HTML format logic for pending tasks.

---

243-244: Checks Valkey presence in health dependencies.
Reflects new architecture.

---

250-265: Valkey ping failure test.
Ensures partial dependency failure is tolerated while overall status remains “ok.”

---

267-297: New class for background tasks.
Thorough end-to-end integration tests verifying pending→success logic.

---

511-514: Status page for invalid task ID
Placing “Task not found” under “FAILURE” in the HTML response is correct.

---

515-549: Ensures 404 for missing tests/plans with background tasks.
Verifies the newly introduced approach of returning 404 for resource not found.

---

551-558: Health check with Valkey.
Correct test for verifying “valkey” is reported as “ok” if running.

---

569-585: Malformed result JSON error handling.
Correctly returns 500, verifying robust error handling in _handle_task_result.

---

597-611: Not-found error scenario in task result.
App now returns a clean 404 for “not found” tasks.

src/tmt_web/utils/task_manager.py (1)

159-160: Confirm that a global TaskManager suits all usage contexts.
Having a single shared instance is typically fine unless multi-tenancy or specialized configurations are required. If so, you might need separate manager instances.

src/tmt_web/api.py (5)

132-139: Handle potential partial or invalid result formats gracefully.
This segment attempts to deserialize and then format data if deserialization succeeds. Although falling back to raw data is logical, you may consider logging or differentiating partial failure for improved observability.

---

264-290: Sufficiently robust background task invocation.
This integration with service.process_request is straightforward. Ensure concurrency limits and resource usage are managed if you anticipate a high volume of tasks.

---

292-316: Good practice in exception handling.
Catching GeneralError and mapping it to specific HTTP codes provides a user-friendly approach. The logic for 404 and 400 statuses is solid.

---

352-397: Redirect on success is user-friendly.
When the task is successful, returning a 303 redirect helps clients fetch final results from the root endpoint properly. This is a neat approach for human-friendly flows.

---

417-455: Health check approach covers key dependencies.
Pinging Valkey is enough under the new architecture. If more dependencies or advanced checks are introduced later, this function can be extended to run additional validations.

[psss]

@janhavlin, @therazix, could you please have a look and review? Thanks.

[martinhoyer]

@coderabbitai full review

[coderabbitai]

Actionable comments posted: 2

🔭 Outside diff range comments (2)

src/tmt_web/service.py (1)

166-177: 🛠️ Refactor suggestion

out_format should be Optional in both signature and docstring

_process_request_worker purposely treats out_format as an optional flag (if out_format:) and the unit–tests call it with None.
However, the type annotation is Literal["html", "json", "yaml"] (non-optional) and the public process_request() forwards the argument unconditionally, making None a type violation and a potential source of static-type-checker noise.

-    out_format: Literal["html", "json", "yaml"],
+    out_format: Literal["html", "json", "yaml"] | None = None,

Amend the docstring accordingly and apply the same change to _process_request_worker.
This keeps the API truthful, avoids mypy/pyright complaints, and aligns with the actual behaviour.

tests/integration/test_api.py (1)

277-297: 🛠️ Refactor suggestion

Polling loops can hang indefinitely — add an upper time bound

The tight while True: loops poll the API until the task reaches a terminal state.
If the task never completes (e.g. Valkey outage in CI) the test will block forever and eventually be killed by the runner.

Guard the loop with a deadline:

- while True:
+ timeout = time.time() + 30  # fail after 30 s
+ while time.time() < timeout:
     ...
- pytest.fail("Unknown status: " + json_data["status"])
+ pytest.fail("Timeout waiting for task completion")

Repeat for every polling loop in this file.

Also applies to: 312-329, 372-391, 405-411

♻️ Duplicate comments (2)

README.md (1)

39-43: Nice architectural overview.

Great job documenting FastAPI's background tasks and Valkey usage, making architecture more transparent.

Consider adding a link to Valkey's official docs to help readers.

src/tmt_web/utils/task_manager.py (1)

32-37: 🛠️ Refactor suggestion

Fail-fast Valkey connection will crash the whole application

TaskManager connects to Valkey in the constructor that is executed at import time (task_manager = TaskManager() at EOF).
If Valkey is down or the URL is mis-configured, the entire FastAPI app (and all unit tests) will raise on import, preventing even endpoints that do not rely on background tasks from starting.

Wrap the connection in a try/except and degrade gracefully (e.g. raise a custom TaskBackendUnavailable, defer connection until first use, or run in “noop” mode).

-        self.client = Valkey.from_url(settings.VALKEY_URL)
-        self.logger.debug(f"Connected to Valkey at {settings.VALKEY_URL}")
+        try:
+            self.client = Valkey.from_url(settings.VALKEY_URL)
+            self.logger.debug(f"Connected to Valkey at {settings.VALKEY_URL}")
+        except Exception as exc:
+            self.logger.fail(f"Unable to connect to Valkey ({settings.VALKEY_URL}): {exc}")
+            # Option 1: re-raise a clearer exception
+            # Option 2: fall back to an in-memory dict so the app can start
+            raise

🧹 Nitpick comments (11)

pyproject.toml (1)

30-31: Dependency updated to Valkey.

The Celery dependency has been correctly replaced with Valkey. However, consider adding a version constraint to ensure compatibility, similar to other dependencies in this file.

Consider updating to specify a version range:

-  "valkey",
+  "valkey~=0.1.0",  # Use appropriate version

src/tmt_web/service.py (1)

263-268: Early return blocks later formatting for JSON‐storage use-case

When out_format is truthy, the data is formatted immediately and the raw model is lost.
For "json" this means we store formatted JSON in Valkey, not the “raw” model (which could later be formatted to HTML/YAML on demand).
If the intended design is to keep the raw data for any future format requests, move the formatting decision out of the worker and postpone it until retrieval.

If immediate formatting is intentional, at least document this side-effect in the docstring so API users know they cannot request another representation later.

src/tmt_web/utils/task_manager.py (1)

135-156: Result may be non-serialisable – handle before update_task

update_task() already logs encode errors, but the original result is discarded, leaving the task in SUCCESS with error: null and no useful payload.

Consider pre-serialising inside the wrapper:

try:
    json.dumps(result)  # probe
except (TypeError, ValueError):
    result = str(result)

or add a custom encoder to ensure callers always receive a usable JSON value.

tests/unit/test_service.py (2)

418-424: Unnecessary patches can mask regressions

serialize_data is patched but never exercised because the call path never reaches it (the worker runs later in a background task).
Removing the unused patch keeps the test focused and avoids false positives if the implementation changes.

-    # Mock serialize_data to ensure it's NOT called
-    mocker.patch("tmt_web.service.serialize_data")

---

425-436: Test relies on a real Valkey connection

process_request() invokes the real task_manager.execute_task; without a running Valkey instance this test will break outside CI.
Either:

1. Patch tmt_web.service.task_manager.execute_task (as done in test_main_with_background_tasks), or
2. Monkey-patch settings.VALKEY_URL to a local fake and stub Valkey.from_url.

Keeping external dependencies out of unit tests improves reliability and speed.

tests/integration/test_api.py (2)

213-217: Mixing HTML‐requested format with JSON assertions may surprise future readers

This block requests the root endpoint with format=html, but immediately parses the response as JSON.
Although the current implementation returns JSON while a task is pending, a future refactor could legitimately decide to always serve HTML for the html format, breaking this test without obvious intent.

Consider either

1. requesting format=json here, or
2. adding an explanatory comment that the API purposefully downgrades to JSON until the task finishes.

---

506-514: Assertion message relies on hard-coded text

The HTML status page is verified via string containment checks like "Error: Task not found".
A small wording tweak in the template would break the test even though the behaviour is intact.

Prefer structural assertions (e.g. CSS selector, JSON API) or, at minimum, factor the literal into a single constant reused by both generator and tests to reduce brittleness.

src/tmt_web/api.py (4)

126-147: json.loads(formatted_result) can raise when output is not JSON

If format_data() returned YAML (legal for out_format="yaml") but the caller accidentally asked for "json", the error is caught and re-raised as a 500 after logging logger.fail().
That’s fine, but the primary cause (“result is not valid JSON”) is lost to clients, and we already did a best-effort deserialisation above.

Consider a safer fallback that still returns the raw string instead of crashing:

- return JSONResponse(content=json.loads(formatted_result))
+ try:
+     return JSONResponse(content=json.loads(formatted_result))
+ except json.JSONDecodeError:
+     # Fallback to plain text to avoid 500 on format mismatch
+     logger.warn("Result is not valid JSON, returning plain text")
+     return PlainTextResponse(content=formatted_result, status_code=200)

This makes the API more forgiving.

---

246-291: Inconsistent UX for html format when task_id is supplied

root() delegates to _handle_task_result() which always returns JSON for PENDING/FAILURE states, even if the caller explicitly requested format=html.
At the same time /status/html serves a dedicated HTML status page.

Users therefore receive:

• / with format=html → JSON while pending
• /status/html → HTML while pending

Aligning both endpoints would reduce confusion. Two options:

1. Reuse the HTML status page here when task_info["status"] != SUCCESS.
2. Document that clients must follow the status-callback URL to get HTML updates.

---

429-436: Lost diagnostics when Valkey ping fails

We swallow any exception from client.ping() and only store "failed".
Log the exception so operators can see why the ping failed:

 except Exception as e:
     valkey_status = "failed"
+    logger.warn(f"Valkey ping failed: {e}")

This keeps the public health response minimal while preserving details in logs.

---

399-414: Expose errors via a dedicated field instead of overloading result

TaskOut.result is populated with the error string when the task fails.
Mixing successful payloads and error messages in the same attribute can confuse consumers and complicate schema evolution.

Introduce an explicit error: str | None attribute:

 class TaskOut(BaseModel):
     id: str
     status: str
-    result: str | None = None
+    result: str | None = None
+    error: str | None = None
     status_callback_url: str | None = None

and adjust _to_task_out() accordingly. This keeps semantics clear and future-proof.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4394861 and e98c492.

📒 Files selected for processing (14)

• .github/workflows/test.yml (1 hunks)
• .pre-commit-config.yaml (0 hunks)
• README.md (3 hunks)
• compose.yaml (2 hunks)
• entrypoint.sh (1 hunks)
• pyproject.toml (3 hunks)
• src/tmt_web/api.py (11 hunks)
• src/tmt_web/generators/html_generator.py (2 hunks)
• src/tmt_web/service.py (4 hunks)
• src/tmt_web/settings.py (1 hunks)
• src/tmt_web/utils/task_manager.py (1 hunks)
• tests/integration/test_api.py (14 hunks)
• tests/unit/test_html_generator.py (4 hunks)
• tests/unit/test_service.py (3 hunks)

💤 Files with no reviewable changes (1)

• .pre-commit-config.yaml

🧰 Additional context used

🧬 Code Graph Analysis (1)

tests/unit/test_service.py (2)

src/tmt_web/service.py (4)

• process_request (166-212)
• process_test_request (59-88)
• process_testplan_request (123-163)
• _process_request_worker (215-268)

src/tmt_web/utils/task_manager.py (2)

• update_task (86-115)
• get_task_info (61-84)

🔇 Additional comments (22)

entrypoint.sh (1)

11-11: Script simplification aligns with architectural changes

The entrypoint script has been simplified to directly launch uvicorn, eliminating the conditional logic previously needed to start either the API server or Celery worker. This change makes sense given the removal of Celery from the architecture.

src/tmt_web/settings.py (1)

4-4: Environment configuration updated for Valkey

The replacement of REDIS_URL with VALKEY_URL correctly updates the connection configuration to use Valkey instead of Redis. The URL scheme change from "redis://" to "valkey://" properly reflects the architectural shift.

tests/unit/test_html_generator.py (5)

125-130: Test refactored to match new task management approach

The test has been correctly updated to pass explicit parameters (task_id, status, result) instead of a mocked Celery AsyncResult object, which aligns with the new architecture using FastAPI background tasks and Valkey.

---

144-146: Consistent parameter updates for retrying status test

This test case has been properly updated to match the new function signature, ensuring consistent testing approach across all status variants.

---

160-162: Success status test updated correctly

The success case test has been appropriately refactored to use the new direct parameter approach instead of a mocked task result.

---

174-176: Failure status test updated correctly

The failure case test has been appropriately updated to match the new function signature, maintaining test coverage for error scenarios.

---

189-191: Unknown status test updated correctly

The unknown status test case has been properly updated to use the new parameter structure, completing the comprehensive test coverage for all status variants.

.github/workflows/test.yml (3)

45-46: Comment updated to reflect architectural changes

The comment has been appropriately updated to mention Valkey instead of Redis, which aligns with the architectural changes in the project.

---

47-49: Valkey container replaces Redis in workflow

The Redis container has been correctly replaced with a Valkey container using the Alpine image variant as suggested in previous reviews. This change supports the overall transition from Redis to Valkey for task state management.

---

54-54: Environment variable updated for Valkey

The environment variable has been updated from REDIS_URL to VALKEY_URL with the appropriate URL scheme change. This ensures the container configuration aligns with the settings updates in the application code.

src/tmt_web/generators/html_generator.py (4)

4-4: Type import update aligns with architectural changes.

Replacing AsyncResult import with generic Any import supports the transition from Celery to FastAPI background tasks with Valkey.

---

42-48: Clean function signature refactoring.

The function signature has been effectively refactored to accept explicit parameters instead of relying on a Celery AsyncResult object. The default values and type annotations are properly implemented.

---

52-56: Documentation properly updated.

The docstring has been correctly updated to reflect the new parameter structure and descriptions.

---

60-66: Task status handling updated correctly.

The implementation now properly constructs the data dictionary from explicit parameters instead of extracting from AsyncResult, and the debug logging includes the task_id for better traceability.

README.md (3)

15-18: Simplified development instructions.

The development instructions have been correctly updated to remove Celery-specific flags, making the setup process cleaner and more straightforward.

---

32-34: Environment variables updated for Valkey.

The environment variables section correctly replaces Redis with Valkey configuration.

---

57-59: API documentation updated for background tasks.

The API documentation has been correctly updated to reference background tasks instead of Celery tasks.

pyproject.toml (2)

136-139: Linting rules fixed and updated.

The per-file ignores section has been properly fixed with the closing bracket and a new ignore rule for the html_generator.py file.

---

162-172: Well-documented pytest warning filters.

The pytest configuration has been clearly documented with appropriate filters for legitimate warnings that should be suppressed, improving test output clarity.

compose.yaml (3)

8-8: Environment variable updated for Valkey.

The environment variable has been correctly updated from REDIS_URL to VALKEY_URL.

---

20-21: Service dependency updated.

The dependency has been correctly updated from redis to valkey.

---

23-34: Valkey service properly configured.

The Valkey service has been properly configured with the correct image name, ports, healthcheck, and restart policy.

[martinhoyer]

@coderabbitai review

[martinhoyer]

@janhavlin @therazix Sorry guys, just one small fix for things rabbit found: ee8d3fd

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/tmt_web/api.py (2)

412-417: Consider adding a dedicated error field to TaskOut.

Currently, you're using the result field to return errors for failed tasks. While this works, it might be clearer to add a dedicated error field to the TaskOut model for better separation of concerns.

class TaskOut(BaseModel):
    """Response model for asynchronous tasks."""

    id: str
    status: str
    result: str | None = None
+   error: str | None = None
    status_callback_url: str | None = None

Then update the _to_task_out function:

return TaskOut(
    id=task_info["id"],
    status=task_info["status"],
-   result=task_info.get("error")
-   if task_info["status"] == FAILURE
-   else task_info.get("result"),
+   result=task_info.get("result") if task_info["status"] != FAILURE else None,
+   error=task_info.get("error") if task_info["status"] == FAILURE else None,
    status_callback_url=status_callback_url,
)

---

433-439: Consider more robust Valkey health check.

The current health check only pings Valkey but doesn't validate that task operations (set/get) are working correctly. Consider enhancing the health check to verify more aspects of Valkey functionality.

# Check Valkey status
valkey_status = "ok"
try:
    # Ping Valkey
    task_manager.ping()
+   # Verify key operations work
+   test_key = "health_check_test"
+   test_value = f"test_{datetime.now(UTC).isoformat()}"
+   task_manager.client.set(test_key, test_value, ex=60)  # Set with 60s expiry
+   retrieved_value = task_manager.client.get(test_key)
+   if retrieved_value != test_value:
+       raise ValueError(f"Value mismatch: {retrieved_value} != {test_value}")
except Exception as e:
    valkey_status = "failed"
+   logger.error(f"Valkey health check failed: {e}")

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e98c492 and ee8d3fd.

📒 Files selected for processing (3)

• src/tmt_web/api.py (11 hunks)
• src/tmt_web/utils/task_manager.py (1 hunks)
• tests/integration/test_api.py (14 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/tmt_web/utils/task_manager.py

🔇 Additional comments (10)

tests/integration/test_api.py (6)

257-257: Good implementation following previous feedback.

Your patch now targets the public task_manager.ping() method rather than the internal client.ping attribute, making the test more resilient to implementation changes in the task manager.

---

155-182: Excellent test coverage for task failure scenarios.

This test properly validates the error propagation path when a background task fails with a generic error, ensuring the API returns a 500 status code with the appropriate error message.

---

183-218: Comprehensive testing of pending task status.

Good coverage of both JSON and HTML response formats for pending tasks, verifying correct status information and callback URLs are returned.

---

250-264: Thorough health check error testing.

The test properly validates that a Valkey connection failure is reported in the health check while maintaining an overall "ok" status, which is the expected behavior for non-critical dependencies.

---

267-297: Class name appropriately updated.

Renaming from TestCelery to TestBackgroundTasks accurately reflects the architectural change from Celery to FastAPI's background tasks.

---

515-549: Thorough "not found" error test.

This test effectively validates the complete flow of task creation, polling, and error handling for non-existent tests or plans, ensuring that 404 status codes are properly returned.

src/tmt_web/api.py (4)

114-124: Good implementation of error handling.

The error handling now properly differentiates between "not found" errors (404) and other failures (500), which provides better clarity to API consumers.

---

338-353: Consistent error handling across endpoints.

The /status endpoint now raises HTTP exceptions with appropriate status codes for failures, ensuring consistency with the root endpoint behavior. This addresses the previous review comment about divergent error handling between endpoints.

---

387-391: Improved error message display for HTML status.

You've enhanced the HTML status response to include the error message when a task fails, improving user experience by providing clearer feedback.

---

292-315: Well-structured global exception handler.

The new exception handler for GeneralError provides consistent error responses across the API and intelligently maps error messages to appropriate HTTP status codes.