feat: configurable foreground waits with background completion

[maxirmx]

No description provided.

[Copilot]

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds configurable foreground waits with cache fallback and durable background report delivery.

Changes:

• Adds asynchronous authorization and cancellable network sessions.
• Introduces transactional report storage, retries, ordering, and migration.
• Adds extensive tests and documentation.

File summaries

File	Description
tests/report_delivery_test.cpp	Updated as part of this pull request.
tests/foreground_backend_test.cpp	Updated as part of this pull request.
tests/controller_test.cpp	Updated as part of this pull request.
tests/cares_resolver_test.cpp	Updated as part of this pull request.
tests/backlog_worker_test.cpp	Updated as part of this pull request.
tests/backend_test.cpp	Updated as part of this pull request.
src/user_cache.cpp	Updated as part of this pull request.
src/state_machine.cpp	Updated as part of this pull request.
src/message_storage.cpp	Updated as part of this pull request.
src/main.cpp	Updated as part of this pull request.
src/controller.cpp	Updated as part of this pull request.
src/cares_resolver.cpp	Updated as part of this pull request.
src/cache_manager.cpp	Updated as part of this pull request.
src/backlog_worker.cpp	Updated as part of this pull request.
src/backend.cpp	Updated as part of this pull request.
src/backend_base.cpp	Updated as part of this pull request.
README.md	Updated as part of this pull request.
include/user_cache.h	Updated as part of this pull request.
include/types.h	Updated as part of this pull request.
include/timing_config.h	Updated as part of this pull request.
include/message_storage.h	Updated as part of this pull request.
include/controller.h	Updated as part of this pull request.
include/cares_resolver.h	Updated as part of this pull request.
include/cache_manager.h	Updated as part of this pull request.
include/backlog_worker.h	Updated as part of this pull request.
include/backend.h	Updated as part of this pull request.
include/authorization_snapshot.h	Updated as part of this pull request.
docs/state_machine.md	Updated as part of this pull request.
docs/foreground_backend_waits.md	Updated as part of this pull request.
CMakeLists.txt	Updated as part of this pull request.
.gitignore	Updated as part of this pull request.

Review details

Suppressed comments (6)

include/controller.h:258

• threadExited_ starts as true, but run() changes it to false only after the caller launches the thread. If shutdown() races with a newly created controller before run() enters, this loop is skipped and shutdownPeripherals()/worker teardown can race with the still-starting event-loop thread; the documented wait is not guaranteed. Track whether the run thread has started (or otherwise synchronize startup) before using this flag to decide that shutdown is complete.

    std::atomic<bool> threadExited_{true};

src/backend_base.cpp:515

• The canonical branch now sends directly to the API after checking only IsAuthorized(), unlike Refuel() which enforces UserRole::Customer and allowance limits. Controller::enterVolume() only applies the role/allowance check to customers, so a non-customer session can reach submitReport() and this path can transmit a refuel that the old backend validation would reject. Preserve the method-specific authorization checks (at minimum the role check) before sending canonical payloads.

    if (!session_.IsAuthorized()) { lastError_ = StdControllerError; return false; }
    const auto body = nlohmann::json::parse(payload, nullptr, false);
    if (body.is_discarded()) { lastError_ = StdControllerError; return false; }
    const auto response = HttpRequestWrapper(intake ? "/api/pump/fuel-intake" : "/api/pump/refuel", "POST", body, true);

src/backlog_worker.cpp:27

• RecoverInFlight() can fail (for example, when the SQLite database is temporarily read-only or locked), but the worker starts regardless. Those rows remain in_flight=1, and ClaimNextBacklog() explicitly excludes them, so the affected reports are silently stranded for the lifetime of this worker. Treat recovery failure as a startup failure or retry it before accepting normal delivery.

    storage_->RecoverInFlight();

src/controller.cpp:687

• The interface's default CreateIndependentSession() returns an empty pointer, but this branch treats that as an authorization failure. Any IBackend implementation that does not add the new override (the default was kept precisely so implementations still compile) will now reject every card/PIN attempt, while CacheManager and BacklogWorker fall back to their existing backend when no independent session is available. Preserve the old behavior with a documented fallback or make the new requirement explicit instead of silently posting AuthorizationFailed.

    auto session = backendPrototype_->CreateIndependentSession();
    if (!session) { postEvent(Event::AuthorizationFailed); return; }

