fix: runner alloc idx logic, api auth for actor get#4443
Open
MasterPtato wants to merge 1 commit intomainfrom
Open
fix: runner alloc idx logic, api auth for actor get#4443MasterPtato wants to merge 1 commit intomainfrom
MasterPtato wants to merge 1 commit intomainfrom
Conversation
|
🚅 Deployed to the rivet-pr-4443 environment in rivet-frontend
|
Contributor
Author
11 tasks
PR Review\n\nThis PR fixes two distinct issues: runner alloc idx logic during draining and API auth for actor list by ID.\n\n---\n\n### Auth change in list.rs\n\nThe new logic skips auth when actor_ids, actor_id, or key are provided. A few edge cases worth considering:\n\n- actor_ids is Option, so a caller sending actor_ids= (present but empty string) causes actor_ids.is_none() to be false, bypassing auth. The subsequent empty-string parse will fail with a 400 so there is no data leak, but this is a latent inconsistency worth closing.\n- key= (present but empty string) also skips auth and proceeds with an empty-key lookup. Worth verifying that downstream key resolution handles an empty string safely.\n- A short inline comment explaining why unauthenticated read-by-ID is safe would help future readers understand the intent.\n\n---\n\n### Runner eviction and alloc idx fix\n\nThe refactor from returning Err(WsError::Eviction) in tasks to Ok(Err(LifecycleResult::Evicted)) is cleaner and correct. The previous path short-circuited the ClearIdx operation in lib.rs because the error was indistinguishable from a normal connection failure. The new path correctly skips ClearIdx only on eviction, preserving the alloc idx entry for draining runners.\n\nThe cascading fix in update_alloc_idx.rs checking DrainTsKey instead of the presence of the old alloc key is the right abstraction. Checking drain state explicitly is more reliable and self-documenting than inferring intent from key existence.\n\n---\n\n### Removing ExpiredTsKey write on drain\n\nDraining and expiry are now correctly decoupled. Draining inhibits new allocation without marking the runner as fully unavailable. This is a meaningful semantic fix.\n\nRemoving the critical: prefix from the error log is appropriate now that the failure path is non-critical.\n\n---\n\n### Minor notes\n\n- Comment placement in lib.rs: the make-runner-immediately-ineligible comment sits between the closing brace of the if block and the else block. Moving it inside the else block would improve readability.\n- notifs logging in mark_eligible: good defensive logging. Consider whether non-empty notifications here represent a bug that should propagate as an error rather than just a warning, since mark_eligible is called during runner startup.\n- db_path display in rocksdb/database.rs: correct fix. PathBuf with debug format emits extra quotes and OS-specific escaping; Display is cleaner. |
MasterPtato
force-pushed
the
03-17-fix_runner_alloc_idx_logic_api_auth_for_actor_get
branch
from
5b2bead to
cfc4fad
Compare
11 tasks
MasterPtato
force-pushed
the
03-17-fix_runner_alloc_idx_logic_api_auth_for_actor_get
branch
from
cfc4fad to
65280b5
Compare
This was referenced Mar 18, 2026
MasterPtato
force-pushed
the
03-17-fix_runner_alloc_idx_logic_api_auth_for_actor_get
branch
from
65280b5 to
84dbf32
Compare
MasterPtato
force-pushed
the
03-17-fix_runner_alloc_idx_logic_api_auth_for_actor_get
branch
from
84dbf32 to
90c2e97
Compare
11 tasks
MasterPtato
force-pushed
the
03-17-fix_runner_alloc_idx_logic_api_auth_for_actor_get
branch
from
90c2e97 to
ebdaa13
Compare
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
1 participant
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.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: