Skip to content

Commit 5c650d9

Browse files
Minipadaclaude
andcommitted
feat(dc_bridge): extract the Uploader into its own dc_uploader process
Closes #446 The Uploader used to run on a worker thread inside dc_bridge, configured by ROS parameters and linking the AWS SDK into the Bridge's own process. It is now dc_uploader, a standalone executable configured entirely by DC_UPLOADER_* environment variables, with no ROS dependency. The Bridge keeps only the Files subscription and the durable intent-queue write side; dc_uploader reads that queue, uploads, and emits the resulting status Records itself. IntentQueue gains rescan() so a reader-only instance in a separate process can discover intents a different process enqueued — the queue's on-disk format was already crash-atomic and safe for this, but each process's in-memory scheduling state previously only ever loaded what was on disk at construction time. dc_bringup.launch.py starts dc_uploader automatically alongside dc_bridge (same ExecuteProcess pattern as dc_mcap_writer) whenever a `receives: files` Destination is configured, translating that Destination plus files.*/uploader.data_dir into dc_uploader's environment — deployments keep the same params file. See docs/adr/0014-uploader-runs-as-its-own-process.md for the full decision. Verified with a full tools/e2e/scripts/build.sh run (colcon build + test): 656 tests, 0 failures, and a live smoke test of dc_uploader against RustFS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VXhyy2nX5JokxtTnGdwzjy Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
1 parent afcda63 commit 5c650d9

17 files changed

Lines changed: 1106 additions & 336 deletions

dc_bridge/CMakeLists.txt

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ find_package(AWSSDK REQUIRED COMPONENTS s3)
3333

3434
# ROS-independent core: Forwarder/Supervisor/Readiness/TopicConfig/ConfigRenderer/
3535
# vector_binary plus the Uploader logic (ADR-0005). Deliberately aws-free — the S3
36-
# ObjectStore implementation lives in the node target below — so the Uploader is
37-
# unit-tested with an in-memory fake and plain gtest, no cloud dependency.
36+
# ObjectStore implementation lives in the dc_uploader executable below (#446) — so the
37+
# Uploader is unit-tested with an in-memory fake and plain gtest, no cloud dependency.
3838
add_library(dc_bridge_core
3939
src/forwarder.cpp
4040
src/readiness.cpp
@@ -52,7 +52,8 @@ add_library(dc_bridge_core
5252
src/uploader/intent_queue.cpp
5353
src/uploader/thumbnail.cpp
5454
src/uploader/uploader.cpp
55-
src/uploader/retention.cpp)
55+
src/uploader/retention.cpp
56+
src/uploader/process_config.cpp)
5657
target_include_directories(dc_bridge_core PUBLIC
5758
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
5859
$<INSTALL_INTERFACE:include>)
@@ -79,16 +80,26 @@ target_link_libraries(dc_bridge_ros dc_bridge_core)
7980
ament_target_dependencies(dc_bridge_ros
8081
rclcpp rosidl_runtime_cpp rosidl_typesupport_introspection_cpp rcpputils)
8182

82-
# The ROS node, plus the aws-sdk-cpp S3 ObjectStore implementation (the only Uploader
83-
# piece that links the SDK).
83+
# The ROS node. No longer links the AWS SDK (#446): uploading moved to dc_uploader below,
84+
# so the ROS container never needs to hold object-storage credentials (epic #440's user
85+
# story 5) and the Bridge's own link line stays aws-free.
8486
add_executable(dc_bridge
8587
src/main.cpp
86-
src/bridge_node.cpp
87-
src/uploader/s3_object_store.cpp)
88-
target_link_libraries(dc_bridge dc_bridge_core dc_bridge_ros ${AWSSDK_LINK_LIBRARIES})
88+
src/bridge_node.cpp)
89+
target_link_libraries(dc_bridge dc_bridge_core dc_bridge_ros)
8990
ament_target_dependencies(dc_bridge rclcpp dc_interfaces std_msgs std_srvs)
9091