src/main.cpp:388

• This cleanup is unreachable on the MAX_RETRIES path: that branch calls CleanupCaresLibrary() and Logger::shutdown() and then enters its permanent-failure loop without returning here. Any queued async deauthorization can therefore continue using DNS or logging after those subsystems are torn down. Stop the shared deauthorization executor before the failure-path cleanup as well as on normal exit.

    BackendBase::ShutdownAsyncRequests();

src/message_storage.cpp:435

• ClaimNextBacklog() already increments attempt when it claims a row. Incrementing it here as well makes one delivery resumed after restart jump two attempt IDs, so the stored attempt no longer represents delivery attempts and diagnostics/limits can be wrong. Recovery should clear in_flight/retry_after and let the next claim perform the single increment.

    return Execute("UPDATE backlog SET in_flight=0,attempt=attempt+1,retry_after=0 WHERE in_flight=1");

• Files reviewed: 30/31 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical compatibility and authorization-safety findings remain, along with session cancellation and cleanup gaps.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

include/backend.h:56

• The no-op default lets an adapter return a non-null independent session, pass the controller's only capability check, and still never cancel a blocked request. Slow-authorization cancellation and shutdown can then wait for the transport deadline instead of stopping promptly. Make cancellation mandatory for controller sessions (for example, a pure virtual method or an explicit capability that is validated at construction).

    virtual void CancelPendingRequests() {}

include/backend.h:60

• The new canonicalTankId argument is discarded by the default implementation, so canonical reports are routed through the legacy payload methods. BackendBase::RefuelPayload() and IntakePayload() apply visual-number mapping, which can remap an already-canonical backend tank ID for adapters that inherit this default. Make canonical delivery an explicit override/contract (or fail closed) instead of silently ignoring the flag.

    virtual bool SendReportPayload(const std::string& payload, bool intake, bool canonicalTankId) {
        (void)canonicalTankId;
        return intake ? IntakePayload(payload) : RefuelPayload(payload);
    }

src/controller.cpp:787

