fix(blaze)!: reject incomplete reset and pool use - #2290
Conversation
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
7068096 to
4a544aa
Compare
|
@codex review This PR rejects incomplete sandbox reset before lifecycle, ownership, pool, persistence, or metric mutation and keeps warm-pool setup test-only until complete reset exists. Please review the complete pull request: every commit, the cumulative diff against the base branch, and the implementation, tests, and documentation as one submitted change. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
本次变更总体实现了 reset 未实现时的 501 返回和状态不变、更安全的行为。上面几点建议主要是关于 reset 状态约束语义和测试辅助函数命名清晰度，可酌情考虑。
🤖 Generated by Qoder • View workflow run
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a544aa2ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4a544aa to
be904b1
Compare
|
@codex review This PR rejects incomplete sandbox resets before any state or ownership mutation while preserving existing identifier and lifecycle error semantics. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be904b1267
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f139625 to
27debfa
Compare
|
@codex review This PR makes the public reset endpoint reject incomplete resets without side effects, preserves existing identifier and lifecycle errors, and documents the exact protocol boundary. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
27debfa to
768b606
Compare
|
@codex review This PR makes the public reset endpoint reject incomplete resets without side effects, preserves existing identifier and lifecycle errors, documents the exact HTTP contract, and keeps internal error types crate-private. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Follow-up non-checkpoint API namespace work is tracked in #2577. This pull request remains limited to correcting the false-success reset behavior on the existing route. #2577 will make the current non-checkpoint The broader goal remains related to #2535, but checkpoint capture and its canonical route stay with #2472. No checkpoint route, namespace migration, compatibility-route removal, or deprecation date is included in #2290. |
768b606 to
90746b4
Compare
WeissonHan
changed the title
|
@codex review |
90746b4 to
f6e19ae
Compare
|
@codex review |
|
@codex review This pull request rejects incomplete reset and inactive reusable-instance operations, preserves legacy state-file compatibility, and documents the public migration contract. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e19ae0ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f6e19ae to
f6f8723
Compare
|
@codex review Reject incomplete reset and inactive reusable-instance behavior while preserving exact packaged RPM defaults for upgrade compatibility. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6f872396d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@casparant, the exact-head review found a documentation-placement conflict that now blocks #2290. Section 5 of Please confirm which boundary should govern. If section 5 must take precedence, the agreed submission plan must be revised before paired design documents can be added and the exact Linux and hosted gates repeated; I will not expand the pull request silently. If the four user-facing documents are sufficient for this corrective compatibility change, the current validated head can remain unchanged. |
Reset changed only lifecycle metadata before returning the same runtime to an instance pool. It did not reset runtime or storage, so callers could receive a false success and later reuse state they expected to discard. Return 501 for running reset requests while preserving 400, 404, and 422 for malformed, missing, and invalid-state requests. Remove the inactive pool implementation because reset was its only source of reusable instances. Keep the four pool routes as 501 compatibility endpoints. Reject unsupported pool settings, but accept and ignore only the exact defaults shipped in older RPM configuration and policy files. This narrow exception prevents an administrator-modified %config(noreplace) file from blocking the upgraded binary before the operator can merge its .rpmnew replacement. Retain decoding for legacy Reset and Warm states, including persisted start_path = warm records, so startup cleanup can release their resources. Remove the three metrics that no longer describe a supported operation. This is a breaking correction for clients using the incomplete pool API, unsupported pool configuration, or removed metrics. Complete reusable-instance support remains out of scope and must arrive with sandbox creation that uses it. Fixes: 1f0cfac ("feat(anvil): scaffold local orchestrator crate skeleton") Signed-off-by: Weisson Han <wenshu.hx@linux.alibaba.com>
f6f8723 to
adbe1c1
Compare
|
@codex review This PR rejects incomplete reset and reserved pool-management operations before lifecycle or resource ownership changes, retains only exact package-default upgrade compatibility, and documents the cold-only and legacy-recovery protocol in the paired Blaze lifecycle design. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@casparant The documentation-standard issue is now addressed in |
|
@casparant All eight review threads are now resolved. The exact head remains |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Why
POST /v1/instances/{id}/resetpreviously returned success after changinglifecycle metadata and placing the same running sandbox into a reusable pool.
It did not reset either the runtime or storage, so a later create request could
receive process or data state that the caller expected reset to discard.
Once incomplete reset is rejected, no supported path can safely produce a
reusable sandbox. Keeping the inactive pool implementation, settings, metrics,
and apparently successful management surface would continue to advertise a
capability that cannot meet its isolation contract.
What changed
runtime ownership, or storage ownership.
always use the cold start path.
GET /v1/poolsGET /v1/pools/{backend}/{class}POST /v1/pools/{backend}/{class}/drainPUT /v1/pools/{backend}/{class}/sizingblaze_instances_resets_total,blaze_pool_hits_total, andblaze_pool_misses_total.path for files retained by
%config(noreplace).The only accepted daemon
[pool]section is the exact historical packagedefault:
The only accepted policy
[pool]sections must contain the exact sixhistorical fields and match one of these packaged policy identities:
mintargetmaxagent-rl-defaultagent-rlagent-tool-defaultagent-toolBoth also require
enabled = true,warm_ttl = "30m", andreset_mode = "full-recreate".These accepted sections are ignored, omitted from serialization, and produce a
warning. They do not enable reusable instances. A missing or additional field,
a changed value or type, a different policy name or workload class, any other
daemon or policy
[pool]section, and every occurrence ofstorage.pool_sizeorstorage.preforkremain validation errors.At initial startup, a policy-loading failure obeys
policy.on_load_error:"fail"stops startup and"warn"starts with anempty policy engine. Administrative and signal-driven reloads construct and
validate the replacement engine before publishing it; a failed reload leaves
the currently active policies unchanged.
Blaze continues to decode persisted
Reset,Warm, andstart_path = "warm"values written by older releases. These values areretained only for persisted-state cleanup: new runtime transitions cannot
enter
ResetorWarm, and new instances always record a cold start. Startupreconciliation treats non-terminal legacy records as cleanup candidates and
never reuses them. Failed cleanup leaves the affected record in
RecoveryRequiredwhile reconciliation continues with other records.Related issue
closes #2288
User impact
Clients requiring a fresh sandbox must stop relying on reset. Until complete
runtime-and-storage reset exists, they should successfully destroy the old
sandbox and create a new one.
RPM upgrades may continue to start when an administrator-modified
%config(noreplace)file still contains one of the exact packaged defaultslisted above. Operators should merge the corresponding
.rpmnewfiles orremove the legacy sections. Any actual pool tuning or other unsupported pool
setting is still rejected.
Monitoring rules and dashboards must stop querying the three removed metrics.
Direct
blaze-coreusers must stop importing the removed pool interfaces.Storage providers no longer implement pool draining, and callers can no longer
select a warm start for a newly constructed
SandboxInstance.Existing persisted state requires no conversion. Legacy reset and warm records
remain readable solely so startup reconciliation can release their owned
resources.
Risk and compatibility
This is an intentional breaking correction for reset clients, pool-management
clients, unsupported pool configuration, removed metrics, and direct users of
the removed Rust interfaces. The narrow package-default exception prevents an
unchanged historical RPM section from causing an otherwise compatible upgrade
failure; it does not preserve pool behavior.
Validation
Frozen source candidate:
adbe1c1220b426e3340877c0599f31d9c2f4237cf7b0485b49f87fdaa7dc3f3251a58a80bd9f106a7d75cd5029ea9800e23d8b088c3caeaa1e798788Current-main GitHub merge candidate:
main:ba20d94d19a73e5b781cab0c687d4034ad0d0dae3b5b4ec3aca098a7f0294f74a36282b51e3db1afba20d94d19a73e5b781cab0c687d4034ad0d0daeandadbe1c1220b426e3340877c0599f31d9c2f4237c20230d68b745dd80373a535048b155482e75a84eBoth exact trees were independently reconstructed from the public GitHub
source on native Linux x86_64 and matched the expected commit, parent, and
tree identities. Rust and Cargo 1.88.0 were used with locked dependencies,
offline Cargo execution, a fresh task-owned Cargo home, and an empty target
directory for every stage. Both trees passed:
cargo fmt --all -- --check;relative-link checks; and
metrics regressions per tree, each required to execute exactly one test.
The final run completed 1,234 full-suite test executions and 34 focused test
executions with no failures. The exact source and merge patches have the same
stable patch identity, and cleanup removed all task source, dependency cache,
target, and harness directories after archiving the logs.
All 26 hosted check runs for the published revision reached a terminal state
with no failures: Blaze, documentation, website, pull-request, and commit
message checks succeeded, while unrelated component and deployment jobs were
skipped by their change filters. The current GitHub check-run surfaces did
not expose a separate contributor-license status item.
Codex reviewed exact commit
adbe1c1220after publication and reported nomajor issues. That review introduced no new inline thread or P1/P2 finding.
The final Linux evidence archive is
/tmp/pr2290-adbe-3b5b-exact-linux-gate-final-logs.tar.gz, with SHA-256cb156a818b4080aa26abace6cd7d90f87292af2162fee7bf3308785dc68abb0b.Documentation and rollback
The client, operator, and lifecycle compatibility contract is documented in
six existing paired documents:
src/blaze/README.mdsrc/blaze/README_zh.mddocs/user-guide/en/runtime/blaze.mddocs/user-guide/zh/runtime/blaze.mdsrc/blaze/docs/design/lifecycle-state-consistency.mdsrc/blaze/docs/design/lifecycle-state-consistency_zh.mdThe lifecycle design pair now formally owns two related boundaries: complete
startup inventory publication, and serving-time rejection of reset or
reusable-instance operations that cannot preserve lifecycle, runtime, and
storage ownership. It defines the reset response matrix, the four reserved
pool-management responses, cold-only creation, compatibility-only decoding of
legacy
Reset,Warm, andstart_path = "warm"records, cleanup outcomes,the retained health-response shape, and the removed metrics.
The four reader-facing documents give the exact accepted upgrade values. The
daemon section must contain only
default_warm_ttl = "30m"andgc_interval = "5m". The policy exceptions are limited toagent-rl-defaultwith workload classagent-rland sizes4/16/64, oragent-tool-defaultwith workload classagent-tooland sizes2/8/32;both require
enabled = true,warm_ttl = "30m", andreset_mode = "full-recreate". They also explain.rpmnewmigration,startup and reload failure behavior, and the recovery failure boundary: failed
legacy cleanup retains the in-memory record as
RecoveryRequiredand attemptsto persist that state; if persistence also fails, the warning reports the
additional error, the durable record may retain its previous state, and
reconciliation continues with other accepted records.
Migration requires:
.rpmnewfiles or removing legacy pool sections;blaze-coreusers to stop using removed pool interfaces.Reverting this commit requires no persisted-state conversion. It also restores
the false-success reset behavior and inactive pool surface, so updating clients
and configuration is safer than relying on rollback to recover those
operations.