91-
install(TARGETS dc_bridge DESTINATION lib/${PROJECT_NAME})
92+
# The Uploader's own entrypoint (#446): reads the durable intent queue the Bridge above
93+
# writes, uploads Files to object storage, and emits each File's metadata Record itself —
94+
# a standalone process, configured entirely by DC_UPLOADER_* environment variables
95+
# (dc_bridge/uploader/process_config.hpp), with no ROS dependency. Links the aws-sdk-cpp
96+
# S3 ObjectStore implementation, the one Uploader piece that needs it.
97+
add_executable(dc_uploader
98+
src/uploader_main.cpp
99+
src/uploader/s3_object_store.cpp)
100+
target_link_libraries(dc_uploader dc_bridge_core ${AWSSDK_LINK_LIBRARIES})
101+
102+
install(TARGETS dc_bridge dc_uploader DESTINATION lib/${PROJECT_NAME})
92103

93104
if(BUILD_TESTING)
94105
find_package(ament_cmake_gtest REQUIRED)
@@ -97,7 +108,7 @@ if(BUILD_TESTING)
97108
# dc_bridge itself needs no part of it — the Bridge is deliberately unaware of incident capture.
98109
find_package(dc_common REQUIRED)
99110
# All gtests link only the aws-free core (uploader_test uses an in-memory ObjectStore).
100-
foreach(t forwarder supervisor render misc uploader intent_queue retention thumbnail raw_config)
111+
foreach(t forwarder supervisor render misc uploader intent_queue retention thumbnail raw_config process_config)
101112
ament_add_gtest(${t}_test test/${t}_test.cpp)
102113
target_link_libraries(${t}_test dc_bridge_core)
103114
ament_target_dependencies(${t}_test dc_common)