• A successful foreground report resets backend_ after handing the session to the delivery worker, while backendPrototype_ remains available for the next operation. This public synchronous compatibility helper checks only backend_, so calling it after one completed report always reports “Backend unavailable” instead of creating another session. Recreate a session from backendPrototype_ when backend_ is empty before applying this guard.

    if (!backend_) {
        showError("Backend unavailable");

src/controller.cpp:1036

• When EnqueueReport fails, the supplied online session remains authorized because BacklogWorker::Submit does not deauthorize a failed submission. Refueling happens to clean this up afterward, but completeIntakeOperation() has no equivalent cleanup; the queued Error event therefore leaves a failed intake session authorized until a later timeout or restart. Deauthorize and clear the supplied session on this failure path.

    if (!id) {
        showError("Ошибка записи");
        postEvent(Event::Error);
        return false;

src/controller.cpp:661

• This destructor only deauthorizes an unadopted attempt when success is true. The authorization worker sets success back to false in its catch path after any exception, including one while copying tank data after the backend has already authorized, so the server session can be leaked. Cleanup should instead check and deauthorize any authorized unadopted backend once the attempt is done.

    if (done.load() && success && !adopted && backend) {
        try { backend->Deauthorize(); } catch (...) {}
    }

• Files reviewed: 31/32 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in report delivery, controller/backend lifecycle, storage, and cache handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

src/backlog_worker.cpp:86

• Stop() can win between the loop's running_ check and this claim. It then sees no active backend and starts joining, while ProcessOnce() can claim the row and create a fresh backend afterward, leaving shutdown waiting on a request that was never cancelled. Recheck running_ while holding this mutex immediately before claiming (or otherwise serialize the claim with Stop()).

        message = storage_->ClaimNextBacklog();

src/controller.cpp:710

• CaptureAuthorizationState can return reportStorageAvailable == false when the write probe or a database read fails, but this branch only discards saved and continues to create an online authorization attempt. That allows a card to reach fueling even though submitReport cannot persist the transaction, violating the fail-closed storage contract. Reject the authorization before starting the backend session whenever local.reportStorageAvailable is false.

    const auto saved = local.reportStorageAvailable ? local.saved : std::nullopt;
    if (messageStorage_ && local.pendingReports) {

src/controller.cpp:685

• This persists an online authorization before StateMachine::onAuthorizationSuccess() rejects snapshots with no available tanks (src/state_machine.cpp:752-757). A successful response with an empty fuelTanks array can therefore overwrite a valid protected snapshot and the general cache; after a later network failure, fallback uses unusable data. Only persist a non-cache authorization after validating that it has usable tanks.

    if (!fromCache) {
        // Keep protection until a synchronization fetch begun after resolution
        // commits. Otherwise an older in-flight sync could undo this fresh reply.
        if (messageStorage_) messageStorage_->RefreshResolvedSnapshot(snapshot);

src/controller.cpp:120

• initializePeripherals() already calls shutdownPeripherals() when a critical peripheral fails (src/controller.cpp:1385-1390), but this unconditionally marks every peripheral for another shutdown. Consequently, a failed initialization followed by the normal shutdown() path tears down the same objects twice; the peripheral interface does not guarantee that shutdown() is idempotent. Track whether initialization actually left live peripherals (and update that flag on reinitialization) instead of setting it before the call.

    peripheralsNeedShutdown_ = true;
    bool ok = initializePeripherals();

src/controller.cpp:685

• RefreshResolvedSnapshot only updates rows that are already protected=1, so this call is a no-op for a card with no pending report and an unprotected (or absent) state row. If a cache population captured the old general cache before this online authorization and commits afterward, its stale data can overwrite UpdateCacheEntry; the next network failure then falls back to that stale general snapshot because no protected row exists. Preserve/protect the fresh snapshot without overwriting pending state, and cover this ordering with a regression test.

    if (!fromCache) {
        // Keep protection until a synchronization fetch begun after resolution
        // commits. Otherwise an older in-flight sync could undo this fresh reply.
        if (messageStorage_) messageStorage_->RefreshResolvedSnapshot(snapshot);

src/controller.cpp:95

• The cache manager is built with CreateDefaultBackendShared(...) rather than an independent session from the backend injected into this Controller. As a result, a custom/mock adapter authorizes foreground users through one backend but synchronization silently uses the production REST adapter, potentially populating the cache with unrelated data and bypassing the adapter's session contract. Construct the synchronization session from backendPrototype_ (or inject an explicit factory) so cache and foreground operations use the same backend implementation.

    try {
        messageStorage_ = std::make_shared<MessageStorage>(persistencePaths.messageStorageDbPath);
        reportWorker_ = std::make_unique<BacklogWorker>(messageStorage_, backendPrototype_, timing::kBacklogWorkerInterval);
        if (cacheManager_) cacheManager_->SetReportStorage(messageStorage_);

• Files reviewed: 32/33 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in report delivery, controller/backend lifecycle, storage, and cache handling....

Addressed the remaining findings from this review in ce50b95.

[Copilot]

🟡 Changes recommended

Three unresolved findings remain, including one critical delivery-handling issue and two moderate lifecycle/cleanup issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/backlog_worker.cpp:44

• This cleanup is unreachable when Stop() is called while running_ is already false. Submit() can retain a supplied online session in sessions_ before Start() (for example, controller APIs can enqueue before initialization); submitReport() then resets backend_, so the session is destroyed without Deauthorize(), leaving the server-side authorization open. Drain/deauthorize retained sessions even for an unstarted worker, while still joining any joinable thread.

    std::lock_guard<std::mutex> lock(mutex_);
    for (auto& entry : sessions_) entry.second->Deauthorize();
    sessions_.clear();

src/controller.cpp:184

• The shutdown deadline only waits for loopActive_; these unconditional worker shutdowns happen afterward and join without a deadline. If an authorization/report backend call cannot be interrupted (for example during a transport or SQLite wait), shutdown() can block until that operation's much longer timeout instead of returning false at kShutdownDeadline, so the caller cannot preserve live state and retry as documented. Bound these joins or include the workers in the deadline/cancellation state.

    authorizationExecutor_.Shutdown();
    if (reportWorker_) reportWorker_->Stop();

• Files reviewed: 32/33 changed files
• Comments generated: 1
• Review effort level: Lite