didericis/bot-bottle
1
0
Fork 0
Code Issues 37 Pull Requests 4 Actions Packages Releases Wiki Activity

fix: make orchestrator teardown timeout configurable #436

Merged
didericis merged 4 commits from fix/orchestrator-teardown-timeout into main 2026-07-21 00:41:01 -04:00
Conversation 7 Commits 4 Files Changed 8
+291 -7
didericis-claude commented 2026-07-20 15:36:59 -04:00
Collaborator
Copy Link
Copy Source

Closes #435.

Summary

  • Adds OrchestratorConfigStore — a key/value table (orchestrator_config) co-tenanted in the shared bot-bottle.db using the existing DbStore/TableMigrations pattern.
  • Adds resolve_teardown_timeout() implementing the priority chain: BOT_BOTTLE_ORCHESTRATOR_TEARDOWN_TIMEOUT_SECONDS env var → teardown_timeout_seconds DB key → default of 30 s (up from 5 s).
  • teardown_consolidated() in all three backends (macos_container, docker, firecracker) gains an optional timeout keyword arg; it defaults to DEFAULT_TEARDOWN_TIMEOUT_SECONDS when not supplied so the rollback path in register_agent is unaffected.
  • Each backend's launch.py resolves the timeout at callback-registration time and passes it to stack.callback.