dc_bridge/README.md

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,24 @@ from an in-memory unacked window, so a Record is only forgotten once the Shipper
1010
actually durably buffered it — see "Delivery guarantees" below. It renders Vector's
1111
configuration from ROS parameters for the blessed Destination set (`postgres`, `s3`,
1212
`file`, `console` — ADR-0003) and passes raw Vector snippets (`custom_config_files`)
13-
through for everything else. It also hosts the
14-
**Uploader** (ADR-0005): `receives: files` Destinations are served by the Bridge itself —
15-
Records referencing Files get their Files uploaded to S3-compatible object storage
16-
(multipart + resumable), verified, and reported as status/metadata Records under the
17-
`dc.files` Tag. Pending uploads are durable (#265): each is written to a disk-backed
18-
intent queue before being handed to the Uploader, so a Bridge restart replays whatever
19-
never got acked instead of forgetting it. That queue and the Uploader's multipart-resume
20-
state live under `uploader.data_dir` (#441), a directory separate from the Shipper's own
21-
`shipper.data_dir` buffer — they default to the same path, so a deployment that only ever
22-
set `shipper.data_dir` keeps working unchanged, but each can be mounted as its own volume.
13+
through for everything else.
14+
15+
This package also builds **`dc_uploader`** (ADR-0005, docs/adr/0014-uploader-runs-as-its-
16+
own-process.md): a separate, ROS-free executable that serves `receives: files`
17+
Destinations — Records referencing Files get their Files uploaded to S3-compatible object
18+
storage (multipart + resumable), verified, and reported as status/metadata Records under
19+
the `dc.files` Tag. The Bridge's own job for a files Destination is only to subscribe and
20+
durably enqueue (#265): each Record referencing a File is written to a disk-backed intent
21+
queue before the callback returns, so a Bridge *or* `dc_uploader` restart replays whatever
22+
never got acked instead of forgetting it. `dc_uploader` is configured entirely by
23+
`DC_UPLOADER_*` environment variables, not ROS parameters — see `process_config.hpp`. The
24+
queue and `dc_uploader`'s multipart-resume state live under `uploader.data_dir` (#441), a
25+
directory separate from the Shipper's own `shipper.data_dir` buffer — they default to the
26+
same path, so a deployment that only ever set `shipper.data_dir` keeps working unchanged,
27+
but each can be mounted as its own volume. `dc_bringup.launch.py` starts `dc_uploader`
28+
alongside `dc_bridge` automatically whenever a `receives: files` Destination is
29+
configured, translating that Destination plus `files.*`/`uploader.data_dir` into
30+
`dc_uploader`'s environment — no separate manual step.
2331

2432
`dc_bridge` is an ordinary `ament_cmake` C++ package. It builds with the same `rosdep
2533
install` + `colcon build` as every other `dc_*` package. See
@@ -46,26 +54,31 @@ install` + `colcon build` as every other `dc_*` package. See
4654
- `atomic_write` — writes a file crash/partial-write-safe (write to `<path>.tmp`, then
4755
`rename()`, #444), used for the rendered Shipper config so a reader polling the path
4856
never observes a partial write.
49-
- `uploader/` — the Uploader (ADR-0005): `group` (parses the Files a Record
57+
- `uploader/` — the Uploader's logic (ADR-0005): `group` (parses the Files a Record
5058
references), `content_type` (magic-byte sniffing), `status` (the Humble-compatible
5159
status-row shapes), `multipart` (resumable multipart with a per-part JSON checkpoint
5260
sidecar), `intent_queue` (#265 — the disk-backed durable queue of pending uploads:
5361
crash-atomic tmp+rename enqueue, ack-by-unlink, oldest-first sweep with per-entry
54-
exponential backoff, replayed on startup), and `uploader` (the verify-then-delete
55-
orchestration). All built on an abstract `ObjectStore` interface, so the logic is
56-
tested against an in-memory fake with no cloud dependency.
62+
exponential backoff, replayed on startup; `rescan()`, #446, is how a second process's
63+
own instance over the same directory learns about an intent it didn't enqueue
64+
itself), `uploader` (the verify-then-delete orchestration), and `process_config`
65+
(#446 — parses `dc_uploader`'s `DC_UPLOADER_*` environment-variable configuration).
66+
All built on an abstract `ObjectStore` interface, so the logic is tested against an
67+
in-memory fake with no cloud dependency.
5768
- **`src/raw_subscriptions.cpp`** (`dc_bridge_ros`) — raw / generic-subscription mode's
5869
rclcpp half (#227): topic discovery, `create_generic_subscription`, and the runtime
5970
introspection walk that turns a message of a type the Bridge was never compiled against
6071
into JSON. Its own library rather than a file in the executable, so the conversion is
6172
gtested against real message types without the node, Vector or the AWS SDK.
6273
- **`src/uploader/s3_object_store.cpp`** — the aws-sdk-cpp implementation of
63-
`ObjectStore` (the only Uploader piece that links the SDK, via `aws_sdk_vendor`).
64-
Verified against RustFS (PutObject + multipart) before adoption.
74+
`ObjectStore`, linked only into `dc_uploader` (the only Uploader piece that links the
75+
SDK, via `aws_sdk_vendor`) — `dc_bridge` itself no longer does. Verified against RustFS
76+
(PutObject + multipart) before adoption.
6577
- **`src/bridge_node.cpp` / `src/main.cpp`** — the `rclcpp` node: declares the
66-
`shipper`/`uploader`/`destinations`/`files` parameters (ADR-0003 config contract + ADR-0005),
67-
renders and atomically writes the config (write then rename, #444), subscribes to every
68-
Destination's `inputs` topics, forwards Records, runs the Uploader worker thread, and
78+
`shipper`/`uploader`/`destinations`/`files.metadata_destination` parameters (ADR-0003
79+
config contract + ADR-0005), renders and atomically writes the config (write then
80+
rename, #444), subscribes to every Destination's `inputs` topics, forwards Records,
81+
writes upload intents to the durable queue for `dc_uploader` to read (#446), and
6982
exposes a `~/ready` (`std_srvs/Trigger`) service. In the default **managed** mode
7083
(`shipper.managed: true`) it also locates the vendored Vector binary, `vector
7184
validate`s the merged config, and spawns/supervises Vector. In **unmanaged** mode
@@ -77,8 +90,14 @@ install` + `colcon build` as every other `dc_*` package. See
7790
probe against the Shipper's ingest port, independent of who spawned it. `rclcpp` handles
7891
SIGINT/SIGTERM and `on_shutdown` stops Vector in managed mode (a no-op in unmanaged
7992
mode, nothing to stop), so it's never orphaned.
93+
- **`src/uploader_main.cpp`**`dc_uploader`'s entry point (#446, no `rclcpp`): loads
94+
`DC_UPLOADER_*` configuration, builds the S3 `ObjectStore`, and runs the same
95+
upload/retention poll loop the Bridge's worker thread used to run, now against its own
96+
`IntentQueue` instance (kept in sync with the Bridge's via `rescan()`). SIGINT/SIGTERM
97+
set an atomic flag the loop checks every poll.
8098
- **`test/`** — gtest suites (`forwarder`, `supervisor`, `render`, `misc`, `uploader`,
81-
`intent_queue`), all linking only the aws-free core.
99+
`intent_queue`, `retention`, `thumbnail`, `raw_config`, `process_config`), all linking
100+
only the aws-free core.
82101

83102
## Config renderer (ADR-0003)
84103

dc_bridge/include/dc_bridge/bridge_node.hpp

Lines changed: 9 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,13 @@
1919
// Records-Destination's `inputs` topics and forwards each Record to Vector over the
2020
// shipper ingest protocol.
2121
//
22-
// `receives: files` Destinations (ADR-0005, the Uploader) are handled in Phase 2.
22+
// `receives: files` Destinations (ADR-0005): the Bridge subscribes and writes upload
23+
// intents to the durable queue; a separate dc_uploader process (#446) reads, uploads, and
24+
// emits the resulting status Records — see docs/adr/0014-uploader-runs-as-its-own-process.md.
2325
#ifndef DC_BRIDGE__BRIDGE_NODE_HPP_
2426
#define DC_BRIDGE__BRIDGE_NODE_HPP_
2527

2628
#include <atomic>
27-
#include <condition_variable>
2829
#include <memory>
2930
#include <mutex>
3031
#include <nlohmann/json.hpp>
@@ -40,10 +41,8 @@
4041
#include "dc_bridge/readiness.hpp"
4142
#include "dc_bridge/render.hpp"
4243
#include "dc_bridge/supervisor.hpp"
44+
#include "dc_bridge/uploader/file_status_tag.hpp"
4345
#include "dc_bridge/uploader/intent_queue.hpp"
44-
#include "dc_bridge/uploader/object_store.hpp"
45-
#include "dc_bridge/uploader/retention.hpp"
46-
#include "dc_bridge/uploader/uploader.hpp"
4746
#include "dc_interfaces/msg/string_stamped.hpp"
4847

4948
namespace dc_bridge
@@ -62,11 +61,6 @@ class BridgeNode : public rclcpp::Node
6261

6362
private:
6463
void run_prober(std::string forward_host, std::uint16_t forward_port);
65-
// The Uploader worker (ADR-0005/#265): sweeps the durable intent queue oldest-first
66-
// (per-entry backoff on failure — one permanently-failing intent can't starve the
67-
// backlog), processes each Record, acks (unlinks) its intent on success, and emits
68-
// status Records through its own Forwarder connection under FILE_STATUS_TAG.
69-
void run_uploader_worker(std::string forward_host, std::uint16_t forward_port);
7064

7165
std::shared_ptr<Supervisor> supervisor_;
7266
std::mutex supervisor_mutex_;
@@ -82,28 +76,12 @@ class BridgeNode : public rclcpp::Node
8276
std::atomic<bool> prober_stop_{ false };
8377
std::atomic<bool> stopped_{ false };
8478

85-
// Uploader (ADR-0005) — created only when a `receives: files` destination is
86-
// configured. Records on files-destination topics are enqueued into the durable
87-
// intent_queue_ (#265); the worker thread replays/sweeps it, uploads Files, and emits
88-
// status Records.
89-
std::unique_ptr<uploader::Uploader> uploader_;
79+
// The durable upload intent queue (ADR-0005/#265) — created only when a `receives:
80+
// files` destination is configured. Records on files-destination topics are enqueued
81+
// here so they survive a Bridge crash/restart. The Bridge only writes; a separate
82+
// dc_uploader process (#446) owns reading, replaying, uploading, and emitting status
83+
// Records — see docs/adr/0014-uploader-runs-as-its-own-process.md.
9084
std::unique_ptr<uploader::IntentQueue> intent_queue_;
91-
std::thread uploader_thread_;
92-
std::mutex uploader_wake_mutex_;
93-
std::condition_variable uploader_wake_cv_;
94-
bool upload_stop_{ false };
95-
96-
// Files retention (#267) — a copy of the storages the Uploader was built with (kept
97-
// alongside it since the Uploader moves its own copy) so the worker thread can build
98-
// shed audit rows without needing an Uploader accessor; a no-op sweep when disabled
99-
// (the default) either way.
100-
uploader::RetentionConfig retention_config_;
101-
std::vector<Storage> files_storages_;
102-
103-
// Thumbnails (#256) — the configured generator binary, kept for the uploader worker's
104-
// "couldn't generate any previews" warning, which is the only place a failed preview
105-
// is observable (it never fails the upload itself).
106-
std::string thumbnail_binary_;
10785

10886
// Raw / generic-subscription mode (#227) — created only when `raw.enabled` is true.
10987
// Its Records go out through the same Forwarder as the Measurement Records above,
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// SPDX-FileCopyrightText: 2022-2026 David Bensoussan
2+
// SPDX-License-Identifier: MPL-2.0
3+
4+
// Split out of uploader.hpp (#446) so a process that only needs to know the Uploader's
5+
// Tag — the Bridge, wiring Vector's routing for it — doesn't have to pull in the full
6+
// Uploader/ObjectStore/Retention class surface just for one constant.
7+
#ifndef DC_BRIDGE__UPLOADER__FILE_STATUS_TAG_HPP_
8+
#define DC_BRIDGE__UPLOADER__FILE_STATUS_TAG_HPP_
9+
10+
namespace dc_bridge::uploader
11+
{
12+
13+
/// The Tag the Uploader emits status/metadata Records under.
14+
inline constexpr const char* FILE_STATUS_TAG = "dc.files";
15+
16+
} // namespace dc_bridge::uploader
17+
18+
#endif // DC_BRIDGE__UPLOADER__FILE_STATUS_TAG_HPP_

dc_bridge/include/dc_bridge/uploader/intent_queue.hpp

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,11 @@
1212
// processing actually succeeds — no cap, no drop-oldest, no dead-letter. An upload
1313
// intent and its Files live and die together.
1414
//
15-
// The in-process wake-up channel (a condition_variable in BridgeNode) is still how the
16-
// worker learns "there's new work" cheaply; this directory is the crash-recovery state,
17-
// not the communication path — on startup every unacked intent left over from a
18-
// previous run is replayed, oldest-first, alongside live traffic.
15+
// This directory is the crash-recovery state, not a notification channel: on startup
16+
// every unacked intent left over from a previous run is replayed, oldest-first, alongside
17+
// live traffic. Since #446 split the Uploader into its own process, there is no
18+
// in-process wake-up signal at all — a Bridge process enqueues and a separate dc_uploader
19+
// process discovers new intents purely by polling rescan() (see that method below).
1920
#ifndef DC_BRIDGE__UPLOADER__INTENT_QUEUE_HPP_
2021
#define DC_BRIDGE__UPLOADER__INTENT_QUEUE_HPP_
2122

@@ -93,6 +94,17 @@ class IntentQueue
9394
std::size_t size() const;
9495
bool empty() const;
9596

97+
/// Picks up any `*.json` file that exists on disk but isn't yet known to this
98+
/// instance — the multi-process split (#446): a Bridge process enqueues intents while a
99+
/// separate Uploader process holds the read side (next_ready()/ack()/record_failure()),
100+
/// each with its own IntentQueue instance over the same directory. enqueue() only
101+
/// updates its own caller's in-memory view, so the Uploader's instance never otherwise
102+
/// learns about an intent a different process wrote; this is what closes that gap.
103+
/// Already-known entries are left untouched (so in-flight backoff state survives);
104+
/// returns the number of newly discovered intents. Safe to call from the same loop that
105+
/// already polls for next_ready(), same cost as the constructor's initial scan.
106+
std::size_t rescan();
107+
96108
private:
97109
struct Entry
98110
{
@@ -105,6 +117,12 @@ class IntentQueue
105117
};
106118

107119
std::string path_for(const std::string& id) const;
120+
/// Every "*.json" filename currently on disk under `dir`, sorted oldest-first (ids sort
121+
/// lexicographically in enqueue order — see intent_queue.cpp's make_id()).
122+
static std::vector<std::string> list_json_names(const std::string& dir);
123+
/// Parses one intent file's body into an Entry, or nullopt if it can't be read/parsed (a
124+
/// "final" .json file should always be complete, since rename is atomic).
125+
static std::optional<Entry> load_entry(const std::string& dir, const std::string& name);
108126

109127
mutable std::mutex mutex_;
110128
std::string dir_;

0 commit comments

Comments
 (0)