RPC in-flight recovery across a tb-core rollout - #35
Open
ShvaykaD wants to merge 32 commits into
Open
Conversation
…sted id on actor init
…ingRpc Terminal one-way DELIVERED/SENT rows past expiry were being force-updated to EXPIRED on every actor init(), corrupting the persisted status of an already-successful one-way RPC. Move the one-way-terminal check ahead of the expiry check so those rows are left untouched; two-way rows still re-expire as before. Also drops the unused fixed-id createToDeviceRpcRequestMsg overload, and adds a guard test asserting TbRpcService.update is never called for a past-due one-way DELIVERED row.
…tent double-exec)
restorePendingRpc's oneway guard skipped SENT (QoS-1, publish->PUBACK window) alongside DELIVERED, permanently stranding a one-way SENT row on a core move. Only one-way DELIVERED is terminal per spec; narrow the guard so SENT falls through to reload/re-arm/re-publish. Also add the missing @Schema annotation on Rpc.requestId (REST- serialized entity) and cover both the one-way-SENT re-publish path and the legacy null-requestId reload path with unit tests.
If a persisted rpc row's request JSON fails to deserialize (null or corrupt), JacksonUtil.convertValue returns null; without this guard the next msg.isOneway() call NPEs and aborts the actor's whole init() reload. Log and skip that unrecoverable row so the remaining RPCs still restore.
A reloaded row whose request JSON deserializes to null is skipped and logged; the reload loop continues, so a following valid row still restores and re-publishes. Guards the null-safe branch added alongside.
…wline Drop the fully-qualified org.assertj.core.api.Assertions.assertThat in the reload tests for a static import (arity keeps it distinct from the file's pre-existing hamcrest assertThat). Add the missing EOF newline.
… reload, unify register, hide requestId - Seed rpcSeq past the highest persisted id in init() before restoring, so a legacy (null request_id) row's fallback id cannot collide with a persisted id reloaded in the same batch; drop the redundant per-row seeding and warn on any pending-map overwrite. - Break same-createdTime ties by requestId (nullsLast) for deterministic reload / sequential re-publish order. - Reload in-flight rows with a single findAllByDeviceIdAndStatusIn query via PageDataIterable instead of three per-status paging loops. - Unify registerRestoredRpc into registerPendingRpcRequest (one register path, put + timeout arming together). - Extract the undelivered(md) predicate; document why V4_3_1_4Migration exists. - Mark Rpc.requestId @JsonIgnore — it is internal actor-recovery plumbing, not part of the public REST/rule-engine contract (DB round-trip via JPA is unaffected). - Tests: single-query stubbing + shared helpers; add reload-EXPIRED coverage, a legacy-null/reused-id collision regression, and a DAO status-IN query test.
…ngsboard into rpc-tests-inflight-recovery
…ration backfills) Adds LtsMigration.applyAfterCommit() and reworks LtsMigrationService to a two-phase apply (schema+apply atomically, then afterSchemaPhase, then per- migration applyAfterCommit + version record), matching PE's lts-4.3 so a future CE->PE merge is conflict-free. SystemPatchApplier uses the 3-arg applyMigrations with CE's own updateSqlViews() in the afterSchemaPhase Runnable. Test callers updated for the new signature.
…orded-on-applyAfterCommit-failure
… + partial index)
… backlog (4.3.1.4)
- RpcRepository.findInFlightForReload: extract the in-flight predicate to a single constant, reused by value + countQuery. - Extract the legacy-close message text to one shared constant on DeviceActorMessageProcessor; the migration references it. - Drop the now-unused TbRpcService.findAllByDeviceIdAndStatus (dead after the reload rework). - Test: default MAX_CONCURRENT_SESSIONS_PER_DEVICE to 1 (the production default; >1 is the rare case) and use a non-system tenant id.
…ated dao SQL test
…on-logic + cleanup TODO
… fixed ids; rename saveRpc
…ify/never, warning-free stubs
…and-new registerPendingRpcRequest
…s switch exhaustive
getAllowedFromStatuses(boolean oneway): TIMEOUT is a SENT-peer in-flight state; terminal trio excludes DELIVERED for one-way. Guarded UPDATE drops the SQL COALESCE(oneway) clause and passes the write's oneway from Java. Reload predicate + idx_rpc_in_flight (both schema files) include TIMEOUT. Actor sources oneway from msg.isOneway() at reload sites.
The out-of-transaction backlog cleanup now also force-closes past-expiry legacy (request_id IS NULL) rows stuck in TIMEOUT, alongside SENT and two-way DELIVERED.
A past-expiry TIMEOUT row is force-closed to EXPIRED on reload; a not-yet-expired TIMEOUT row is re-tracked (no terminal write).
The allowed-from set is a pure function of (target status, oneway), so build the String[] arrays once at class load into EnumMaps and look them up per row, instead of rebuilding an EnumSet + stream + array for every row on the batch hot path. Behavior-preserving (JpaRpcDaoTest unchanged, 14/14).
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.
Problem
During a tb-core rolling restart / device-actor migration, in-flight RPCs were lost: persistent RPCs got stuck in
DELIVEREDforever and were never re-published or resolved (~47% loss observed in load testing). Root cause: the per-actorrpcSeqcounter resets to0when the actor is recreated, and the actor did not reload the in-flight (SENT/DELIVERED) rows it had persisted, so responses arriving after the move could not be matched back to a request.Goal
Persistent RPCs = zero-loss and no mis-match across a core rollout. Non-persistent RPCs remain best-effort (unchanged).
Approach
Persist the transport
requestIdon therpcrow and rebuild actor state from it oninit():request_idandonewaycolumns onrpc, plus a partial indexidx_rpc_in_flight, applied via the LTS4.3.1.4patch (auto-applies on core startup throughSystemPatchApplier; nullable so pre-migration rows load fine).requestIdand theonewayflag are written to the row at RPC creation.init(), in-flight rows are reloaded under their persisted id, sorted bycreatedTime(thenrequestId) to preserve submission order, and their timeouts re-armed. The reload query + partial index deliberately exclude one-wayDELIVERED(a terminal success that would otherwise accumulate and be loaded for nothing).rpcSeqis seeded once past the highest reloaded id, so a new RPC never collides with a reloaded one; with nothing in flight it still starts from0(the gateway dedups in-flight only and forgets an id once answered).DELIVEREDis terminal for re-publication; on resubscribe it is not pushed again (removes a latent double-execution). One-waySENTis re-published (only one-wayDELIVEREDis terminal); a null/corrupt persisted request is logged and skipped without aborting the reload.request_idand can't be tracked across the upgrade:QUEUEDare still delivered; stuckSENT/ two-wayDELIVEREDare closed to a terminal state with an explanatory response. The bulk cleanup runs outside the schema transaction, in keyset-paginated self-committing batches (a portedapplyAfterCommitLTS hook) so it never blocks a serving node; rolling-window stragglers are closed by the actor's reload guard.No proto / wire / device / transport change. Transport and rule-engine rolls are unaffected — the reload path runs only when the actor is recreated.
Tests
DeviceActorMessageProcessorTest— reload/seed/guard behavior: persist-on-create,DELIVEREDreload matches a late device response, one-wayDELIVEREDnot re-expired, one-waySENTre-published, legacy null-request_idSENTclosed (not re-published), rpcSeq seeding, resubscribe skipsDELIVERED.JpaRpcDaoTest—request_id/onewayDB round-trip and the reload query excluding one-wayDELIVERED/ terminal rows.V4_3_1_4MigrationIntegrationTest— the batchedapplyAfterCommit()cleanup driven across multiple keyset windows against a real DB: only stuck legacy rows are closed; one-wayDELIVERED, future-expiry, tracked (request_idset) andQUEUEDrows are left untouched.LtsMigrationServiceTest/LtsMigrationIntegrationTest— the two-phaseapplyMigrations(schema, then out-of-transaction backfill, then version record) and dir↔bean pairing.