src/services/config.rs (1)

167-179: Consider requiring username and password for Firebird.

Firebird is grouped with optional credentials (like MongoDB/Redis), but the backup and restore implementations pass cfg.username and cfg.password directly to gbak. If these are empty, the commands will likely fail at runtime. Other networked databases like PostgreSQL, MySQL, and MariaDB require these fields.

♻️ Proposed fix to require credentials for Firebird

             let username = match db.db_type {
-                DbType::Postgresql | DbType::Mysql | DbType::Mariadb => {
+                DbType::Postgresql | DbType::Mysql | DbType::Mariadb | DbType::Firebird => {
                     required(&db.username, &db.name, "username")?
                 }
                 _ => optional(&db.username),
             };

             let password = match db.db_type {
-                DbType::Postgresql | DbType::Mysql | DbType::Mariadb => {
+                DbType::Postgresql | DbType::Mysql | DbType::Mariadb | DbType::Firebird => {
                     required(&db.password, &db.name, "password")?
                 }
                 _ => optional(&db.password),
             };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/config.rs` around lines 167 - 179, The Firebird DB type is
currently treated as optional for credentials but backup/restore pass
cfg.username and cfg.password to gbak, so make Firebird require username and
password like Postgresql/Mysql/Mariadb: update the match arms that set username
and password (referencing db.db_type, DbType::Postgresql | DbType::Mysql |
DbType::Mariadb and the required(...) and optional(...) calls) to include
DbType::Firebird so required(&db.username, &db.name, "username")? and
required(&db.password, &db.name, "password")? are used for Firebird as well.

Makefile (1)

52-57: Consider using environment variables for consistency with other seed targets.

Other seed targets use environment variables ($$MYSQL_USER, $$PG_PASSWORD, etc.) from the .env file, but Firebird credentials are hardcoded. This inconsistency makes future maintenance harder.

♻️ Proposed refactor using environment variables

 seed-firebird:
 	`@echo` "Seeding Firebird..."
-	docker exec -i db-firebird isql -user alice -password fake_password /var/lib/firebird/data/mirror.fdb < ./scripts/firebird/seed.sql
+	docker exec -i db-firebird isql -user $$FIREBIRD_USER -password $$FIREBIRD_PASSWORD /var/lib/firebird/data/$$FIREBIRD_DATABASE < ./scripts/firebird/seed.sql
 	`@echo` "Verifying Firebird tables..."
-	docker exec -i db-firebird isql -user alice -password fake_password /var/lib/firebird/data/mirror.fdb \
+	docker exec -i db-firebird isql -user $$FIREBIRD_USER -password $$FIREBIRD_PASSWORD /var/lib/firebird/data/$$FIREBIRD_DATABASE \
 		-sql "SHOW TABLES;"

Then add corresponding variables to .env:

FIREBIRD_USER=alice
FIREBIRD_PASSWORD=fake_password
FIREBIRD_DATABASE=mirror.fdb

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 52 - 57, Replace the hardcoded Firebird credentials in
the seed-firebird target with environment variables and document them in .env:
change occurrences of "alice" to $$FIREBIRD_USER, "fake_password" to
$$FIREBIRD_PASSWORD, and "mirror.fdb" to $$FIREBIRD_DATABASE in the docker exec
lines inside the seed-firebird recipe so the target uses shell environment vars
(matches other seed targets); also add FIREBIRD_USER, FIREBIRD_PASSWORD, and
FIREBIRD_DATABASE entries to .env.

src/domain/firebird/database.rs (1)

30-40: Consider using RAII guard pattern for lock management.

If FileLock::release() fails after a successful backup, the backup result is discarded. Also, if the backup::run() call panics (unlikely but possible), the lock won't be released. A guard-based approach would be more robust.

That said, this follows the existing pattern in the codebase, so it's acceptable to keep as-is and address holistically later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/domain/firebird/database.rs` around lines 30 - 40, The current backup()
implementation acquires a file lock with FileLock::acquire(...) and then calls
backup::run(...), but it manually calls FileLock::release(...) afterward which
can fail or be skipped if backup::run panics; replace this manual release with
an RAII guard that acquires the lock in FileLock::acquire and returns a guard
type (e.g., FileLockGuard) that releases the lock in Drop so backup() simply
creates the guard, calls backup::run(self.cfg.clone(), dir.to_path_buf(),
self.file_extension()).await, and returns the result; update FileLock::acquire
to return the guard (or add a new method returning a guard) and remove the
explicit FileLock::release call from backup().

src/domain/firebird/ping.rs (2)

47-49: Stdout check may produce false positives.

stdout.contains("1") could match unintended content like "10", "21", or other text containing "1". Consider a more precise check.

Proposed fix

-    if stdout.contains("1") {
+    // isql-fb outputs the result with whitespace; trim and check for standalone "1"
+    if stdout.lines().any(|line| line.trim() == "1") {
         return Ok(true);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/domain/firebird/ping.rs` around lines 47 - 49, The check using
stdout.contains("1") can yield false positives (matching "10", "21", etc.);
change the condition to ensure an exact match — e.g., replace
stdout.contains("1") with a strict comparison like stdout.trim() == "1" or parse
stdout to an integer and compare to 1 (using the stdout variable and the
surrounding conditional in ping.rs) so only an exact "1" result returns
Ok(true).

---

29-32: Add logging for timeout case to aid debugging.

When the ping times out, the function silently returns Ok(false) without any logging. This makes it difficult to distinguish between a timeout and other failure modes during troubleshooting.

Proposed fix

     let output = match timeout(Duration::from_secs(5), child.wait_with_output()).await {
         Ok(res) => res?,
-        Err(_) => return Ok(false),
+        Err(_) => {
+            error!("Firebird ping timed out for {}", cfg.name);
+            return Ok(false);
+        }
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/domain/firebird/ping.rs` around lines 29 - 32, The timeout branch in the
timeout(Duration::from_secs(5), child.wait_with_output()).await match currently
returns Ok(false) silently; update that Err(_) arm to log a warning/error before
returning, e.g. emit a clear message like "firebird ping timed out" and include
contextual details (the child/command or other available identifiers) using the
crate's logging facility (e.g. tracing::warn! or log::warn!) so timeouts are
distinguishable from other failures in the function containing timeout(...) and
child.wait_with_output().

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/tests/domain/firebird.rs (1)

56-57: Don’t mask ping errors with unwrap_or(false) in tests.

Use expect(...) so failures preserve root-cause context.

Suggested fix

-    let reachable = db.ping().await.unwrap_or(false);
+    let reachable = db.ping().await.expect("firebird ping failed");
@@
-    let reachable = db.ping().await.unwrap_or(false);
+    let reachable = db.ping().await.expect("firebird ping failed before restore");

Also applies to: 93-95

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tests/domain/firebird.rs` around lines 56 - 57, Replace the use of
unwrap_or(false) which masks errors when calling db.ping().await in the tests:
change the lines using let reachable = db.ping().await.unwrap_or(false); (and
the other occurrences at the same file) to use expect(...) on the await result
so a failing ping preserves the original error context (i.e., call
db.ping().await.expect("ping failed in firebird test") and keep the reachable
variable logic the same).

docker-compose.databases.yml (2)

18-18: Consider pinning the MariaDB image version.

Using latest tag can lead to unexpected behavior when the image updates. Consider pinning to a specific version (e.g., mariadb:11) for reproducibility, similar to how mysql:9.5 is specified.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker-compose.databases.yml` at line 18, Replace the floating tag
"mariadb:latest" used in the service image declaration (currently "image:
mariadb:latest") with a pinned, explicit version (for example "mariadb:11" or
another tested patch version) to ensure reproducible builds; mirror the approach
used for "mysql:9.5" so the compose file uses fixed database image tags rather
than "latest".

---

142-158: The firebirdsql/firebird image includes a built-in healthcheck; adding an explicit one is optional.

The official firebirdsql/firebird image already includes a built-in healthcheck via docker-healthcheck.sh that checks port 3050. If you want to add a custom healthcheck for consistency with MongoDB services, note that the proposed command has syntax issues (the database path should use / not :). A simpler alternative is to use a port-based check: ["CMD", "nc", "-z", "127.0.0.1", "3050"]. However, since most other database services in this file (PostgreSQL, MariaDB, MySQL, Redis, Valkey) do not have explicit healthchecks, the built-in one is sufficient for a development environment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker-compose.databases.yml` around lines 142 - 158, The db-firebird service
uses the official firebirdsql/firebird image which already provides a built-in
healthcheck, so remove any added/incorrect custom healthcheck or, if you prefer
consistency, replace the broken command with a simple port check (e.g., use nc
against 127.0.0.1:3050) instead of the incorrect database path syntax; locate
the db-firebird service (container_name: db-firebird, image:
firebirdsql/firebird) and either omit a custom healthcheck entirely or implement
the port-based CMD healthcheck.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/domain/firebird/ping.rs (2)

27-29: Ensure stdin is closed before waiting for output.

Taking stdin from the child process is correct, but the stdin handle should be explicitly dropped after writing to signal EOF to isql-fb. Currently, stdin goes out of scope at the end of the if let block, which should work, but making this explicit improves clarity and avoids potential deadlock if the compiler reorders.

♻️ Suggested improvement

     if let Some(mut stdin) = child.stdin.take() {
         stdin.write_all(query).await?;
+        drop(stdin); // Explicitly close stdin to signal EOF
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/domain/firebird/ping.rs` around lines 27 - 29, The code writes to the
child's stdin via child.stdin.take() and stdin.write_all(query).await but
doesn't explicitly close stdin; after awaiting write_all, explicitly drop the
stdin handle (e.g., call drop(stdin)) to ensure EOF is sent to isql-fb before
waiting for output — locate the child.stdin.take() /
stdin.write_all(query).await sequence in ping.rs and add the explicit drop of
stdin immediately after the awaited write.

---

49-51: Fragile success check - stdout.contains("1") may produce false positives.

The check stdout.contains("1") could incorrectly match version strings, row counts, or other output containing "1". The expected output from SELECT 1 FROM RDB$DATABASE; is a single row with value 1, but isql-fb output formatting may include headers or other text.

Consider a more precise pattern match or trimming/parsing the output.

♻️ Suggested improvement

-    if stdout.contains("1") {
+    // isql-fb outputs the result with column alignment; look for standalone "1"
+    if stdout.lines().any(|line| line.trim() == "1") {
         return Ok(true);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/domain/firebird/ping.rs` around lines 49 - 51, The success check using
stdout.contains("1") is too broad; update the check in ping.rs (the code reading
isql-fb output stored in stdout) to parse/trim output and match a precise
pattern such as the last non-empty line equaling "1" or a regex like ^\s*1\s*$
instead of contains, so headers/footer or other "1" occurrences won't produce
false positives; ensure you perform trim/split on stdout and verify the exact
value before returning Ok(true).

src/tests/domain/firebird.rs (2)

104-113: Replace verbose match with idiomatic assertion.

The match block with assert!(true) and assert!(false) is non-idiomatic and obscures test intent. Use unwrap(), expect(), or assert! directly on the result.

♻️ Suggested improvement

-    match db.restore(&backup_file).await {
-        Ok(_) => {
-            info!("Restore succeeded for {}", config.generated_id);
-            assert!(true)
-        }
-        Err(e) => {
-            error!("Restore failed for {}: {:?}", config.generated_id, e);
-            assert!(false)
-        }
-    }
+    db.restore(&backup_file)
+        .await
+        .expect("Restore should succeed");
+    info!("Restore succeeded for {}", config.generated_id);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tests/domain/firebird.rs` around lines 104 - 113, The match on
db.restore(&backup_file).await should be replaced with an idiomatic assertion:
call db.restore(&backup_file).await.expect(...) or .unwrap() so the test fails
with a clear message instead of assert!(true)/assert!(false); include
config.generated_id in the expect message for context (e.g.,
db.restore(&backup_file).await.expect(&format!("Restore failed for {}",
config.generated_id))). This change targets the restore call in the test
(db.restore, backup_file, config.generated_id) and removes the verbose match
block.

---

78-78: Remove or justify the 10-second sleep after backup.

This sleep appears arbitrary. If it's waiting for file system sync or some async completion, consider polling the file existence or using a more deterministic approach.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tests/domain/firebird.rs` at line 78, The arbitrary
tokio::time::sleep(Duration::from_secs(10)).await; should be removed or replaced
with a deterministic wait: instead of sleeping 10s, poll for the expected backup
completion (e.g., loop using tokio::time::sleep with a short interval and check
file existence/metadata or a completion flag) and bound the loop with a timeout
(tokio::time::timeout) to fail deterministically; update the test in
src/tests/domain/firebird.rs to use that polling+timeout approach (replace the
direct call to tokio::time::sleep(Duration::from_secs(10)).await with a check
for the backup file or process completion).

docker-compose.databases.yml (1)

142-158: Firebird service configuration looks good; consider adding a healthcheck.

The Firebird service is well-configured with proper environment variables, volume mount, and network. For consistency with other database services and to enable dependent services to wait for readiness, consider adding a healthcheck.

♻️ Suggested healthcheck

   db-firebird:
     image: firebirdsql/firebird
     container_name: db-firebird
     restart: always
     environment:
       - FIREBIRD_ROOT_PASSWORD=fake_root_password
       - FIREBIRD_USER=alice
       - FIREBIRD_PASSWORD=fake_password
       - FIREBIRD_DATABASE=mirror.fdb
       - FIREBIRD_DATABASE_DEFAULT_CHARSET=UTF8
     volumes:
       - firebird-data:/var/lib/firebird/data
     ports:
       - "3060:3050"
+    healthcheck:
+      test: ["CMD-SHELL", "echo 'SELECT 1 FROM RDB$DATABASE;' | isql-fb -q -user alice -password fake_password localhost:/var/lib/firebird/data/mirror.fdb || exit 1"]
+      interval: 10s
+      timeout: 5s
+      retries: 5
     networks:
       - portabase

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker-compose.databases.yml` around lines 142 - 158, Add a Docker
healthcheck entry to the db-firebird service to allow dependent services to wait
for readiness: add a healthcheck block under the db-firebird service that runs a
short command which verifies the database is accepting connections (for example
using gstat or isql and the existing
FIREBIRD_USER/FIREBIRD_PASSWORD/FIREBIRD_DATABASE env vars), and configure
sensible timing (e.g., interval ~30s, timeout ~10s, retries ~5, start_period
~30s) so Docker reports healthy/unhealthy reliably.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes