feat: Change HITL default TTL from unlimited to 90 days#1796
Open
vishnuszipstack wants to merge 3 commits intomainfrom
Open
feat: Change HITL default TTL from unlimited to 90 days#1796vishnuszipstack wants to merge 3 commits intomainfrom
vishnuszipstack wants to merge 3 commits intomainfrom
Conversation
Update WorkflowUtil.get_hitl_ttl_seconds() to always return an int TTL value instead of None for unlimited. The ImportError fallback now returns 2160 * 3600 (90 days in seconds) instead of None. This is the open-source counterpart to the full TTL enforcement in unstract-cloud, where the backend models, helpers, serializers, migration, and frontend are also updated. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to Reviews > Disable Cache setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughReworked HITL TTL logic: introduced Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/plugins/workflow_manager/workflow_v2/utils.py (1)
166-173:⚠️ Potential issue | 🟡 MinorGuard against
Nonereturn from the plugin function.The method signature declares
-> int, but line 171 returns the result ofget_hitl_ttl_seconds_by_workflow(workflow)without checking if it returnsNone. If the external plugin function returnsNone, this violates the type contract. Usereturn get_hitl_ttl_seconds_by_workflow(workflow) or 2160 * 3600to ensure anintis always returned.Additionally,
2160 * 3600(90 days) is a magic number—consider extracting it as a named constant for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/plugins/workflow_manager/workflow_v2/utils.py` around lines 166 - 173, The call to get_hitl_ttl_seconds_by_workflow(workflow) may return None but the function is declared to return int; change the return to use a safe fallback (e.g. return get_hitl_ttl_seconds_by_workflow(workflow) or DEFAULT_HITL_TTL_SECONDS) and extract the magic value 2160 * 3600 into a named constant (e.g. DEFAULT_HITL_TTL_SECONDS) at the top of the module so the function always returns an int and the default is clear; update the try/except block that currently returns the plugin value to use this fallback and keep the ImportError branch returning the same constant.
🧹 Nitpick comments (1)
backend/plugins/workflow_manager/workflow_v2/utils.py (1)
173-173: Consider extracting the magic number into a named constant.
2160 * 3600requires mental arithmetic to confirm it equals 90 days. A named constant improves readability and makes the value reusable.♻️ Suggested refactor
+# Default HITL TTL: 90 days in seconds +DEFAULT_HITL_TTL_SECONDS = 90 * 24 * 3600 + class WorkflowUtil:except ImportError: - return 2160 * 3600 + return DEFAULT_HITL_TTL_SECONDSUsing
90 * 24 * 3600also makes the "90 days" intent immediately obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/plugins/workflow_manager/workflow_v2/utils.py` at line 173, Extract the magic number "2160 * 3600" in utils.py into a clearly named module-level constant (e.g., NINETY_DAYS_SECONDS or DEFAULT_RETENTION_SECONDS) defined near the top of the file and replace the literal return expression with that constant; use an explicit expression like 90 * 24 * 3600 for the constant so the "90 days" intent is obvious and reusable across functions that reference the same TTL/retention value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/plugins/workflow_manager/workflow_v2/utils.py`:
- Around line 166-173: The call to get_hitl_ttl_seconds_by_workflow(workflow)
may return None but the function is declared to return int; change the return to
use a safe fallback (e.g. return get_hitl_ttl_seconds_by_workflow(workflow) or
DEFAULT_HITL_TTL_SECONDS) and extract the magic value 2160 * 3600 into a named
constant (e.g. DEFAULT_HITL_TTL_SECONDS) at the top of the module so the
function always returns an int and the default is clear; update the try/except
block that currently returns the plugin value to use this fallback and keep the
ImportError branch returning the same constant.
---
Nitpick comments:
In `@backend/plugins/workflow_manager/workflow_v2/utils.py`:
- Line 173: Extract the magic number "2160 * 3600" in utils.py into a clearly
named module-level constant (e.g., NINETY_DAYS_SECONDS or
DEFAULT_RETENTION_SECONDS) defined near the top of the file and replace the
literal return expression with that constant; use an explicit expression like 90
* 24 * 3600 for the constant so the "90 days" intent is obvious and reusable
across functions that reference the same TTL/retention value.
vishnuszipstack
requested review from
Deepak-Kesavan,
athul-rs,
chandrasekharan-zipstack,
jagadeeswaran-zipstack and
muhammad-ali-e
Deepak-Kesavan
approved these changes
Feb 23, 2026
athul-rs
requested changes
Feb 23, 2026
athul-rs
requested changes
Feb 23, 2026
vishnuszipstack
force-pushed
the
feat/hitl-default-ttl-90-days
branch
from
da3819d to
c689705
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/plugins/workflow_manager/workflow_v2/utils.py (1)
162-180:⚠️ Potential issue | 🟠 MajorPlugin's
get_hitl_ttl_seconds_by_workflowmay returnNone, violating the declaredintreturn type.The return type annotation is
-> int(line 157), but line 171 passes the plugin's return value through unguarded. Although the plugin code is not accessible in this repository, the fallback helper implementations inworkers/shared/utils/manual_review_factory.py(lines 150, 348, 456) consistently returnNonefor OSS/fallback cases, suggesting the cloud plugin'sget_hitl_ttl_seconds_by_workflowmay do the same for workflows without explicit TTL configuration. This would cause a type violation since callers rely on the declared-> intcontract.While the queue operations accept
ttl_seconds: int | None, the function should guard againstNonefrom the plugin to maintain type safety:🛡️ Proposed guard against None from plugin
try: from pluggable_apps.manual_review_v2.helper import ( get_hitl_ttl_seconds_by_workflow, ) - return get_hitl_ttl_seconds_by_workflow(workflow) + ttl = get_hitl_ttl_seconds_by_workflow(workflow) + if ttl is None: + return 2160 * 3600 + return ttl except ImportError: return 2160 * 3600🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/plugins/workflow_manager/workflow_v2/utils.py` around lines 162 - 180, The get_hitl_ttl_seconds function currently forwards the plugin result from get_hitl_ttl_seconds_by_workflow unguarded, which may be None and violates the declared int return; update get_hitl_ttl_seconds to capture the plugin return into a local (e.g., ttl = get_hitl_ttl_seconds_by_workflow(workflow)), check if ttl is None or not an int, and if so return DEFAULT_HITL_TTL_HOURS * 3600; preserve the existing ImportError fallback behavior but ensure the function always returns an int.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/plugins/workflow_manager/workflow_v2/utils.py`:
- Around line 18-20: DEFAULT_HITL_TTL_HOURS is computed at import time with
int(os.environ.get(...)) which will raise ValueError on non-numeric input and
accepts non-positive numbers; change this to read the raw env var (e.g., env_val
= os.environ.get("HITL_DEFAULT_TTL_HOURS")), attempt to convert it inside a
try/except (catch ValueError/TypeError), and fall back to the default 2160 on
failure; after conversion validate that the integer is > 0 and if not use the
default (optionally log/warn), ensuring the module import never raises and
DEFAULT_HITL_TTL_HOURS is always a positive int.
---
Outside diff comments:
In `@backend/plugins/workflow_manager/workflow_v2/utils.py`:
- Around line 162-180: The get_hitl_ttl_seconds function currently forwards the
plugin result from get_hitl_ttl_seconds_by_workflow unguarded, which may be None
and violates the declared int return; update get_hitl_ttl_seconds to capture the
plugin return into a local (e.g., ttl =
get_hitl_ttl_seconds_by_workflow(workflow)), check if ttl is None or not an int,
and if so return DEFAULT_HITL_TTL_HOURS * 3600; preserve the existing
ImportError fallback behavior but ensure the function always returns an int.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
backend/plugins/workflow_manager/workflow_v2/utils.py
chandrasekharan-zipstack
approved these changes
Feb 24, 2026
Contributor
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
I'd suggest adding it as a constant instead of leaving it this way. The constant could just be a local variable if you think it won't be used elsewhere.
Or it could be something like 90 * SECS_IN_DAY
Replace raw `2160 * 3600` with `DEFAULT_HITL_TTL_SECONDS = 90 * SECS_IN_DAY` for readability. Also guard against None return from plugin function. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chandrasekharan-zipstack
approved these changes
Feb 24, 2026
athul-rs
approved these changes
Feb 24, 2026
|
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
WorkflowUtil.get_hitl_ttl_seconds()to always return a valid TTL (default 90 days) instead ofNonefor unlimitedunstract-cloudWhy
How
WorkflowUtil.get_hitl_ttl_seconds()fromint | None→intImportErrorfallback (whenmanual_review_v2plugin is not installed) now returns2160 * 3600(90 days in seconds) instead ofNonebackend/plugins/workflow_manager/workflow_v2/utils.pyint | None→int;ImportErrorfallback returns2160 * 3600(90 days) instead ofNoneCan this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
None(unlimited TTL). Now it always returns an integer. All callers already handle an integer TTL, so existing behavior is preserved — records just get a default expiry instead of living forever.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
WorkflowUtil.get_hitl_ttl_seconds(workflow)returns7776000(90 days in seconds) whenmanual_review_v2plugin is not installedScreenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code