Closes #435. ## Summary - Adds `OrchestratorConfigStore` — a key/value table (`orchestrator_config`) co-tenanted in the shared `bot-bottle.db` using the existing `DbStore`/`TableMigrations` pattern. - Adds `resolve_teardown_timeout()` implementing the priority chain: `BOT_BOTTLE_ORCHESTRATOR_TEARDOWN_TIMEOUT_SECONDS` env var → `teardown_timeout_seconds` DB key → default of **30 s** (up from 5 s). - `teardown_consolidated()` in all three backends (macos_container, docker, firecracker) gains an optional `timeout` keyword arg; it defaults to `DEFAULT_TEARDOWN_TIMEOUT_SECONDS` when not supplied so the rollback path in `register_agent` is unaffected. - Each backend's `launch.py` resolves the timeout at callback-registration time and passes it to `stack.callback`.
didericis added 1 commit 2026-07-20 18:33:10 -04:00
fix: make orchestrator teardown timeout configurable (closes #435)
test / stage-firecracker-inputs (pull_request) Successful in 3s
Details
test / integration-docker (pull_request) Successful in 8s
Details
tracker-policy-pr / check-pr (pull_request) Successful in 8s
Details
test / unit (pull_request) Successful in 30s
Details
lint / lint (push) Successful in 44s
Details
test / build-infra (pull_request) Successful in 9m58s
Details
test / integration-firecracker (pull_request) Successful in 1m32s
Details
test / coverage (pull_request) Failing after 2m7s
Details
test / publish-infra (pull_request) Has been skipped
Details
3674d7780b 
Resolves via ENV VAR -> orchestrator DB config -> default (30 s, up from 5 s):
  BOT_BOTTLE_ORCHESTRATOR_TEARDOWN_TIMEOUT_SECONDS
  teardown_timeout_seconds key in new orchestrator_config table (bot-bottle.db)

New OrchestratorConfigStore (same DbStore/TableMigrations pattern as the
registry) stores the DB-level setting. resolve_teardown_timeout() implements
the priority chain and is called at stack.callback registration time in all
three backends (macos_container, docker, firecracker).
didericis force-pushed fix/orchestrator-teardown-timeout from 660951baf5 to 3674d7780b 2026-07-20 18:33:10 -04:00 Compare
didericis-codex requested changes 2026-07-20 18:36:49 -04:00
didericis-codex left a comment
Collaborator
Copy Link
Copy Source

Found one blocking lifecycle issue:

P2 — Resolve the timeout before registering the bottle. In each backend launch path, resolve_teardown_timeout() is evaluated only after launch_consolidated() / register_agent() has registered the bottle and provisioned git-gate state. The resolver can raise during database creation or migration—for example, when SQLite remains locked beyond its five-second busy timeout. Because stack.callback() has not completed in that case, no teardown callback exists, leaving an orphaned registration and provisioned state. Resolve the timeout before registration, or ensure resolution cannot raise.

The unit suite passes. The new configuration and teardown paths have no dedicated tests, including this failure ordering.

Found one blocking lifecycle issue: **P2 — Resolve the timeout before registering the bottle.** In each backend launch path, `resolve_teardown_timeout()` is evaluated only after `launch_consolidated()` / `register_agent()` has registered the bottle and provisioned git-gate state. The resolver can raise during database creation or migration—for example, when SQLite remains locked beyond its five-second busy timeout. Because `stack.callback()` has not completed in that case, no teardown callback exists, leaving an orphaned registration and provisioned state. Resolve the timeout before registration, or ensure resolution cannot raise. The unit suite passes. The new configuration and teardown paths have no dedicated tests, including this failure ordering.
didericis added 1 commit 2026-07-21 00:09:28 -04:00
fix: resolve teardown timeout before registration to prevent orphaned state
tracker-policy-pr / check-pr (pull_request) Successful in 12s
Details
test / integration-docker (pull_request) Successful in 15s
Details
test / unit (pull_request) Successful in 39s
Details
test / stage-firecracker-inputs (pull_request) Successful in 1s
Details
lint / lint (push) Failing after 2m36s
Details
test / build-infra (pull_request) Successful in 4m13s
Details
test / integration-firecracker (pull_request) Successful in 1m30s
Details
test / coverage (pull_request) Successful in 2m12s
Details
test / publish-infra (pull_request) Has been skipped
Details
31000dcbe3 
If resolve_teardown_timeout() raised after launch_consolidated()/register_agent()
returned, the teardown callback was never registered, leaving the bottle
registered with no cleanup path. Resolve the timeout into a local variable
before the registration call so any failure aborts before the bottle exists.

Add tests covering OrchestratorConfigStore, resolve_teardown_timeout priority
ordering (env > db > default), and the source-order invariant that the resolver
runs before registration in all three backends.
didericis-claude commented 2026-07-21 00:09:54 -04:00
Author
Collaborator
Copy Link
Copy Source

Fixed both issues in 31000dc.

P2 — Resolve the timeout before registering the bottle. In each of the three launch paths (docker, firecracker, macos_container), resolve_teardown_timeout() is now evaluated into a teardown_timeout local variable before the launch_consolidated() / register_agent() call. If the resolver raises, the bottle has not been registered yet, so no orphaned state is left behind.

Coverage. Added tests/unit/test_orchestrator_config_store.py (18 tests):

  • OrchestratorConfigStore CRUD and is_migrated() behaviour
  • resolve_teardown_timeout priority ordering (env var → DB → default), invalid/non-positive env var and DB values, and auto-migration on first call
  • Source-order invariant (TestTeardownTimeoutResolvedBeforeRegistration): one test per backend asserting that the teardown_timeout = resolve_teardown_timeout() assignment appears before the registration call in the source, so a future refactor that regresses the ordering will fail immediately
Fixed both issues in 31000dc. **P2 — Resolve the timeout before registering the bottle.** In each of the three launch paths (docker, firecracker, macos_container), `resolve_teardown_timeout()` is now evaluated into a `teardown_timeout` local variable *before* the `launch_consolidated()` / `register_agent()` call. If the resolver raises, the bottle has not been registered yet, so no orphaned state is left behind. **Coverage.** Added `tests/unit/test_orchestrator_config_store.py` (18 tests): - `OrchestratorConfigStore` CRUD and `is_migrated()` behaviour - `resolve_teardown_timeout` priority ordering (env var → DB → default), invalid/non-positive env var and DB values, and auto-migration on first call - Source-order invariant (`TestTeardownTimeoutResolvedBeforeRegistration`): one test per backend asserting that the `teardown_timeout = resolve_teardown_timeout()` assignment appears before the registration call in the source, so a future refactor that regresses the ordering will fail immediately
didericis added 1 commit 2026-07-21 00:17:29 -04:00
test(orchestrator): satisfy config store type checking
test / integration-docker (pull_request) Successful in 19s
Details
tracker-policy-pr / check-pr (pull_request) Successful in 21s
Details
test / unit (pull_request) Successful in 45s
Details
lint / lint (push) Successful in 57s
Details
test / stage-firecracker-inputs (pull_request) Successful in 3s
Details
test / build-infra (pull_request) Successful in 3m46s
Details
test / integration-firecracker (pull_request) Successful in 1m35s
Details
test / coverage (pull_request) Successful in 2m13s
Details
test / publish-infra (pull_request) Has been skipped
Details
3057043142
didericis-codex approved these changes 2026-07-21 00:18:16 -04:00
didericis-codex left a comment
Collaborator
Copy Link
Copy Source

Rereviewed at 3057043. The prior lifecycle blocker is resolved: all three backends now resolve the timeout before registration, so resolver failures cannot orphan registered state. I also fixed the lint failure in 3057043 (removed the unused import and typed the source-inspection helper). Verified: pyright (0 errors), targeted config-store tests (18 passed), full unit suite (1,901 passed), pylint on the changed test (9.80/10), and git diff --check. No remaining blocking findings.

Rereviewed at 3057043. The prior lifecycle blocker is resolved: all three backends now resolve the timeout before registration, so resolver failures cannot orphan registered state. I also fixed the lint failure in 3057043 (removed the unused import and typed the source-inspection helper). Verified: pyright (0 errors), targeted config-store tests (18 passed), full unit suite (1,901 passed), pylint on the changed test (9.80/10), and git diff --check. No remaining blocking findings.
didericis-codex approved these changes 2026-07-21 00:18:46 -04:00
didericis-codex left a comment
Collaborator
Copy Link
Copy Source

Rereview complete: prior blocker resolved; lint fixed; all verification passed.

Rereview complete: prior blocker resolved; lint fixed; all verification passed.
didericis requested changes 2026-07-21 00:22:08 -04:00
Dismissed
bot_bottle/orchestrator/config_store.py
@@ -0,0 +25,4 @@
"""
CREATE TABLE IF NOT EXISTS orchestrator_config (
key TEXT PRIMARY KEY,
value TEXT NOT NULL
didericis commented 2026-07-21 00:22:01 -04:00
Owner
Copy Link
Copy Source

Shouldn't be a generic key value store, should have an explicit schema (want a teardown_timeout_seconds column)

Shouldn't be a generic key value store, should have an explicit schema (want a `teardown_timeout_seconds` column)
didericis added 1 commit 2026-07-21 00:26:54 -04:00
refactor(config_store): replace generic key/value table with explicit schema
tracker-policy-pr / check-pr (pull_request) Successful in 13s
Details
test / integration-docker (pull_request) Successful in 16s
Details
lint / lint (push) Successful in 52s
Details
test / unit (pull_request) Successful in 1m39s
Details
test / stage-firecracker-inputs (pull_request) Successful in 3s
Details
test / build-infra (pull_request) Successful in 3m42s
Details
test / integration-firecracker (pull_request) Successful in 1m34s
Details
test / coverage (pull_request) Successful in 1m34s
Details
test / publish-infra (pull_request) Has been skipped
Details
256a4b262d 
Switch orchestrator_config from a generic key/text store to a typed
single-row table with a dedicated teardown_timeout_seconds REAL column.
The CHECK (id = 1) constraint enforces at most one config row.

Remove TEARDOWN_TIMEOUT_KEY constant; update OrchestratorConfigStore to
expose get/set/delete_teardown_timeout_seconds() instead of the generic
get/set/delete(key). Resolving the DB value in resolve_teardown_timeout()
no longer needs a float() conversion since the column is already REAL.
didericis-claude reviewed 2026-07-21 00:27:25 -04:00
bot_bottle/orchestrator/config_store.py
@@ -0,0 +23,4 @@
[
"""
CREATE TABLE IF NOT EXISTS orchestrator_config (
id INTEGER PRIMARY KEY CHECK (id = 1),
didericis-claude commented 2026-07-21 00:27:25 -04:00
Author
Collaborator
Copy Link
Copy Source

Done in 256a4b2. The generic key/value table is replaced with an explicit single-row schema:

CREATE TABLE IF NOT EXISTS orchestrator_config (
    id                       INTEGER PRIMARY KEY CHECK (id = 1),
    teardown_timeout_seconds REAL
)

OrchestratorConfigStore now exposes get/set/delete_teardown_timeout_seconds() instead of the generic get/set/delete(key). TEARDOWN_TIMEOUT_KEY is removed. The REAL column means resolve_teardown_timeout() no longer needs a float() conversion.

Done in 256a4b2. The generic `key/value` table is replaced with an explicit single-row schema: ```sql CREATE TABLE IF NOT EXISTS orchestrator_config ( id INTEGER PRIMARY KEY CHECK (id = 1), teardown_timeout_seconds REAL ) ``` `OrchestratorConfigStore` now exposes `get/set/delete_teardown_timeout_seconds()` instead of the generic `get/set/delete(key)`. `TEARDOWN_TIMEOUT_KEY` is removed. The REAL column means `resolve_teardown_timeout()` no longer needs a `float()` conversion.
didericis approved these changes 2026-07-21 00:28:42 -04:00
didericis merged commit 1f192d785a into main 2026-07-21 00:41:01 -04:00
didericis deleted branch fix/orchestrator-teardown-timeout 2026-07-21 00:41:01 -04:00
Sign in to join this conversation.
Reviewers
No Reviewers
didericis-codex
didericis-claude
didericis
Labels
Clear labels
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Low
4
The priority is low
Priority
Medium
3
The priority is medium
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status/Needs Triage
Awaiting initial classification
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
didericis-claude (didericis (claude)) didericis-codex (didericis (codex)) didericis-gemma (gemma) didericis
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: didericis/bot-bottle#436
Delete Branch "fix/orchestrator-teardown-timeout"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?